ResponseStatusExceptionFilter: Fix unwrap of JSON message #1223
Conversation
| @@ -80,9 +80,12 @@ private String getBodyAsMessage(ClientResponseContext responseContext) { | |||
|
|
|||
| if (MediaType.APPLICATION_JSON_TYPE.equals(mediaType)) { | |||
| ObjectMapper mapper = new ObjectMapper(); | |||
| JsonNode node = mapper.readTree(entityStream).get("message"); | |||
| JsonNode node = mapper.readTree(message); | |||
There was a problem hiding this comment.
maybe surround with try? Parsing here is optional and shouldn't fail response exceptions
Codecov Report
@@ Coverage Diff @@
## master #1223 +/- ##
==========================================
- Coverage 57.97% 57.96% -0.02%
==========================================
Files 455 455
Lines 8833 8838 +5
Branches 531 532 +1
==========================================
+ Hits 5121 5123 +2
- Misses 3435 3437 +2
- Partials 277 278 +1
Continue to review full report at Codecov.
|
| } | ||
| } | ||
| } catch (IOException ignored) { | ||
| //ignore parsing errors and return the message as is |
There was a problem hiding this comment.
please log to debug/trace logger, that may help to identify issue and fix in future
There was a problem hiding this comment.
Done, I also logged the stack trace. let me know if you prefer to log only the error message
| @@ -31,6 +33,8 @@ | |||
| */ | |||
| public class ResponseStatusExceptionFilter implements ClientResponseFilter { | |||
|
|
|||
| private static final Logger LOGGER = LoggerFactory.getLogger(ResponseStatusExceptionFilter.class.getName()); | |||
There was a problem hiding this comment.
without .getName() just class is enough
| @@ -31,6 +33,8 @@ | |||
| */ | |||
| public class ResponseStatusExceptionFilter implements ClientResponseFilter { | |||
|
|
|||
| private static final Logger LOGGER = LoggerFactory.getLogger(ResponseStatusExceptionFilter.class.getName()); | |||
There was a problem hiding this comment.
FYI, there is 59 places using LOG vs 147 for LOGGER, should we consider changing the others for consistency (In a separate PR of course)?
There was a problem hiding this comment.
to minimise new lines i'm using shorter variant. No need to change old code, not critical.
Reading the entity stream a second time without resetting it was causing
the ObjectMapper to return null then a NullPointerExceptions when
calling get('message') method on it.
Fix the issue by creating the ObjectMapper from the String
representation of the entity stream instead of resetting the stream to
read it a second time. Also check to make sure the returned JsonNode is
not null and that the content is text.
Closes docker-java#1222
Reading the entity stream a second time without resetting it was causing
the ObjectMapper to return null then a NullPointerExceptions when
calling get('message') method on it.
Fix the issue by creating the ObjectMapper from the String
representation of the entity stream instead of resetting the stream to
read it a second time. Also check to make sure the returned JsonNode is
not null and that the content is text.
Closes #1222
This change is