Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation: 'base_path' Swagger attribute confuses users #918

Closed
nu11ptr opened this issue Apr 4, 2019 · 5 comments · Fixed by #919
Closed

Documentation: 'base_path' Swagger attribute confuses users #918

nu11ptr opened this issue Apr 4, 2019 · 5 comments · Fixed by #919

Comments

@nu11ptr
Copy link
Contributor

nu11ptr commented Apr 4, 2019

Steps you follow to reproduce the error:

  1. I start with a working REST API and with properly generating Swagger definitions
  2. I added a 'base_path' attribute to the 'openapiv2_swagger' annotation in my proto file
  3. I removed the equivalent base_path prefix from each 'google.api.http' annotation (base_path = "/api/v1")
  4. I regenerated my Go code and restarted my API
  5. All my REST APIs now return 404 errors

What did you expect to happen instead:

I expected the API to remain working

What's your theory on why it isn't working:

The generated code was previously generating this:

pattern_Provider_EncryptClientSecret_0 = runtime.MustPattern(runtime.NewPattern(1, []int{2, 0, 2, 1, 2, 2, 2, 3}, []string{"api", "v1", "provider", "client_secret_encrypter"}, ""))

It is now generating this:

pattern_Provider_EncryptClientSecret_0 = runtime.MustPattern(runtime.NewPattern(1, []int{2, 0, 2, 1}, []string{"provider", "client_secret_encrypter"}, ""))

It seems like the generator is not aware of the 'base_path' attribute and expects the 'google.api.http' to hold the entire path. However, adding this path back on (while retaining 'base_path' attribute) has the impact of:

  1. Adding that path to all endpoints displayed in the Swagger UI which makes it very 'noisy' and hard to read
  2. Breaking the Swagger UI "try it out" feature which doubles up the 'base_path' be prefixing base_path to all operations
@johanbrandhorst
Copy link
Collaborator

johanbrandhorst commented Apr 4, 2019

I see why this is confusing, but the swagger annotations in general change nothing about how the go files are generated. The base path is for configuring your swagger annotations when you're not serving the grpc-gateway on /, so those paths would otherwise be incorrect.

I think the gateway is behaving correctly here, but we could clearly do with adding some better documentation. Would you be interested in contributing some documentation updates for the base_path variable?

@johanbrandhorst johanbrandhorst changed the title 'base_path' Swagger attribute breaks APIs Documentation: 'base_path' Swagger attribute confuses users Apr 4, 2019
@nu11ptr
Copy link
Contributor Author

nu11ptr commented Apr 4, 2019

I'm am trying to understand in what use case using 'base_path' would yield correct behavior. AFAIK, every swagger tool is going to prepend 'base_path' to all API paths, so if path in 'google.api.http' is absolute, but swagger assumes when basePath is present generated paths are relative - this seems like it would always result incorrect code from the Swagger that breaks. In short, I don't see any way to generate correct swagger when using 'base_path' - maybe i'm just missing it.

It seems like the valid 'fixes' here would be:

  1. Removing 'base_path' as an option (don't like this one)
  2. Modifying generator to respect base_path and treat paths as relative
  3. Adding another swagger attribute, that when present, is used to generate swagger path (is there one? couldn't find)
  4. Workaround: Add a route for basepath that strips basepath prefix before sending to servemux

@nu11ptr
Copy link
Contributor Author

nu11ptr commented Apr 4, 2019

Thinking more on this, perhaps you are suggesting # 4 is how we should work with base_path in general? I suspect this would work just fine for me, but yes, that is confusing. Not against updating the docs to indicate that, but since I never found docs on base_path, wouldn't know where to update.

@johanbrandhorst
Copy link
Collaborator

I think the following is true:

  1. The google.api.http paths are relative to the path which the runtime.ServeMux is served on. This defaults to /, so the paths are easy to confused as absolute.
  2. The swagger annotation is for cases where the runtime.ServeMux is not served on /, but perhaps something like /api/. In order to make the generated swagger file line up with what the API is exposed under, the base path must be specified.
  3. The swagger annotations affect only the swagger output, and nothing about the generated go files - this is unlikely to change.

I think the best place to add some documentation about the base path is in the proto definition:

. We could also add a section to the docs about configuring the base path, and when you may want to do that.

How does that sound?

@nu11ptr
Copy link
Contributor Author

nu11ptr commented Apr 4, 2019

Works for me - thanks for taking the time to explain

johanbrandhorst pushed a commit that referenced this issue Jun 29, 2019
* Add base_path docs to explain behavior

* Add to 'base_path' description

* Regenerated Go code from protobuf

* Revert go.mod to previous version

Closes #918
adasari pushed a commit to adasari/grpc-gateway that referenced this issue Apr 9, 2020
* Add base_path docs to explain behavior

* Add to 'base_path' description

* Regenerated Go code from protobuf

* Revert go.mod to previous version

Closes grpc-ecosystem#918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants