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

Allow explicit empty security definition to overwrite existing definitions #666

Merged
merged 2 commits into from Jun 23, 2018

Conversation

Projects
None yet
6 participants
@co3k
Contributor

co3k commented Jun 9, 2018

It enables to clear default security configuration in operationObject.Security (to make public api).

protoc-gen-swagger doesn't allow empty array (and it causes segfault) in operationObject.Security but it is really needed resetting security requirement configuration by Swagger.

@googlebot googlebot added the cla: yes label Jun 9, 2018

@co3k

This comment has been minimized.

Show comment
Hide comment
@co3k

co3k Jun 9, 2018

Contributor

I wonder why the https://travis-ci.org/grpc-ecosystem/grpc-gateway/builds/390065893 build fails in Go: 1.10.x, GATEWAY_PLUGIN_FLAGS= job. Can anyone advice me to solve this problem?

Contributor

co3k commented Jun 9, 2018

I wonder why the https://travis-ci.org/grpc-ecosystem/grpc-gateway/builds/390065893 build fails in Go: 1.10.x, GATEWAY_PLUGIN_FLAGS= job. Can anyone advice me to solve this problem?

@yugui

This comment has been minimized.

Show comment
Hide comment
@yugui

yugui Jun 10, 2018

Member

The test failure looks to be because some of example files are out-of-date. c.f. golang/protobuf@05f48f4#diff-8c603013608023320d5242916c4ea03bR1571

Would you mind running make clean && make examples to let the files catch the change in protoc-gen-go?

Member

yugui commented Jun 10, 2018

The test failure looks to be because some of example files are out-of-date. c.f. golang/protobuf@05f48f4#diff-8c603013608023320d5242916c4ea03bR1571

Would you mind running make clean && make examples to let the files catch the change in protoc-gen-go?

@co3k

This comment has been minimized.

Show comment
Hide comment
@co3k

co3k Jun 10, 2018

Contributor

@yugui
Thank you for your response, but I can't get any diffs by running that command on my macOS.

$ go version
go version go1.10.3 darwin/amd64

$ protoc --version
libprotoc 3.5.1

$ brew info protobuf
protobuf: stable 3.5.1 (bottled), HEAD
Protocol buffers (Google's data interchange format)
https://github.com/google/protobuf/
/usr/local/Cellar/protobuf/3.2.0_1 (257 files, 15.9MB)
  Poured from bottle on 2017-04-28 at 18:02:34
/usr/local/Cellar/protobuf/3.5.1_1 (267 files, 18.5MB) *
  Poured from bottle on 2018-06-11 at 00:22:59
From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/protobuf.rb
==> Dependencies
Build: autoconf ✔, automake ✔, libtool ✔
Recommended: python@2 ✔
Optional: python ✘
==> Options
--with-python
	Build with python support
--with-test
	Run build-time check
--without-python@2
	Build without python2 support
--HEAD
	Install HEAD version
==> Caveats
Editor support and examples have been installed to:
  /usr/local/opt/protobuf/share/doc/protobuf

$ cd $GOPATH/src/github.com/golang/protobuf/protoc-gen-go; git log -1 --oneline; cd -
05f48f4 (HEAD -> master, origin/master, origin/HEAD) proto: revert UTF-8 validation for proto2 (#628)

$ make clean && make examples
rm -f bin/protoc-gen-grpc-gateway bin/protoc-gen-swagger
go build -o bin/protoc-gen-grpc-gateway github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway
**SNIP**

$ git status
On branch allow-empty-array-in-security
nothing to commit, working tree clean

$ make clean && make test
rm -f bin/protoc-gen-grpc-gateway bin/protoc-gen-swagger
go build -o bin/protoc-gen-grpc-gateway github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway
**SNIP**

$ git status             
On branch allow-empty-array-in-security
nothing to commit, working tree clean
Contributor

co3k commented Jun 10, 2018

@yugui
Thank you for your response, but I can't get any diffs by running that command on my macOS.

$ go version
go version go1.10.3 darwin/amd64

$ protoc --version
libprotoc 3.5.1

$ brew info protobuf
protobuf: stable 3.5.1 (bottled), HEAD
Protocol buffers (Google's data interchange format)
https://github.com/google/protobuf/
/usr/local/Cellar/protobuf/3.2.0_1 (257 files, 15.9MB)
  Poured from bottle on 2017-04-28 at 18:02:34
/usr/local/Cellar/protobuf/3.5.1_1 (267 files, 18.5MB) *
  Poured from bottle on 2018-06-11 at 00:22:59
From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/protobuf.rb
==> Dependencies
Build: autoconf ✔, automake ✔, libtool ✔
Recommended: python@2 ✔
Optional: python ✘
==> Options
--with-python
	Build with python support
--with-test
	Run build-time check
--without-python@2
	Build without python2 support
--HEAD
	Install HEAD version
==> Caveats
Editor support and examples have been installed to:
  /usr/local/opt/protobuf/share/doc/protobuf

$ cd $GOPATH/src/github.com/golang/protobuf/protoc-gen-go; git log -1 --oneline; cd -
05f48f4 (HEAD -> master, origin/master, origin/HEAD) proto: revert UTF-8 validation for proto2 (#628)

$ make clean && make examples
rm -f bin/protoc-gen-grpc-gateway bin/protoc-gen-swagger
go build -o bin/protoc-gen-grpc-gateway github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway
**SNIP**

$ git status
On branch allow-empty-array-in-security
nothing to commit, working tree clean

$ make clean && make test
rm -f bin/protoc-gen-grpc-gateway bin/protoc-gen-swagger
go build -o bin/protoc-gen-grpc-gateway github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway
**SNIP**

$ git status             
On branch allow-empty-array-in-security
nothing to commit, working tree clean
@co3k

This comment has been minimized.

Show comment
Hide comment
@co3k

co3k Jun 11, 2018

Contributor

The error has been fixed at d1985fc .
I might need to run make realclean && make examples for solving this problem.
Thanks again to yugui!

The https://travis-ci.org/grpc-ecosystem/grpc-gateway/jobs/390576866 job for Go: 1.9.x, GATEWAY_PLUGIN_FLAGS= has been timed out.
Is it a temporary error? But I cannot retry this job because I don't have permission to do so.

Contributor

co3k commented Jun 11, 2018

The error has been fixed at d1985fc .
I might need to run make realclean && make examples for solving this problem.
Thanks again to yugui!

The https://travis-ci.org/grpc-ecosystem/grpc-gateway/jobs/390576866 job for Go: 1.9.x, GATEWAY_PLUGIN_FLAGS= has been timed out.
Is it a temporary error? But I cannot retry this job because I don't have permission to do so.

@tmc

This comment has been minimized.

Show comment
Hide comment
@tmc

tmc Jun 17, 2018

Collaborator

@co3k thanks for your contribution, can you rebase this so CI runs again?

Collaborator

tmc commented Jun 17, 2018

@co3k thanks for your contribution, can you rebase this so CI runs again?

@co3k

This comment has been minimized.

Show comment
Hide comment
@co3k

co3k Jun 20, 2018

Contributor

@tmc Thanks for your response but the https://travis-ci.org/grpc-ecosystem/grpc-gateway/jobs/394549559#L554-L557 test is failed that is due to timed out in go get phase. I cannot rebase it again because the upstream is not updated yet. How should I do next?

Contributor

co3k commented Jun 20, 2018

@tmc Thanks for your response but the https://travis-ci.org/grpc-ecosystem/grpc-gateway/jobs/394549559#L554-L557 test is failed that is due to timed out in go get phase. I cannot rebase it again because the upstream is not updated yet. How should I do next?

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 20, 2018

Collaborator

I have re-triggered the job.

Collaborator

johanbrandhorst commented Jun 20, 2018

I have re-triggered the job.

@@ -105,7 +105,7 @@ type swaggerOperationObject struct {
Tags []string `json:"tags,omitempty"`
Deprecated bool `json:"deprecated,omitempty"`
Security []swaggerSecurityRequirementObject `json:"security,omitempty"`
Security *[]swaggerSecurityRequirementObject `json:"security,omitempty"`

This comment has been minimized.

@johanbrandhorst

johanbrandhorst Jun 20, 2018

Collaborator

Is this required? Can't we just have an empty slice?

@johanbrandhorst

johanbrandhorst Jun 20, 2018

Collaborator

Is this required? Can't we just have an empty slice?

This comment has been minimized.

@co3k

co3k Jun 21, 2018

Contributor

Required. The omitempty tag removes an empty slice. However we don't want to print null value in every JSON response so we can't remove this omitempty tag.

@co3k

co3k Jun 21, 2018

Contributor

Required. The omitempty tag removes an empty slice. However we don't want to print null value in every JSON response so we can't remove this omitempty tag.

This comment has been minimized.

@johanbrandhorst

johanbrandhorst Jun 21, 2018

Collaborator

OK

@johanbrandhorst

johanbrandhorst Jun 21, 2018

Collaborator

OK

} else {
newSecurity = operationObject.Security
if operationObject.Security != nil {
newSecurity = *operationObject.Security
}

This comment has been minimized.

@johanbrandhorst

johanbrandhorst Jun 20, 2018

Collaborator

Can we not just set newSecurity = nil when operationObject.Security == nil?

@johanbrandhorst

johanbrandhorst Jun 20, 2018

Collaborator

Can we not just set newSecurity = nil when operationObject.Security == nil?

This comment has been minimized.

@co3k

co3k Jun 21, 2018

Contributor

I think we really need empty slice in this case.

@co3k

co3k Jun 21, 2018

Contributor

I think we really need empty slice in this case.

This comment has been minimized.

@johanbrandhorst

johanbrandhorst Jun 21, 2018

Collaborator

OK

@johanbrandhorst

johanbrandhorst Jun 21, 2018

Collaborator

OK

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 21, 2018

Collaborator

As with the other PR, could you please squash the commits? Thank you.

Collaborator

johanbrandhorst commented Jun 21, 2018

As with the other PR, could you please squash the commits? Thank you.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 21, 2018

Collaborator

Could you also please reword this title, description and commit message to clearly state the intent please? Something along the lines of:

Allow explicit empty security definition to overwrite existing definitions.

Collaborator

johanbrandhorst commented Jun 21, 2018

Could you also please reword this title, description and commit message to clearly state the intent please? Something along the lines of:

Allow explicit empty security definition to overwrite existing definitions.

co3k added some commits Jun 8, 2018

Allow explicit empty security definition to overwrite existing defini…
…tions.

It enables to clear default security configuration in operationObject.Security (to make public api).
protoc-gen-swagger doesn't allow empty array (and it causes segfault) in operationObject.Security but it is really needed
resetting security requirement configuration by Swagger.

@co3k co3k changed the title from Enable to clear default security configuration in operationObject.Security (to make public api) to Allow explicit empty security definition to overwrite existing definitions Jun 23, 2018

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 23, 2018

Codecov Report

Merging #666 into master will decrease coverage by 0.07%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
- Coverage   56.49%   56.42%   -0.08%     
==========================================
  Files          30       30              
  Lines        3004     3006       +2     
==========================================
- Hits         1697     1696       -1     
- Misses       1144     1146       +2     
- Partials      163      164       +1
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/types.go 19.04% <ø> (ø) ⬆️
protoc-gen-swagger/genswagger/template.go 38.47% <0%> (+0.19%) ⬆️
runtime/errors.go 37.66% <0%> (-7.41%) ⬇️
runtime/marshal_json.go 83.33% <0%> (+16.66%) ⬆️

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 11bef10...dcd2104. Read the comment docs.

codecov-io commented Jun 23, 2018

Codecov Report

Merging #666 into master will decrease coverage by 0.07%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
- Coverage   56.49%   56.42%   -0.08%     
==========================================
  Files          30       30              
  Lines        3004     3006       +2     
==========================================
- Hits         1697     1696       -1     
- Misses       1144     1146       +2     
- Partials      163      164       +1
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/types.go 19.04% <ø> (ø) ⬆️
protoc-gen-swagger/genswagger/template.go 38.47% <0%> (+0.19%) ⬆️
runtime/errors.go 37.66% <0%> (-7.41%) ⬇️
runtime/marshal_json.go 83.33% <0%> (+16.66%) ⬆️

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 11bef10...dcd2104. Read the comment docs.

@co3k

This comment has been minimized.

Show comment
Hide comment
@co3k

co3k Jun 23, 2018

Contributor

@johanbrandhorst Thanks for your advice! I've squashed commits and changed the title of this pull request and the first commit to follow your suggestion.

Contributor

co3k commented Jun 23, 2018

@johanbrandhorst Thanks for your advice! I've squashed commits and changed the title of this pull request and the first commit to follow your suggestion.

@johanbrandhorst johanbrandhorst merged commit ee3ef70 into grpc-ecosystem:master Jun 23, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 23, 2018

Collaborator

Thank you for your contribution!

Collaborator

johanbrandhorst commented Jun 23, 2018

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment