[protoc-gen-openapiv2] Incorrectly generated OpenAPIv2 spec when using field_behaviour annotation

[jan-sykora]

🐛 Bug Report

This is almost exactly the same error as in #1937:

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.

The #1937 is already resolved & closed but it seems this type of error was not resolved completely since I can reproduce it.

To Reproduce

I'm using Buf to deal with external dependencies (googleapis). But you should be able to reproduce it using only protoc, protoc-gen-openapiv2 and foo.proto, if you manually provide the googleapis.

Here's the whole Buf setup:

.
├── buf.gen.yaml
├── buf.work.yaml
└── proto
    ├── buf.lock
    ├── buf.yaml
    └── v1
        └── foo.proto

buf.gen.yaml

version: v1
managed:
  enabled: true
  go_package_prefix:
    default: github.com/foo
plugins:
  - name: openapiv2
    out: gen

buf.work.yaml

version: v1
directories:
  - proto

proto/buf.yaml

version: v1
deps:
  - buf.build/googleapis/googleapis

proto/buf.lock

Is generated by command:

cd proto && buf mod update

example content:

# Generated by buf. DO NOT EDIT.
version: v1
deps:
  - remote: buf.build
    owner: googleapis
    repository: googleapis
    commit: faacf837d7304c58b7c9020c7807fa6e

proto/v1/foo.proto

syntax = "proto3";

package v1;

import "google/api/annotations.proto";
import "google/api/field_behavior.proto";

message Foo {
}

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

service FooService {
  rpc CreateFoo (CreateFooRequest) returns (Foo) {
    option (google.api.http) = {
      post: "/v1/foos"
      body: "foo"
    };
  }
}

Finally, generate openapiv2 via buf command:

buf generate

(you should be also able to generate directly by using protoc but you have to provide the googleapis deps)

Expected behavior

gen/v1/foo.swagger.json

{
  "swagger": "2.0",
  "info": {
    "title": "v1/foo.proto",
    "version": "version not set"
  },
  "tags": [
    {
      "name": "FooService"
    }
  ],
  "consumes": [
    "application/json"
  ],
  "produces": [
    "application/json"
  ],
  "paths": {
    "/v1/foos": {
      "post": {
        "operationId": "FooService_CreateFoo",
        "responses": {
          "200": {
            "description": "A successful response.",
            "schema": {
              "$ref": "#/definitions/v1Foo"
            }
          },
          "default": {
            "description": "An unexpected error response.",
            "schema": {
              "$ref": "#/definitions/rpcStatus"
            }
          }
        },
        "parameters": [
          {
            "name": "foo",
            "in": "body",
            "required": true,
            "schema": {
              "$ref": "#/definitions/v1Foo",
            }
          }
        ],
        "tags": [
          "FooService"
        ]
      }
    }
  },
  "definitions": {
    "protobufAny": {
      "type": "object",
      "properties": {
        "@type": {
          "type": "string"
        }
      },
      "additionalProperties": {}
    },
    "rpcStatus": {
      "type": "object",
      "properties": {
        "code": {
          "type": "integer",
          "format": "int32"
        },
        "message": {
          "type": "string"
        },
        "details": {
          "type": "array",
          "items": {
            "$ref": "#/definitions/protobufAny"
          }
        }
      }
    },
    "v1Foo": {
      "type": "object"
    }
  }
}

Actual Behavior

gen/v1/foo.swagger.json

{
  "swagger": "2.0",
  "info": {
    "title": "v1/foo.proto",
    "version": "version not set"
  },
  "tags": [
    {
      "name": "FooService"
    }
  ],
  "consumes": [
    "application/json"
  ],
  "produces": [
    "application/json"
  ],
  "paths": {
    "/v1/foos": {
      "post": {
        "operationId": "FooService_CreateFoo",
        "responses": {
          "200": {
            "description": "A successful response.",
            "schema": {
              "$ref": "#/definitions/v1Foo"
            }
          },
          "default": {
            "description": "An unexpected error response.",
            "schema": {
              "$ref": "#/definitions/rpcStatus"
            }
          }
        },
        "parameters": [
          {
            "name": "foo",
            "in": "body",
            "required": true,
            "schema": {
              "$ref": "#/definitions/v1Foo",
              "required": [ <-- THIS SHOULDN'T BE HERE
                "foo"           
              ]
            }
          }
        ],
        "tags": [
          "FooService"
        ]
      }
    }
  },
  "definitions": {
    "protobufAny": {
      "type": "object",
      "properties": {
        "@type": {
          "type": "string"
        }
      },
      "additionalProperties": {}
    },
    "rpcStatus": {
      "type": "object",
      "properties": {
        "code": {
          "type": "integer",
          "format": "int32"
        },
        "message": {
          "type": "string"
        },
        "details": {
          "type": "array",
          "items": {
            "$ref": "#/definitions/protobufAny"
          }
        }
      }
    },
    "v1Foo": {
      "type": "object"
    }
  }
}

Your Environment

buf version: 1.10.0
protoc-gen-openapiv2 version: v2.14.0
go version: go1.19.1 darwin/amd64

[johanbrandhorst]

