Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protoc-gen-swagger: json_names_for_fields=true does not respect json_name for path parameters #1084

Closed
brocaar opened this issue Nov 13, 2019 · 5 comments

Comments

@brocaar
Copy link
Contributor

brocaar commented Nov 13, 2019

I'm using the json_names_for_fields=true option and am seeing inconsistent results, specifically between the URL parameter names and the names in the request / response bodies. Example:

service DeviceService {
    rpc Get(GetDeviceRequest) returns (GetDeviceResponse) {
        option (google.api.http) = {
            get: "/api/devices/{dev_eui}"
        };
    }

message GetDeviceRequest {
    // Device EUI (HEX encoded).
    string dev_eui = 1 [json_name = "devEUI"];
}

I believe this is because the fieldnames in the request / response body are get using the f.GetJsonName() (e.g. https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-swagger/genswagger/template.go#L342) where the URL parameter names are get using the lowerCamelCase function (e.g. https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-swagger/genswagger/template.go#L772).

The json_name is respected by the GetJsonName() function (e.g. [json_name = "devEUI"], but not by the lowerCamelCase function.

The generated Swagger JSON looks like:

    "/api/devices/{devEui}": {
      "get": {
        "summary": "Get returns the device matching the given DevEUI.",
        "operationId": "Get",
        "responses": {
          "200": {
            "description": "A successful response.",
            "schema": {
              "$ref": "#/definitions/apiGetDeviceResponse"
            }
          }
        },
        "parameters": [
          {
            "name": "devEui",
            "description": "Device EUI (HEX encoded).",
            "in": "path",
            "required": true,
            "type": "string"
          }
        ],
        "tags": [
          "DeviceService"
        ]
      },

I would have expected:

    "/api/devices/{devEUI}": {
      "get": {
        "summary": "Get returns the device matching the given DevEUI.",
        "operationId": "Get",
        "responses": {
          "200": {
            "description": "A successful response.",
            "schema": {
              "$ref": "#/definitions/apiGetDeviceResponse"
            }
          }
        },
        "parameters": [
          {
            "name": "devEUI",
            "description": "Device EUI (HEX encoded).",
            "in": "path",
            "required": true,
            "type": "string"
          }
        ],
        "tags": [
          "DeviceService"
        ]
      },

Note that the camel casing for URL parameters was implemented in v1.9.6 (#986). Before this version URL parameters were always using the Protobuf name (in the above example it would have been dev_eui).

@xin-au since you have worked on #986, do you think this would be easy to fix? I think instead of generating the name using the lowerCamelCase function and replicate the Protobuf (default) JSON naming convention, it would be better to somehow lookup the Protobuf field object and call its GetJsonName method.

brocaar added a commit to brocaar/chirpstack-api that referenced this issue Nov 13, 2019
v1.9.6 changes the URL parameter naming. The updated naming does not
respect the `json_name` in the Protobuf file. See also:
grpc-ecosystem/grpc-gateway#1084.
@johanbrandhorst
Copy link
Collaborator

Thanks for the report @brocaar, as you surmise this is because of a complex interaction between the two different parsers we have, the URL path/query parameter parser and the JSON content parser. It's not an easy fix unfortunately, as you'll need to build the JSON options into the URL path and query parameter parsing.

@brocaar
Copy link
Contributor Author

brocaar commented Nov 13, 2019

Thanks for the quick response @johanbrandhorst . Please note that this only seems to affect the path parameters, I believe the query parameters are named as expected and do respect the json_name.

I'm not 100% familiar all the internal logic, I would need to dive in deeper. However, the code does generate:

        "parameters": [
          {
            "name": "devEui",
            "description": "Device EUI (HEX encoded).",
            "in": "path",
            "required": true,
            "type": "string"
          }
        ],

Since it is able to read the description from the Protobuf file, wouldn't that mean that it would be possible to iterate over the fields in the Protobuf message find the field (*descriptor.Field) with the Protobuf name "dev_eui" and then call GetJsonName()?

@johanbrandhorst
Copy link
Collaborator

Try it :). I don't know what issues you might run into.

@xin-au
Copy link
Contributor

xin-au commented Nov 13, 2019

@brocaar , Thanks for pointing it out! I will look into the code.

@xin-au
Copy link
Contributor

xin-au commented Nov 14, 2019

@brocaar , @johanbrandhorst , just created a PR for this: #1085

adasari pushed a commit to adasari/grpc-gateway that referenced this issue Apr 9, 2020
* Support reserved json name and add tests

* Correct some variable names

* Optimize a logic for assigning a reserved json name to jsonCamelCaseName

* Put a logic for checking if there is a reseved json name in the method of lowerCamelCase

Fixes grpc-ecosystem#1084
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants