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

Camel case backward compatible in Swagger doc #988

Merged
merged 2 commits into from
Aug 10, 2019
Merged

Camel case backward compatible in Swagger doc #988

merged 2 commits into from
Aug 10, 2019

Conversation

xin-au
Copy link
Contributor

@xin-au xin-au commented Aug 10, 2019

Hi @johanbrandhorst,
In the PR, #985, the snake case arguments will be converted to lower camel case when the json_names_for_fields is enabled.
Now, I noticed that there is a backward compatible use case may need to support, which is / could be inside {}, for example, "/{user.name=prefix/*}:customMethod". (Sorry I did not notice that)
Also, when calculating jsonSnakeCaseName, if the name includes /, then it could cause out of range potentially. So, fixed it too.
Sorry for about.

@codecov-io
Copy link

Codecov Report

Merging #988 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #988      +/-   ##
==========================================
+ Coverage   53.43%   53.45%   +0.01%     
==========================================
  Files          40       40              
  Lines        4027     4028       +1     
==========================================
+ Hits         2152     2153       +1     
  Misses       1676     1676              
  Partials      199      199
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/template.go 54.27% <100%> (+0.04%) ⬆️

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 b6e6efb...11586e0. Read the comment docs.

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.

Do these new tests cover the case that caused an index out of range error too?

@xin-au
Copy link
Contributor Author

xin-au commented Aug 10, 2019

@johanbrandhorst , yes, it covered the test case (/ inside {})
In TestFQMNtoSwaggerName and TestTemplateToSwaggerPath with enabling SetUseJSONNamesForFields by setting it as true, which means to enable the JSON camle case, otherwise it is false by default. The following links are the tests with SetUseJSONNamesForFields(true)
https://github.com/grpc-ecosystem/grpc-gateway/pull/988/files#diff-20461b822a903046234d0fa5073ba2c4R924
https://github.com/grpc-ecosystem/grpc-gateway/pull/988/files#diff-20461b822a903046234d0fa5073ba2c4R1009

@johanbrandhorst johanbrandhorst merged commit 86e5755 into grpc-ecosystem:master Aug 10, 2019
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

@xin-au
Copy link
Contributor Author

xin-au commented Aug 10, 2019

Thank you so much @johanbrandhorst !

@xin-au xin-au deleted the xin/swagger-camel-case-back-compatible branch August 10, 2019 15:56
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
* Avoid potential out of range and support the convention that  could be inside

* Add some test cases for testing backward compatible
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.

4 participants