status: Implement *statusError.Is #2868

jsm · 2019-06-13T23:36:37Z

In the go2 error proposal the Is function adds matching of error targets.
On this line of the proposal implementation, there is an interface check for Is, to provide
custom implementation of this matching.

gRPC status errors that are defined as a var, currently don't successfully work
with this function when crossing the gRPC boundary, as the pointer values will
not be equivalent.

Ideally, the following should work across gRPC boundaries:

var ErrAuthInvalidToken = status.Error(codes.Unauthenticated, "Invalid token")
_, err := grpcClient.Call(...) // Returns the above
errors.Is(err, ErrAuthInvalidToken) == true

thelinuxfoundation · 2019-06-13T23:36:39Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @jsm isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
CLA GitHub bot

dfawley

Thanks for the PR, I agree this will be useful to have.

The CLAs still seem to be unhappy - can you take another look at that, please?

dfawley · 2019-06-21T19:46:48Z

status/status.go

+		return false
+	}
+
+	return se.Code == tse.Code && se.Message == tse.Message


This doesn't check the Details field -- to do so, I believe you can use proto.Equal(se, tse) instead of comparing the fields individually.

@dfawley I believe we don't actually want to check the Details field, as we very well might have different details added onto the same error "type"

I imagine WithDetails could be used for things like tagging something like the userId, or host source, which would break the equivalency.

The same types of arguments could be used for checking the Message field as well. It's common for users to put debugging details (username, raw request information, etc) into RPC error strings. Whether this is more or less likely is conjecture. I believe the spirit of "Is" is to perform an exact match, not a fuzzy one. But I'll have to look at the errors proposal again.

Thinking about this some more, I think since Details is just appended to the end of the message, equality would break if they were added in a different order, which doesn't seem great.

I'm still of the opinion that it should be matching on just the code and message, but I'm happy to do a strict comparison instead if that will help push this through.

For As, I'm unsure about implementing it on &status.Status, as this isn't actually an error type, and it might be an anti-pattern to have As work on a non-error type? Maybe they intend for it to be usable this way but it seems a little weird.

I agree this seems a bit weird. As would need to be implemented on *statusError, but it would assign to a *status.Status. I'm not sure if this would be considered an anti-pattern, but I will pose the question.

Alternatively, we could export a interface{ GRPCStatus() *status.Status } type to be used in As.

I considered this as well. I'm not sure this option would be intuitive or discoverable, and I'd rather just offer a status.Unwrap(error) helper instead.

For the convenience wrapper, it seems like it would make sense to implement it in FromError(), although that seems like it would have to be in a separate go2 package.

We would have to move FromError into its own file, and define two versions, one for pre-1.13 versions of Go and one for 1.13+. It might be undesirable/unexpected to unwrap in that function automatically, although I'm having trouble seeing where it would cause problems to do so.

I'm not sure if this would be considered an anti-pattern, but I will pose the question.

Did you get a chance to think about this more?

Your plan for FromError seems good.

Sorry, I have not had a chance to ask yet. I'll see if I can find something out soon.

errors.Is() compares every single element in a chain of errors by Unwrap()ing the next level.
WithDetails() just adds such an element to the chain, so why should it be part of the comparison?

It just uses a different way for adding an element than the error package, but the principle is the same.

That's an interesting thought, but the problem is that WithDetails isn't necessarily wrapping a different error - it can also explain more about the error itself. We can't simply ignore it in a function intended to test strict equality.

jsm · 2019-06-21T23:55:10Z

@dfawley Working on getting a CCLA pushed through!

jsm · 2019-06-26T00:44:18Z

@dfawley CLA got signed, thanks for your patience.

jsm · 2019-07-09T20:11:40Z

@dfawley I updated this PR to use proto.Equal

jsm · 2019-07-23T04:54:21Z

@dfawley Friendly bump :)

dfawley · 2019-07-23T19:50:18Z

status/status.go

+// Is implements future error.Is functionality.
+// A statusError is equivalent if the code and message are identical.
+func (se *statusError) Is(target error) bool {
+	if target == nil {


Minor point: a nil target will also make the following type assertion fail, so this condition is unnecessary.

dfawley · 2019-07-23T19:51:51Z

status/status_ext_test.go

+	testErrWithDetails := statusWithDetails.Err()
+
+	// Test cases.
+	testCases := []testCaseErrorIs{


This type can be declared inline to avoid putting it in the global namespace:

testCases := []struct { err1, err2 error want bool }{ ...

Was necessary for the appends, but I fixed that!

dfawley · 2019-07-23T19:56:06Z

status/status_ext_test.go

+func TestErrorIs(t *testing.T) {
+	// Test errors.
+	testErr := status.Error(codes.Internal, "internal server error")
+	statusWithDetails, _ := status.New(codes.Internal, "internal server error").WithDetails(&grpc_testing.Empty{})


I'd recommend a helper:

func errWithDetails(t *testing.T, s *Status, details ...proto.Message) error { t.Helper() res, err := s.WithDetails(details) if err != nil { t.Fatalf("(%v).WithDetails(%v) = %v, %v; want _, <nil>", s, details, res, err) } return res } ... testErrWithDetails := errWithDetails(status.New(codes.Internal, "internal server error"), &grpc_testing.Empty{})

This will also make it so you don't need to append any test cases.

dfawley · 2019-07-23T21:11:59Z

status/status.go

+		return false
+	}
+
+	return se.Code == tse.Code && se.Message == tse.Message


Sorry, I have not had a chance to ask yet. I'll see if I can find something out soon.

dfawley

Please also add a grpc copyright notice to the top of status/status_ext_test.go - or merge it into status_test.go. (This is what is causing travis to fail.)

jsm · 2019-07-24T03:12:45Z

@dfawley Updated!

I need to keep status_ext_test.go separate, as I needed it to be outside of the status package (it's package is status_test) to use the grpc_testing package without cycles.

dfawley

Looks good, but please change the copyright year. Thanks!

dfawley · 2019-07-24T17:24:54Z

status/status_ext_test.go

+
+		is := isError.Is(tc.err2)
+		if is != tc.want {
+			t.Errorf("(%v).Is(%v) = %t; want %t", tc.err1, tc.err2, is, tc.want)


dfawley · 2019-07-24T17:24:56Z

status/status_ext_test.go

@@ -0,0 +1,72 @@
+/*
+ *
+ * Copyright 2017 gRPC authors.


2019 please.

In the go2 error proposal the Is function adds matching of error targets. On this line: https://github.com/golang/exp/blob/sumdb/v0.0.2/errors/wrap.go#L58 of the proposal implementation, there is an interface check for Is, to provide custom implementation of this matching. gRPC status errors that are defined as a var, currently don't successfully work with this function when crossing the gRPC boundary, as the pointer values will not be equivalent. Ideally, the following should work across gRPC boundaries: var ErrAuthInvalidToken = status.Error(codes.Unauthenticated, "Invalid token") _, err := grpcClient.Call(...) // Returns the above err.Is(ErrAuthInvalidToken) == true

jsm · 2019-07-24T22:23:42Z

🎉 Let me know if you'd like me to work with you on the As implementation.

dfawley · 2019-07-24T23:07:24Z

🎉 Let me know if you'd like me to work with you on the As implementation.

Thanks for the offer. I filed an issue for this and CC'd you on it.

Edit: #2934

### Release notes - https://github.com/grpc/grpc-go/releases/tag/v1.23.0 - much stuff! a bunch of HTTP2-related fixes -- we don't _expose_ go-grpc's listener, but we use it interally all over the place - there's `*satus.Is` now: grpc/grpc-go#2868 -- we could make some use of that, I suppose. - a bunch of perf improvements, always welcome :) - https://github.com/golang/protobuf/releases/tag/v1.3.2 - small fry, but nice stuff: golang/protobuf#785 adds an Unimplemented method to each server, allowing old servers to be used when there's proto changes. * deps: bump protobuf (1.3.2) and go-grpc (1.23.0) * deps: bump protobuf (1.3.2) and go-grpc (1.23.0) [revendor] * deps: bump protobuf (1.3.2) and go-grpc (1.23.0) [regenerate] Signed-off-by: Stephan Renatus <[email protected]>

dfawley self-requested a review June 20, 2019 20:22

dfawley self-assigned this Jun 20, 2019

dfawley reviewed Jun 21, 2019

View reviewed changes

dfawley assigned jsm and unassigned dfawley Jun 21, 2019

dfawley assigned dfawley and unassigned jsm Jul 11, 2019

dfawley reviewed Jul 23, 2019

View reviewed changes

dfawley requested changes Jul 23, 2019

View reviewed changes

dfawley reviewed Jul 24, 2019

View reviewed changes

dfawley approved these changes Jul 24, 2019

View reviewed changes

dfawley added the Type: Feature New features or improvements in behavior label Jul 24, 2019

dfawley added this to the 1.23 Release milestone Jul 24, 2019

dfawley merged commit 61f27c1 into grpc:master Jul 24, 2019

srenatus mentioned this pull request Aug 14, 2019

deps: bump go-grpc (1.23.0) and protobuf (1.3.2) chef/automate#1237

Merged

lock bot locked as resolved and limited conversation to collaborators Feb 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

status: Implement *statusError.Is #2868

status: Implement *statusError.Is #2868

jsm commented Jun 13, 2019 •

edited

Loading

thelinuxfoundation commented Jun 13, 2019

dfawley left a comment

dfawley Jun 21, 2019

jsm Jun 21, 2019

dfawley Jun 24, 2019

jsm Jun 24, 2019

jsm Jun 26, 2019

dfawley Jun 28, 2019

jsm Jul 9, 2019

dfawley Jul 23, 2019

ekle Aug 5, 2019

dfawley Aug 12, 2019

jsm commented Jun 21, 2019

jsm commented Jun 26, 2019

jsm commented Jul 9, 2019

jsm commented Jul 23, 2019

dfawley Jul 23, 2019

jsm Jul 24, 2019

dfawley Jul 23, 2019

jsm Jul 24, 2019

dfawley Jul 23, 2019 •

edited

Loading

jsm Jul 24, 2019

dfawley Jul 23, 2019

dfawley left a comment

jsm commented Jul 24, 2019

dfawley left a comment

dfawley Jul 24, 2019

dfawley Jul 24, 2019

jsm Jul 24, 2019

jsm commented Jul 24, 2019

dfawley commented Jul 24, 2019 •

edited

Loading

status: Implement *statusError.Is #2868

status: Implement *statusError.Is #2868

Conversation

jsm commented Jun 13, 2019 • edited Loading

thelinuxfoundation commented Jun 13, 2019

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsm commented Jun 21, 2019

jsm commented Jun 26, 2019

jsm commented Jul 9, 2019

jsm commented Jul 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley Jul 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

jsm commented Jul 24, 2019

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsm commented Jul 24, 2019

dfawley commented Jul 24, 2019 • edited Loading

jsm commented Jun 13, 2019 •

edited

Loading

dfawley Jul 23, 2019 •

edited

Loading

dfawley commented Jul 24, 2019 •

edited

Loading