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

Add support for the richer error model for gRPC ("google.rpc.status") #303

Closed
shirhatti opened this issue Jun 7, 2019 · 25 comments
Closed
Labels
enhancement New feature or request
Milestone

Comments

@shirhatti
Copy link
Contributor

See https://grpc.io/docs/guides/error/

https://github.com/googleapis/googleapis/tree/master/google/rpc

@JunTaoLuo JunTaoLuo added the enhancement New feature or request label Jun 10, 2019
@JunTaoLuo JunTaoLuo added this to the Backlog milestone Jun 10, 2019
@jtattermusch jtattermusch changed the title Add support for the richer error model for gRPC Add support for the richer error model for gRPC ("google.rpc.status") Jun 11, 2019
@jtattermusch
Copy link
Contributor

One challenge is that e.g. Grpc.AspNetCore.Server doesn't have dependency on Google.Protobuf, which is needed to be able to add the status.proto message, so this support probably needs to come as a separate nuget package (we should start thinking about creating the metapackage, because we now have more nugets and we'll probably add some more).

@ThatRendle
Copy link

Glad to see there's already an issue open on this.

I wonder if it would be possible to provide an extended RpcException<T> with a detail object, similar to WCF's FaultException<T>, which has a Details property of T, where T is a Protobuf type.

Would probably look like this:

using pb = global::Google.Protobuf;

public class RpcException<T> where T : pb:IMessage<T>
{
    public T Details { get; }
}

And maybe the client could add extra code to look for the Status.details property and convert it to the generic RpcException<T>? Guessing that would be in the Core library rather than this one?

@JamesNK
Copy link
Member

JamesNK commented Aug 2, 2019

If it was added then it should probably be added to grpc/grpc. The type constraint couldn't be IMessage<T>. There is no dependency on Protobuf in the libraries.

Could you link to an article showing how it is useful?

@jtattermusch
Copy link
Contributor

I'll try to look into providing a nuget package with google.rpc.status support (which would be usable by both Grpc.Core and grpc-dotnet), but can't promise any ETA.

@Ciantic
Copy link

Ciantic commented Oct 4, 2019

Especially the BadRequest with field violations is must. Otherwise .NET people are bound to reinvent the wheel for basic validation e.g. "Username has wrong character" "You must be at least 21 years old".

https://grpc.io/docs/guides/error/

This richer error model is already supported in the C++, Go, Java, Python, and Ruby libraries, and at least the grpc-web and Node.js libraries have open issues requesting it. Other language libraries may add support in the future if there’s demand, so check their github repos if interested.

Example how it's used in Go lang ( https://jbrandhorst.com/post/grpc-errors/ )

st := status.New(codes.InvalidArgument, "One or more fields has errors")
v := &errdetails.BadRequest_FieldViolation{
    Field: "username",
    Description: "The username must only contain alphanumeric characters",
}
br := &errdetails.BadRequest{}
br.FieldViolations = append(br.FieldViolations, v)
st, err := st.WithDetails(br)
...

(Code snippet editted from above example page)

In my opinion, this is bare minimum all API should support, standard way to convey which fields has incorrect input.

@Ciantic
Copy link

Ciantic commented Oct 4, 2019

@jtattermusch if you are working on this as a separate package I have a suggestion for API:

In server side, for each standard error message have a new exception e.g. with BadRequestException which could inherit from RpcException:

public override Task<HelloReply> SayHello(HelloRequest request, ServerCallContext context)
{

    var error = new BadRequestException()
    if (usernameIsNotValid(username)) {
        error.AddFieldViolation("username", "Username has wrong chars")
    }

    if (ageIsnotValid(age)) {
        error.AddFieldViolation("age", "You are too young to view this");
    }

    if (error.HasFieldViolations()) {
        throw error;
    }

    return Task.FromResult(new MyReply
    {
    });
}

