Skip to content

[TS Client] Fix CompletionMessage result when passing false or null #45169

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
amcasey merged 1 commit into from
Dec 12, 2022
Merged

[TS Client] Fix CompletionMessage result when passing false or null #45169

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
3 changes: 2 additions & 1 deletion src/SignalR/clients/ts/signalr-protocol-msgpack/src/MessagePackHubProtocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,8 @@ export class MessagePackHubProtocol implements IHubProtocol {
}

private _writeCompletion(completionMessage: CompletionMessage): ArrayBuffer {
const resultKind = completionMessage.error ? this._errorResult : completionMessage.result ? this._nonVoidResult : this._voidResult;
const resultKind = completionMessage.error ? this._errorResult :
Copy link
Member

@halter73 halter73 Nov 29, 2022

Choose a reason for hiding this comment

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

What if completionMessage.error is an empty string instead of undefined or null? Should that be treated as an error result still?

Copy link
Member Author

@BrennanConroy BrennanConroy Nov 29, 2022

Choose a reason for hiding this comment

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

Yeah, any non-null value should be considered an error. I don't think there is anyway today for an empty string to be set, but even if it was that should be considered an error.
In theory we should throw for both error and result being set, but since I want to backport this change, and we control the creation of completion messages it would basically only be an assert, I'm not going to add that.

(completionMessage.result !== undefined) ? this._nonVoidResult : this._voidResult;

let payload: any;
switch (resultKind) {
Expand Down
34 changes: 34 additions & 0 deletions src/SignalR/clients/ts/signalr-protocol-msgpack/tests/MessagePackHubProtocol.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,40 @@ describe("MessagePackHubProtocol", () => {
expect(new Uint8Array(buffer)).toEqual(payload);
});

it("can write completion message with false result", () => {
const payload = new Uint8Array([
0x09, // length prefix
0x95, // message array length = 5 (fixarray)
0x03, // type = 3 = Completion
0x80, // headers
0xa3, // invocationID = string length 3
0x61, // a
0x62, // b
0x63, // c
0x03, // result type, 3 - non-void result
0xc2, // false
]);
const buffer = new MessagePackHubProtocol().writeMessage({ type: MessageType.Completion, invocationId: "abc", result: false });
expect(new Uint8Array(buffer)).toEqual(payload);
});

it("can write completion message with null result", () => {
const payload = new Uint8Array([
0x09, // length prefix
0x95, // message array length = 5 (fixarray)
0x03, // type = 3 = Completion
0x80, // headers
0xa3, // invocationID = string length 3
0x61, // a
0x62, // b
0x63, // c
0x03, // result type, 3 - non-void result
0xc0, // null
]);
const buffer = new MessagePackHubProtocol().writeMessage({ type: MessageType.Completion, invocationId: "abc", result: null });
expect(new Uint8Array(buffer)).toEqual(payload);
});

it("will preserve double precision", () => {
const invocation = {
arguments: [Number(0.005)],
Expand Down