Thanks for the detailed issue, and I'm sorry this is causing trouble for you. It seems to me there might only be a minor additional fix to the original fix, would you be interested in pursuing it? The first step would probably be to add an example of this problem to our monster example protobuf file, then we could work on correcting the behavior.

[jan-sykora]

Hi, yes, I can try to fix that. I've added the [(google.api.field_behavior) = REQUIRED] annotation to the CreateBookRequest in the a_bit_of_everything.proto and it generates the swagger file incorrectly (as described in this issue).

I'll try to fix it analogically as was done in #1944.

[jan-sykora]

This turned out more difficult than expected. The main problem is that it is not clear how the Reference Object $ref should be generated.

Context

The OpenAPI v2 spec defines Reference Object:

The Reference Object is a JSON Reference that uses a JSON Pointer as its value.

The linked JSON Reference definition mentions:

Any members other than "$ref" in a JSON Reference object SHALL be ignored.

As I understand it, this definition does not explicitly forbid other members (siblings) of the ref object. It only says, that other members shall be ignored; so the actual behaviour I put into issue description may be "kind of" correct.

However, some OpenAPIv2 tools consider it an invalid openAPIv2 spec. For example, the https://github.com/OpenAPITools/openapi-generator for generating client code from OpenAPIv2 spec. If it processes spec that contains ref object with siblings, then this openapi-generator considers this spec as invalid.

This limitation has been discussed in the OpenAPI Specification project (OAI/OpenAPI-Specification#556) and it seems that it has been allowed to add siblings to the ref object (in contrary with the JSON standard), but it applies only for OpenAPI v3.1 and only in some cases (OAI/OpenAPI-Specification#556 (comment)). It does not apply for the OpenAPI v2 at all.

Current state of protoc-gen-openapiv2

protoc-gen-openapiv2 currently generates reference objects with siblings. In #2904 was removed the protection of not adding siblings to the $ref object and in #2906 is even mentioned that in OpenAPI v2 it's ok to have sibling fields of the $ref field (which depends on how you define ok).

The reason why the original issue #1937 does not suffer from this invalid Reference Object issue is that in #2553 the reference object was replaced with a full object definition.

IMHO this is wrong because it's duplicating the object definition (there could have been some reason for it which I don't see) but that's a different topic.

Solution

IMHO it comes down to a decision whether it's ok to keep it as it is. The standards discourage from it and it makes it incompatible with some other OpenAPIv2 tools. On the other hand, it's not explicitly forbidden.

There's a workaround by using allOf (OAI/OpenAPI-Specification#556 (comment)). It fully conforms to the rules. Nevertheles, it could be a potential breaking change for those consumers who are already using the current protoc-gen-openapiv2 implementation with ref siblings.

I'd prefer to follow the standards either by using the allOf or by not allowing the siblings but I'm not sure if it's possible to do this change.

@johanbrandhorst what do you think? Is there someone who could help with this?

[johanbrandhorst]

Thanks for the great analysis. A couple of points:

1. protoc-gen-openapiv2: remove path parameters from body parameters #2553 is correct, it doesn't duplicate the whole object, it inlines the object minus any parameters that are in the path.
2. As you notice, we have gone back and forth on whether to allow siblings to $ref. In the end, I think we should allow them, as they seem correct in some places, but we should be pragmatic with how to apply it. The fact that this breaks well established tools is enough for me to want to fix this specific case. Could you elaborate on what the error looks like? Is the problem specifically that required appears next to $ref or that anything appears next to $ref?
3. Note that we introduced the allOf generation already in Make read_only work with references using AllOf #3082. Actually does that fix this issue 🤔?

CC @gostajonasson.

[jan-sykora]

Thanks for the quick response!

Re 1.
I am following the https://google.aip.dev/134 from Google AIP. According to this guide, the request message must contain a field for the resource and the resource name should map to the URI path. So it's expected that the identifier is duplicated in both URI and body. I'm not saying Google AIP should be always followed (although it worked well for me so far) but in this case it makes sense to me and I guess authors had a good reason to design it that way.

The #2553 effectively creates a new object (it's no longer a reference to the resource) and that seems to me as a worse duplication. Client using the OpenAPI has no guarantee that the object in the request body is the same object. There could be some other fields missing or some other fields added (which wouldn't make sense but it's possible when it's a different object).

Re 2.
If the example from this issue is copied into https://editor.swagger.io/ then there's a warning:

Sibling values alongside $refs are ignored.
To add properties to a $ref, wrap the $ref into allOf, or move the extra properties into the referenced definition (if applicable).

If we add a different valid schema property (e.g. title) then the result is the same / there will be the same warning:

The https://github.com/OpenAPITools/openapi-generator (tested with latest version 6.2.1) considers even the "required" ref sibling as an error (example of generating code the example Foo OpenAPIv2 spec with ref sibling):

Exception in thread "main" org.openapitools.codegen.SpecValidationException: There were issues with the specification. The option can be disabled via validateSpec (Maven/Gradle) or --skip-validate-spec (CLI).
 | Error count: 1, Warning count: 0
