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

add support for field_behavior options on swagger plugin #1806

Merged
merged 10 commits into from
Nov 14, 2020

Conversation

ewhauser
Copy link
Contributor

References to other Issues or PRs

There is a discussion in #669 about adding support for x-nullable using FieldBehavior.OPTIONAL. While I haven't implemented that behavior here yet, it should be relatively trivial to add.

Brief description of what is fixed or changed

This adds support for using google.api.field_behavior options to specify whether fields are required or output only. The syntax is a little nicer than grpc.gateway.protoc_gen_swagger.options.openapiv2_schema because you can define the values on the individual fields as opposed to the message level.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really interesting, thanks for the PR! I'd like to understand the consequences of this change, as it may change the expectations of the users. For example:

  • Should we add any handling to protoc-gen-grpc-gateway of these annotations? What would that mean?
  • Adding this file as a dependency of the library runs the risk of annoying all current users who do not have field_behavior.proto vendored (as it wasn't a reqiurement), is this going to break users that don't vendor this file? From a cursory reading I don't think it should since we're just checking if the extensions are set.

Also, could we update one of the fields in examples/internal/proto/a_bit_of_everything.proto to add some of these descriptions to show users how they're used?

Thanks!

protoc-gen-openapiv2/internal/genopenapi/template.go Outdated Show resolved Hide resolved
@ewhauser
Copy link
Contributor Author

This is really interesting, thanks for the PR! I'd like to understand the consequences of this change, as it may change the expectations of the users. For example:

  • Should we add any handling to protoc-gen-grpc-gateway of these annotations? What would that mean?

You could certainly add runtime checking for some of the options - REQUIRED, INPUT_ONLY, OUTPUT_ONLY - but I don't believe gateway does anything with the existing required or readonly fields at runtime today? It would have to be an opt in feature because it could potentially be a breaking change for people who documented that behavior but didn't enforce it.

I think it's a reasonable discussion but probably would be a separate issue.

  • Adding this file as a dependency of the library runs the risk of annoying all current users who do not have field_behavior.proto vendored (as it wasn't a reqiurement), is this going to break users that don't vendor this file? From a cursory reading I don't think it should since we're just checking if the extensions are set.

Confirmed that there is no compile time dependency on field_behavior.proto.

Also, could we update one of the fields in examples/internal/proto/a_bit_of_everything.proto to add some of these descriptions to show users how they're used?

Will do.

Thanks!

@ewhauser ewhauser changed the title add support for field_behavior optons on swagger plugin add support for field_behavior options on swagger plugin Nov 12, 2020
@ewhauser
Copy link
Contributor Author

@johanbrandhorst - I think we are all set here.

In relation to the original runtime behavior question, I think there are a couple options for anyone who wants automatic runtime validation against the Swagger spec:

  1. Integrate with go-openapi which has an existing validator for OpenAPI schemas: https://godoc.org/github.com/go-openapi/validate#hdr-Validating_a_schema. This would have to be done at the HTTP level since it's validates against JSON which wouldn't be ideal for performance reasons since you'd be parsing the request twice.
  2. Code generate validators against the Swagger spec (a la https://github.com/mwitkow/go-proto-validators or https://github.com/envoyproxy/protoc-gen-validate) and use validator middleware to validate at runtime.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, one small nit, but I have a final request - now that we've added this behaviour, lets write a little docs page about how to use it! Our docs pages are in docs/docs/. We could either add it to "customizing your gateway" (though that page is getting very big) or consider adding a new one for customizing the swagger output (we should probably have a page for this anyway!).

Thanks for your patience!

protoc-gen-openapiv2/internal/genopenapi/template.go Outdated Show resolved Hide resolved
@ewhauser
Copy link
Contributor Author

I'll update the docs sometime in the next few days.

@codecov-io
Copy link

codecov-io commented Nov 13, 2020

Codecov Report

Merging #1806 (8d9358d) into master (1f6e7b0) will increase coverage by 0.06%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1806      +/-   ##
==========================================
+ Coverage   57.32%   57.39%   +0.06%     
==========================================
  Files          34       34              
  Lines        3616     3652      +36     
==========================================
+ Hits         2073     2096      +23     
- Misses       1282     1290       +8     
- Partials      261      266       +5     
Impacted Files Coverage Δ
...otoc-gen-openapiv2/internal/genopenapi/template.go 58.84% <58.33%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f6e7b0...8d9358d. Read the comment docs.

@ewhauser
Copy link
Contributor Author

ewhauser commented Nov 13, 2020

@johanbrandhorst I've opened another PR (#1822) to cover the docs piece since the changes were a bit beyond the scope of just field behavior support. Figured we could get this merged and iterate on any docs changes there.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, SGTM!

@johanbrandhorst johanbrandhorst merged commit 58f6deb into grpc-ecosystem:master Nov 14, 2020
@johanbrandhorst
Copy link
Collaborator

Thanks a ton for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants