spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.7k stars 38.14k forks source link

SseEmitter should format a multiline String #30965

Closed dsyer closed 1 year ago

dsyer commented 1 year ago

The ServerSentEventHttpMessageWriter (used in Webflux but not MVC I think) has some logic to deal with multiline string data (e.g. when a @Controller method returns a Flux<String>):

if (data instanceof String text) {
  text = StringUtils.replace(text, "\n", "\ndata:");
  result = Flux.just(encodeText(sb + text + "\n\n", mediaType, factory));
}

which produces correct output if the input text has multiple lines. In MVC you have to write an SseEmitter and send data to it, e.g.

emitter.send(text, MediaType.TEXT_PLAIN);

but the emitter doesn't check for multiline text and you don't get the data: prefix after the first line. I can work around it by using text.replace("\n", "\ndata:") but it seems like I should expect Spring to do that for me, like it does in WebFlux.

rstoyanchev commented 1 year ago

SseEmitter does add the "data: " prefix, and also has the logic for the extra newline (one after each event line, one on build()). It's not clear what the issue is. A sample would be great.

dsyer commented 1 year ago

It adds the prefix, but only at the start of the entry - if the entry itself has multiple lines then it doesn't put the prefix at the start of every line (like you get for free in WebFlux IIUC). You can see what I mean in this sample: https://github.com/wimdeblauwe/blog-example-code/blob/master/htmx-sse/src/main/java/com/wimdeblauwe/examples/htmxsse/PdfGenerationController.java#L61 (he has to escape the new lines with \ so that the data is a single line).

rstoyanchev commented 1 year ago

We have tests for this. I will need a sample or something that demonstrates the issue.

rstoyanchev commented 1 year ago

As for the PdfGenerationController, I think what you're pointing out seems to be about new lines within the data (XML, JSON) itself? I don't believe we support with WebFlux either. Again a sample would be best to proceed.

dsyer commented 1 year ago

I don't think any of those tests uses a multiline data. One of us is missing something. Here's a tiny sample: https://github.com/scratches/sse-sample (and a "webflux" branch that shows you don't need the workaround).

wimdeblauwe commented 1 year ago

@rstoyanchev Did you have time to check out the example of Dave? Any other information you need?

rstoyanchev commented 1 year ago

@wimdeblauwe nothing further needed. Thanks for the sample @dsyer, I see what you mean now.