Grpc-gateway: Nested message names can collide silently in swagger generation and result in randomized swagger message definitions

[johnchildren]

Generated swagger files can silently have name collisions between message names when messages are nested. For example:

syntax = "proto3";

message FooBar {
  string value = 1;
}

message Foo {
  message Bar {
    string value = 1;
  }
  Bar bar = 1;
}

service FooBarService {
  rpc BarYourFoos(FooBar) returns (Foo);
}

Here you will _sometimes_ end up with a different definition of FooBar depending on how two go maps are presumably being unioned? For example:

{
  "swagger": "2.0",
  "info": {
    "title": "test.proto",
    "version": "version not set"
  },
  "schemes": [
    "http",
    "https"
  ],
  "consumes": [
    "application/json"
  ],
  "produces": [
    "application/json"
  ],
  "paths": {},
  "definitions": {
    "Foo": {
      "type": "object",
      "properties": {
        "bar": {
          "$ref": "#/definitions/FooBar"
        }
      }
    },
    "FooBar": {
      "type": "object",
      "properties": {
        "value": {
          "type": "string"
        }
      }
    }
  }
}

Which contains a ref to the wrong field type. This behavior can happen across imports so if you have one nested message in another file you can end up with unexpected random collisions which lead to different file output.

This was causing a CI pipeline to randomly fail internally as one message field in one file randomly collided with another.

I would expect that a collision like this should instead error or have sufficient namespacing so as to be avoided.

Have prepared an example repo with commited generated files, you can clone it and run:

prototool generate

To reproduce and if you use nix a shell is provided to pin against the latest release.

https://github.com/johnchildren/grpc-gateway-namespacing-issue-example

edit: Actually upon re-running it looks like it is only randomly wrong when you import it from another file? I can extend my test case but it is still behaving incorrectly in the example.

[johanbrandhorst]

This was causing a CI pipeline to randomly fail internally as one message field in one file randomly collided with another.

This sounds incredibly annoying, sorry that this happened to you! Thanks for reporting it. Would you be interested in trying to fix this? I would accept something like a detection that gives an error at generate time, but ideally having some way to namespace the two would be best, of course.

[johnchildren]

Sure thing, I don't think I have successfully managed to make this completely reproducible, but I think a good first step would be trying to detect name clashes like this just for nested messages and emitting an error. Hopefully this should be reasonably easy to implement and easily testable enough and I can look at doing more if this does not adequately prevent this problem from happening again.

I can take a look at trying to do this in the next few days depending on how busy I am.

[achew22]

To echo what Johan said, if there is anything we can do to help reproduce this, please don't hesitate to ask

[johnchildren]

Update: I've identified the file and line where the merge occurs, which is here: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-swagger/genswagger/template.go#L351 .

There are a few other places where something similar happens but I suppose I need to categorize them into possible and impossible collisions based on the translation of protobuf to swagger semantics. It's possible this might be the only place this can happen.

In any case what would you recommend I do here? One solution would be to check if the key already exists in the map each time and panic if it is present, but perhaps there are other solutions?

[johanbrandhorst]

For now we can check if it exists and just return and error. That should make this confusing behaviour impossible, though it obviously doesnt fix the namespacing issue.

[johnchildren]

That makes sense; and hopefully this isn't something most users will run into anyway as I've noticed the namespacing works a bit differently on newer releases than the one we were using before. This may mean that with this change any issues will be resolved.

Will take a look again and potentially submit a PR this evening.