protoc-gen-openapiv2: json_names_for_fields=true can generate invalid OpenAPI output

[rogpeppe]

🐛 Bug Report

With json_names_for_fields=true (the default), some parameter names cause the generated OpenAPI JSON to contain parameter name mismatches.

To Reproduce

Create an example.proto file with the following content:

syntax = "proto3";
package example;
option go_package = ".;example";

import "google/api/annotations.proto";

message Get1Request {
	string FooBar = 1;
}

message Get1Response {
	string X = 1;
}

message Get2Request {
	string ID = 1;
}

message Get2Response {
	string Y = 1;
}

service Example {
	rpc Get1(Get1Request) returns (Get1Response) {
		option (google.api.http) = {
			get: "/v1/foo/{FooBar=*}"
		};
	}
	rpc Get2(Get2Request) returns (Get2Response) {
		option (google.api.http) = {
			get: "/v1/bar/{ID=*}"
		};
	}
}

Copy the third_party directory from the github.com/grpc-ecosystem/grpc-gateway repository into a local third_party directory.

Generate the OpenAPI JSON by running:

protoc -I. -I./third_party/googleapis --openapiv2_out=. example.proto

Run the generated example.swagger.json file through an OpenAPI verifier, such as https://editor.swagger.io/.

Expected behavior

I'd expect the verification to succeed (i.e. the generated OpenAPI should be valid).

Actual Behavior

There is an inconsistency between the parameter names in the URL and those in the parameters field.

For example:

   "/v1/bar/{iD}": {
     "get": {
...
       "parameters": [
         {
           "name": "ID",

OpenAPI parameters are case sensitive.

Here's an example of the reported errors:

Semantic error at paths./v1/bar/{iD}
Parameter names are case-sensitive. The parameter named "ID" does not match the case used in the path "/v1/bar/{iD}".
Jump to line 18
Semantic error at paths./v1/foo/{podName}
Parameter names are case-sensitive. The parameter named "PodName" does not match the case used in the path "/v1/foo/{podName}".
Jump to line 48

(as an aside: iD is not a great spelling for the JSON name).

Your Environment

I'm using github.com/grpc-ecosystem/grpc-gateway/v2 at v2.5.0 under Ubuntu.

[johanbrandhorst]

Hi Roger, good to see you in our neck of the woods 😁. That certainly looks like a bug, and I think it's probably because of this line:

grpc-gateway/protoc-gen-openapiv2/internal/genopenapi/template.go

Line 937 in dd60b3d

parameterString = lowerCamelCase(parameterString, meth.RequestType.Fields, msgs)

. If we could add a failing test to https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-openapiv2/internal/genopenapi/template_test.go it should hopefully be fairly straightforward to fix. Would you be interested in contributing a fix for this?

[stale]

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.

[rogpeppe]

Go away stalebot.

[oyvindwe]

I'm not sure if it is the path template that is wrong or the parameter name (or both).

What are the expected JSON field names for ID and FooBar? The Proto 3 specification is a bit vague:

Message field names are mapped to lowerCamelCase and become JSON object keys. Parsers accept both the lowerCamelCase name (or the one specified by the json_name option) and the original proto field name.

I could argue for any of these 3 variants:
ID -> id, URL - >url, URLString -> urlstring, FooBar -> fooBar (lowercase continuous uppercased characters)
ID -> iD, URL -> uRL, uRLString, FooBar -> fooBar (lowercase first uppercased character)
ID -> iD, URL -> urL, urlString, FooBar -> fooBar (lowercase continuous uppercased characters except the last one)
ID -> ID, URL -> URL, FooBar -> FooBar (preserve uppercase characters)

I guess what matters is how is this implemented (for query parameters and JSON fields) in the actual gateway (grpc-gateway as well as ESPv1 and ESPv2).

[oyvindwe]

Preserve uppercase characters is probably the safe choice, given that the original proto field must be supported. However, that may lead to problems if combining with _, e.g. what's expected for Foo_Bar? fooBar or FooBar?

[oyvindwe]

I could take a stab at this if I get help to figure out the expected casing. Do you know, @rogpeppe or @johanbrandhorst?

[johanbrandhorst]

I believe https://github.com/protocolbuffers/protobuf-go/blob/3992ea83a23c00882339f33511074d251e19822c/internal/strs/strings.go#L92-L106 is the canonical mapping function that protojson uses. https://github.com/protocolbuffers/protobuf-go/blob/3992ea83a23c00882339f33511074d251e19822c/internal/strs/strings_test.go#L67 has some test cases, but you could probably quite easily tell what your examples should resolve to.

[oyvindwe]

protoc-gen-openapiv2 already depends on google.golang.org/protobuf/, but I'm not able to use google.golang.org/protobuf/internal/strs directly; I assume this is because it is an internal package:

ERROR: /Users/omw/proj/grpc-gateway/protoc-gen-openapiv2/BUILD.bazel:5:11: in go_library rule //protoc-gen-openapiv2:protoc-gen-openapiv2_lib: target '@org_golang_google_protobuf//internal/strs:strs' is not visible from target '//protoc-gen-openapiv2:protoc-gen-openapiv2_lib'. Check the visibility declaration of the former target if you think the dependency is legitimate

It doesn't look like a license violation to copy the above method, but copyright notice has to be copied as well to relevant places. Is this acceptable?

[johanbrandhorst]

Yes, that sounds good to me