Fix SSE event classification to follow spec for missing event field#913
Open
iaJingda wants to merge 2 commits intomodelcontextprotocol:mainfrom
Open
Fix SSE event classification to follow spec for missing event field#913iaJingda wants to merge 2 commits intomodelcontextprotocol:mainfrom
iaJingda wants to merge 2 commits intomodelcontextprotocol:mainfrom
Conversation
Per the SSE specification (WHATWG HTML Living Standard 9.2.6), an event with no explicit event field MUST be dispatched as a message event. HttpClientStreamableHttpTransport previously used strict equality and silently dropped such frames in the reconnect/GET stream path, causing server-initiated notifications to never reach the handler. Extract classification into a package-private isMessageEvent helper and cover with parameterized unit tests. Closes modelcontextprotocolgh-885
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix
HttpClientStreamableHttpTransportsilently dropping SSE frames that omit theevent:field, in violation of the SSE specification.Motivation and Context
Per the SSE specification (WHATWG HTML Living Standard §9.2.6):
The current
reconnect()/ GET SSE stream path inHttpClientStreamableHttpTransportuses a strict equality check:When a server emits a bare
data:frame without an accompanyingevent:line (which is valid per the spec),event()isnulland the frame is silently dropped. This manifests as server-initiated notifications never reaching the handler after initialization completes.Note the asymmetry: the
sendMessage()POST path does not check the event type at all and is unaffected, which is whyinitialize()succeeds on HttpClient. The bug only impacts server-pushed messages on the open GET stream.Originally reported in #885 against the Spring
WebClientStreamableHttpTransportvariant, which was moved to Spring AI 2.0 in #805. This PR fixes theHttpClientvariant that remains in this repository.How Has This Been Tested?
Added
HttpClientStreamableHttpTransportSseEventTypeTestwith parameterized unit tests covering:nulland empty event names → parsed as message (the bug scenario)"message"→ parsed as message (regression)ping,error,notification,MESSAGE/Messagecase variants, custom names) → not parsed as messageVerified locally:
Breaking Changes
None. This is a pure bug fix that relaxes an overly strict check to match the SSE specification. Existing behavior for explicit
"message"events is preserved, and non-message events are still ignored.Types of changes
Checklist
Additional context
Implementation note: I extracted the classification into a package-private static helper
isMessageEvent(String)rather than inlining the null/empty check. This keeps the call site readable, documents the spec rule in one place (with a link to WHATWG §9.2.6 in the Javadoc), and enables direct unit testing without setting up an HTTP server.Scope: This PR only addresses the
HttpClientvariant in this repository. TheWebClientvariant mentioned in #885 lives in Spring AI 2.0 after #805 and would need a separate fix there. See my comment on #885 for details on the asymmetry between the POST and GET paths.Closes gh-885