swagger.json: Generated examples for PUT requests don't omit properties provided by path-parameters

[RobinNoHood]

🐛 Bug Report

When generating the swagger.json from the proto definition, the generated examples for PUT request bodies that use path parameters seem incorrect.
Path parameters that are mapped to fields in the proto-request are not omitted from the generated swagger examples.

To Reproduce/Example

(from the ABitOfEverything service definition):

rpc Update(ABitOfEverything) returns (google.protobuf.Empty) {
		option (google.api.http) = {
			put: "/v1/example/a_bit_of_everything/{uuid}"
			body: "*"
		};l
	}

Here, we have an update request that attempts to update the resource at /v1/example/a_bit_of_everything/{uuid}
On the server gateway side, the UUID from the path is correctly mapped to ABitOfEverything.uuid

In the generated swagger.json, we get this PUT definition (omitted unnecessary parts):

"put": {
        "operationId": "ABitOfEverythingService_Update",
        "parameters": [
                 {
                   "name": "uuid",
                   "in": "path",
                   "required": true,
                   "type": "string"
                 },
                 {
                   "name": "body",
                   "in": "body",
                   "required": true,
                   "schema": {
                     "$ref": "#/definitions/examplepbABitOfEverything"
                   }
                 }
               ]
        }

Expected Behavior

The uuid field in the example-update-body should be omitted, because it is provided by the path parameter.

"examplepbUpdateABitOfEverything": {
      "type": "object",
      "properties": {
         (... lots of properties but no uuid ...)
      }
}

Actual Behavior

However, the actually generated #/definitions/examplepbABitOfEverything is represented by this generated swagger object:

"examplepbABitOfEverything": {
      "type": "object",
      "properties": {
        "uuid": {
          "type": "string",
          "minLength": 1,
          "pattern": "[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}"
        }
        (... lots of properties ...)
      }
}

As you can see, the example body contains the uuid property AGAIN, even though it's already part of the path parameters.
I would expect this property to be omitted in the example put-request.

The reason for this seems to be excessive code reuse in the generated swagger json - it uses the same "examplepbABitOfEverything" for every endpoint based on the "ABitOfEverything" proto definition, even though the way that the properties are aggregated might differ (some might come from path or query parameters while the remaining ones are provided via request body).

My issue with this is that from an API-consumers point of view, the generated swagger doc implies that you should provide a UUID as part of the body, even though this field will be ignored by the gateway (it's actually overridden by the path-UUID). This is confusing and misleading.

In my opinion, the correct solution to this would be to remove path-based properties from the generated examples. Of course you would then need to generate different variations of "examplepbABitOfEverything" for different API-endpoints (or at least limit the reuse of these examples to endpoints that actually use the same path/query/body configurations).

Alternatively, give the user a way to define which properties to omit. So instead of using body: "*" to include the whole request body, add a more powerful syntax that allows you to omit certain properties.

[johanbrandhorst]

Hi @RobinNoHood, thanks for your interest in the project! I agree that this seems like a bug, and ideally we'd like to fix it, but I can see it being quite complicated to get right, since we'll have to dynamically drop fields from a type and won't be able to simply reuse it anymore. I don't think we'll be adding any new annotations to drop fields, so this would have to be fixed in the swagger generator. What can I do to help you bring about a fix for this?

[oyvindwe]

If anybody looks into this, fields annotated with [(google.api.field_behavior) = OUTPUT_ONLY] should also be skipped in request examples.

A workaround seems to be to provide a complete example in the request message. There's no type checking of the example message though, it's only validated to be correct JSON, so it's a bit error prone for complex messages.

[oyvindwe]

I've been digging a bit further into this, and first of all I think I misunderstood the title - it refers to the examples provided in the repository, but the bug applies to both the generated body request types as well as the generated example value for the request body.

This also seem to apply to all requests with body (PUT, PATCH, POST), not only PUT. I think a better title would be something like "protoc-gen-openapiv2 includes path parameters in body request types".

Overriding example does work for "Try it out!" in SwaggerUI, but not for some other tools that generates forms based on the request types (e.g. readme.io) or code generation. Using readme.io rendering, a required path parameter also shows as a required body parameter, which is correct according to the generated OpenAPI file.

As path parameters can refer arbitrarily to any field in the protobuf message structure [1], this means that whenever a path parameter is used, the types for the body request have to be customised for that URL mapping (as the path can be different for each additional_binding) by removing all the path parameter fields.

I agree with @johanbrandhorst in #1670 (comment) that this should probably only be fixed in the swagger/openapiv2 generator. Whenever types are customised for a request, I guess they should rather be specified inline in the operation than referred to in definitions.

This behaviour is the same for versions 2.7.1 and 2.7.2.

[1] https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L224-L231

[oyvindwe]

I think I've isolated the issue:

grpc-gateway/protoc-gen-openapiv2/internal/genopenapi/template.go

Lines 1028 to 1031 in ed37f51

	// We only support excluding top-level fields captured by path parameters.
	if len(p.FieldPath) == 1 {
	bodyExcludedFields = append(bodyExcludedFields, p.FieldPath[0].Target)
	}

This needs to be done for all levels.

[johanbrandhorst]

Interesting, would you be interested in contributing a fix for this?

[oyvindwe]

I'm taking a stab at it, but this is my first Go project, so there's lots of idioms in this code that's completely new to me.

[oyvindwe]

Adressed in PR #2553