Generated spec missing grpc errors

[seanlaff]

The gateway has an error object which any rpc can return- however it doesn't end up in the generated swagger/openapi.

grpc-gateway/runtime/errors.go

Lines 68 to 76 in 2da5bfa

	type errorBody struct {
	Error string `protobuf:"bytes,100,name=error" json:"error"`
	// This is to make the error more compatible with users that expect errors to be Status objects:
	// https://github.com/grpc/grpc/blob/master/src/proto/grpc/status/status.proto
	// It should be the exact same message as the Error field.
	Code int32 `protobuf:"varint,1,name=code" json:"code"`
	Message string `protobuf:"bytes,2,name=message" json:"message"`
	Details []*any.Any `protobuf:"bytes,3,rep,name=details" json:"details,omitempty"`
	}

OpenAPI's pattern for something like this is to include it as the default case.
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#responses-object

The default MAY be used as a default response object for all HTTP codes that are not covered individually by the specification.

Can we do the following?

 {
   "/api/v1/rbac/users": {
     "get": {
       "summary": "GetUsers",
       "description": "Get the users' information",
       "operationId": "GetUsers",
       "responses": {
         "200": {
           "description": "A successful response.",
           "content": {
             "application/json": {
               "schema": {
                 "$ref": "#/components/schemas/rbacUsers"
               }
             }
           }
+        },
+        "default": {
+          "description": "Error",
+          "content": {
+            "application/json": {
+              "schema": {
+                "$ref": "#/components/schemas/grpcError"
+              }
+            }
+          }
         }
       },
       "tags": [
         "sevone.rbac.RBAC"
       ]
     }
   }
 }

With an entry in schemas for the new type:

{
  "grpcError": {
    "description": "Error",
    "properties": {
      "error": {
        "type": "string"
      },
      "code": {
        "type": "integer",
        "format": "int32"
      },
      "message": {
        "type": "string"
      },
      "details": {
        "items": {
          "$ref": "#/components/schemas/protobufAny"
        },
        "type": "array"
      }
    }
  }
}

I wrote some jq to handle this in the meantime:

def addGRPCErrorType:
    .components.schemas["grpcError"] = {"description":"gRPC Error","properties":{"error":{"type":"string"},"code":{"type":"integer","format":"int32"},"message":{"type":"string"},"details":{"items":{"$ref":"#/components/schemas/protobufAny"},"type":"array"}}}
;

def addGRPCErrorDefaultResponse:
    .paths[][].responses.default = {"description":"Error","content":{"application/json":{"schema":{"$ref":"#/components/schemas/grpcError"}}}}
;

def addGRPCErrors:
    addGRPCErrorType | addGRPCErrorDefaultResponse
;

Not sure all that's required to make this change. I think at the least the errorBody struct would need to be made public. Thoughts?

[seanlaff]

If we wanted to be a little more specific and bound the error type to the 400/500 range, maybe we can use the status code range syntax that the spec defines, i.e 5XX, although I think thats a little less common?

[johanbrandhorst]

Hi Sean!

To be pedantic, we only support swagger 2, but happily the default field of the responses object is a thing in 2.0 as well, so no worries!

This makes great sense to me, and shouldn't break any existing users. I'd be happy to review this if you have spare cycles to contribute this. I'm not sure we need to export the type, unless you mean just to the swagger generator? We could still solve that with an internal package, which I'd prefer to adding to the API surface.

If we wanted to be a little more specific and bound the error type to the 400/500 range, maybe we can use the status code range syntax that the spec defines, i.e 5XX, although I think thats a little less common?

I think we can keep it as the default, we already allow users to specify custom responses, so really anything that isn't defined should be an error.

[jgiles]

@johanbrandhorst I think that adding this default to all responses without a config option is a bit aggressive - for example, we have a custom error handler that translates an internal error details message into an RFC7807 https://tools.ietf.org/html/rfc7807 response for our REST clients (and so this default misrepresents the error format we return to clients)

[johanbrandhorst]

Argh, I did know this would be incorrect for those with custom error handlers, but I figured it was better than nothing. I agree it should be possible to turn this off... I'll add something.

[johanbrandhorst]

#1141