Errors: 
        -attribute paths.'/v1/foos'(post).[foo].required is unexpected

        at org.openapitools.codegen.config.CodegenConfigurator.toContext(CodegenConfigurator.java:604)
        at org.openapitools.codegen.config.CodegenConfigurator.toClientOptInput(CodegenConfigurator.java:631)
        at org.openapitools.codegen.cmd.Generate.execute(Generate.java:457)
        at org.openapitools.codegen.cmd.OpenApiGeneratorCommand.run(OpenApiGeneratorCommand.java:32)
        at org.openapitools.codegen.OpenAPIGenerator.main(OpenAPIGenerator.java:66)

As the error message mentions, it is possible to disable spec validation when using this tool but I wanted to use this option only as a last resort because it's not safe.

So other OpenAPIv2 tools treat this issue differently which doesn't help a lot. It would be nice if those spec validators would consider the ref siblings only a warning - maybe those tools should be the ones that should be fixed?

Re 3.
To me it seems more as a hack rather than a proper solution. The allOf purpose is to combine an array of object definitions that compose a single object. I think that it's not a way to go.

Another thought

Besides all these points, I'd like to ask, if it actually makes sense in this exact case to add the "required" property to the schema. This is the example from issue description.

        "parameters": [
          {
            "name": "foo",
            "in": "body",
            "required": true,
            "schema": {
              "$ref": "#/definitions/v1Foo",
              "required": [
                "foo"           
              ]
            }
          }
        ],

Only the body request parameter "foo" itself is required as defined in protobuf (reflected in OpenAPI by "required": true). But the 'required schema property' in my opinion does not make sense. If I understand it correctly, this says property "foo" of referenced object Foo is required:

            "schema": {
              "$ref": "#/definitions/v1Foo",
              "required": [
                "foo"           
              ]

Am I correct? If so, then it doesn't make sense. The Foo object has no property called foo. And even if it had this property, then is it ok to change behaviour of property of a referenced object? I'd say not.

There are other examples in the a_bit_of_everything that seem suspicious to me:

1. Overridden type:

                "nested": {
                  "type": "array",
                  "items": {
                    "type": "object",
                    "$ref": "#/definitions/ABitOfEverythingNested"
                  }
                }

Why is it necessary to redefine type of a referred object? This information is already included in the object definition:

"definitions": {
    "ABitOfEverythingNested": {
      "type": "object",
      "example": {
        "ok": "TRUE"
      },
      "properties": {

2. Overridden description / title

               "enumValueAnnotation": {
                  "$ref": "#/definitions/examplepbNumericEnum",
                  "description": "Numeric enum description.",
                  "title": "Numeric enum title"
                }

The examplepbNumericEnum object has already a description:

    "examplepbNumericEnum": {
      "type": "string",
      "enum": [
        "ZERO",
        "ONE"
      ],
      "default": "ZERO",
      "description": "NumericEnum is one or zero.\n\n - ZERO: ZERO means 0\n - ONE: ONE means 1"
    }

What do you think?

PS: @johanbrandhorst thank you for all your help! I don't have a strong knowledge of OpenAPI so please correct me if I don't understand something correctly.

[johanbrandhorst]

So it's expected that the identifier is duplicated in both URI and body.

I don't agree with this interpretation of the AIP. It doesn't say what the behavior should be when the name exists in both the path and the body, but it seems clear to me that the path should be the only one that matters, so I think the behavior of our generator is consistent with this AIPs recommendations. The concern about having it be a separate object is moot since it's generated from the protobuf source of truth, which if it applies the AIP recommendations will be the same object minus the variables in the path.

For point 2, thank you for supplying the error from the generator. It's still not clear to me whether it's a problem specifically with required or if any field would trip this validation. I agree that turning off validation should be a last resort and I would hope we could make the output of our generator conform without any warnings. I don't think it's right that this required field appears as a sibling but I believe there were reasons for other elements to appear as siblings. I also would agree that the validator shouldn't consider this an error, only a warning, since it says sibling fields should be ignored, not that they shouldn't exist 🤷🏻.

3. It does seem a bit of a hack, but if it does the job 😁.

For your other questions:

1. Agreed, required in the schema object is weird and probably shouldn't be there
2. Overridden types were added in recursive objects to fix a separate bug: Recursive Child elements do not generate correctly for OpenAPI #3049
3. Overriden descriptions and titles are because you can add comments to messages, which is the default title and description, but you can also override it on a per-field basis. This behavior is intentional as far as I know.

   grpc-gateway/examples/internal/proto/examplepb/a_bit_of_everything.proto

   Lines 355 to 358 in 257af99

   	NumericEnum enum_value_annotation = 33 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {
   	title: "Numeric enum title",
   	description: "Numeric enum description."
   	}];

Great questions, hope that makes sense.

[czabaj]

If I understand it correctly, this says property "foo" of referenced object Foo is required:

Your understanding is correct, even if the required field in the foo parameter would not be ignored, it would be an error because it will mean, that "the property foo in the reference v1Foo is required" but the reference v1Foo has no member foo. The field required in the body parameter simply should not be there at all, it seems that it originates from the field required in the body parameter definition (above the schema) and is appended to the schema by accident.