@rstoyanchev Based on the Jackson PrettyPrinter
based trick with discussed + Jackson capabilities to specify such PrettyPrinter
at ObjectWriter
level, I think I ended up with a pretty good solution in the sense it is not intrusive and it does not lead to performance penalty. Could you say me if that's fine for you?
Also could you confirm that we should apply that to 4.3.x
branch as well?
Issue: SPR-14899
Comment From: rstoyanchev
@sdeleuze I'm wondering why the Jackson2JsonEncoder has to check a hint from ServerSentEventHttpMessageWriter? Couldn't it work like it's done in the AbstractJackson2HttpMessageConverter by checking the content type of the response? If that hint is removed essentially the Jackson encoder and converter have independent, built-in support to pretty print SSE data correctly which is a good place to be.
Comment From: sdeleuze
Good point, I missed the fact that encodeValue()
is a private method and that encode()
has the mimeType
parameter, I will change that and if you are ok with the rest, merge to master
.
Comment From: rstoyanchev
Yes
Comment From: sdeleuze
Hum in fact I am afraid we need the SSE hint, because the mime type we get at the encoder level is application/json
and not text/event-stream
. For Spring MVC that works because we retrieve the content type of the response, but on the encoder side, we don't have access to that so the hint is the only way I think.
Comment From: rstoyanchev
It looks like ServerSentEventMessageWriter passes application/json when it delegates to the Jacson encoder. I'm not sure that should be so actually. It's probably okay for locating the encoder but the content type of the response is text/event-stream after all. Would it cause an issue to pass text/event-stream?
Comment From: sdeleuze
In that Jackson particular case, that would work but nothing prevent an encoder implementation to re-validate the content type by calling canEncode()
again + I find a little bit weird to pass not the same content type to canEncode()
and encode()
, that's why I tend to prefer using the SSE hint since hints are designed for such use case.
Comment From: rstoyanchev
One could ask then if the JacksonEncoder supports "text/event-stream" but it's more like it's the ServerSentEventMessageWriter that supports it and delegates to other encoders. So the JacksonEncoder still only knows how to render JSON but it is also aware that's happening in the context of an SSE response.
Interestingly if JacksonEncoder was in spring-core where it could be shared with non-web modules, it would be harder or not possible to do this. Having a JacksonEncoder in spring-web is paying off and it's okay to make it aware of a hint from ServerSentEventMessageWriter, which is in the codec package above.
In short I'm okay with this. I would only change the hint to be more broadly about indicating that encoding is happening in the context of an SSE response -- something like SSE_CONTENT_HINT and in the Javadoc the "data:" prefixing is just an example of what the hint can be used for.