[protoc-gen-openapiv2] [BUG] Incorrect handling of non-wildcard google.api.http.body when using field_behaviour annotation

[gganley]

🐛 Bug Report

With the introduction of #1806 rpc definitions definitions that have a non-wildcard body property now fail validation. The generated code breaks because it produces a swagger document whose body parameter is a reference to the request definition but also has a required entry in the object.

Note that this file swagger file failed npx @opernapitools/openapi-generator-cli generate with error saying that required is unexpected.

To Reproduce

Proto File (./genproto/test/v1alpha/test.proto)

syntax = "proto3";

package example.test.v1alpha;

import "google/api/annotations.proto";
import "google/api/client.proto";
import "google/api/field_behavior.proto";
import "google/api/resource.proto";
import "google/protobuf/field_mask.proto";
import "protoc-gen-openapiv2/options/annotations.proto";

option go_package = "gitlab.com/example/example/genproto/test/v1alpha;test";
option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger) = {
  host: "api.example.com"
  schemes: HTTPS,
};

service Test {
  rpc UpdateFoo(UpdateFooRequest) returns (Foo) {
    option (google.api.http) = {
      patch: "/v1alpha/foo/{foo.id}",
      body: "foo"
    };
    option (google.api.method_signature) = "foo,update_mask";
  }
}

message UpdateFooRequest {
  Foo foo = 1 [(google.api.field_behavior) = REQUIRED];

  google.protobuf.FieldMask update_mask = 2;
}

// Foo test.
message Foo {
  option (google.api.resource) = {
    type: "api.example.com/Foo"
    pattern: "foo/{id}"
  };

  int64 id = 1;
}

Run

 protoc -I. --openapiv2_out . --openapiv2_opt logtostderr=true ./genproto/test/v1alpha/test.proto

Expected behavior

{
  "swagger": "2.0",
  "info": {
    "title": "genproto/test/v1alpha/test.proto",
    "version": "version not set"
  },
  "tags": [
    {
      "name": "Test"
    }
  ],
  "host": "api.example.com",
  "schemes": [
    "https"
  ],
  "consumes": [
    "application/json"
  ],
  "produces": [
    "application/json"
  ],
  "paths": {
    "/v1alpha/foo/{foo.id}": {
      "patch": {
        "operationId": "Test_UpdateFoo",
        "responses": {
          "200": {
            "description": "A successful response.",
            "schema": {
              "$ref": "#/definitions/v1alphaFoo"
            }
          },
          "default": {
            "description": "An unexpected error response.",
            "schema": {
              "$ref": "#/definitions/rpcStatus"
            }
          }
        },
        "parameters": [
          {
            "name": "foo.id",
            "in": "path",
            "required": true,
            "type": "string",
            "format": "int64"
          },
          {
            "name": "body",
            "in": "body",
            "required": true,
            "schema": {
              "$ref": "#/definitions/v1alphaUpdateFooRequest"
            }
          }
        ],
        "tags": [
          "Test"
        ]
      }
    }
  },
  "definitions": {
    "protobufAny": {
      "type": "object",
      "properties": {
        "typeUrl": {
          "type": "string"
        },
        "value": {
          "type": "string",
          "format": "byte"
        }
      }
    },
    "rpcStatus": {
      "type": "object",
      "properties": {
        "code": {
          "type": "integer",
          "format": "int32"
        },
        "message": {
          "type": "string"
        },
        "details": {
          "type": "array",
          "items": {
            "$ref": "#/definitions/protobufAny"
          }
        }
      }
    },
    "v1alphaFoo": {
      "type": "object",
      "properties": {
        "id": {
          "type": "string",
          "format": "int64"
        }
      },
      "description": "Foo test."
    },
    "v1alphaUpdateFooRequest": {
      "type": "object",
      "properties": {
        "foo": {
          "$ref": "#/definitions/v1alphaFoo",
          "required": [
            "foo"
          ]
        },
        "updateMask": {
          "type": "string"
        }
      },
      "required": [
        "foo"
      ]
    }
  }
}

Actual Behavior

Note the schema change under the request parameters

{
  "swagger": "2.0",
  "info": {
    "title": "genproto/test/v1alpha/test.proto",
    "version": "version not set"
  },
  "tags": [
    {
      "name": "Test"
    }
  ],
  "host": "api.example.com",
  "schemes": [
    "https"
  ],
  "consumes": [
    "application/json"
  ],
  "produces": [
    "application/json"
  ],
  "paths": {
    "/v1alpha/foo/{foo.id}": {
      "patch": {
        "operationId": "Test_UpdateFoo",
        "responses": {
          "200": {
            "description": "A successful response.",
            "schema": {
              "$ref": "#/definitions/v1alphaFoo"
            }
          },
          "default": {
            "description": "An unexpected error response.",
            "schema": {
              "$ref": "#/definitions/rpcStatus"
            }
          }
        },
        "parameters": [
          {
            "name": "foo.id",
            "in": "path",
            "required": true,
            "type": "string",
            "format": "int64"
          },
          {
            "name": "body",
            "in": "body",
            "required": true,
            "schema": {
              "$ref": "#/definitions/v1alphaFoo",
              "required": [ // !!!!! here is the issue, "$ref" should be alone here
                "foo"
              ]
            }
          },
          {
            "name": "updateMask",
            "in": "query",
            "required": false,
            "type": "string"
          }
        ],
        "tags": [
          "Test"
        ]
      }
    }
  },
  "definitions": {
    "protobufAny": {
      "type": "object",
      "properties": {
        "typeUrl": {
          "type": "string"
        },
        "value": {
          "type": "string",
          "format": "byte"
        }
      }
    },
    "rpcStatus": {
      "type": "object",
      "properties": {
        "code": {
          "type": "integer",
          "format": "int32"
        },
        "message": {
          "type": "string"
        },
        "details": {
          "type": "array",
          "items": {
            "$ref": "#/definitions/protobufAny"
          }
        }
      }
    },
    "v1alphaFoo": {
      "type": "object",
      "properties": {
        "id": {
          "type": "string",
          "format": "int64"
        }
      },
      "description": "Foo test."
    }
  }
}

Your Environment

(Environment name, version and operating system.)

uname -a: Linux 1319df9505dc 4.19.104-microsoft-standard #1 SMP Wed Feb 19 06:37:35 UTC 2020 x86_64 GNU/Linux

go version: 1.13.5

[johanbrandhorst]

Hi @gganley, thanks for raising this issue! That seems like an unfortunate combination of things, would you prefer that the generator errors here and refuses to produce a swagger file, or that it should drop the annotation that the user clearly added explicitly?

CC @ewhauser

[gganley]

This style of defining RPCs follows Google's AIP guidance (https://google.aip.dev/134) so it should be considered a valid way of defining methods.

Open API 2.0 follows JSON Schema rules surrounding the $ref property, which according to the RFC, siblings found next to a $ref are erroneous and SHALL be ignored.

The generated document is incorrect in that the required array is listing foo but the reference itself is to said object which is already set to required.

[johanbrandhorst]

I see, well, it sounds straightforward enough not to add a required property to the field in this case. Would you be willing to contribute this fix?

[gganley]

Sure I'll take a crack at it.

[gganley]

I see, well, it sounds straightforward enough not to add a required property to the field in this case. Would you be willing to contribute this fix?

I have a really simple fix for this in particular. One question I had though was for the files under third_party/googleapis/google is there a systematic way those are supposed to be added/updated? The readme in that directory lists the imported files and the revision they were taken from but A) does not list all of the files and B) some of the files that are in the directory are introduced after the given revision. Would it be out-of-bounds to

1. add new files (google/api/client.proto and google/api/resource.proto). This is for this PR.
2. add the missing files to the readme (google/api/field_behavior.proto)
3. Bump the revision to master/earliest commit using all the proto files needed. Not sure if this would cause breaking changes.

[johanbrandhorst]

Thanks for asking! I left a comment on the PR re: the new files.

2. add the missing files to the readme (google/api/field_behavior.proto)

I'm not entirely sure what you mean by this, we have a section in the docs about using field_behaviour.proto (https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_openapi_output/#using-googleapifield_behavior), but since it's not a mandatory dependency we haven't included it in the main repo. Reading through that snippet again now I think we should make it clear that the user has to provide these files themselves, and where to source them from, so I'd greatly appreciate a separate PR adding that to the docs.

3. Bump the revision to master/earliest commit using all the proto files needed. Not sure if this would cause breaking changes.

This would be great 😄. We don't have an automated process for this, and I don't think they've changed anything that will affect us, by "why not" is good enough for me.