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

[acoutts]

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]

The committers are authorized under a signed CLA.

• ✅ Andrew Coutts (d197e98)

[acoutts]

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
      }
    });
  }
}

[acoutts]

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

[acoutts]

cc @kevmoo and @mit-mit

[mit-mit]

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.

[acoutts]

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

[mraleph]

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

[acoutts]

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.

[mraleph]

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.

[acoutts]

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');

[mraleph]

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

[mraleph]

I think this better be moved to CONTRIBUTING.md

[acoutts]

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

[acoutts]

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

[mraleph]

I think this comment still applies.

[mraleph]

LGTM! I will merge once merging is unblocked.

[acoutts]

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

[acoutts]

@mraleph any estimate when merging will be unblocked?

[mit-mit]

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]

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

[mraleph]

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]

Hey @mraleph is there any update for this merge?

[mraleph]

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

[acoutts]

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

[acoutts]

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

[mraleph]

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

[mraleph]

@acoutts 2.7.0 released