Cleaner Swagger-generated schema names (omitting prefixes)

[jgiles]

This issue resurrects some of the discussion in #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 #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

grpc-gateway/protoc-gen-swagger/genswagger/template.go

Line 542 in 9a7c952

func resolveFullyQualifiedNameToSwaggerNames(messages []string) map[string]string {

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]

#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?