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 flag 'allow_repeated_fields_in_body' to protoc-gen-swagger #853

Merged
merged 2 commits into from
Jan 17, 2019

Conversation

abice
Copy link
Contributor

@abice abice commented Jan 17, 2019

PR #712 provided support for repeated fields in the body of a response to protoc-gen-gateway, but failed to add it to protoc-gen-swagger.

I simply copied the flag over and updated the registry, and it handles the swagger generation for the same scenario.

syntax = "proto3";

package v1;

import "google/api/annotations.proto";

service Service {
  rpc ListStrings(ListStringsRequest) returns (ListStringsResponse) {
    option (google.api.http) = {
      get : "/v1/strings"
      response_body : "values"
    };
  }
}

message ListStringsRequest {}
message ListStringsResponse { repeated string values = 1; }

@codecov-io
Copy link

codecov-io commented Jan 17, 2019

Codecov Report

Merging #853 into master will decrease coverage by 0.16%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #853      +/-   ##
==========================================
- Coverage   51.83%   51.66%   -0.17%     
==========================================
  Files          39       39              
  Lines        3764     3782      +18     
==========================================
+ Hits         1951     1954       +3     
- Misses       1629     1643      +14     
- Partials      184      185       +1
Impacted Files Coverage Δ
examples/server/responsebody.go 0% <0%> (ø) ⬆️
protoc-gen-swagger/main.go 26.8% <50%> (+1.52%) ⬆️

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 20e8cf9...e76e705. 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.

Thanks for this, but I'm a little confused, this doesn't seem to change the output of the swagger file. I don't know if this accomplishes anything at the moment? If it does change behaviour, could you add an example of how to make use of this new functionality to a_little_bit_of_everything.proto in examples/proto?

@abice
Copy link
Contributor Author

abice commented Jan 17, 2019

I asked that question in the slack room, this change does change the output of the swagger file, but it requires a CLI parameter in order to trigger the change, and I was having trouble getting the bazel part to run with the added flag required. I can go back and check to see what bazel stuff I need to change in order to pass in the flag, but I'm unfamiliar with bazel, and it might take a bit.

@johanbrandhorst
Copy link
Collaborator

I see.. could you add a new file (e.g. the snippet in your PR) that we can generate with the relevant CLI flag and add it to the Makefile?

@johanbrandhorst
Copy link
Collaborator

Ignore Bazel, I don't know if it's relevant in this case, the generation all goes through the Makefile AFAIK.

@abice
Copy link
Contributor Author

abice commented Jan 17, 2019

Well, it's less bazel, and more the circle step of running bazel... I had updated the response_body_service.proto to include a sample since it was related, but ran into problems... I'll add a new proto that just requires that flag.

@johanbrandhorst
Copy link
Collaborator

What was the problems with that proto if I might ask? I hope the generation step is easy enough? A lot of effort has gone into making this process simpler so let me know if there's anything we can improve.

@abice
Copy link
Contributor Author

abice commented Jan 17, 2019

Well, my first attempt was to just change the circleci config to add the SWAGGER_PLUGIN_FLAGS and GATEWAY_PLUGIN_FLAGS to the make examples line like:

GATEWAY_PLUGIN_FLAGS=allow_repeated_fields_in_body=true SWAGGER_PLUGIN_FLAGS=allow_repeated_fields_in_body=true make examples SWAGGER_CODEGEN="${SWAGGER_CODEGEN}" # Set in Docker image

and then generated everything like it says in the contributing.md

but when I ran the bazel job locally using circleci local execute --job bazel, it failed on

/go/src/github.com/grpc-ecosystem/grpc-gateway/examples/proto/examplepb/BUILD.bazel:39:1: Generating into bazel-out/k8-fastbuild/bin/examples/proto/examplepb/linux_amd64_race_stripped/examplepb_go_proto%/github.com/grpc-ecosystem/grpc-gateway/examples/proto/examplepb failed (Exit 1) go-protoc failed: error executing command bazel-out/host/bin/external/io_bazel_rules_go/go/tools/builders/linux_amd64_stripped/go-protoc -protoc bazel-out/host/bin/external/com_google_protobuf/protoc -importpath ... (remaining 86 argument(s) skipped)

Maybe I missed that it was looking at the BUILD.bazel file in the examplepb folder. But I dont' know where I would add the extra flag in that file.

@johanbrandhorst
Copy link
Collaborator

I can't help with that error I'm afraid (@achew22?) but perhaps it will be easier to add a new file 😅.

@achew22
Copy link
Collaborator

achew22 commented Jan 17, 2019

I think adding a new option to the compiler is the way to go.

    options = ["logtostderr=true"],

Becomes

options = [
    "your_new=option",
    "logtostderr=true",
]

options = ["logtostderr=true"],

@abice
Copy link
Contributor Author

abice commented Jan 17, 2019

Awesome, thanks! I'll add that and see where I get

Also:
* Added integration tests
* Added an extra cli test for parsing the flag
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, this makes sense now!

@johanbrandhorst johanbrandhorst merged commit a0500cb into grpc-ecosystem:master Jan 17, 2019
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
* protoc-gen-swagger: add flag 'allow_repeated_fields_in_body'
Also added/updated tests for flags

* protoc-gen-swagger: updated response body example
Also:
* Added integration tests
* Added an extra cli test for parsing the flag
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.

5 participants