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: validate custom json_name configuration #10581

Merged
merged 7 commits into from
Sep 22, 2022

Conversation

jhump
Copy link
Contributor

@jhump jhump commented Sep 14, 2022

This makes a few other related changes, for consistency:

  • Adds the existing checks, for default JSON names, to run even for proto2 files, but only as warnings (symmetry with similar check for enum value names).
  • Tweaks the similar check for enum value names to include "This is not allowed in proto3", for clarity, since it's only a warning in proto2.

When checking custom JSON names, this considers it an error even in proto2 if two json_name options conflict. However, if a json_name option conflicts with a default JSON name, it is just a warning in proto2 (aligns with the existing check and the similar check for enum value names).

Fixes #5063.

- also, include check for default JSON name conflicts even in proto2
  files (but only warn)
- if custom JSON name conflicts with other default name, only a
  warning in proto2
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
Copy link
Contributor Author

@jhump jhump 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 the feedback! I'll ping here again after I push revisions to address them.

src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
Copy link
Contributor Author

@jhump jhump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcy, I think this is ready for another look.

src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
@jhump
Copy link
Contributor Author

jhump commented Sep 16, 2022

ping @mcy, if you have a chance to make another pass over this

src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved
@jhump jhump force-pushed the jh/custom-json-name-validation branch from d5b14c8 to c7ba055 Compare September 21, 2022 16:56
@jhump
Copy link
Contributor Author

jhump commented Sep 21, 2022

@mcy, I think I've addressed your latest comments. PTAL.

@mcy mcy merged commit d3b5389 into protocolbuffers:main Sep 22, 2022
mkruskal-google added a commit to mkruskal-google/protobuf that referenced this pull request Sep 26, 2022
…-json-name-validation"

This reverts commit d3b5389, reversing
changes made to bcd1755.
mkruskal-google added a commit that referenced this pull request Sep 26, 2022
…dation" (#10657)

This reverts commit d3b5389, reversing
changes made to bcd1755.
bithium pushed a commit to bithium/protobuf that referenced this pull request Sep 4, 2023
…me-validation

protoc: validate custom json_name configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

protoc: does not verify that json_name does not conflict
5 participants