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

Conversation

sofisl
Copy link
Contributor

@sofisl sofisl commented Jul 12, 2022

No description provided.

@sofisl sofisl requested a review from a team as a code owner July 12, 2022 01:06
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Jul 12, 2022
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jul 12, 2022
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!

@@ -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.

@sofisl
Copy link
Contributor Author

sofisl commented Jul 12, 2022

Fixes #1203

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.

@alexander-fenster alexander-fenster merged commit db73c27 into main Aug 17, 2022
@alexander-fenster alexander-fenster deleted the returnGoogleError branch August 17, 2022 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants