-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Added support for path param repeated fields #742
Added support for path param repeated fields #742
Conversation
All Travis jobs succeded except one with a failure not related to this PR. It says |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! Just a few comments, but looks great!
I reran the jobs as well and there seems to be one error: https://travis-ci.org/grpc-ecosystem/grpc-gateway/jobs/422808821. EDIT: That build error really doesn't look like it's your fault... let me investigate a bit. |
Update on the build failure: I think it must be something with your generation still, I reran master to see if it was failing on there but it passed: https://travis-ci.org/grpc-ecosystem/grpc-gateway/builds/422220357 (unrelated failure). Make sure you generate the files with the same versions used to generate the files previously. You can run |
Hmm, ok. Travis ran successfully on first commit, except the swagger codegen download thingy. Then I just added a missing test. But I will look into it. |
I removed bin/ and vendor/, then re-ran tests (which also re-generates examples and runs dep/go install protoc) with make -B test. No errors here. |
Travis ran with same result as master. I.e. one job ended with the same, unrelated failure: 0.32s$ test "${USE_BAZEL}" = true || sh -c 'cd examples/browser && node ./node_modules/gulp/bin/gulp'
module.js:538
throw err;
^
Error: Cannot find module 'bower-logger'
at Function.Module._resolveFilename (module.js:536:15)
at Function.Module._load (module.js:466:25)
at Module.require (module.js:579:17)
at require (internal/module.js:11:18)
at Object.<anonymous> (/home/travis/gopath/src/github.com/grpc-ecosystem/grpc-gateway/examples/browser/node_modules/bower/lib/commands/index.js:2:14)
at Module._compile (module.js:635:30)
at Object.Module._extensions..js (module.js:646:10)
at Module.load (module.js:554:32)
at tryModuleLoad (module.js:497:12)
at Function.Module._load (module.js:489:3)
The command "test "${USE_BAZEL}" = true || sh -c 'cd examples/browser && node ./node_modules/gulp/bin/gulp'" exited with 1. |
Master has been fixed, please rebase and let's hope the tests pass. |
Added conversion functions for repeated path params Updated bytes converter to support URL encoded base64 strings Added support for repeated primitive path params in protoc-gen-grpc-gateway Added support for repeated primitive path params in protoc-gen-swagger Added --repeated_path_param_separator cmd line param to support setting separator to `csv`, `ssv`, `pipes` and `tsv` Re-generated examples Added ABitOfEverythingRepeated to validate repeated path parameter functionality Added GetRepeatedQuery rpc endpoint Updated browser tests to test GetRepeatedQuery rpc endpoint Updated integration tests to test GetRepeatedQuery rpc endpoint Added GetRepeatedQuery to ABitOfEverythingServer implementation Added missing reflect.DeepEqual test Change separator type from string to rune Fixed slice duplication in string slice conversion method Reverted --allowRepeatedFieldsInBody in swagger generator Changed TODO of bytes slice proto2 function Corrected if-statement releated to repeated fields in resolveFieldPath function Rebase
d468f93
to
9dc2ea3
Compare
Codecov Report
@@ Coverage Diff @@
## master #742 +/- ##
=========================================
Coverage ? 53.38%
=========================================
Files ? 30
Lines ? 3368
Branches ? 0
=========================================
Hits ? 1798
Misses ? 1393
Partials ? 177
Continue to review full report at Codecov.
|
Rebased and squashed. All tests pass now. Thanks! |
Thanks for your contribution! |
Related to #741
Now it is possible to have repeated fields in the request, e.g. a message with
repeated string id = 1;
and a RPC service/orders/{id}
will make it possible to do a HTTP call likeGET /orders/<id>,<id>,...
. By default the plugin will generate code that expects separation by comma. But I also added support via a command line parameter to specify other separators supported as per OpenAPI 2.0. I.e.csv
,ssv
,pipes
andtsv
. Please note that the swagger-codegen doesn't generate a proper go client. I noted this in the issue I created. But the javascript client, used in the browser tests, works as expected.