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

protoc-gen-swagger: check typeIndex when typeName is Method #833

Merged
merged 3 commits into from
Dec 20, 2018

Conversation

hexfusion
Copy link
Contributor

isProtoPathMatches() doesn't return false if svcIdx != methIdx when typeName == Method. The results in comments belonging to other services polluting swagger spec. We noticed this regression when using protoc 3.5.1+, although I still can't say why it started.

Fixes: #746

Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@johanbrandhorst
Copy link
Collaborator

Hi @hexfusion, thanks a bunch for the contribution! Any chance you could add some new test casee for this to prevent future regressions? Thanks!

Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@johanbrandhorst
Copy link
Collaborator

It seems the generator changes caused a diff in one of the generated files. Is that expected? If so, you'll need to rerun the generator (see CONTRIBUTING.md).

@hexfusion
Copy link
Contributor Author

@johanbrandhorst looking at the source of the proto I believe the change is expected, will update.

Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@johanbrandhorst
Copy link
Collaborator

Looks great, thanks!

@johanbrandhorst johanbrandhorst merged commit 11787b1 into grpc-ecosystem:master Dec 20, 2018
@hexfusion
Copy link
Contributor Author

/cc @gyuho

@hexfusion hexfusion deleted the fx_gw746 branch December 20, 2018 19:27
@hexfusion
Copy link
Contributor Author

@johanbrandhorst just curious about general time-frame when we might expect this to be released. Thanks!

@johanbrandhorst
Copy link
Collaborator

A new release you mean? We don't currently have a set release schedule, I can see if we're waiting for anything in particular but I might just release 1.6.4 tomorrow.

@hexfusion
Copy link
Contributor Author

that would be fantastic, and greatly appreciated.

johanbrandhorst added a commit that referenced this pull request Jun 13, 2019
This reverts the changes introduced in #833. It caused
a regression in the swagger generator allow_merge behaviour.

Fixes #923
@johanbrandhorst
Copy link
Collaborator

FYI @hexfusion I will be backing out these changes, it seems they caused #923, please see #924. Please feel free to open another PR reverting the revert where we can add some more tests to ensure we get the best of both worlds.

johanbrandhorst added a commit that referenced this pull request Jun 13, 2019
This reverts the changes introduced in #833. It caused
a regression in the swagger generator allow_merge behaviour.

Fixes #923
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
This reverts the changes introduced in grpc-ecosystem#833. It caused
a regression in the swagger generator allow_merge behaviour.

Fixes grpc-ecosystem#923
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.

Comments of rpc method gets copied if multiple services are present in a proto file.
3 participants