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

Added support for path param repeated fields #742

Conversation

maros7
Copy link

@maros7 maros7 commented Aug 30, 2018

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 like GET /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 and tsv. 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.

@maros7
Copy link
Author

maros7 commented Aug 30, 2018

All Travis jobs succeded except one with a failure not related to this PR. It says HTTP request sent, awaiting response... 403 Forbidden when fetching swagger-codegen.

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.

Fantastic work! Just a few comments, but looks great!

protoc-gen-grpc-gateway/descriptor/registry.go Outdated Show resolved Hide resolved
protoc-gen-grpc-gateway/descriptor/types.go Outdated Show resolved Hide resolved
protoc-gen-swagger/main.go Outdated Show resolved Hide resolved
runtime/convert.go Outdated Show resolved Hide resolved
@johanbrandhorst
Copy link
Collaborator

johanbrandhorst commented Sep 1, 2018

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.

@johanbrandhorst
Copy link
Collaborator

johanbrandhorst commented Sep 1, 2018

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 dep ensure --vendor-only and go install ./vendor/github.com/golang/protobuf/protoc-gen-go etc to get the correct versions.

@maros7
Copy link
Author

maros7 commented Sep 1, 2018

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.

@maros7
Copy link
Author

maros7 commented Sep 1, 2018

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.

@maros7
Copy link
Author

maros7 commented Sep 1, 2018

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.

@johanbrandhorst
Copy link
Collaborator

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
@maros7 maros7 force-pushed the repeated-field-path-parameter-support branch from d468f93 to 9dc2ea3 Compare September 1, 2018 21:59
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@0e59487). Click here to learn what that means.
The diff coverage is 13.29%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #742   +/-   ##
=========================================
  Coverage          ?   53.38%           
=========================================
  Files             ?       30           
  Lines             ?     3368           
  Branches          ?        0           
=========================================
  Hits              ?     1798           
  Misses            ?     1393           
  Partials          ?      177
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/types.go 19.04% <ø> (ø)
runtime/convert.go 16.41% <0%> (ø)
protoc-gen-swagger/main.go 26.13% <0%> (ø)
protoc-gen-grpc-gateway/descriptor/types.go 41.08% <0%> (ø)
protoc-gen-grpc-gateway/descriptor/registry.go 62.03% <16%> (ø)
protoc-gen-swagger/genswagger/template.go 38.69% <39.13%> (ø)
protoc-gen-grpc-gateway/gengateway/template.go 60.6% <80%> (ø)
protoc-gen-grpc-gateway/descriptor/services.go 73.97% <80%> (ø)

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 0e59487...9dc2ea3. Read the comment docs.

@maros7
Copy link
Author

maros7 commented Sep 1, 2018

Rebased and squashed. All tests pass now. Thanks!

@johanbrandhorst johanbrandhorst merged commit ab0345b into grpc-ecosystem:master Sep 1, 2018
@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.

4 participants