-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[SignalR] Don't throw for message headers in Java client #62739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the Java SignalR client to gracefully ignore message headers instead of throwing, and adds tests to verify header parsing across various message types.
- The
GsonHubProtocol
is modified to skip over the"headers"
JSON property rather than throwing an exception. - New unit tests in
GsonHubProtocolTest
ensure invocation, completion, and streaming messages with headers (including empty and reordered headers) are parsed without error.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/SignalR/clients/java/signalr/core/src/main/java/com/microsoft/signalr/GsonHubProtocol.java | Replace exception on "headers" field with code that skips its contents |
src/SignalR/clients/java/signalr/test/src/main/java/com/microsoft/signalr/GsonHubProtocolTest.java | Add tests covering parsing of messages with headers in different scenarios |
src/SignalR/clients/java/signalr/core/src/main/java/com/microsoft/signalr/GsonHubProtocol.java
Show resolved
Hide resolved
// Parse headers as Map<String, String> but don't store for now as it's unused | ||
reader.beginObject(); | ||
while (reader.hasNext()) { | ||
reader.nextName(); // Read the key | ||
reader.nextString(); // Read the value | ||
} | ||
reader.endObject(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify skipping the entire headers object by using reader.skipValue()
instead of manually iterating through each property—this reduces boilerplate and potential parsing errors.
// Parse headers as Map<String, String> but don't store for now as it's unused | |
reader.beginObject(); | |
while (reader.hasNext()) { | |
reader.nextName(); // Read the key | |
reader.nextString(); // Read the value | |
} | |
reader.endObject(); | |
// Skip the entire headers object as it's unused | |
reader.skipValue(); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a Java expert but I looked up this API and it seems like JsonReader.skipValue is a better way to skip processing the entire "headers" object. Is there a reason we aren't using it here, especially if the value isn't stored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ye, seems a nice API to use here. also as Brennan explained in one of the issue comments headers are parsed in MessagePack - but they are used later:
Line 194 in 36e93fa
Map<String, String> headers = readHeaders(unpacker); |
Line 224 in 36e93fa
return new InvocationMessage(headers, invocationId, target, arguments, streams); |
why do we completely ignore them here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we completely ignore them here?
They aren't really used. We set them on the messages but don't actually use them.
Is there a reason we aren't using it here, especially if the value isn't stored?
It doesn't really matter what we do here, but I do like the current code since it technically validates the json is {string:string}
whereas skip would just validate that some type of object exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, approved.
/backport to release/9.0 |
Started backporting to release/9.0: https://github.com/dotnet/aspnetcore/actions/runs/16352820503 |
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/16352823734 |
Fixes #62713