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

status: handle wrapped errors #6031

Merged
merged 1 commit into from Mar 17, 2023
Merged

status: handle wrapped errors #6031

merged 1 commit into from Mar 17, 2023

Conversation

psyhatter
Copy link
Contributor

RELEASE NOTES:

  • status: status.Code and status.FromError handle wrapped errors

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 17, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: psyhatter / name: Rusakov Andrei (6cc3ab3)

@zasweq zasweq requested a review from easwars February 21, 2023 18:50
@psyhatter
Copy link
Contributor Author

Hello, @easwars. Please take a look at PR. It seems like a small one =)

@easwars easwars added the Type: Behavior Change Behavior changes not categorized as bugs label Feb 28, 2023
@easwars easwars added this to the 1.54 Release milestone Feb 28, 2023
@easwars
Copy link
Contributor

easwars commented Feb 28, 2023

@psyhatter : Just wondering the use case behind the change? I agree that it would be nice for the status package to handle wrapped errors.

@dfawley : As I understand, this would be a behavior change. I vaguely remember issues/PRs/questions about switching the status package to support wrapped errors in the past, but I don't quite remember the outcome of those discussions. I feel that this would a good change. If we go with this, we would need to update the docstrings for these functions and also make a bigger noise in the release notes. But I also wanted to check with your thoughts on this before proceeding much further with the review.

Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

I'm okay with this particular aspect of error wrapping. IIRC we didn't do this earlier because errors.As was too new for us to use given our Go verison support policy.

But we should take a look over the old related issues/PRs and make sure we aren't forgetting anything that we decided previously.

status/status.go Show resolved Hide resolved
@dfawley dfawley removed their assignment Mar 2, 2023
@psyhatter
Copy link
Contributor Author

psyhatter commented Mar 4, 2023

@psyhatter : Just wondering the use case behind the change?

Suppose we have a sentinel error:

package model

import (
	"google.golang.org/grpc/codes"
	"google.golang.org/grpc/status"
)

var ErrUserNotFound = status.Errorf(codes.NotFound, "user not found")

At the repository level, we return this sentinel error so that it can be handled at the service level with errors.Is(err, model.ErrUserNotFound). But sometimes handling is not required, but it is required to wrap the received error in order to add context to it:

sender, err := s.repo.GetUser(ctx, senderID)
if err != nil {
	return fmt.Errorf("getting sender: %w", err)
}

receiver, err := s.repo.GetUser(ctx, receiverID)
if err != nil {
	return fmt.Errorf("getting receiver: %w", err)
}

But in this case, the grpc server will return an incorrect error code, because it uses this function, which does not know how to parse wrapped errors

Probably, sentinel errors used at the service level should not implement transporter level errors, and there should be some kind of transformation. But this is a fairly convenient way when there is only one transport layer

@biryukovmaxim
Copy link

@easwars what this pr waiting for?

@easwars
Copy link
Contributor

easwars commented Mar 6, 2023

@easwars what this pr waiting for?

Nothing. Why do you ask?
We are having a discussion.

status/status.go Show resolved Hide resolved
status/status.go Show resolved Hide resolved
@easwars easwars assigned dfawley and unassigned easwars and psyhatter Mar 14, 2023
@easwars
Copy link
Contributor

easwars commented Mar 14, 2023

Moving to @dfawley for second set of eyes.

Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM, except for 2 small nits.

status/status.go Outdated
Comment on lines 80 to 81
// *Status`, the appropriate Status is returned. Function takes into account if
// such errors have been wrapped.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this is not a complete sentence, and doesn't explain clearly (to me, anyway) what's happening. Maybe:

//   - If err was produced by this package or implements the method `GRPCStatus()
//     *Status`, or if err wraps a type satisfying this, the appropriate Status is returned.

@@ -144,6 +145,37 @@ func (s) TestFromErrorOK(t *testing.T) {
}
}

func (s) TestFromErrorWrapped(t *testing.T) {
code, message := codes.Internal, "test description"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

const code, message = codes.Internal, "test description"

(to avoid these being mutable variables when they should be invariants.)

And the other test cases, below.

@dfawley dfawley assigned psyhatter and unassigned dfawley Mar 14, 2023
@dfawley dfawley added Type: Feature and removed Type: Behavior Change Behavior changes not categorized as bugs labels Mar 17, 2023
@dfawley dfawley merged commit 66e3533 into grpc:master Mar 17, 2023
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants