Fix more coverity defects #73
Conversation
…es, throwing in constructors etc.
| @@ -164,8 +164,8 @@ private FullHttpResponse generateResponse(Map<String, String> stringHeaders) { | |||
|
|
|||
| final FullHttpResponse response = new DefaultFullHttpResponse(req.protocolVersion(), responseStatus); | |||
| final DefaultHttpHeaders headers = new DefaultHttpHeaders(); | |||
| for (String key : stringHeaders.keySet()) { | |||
| headers.set(key, stringHeaders.get(key)); | |||
| for (Map.Entry<String, String> entry : stringHeaders.entrySet()) { | |||
There was a problem hiding this comment.
This new change is the opposite of what I am fixing in teragrep/cfe_31#52.
Architect's opinion requested on this particular case
| @@ -80,14 +75,15 @@ public NettyHttpServer( | |||
| messageHandler, | |||
| executorGroup, | |||
| nettyConfig.maxContentLength, | |||
| responseStatus, | |||
| HttpResponseStatus.valueOf(responseCode), | |||
| internalEndpointUrlConfig | |||
| ); | |||
|
|
|||
| if (sslHandlerProvider != null) { | |||
| @@ -74,8 +71,8 @@ public void channelRead0(ChannelHandlerContext ctx, FullHttpRequest msg) { | |||
| } | |||
|
|
|||
| @Override | |||
| public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { | |||
| final ByteBuf content = copiedBuffer(cause.getMessage().getBytes()); | |||
| public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { | |||
There was a problem hiding this comment.
Does this method override something at compile-time?
There are no usages in code at the moment
There was a problem hiding this comment.
The whole object is added to a ChannelPipeline in HttpInitializer. The default implementation ignores exceptions (forwards them to the next item in the pipeline). Overriding this sends an appropriate HTTP request back instead.
|
|
||
| private final EventLoopGroup processorGroup; | ||
| private final ThreadPoolExecutor executorGroup; | ||
| private final HttpResponseStatus responseStatus; | ||
| private final InternalEndpointUrlConfig internalEndpointUrlConfig; | ||
|
|
||
| public NettyHttpServer( |
There was a problem hiding this comment.
We had some discussion about using primary and secondary ctors, but we need to get back to that
There was a problem hiding this comment.
So primary and secondary constructors should be used here
Related to #69.
Refactored the code to fix defects reported by Coverity. The list of defects below doesn't tell much if there is no access to Coverity, but the list is there just to give context on what needed fixing. The PR is still merely about refactoring the code to be more secure and clean.
Coverity defects that should be solved by this:
467201 CT: Constructor throws
456623 Dm: Dubious method used
456622 Dm: Dubious method used
456284 Dm: Dubious method used
452921 SS: Unread field should be static (not actually changing to static, but changing it to a local variable)
452920 WMI: Inefficient Map Iterator
452918 Dm: Dubious method used
As you can see, 4 of them are "Dubious method used". These were about using default charsets for example in a FileReader call. I wasn't quite sure with charset to use, but the default for me locally was UTF-8, so I went with that. This is probably going to need input from the architect whether that is correct.
Two defects are not solved, because I think solving them would introduce code that is not in the spirit of Elegant Objects or otherwise weird:
NOTE:
The PR includes ConfigTests for the reason that I had to refactor the Exception throwing out of the constructor. I wanted to make sure I didn't break it in the process.