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: Allow external packages to produce status-compatible errors #1927

Merged
merged 1 commit into from Mar 26, 2018

Conversation

@johanbrandhorst
Copy link
Contributor

@johanbrandhorst johanbrandhorst commented Mar 18, 2018

Embues the status package with the ability to create statuses
from generic errors that implement a special interface.

This was designed with the github.com/gogo/protobuf project in mind,
but is implemented in a fashion that makes it accessible to arbitrary
payloads.

Fixes #1885.

@thelinuxfoundation
Copy link

@thelinuxfoundation thelinuxfoundation commented Mar 18, 2018

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.

Regards,
CLA GitHub bot

@johanbrandhorst
Copy link
Contributor Author

@johanbrandhorst johanbrandhorst commented Mar 18, 2018

I've signed the CLA

@dfawley
Copy link
Contributor

@dfawley dfawley commented Mar 22, 2018

Thanks for the PR.

Instead of this approach, what if WithDetails() does a type assertion, and if the parameter is already an any.Any, it appends it directly. Would that satisfy all the same use cases?

@johanbrandhorst
Copy link
Contributor Author

@johanbrandhorst johanbrandhorst commented Mar 23, 2018

@dfawley That's certainly a middle ground, and better than today, but unfortunately it's still not a very ergonomic experience for users of gogoproto registered types. I think it would look something like this:

package server

import (
	"context"

	"google.golang.org/grpc/codes"
	"google.golang.org/grpc/status"
	spb "google.golang.org/genproto/googleapis/rpc/status"
	"github.com/gogo/googleapis/google/rpc"
	"github.com/golang/protobuf/ptypes/any"
	"github.com/gogo/protobuf/proto"
)

func GetError(ctx context.Context, req *rpc.DebugInfo) (*rpc.DebugInfo, error) {
	st := status.New(codes.InvalidArgument, "some error")
	br := &rpc.BadRequest{
		FieldViolations: []*rpc.BadRequest_FieldViolation{
			{
				Field:       "Some field",
				Description: "Some error description",
			},
		},
	}
	byts, err := proto.Marshal(br)
	if err != nil {
		return nil, err
	}
	stWithDetails, err := st.WithDetails(&any.Any{
		TypeUrl: "type.googleapis.com/" + proto.MessageName(br)
		Value: byts
	})
	if err != nil {
		return nil, st.Err()
	}
	return nil, stWithDetails.Err()
}

This isn't all that much better than just constructing the spb.Status ourselves and using status.FromProto as you suggested in your reply to #1885. The primary problems with this proposal is:

  1. The user has to deal with a lot of boilerplate that status.WithDetails can hide.
  2. The user has to import both gogo/protobuf and golang/protobuf and types from both generated googleapis repos. This goes against the desire of the GoGo maintainers to allow users to use one or the other.

