Fields bound by path template also in body (Swagger)

[brocaar]

https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L123

HTTP	RPC
PUT /v1/messages/123456 { "text": "Hi!" }	UpdateMessage(message_id: "123456" message { text: "Hi!" })

The special name * can be used in the body mapping to define that
every field not bound by the path template should be mapped to the
request body. This enables the following alternative definition of
the update method:

service Messaging {
  rpc UpdateMessage(Message) returns (Message) {
    option (google.api.http) = {
      put: "/v1/messages/{message_id}"
      body: "*"
    };
  }
}
message Message {
  string message_id = 1;
  string text = 2;
}

---

Currently, this doesn't seem to work when generating the Swagger definition, as both fields are showing up. This is a snippet from my .proto file.

service Application {
    rpc Update(UpdateApplicationRequest) returns (UpdateApplicationResponse) {
        option (google.api.http) = {
            put: "/api/v1/application/{appEUI}"
            body: "*"
        };
    }
}

message UpdateApplicationRequest {
    string appEUI = 1;
    string name = 2;
}

I would have expected to ony have name in the request body. Leaving out the devEUI does work however when making requests!

When setting body: "name" in the .proto file (see also http.proto#L109), the API returns the following error (even if the appEUI is included in the body too):

{
  "Error": "json: cannot unmarshal object into Go value of type string",
  "Code": 3
}

[tmc]

@brocaar are you posting like {"name": "foo"} when you have body: "name"? That'd be incorrect.

[brocaar]

@tmc sorry for my late response. I needed to find some time to test with the latest grpc-gateway version. One the json: cannot unmarshal object into Go value of type string issue is no longer present. When a field is part of the url, in the above example appEUI, it can be left out of the request body.

What I still would expect however is that path arguments are left out of the body in the Swagger definition. As the appEUI fields is already mapped to the url, there is (in my opinion) no need to include it in the request body too.

[Dasio]

Any way how to to avoid redundant fields in message, when it's already defined in path? Talking about swagger definition.

[johanbrandhorst]

No, this is a bug in the swagger generator. Fields in the path should be excluded from the body when * is used. See https://github.com/grpc-ecosystem/grpc-gateway/blob/master/third_party/googleapis/google/api/http.proto#L151-L153. Would you be interested in fixing this?

[Dasio]

I was looking in to it.
1.) Not sure how to properly test it, how to run it local? I can see -file parameter, but I dont know how to get input for it
2.) Also not sure how to do it. There are messages generated 1:1 to proto file. And then this model is referenced from body.

  "parameters": [
          {
            "name": "id",
            "in": "path",
            "required": true,
            "type": "string"
          },
          {
            "name": "body",
            "in": "body",
            "required": true,
            "schema": {
              "$ref": "#/definitions/IdRequest"
            }
          }
        ],

 "IdRequest": {
      "type": "object",
      "properties": {
        "id": {
          "type": "string"
        },
        "something": {
          "type": "string"
        }
      }
    }

I can't remove id from IdRequest because it could be used elsewhere without path.

[johanbrandhorst]

I think we have to use the existing tests: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-swagger/genswagger/template_test.go

As to the problem of the type, yes, indeed, that is unfortunate. Maybe the best thing to do is to define a new type for the requests that take some parameters in the path and some in the body?

[jgiles]

We recently identified this issue in our generated Swagger docs, and I'm interested in contributing a fix.

The safest option seems to be generating an inline schema definition for the request body, with all the fields from the proto request message other than those captured by path parameters (see https://swagger.io/docs/specification/2-0/describing-request-body/ ). Defining a named schema for the body with the needed field subset and using $ref seems like it would risk schema-name collisions.

With this approach, we might not end up needing a schema definition for the proto request message at all (if it's not used anywhere without path params).

@johanbrandhorst does this approach make sense to you?

[johanbrandhorst]

That sounds perfect! Please take a look if you can find the time.