Add decoded grpc-status-details-bin details to GrpcError #349

acoutts · 2020-09-14T13:54:20Z

Currently there is no way to access the details of a GrpcError in the Dart implementation. This PR adds the fully-typed details into the dart exception that gets thrown.

This implements what's requested in #209.

cc @jakobr-google as I saw you had a TODO comment in call.dart to do this someday.

linux-foundation-easycla · 2020-09-14T13:54:23Z

The committers are authorized under a signed CLA.

✅ Andrew Coutts (d197e98)

acoutts · 2020-09-14T15:34:18Z

I've just added handling to parse the details into the correct GeneratedMessage types and added the required proto / generated files for the typings.

You can now handle your errors like this:

import 'package:grpc/grpc.dart';

try {
  // Some gRPC call
  await myCall();
} catch (e) {
  /// Handle error details
  if (e is GrpcError) {
    e.details.forEach((f) {
      if (f is BadRequest) {
        print('Bad request: ${f.first.fieldViolations.first.description}') // Bad request: The specified forex pair does not exist
      }
    });
  }
}

… code.proto. Includes status code string name in the error now

…ent from code.proto. Includes status code string name in the error now" This reverts commit e6049a4.

…arsing

acoutts · 2020-09-15T01:36:24Z

Anyone know why only one of the CI jobs are failing? Dart test and analyze both pass on my system.

acoutts · 2020-09-15T08:33:36Z

cc @kevmoo and @mit-mit

mit-mit · 2020-09-16T12:31:58Z

Logs:

Passing job in master: https://api.travis-ci.org/v3/job/727680315/log.txt
Failing job for this PR: https://api.travis-ci.org/v3/job/727185064/log.txt

So this is failing because of a resolution issue related to the new protobuf dependency that this PR adds.

Example: route guide client cleanup (grpc#350)

Merge master

acoutts · 2020-09-16T16:01:18Z

Thanks! Looks to be all fixed up now after bumping the version for protobuf in the other pubspec files.

mraleph · 2020-09-17T14:16:44Z

This PR needs at least some basic tests for the added functionality to make sure it works.

…s-bin

acoutts · 2020-09-17T18:23:44Z

Thanks @mraleph - I've just added in some tests. Let me know if there's anything you think needs deeper testing coverage. The main things in this PR are this:

If there is grpc-status-details-bin present in the headers, then we attempt to parse it to pull out the exception details.
The base64url string is padded if needed, as the base64url string must be a multiple of 4.
We instantiate a Status object from the decoded base64url, using Status.fromBuffer. This gives us a valid Status object, with the exception details now available in the details field as a List<Any> (pbAny).
We then parse each Any object into its respective GeneratedMessage type: BadRequest, RetryInfo, etc.
The list of detail objects (GeneratedMessage) are then added to a new field on GrpcError called details, which is a List<GeneratedMessage>.

If there is a failure during deserialization, the fallback is the GrcpError object will just have an empty list for its details.

I've added the protos we use to the project so those can be updated over time, using the included lib/src/tool/regenerate.sh script. We mainly need those to use the generated Status, Any, and various detail object types like BadRequest.

mraleph

This looks good. Thank you for the contribution! I have left few comments on the PR.

We are waiting right now on a manual internal-to-external synchronisation to complete (bringing our internal fork of this repo in sync with external). Once that completes and you address the comments - I will approve and merge this PR.

lib/src/tool/regenerate.sh

lib/src/client/call.dart

mraleph · 2020-09-21T08:01:26Z

lib/src/client/call.dart

-        if (status != 0) {
-          _responseError(GrpcError.custom(status, message));
+        final status = metadata['grpc-status'];
+        final statusCode = status != null ? int.parse(status) : 0;


It is not clear to me if the check for null is needed. I think it is an error if it is null at this point.

I believe the original author put this check in to ensure we don't try to do int.parse(null). Perhaps this is cleaner?

final statusCode = int.parse(status ?? '0');

You mean the check below on line 286 (because there was no check here before your change)?

I think the check there was just another way to write _headerMetadata.containsKey('grpc-status'). It's unclear to me why they choose two inconsistent ways to write it, but I also don't think that this header can even be null at this point.

(Well, NNBD migration will reveal the truth).

lib/src/client/call.dart

lib/src/shared/status.dart

test/client_tests/client_test.dart

…te protobuf definitions. Moved the regenerate script to the project root tool/ directory.

mraleph · 2020-09-22T11:16:57Z

README.md

@@ -25,3 +25,6 @@ For complete documentation, see [Dart gRPC](https://grpc.io/docs/languages/dart)
 If you experience problems or have feature requests, [open an issue](https://github.com/dart-lang/grpc-dart/issues/new).

 Note that we have limited bandwidth to accept PRs, and that all PRs require signing the [EasyCLA](https://lfcla.com).
+
+## Updating protobuf definitions


I think this better be moved to CONTRIBUTING.md

test/client_tests/client_test.dart

lib/src/client/call.dart

acoutts · 2020-09-22T12:00:43Z

Thanks @mraleph! Just pushed changes for those final items.

acoutts · 2020-09-22T13:07:17Z

The CI failures there don't seem related to the changes I've made- seems it failed to install chrome due to a bad checksum.

mraleph

Just two comments left.

I have landed a workaround for the issue with Travis in dd34af2

README.md

mraleph · 2020-09-23T09:41:40Z

lib/src/client/call.dart

+
+List<GeneratedMessage> decodeStatusDetails(String data) {
+  try {
+    /// Parse each Any type into the correct GeneratedMessage


I think this comment still applies.

mraleph

LGTM! I will merge once merging is unblocked.

acoutts · 2020-09-23T14:15:28Z

Just pushed one last change - bumped version in the pubspec to match the changelog entry.

acoutts · 2020-09-29T12:49:33Z

@mraleph any estimate when merging will be unblocked?

mit-mit · 2020-09-29T13:11:04Z

Earlier today the status update in a different PR was: "We are almost done with internal-to-external sync. So I would expect to be able to merge this by the end of the week (or maybe early next week)"

acoutts · 2020-09-29T13:12:41Z

Thanks @mit-mit! Shall I try and correct the pubspec version here or leave that for the final merge?

mraleph · 2020-09-29T14:00:35Z

Let's wait, we might have another PR or two coming which would change versions. I will ping this PR once we are ready to go.

acoutts · 2020-10-26T12:52:24Z

Hey @mraleph is there any update for this merge?

mraleph · 2020-10-26T14:40:57Z

We are ready to land. Could you rebase and resolve conflicts? I will merge after that.

acoutts · 2020-10-26T14:45:07Z

Alright just merge latest master from upstream (were no conflicts), and bumped version to 2.7.0.

acoutts · 2020-10-29T14:33:36Z

@mraleph the pub package is still at v2.3.0 with last release on 9/21. Master seems to be at v2.7.0. When will the next release be put on pub.dev?

* Bump SDK version constraint to 2.3 * Add dependency on fixnum package

mraleph · 2020-10-29T15:10:32Z

I am going to publish but I need to address few issues which slipped through testing (and analysis): #377

mraleph · 2020-10-30T11:09:06Z

@acoutts 2.7.0 released

Add status details as Uint8List to GrpcError

d197e98

acoutts mentioned this pull request Sep 14, 2020

Support 'details' from the RPC Error Model #209

Closed

acoutts added 3 commits September 14, 2020 10:15

Handle case where grpc-status-details-bin might be null

9d08aee

Added handling of all detail Any types such as RetryInfo and BadRequest

d7f806f

Add export for status detail types to for easier consumption

bf63427

acoutts added 5 commits September 14, 2020 11:41

Add export for generated error details and cleanup empty space

df0ba21

Replace StatusCode implementation with auto-generated equivalent from…

e6049a4

… code.proto. Includes status code string name in the error now

Revert "Replace StatusCode implementation with auto-generated equival…

2600cc1

…ent from code.proto. Includes status code string name in the error now" This reverts commit e6049a4.

Added status code name to GrpcError

32f30c6

Fixed status code parsing to be consistent | Fixed status code name p…

7076f88

…arsing

acoutts added 3 commits September 16, 2020 10:29

Bump protobuf dependency in examples and interop

f970c33

Merge pull request #1 from grpc/master

bc2ef04

Example: route guide client cleanup (grpc#350)

Merge pull request #2 from bottlepay/master

d41778c

Merge master

mraleph self-requested a review September 17, 2020 14:16

Fix googleapis generated protobuf files by running generator again

490b2b9

pull-request-size bot added the size/XXL label Sep 17, 2020

Added unit tests for extracting error details from grpc-status-detail…

f06943d

…s-bin

acoutts added 2 commits September 17, 2020 14:42

Merge branch 'master' into add-status-details

6d734ec

Bumped version and added changelog entry

a72916d

mraleph reviewed Sep 21, 2020

View reviewed changes

acoutts added 2 commits September 21, 2020 09:13

Merged master

0b9dc16

Updated readme to include how to use the regenerate.sh script to upda…

9b94792

…te protobuf definitions. Moved the regenerate script to the project root tool/ directory.

This comment has been minimized.

Sign in to view

More code review fixes

cd19716

mraleph requested changes Sep 22, 2020

View reviewed changes

Fixes for code review

058303a

mraleph requested changes Sep 23, 2020

View reviewed changes

acoutts added 3 commits September 23, 2020 08:42

Add return carriage to last line of readme

b8ffd50

Improve comment for decodeStatusDetails

150268e

Merge remote-tracking branch 'upstream/master' into add-status-details

d2d41f9

mraleph approved these changes Sep 23, 2020

View reviewed changes

Bump pubspec version

d5db793

mraleph linked an issue Sep 28, 2020 that may be closed by this pull request

Support 'details' from the RPC Error Model #209

Closed

acoutts added 2 commits October 26, 2020 10:43

Bump version and add DS_Store to gitignore

97fa899

Merge master from upstream

afa92e3

mraleph merged commit b6e40c3 into grpc:master Oct 27, 2020

mraleph added a commit to mraleph/grpc-dart that referenced this pull request Oct 29, 2020

Fix issues introduced by grpc#349

8b53d85

* Bump SDK version constraint to 2.3 * Add dependency on fixnum package

mraleph mentioned this pull request Aug 9, 2021

GrpcError details list is empty on client side #509

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add decoded grpc-status-details-bin details to GrpcError #349

Add decoded grpc-status-details-bin details to GrpcError #349

acoutts commented Sep 14, 2020 •

edited

Loading

linux-foundation-easycla bot commented Sep 14, 2020 •

edited

Loading

acoutts commented Sep 14, 2020 •

edited

Loading

acoutts commented Sep 15, 2020

acoutts commented Sep 15, 2020

mit-mit commented Sep 16, 2020

acoutts commented Sep 16, 2020

mraleph commented Sep 17, 2020

acoutts commented Sep 17, 2020 •

edited

Loading

mraleph left a comment

mraleph Sep 21, 2020

acoutts Sep 21, 2020

mraleph Sep 21, 2020

This comment has been minimized.

This comment has been minimized.

mraleph Sep 22, 2020

acoutts commented Sep 22, 2020

acoutts commented Sep 22, 2020

mraleph left a comment

mraleph Sep 23, 2020

mraleph left a comment

acoutts commented Sep 23, 2020

acoutts commented Sep 29, 2020

mit-mit commented Sep 29, 2020

acoutts commented Sep 29, 2020

mraleph commented Sep 29, 2020

acoutts commented Oct 26, 2020

mraleph commented Oct 26, 2020

acoutts commented Oct 26, 2020

acoutts commented Oct 29, 2020

mraleph commented Oct 29, 2020

mraleph commented Oct 30, 2020

Add decoded grpc-status-details-bin details to GrpcError #349

Add decoded grpc-status-details-bin details to GrpcError #349

Conversation

acoutts commented Sep 14, 2020 • edited Loading

linux-foundation-easycla bot commented Sep 14, 2020 • edited Loading

acoutts commented Sep 14, 2020 • edited Loading

acoutts commented Sep 15, 2020

acoutts commented Sep 15, 2020

mit-mit commented Sep 16, 2020

acoutts commented Sep 16, 2020

mraleph commented Sep 17, 2020

acoutts commented Sep 17, 2020 • edited Loading

mraleph left a comment

Choose a reason for hiding this comment

mraleph Sep 21, 2020

Choose a reason for hiding this comment

acoutts Sep 21, 2020

Choose a reason for hiding this comment

mraleph Sep 21, 2020

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

mraleph Sep 22, 2020

Choose a reason for hiding this comment

acoutts commented Sep 22, 2020

acoutts commented Sep 22, 2020

mraleph left a comment

Choose a reason for hiding this comment

mraleph Sep 23, 2020

Choose a reason for hiding this comment

mraleph left a comment

Choose a reason for hiding this comment

acoutts commented Sep 23, 2020

acoutts commented Sep 29, 2020

mit-mit commented Sep 29, 2020

acoutts commented Sep 29, 2020

mraleph commented Sep 29, 2020

acoutts commented Oct 26, 2020

mraleph commented Oct 26, 2020

acoutts commented Oct 26, 2020

acoutts commented Oct 29, 2020

mraleph commented Oct 29, 2020

mraleph commented Oct 30, 2020

acoutts commented Sep 14, 2020 •

edited

Loading

linux-foundation-easycla bot commented Sep 14, 2020 •

edited

Loading

acoutts commented Sep 14, 2020 •

edited

Loading

acoutts commented Sep 17, 2020 •

edited

Loading