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: does not verify that json_name does not conflict #5063

Closed
dsnet opened this issue Aug 21, 2018 · 6 comments · Fixed by #10581
Closed

protoc: does not verify that json_name does not conflict #5063

dsnet opened this issue Aug 21, 2018 · 6 comments · Fixed by #10581
Assignees

Comments

@dsnet
Copy link
Contributor

dsnet commented Aug 21, 2018

Using protoc v3.6.1.

Compile this file:

syntax = "proto3";
message M {
	string f1 = 1 [json_name = "fooBar"];
	string f2 = 2 [json_name = "fooBar"];
}

What I see: compile succeeds.
What I expect: compilation failure since fooBar is specified twice.

@xfxyjwf xfxyjwf self-assigned this Aug 21, 2018
@dsnet dsnet changed the title protoc does not verify that json_name does not conflict protoc: does not verify that json_name does not conflict Jul 3, 2019
@jhump
Copy link
Contributor

jhump commented Sep 18, 2020

Is there appetite for reviewing and accepting a fix for this and for #7192, if I were to open a pull request?

@acozzette
Copy link
Member

@jhump Sure, I can review the pull request if you would like to work on it.

@jhump
Copy link
Contributor

jhump commented Sep 28, 2022

@mcy, @mkruskal-google, can this be reopened since the fix was reverted in #10657?

@mkruskal-google
Copy link
Member

I think you can roll both of these forward. Part of the issue is that our full test suite wasn't even run over your initial PR. The other part of the problem was that downstream code (both internal and external) was broken by it. That's not necessarily a blocker, but it does mean we need to be more careful about merging it in

@jhump
Copy link
Contributor

jhump commented Oct 3, 2022

@mkruskal-google, to make sure I understand your comment above: are you suggesting I open a new PR with this same fix, and that it simply needs to be merged more carefully next time (like running more tests and possibly requiring some internal changes before those tests all pass)?

@mkruskal-google
Copy link
Member

Yes exactly! If you can get some information from jaysachs about why that one failed in kythe it would be helpful. But part of the problem is that our tests weren't even fully run on your PRs, so it's likely they'll show some of the issues. I can also manually port it into Google to check it internally

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