Cleaner Swagger-generated schema names (omitting prefixes) #877
Comments
|
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? |
|
#881 can help ? The PR helps in better readability and repeatability, ensuring unicity of names. |
|
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. |
|
Reopening issue to discuss alternatives. I think a flag in the swagger generator to omit package namespaces might work? |
|
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 What do you think ? |
|
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? |
|
@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 :
What do you think of fixing those 2 issues as a first step ? |
|
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. |
|
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? |
|
Small note: the |
|
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. |
|
@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 |
|
Scenario : I have a package Available as of 1.8.0 :
Still in discussion :
So I was thinking to introduce a naming strategy configuration
Would be happy to help deprecate the flag I introduced in favor of a naming strategy. What do you think ? |
|
Sounds good to me! |
|
+1 |
|
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.
|
|
Don't think anything like this has been implemented |
|
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. |
|
I still don't think anyone has actually done the work for this, so tackling it now with v2 would still be welcome 😄. |
|
@johanbrandhorst I gave it a shot, following a suggestion that was posted here earlier: #1968 |
|
This should be fixed in #2310 |
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
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.
The text was updated successfully, but these errors were encountered: