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

Use collectionFormat multi for query params of repeated fields #902

Merged

Conversation

bmperrea
Copy link
Contributor

Solves #756 . Also formats protoc-gen-swagger/genswagger/template_test.go according to go fmt.

Solves grpc-ecosystem#756 . Also formats protoc-gen-swagger/genswagger/template_test.go according to go fmt.
@achew22
Copy link
Collaborator

achew22 commented Mar 10, 2019

Looks like really great work! FYI, I think the way to fix the generate CI failure is to run

docker run -v $(pwd):/go/src/github.com/grpc-ecosystem/grpc-gateway --rm jfbrandhorst/grpc-gateway-build-env:1.12 \
    /bin/bash -c 'cd /go/src/github.com/grpc-ecosystem/grpc-gateway && \
        make realclean && \
        make examples SWAGGER_CODEGEN="${SWAGGER_CODEGEN}"'

which will regenerate the examples. I'm really looking forward to this!

@bmperrea
Copy link
Contributor Author

bmperrea commented Mar 10, 2019

Thanks @achew22 ! A docker run to generate these files will be great - I was having trouble figuring out how to get the right version of swagger-codegen (2.2.2).

When I run the command you posted I get

can't load package: package github.com/golang/protobuf/protoc-gen-go: cannot find package "github.com/golang/protobuf/protoc-gen-go" in any of:
	/usr/local/go/src/github.com/golang/protobuf/protoc-gen-go (from $GOROOT)
	/go/src/github.com/golang/protobuf/protoc-gen-go (from $GOPATH)

I have protoc-gen-go installed on my local, but I guess the docker run is missing a dependency.

@johanbrandhorst
Copy link
Collaborator

That's very strange, this is the command run by CI when generating the diff that's failing. Are you sure you ran the command correctly?

@johanbrandhorst
Copy link
Collaborator

Ah, there is a problem with the mount path in Andrews link. Try this:

docker run -v $(pwd):/src/grpc-gateway --rm jfbrandhorst/grpc-gateway-build-env:1.12 \ /bin/bash -c 'cd /src/grpc-gateway && \         make realclean && \         make examples SWAGGER_CODEGEN="${SWAGGER_CODEGEN}"'

@johanbrandhorst
Copy link
Collaborator

FYI the source of truth for this command is CONTRIBUTING.md

@johanbrandhorst
Copy link
Collaborator

(Andrews command is how we used to run it before we switched to go modules).

@codecov-io
Copy link

Codecov Report

Merging #902 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #902      +/-   ##
==========================================
+ Coverage   53.59%   53.61%   +0.02%     
==========================================
  Files          39       39              
  Lines        3935     3937       +2     
==========================================
+ Hits         2109     2111       +2     
  Misses       1629     1629              
  Partials      197      197
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/types.go 19.04% <ø> (ø) ⬆️
protoc-gen-swagger/genswagger/template.go 56.42% <100%> (+0.08%) ⬆️

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 9bb0d96...30a13e5. Read the comment docs.

@johanbrandhorst
Copy link
Collaborator

Note to self: tests are a bit flaky, we should consider increasing the wait for the server to start up.

@johanbrandhorst johanbrandhorst merged commit 2b6cab6 into grpc-ecosystem:master Mar 10, 2019
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

@johanbrandhorst
Copy link
Collaborator

We should make a release with this new functionality

@mmarod
Copy link

mmarod commented Mar 13, 2019

I believe that this may have inadvertently broken spec...

  "definitions": {
    "Foo": {
      "type": "object",
      "properties": {
        "foo": {
          "type": "array",
          "items": {
            "$ref": "#/definitions/Bar"
          },
          "description": "foo",
          "collectionFormat": "multi"
        }
...

This spits out the following error when I run swagger generate server

- definitions.Foo.properties.foo.collectionFormat in body is a forbidden property

@johanbrandhorst
Copy link
Collaborator

Thanks for your report @mmarod, could you raise an issue please? That is unfortunate.

@johanbrandhorst
Copy link
Collaborator

My immediate thought is that is happens when we have repeated fields in the body of a request instead of in the query parameters of the URL. Might just need a little extra logic.

@johanbrandhorst
Copy link
Collaborator

I raised #906

@gomisc
Copy link

gomisc commented Mar 13, 2019

I have same trouble when do swagger spec validation...
Fix please guys. Thanks

Look here
OAI/OpenAPI-Specification#419 (comment)

@johanbrandhorst
Copy link
Collaborator

Since that makes two of you that have spoken up I'm going to revert this until we can fix #906 at the same time.

johanbrandhorst added a commit that referenced this pull request Mar 13, 2019
#902)"

This reverts commit 2b6cab6. It was found
to have introduced a swagger spec violation, see #906.
johanbrandhorst added a commit that referenced this pull request Mar 13, 2019
#902)"

This reverts commit 2b6cab6. It was found
to have introduced a swagger spec violation, see #906.
@johanbrandhorst
Copy link
Collaborator

I've reverted the change in master and will make another release.

adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
…ecosystem#902)

* Use collectionFormat multi for query params of repeated fields

Fixes grpc-ecosystem#756 . Also formats protoc-gen-swagger/genswagger/template_test.go according to go fmt.
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
grpc-ecosystem#902)"

This reverts commit 2b6cab6. It was found
to have introduced a swagger spec violation, see grpc-ecosystem#906.
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.

7 participants