Skip to content
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

Enhance HTTP Status handling #1472

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enhance HTTP Status handling #1472

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
21 changes: 21 additions & 0 deletions javascript/net/grpc/web/grpcwebclientbase_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,27 @@ testSuite({
assertEquals(3, error.code);
},

async testRpcErrorWithHttpStatusCode() {
const xhr = new XhrIo();
const client = new GrpcWebClientBase(/* options= */ {}, xhr);
const methodDescriptor = createMethodDescriptor((bytes) => new MockReply());

const error = await new Promise((resolve, reject) => {
client.rpcCall(
'urlurl', new MockRequest(), /* metadata= */ {}, methodDescriptor,
(error, response) => {
assertNull(response);
resolve(error);
});
// This decodes to "grpc-status: 3"
xhr.simulateResponse(505, '', {'Content-Type': 'text/html'});
});
assertTrue(error instanceof RpcError);
assert('metadata' in error);
assert('httpStatusCode' in error.metadata);
assertEquals(505, error.metadata.httpStatusCode);
},

async testRpcDeserializationError() {
const xhr = new XhrIo();
const client = new GrpcWebClientBase(/* options= */ {}, xhr);
Expand Down
15 changes: 7 additions & 8 deletions javascript/net/grpc/web/grpcwebclientreadablestream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,8 @@ class GrpcWebClientReadableStream {

const self = this;
events.listen(this.xhr_, EventType.READY_STATE_CHANGE, function(e) {
let contentType = self.xhr_.getStreamingResponseHeader('Content-Type');
if (!contentType) return;
contentType = contentType.toLowerCase();
const contentType = (self.xhr_.getStreamingResponseHeader('Content-Type') || '').toLowerCase();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain a bit more on the motivation of all these changes (besides just adding the httpStatusCode in metadata)?

It looks to me that it's altering some existing logic, and I'm not sure if it can cause regressions.

From the PR description, I got the impression that adding httpStatusCode was all we needed.

Is that not the case?

Copy link
Author

Choose a reason for hiding this comment

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

Hi, already described it above: #1472 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

@sampajano can we move this forward?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wszydlak Hey sorry i was caught up with other business.. I'll try to get back to this early next week!

Copy link
Author

Choose a reason for hiding this comment

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

@sampajano any updates?

if (!contentType.startsWith('application/grpc')) return;

let byteSource;
if (googString.startsWith(contentType, 'application/grpc-web-text')) {
Expand All @@ -152,11 +151,8 @@ class GrpcWebClientReadableStream {
} else if (googString.startsWith(contentType, 'application/grpc')) {
byteSource = new Uint8Array(
/** @type {!ArrayBuffer} */ (self.xhr_.getResponse()));
} else {
self.handleError_(
new RpcError(StatusCode.UNKNOWN, 'Unknown Content-type received.'));
return;
}

let messages = null;
try {
messages = self.parser_.parse(byteSource);
Expand Down Expand Up @@ -257,11 +253,14 @@ class GrpcWebClientReadableStream {
return;
}
let errorMessage = ErrorCode.getDebugMessage(lastErrorCode);

const errorMetadata = /** @type {!StatusMetadata} */ ({});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line won't pass closure compilation (it will break internal tests, but not sure why it's not throwing up in tests :)) — since StatusMetadata is not a defined type in Closure JS.

Copy link
Author

Choose a reason for hiding this comment

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

How then can we define it as a type for Closure JS? How is Metadata defined in closure js?

Copy link
Author

@wszydlak wszydlak Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is this about creating file like this: javascript/net/grpc/web/metadata.js ?

if (xhrStatusCode != -1) {
errorMessage += ', http status code: ' + xhrStatusCode;
errorMetadata['httpStatusCode'] = xhrStatusCode;
}

self.handleError_(new RpcError(grpcStatusCode, errorMessage));
self.handleError_(new RpcError(grpcStatusCode, errorMessage, errorMetadata));
return;
}

Expand Down
9 changes: 6 additions & 3 deletions packages/grpc-web/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
declare module "grpc-web" {

export interface Metadata { [s: string]: string; }
export type StatusMetadata = Metadata & {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I appreciate the type extension, and i personally prefer this kind of change.

However, since I plan to have this feature only in open source grpc-web, i prefer to make minimal changes to the rest of the API (e.g. in RpcError, etc.). Would it be possible if you just modify Metadata instead?

Copy link
Author

Choose a reason for hiding this comment

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

That was my first tought, but, Metadata type is used both for input and output to grpc-services. Also i assume that previous definition od Metadata follows this: https://grpc.io/docs/guides/metadata/#overview (keys are string, values are string or binary).

To not make a breaking change for those who rely on Record<string, string> i created separate Metadata for request statuses (status + error) that additionaly can have new httpStatusCode property of number type.

I think this is more secure way to handle Metadata for status and error.

Don't see any reason why httpStatusCode should be valid property for standard messaging (in-out).

That's why i decided to make separate type.

httpStatusCode?: number
};

export class AbstractClientBase {
thenableCall<REQ, RESP> (
Expand Down Expand Up @@ -105,15 +108,15 @@ declare module "grpc-web" {
}

export class RpcError extends Error {
constructor(code: StatusCode, message: string, metadata: Metadata);
constructor(code: StatusCode, message: string, metadata: StatusMetadata);
code: StatusCode;
metadata: Metadata;
metadata: StatusMetadata;
}

export interface Status {
code: number;
details: string;
metadata?: Metadata;
metadata?: StatusMetadata;
}

export enum StatusCode {
Expand Down
77 changes: 77 additions & 0 deletions packages/grpc-web/test/generated_code_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,29 @@ describe('grpc-web generated code: promise-based client', function() {
});
});

it('should receive error, on http error - Content-Type not matching application/grpc*', function(done) {
const {EchoServicePromiseClient} = require(genCodePath);
const {EchoRequest} = require(protoGenCodePath);
MockXMLHttpRequest.onSend = function(xhr) {
xhr.respond(
505, {'Content-Type': 'text/html'});
};
var echoService = new EchoServicePromiseClient('MyHostname', null, null);
var request = new EchoRequest();
request.setMessage('aaa');

echoService.echo(request, {})
.then((response) => {
assert.fail('should not receive response');
})
.catch((error) => {
assert('metadata' in error);
assert('httpStatusCode' in error.metadata);
assert.equal(505, error.metadata.httpStatusCode);
done();
});
});

it('should receive error', function(done) {
const {EchoServicePromiseClient} = require(genCodePath);
const {EchoRequest} = require(protoGenCodePath);
Expand Down Expand Up @@ -613,6 +636,36 @@ describe('grpc-web generated code: callbacks tests', function() {
});
});

it('should receive error, on http error - Content-Type not matching application/grpc*', function(done) {
done = multiDone(done, 2);
MockXMLHttpRequest.onSend = function(xhr) {
xhr.respond(
505, {'Content-Type': 'text/html'});
};
var call = echoService.echo(
request, {},
function(err, response) {
if (response) {
assert.fail('should not have received response with non-OK status');
} else {
assert('metadata' in err);
assert('httpStatusCode' in err.metadata);
assert.equal(505, err.metadata.httpStatusCode);
}
done();
}
);
call.on('status', (status) => {
assert('metadata' in status);
assert('httpStatusCode' in status.metadata);
assert.equal(505, status.metadata.httpStatusCode);
done();
});
call.on('error', (error) => {
assert.fail('error callback should not be called for unary calls');
});
});

it('should receive error, on http error', function(done) {
done = multiDone(done, 2);
MockXMLHttpRequest.onSend = function(xhr) {
Expand Down Expand Up @@ -802,6 +855,30 @@ describe('grpc-web generated code: callbacks tests', function() {
});
});

it('should receive error, on http error (streaming) - Content-Type not matching application/grpc*', function(done) {
done = multiDone(done, 2);
MockXMLHttpRequest.onSend = function(xhr) {
xhr.respond(
505, {'Content-Type': 'text/html'});
};
var call = echoService.serverStreamingEcho(request, {});
call.on('data', (response) => {
assert.fail('should not receive data response');
});
call.on('status', (status) => {
assert('metadata' in status);
assert('httpStatusCode' in status.metadata);
assert.equal(505, status.metadata.httpStatusCode);
done();
});
call.on('error', (error) => {
assert('metadata' in error);
assert('httpStatusCode' in error.metadata);
assert.equal(505, error.metadata.httpStatusCode);
done();
});
});

it('should receive error, on http error (streaming)', function(done) {
done = multiDone(done, 2);
MockXMLHttpRequest.onSend = function(xhr) {
Expand Down