On Client side you could just catch it:

    try {
       var reply = await client.SayHelloAsync(new HelloRequest { Name = "GreeterClient" });
    }
    catch (BadRequestException ex) {
         ex.GetFieldViolations(); 
         // ...
    }

@Ciantic
Copy link

Ciantic commented Oct 5, 2019

I found example how it's used in the Java side avinassh/grpc-errors#10

        Status s = com.google.rpc.Status.newBuilder()
                .setCode(com.google.rpc.Code.INVALID_ARGUMENT.getNumber())
                .setMessage("Invalid age")
                .addDetails(Any.pack(BadRequest.newBuilder()
                                .addFieldViolations(BadRequest.FieldViolation
                                        .newBuilder()
                                        .setField("age")
                                        .setDescription("age must be positive ")
                                        .build()
                                ).build()
                        )
                ).build();

        throw io.grpc.protobuf.StatusProto.toStatusRuntimeException(s);

Needless to say, that's a bit complicated, it could be simplified if it's done by separate package.

For example it's pointless to require build Invalid Argument, when the spec says it should always be invalid argument with BadRequest. In otherwords the package could abstract that detail away.

@bastianeicher
Copy link

I've published a small NuGet package with extension methods that that add rudimentary support for the richer gRPC error model:
https://github.com/nano-byte/grpc-rich-error
https://www.nuget.org/packages/GrpcRichError/

@JimHume
Copy link

JimHume commented Oct 13, 2020

What is the best pattern to use if I want to have properties of type google.rpc.* within my own proto messages?

This is somewhat outside the scope of the original issue, but I can't find a way to properly consume / use those protos and this is the only (even slightly) relevant issue I could find.

The Google.Protobuf.Tools package has the very basic protobuf namespace and files (./tools/google/protobuf), and I was hoping something similar existed for the rpc namespace seeing as the Grpc.Core namespace has its own definition of those exact same objects ('Grpc.Core.StatusCode', 'Grpc.Core.Status', both of which are used in 'RpcException').

