-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Add SignalR client log for stream item binding failure #60857
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,13 +92,26 @@ public List<HubMessage> parseMessages(ByteBuffer payload, InvocationBinder binde | |
| error = reader.nextString(); | ||
| break; | ||
| case "result": | ||
| case "item": | ||
| if (invocationId == null || binder.getReturnType(invocationId) == null) { | ||
| resultToken = JsonParser.parseReader(reader); | ||
| } else { | ||
| result = gson.fromJson(reader, binder.getReturnType(invocationId)); | ||
| } | ||
| break; | ||
| case "item": | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason we don't need to set the same guards when we're parsing properties from the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason is that it's a different bug/improvement and I don't want to pollute this PR 😆 |
||
| if (invocationId == null || binder.getReturnType(invocationId) == null) { | ||
| resultToken = JsonParser.parseReader(reader); | ||
| } else { | ||
| try { | ||
| result = gson.fromJson(reader, binder.getReturnType(invocationId)); | ||
| } catch (Exception ex) { | ||
| argumentBindingException = ex; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a way of capturing the value that failed to bind here? If so, that might be helpful to capture in the exception. Ditto with the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Relying on the underlying json library to provide the error, in the case of the test I added the error would contain: |
||
| // Since we failed to parse the value, tell the reader to skip the failed item | ||
| // so it can successfully continue reading | ||
| reader.skipValue(); | ||
| } | ||
| } | ||
| break; | ||
| case "arguments": | ||
| if (target != null) { | ||
| boolean startedArray = false; | ||
|
|
@@ -167,9 +180,17 @@ public List<HubMessage> parseMessages(ByteBuffer payload, InvocationBinder binde | |
| case STREAM_ITEM: | ||
| if (resultToken != null) { | ||
| Type returnType = binder.getReturnType(invocationId); | ||
| result = gson.fromJson(resultToken, returnType != null ? returnType : Object.class); | ||
| try { | ||
| result = gson.fromJson(resultToken, returnType != null ? returnType : Object.class); | ||
| } catch (Exception ex) { | ||
| argumentBindingException = ex; | ||
| } | ||
| } | ||
| if (argumentBindingException != null) { | ||
| hubMessages.add(new StreamBindingFailureMessage(invocationId, argumentBindingException)); | ||
| } else { | ||
| hubMessages.add(new StreamItem(null, invocationId, result)); | ||
| } | ||
| hubMessages.add(new StreamItem(null, invocationId, result)); | ||
| break; | ||
| case STREAM_INVOCATION: | ||
| case CANCEL_INVOCATION: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,11 +10,10 @@ dependencies { | |
| implementation 'org.junit.jupiter:junit-jupiter-params:5.11.2' | ||
| testImplementation 'org.junit.jupiter:junit-jupiter:5.11.2' | ||
| testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.11.2' | ||
| implementation 'com.google.code.gson:gson:2.8.5' | ||
| implementation 'com.google.code.gson:gson:2.8.9' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this bump related to the fix or just for good measure?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't have been needed, but for some reason when debugging the test in VS Code it complained about a method not being found in gson, command line passed just fine. So I updated the dep and VS Code started working 🤷 |
||
| implementation 'ch.qos.logback:logback-classic:1.2.3' | ||
| implementation project(':core') | ||
| implementation project(':messagepack') | ||
| implementation project(':messagepack') | ||
| antJUnit 'org.apache.ant:ant-junit:1.10.15' | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.