Skip to content

Add support for SSE comments - #478

Merged
adamw merged 21 commits into
masterfrom
sse-comments
Sep 15, 2026
Merged

adamw merged 21 commits into
masterfrom
sse-comments

Conversation

@magdzikk

@magdzikk magdzikk commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #379.

ServerSentEvent gains a comments: List[String] field, so comment lines (those starting with :) round-trip through both parse and toString. ServerSentEvent.comment("ping") builds the keep-alive frame the WhatWG specification recommends sending every 15 seconds or so, to stop legacy proxies from dropping an idle connection.

Design notes

List[String] rather than Option[String], because a single event block may carry several comment lines.

There is deliberately no assertion that a comment and data aren't both set (as considered in the issue discussion). The specification ignores comments per line, not per event, so a block such as

: keep-alive
data: abc

is valid and still dispatches abc. A guard would reject valid streams.

Binary compatibility

Preserved with the same approach already used for ContentTypeRange: the old-arity constructor, copy and apply are kept alongside the new ones, and mimaReportBinaryIssues is green.

MiMa does not see everything, though. The source-level and runtime-shape changes it cannot report are listed under Migration below.

Migration

Everything here follows from ServerSentEvent gaining a field. Recompiling against the new version is the general fix; the two behaviour changes need a code change as well.

Comment-only events are no longer empty. A : ping block used to parse to ServerSentEvent(), and now parses to an event with comments set, so code that dropped keep-alives by comparing against an empty event stops dropping them:

events.filterNot(_ == ServerSentEvent())  // no longer drops keep-alives
events.filterNot(_.hasNoFields)           // use this instead

hasNoFields is true when an event carries no data, event type, id or retry, so it drops exactly the set that the old comparison dropped — keep-alives, empty blocks, and blocks of unknown fields alike.

Events parsed from a stream containing comments no longer equal hand-built events. Streams often open with a : handshake comment, so expected values need updating, or the comments dropped before comparing:

ServerSentEvent.parse(lines) == ServerSentEvent(Some("x"))                      // false once a comment is present
ServerSentEvent.parse(lines).copy(comments = Nil) == ServerSentEvent(Some("x")) // compare without them

Patterns over the four original fields no longer compile. Add a fifth _, or use the accessors:

case ServerSentEvent(d, e, i, r)    => ... // does not compile
case ServerSentEvent(d, e, i, r, _) => ...

Code compiled against an earlier version has to be recompiled. The changes below leave the erased signatures intact, so such code links and then fails at runtime, and MiMa cannot report it:

  • ServerSentEvent.unapply now returns a Tuple5. A caller compiled earlier fails with ClassCastException: scala.Tuple5 cannot be cast to scala.Tuple4. Compiled patterns are unaffected — they lower to accessor calls rather than to unapply.
  • Scala 3 codecs derived through Mirror read a fifth element. One compiled earlier fails with IndexOutOfBoundsException: 4.

Serialised forms change.

  • serialVersionUID changes from 5877847238202557287 to -2395116303002558390, so Java-serialised events do not cross the version boundary.
  • JSON codecs derived from the case class gain a comments field. circe and play-json ignore Scala default arguments, so their derived decoders reject JSON that lacks it; jsoniter-scala and zio-json read the default and are unaffected.

`ServerSentEvent` now carries a `comments` field, so that comment lines
(those starting with `:`) can be both parsed and serialised. Per the
WhatWG specification such lines are ignored by clients, which makes them
the idiomatic keep-alive: they stop proxies from dropping an idle
connection without dispatching an event to the application.

Binary compatibility with 1.7.18 is preserved in the same way as for
`ContentTypeRange`: the old-arity constructor, `copy` and `apply` are
kept alongside the new ones.

Closes #379

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@magdzikk
magdzikk marked this pull request as ready for review August 28, 2026 14:44

@adamw adamw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by Claude (posted by @adamw). Main finding: comments are not split on newlines in toString. Details inline.

Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/test/scala/sttp/model/sse/ServerSentEventTest.scala
Comment thread core/src/test/scala/sttp/model/sse/ServerSentEventTest.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala
adamw added a commit that referenced this pull request Sep 15, 2026
Two things were wrong with how `ServerSentEvent.parse` reads a `retry:`
line. The
[spec](https://html.spec.whatwg.org/multipage/server-sent-events.html#event-stream-interpretation)
says the value must be ASCII digits only, and that the field is ignored
otherwise.

**An unreadable value used to reset `retry` to `None`**, dropping a
value from an earlier line of the same event:

```scala
ServerSentEvent.parse(List("retry: 5", "retry: x")).retry  // was None, now Some(5)
```

**A signed value used to parse**, so a consumer could get a negative
reconnect delay:

```scala
ServerSentEvent.parse(List("retry: -1")).retry  // was Some(-1), now None
```

Both now ignore the line and keep whatever was read before.

A note on the digit check: `Integer.parseInt` accepts non-ASCII digits,
and so does `Character.isDigit`, so `retry: ٥` parsed to `Some(5)`. The
check is an explicit `'0'` to `'9'` range for that reason. `toIntOption`
is still needed after it, to reject a value too long to fit an `Int`.

Found while reviewing #478, but unrelated to it.
Conflicts in ServerSentEvent: master added combineRetry, this branch
rewrote combineData to use accessors instead of a 4-field pattern, which
no longer compiles now that the class has five fields. Kept both.

Master also stopped parsing a negative retry value, so the property test
generator no longer produces one - such an event cannot round-trip.
@adamw
adamw merged commit 887f231 into master Sep 15, 2026
10 checks passed
@adamw
adamw deleted the sse-comments branch September 15, 2026 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ServerSentEvent type does not match the WhatWG specification WRT comments

2 participants