Properly omit wrappers and google.protobuf.empty from swagger definitions

[birdayz]

• swaggerSchemaObject.Properties is now optional (pointer), because
  google.protobuf.Empty requires us to set Properties{} instead of nil, because
  this equals in Swagger to an empty JSON object (which is exactly what Empty
  represents and how the gateway treats it). If it's not a pointer, we can't
  distinguish between "not set" (in most cases, we don't want Properties to be
  set, instead usually just .Ref is set), and "set to an empty value e.g.
  Properties{} with length 0). We don't want Properties{} to occur except for
  specific cases, e.g. for Empty.
• Wrappers and Empty are not rendered as definitions, now also for RPC Method
  input/output. instead, all necessary schema information is provided "in-line"
  via schema.type and schema.properties.
• Empty is now omitted as well if it's input/output of a RPC Method.

[codecov-io]

Codecov Report

Merging #801 into master will increase coverage by 4.15%.
The diff coverage is 48.48%.

@@            Coverage Diff             @@
##           master     #801      +/-   ##
==========================================
+ Coverage   49.32%   53.47%   +4.15%     
==========================================
  Files          39       30       -9     
  Lines        3751     3370     -381     
==========================================
- Hits         1850     1802      -48     
+ Misses       1723     1389     -334     
- Partials      178      179       +1

Impacted Files	Coverage Δ
protoc-gen-swagger/genswagger/types.go	19.04% <ø> (ø)	⬆️
protoc-gen-swagger/genswagger/template.go	39.12% <48.48%> (+0.59%)	⬆️
examples/server/unannotatedecho.go
examples/server/a_bit_of_everything.go
examples/server/responsebody.go
examples/server/echo.go
examples/server/fieldmask_helper.go
examples/server/flow_combination.go
utilities/readerfactory.go
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b7aa47...ed0dd91. Read the comment docs.

[birdayz]

One thing i noticed:
the message names in template.go/renderMessagesAsDefinition() are highly inconsistent. I printed some out, and it looks like this:

.com.eon.abc
.com.eon.def
protobufBoolValue
Pet
protobufEmpty

It looks strange. I digged a bit and only the enums are prefixed with the dot (findNestedMessagesAndEnumerations(msg, reg, m, e))
The others look like those above. And well, this switch ist mostly ineffective i think:

	for name, msg := range messages {

		switch name {
		case ".google.protobuf.Timestamp":
			continue
		case ".google.protobuf.Duration":
			continue
		case ".google.protobuf.StringValue":

I just tested, with my addition it didn't work, since it's called protobufEmpty and protobufBoolValue instead of .google.protobuf.Empty.

Looks like this has once worked, but something went wrong.. Idea how to proceed?

[johanbrandhorst]

Sounds like at the very least we need some tests to establish the expected behaviour. If they fail, we'll have to implement some fixes. Would you be interested on adding these tests?

[birdayz]

Will do. These things are screwing up my swagger-ui, so i'm very interested in cleaning up where needed :)

[birdayz]

Okay it's quite difficult to get a test up and running. Aren't there tests where i can just provide a .proto file as input and a .json as output? That would make it much easier to get started. To be honest i spent more time to get a test working than i feel should be necessary..

I added an example with BoolValue to the examples, and this occurs in the output:

    "protobufBoolValue": {
      "type": "object",
      "properties": {
        "value": {
          "type": "boolean",
          "format": "boolean",
          "description": "The bool value."
        }
      },
      "description": "Wrapper message for `bool`.\n\nThe JSON representation for `BoolValue` is JSON `true` and `false`."
    },

Shouldn't be there, we don't want those wrappers in the list of models. So at some point i guess .google.protobuf.BoolValue became protobufBoolValue and the switch became useless.

I'm not going to commit this example because i got a lot of strange changes after re-generating with the docker image of the contributor guide.. Just added one Service, but now i have a totally unrelated diff:

deleted:    examples/clients/abe/camel_case_service_name_api.go

I only added:

service BoolService {
        rpc Empty(google.protobuf.BoolValue) returns (google.protobuf.BoolValue) {
                option (google.api.http) = {
                        get: "/v2/example/empty",
                };
        }
}

I'm a little stuck with the tooling .. or could use some help to set up a test. It's just non-trivial to get all the inputs set up, especially with multiple proto files. Is there some helper method to just load a proto file in a test, and get the descriptors etc? would really help me.

[johanbrandhorst]

Deleting files implies that not all generators ran. Curious findings indeed. Generally the way to test generated files like this is to compare against a "golden", known correct file. So it wouldn't be a "go test" kind of test, it'd be a shell scripting kind of test.

[birdayz]

Yes this is what i would have expected, but such tests are not in place it seems.
Fun fact i recently fixed a bug on godoctor and they use exactly this kind of tests: https://github.com/godoctor/godoctor/tree/master/refactoring/testdata/rename/072-funcdecl-rename

They even run it from go for coverage etc : )
But i'm not sure if i have the time to add this to the project.
How could we proceed, or how could a lightweight test for this look like?

edit: to some extent the examples do this, because you fail the CI if there's a diff detected.

[johanbrandhorst]

Yes, but the examples didn't pick up the change in well known types. Maybe just adding a message using all the well known types is enough?

[birdayz]

ok we're getting closer, at least the examples now reproduce it. see the latest diff 2cf7065 .

apparently this issue only occurs if these datatypes are used directly as method in or output.

there must be some piece of code where these types are added 'differently (different names)' if they are direct method in/out. i'll have another shot after work today.

edit2:
Okay. On template.go:188

func findNestedMessagesAndEnumerations(message *descriptor.Message, reg *descriptor.Registry, m messageMap, e enumMap) 

Nested uses the format with the full qualified name (e.g. .google.protobuf.Empty). So for nested values, the names are (at least internally) used like this.

The other way at func findServicesMessagesAndEnumerations does differently, it pre-processes the string with fullyQualifiedNameToSwaggerName. which "camel cases" it.
So i wonder if we can homogenize this behavior, because the same thing is written into the same map differently, depending on where it was found. i hope i described it good enough.

so choices:

a) keep it as it is and add the same checks for well-known types into the switch at renderMessagesAsDefinition twice, once for the "fully qualified format" and once for the camelCase format
b) use only camel case
c) use only fully qualified

I guess this has significant implications on the generated json, so i'll have to try it out. Maybe someone with more knowledge of the codebase can elaborate :)

[birdayz]

Implemented c). At least from the examples, the result looks good. In the end, all types are converted to the camel case version anyway, so it's a good idea to not do it before as well. This way, the filtering of well-known types works as expected.

[johanbrandhorst]

@birdayz fantastic work, really appreciate the effort you're putting in right now. Is there anything else you want to add to this PR before we review it?

[johanbrandhorst]

Right, I think we should just try and add a step where we generate some swagger files from the wrappers.proto as well. The generated pb.go now contains the types but I can't see a swagger file?

[birdayz]

I'm confused, what do you mean?
The purpose of this PR is that these very basic things - boolValue and empty etc- are omitted from the definitions section of the swagger JSON. So they should not be in the .json file. that's why 2cf7065 adds them (reproducing the faulty behavior) and after the fix they are gone (82c2cdf)

[johanbrandhorst]

Ok, if you want we can just resolve that in this PR and raise an issue about making sure that the generated swagger files are correctly formatted for all the well known types. I think it ought to be part of this PR to ensure that the bool and empty types are correctly formatted though. Is that already covered with existing examples? Otherwise lets just add swagger generated file of the wrappers.proto example.

[birdayz]

Yep it's fully covered now. So there should not be a need for a separate issue, the wrappers are now handled correctly. At least from my PoV, if these two points are valid:

• Wrappers are intentionally not included in the description section of the swagger config.
• In addition, this PR introduces the same behavior for google.protobuf.Empty. It's not part of the wrappers proto file of google, but IMHO should still be omitted as it provides no value at all.

The current code already does the stuff highlighted in bold, but only for in/out messages of RPC methods. Nested messages still produced those entries, even with a different name. This was fixed with 84895aa

edit: the .swagger file is generated and up-to-date: https://github.com/birdayz/grpc-gateway/blob/dont_render_well-known_empty/examples/proto/examplepb/wrappers.swagger.json

[johanbrandhorst]

Right the thing that's still confusing me is that this PR adds a number of methods to wrappers.proto in the examples, but there are no generated swagger file changes, only pb.go changes. Is that correct? Should we not generate some swagger content for these new methods too?

[birdayz]

I think we have a misunderstanding here!
Of course it's in the .pb.go file, because this is just the go part. It should be there.

However for the swagger part, the swagger generation code deliberately chooses to NOT generate those things - for wrappers. So this is expected and intentional that it's NOT in the .json output!

See this part of the code in template.go:

func renderMessagesAsDefinition(messages messageMap, d swaggerDefinitionsObject, reg *descriptor.Registry, customRefs refMap) {
	for name, msg := range messages {
		switch name {
		case ".google.protobuf.Timestamp":
			continue
		case ".google.protobuf.Duration":
			continue
		case ".google.protobuf.StringValue":
			continue
		case ".google.protobuf.BytesValue":
			continue
		case ".google.protobuf.Int32Value":
			continue
		case ".google.protobuf.UInt32Value":
			continue
		case ".google.protobuf.Int64Value":
			continue
		case ".google.protobuf.UInt64Value":
			continue
		case ".google.protobuf.FloatValue":
			continue
		case ".google.protobuf.DoubleValue":
			continue
		case ".google.protobuf.BoolValue":
			continue
		case ".google.protobuf.Empty":
			continue
		}
(continue means, don't put it into the definitions section of the JSON)

This was already in place before this PR. It only affects the swagger output, not the go bindings .pb.go output. It was just buggy before my changes, for a specific subset of inputs.

the generated stuff is up to date, ci does a check for that and fails in case it's not up to date.

[johanbrandhorst]

Not generating the types as the proto types is all well and good, but we're talking about 10ish new RPCs and no corresponding methods added to the swagger files at all. I'm wondering if it's simply because you didn't add the google.api.http option to the RPCs? I would still expect the methods to be reflected in a swagger files.

[birdayz]

Ah now i got your concerns..
Most likely you are right. I can add an http annotation later to be sure.

[johanbrandhorst]

Great, again, thanks for your hard work here! Really appreciated.

[birdayz]

I added one google.api.http directive, does this look better now?
from my side we are good to go. i suppose, for the future, we could integrate the examples as a real test.

[johanbrandhorst]

Well slightly worryingly the string well known type is not showing up as the native string, is that not wrong?

[johanbrandhorst]

Also why not add all the others as well?

[birdayz]

i just had a look and i'm not sure if we can just omit those definitions, because clearly the path entries reference them.

            "schema": {
              "$ref": "#/definitions/protobufStringValue"
            }

so i suspect it would not even be a valid swagger. i don't know, i'll put it back to WIP for the moment until i've figured out if this ok or a better solution.

[johanbrandhorst]

I think ideally a google.protobuf.StringValue type would render as a string in swagger, without the message definition. I thought that was what these type exceptions were for.

In any case, can we add google.api.http options for the other RPCs as well please and check those renderings too?

Don't hesitate to push back, I'm happy to resolve things in separate PRs if you think its going out of scope.

[birdayz]

my assumption is correct, this change is not legit without fixing the underlying behavior.

• Apparently for in/out of a method, the StringValue (same applies for other wrappers) object is generated , instead of the primitive. (for both the generated code and swagger).
• Simply removing this entry from the definitions because it looks ugly, as this PR tries so far, results in a faulty swagger.json; Reproduce by going to https://petstore.swagger.io and entering https://raw.githubusercontent.com/birdayz/grpc-gateway/dont_render_well-known_empty/examples/proto/examplepb/wrappers.swagger.json . It will show an error

 
Resolver error at paths./v1/test.post.responses.200.schema.$ref
Could not resolve reference: Could not resolve pointer: /definitions/protobufStringValue does not exist in document
Resolver error at paths./v1/test.post.parameters.0.schema.$ref
Could not resolve reference: Could not resolve pointer: /definitions/protobufStringValue does not exist in document

So, we can't proceed this way..well, i think we could at least add a fix for the "Empty" thing, because it does really render nothing.

But, in general, i agree with you: Should wrappers be used in the REST interface? clearly, clearly not. It directly shows that this is a grpc-gateway generated API and makes it look WAY less "clean". But to address this, which should certainly be possible, we would break backwards compatibility. Newly generated gateways will have a different API if they use wrappers in in/out. Maybe we can discuss this in a new issue if you want. I'm curious how you handle backwards compatibility guarantees, or how we can introduce such changes (e.g. going for a new major release).

Anyway, i'll clean up this PR and try to reduce it to a minimum for the "Empty" problem, and move the rest to a new one. Thanks for all the support.

[johanbrandhorst]

@birdayz agreed, let's discuss the faulty behaviour separately and make a decision on what to do next. Great digging!

[birdayz]

@johanbrandhorst i've updated the PR accordingly, with changes for both Empty and Wrappers. Those two are very close (and have many common problems, like the handling of most well known types).

It's squashed & rebased. I'm ready for the new code review round, if you have time. Thanks!

[johanbrandhorst]

LGTM, but let's have a look by @ivucica as well.

[birdayz]

How can we move this forward? I make heavy use of well known types; and them being present in the definitions section of swagger is quite ugly. ;)

i'd like also to improve the same situation with google.protobuf.struct; but i won't invest time into it unless i see that this comparable PR goes through :)

[johanbrandhorst]

I apologize for this falling between the chairs, I'll give @ivucica another few days but if I hear nothing I'll just merge it. Thanks for the bump!

[johanbrandhorst]

It looks like you'll need to rebase again though, could you do that please?

[birdayz]

done

[johanbrandhorst]

Thanks for your contribution!