protoc-gen-swagger: support generating a Swagger definition with no schemes

[ailurarctos]

It would be nice if grpc-gateway supported generating a Swagger definition with no schemes key. According to the openapiv2 spec this means the following:

If the schemes is not included, the default scheme to be used is the one used to access the Swagger definition itself.

This is currently not possible because the default schemes is ["http","https"] and this default is only overridden when there is at least one scheme sepecified.

I'm willing go contribute a change for this but I'd like some guidance on how to proceed. I can think of a few ways to do this:

1. Filter out UNKNOWN values and thus make the list [UNKNOWN] mean no schemes. This is kind of hacky. Arguably it would be better for protoc-gen-swagger to error out if it hits UNKNOWN since it is the fallback for unknown enum values. Right now UNKNOWN will be written to the definition as "unknown" which is not valid.
2. Add a new enum value (e.g. EMPTY) that explicitly means there are no schemes.
3. Use another field to specify that schemes is set to disable the default value. E.g. (bool schemes_set = 105).

Or if there is another way I didn't think of let me know!

[johanbrandhorst]

Thanks for raising this, seems a reasonable suggestion. I think in this case it probably wouldn't be harmful to change the default from ["http", "https"] to [], since functionally it should mean, at worst, that we use https instead of http when the schema is served over https, right?

I think changing the default should be reasonably straightforward, but let me know if you need anymore pointers.

CC @ivucica

[ailurarctos]

Changing the default to [] sounds good to me. This works nicely with the default for host (which is to leave it absent) since that means to use the host serving the Swagger definition.

There is a chance this could break existing users if they somehow rely on the schemes key being set to ["http", "https"]. As you said if they serve the definition over HTTP or HTTPS then it's probably not a problem since those are both in the default list. If they read the definition from a file I guess it could be an issue (since no schemes can be automatically determined).

[johanbrandhorst]

You have reminded me about a slight concern - what about code generators? The spec says to use the scheme which the schema is accessed on, but for generators that will likely be the file system itself. Could you maybe look into how a few popular static swagger API generators handle an empty scheme? If they all handle an empty scheme well then I think we can go ahead with that change.

[ailurarctos]

I'm not sure which code generators are the most popular but I'll try these and report back:

• https://github.com/swagger-api/swagger-codegen
• https://github.com/OpenAPITools/openapi-generator
• https://github.com/go-swagger/go-swagger

Let me know if I missed one you think is important.

[johanbrandhorst]

Those 3 sound perfect 👍.

[ailurarctos]

I tested the code generators. All of them were able to generate code when loading a definition having no schemes from the filesystem.

• Swagger Codegen (2.4.9) - The generator will default to HTTPS. This is different from when schemes is ["http","https"] (it will choose HTTP).
• OpenAPI Generator (4.1.3) - The generator will default to HTTP. This is the same as when schemes is ["http","https"].
• go-swagger (v0.21.0) - The generator will default the schemes to ["http"]. This is different in that HTTPS is no longer in the list. This will cause clients to use HTTP instead of HTTPS, since the go-openapi runtime chooses HTTPS if available.

I only tested client code generation; I assume that if someone is using grpc-gateway then they are using it for their server.

What do you think?

[johanbrandhorst]

The go-swagger behaviour is somewhat concerning, since it's unsafe by default. This may be something we can raise with the go-swagger project though and shouldn't be a blocker for this change. I'm still happy for us to change the default, since it shouldn't, according to the spec, imply unsafe by default. Let me know if you need any help with the pull request. Could we raise an issue with the go-swagger project as well to see if they can change their behaviour (or we can change it for them 🙂)?

[ailurarctos]

Thanks @johanbrandhorst! I made a pull request. Let me know if I missed something or need to change anything.

Could we raise an issue with the go-swagger project as well to see if they can change their behaviour (or we can change it for them 🙂)?

Sure! I'll make an issue and reference this one from it.