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.

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

fix: return Google Error when there is a missing required parameter #1291

Merged
merged 22 commits into from
Aug 17, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
4 changes: 4 additions & 0 deletions src/fallbackRest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ export function encodeRequest(
rpc.parsedOptions,
rpc.resolvedRequestType!.fields
);
if (transcoded instanceof GoogleError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I think it's pretty much the same as putting the call to transcode() into try ... catch, then we won't need to change its return value. It may or may not be cleaner.

throw transcoded;
Copy link
Contributor

Choose a reason for hiding this comment

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

I finally got back to reviewing this. In line 68, you will throw undefined if the transcode() call failed, this does not seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, the only way I can think to fix this is how I had it before, namely:

if (transcoded instanceof GoogleError) {
    throw transcoded;
}

I think changing it to try-catch causes the problem you're describing.

}

if (!transcoded) {
throw new Error(
`Cannot build HTTP request for ${JSON.stringify(json)}, method: ${
Expand Down
23 changes: 14 additions & 9 deletions src/fallbackServiceStub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {hasWindowFetch, hasAbortController} from './featureDetection';
import {AuthClient} from './fallback';
import {StreamArrayParser} from './streamArrayParser';
import {pipeline, PipelineSource} from 'stream';
import {GoogleError} from './googleError';

interface NodeFetchType {
(url: RequestInfo, init?: RequestInit): Promise<Response>;
Expand All @@ -37,7 +38,7 @@ export interface FallbackServiceStub {
options: {},
metadata: {},
callback: (err?: Error, response?: {} | undefined) => void
) => StreamArrayParser | {cancel: () => void};
) => StreamArrayParser | {cancel: () => void} | Promise<GoogleError>;
}

export interface FetchParameters {
Expand Down Expand Up @@ -91,14 +92,18 @@ export function generateServiceStub(
: new NodeAbortController();
const cancelSignal = cancelController.signal as AbortSignal;
let cancelRequested = false;

const fetchParameters = requestEncoder(
rpc,
protocol,
servicePath,
servicePort,
request
);
let fetchParameters: FetchParameters;
try {
fetchParameters = requestEncoder(
rpc,
protocol,
servicePath,
servicePort,
request
);
} catch (err) {
return Promise.resolve(callback(err));
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be the right place to catch it.

I wonder if just doing if (callback) { callback(err); } return; will do the trick without explicitly creating a promise here. Can you try that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave it a try, it didn't work!

}
const url = fetchParameters.url;
const headers = fetchParameters.headers;
for (const key of Object.keys(options)) {
Expand Down
5 changes: 3 additions & 2 deletions src/transcoding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import {JSONObject, JSONValue} from 'proto3-json-serializer';
import {Field} from 'protobufjs';
import {google} from '../protos/http';
import {GoogleError} from './googleError';
import {camelToSnakeCase, toCamelCase as snakeToCamelCase} from './util';

export interface TranscodedRequest {
Expand Down Expand Up @@ -297,13 +298,13 @@ export function transcode(
request: JSONObject,
parsedOptions: ParsedOptionsType,
requestFields?: {[k: string]: Field}
): TranscodedRequest | undefined {
): TranscodedRequest | GoogleError | undefined {
const {requiredFields, optionalFields} =
getFieldNameOnBehavior(requestFields);
// all fields annotated as REQUIRED MUST be emitted in the body.
for (const requiredField of requiredFields) {
if (!(requiredField in request) || request[requiredField] === 'undefined') {
throw new Error(
return new GoogleError(
`Required field ${requiredField} is not present in the request.`
);
}
Expand Down
3 changes: 2 additions & 1 deletion test/browser-test/test.grpc-fallback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import * as protobuf from 'protobufjs';
import * as sinon from 'sinon';

import * as fallback from '../../src/fallback';
import {StreamArrayParser} from '../../src/streamArrayParser';
import {echoProtoJson} from '../fixtures/echoProtoJson';
import {EchoClient} from '../fixtures/google-gax-packaging-test-app/src/v1beta1/echo_client';

Expand Down Expand Up @@ -220,7 +221,7 @@ describe('grpc-fallback', () => {
const request = {content: 'content' + new Date().toString()};
const call = echoStub.echo(request, {}, {}, () => {});

call.cancel();
(call as StreamArrayParser).cancel();

// @ts-ignore
assert.strictEqual(createdAbortControllers[0].abortCalled, true);
Expand Down
3 changes: 2 additions & 1 deletion test/unit/grpc-fallback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import * as sinon from 'sinon';
import {echoProtoJson} from '../fixtures/echoProtoJson';
import {GrpcClient} from '../../src/fallback';
import {GoogleError} from '../../src';
import {StreamArrayParser} from '../../src/streamArrayParser';

// @ts-ignore
const hasAbortController = typeof AbortController !== 'undefined';
Expand Down Expand Up @@ -402,7 +403,7 @@ describe('grpc-fallback', () => {
const request = {content: 'content' + new Date().toString()};
const call = echoStub.echo(request, {}, {}, () => {});

call.cancel();
(call as StreamArrayParser).cancel();

// @ts-ignore
assert.strictEqual(createdAbortControllers[0].abortCalled, true);
Expand Down
35 changes: 24 additions & 11 deletions test/unit/transcoding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
buildQueryStringComponents,
requestChangeCaseAndCleanup,
overrideHttpRules,
TranscodedRequest,
} from '../../src/transcoding';
import * as assert from 'assert';
import {
Expand All @@ -42,6 +43,7 @@ import * as protobuf from 'protobufjs';
import {testMessageJson} from '../fixtures/fallbackOptional';
import {echoProtoJson} from '../fixtures/echoProtoJson';
import {google} from '../../protos/http';
import {GoogleError} from '../../src';

describe('gRPC to HTTP transcoding', () => {
const parsedOptions: ParsedOptionsType = [
Expand Down Expand Up @@ -594,7 +596,7 @@ describe('validate proto3 field with default value', () => {
];
const transcoded = transcode(request, parsedOptions, badTestMessageFields);
assert.deepStrictEqual(
transcoded?.url,
(transcoded as TranscodedRequest)?.url,
'projects/test-project/contents/test-content'
);
});
Expand All @@ -620,9 +622,9 @@ describe('validate proto3 field with default value', () => {
},
];
for (const request of requests) {
assert.throws(
() => transcode(request, parsedOptions, testMessageFields),
Error
assert.ok(
transcode(request, parsedOptions, testMessageFields) instanceof
GoogleError
);
}
});
Expand All @@ -640,8 +642,13 @@ describe('validate proto3 field with default value', () => {
},
];
const transcoded = transcode(request, parsedOptions, testMessageFields);
assert.deepStrictEqual(transcoded?.url, 'projects/test-project');
assert.deepStrictEqual(transcoded?.data, {content: 'test-content'});
assert.deepStrictEqual(
(transcoded as TranscodedRequest)?.url,
'projects/test-project'
);
assert.deepStrictEqual((transcoded as TranscodedRequest)?.data, {
content: 'test-content',
});
});
it('when body="*", unset optional field should remove from body', () => {
const requests: RequestType[] = [
Expand All @@ -666,10 +673,10 @@ describe('validate proto3 field with default value', () => {
for (const request of requests) {
const transcoded = transcode(request, parsedOptions, testMessageFields);
assert.deepStrictEqual(
transcoded?.url,
(transcoded as TranscodedRequest)?.url,
'projects/test-project/contents/test-content'
);
assert.deepStrictEqual(transcoded?.data, {});
assert.deepStrictEqual((transcoded as TranscodedRequest)?.data, {});
}
});
it('unset optional fields should not appear in query params', () => {
Expand Down Expand Up @@ -697,9 +704,15 @@ describe('validate proto3 field with default value', () => {
];
for (const request of requests) {
const transcoded = transcode(request, parsedOptions, testMessageFields);
assert.deepStrictEqual(transcoded?.url, 'projects/test-project');
assert.deepStrictEqual(transcoded?.data, 'test-content');
assert.deepStrictEqual(transcoded.queryString, '');
assert.deepStrictEqual(
(transcoded as TranscodedRequest)?.url,
'projects/test-project'
);
assert.deepStrictEqual(
(transcoded as TranscodedRequest)?.data,
'test-content'
);
assert.deepStrictEqual((transcoded as TranscodedRequest).queryString, '');
}
});
});
Expand Down