Skip to content

[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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 17, 2025
Merged

[SignalR] Don't throw for message headers in Java client #62739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion ...ignalR/clients/java/signalr/core/src/main/java/com/microsoft/signalr/GsonHubProtocol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,14 @@ public List<HubMessage> parseMessages(ByteBuffer payload, InvocationBinder binde
}
break;
case "headers":
throw new RuntimeException("Headers not implemented yet.");
// 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();
Comment on lines +142 to +148
Copy link

Copilot AI Jul 15, 2025

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.

Suggested change
// 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.

Copy link
Member

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?

Copy link
Member

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:

aspnetcore/src/SignalR/clients/java/signalr/messagepack/src/main/java/com/microsoft/signalr/messagepack/MessagePackHubProtocol.java

Line 194 in 36e93fa

Map<String, String> headers = readHeaders(unpacker);

aspnetcore/src/SignalR/clients/java/signalr/messagepack/src/main/java/com/microsoft/signalr/messagepack/MessagePackHubProtocol.java

Line 224 in 36e93fa

return new InvocationMessage(headers, invocationId, target, arguments, streams);

why do we completely ignore them here?

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense, approved.

break;
default:
// Skip unknown property, allows new clients to still work with old protocols
reader.skipValue();
Expand Down
94 changes: 94 additions & 0 deletions ...lR/clients/java/signalr/test/src/main/java/com/microsoft/signalr/GsonHubProtocolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,4 +527,98 @@ public void canRegisterTypeAdaptorWithoutAffectingJsonProtocol() {
assertEquals(3, (int) invocationMessage.getArguments()[0]);
assertEquals("four", invocationMessage.getArguments()[1]);
}

@Test
public void canParseInvocationMessageWithHeaders() {
String stringifiedMessage = "{\"type\":1,\"target\":\"test\",\"arguments\":[42],\"headers\":{\"a\":\"b\",\"c\":\"d\"}}\u001E";
ByteBuffer message = TestUtils.stringToByteBuffer(stringifiedMessage);
TestBinder binder = new TestBinder(new Type[] { int.class }, null);

List<HubMessage> messages = hubProtocol.parseMessages(message, binder);

assertNotNull(messages);
assertEquals(1, messages.size());

assertEquals(HubMessageType.INVOCATION, messages.get(0).getMessageType());
InvocationMessage invocationMessage = (InvocationMessage) messages.get(0);

assertEquals("test", invocationMessage.getTarget());
assertEquals(null, invocationMessage.getInvocationId());
int messageResult = (int)invocationMessage.getArguments()[0];
assertEquals(42, messageResult);
// Headers are parsed but not stored, so we just verify the message was processed successfully
}

@Test
public void canParseInvocationMessageWithEmptyHeaders() {
String stringifiedMessage = "{\"type\":1,\"target\":\"test\",\"arguments\":[42],\"headers\":{}}\u001E";
ByteBuffer message = TestUtils.stringToByteBuffer(stringifiedMessage);
TestBinder binder = new TestBinder(new Type[] { int.class }, null);

List<HubMessage> messages = hubProtocol.parseMessages(message, binder);

assertNotNull(messages);
assertEquals(1, messages.size());

assertEquals(HubMessageType.INVOCATION, messages.get(0).getMessageType());
InvocationMessage invocationMessage = (InvocationMessage) messages.get(0);

assertEquals("test", invocationMessage.getTarget());
int messageResult = (int)invocationMessage.getArguments()[0];
assertEquals(42, messageResult);
}

@Test
public void canParseCompletionMessageWithHeaders() {
String stringifiedMessage = "{\"type\":3,\"invocationId\":\"1\",\"result\":42,\"headers\":{\"a\":\"b\",\"c\":\"d\"}}\u001E";
ByteBuffer message = TestUtils.stringToByteBuffer(stringifiedMessage);
TestBinder binder = new TestBinder(null, int.class);

List<HubMessage> messages = hubProtocol.parseMessages(message, binder);

assertNotNull(messages);
assertEquals(1, messages.size());

assertEquals(HubMessageType.COMPLETION, messages.get(0).getMessageType());
CompletionMessage completionMessage = (CompletionMessage) messages.get(0);
assertEquals("1", completionMessage.getInvocationId());
assertEquals(42, completionMessage.getResult());
assertEquals(null, completionMessage.getError());
}

@Test
public void canParseStreamItemMessageWithHeaders() {
String stringifiedMessage = "{\"type\":2,\"invocationId\":\"1\",\"item\":\"test-item\",\"headers\":{\"a\":\"b\"}}\u001E";
ByteBuffer message = TestUtils.stringToByteBuffer(stringifiedMessage);
TestBinder binder = new TestBinder(null, String.class);

List<HubMessage> messages = hubProtocol.parseMessages(message, binder);

assertNotNull(messages);
assertEquals(1, messages.size());

assertEquals(HubMessageType.STREAM_ITEM, messages.get(0).getMessageType());
StreamItem streamItem = (StreamItem) messages.get(0);
assertEquals("1", streamItem.getInvocationId());
assertEquals("test-item", streamItem.getItem());
}

@Test
public void canParseMessageWithHeadersInDifferentOrder() {
String stringifiedMessage = "{\"headers\":{\"First\":\"value1\",\"Second\":\"value2\"},\"type\":1,\"target\":\"test\",\"arguments\":[42]}\u001E";
ByteBuffer message = TestUtils.stringToByteBuffer(stringifiedMessage);
TestBinder binder = new TestBinder(new Type[] { int.class }, null);

List<HubMessage> messages = hubProtocol.parseMessages(message, binder);

assertNotNull(messages);
assertEquals(1, messages.size());

assertEquals(HubMessageType.INVOCATION, messages.get(0).getMessageType());
InvocationMessage invocationMessage = (InvocationMessage) messages.get(0);

assertEquals("test", invocationMessage.getTarget());
int messageResult = (int)invocationMessage.getArguments()[0];
assertEquals(42, messageResult);
}
}
Loading