Skip to content

fix(google_cloud_rpc): use the google.rpc.Code of JSON errors in Status.code - #382

Open
quaaantumdev wants to merge 1 commit into
googleapis:mainfrom
quaaantumdev:rpc-json-error-code
Open

quaaantumdev wants to merge 1 commit into
googleapis:mainfrom
quaaantumdev:rpc-json-error-code

Conversation

@quaaantumdev

Copy link
Copy Markdown

Fixes #381.

For an error received as JSON, ServiceException.fromHttpResponse put the HTTP status code into status.code and dropped the google.rpc.Code that the error names in status. gRPC errors carry the google.rpc.Code there (see _mapGrpcError in google_cloud_pubsub), and ExponentialRetry.isRetryable relies on it. Over REST, an ABORTED error (409) was therefore never retried, and callers couldn't tell ABORTED from ALREADY_EXISTS, or FAILED_PRECONDITION from INVALID_ARGUMENT.

Changes

  • fromHttpResponse reads the error's status into Status.code, for example "ABORTED" becomes 10. When status is missing or isn't a google.rpc.Code, the HTTP status code stays, as before. ServiceException.statusCode still has the HTTP status code.
  • The doc comment of ServiceException.status says what code holds.

This changes status.code for JSON errors that name their code (400 becomes 3, and so on). The only other place in this repository that reads status.code for a REST error is google_cloud_storage's create_bucket_test.dart. Cloud Storage's JSON API errors don't carry a status, so that test still gets 409.

Tests

  • The existing valid error 400, 409 and other tests now expect the google.rpc.Code (3, 6 and 13).
  • New tests: ABORTED gives 10 and is retryable, and ALREADY_EXISTS is not. An error without a status, or with an unknown one, keeps the HTTP status code.
  • Without the fix, the four tests that expect a google.rpc.Code fail.
  • dart test in generated/google_cloud_rpc: 86 passed. dart analyze --fatal-infos and dart format are clean.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the test suite for service exceptions by refining status code assertions to match specific Google Cloud RPC error codes and adding new test cases to verify retryable behavior for specific error scenarios. I have no further feedback to provide as there were no review comments submitted.

…us.code

fromHttpResponse put the HTTP status code into Status.code and dropped
the google.rpc.Code that a JSON error names in its `status`. gRPC
errors carry the google.rpc.Code there, and ExponentialRetry.isRetryable
relies on it, so an ABORTED error received over REST was never retried.

Read `status` into Status.code, and keep the HTTP status code when it
is missing or unknown.

Fixes googleapis#381
@brianquinlan

Copy link
Copy Markdown
Contributor

Can you wait until #373 for a review? It significantly change how package:google_cloud_rpc works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[google_cloud_rpc] fromHttpResponse puts the HTTP status into Status.code and drops the google.rpc.Code, so ABORTED is never retried over REST

2 participants