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

Change FieldMask OpenAPI type to string. #1820

Merged
merged 4 commits into from Nov 17, 2020
Merged

Change FieldMask OpenAPI type to string. #1820

merged 4 commits into from Nov 17, 2020

Conversation

zoido
Copy link
Contributor

@zoido zoido commented Nov 13, 2020

References to other Issues or PRs

RESOLVES #1816

Have you read the Contributing Guidelines?

Yes

Brief description of what is fixed or changed

  • sets type for FieldMask to string
  • adjust test expectations accordingly
  • update examples generated code accordingly
  • updates patch.md doc to contains the corect format for the FieldMask in the example
    • also changes the case to smallCamel that is the what the Marshaler accepts

@google-cla google-cla bot added the cla: yes label Nov 13, 2020
@zoido zoido changed the title Chage FieldMAsk OpenAPI type to string. Chage FieldMask OpenAPI type to string. Nov 13, 2020
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.

Hi! Thanks for your contribution, could you confirm that this works when using it with the generated swagger file? Thanks!

@johanbrandhorst
Copy link
Collaborator

Related to #1816

@vtolstov
Copy link

vtolstov commented Nov 14, 2020 via email

@codecov-io
Copy link

Codecov Report

Merging #1820 (3d984a8) into master (1f6e7b0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1820   +/-   ##
=======================================
  Coverage   57.32%   57.32%           
=======================================
  Files          34       34           
  Lines        3616     3616           
=======================================
  Hits         2073     2073           
  Misses       1282     1282           
  Partials      261      261           
Impacted Files Coverage Δ
...otoc-gen-openapiv2/internal/genopenapi/template.go 58.68% <ø> (ø)

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...3d984a8. Read the comment docs.

@zoido zoido changed the title Chage FieldMask OpenAPI type to string. Change FieldMask OpenAPI type to string. Nov 15, 2020
@johanbrandhorst johanbrandhorst marked this pull request as ready for review November 15, 2020 10:28
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.

Hi! This looks fine, could you just confirm that this fixes your issue before I merge it?

@zoido
Copy link
Contributor Author

zoido commented Nov 15, 2020

@johanbrandhorst Hi. It fixes it for us. Would you recommend the place where to include some kind of test to make sure it's covered?

I have also spotted that patch.md doc contains the example that seems to use even older format. I'd like also adjust test that before I mark this as ready for review.

@johanbrandhorst
Copy link
Collaborator

I don't think we ever actually encourage the user to specify a field mask, we automatically create the field mask from input JSON when possible, so the input format has never been tested. If you want to add a test for this explicit specification, it'd make sense to add it to the integration tests in https://github.com/grpc-ecosystem/grpc-gateway/blob/master/examples/internal/integration/integration_test.go#L801. Looks like we rely on the marshaler, maybe a new test which does it explicitly would make sense.

@johanbrandhorst johanbrandhorst marked this pull request as draft November 15, 2020 10:56
@zoido zoido marked this pull request as ready for review November 17, 2020 10:37
@zoido
Copy link
Contributor Author

zoido commented Nov 17, 2020

@johanbrandhorst Checked and updated the patch.md. PTAL.

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.

Small question on the new docs.

docs/docs/mapping/patch.md Outdated Show resolved Hide resolved
@johanbrandhorst johanbrandhorst merged commit bdca789 into grpc-ecosystem:master Nov 17, 2020
@johanbrandhorst
Copy link
Collaborator

Thanks 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.

generate_unbound_methods option and FieldMask fields don't work together.
4 participants