The approach taken in this PR would allow something like this instead (using https://github.com/gogo/status in place of the gRPC status package):

package server

import (
	"context"

	"github.com/gogo/googleapis/google/rpc"
	"github.com/gogo/status"
	"google.golang.org/grpc/codes"
)

func GetError(ctx context.Context, req *rpc.DebugInfo) (*rpc.DebugInfo, error) {
	st := status.New(codes.InvalidArgument, "some error")
	stWithDetails, err := st.WithDetails(&rpc.BadRequest{
		FieldViolations: []*rpc.BadRequest_FieldViolation{
			{
				Field:       "Some field",
				Description: "Some error description",
			},
		},
	})
	if err != nil {
		return nil, st.Err()
	}
	return nil, stWithDetails.Err()
}

Because gogo/status.Status.Err() implements the interface exposed in this PR, the gRPC runtime can transparently convert the gogo/status produced errors into grpc/status types. On the other side, because gogo/status can convert grpc/status error types, the user can stay entirely within the GoGo ecosystem.

I hope that further clarifies the intent. Please see my blog post on errors, in particular the problems associated with using GoGo protobuf with the gRPC status package for more information: https://jbrandhorst.com/post/grpc-errors/.

@dfawley
Copy link
Contributor

@dfawley dfawley commented Mar 23, 2018

@johanbrandhorst Thank you for the code samples. However, the second sample doesn't really look right for what is proposed in this PR. You should show FromError being used, not WithDetails. And please consider the definition of the custom error type itself as well, which must include Error, GetCode, GetMessage, and GetDetails methods, with an implementation of the type returned by GetDetails having GetTypeURL and GetValue methods. This part seems extremely complicated to me. You'd be better off IMO doing example 1 (or using FromProto), but putting the reusable parts of the logic into a separate function.

Also, what is the concern with using FromProto?

@johanbrandhorst
Copy link
Contributor Author

@johanbrandhorst johanbrandhorst commented Mar 23, 2018

I think you misunderstood the point of my reply. I was showing the implications of this PR versus the implications of your suggestion, for gogoproto users, when creating a status error. The FromError case is used when parsing an error from a gRPC call, which I was not trying to show.

I pointed you to https://github.com/gogo/status which indeed implements the proposed interface:
https://github.com/gogo/status/blob/b2af61acbd13657c2e3f375c04281e38d7c941f3/status.go#L43.

I further direct you to the implementation of gogo/status.FromError to give you the full picture:
https://github.com/gogo/status/blob/b2af61acbd13657c2e3f375c04281e38d7c941f3/status.go#L153.

Indeed, just to be explicit, the point of this PR is to allow gogo/protobuf users to work effortlessly within the gogo/protobuf ecosystem which they are forced to by the design of the golang/protobuf project.

The interface is created so that we can return gogo/status.statusError from a users function and have it correctly parsed by the gRPC runtime. On the other side of the gRPC transport, a user will again be able to use gogo/status.FromError to translate from a grpc/status to a gogo/status.

@dfawley
Copy link
Contributor

@dfawley dfawley commented Mar 23, 2018

OK, so you want this so applications can return a gogo/status.Status.Err() from RPC handlers, and then we can convert it into a grpc/status.Status from within gRPC. Correct?

I would prefer to do this slightly differently:

  1. Export the existing status() *Status from grpc/status.statusError (as Status())
  2. Change grpc/status.FromError to check for a Status() *Status method and call that, instead of type asserting to *statusError.
  3. Implement Status() *grpc/status.Status in gogo/status.

This was actually part of an earlier implementation, but I made it more restrictive, because this use case was not apparent at the time.

I think this would have all the same benefits for you, but it would keep things simpler and easier to understand in our API. What do you think?

@johanbrandhorst
Copy link
Contributor Author

@johanbrandhorst johanbrandhorst commented Mar 23, 2018

@dfawley that sounds promising, I'll give it a go and update the PR when I have a moment. Thanks!

@johanbrandhorst johanbrandhorst force-pushed the johanbrandhorst:patch-1 branch from 6b7bae6 to 106d5c0 Mar 24, 2018
@johanbrandhorst
Copy link
Contributor Author

@johanbrandhorst johanbrandhorst commented Mar 24, 2018

I've given this some testing, and it works just as well as the previously suggested code, so I've updated the PR with the simpler interface.

Copy link
Contributor

@dfawley dfawley left a comment

Looks good, thanks! Just one request to change the docstring to advertise this new functionality.

@@ -126,8 +126,8 @@ func FromError(err error) (s *Status, ok bool) {
if err == nil {
return &Status{s: &spb.Status{Code: int32(codes.OK)}}, true
}
if se, ok := err.(*statusError); ok {
return se.status(), true
if se, ok := err.(interface{ Status() *Status }); ok {

This comment has been minimized.

@dfawley

dfawley Mar 26, 2018
Contributor

Could you change the docstring to say "if it was produced from this package or has a method Status() *Status"

This comment has been minimized.

@johanbrandhorst

johanbrandhorst Mar 26, 2018
Author Contributor

Done

Embues the status package with the ability to create statuses
from generic errors that implement the interface:

type StatusError interface {
    Status() *Status
}

This was designed with the github.com/gogo/protobuf project in mind,
but is implemented in a fashion that makes it accessible to arbitrary
payloads.

Fixes #1885.
@johanbrandhorst johanbrandhorst force-pushed the johanbrandhorst:patch-1 branch from 106d5c0 to 64d5527 Mar 26, 2018
Copy link
Contributor

@dfawley dfawley left a comment

Thank you again for the contribution!

@johanbrandhorst
Copy link
Contributor Author

@johanbrandhorst johanbrandhorst commented Mar 26, 2018

🎉 thanks a lot! Excited to see this go in :).

@dfawley dfawley merged commit 2756956 into grpc:master Mar 26, 2018
2 checks passed
2 checks passed
cla/linuxfoundation johanbrandhorst authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dfawley dfawley changed the title Allow statuses from compatible errors status: Allow external packages to produce status-compatible errors Mar 26, 2018
@menghanl menghanl added this to the 1.11 Release milestone Mar 29, 2018
@johanbrandhorst johanbrandhorst deleted the johanbrandhorst:patch-1 branch Aug 10, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Feb 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants