fix: replace raw SSE string handling with SSEEvent model and SSEParser#842
fix: replace raw SSE string handling with SSEEvent model and SSEParser#842ehsavoie wants to merge 1 commit intoa2aproject:0.3.xfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a structured SSEEvent class and a dedicated SSEParser to handle Server-Sent Events across JSON-RPC, REST, and Android transport layers, replacing simple string consumers. The feedback identifies several areas in the SSEParser where strict adherence to the SSE specification is missing, specifically regarding state management during event dispatch, the handling of null characters in event IDs, and the requirement to ignore non-numeric retry fields. Additionally, a suggestion was made to improve the parser's resilience to Denial of Service attacks by clearing the buffer upon overflow.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated SSEEvent model and an SSEParser to improve Server-Sent Events (SSE) handling across the client and transport layers. The changes replace simple string-based data extraction with a spec-compliant parser that supports multi-line data, event types, IDs, and retry intervals. Corresponding updates were made to A2AHttpClient implementations and transport listeners to consume SSEEvent objects. A review comment suggests refining the retry field parsing in SSEParser to strictly follow the SSE specification by ignoring non-ASCII digit values instead of triggering an error handler.
76c86d7 to
e847ad4
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a structured SSEEvent class and a dedicated SSEParser to handle Server-Sent Events according to the specification, replacing simple string-based consumers across the HTTP client and transport layers. While the implementation significantly improves SSE handling, feedback suggests several refinements to the SSEParser for better robustness and spec compliance. Specifically, the data buffer should be cleared when line length limits are exceeded to prevent event corruption, and the event ID should persist across subsequent events in a stream. Furthermore, the error handling strategy for buffer overflows could be improved by discarding all fields until the next event boundary to ensure data integrity.
Introduce SSEEvent (data, id, event type, retry) and a spec-compliant SSEParser with buffer and line-length guards. Update postAsyncSSE and all SSE listeners across JDK, Android, JSONRPC, and REST transports to consume SSEEvent instead of raw strings. Using proper SSE Events Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a structured SSEEvent class and a dedicated SSEParser to replace raw string handling for Server-Sent Events, ensuring better compliance with the SSE specification. The changes update the A2AHttpClient interface and its implementations, along with the corresponding transport layers and listeners. Feedback suggests using Long.parseLong for the retry field to avoid potential overflows and refining the buffer size limit calculation in SSEParser to account for newline characters added during event dispatch.
Introduce SSEEvent (data, id, event type, retry) and a spec-compliant SSEParser with buffer and line-length guards. Update postAsyncSSE and all SSE listeners across JDK, Android, JSONRPC, and REST transports to consume SSEEvent instead of raw strings. Using proper SSE Events
Fixes #841 🦕