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 *nested* path parameters #1187

Closed
brocaar opened this issue Mar 25, 2020 · 7 comments

Comments

@brocaar
Copy link
Contributor

brocaar commented Mar 25, 2020

This is probably related to #1084 which was fixed by #1084.

Version: v1.14.3

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"];
}

Does result in the expected endpoint: /api/devices/{devEUI}.

However, if the Protobuf definition is modified to:

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

message GetDeviceRequest {v1.14.3
    Device device = 1;
}

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

The expected endpoint would be: /api/devices/{device.devEUI}.
However, the actual generated endpoint is: /api/devices/{device.devEui}.

Could it be that in case of a nested argument, the GetJsonName() is not triggered?

@brocaar brocaar changed the title json_names_for_fields=true does not respect json_name for nested path parameters protoc-gen-swagger: json_names_for_fields=true does not respect json_name for *nested* path parameters Mar 25, 2020
@johanbrandhorst
Copy link
Collaborator

Seems right. Are you saying this was broken by the change or that it wasn't supported to begin with and still isn't supported? Would you be interested in fixing this?

CC @xin-au

@brocaar
Copy link
Contributor Author

brocaar commented Mar 25, 2020

@johanbrandhorst I don't think this was ever supported. It might be a case which was not captured in #1084

@xin-au
Copy link
Contributor

xin-au commented Mar 25, 2020

@johanbrandhorst , @brocaar , thanks for pointing it out! I will be working on it. Sorry about that.

@brocaar
Copy link
Contributor Author

brocaar commented Mar 25, 2020

Thank you @xin-au !

@xin-au
Copy link
Contributor

xin-au commented Mar 26, 2020

@brocaar , @johanbrandhorst , quick update:
I was investigating on it. And I realized that if a reserved json name is defined not in the root level of the request data structure of a grpc call, then that reserved json name will not be in the field data structure(the data structure which should carry this info, its name FieldDescriptorProto). So, the root cause is the reserved json names are not read recursively. I was trying to fix it. And found that this structure FieldDescriptorProto is from protobuf code, we could do nothing about it.
Let me try if there is any workaround.

@xin-au
Copy link
Contributor

xin-au commented Mar 28, 2020

@brocaar , @johanbrandhorst , update:
Still working on it, found some solution could fix it.

@xin-au
Copy link
Contributor

xin-au commented Mar 29, 2020

@brocaar @johanbrandhorst , just created a PR #1191
@brocaar , by chance you could clone and build it and try if this will be what you expect to unblock you.

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