Generation of invalid swagger specification

[bvwells]

🐛 Bug Report

Invalid swagger specifications are generated when trying to have get/patch requests on same path. The issue arises through the recommended use of field masks for the updating of a resource. This seems like a bit of a UX issue as the workaround is to not use field masks for update rpcs.

Apologies if there is a mistake in how I've added annotations to the protocol buffer service definition.

To Reproduce

Take the following protocol buffer definition:

syntax = "proto3";
option go_package = "github.com/bvwells/grpc-gateway-example/proto/beers";

import "google/api/annotations.proto";
import "google/protobuf/field_mask.proto";

message Thing {
  string id = 1;
  string name = 2;
}

message GetThingRequest {
    string id = 1;
}

message GetThingResponse {
    Thing thing = 1;
}

message UpdateThingRequest {
    Thing thing = 1;
    google.protobuf.FieldMask update_mask = 2;
}

message UpdateThingResponse {
    Thing thing = 1;
}

// Thing service.
service ThingService {

    // GetThing gets a thing given its ID.
    rpc GetThing(GetThingRequest) returns (GetThingResponse) {
        option (google.api.http) = {
            get: "/api/v1/things/{id}"
        };
    }

    // UpdateThing updates a thing given its ID.
    rpc UpdateThing(UpdateThingRequest) returns (UpdateThingResponse) {
        option (google.api.http) = {
            patch: "/api/v1/things/{thing.id}"
            body: "thing"
        };
    }
}

Run the protoc-gen-swagger generator:

protoc -I. --swagger_out=disable_default_errors=true,logtostderr=true:./ api.proto

Expected behavior

I would expect a valid swagger specification to be generated or an error in the generation process.

Actual Behavior

The swagger specification is generated successfully, but is invalid. Validating the swagger specification shows, e.g. in swagger editor (https://editor.swagger.io/)

Semantic error at paths./api/v1/things/{thing.id}
Equivalent paths are not allowed.

A similar issue can also be seen in https://github.com/grpc-ecosystem/grpc-gateway/blob/master/examples/internal/proto/examplepb/a_bit_of_everything.swagger.json which is an invalid swagger specification.

Let me know if you require any further information.

[johanbrandhorst]

Hi Ben!

Thanks for the bug report. That's really unfortunate, good find. Could you link to the specific part of the swagger definition in our examples that is wrong, and what should it look like? Would you be able to help us get this fixed? I'd be happy to assist where I can. I would start looking in https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-swagger/genswagger/template.go.

[bvwells]

I believe that the issue in a_bit_of_everything.proto is caused by the following annotations:

    rpc Lookup(sub2.IdMessage) returns (ABitOfEverything) {
        option (google.api.http) = {
            get: "/v1/example/a_bit_of_everything/{uuid}"
        };
    }

https://github.com/grpc-ecosystem/grpc-gateway/blob/294b4d84b592a989470a4b5d0fe223c1c0164cfa/examples/internal/proto/examplepb/a_bit_of_everything.proto#L353

    rpc DeepPathEcho(ABitOfEverything) returns (ABitOfEverything) {
        option (google.api.http) = {
            post: "/v1/example/a_bit_of_everything/{single_nested.name}"
            body: "*"
        };
    }

https://github.com/grpc-ecosystem/grpc-gateway/blob/294b4d84b592a989470a4b5d0fe223c1c0164cfa/examples/internal/proto/examplepb/a_bit_of_everything.proto#L487

The swagger specification generated has the following paths:

GET:  /v1/example/a_bit_of_everything/{uuid}
POST: /v1/example/a_bit_of_everything/{single_nested.name}

I'll have a dig and see what I can find...

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[johanbrandhorst]

Reopening

[johanbrandhorst]

@jackwootton are you interested in fixing this?

[jackwootton]

I can take a look, but noticed the file you recommended starting with (above), cannot be found https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-swagger/genswagger/template.go

[johanbrandhorst]

It has been moved https://github.com/grpc-ecosystem/grpc-gateway/blob/963f1aa09bbbcf62bacdf2e3d0d6c45c3f76d7e4/protoc-gen-openapiv2/internal/genopenapi/template.go

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[hxtk]

This issue is important to me for a different reason. When creating services based on Google's API recommendations (or even Google's APIs themselves), it is typical for a resource to be identified by a canonical name, which is a URN, rather than being split up into segments.

However, it is possible to deterministically translate using the (google.api.resource).pattern bindings to build out a more tightly-constrained path. runtime does this.

This would be a backwards-incompatible change to the generated swagger docs, so I imagine it would need to be configurable and default to the current behavior.

I'll be working on it in my own fork because it's a feature that I need; would you be willing to accept a pull request for that change?

[johanbrandhorst]

I'd be happy to review any PR of course but we're unlikely to merge a breaking change