Fallback to simple String for invalid JSON body #39
Conversation
| addUnless(builder, "body", request.getBodyAsString(), String::isEmpty); | ||
| try { | ||
| builder.put("body", body.isEmpty() ? JsonBody.EMPTY : new JsonBody(compactJson(body))); | ||
| return; |
There was a problem hiding this comment.
Control flow is a bit confusing. Can we maybe extract a method returning an optional or nullable JsonBody?
There was a problem hiding this comment.
Yes - actually, why do we differ in behavior for empty JSON-body and empty non-JSON-body @whiskeysierra ?
There was a problem hiding this comment.
Yes - actually, why do we differ in behavior for empty JSON-body and empty non-JSON-body @whiskeysierra ?
I think we can switch those conditions and
- check for empty body
- check for content type json
There was a problem hiding this comment.
I tried this but then some tests were failing (those that are checking that for JSON an empty body is serialized as an empty string and for all other content-type the body is omitted). I'd go with omitting the body for an empty body, regardless of the content-type.
There was a problem hiding this comment.
I tried this but then some tests were failing (those that are checking that for JSON an empty body is serialized as an empty string and for all other content-type the body is omitted). I'd go with omitting the body for an empty body, regardless of the content-type.
I think I remember now. Depending on which RFC one tries to honor, a json body can be an empty json string, i.e. "", which would be different from an empty body.
There was a problem hiding this comment.
Control flow is a bit confusing. Can we maybe extract a method returning an optional or nullable JsonBody?
Would it make sense to use some kind of SPI here to allow embedding bodies as json for different content types? Because with the current approach one always needs to change the JsonHttpLogFormatter.
|
As this is a kind-of breaking bug in this library, should we merge this? We could open an issue to implement the SPI later. |
Agreed. I created #41 |
Fallback to simple String for invalid JSON body
Fixes #38