grpc-ecosystem / grpc-gateway

gRPC to JSON proxy generator following the gRPC HTTP spec
https://grpc-ecosystem.github.io/grpc-gateway/
BSD 3-Clause "New" or "Revised" License
18.16k stars 2.23k forks source link

Adding flag not to render 200 response in openapi #2243

Closed 0daryo closed 3 years ago

0daryo commented 3 years ago

🚀 Feature

Hi team.

I found that I can control http status like 201 or 204 etc. https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_your_gateway/#controlling-http-response-status-codes But protoc-gen-openapiv2 render always 200 response in openapiv2 file.

I always delete auto-generated 200 response by jq command with this case.

How about adding parameter no200Response flag to Operation message in openapiv2.proto. And if true, generator does not generate default 200 response. https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-openapiv2/options/openapiv2.proto#L129

johanbrandhorst commented 3 years ago

Hi, thanks for your issue! This feels like a rare enough request that I'm not sure we want to add the extra code for it, in particular since you've already got a workaround for it. More interesting would be some way to define the status code, so that a user could set it to be 201 or 204 in their annotations. What do you think?

0daryo commented 3 years ago

Thank you for replyng!

This feels like a rare enough request that I'm not sure we want to add the extra code for it

I understand your idea.

More interesting would be some way to define the status code

you mean something like this? if success_status_code is not zero value, then protoc-gen-openapiv2 render 201 as default response status code?

rpc SayHello (HelloRequest) returns (HelloReply) {
    option (google.api.http) = {
      post: "/v1/example/echo"
      body: "*"
    };
    option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_operation) = {
      success_status_code: "201" # I added this time
    };
  }

if so, my concern is that if status_code is conflict with responses.key like this. I think if both success_status_code and responses.key are the same value, just returning error is enough. or overriding.

option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_operation) = {
      success_status_code: "201" # I added this time
      responses: {
        key: "201"
        value: {
          description: "A successful response."
          schema: {
            json_schema: {
              ref: ".mypackage.HelloReply"
            }
          }
        }
      }
    };

What do you think?(It may be a bit too far though...)

johanbrandhorst commented 3 years ago

I don't think we want to add a new status code option, as you say it will just add confusion to users. Are you saying that you can change the return type in the gateway, and you can add a custom response, but you don't want the 200 response to also be added? If so, I can see how that is annoying, but I think for now it's OK for this to be a manual post-processing step. Maybe you could share the jq command you are using so that future users can use it too?

0daryo commented 3 years ago

If so, I can see how that is annoying, but I think for now it's OK for this to be a manual post-processing step.

Yes. That is my case. But I understand your idea!

My workaround is like this.

Proto file with 201 response.

rpc Login (LoginRequest) returns (LoginReply) {
    option (google.api.http) = {
        get: "/v1/example/login"
    };
    option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_operation) = {
      responses: {
        key: "201"
        value: {
          description: "A successful 201 response."
        }
      }
    };
  }

After generated, remove 200 response from doc because its confusing for client .

cat alive.swagger.json | jq del\(.paths.'"/v1/example/login"'.get.responses.'"200"'\)
mixedCase commented 2 years ago

@johanbrandhorst Could this decision be re-evaluated? Documenting APIs that regularly stray away from 200 using grpc-gateway is turning into a lot of boilerplate and swagger post-processing. If there was an option that not only affected the swagger docs, but also allowed setting the status code for the response without the need for custom middleware, this would be a major grpc-gateway QoL improvement.

johanbrandhorst commented 2 years ago

The grpc-gateway is trying to produce a HTTP reverse proxy for gRPC following the http.proto spec. The spec does not allow you to specify return codes anywhere, so the only reasonable mapping is found by mapping the gRPC status code in any returned error to its HTTP counterpart. This is the automatic behavior in the gRPC gateway. It's simple to understand and implement.

Beyond the default behavior, we allow users to create their own forward response handlers, to do arbitrary manipulation of responses. I do not think we need anything more specialized than that, and I worry that introducing more configuration options like this (especially straddling both the gateway and the openapi generator) is going to make things a lot more complicated both to maintain and to explain.

mixedCase commented 2 years ago

Thank you for the detailed answer @johanbrandhorst ! Even if getting a job at Google and earning enough goodwill to get a feature merged into a company-wide spec is slightly harder than I hoped for.

While I work on that, if anyone's coming here from search engines like I was, I'm leaving a jq one-liner that in combination with the forward response handler for changing status codes described in the docs (https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_your_gateway/#controlling-http-response-status-codes) will help in returning and documenting the right status codes for your APIs.

This will remove every status 200 response in every operation where you have declared any other successful (2xx) response in your swagger.json:

jq '.paths |= map_values(. |= map_values(.responses |= if (with_entries(select((.key|startswith("2")) and .key != "200"))|length > 0) and ."200".description == "A successful response." then del(."200") else . end))' your_swagger_file.json

It will only delete status 200 responses that are using the default "A successful response." description, so that if you do need multiple successful responses that include a 200, all you need to do is to manually declare it and set a different description. Since these definitions are merged with the default JSON for the 200 response, you don't even need to set the schema like you would with other status codes. Example:

rpc ExampleRPC(ExampleRPCRequest) returns (ExampleRPCResponse) {
  option (google.api.http) = {
    post : "/v1/example"
    body : "*"
  };
  option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_operation) = {
    responses : {
      key : "200"
      value : {description : "This is a 200 response that won't be deleted"}
    }
    responses : {
      key : "203"
      value : {
        description : "This is a successful, but non-default response"
        schema : {json_schema : {ref : ".myorg.mypackage.ExampleRPCResponse"}}
      }
    }
  };
}

Cheers