Grpc-gateway: Cleaner Swagger-generated schema names (omitting prefixes)

[jgiles]

This issue resurrects some of the discussion in https://github.com/grpc-ecosystem/grpc-gateway/issues/525, and seeks a resolution to the problem of "ugly" prefixing without violating the principle of least surprise.

For the first time, we're planning to use grpc-gateway and its generated Swagger definitions to generate a public-facing API rather than an internal API. This means the "cosmetic" concern of prefixes we do not control on the generated Swagger schema names takes on new importance.

I've hacked together a bit of code that does "minimal" package prefixing as needed, but I think the conclusion in https://github.com/grpc-ecosystem/grpc-gateway/issues/525 is correct - more magic here is undesirable.

Short Term

In the short term, it would be great fix the simple common case where only messages from one proto package are rendered into the Swagger doc.

The current uniquification code here https://github.com/grpc-ecosystem/grpc-gateway/blob/9a7c952d3c7e6c0239ddb5d1d351e0fdb6c3c8d3/protoc-gen-swagger/genswagger/template.go#L542 is called on a bunch of fully-qualified proto names which do not actually get rendered into Swagger. In a build for my proto service with one endpoint, this is the list of names uniquified:

[]string{".google.protobuf.SourceCodeInfo.Location", ".grpc.gateway.protoc_gen_swagger.options.Operation.ResponsesEntry", ".google.protobuf.OneofDescriptorProto", ".google.protobuf.FileOptions", ".google.protobuf.Any", ".google.protobuf.DescriptorProto.ExtensionRange", ".google.protobuf.DescriptorProto.ReservedRange", ".google.protobuf.FieldDescriptorProto", ".google.protobuf.EnumDescriptorProto.EnumReservedRange", ".grpc.gateway.protoc_gen_swagger.options.Contact", ".grpc.gateway.protoc_gen_swagger.options.SecurityRequirement.SecurityRequirementValue", ".google.protobuf.FileDescriptorProto", ".google.protobuf.FieldOptions", ".google.protobuf.ServiceOptions", ".google.protobuf.GeneratedCodeInfo.Annotation", ".grpc.gateway.protoc_gen_swagger.options.Swagger.ResponsesEntry", ".grpc.gateway.protoc_gen_swagger.options.Info", ".grpc.gateway.protoc_gen_swagger.options.Schema", ".grpc.gateway.protoc_gen_swagger.options.SecurityDefinitions.SecurityEntry", ".grpc.gateway.protoc_gen_swagger.options.SecurityScheme", ".grpc.gateway.protoc_gen_swagger.options.SecurityRequirement.SecurityRequirementEntry", ".google.api.CustomHttpPattern", ".google.protobuf.ExtensionRangeOptions", ".google.protobuf.OneofOptions", ".google.protobuf.GeneratedCodeInfo", ".grpc.gateway.protoc_gen_swagger.options.Swagger", ".grpc.gateway.protoc_gen_swagger.options.ExternalDocumentation", ".grpc.gateway.protoc_gen_swagger.options.Tag", ".grpc.gateway.protoc_gen_swagger.options.SecurityRequirement", ".google.protobuf.EnumOptions", ".grpc.gateway.protoc_gen_swagger.options.Operation", ".grpc.gateway.protoc_gen_swagger.options.Response", ".grpc.gateway.protoc_gen_swagger.options.JSONSchema", ".paxos.pax.apiv0.GetBalanceResponse", ".google.api.Http", ".google.protobuf.FileDescriptorSet", ".google.protobuf.DescriptorProto", ".google.protobuf.EnumDescriptorProto", ".google.protobuf.MethodDescriptorProto", ".google.protobuf.MessageOptions", ".google.protobuf.SourceCodeInfo", ".grpc.gateway.protoc_gen_swagger.options.Scopes", ".google.api.HttpRule", ".google.protobuf.EnumValueDescriptorProto", ".google.protobuf.ServiceDescriptorProto", ".google.protobuf.EnumValueOptions", ".google.protobuf.MethodOptions", ".google.protobuf.UninterpretedOption", ".google.protobuf.UninterpretedOption.NamePart", ".grpc.gateway.protoc_gen_swagger.options.SecurityDefinitions", ".grpc.gateway.protoc_gen_swagger.options.Scopes.ScopeEntry", ".paxos.pax.apiv0.GetBalanceRequest", ".google.protobuf.FieldDescriptorProto.Label", ".google.protobuf.FieldOptions.CType", ".google.protobuf.MethodOptions.IdempotencyLevel", ".grpc.gateway.protoc_gen_swagger.options.SecurityScheme.Flow", ".google.protobuf.FieldDescriptorProto.Type", ".google.protobuf.FileOptions.OptimizeMode", ".google.protobuf.FieldOptions.JSType", ".grpc.gateway.protoc_gen_swagger.options.Swagger.SwaggerScheme", ".grpc.gateway.protoc_gen_swagger.options.JSONSchema.JSONSchemaSimpleTypes", ".grpc.gateway.protoc_gen_swagger.options.SecurityScheme.Type", ".grpc.gateway.protoc_gen_swagger.options.SecurityScheme.In"}