Is there no such thing / no way to consume the google/rpc/ (or any namespace within the google api's project) within proto for use in a C# solution?

@chwarr
Copy link

chwarr commented Oct 15, 2020

@JimHume: Google publishes the NuGet package Google.Api.CommonProtos with a C# implementation of the google.rpc.Status and others like google.rpc.BadRequest message.

I have a fully worked example of the grpc-status-details-bin convention that uses this package in the https://github.com/chwarr/grpc-dotnet-google-rpc-status repository.

If this package doesn't work, the .proto files for these messages live at https://github.com/googleapis/googleapis/tree/master/google/rpc. You could, for example, add this repository as a submodule of your own repository and consume the .proto files in your own projects.

@JimHume
Copy link

JimHume commented Oct 15, 2020

Ah, very interesting--I hadn't considered the submodule idea.

The proto's are what I'm truly after but knowing that there are nuget's is very helpful. I didn't realize that the CommonProtos nuget was a thing despite searching for all things 'google' and 'api'.

This is great, thanks @chwarr .

@kwaclaw
Copy link

kwaclaw commented Feb 25, 2021

So, is using Google.Api.CommonProtos now the canonical way of using the rich error model?

@chwarr
Copy link

chwarr commented Mar 3, 2021

@kwaclaw: for APIs that you are designing, you need to decide how you want to model errors. The grpc-status-details-bin metadata is a common way to do this, but you need to make a decision yourself about how you want to model errors.

For APIs that you are consuming, you need to check each one to see how it models errors.

The Google.Api.CommonProtos package only includes the ProtoBuf messages that are used in the "rich error model". The grpc-status-details-bin model is an unspecified convention in gRPC--though many APIs, especially those published by Google, use it. (Contrast this with something like SOAP faults, which can be defined in a machine-readable way in the definition of the service.) Additional code, like that written by @bastianeicher or me is needed to produce/consume rich errors in gRPC.NET.

One thing to keep in mind: gRPC is agnostic of serialization format and interface definition language. It is very commonly used with Protocol Buffers. This presents a problem for specifying grpc-status-details-bin: if it is specified as containing Protocol Buffer serialized data, it will have to be optional, as Protocol Buffers cannot be required to use gRPC. On the other hand, if the format isn't specified, it is of limited use for programmatic dispatch. My guess: grpc-status-details-bin will remain a community convention with helper libraries in the various gRPC implementations, but it will never become part of the core gRPC spec.

Hope this helps!

(Note: While I work at Microsoft, I don't work on the gRPC.NET project directly. I'm basing this on my experience writing and using gRPC APIs in various other projects.)

@thesobercoder
Copy link

This is such an important feature to have. I hope it gets implemented soon.

@ziaulhasanhamim
Copy link

This feature is in serious need. There is not a good way to properly return an error in a language-neutral way in grpc-dotnet. Almost all major grpc implementations support the richer error model. so why not .net and c#?

@xxnickles
Copy link

I am very interested understanding why this feature is not part of the core. Is not that popular/needed among GRPC users? How are devs dealing with errors and validation messages? I was looking to use GRPC but the lack of clarity on how to deal with errors is having me on the fence

@ziaulhasanhamim
Copy link

@xxnickles Currently I use response trailers for sending error messages.

@kwaclaw
Copy link

kwaclaw commented Jun 4, 2022

I am very interested understanding why this feature is not part of the core. Is not that popular/needed among GRPC users? How are devs dealing with errors and validation messages? I was looking to use GRPC but the lack of clarity on how to deal with errors is having me on the fence

I think @chwarr explained that already.

@irperez
Copy link

irperez commented Aug 30, 2022

Just a thought. But won't throwing exceptions cause issues in general? Slowness as well as monitoring noise? Especially in scenarios where a bad request is returned. That is not truly an exception.

I'm new to gRPC, but in the REST API world, we strayed away from exceptions unless they were truly 500 errors. This sounds like we are throwing exceptions for all scenarios.

Is there any guidance for this?

@ziaulhasanhamim
Copy link

@irperez I had the same problem. It doesn't make sense to throw exception when there is invalid request data. Personally I set the context.Status property for returning errors. It is at least better than throwing RpcException.

@irperez
Copy link

irperez commented Aug 30, 2022

In the REST APIs the beauty of IActionResult was the ability to add these types of errors in the payload itself. Not as an exception. Its like we're missing that here.

https://docs.microsoft.com/en-us/aspnet/core/web-api/action-return-types?view=aspnetcore-6.0#actionresultt-type

High load servers and high performance applications can't use this model of exceptions for all error types.

Every server response had an HTTP code. I know gRPC is different, but my point is that not all types of errors are exceptions. And designing a gRPC to throw exceptions for all types of errors, including business errors/validation(bad request) is going to bring alot of pain.

And I'm surprised to see that there is very little guidance around this topic. I know that ResponseTrailors is a thing, but boy that is really lacking and up to interpretation.

@JamesNK any thoughts?

@irperez
Copy link

irperez commented Aug 30, 2022

Google's docs indicate its a language design choice.

I really miss the return BadRequest(errorObj) or return NotFound(messageObj) of the IActionResult interface here.

And I noticed that C++ and Python not throwing exceptions.

@vadimkhm
Copy link

We are still waiting for this essential part of the application flow. The main reason to have a correct richer model for errors is to have much easy handling, parsing, and compatibility with tools from the testing perspective. Please add that support or allow us to build Status responses directly without throwing RpcException. Thanks!

@tonydnewell
Copy link
Contributor

I've created a PR with a proposed API and a new NuGet package to support the richer error model - still under discussion.
#2205

@tonydnewell
Copy link
Contributor

FYI #2205 has been merged into master

@JamesNK JamesNK modified the milestones: 8.0, 9.0 Jan 10, 2024
@JamesNK JamesNK closed this as completed Jun 5, 2024
@github-project-automation github-project-automation bot moved this to Done in .NET gRPC Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests