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 19 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
17 changes: 12 additions & 5 deletions src/fallbackRest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,18 @@ export function encodeRequest(
if (typeof json !== 'object' || Array.isArray(json)) {
throw new Error(`Request to RPC ${rpc.name} must be an object.`);
}
const transcoded = transcode(
json,
rpc.parsedOptions,
rpc.resolvedRequestType!.fields
);

let transcoded;
try {
transcoded = transcode(
json,
rpc.parsedOptions,
rpc.resolvedRequestType!.fields
);
} catch (err) {
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
3 changes: 2 additions & 1 deletion 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 @@ -303,7 +304,7 @@ export function transcode(
// 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(
throw 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 @@ -414,7 +415,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
31 changes: 22 additions & 9 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 Down Expand Up @@ -622,7 +624,7 @@ describe('validate proto3 field with default value', () => {
for (const request of requests) {
assert.throws(
() => transcode(request, parsedOptions, testMessageFields),
Error
/Error: Required field content is not present in the request/
);
}
});
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