Only ".paxos.pax.apiv0.GetBalanceResponse" is actually rendered into Swagger.

In cases like this where only one package's messages (plus well-known types) are rendered into Swagger, we should drop the prefix (probably, by running the same uniquification code on the smaller list of rendered types).

Longer Term: Control Instead of Magic

It would be great to augment https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-swagger/options/openapiv2.proto so that users could explicitly declare prefixes on the packages they import. Then, rather than trying to automatically avoid conflicts, protoc-gen-swagger could check for conflicts, report them as errors, and prompt the user to disambiguate manually with a human-friendly prefix.

[johanbrandhorst]

Hi Josh, thanks for opening this issue. I've had the pleasure of generating public APIs with the gateway myself and appreciate your concerns. What can I do to help you bring these improvements into a PR?

[hypnoce]

https://github.com/grpc-ecosystem/grpc-gateway/pull/881 can help ? The PR helps in better readability and repeatability, ensuring unicity of names.

[jgiles]

Thanks for so quickly responding an merging a change!

I've tried this change out, and while it does make the names more predictable, the Swagger code generators I have tried pass the much-longer names on to the client code (e.g. generated Typescript from the resulting Swagger file has names like MyFullPackageMessageSubmessage).

It still seems like there should be a way to generate more concise names, especially for the simple case of only one package.

I've tested omitting the package name declaration entirely, and that appears to "work" for this simple case, though it feels wrong and likely boxes me in on importability.

[johanbrandhorst]

Reopening issue to discuss alternatives. I think a flag in the swagger generator to omit package namespaces might work?

[hypnoce]

Currently, the default behavior for swagger names is to traverse the depth of packages and inner message, check for unicity and generate a name that prepends one more package (because of an of-by-one error) :

for _, p := range messages {
    h := hierarchy(p)
    for depth := range h {
        if _, ok := packagesByDepth[depth]; !ok {
            packagesByDepth[depth] = make([][]string, 0)
        }
        packagesByDepth[depth] = append(packagesByDepth[depth], h[len(h)-depth - 1:])
    }
}

count := func(list [][]string, item []string) int {
        i := 0
    for _, element := range list {
        if reflect.DeepEqual(element, item) {
            i++
        }
    }
    return i
}
.....
h := hierarchy(p)
for depth := 0; depth < len(h); depth++ {
    if count(packagesByDepth[depth], h[len(h)-depth:]) == 1 {
        uniqueNames[p] = strings.Join(h[len(h)-depth-1:], "")
        break
    }
    if depth == len(h)-1 {
        uniqueNames[p] = strings.Join(h, "")
    }
}

First, it leads to a silent failure when depth == 0. Second, it prepends an unnecessary level. Also, it joins package and inner class names without . which can be a bit unreadable.
I would propose to remove the fqn_for_swagger_name boolean option to introduce a swagger_name_generation_strategy that can have values legacy, fqn, simple.
legacy: keep the current behavior with the bugs
fqn: alway take the full qualified domain name
simple: checks for unicity and join with . if needed for unicity.

What do you think ?

[johanbrandhorst]

Your analysis is sound, however I don't think we want to remove the new flag as we try to maintain strict backwards compatibility between releases. We still plan on making a V2 at some point where this kind of change could happen.

Obviously you've still hit on a problem and proposed a solution which sounds good, but maybe let's just add another flag for now 😅? I'd like to explore the appetite for making this "simple" proposal the new default instead as well since I'm not sure swagger file output necessarily needs to be backwards compatible in case of bugs.

@achew22 @ivucica your thoughts on the two options I've presented?

[hypnoce]

@johanbrandhorst makes sense not to support backward compatibility of bugs. So a first step would be to fix the name generator. There are 2 main issues :

• of-by-one bug that leads to more package being prepended than actually needed for unicity
• the elements are joined without .. So instead of generating foo.bar.Message, we endup with foobarMessage

What do you think of fixing those 2 issues as a first step ?

[johanbrandhorst]

Yeah I think both of those sound good, but I invited my fellow collaborators to express their views as well before we go ahead with this change. They may know of some reason why we're not using dot-separation, for example. It's not so clear cut as to call that behaviour a bug, necessarily.

[ivucica]

Well, I don't have a major objection introducing a new flag. But I would mark the old flag as deprecated, give a warning for a bit, and eventually remove it. Not immediately. We don't want to be "that project" breaking people's build systems without ample notice.

And changing the default without ample notice is also something not to be done lightly. Client libraries (and maybe even servers -- not everyone who wants to define an API using protobufs will want gRPC in the picture) would break and people would be rightly upset if we went and changed generated method and message names willy-nilly.

So a flag is a must, removal of a flag is only-after-deprecating-the-old-one, and addressing "prettiness of generated names" is a yes-please.

Suggestion to use dots in method and message names (if I am interpreting that correctly) confuses me. Is that allowed in OpenAPIv2? Can you give concrete examples of an input, current output and new output so it's easier to understand the proposal, rather than think about what exactly "one-off" or "fqn" means in this context? How would each flag behave?

[johanbrandhorst]

Small note: the fqn_for_swagger_name flag was only introduced _last week_, though it has made it into a release.

[ivucica]

I just tried it out with editor.swagger.io and, curiously, dots are fully acceptable and even ~correctly handled by the Go code generator. So with a flag, I have no problem with this.

I'd still love to see exact proposed examples of the changes with various flag values.

[johanbrandhorst]

@hypnoce It seems the concensus for now is to hide this behind a second flag. If you want to submit a PR, you could implement your swagger_name_generation_strategy proposal. We could then phase out the use of fqn_for_swagger_name as stated. Thanks!

[hypnoce]

Scenario : I have a package foo.bar and 2 messages MyMessage and MyNestedMessage

Available as of 1.8.0 :

• Current behavior (no flag): names will be as follow : barMyMessage and MyMessageMyNestedMessage even if MyNestedMessage is already unique. In case the unicity cannot be ensured, traverse the fully qualified name upwards until the generated swagger name is unique.
• fqn_for_swagger_name flag : taking the same example, names will be generated as follow : foo.bar.MyMessage and foo.bar.MyMessage.MyNestedMessage always joining with .

Still in discussion :

• with the off-by-one fix (no flag) : MyMessage MyNestedMessage will be generated. If unicity is not ensured, any message will be prepended with previous names in the fqn hierarchy, without ., ie MyMessageMyNestedMessage.
• with . joining (behind a flag or not) : all joining will use . as delimiter to ensure, when unicity is broken, that names are still kind of readable.

So I was thinking to introduce a naming strategy configuration

• legacy : default in grpc-gateway 1.x.x. Does not change the current behavior.
• fqn : similar to fqn_for_swagger_name=true
• third one (simple?) : removes the current off-by-one bug and always join with .

Would be happy to help deprecate the flag I introduced in favor of a naming strategy.

What do you think ?

[johanbrandhorst]

Sounds good to me!

[ivucica]

+1

[ashutshkumr]

Hi, thank you for tracking this. Did the first fix mentioned under "Still in discussion" ever make it to a release ? I'm still hitting following on v1.14.5.

Current behavior (no flag): names will be as follow : barMyMessage and MyMessageMyNestedMessage even if MyNestedMessage is already unique. In case the unicity cannot be ensured, traverse the fully qualified name upwards until the generated swagger name is unique

[johanbrandhorst]

Don't think anything like this has been implemented

[bconway]

Was anything similar to this tackled prior to v2? I believe I'm still seeing the off-by-one behavior in v2.0.1 (with default flag for FQN) and would like... not to.

[johanbrandhorst]

I still don't think anyone has actually done the work for this, so tackling it now with v2 would still be welcome 😄.