protoc: validate custom json_name configuration (take 2) by jhump · Pull Request #10750 · protocolbuffers/protobuf

jhump · 2022-10-10T14:15:37Z

This is a re-do of #10581. Merging this PR previously caused issues when importing the change into Google's internal repo (IIUC). So additional testing is needed before attempting to merge again. cc @mkruskal-google

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.

mkruskal-google · 2022-10-11T03:33:44Z

Ok so on the plus side, this does break our tests. Let's try to get those green and then I can test internally

jhump · 2022-10-11T14:35:34Z

@mkruskal-google, I guess you are referring to the red "Bazel" and various "Windows C++ ..." kokoro builds?

How do I troubleshoot these? The links for the failed checks do not provide me any details whatsoever as to what exactly failed. I get "Unable to retrieve invocation log". If I look at assets and try to download the build log, I get a 403 Forbidden error.

mkruskal-google · 2022-10-11T15:21:49Z

Ah sorry, I guess the results are internal only >.<. I'll look into that, but in the meantime I'll summarize the issues and send them your way

mkruskal-google · 2022-10-11T15:26:15Z

These tests fail on linux under --config=asan, and on windows for normal builds:
//src/google/protobuf:descriptor_unittest
//src/google/protobuf/compiler:parser_unittest

jhump · 2022-10-12T14:46:57Z

@mkruskal-google, I don't suppose you could provide any advice on how I'd run linux tests with --config=asan? Is that a command-line argument to bazel?

Also, I don't suppose you can dig up any more details (like error message details for which assertion failed)?

If you can, thanks in advance.

fowles · 2022-10-12T14:48:22Z

bazel build --config asan ... will produce an asan enabled binary.

src/google/protobuf/descriptor.cc

mkruskal-google · 2022-10-12T15:20:51Z

Here are your errors in windows:

[ RUN      ] ValidationErrorTest.ValidateProto3JsonName

src/google/protobuf/descriptor_unittest.cc(3935): error: Expected equality of these values:

  expected_errors

    Which is: "foo.proto: Foo: NAME: The default JSON name of field \"Name\" (\"Name\") conflicts with the default JSON name of field \"name\" (\"name\"). This is not allowed in proto3.\n"

  error_collector.text_

    Which is: "foo.proto: Foo: NAME: The default JSON name of field \"Name\" (\"Name\") conflicts with the default JSON name of field \"name\"\0(\"name\"). This is not allowed in proto3.\n"

src/google/protobuf/descriptor_unittest.cc(3935): error: Expected equality of these values:

  expected_errors

    Which is: "foo.proto: Foo: NAME: The default JSON name of field \"_a__b_\" (\"AB\") conflicts with the default JSON name of field \"ab\" (\"ab\"). This is not allowed in proto3.\n"

  error_collector.text_

    Which is: "foo.proto: Foo: NAME: The default JSON name of field \"_a__b_\" (\"AB\") conflicts with the default JSON name of field \"ab\"\0(\"ab\"). This is not allowed in proto3.\n"

[  FAILED  ] ValidationErrorTest.ValidateProto3JsonName (1 ms)

[ RUN      ] ParserValidationErrorTest.Proto3JsonConflictError

src/google/protobuf/compiler/parser_unittest.cc(187): error: Expected equality of these values:

  expected_errors

    Which is: "4:15: The default JSON name of field \"Foo\" (\"Foo\") conflicts with the default JSON name of field \"foo\" (\"foo\"). This is not allowed in proto3.\n"

  error_collector_.text_

    Which is: "4:15: The default JSON name of field \"Foo\" (\"Foo\") conflicts with the default JSON name of field \"foo\"\0(\"foo\"). This is not allowed in proto3.\n"

[  FAILED  ] ParserValidationErrorTest.Proto3JsonConflictError (0 ms)

As for the ASAN failures, Matt seems to have tracked it down :)

jhump · 2022-10-12T15:46:49Z

I am hoping the same issue tickling ASAN errors may also be causing the Windows issues. I really have no idea how a NUL character is sneaking into that string where a space belongs, but it is exactly the same part of code as the ASAN issue, with construction of the name_suffix variable. Maybe changing the type will fix it there, too. 🤞

mkruskal-google · 2022-10-12T15:47:50Z

Yea I was thinking the same thing. It could just be UB acting differently on windows and linux

jhump · 2022-10-12T18:21:07Z

@mkruskal-google, looks like everything is green. 🎉

Anything else I can do to help this get merged safely?

mkruskal-google · 2022-10-12T18:29:45Z

Nope, I will try to import this into google and see if there's any uncaught issues. Side note: what's the timeline on this look like? If we can wait a few weeks this would be much easier to port than it is today

jhump · 2022-10-12T19:18:58Z

If we can wait a few weeks this would be much easier to port than it is today

@mkruskal-google, in my ideal world, this would get into the next release. So that question goes back to you: if we wait a few weeks, would it still be able to make it into the next release?

I'm trying to reconcile Buf's compiler with protoc. In most cases when there are differences, I update the compiler in Buf to match protoc. But in cases that seem like bugs in protoc (especially when there are issues filed against protoc where the team has suggested they're open to a fix, like this one), I prefer to update/fix in protoc.

In this case, we implemented some checks in Buf's compiler since it was indicated in #5063 that this functionality is acceptable for protoc, too. So now I want to get this into the next release so that the two compilers will behave the same.

fowles · 2022-10-12T19:31:33Z

Yes, this is not at risk of missing the 22.x release

- 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

See #10750 for more information. PiperOrigin-RevId: 493702276

…eld validation. See #10750 for more information. PiperOrigin-RevId: 493702276

…eld validation. See #10750 for more information. PiperOrigin-RevId: 493778412

-- 3eac250 by Josh Humphries <[email protected]>: add check for custom JSON name conflicts - 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 -- b23b387 by Josh Humphries <[email protected]>: update existing test expectations and add new tests -- aa34e0e by Josh Humphries <[email protected]>: JSON -> Json -- fdaa464 by Josh Humphries <[email protected]>: address review feedback wrt absl string functions also moves helpers into anonymous namespace -- 6d5f2fc by Josh Humphries <[email protected]>: apply clang-format changes; change really long pair type to auto -- 81b5cbe by Josh Humphries <[email protected]>: address review feedback -- 8fa8b10 by Josh Humphries <[email protected]>: return struct instead of using out param -- b405717 by Josh Humphries <[email protected]>: address review feedback FUTURE_COPYBARA_INTEGRATE_REVIEW=#10750 from jhump:jh/custom-json-name-validation b405717 PiperOrigin-RevId: 485415358

…eld validation. See #10750 for more information. PiperOrigin-RevId: 493778412

…eld validation. See #10750 for more information. FUTURE_COPYBARA_INTEGRATE_REVIEW=#11257 from Neakxs:apply-registration-for-protocel-extensions 8d04626 PiperOrigin-RevId: 493778412

…eld validation. See #10750 for more information. PiperOrigin-RevId: 493778412

…eld validation. See #10750 for more information. PiperOrigin-RevId: 495354518

-- 3eac250 by Josh Humphries <[email protected]>: add check for custom JSON name conflicts - 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 -- b23b387 by Josh Humphries <[email protected]>: update existing test expectations and add new tests -- aa34e0e by Josh Humphries <[email protected]>: JSON -> Json -- fdaa464 by Josh Humphries <[email protected]>: address review feedback wrt absl string functions also moves helpers into anonymous namespace -- 6d5f2fc by Josh Humphries <[email protected]>: apply clang-format changes; change really long pair type to auto -- 81b5cbe by Josh Humphries <[email protected]>: address review feedback -- 8fa8b10 by Josh Humphries <[email protected]>: return struct instead of using out param -- b405717 by Josh Humphries <[email protected]>: address review feedback FUTURE_COPYBARA_INTEGRATE_REVIEW=#10750 from jhump:jh/custom-json-name-validation b405717 PiperOrigin-RevId: 485415358

-- 3eac250 by Josh Humphries <[email protected]>: add check for custom JSON name conflicts - 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 -- b23b387 by Josh Humphries <[email protected]>: update existing test expectations and add new tests -- aa34e0e by Josh Humphries <[email protected]>: JSON -> Json -- fdaa464 by Josh Humphries <[email protected]>: address review feedback wrt absl string functions also moves helpers into anonymous namespace -- 6d5f2fc by Josh Humphries <[email protected]>: apply clang-format changes; change really long pair type to auto -- 81b5cbe by Josh Humphries <[email protected]>: address review feedback -- 8fa8b10 by Josh Humphries <[email protected]>: return struct instead of using out param -- b405717 by Josh Humphries <[email protected]>: address review feedback COPYBARA_INTEGRATE_REVIEW=#10750 from jhump:jh/custom-json-name-validation b405717 FUTURE_COPYBARA_INTEGRATE_REVIEW=#10750 from jhump:jh/custom-json-name-validation b405717 PiperOrigin-RevId: 485415358

-- 3eac250 by Josh Humphries <[email protected]>: add check for custom JSON name conflicts - 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 -- b23b387 by Josh Humphries <[email protected]>: update existing test expectations and add new tests -- aa34e0e by Josh Humphries <[email protected]>: JSON -> Json -- fdaa464 by Josh Humphries <[email protected]>: address review feedback wrt absl string functions also moves helpers into anonymous namespace -- 6d5f2fc by Josh Humphries <[email protected]>: apply clang-format changes; change really long pair type to auto -- 81b5cbe by Josh Humphries <[email protected]>: address review feedback -- 8fa8b10 by Josh Humphries <[email protected]>: return struct instead of using out param -- b405717 by Josh Humphries <[email protected]>: address review feedback COPYBARA_INTEGRATE_REVIEW=#10750 from jhump:jh/custom-json-name-validation b405717 PiperOrigin-RevId: 496041006

mkruskal-google · 2022-12-17T07:48:24Z

Just merged into main! Currently it's setup to only warn for proto2, I'm going to work on enabling errors there next

This will apply uniformly in both proto2 and proto3, taking into account `json_name` options. See #10750 for more details. PiperOrigin-RevId: 496005994

This will apply uniformly in both proto2 and proto3, taking into account `json_name` options. See #10750 for more details. PiperOrigin-RevId: 502972769

…eld validation. See protocolbuffers#10750 for more information. PiperOrigin-RevId: 495354518

mkruskal-google self-requested a review October 10, 2022 18:16

mkruskal-google added kokoro:run json labels Oct 10, 2022

protobuf-kokoro removed the kokoro:run label Oct 10, 2022

mkruskal-google added c++ kokoro:run and removed release notes: no labels Oct 10, 2022

protobuf-kokoro removed the kokoro:run label Oct 10, 2022

fowles reviewed Oct 12, 2022

View reviewed changes

src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved

src/google/protobuf/descriptor.cc Show resolved Hide resolved

src/google/protobuf/descriptor.cc Outdated Show resolved Hide resolved

mkruskal-google added the kokoro:run label Oct 12, 2022

protobuf-kokoro removed the kokoro:run label Oct 12, 2022

googleberg assigned fowles and mkruskal-google and unassigned fowles Oct 13, 2022

add check for custom JSON name conflicts

3eac250

- 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

copybara-service bot mentioned this pull request Dec 7, 2022

Add a deprecated message and enum option for restoring legacy json field validation. #11188

Closed

copybara-service bot pushed a commit that referenced this pull request Dec 7, 2022

Add a deprecated file option for restoring legacy json field validation.

0eb476c

See #10750 for more information. PiperOrigin-RevId: 493702276

copybara-service bot pushed a commit that referenced this pull request Dec 7, 2022

Add a deprecated message and enum option for restoring legacy json fi…

3cf53be

…eld validation. See #10750 for more information. PiperOrigin-RevId: 493702276

copybara-service bot mentioned this pull request Dec 8, 2022

Add a deprecated message and enum option for restoring legacy json field validation. #11198

Closed

copybara-service bot pushed a commit that referenced this pull request Dec 8, 2022

Add a deprecated message and enum option for restoring legacy json fi…

0e36da0

…eld validation. See #10750 for more information. PiperOrigin-RevId: 493778412

copybara-service bot pushed a commit that referenced this pull request Dec 8, 2022

Add a deprecated message and enum option for restoring legacy json fi…

2dfc118

…eld validation. See #10750 for more information. PiperOrigin-RevId: 493778412

copybara-service bot mentioned this pull request Dec 8, 2022

Copybara import of the project: #11200

Closed

copybara-service bot pushed a commit that referenced this pull request Dec 8, 2022

Add a deprecated message and enum option for restoring legacy json fi…

7bd670a

…eld validation. See #10750 for more information. PiperOrigin-RevId: 493778412

copybara-service bot pushed a commit that referenced this pull request Dec 14, 2022

Add a deprecated message and enum option for restoring legacy json fi…

594e2c4

…eld validation. See #10750 for more information. PiperOrigin-RevId: 493778412

copybara-service bot pushed a commit that referenced this pull request Dec 14, 2022

Add a deprecated message and enum option for restoring legacy json fi…

8291209

…eld validation. See #10750 for more information. PiperOrigin-RevId: 493778412

copybara-service bot pushed a commit that referenced this pull request Dec 14, 2022

Add a deprecated message and enum option for restoring legacy json fi…

25423a8

…eld validation. See #10750 for more information. PiperOrigin-RevId: 493778412

copybara-service bot pushed a commit that referenced this pull request Dec 14, 2022

Add a deprecated message and enum option for restoring legacy json fi…

6b87d6f

…eld validation. See #10750 for more information. PiperOrigin-RevId: 495354518

copybara-service bot mentioned this pull request Dec 17, 2022

Copybara import of the project: #11349

Closed

mkruskal-google closed this Dec 17, 2022

copybara-service bot pushed a commit that referenced this pull request Jan 18, 2023

Enable new JSON field name conflict handling.

e1f46f7

This will apply uniformly in both proto2 and proto3, taking into account `json_name` options. See #10750 for more details. PiperOrigin-RevId: 496005994

copybara-service bot mentioned this pull request Jan 18, 2023

Enable new JSON field name conflict handling. #11577

Closed

copybara-service bot pushed a commit that referenced this pull request Jan 18, 2023

Enable new JSON field name conflict handling.

a2aa30e

This will apply uniformly in both proto2 and proto3, taking into account `json_name` options. See #10750 for more details. PiperOrigin-RevId: 496005994

copybara-service bot pushed a commit that referenced this pull request Jan 18, 2023

Enable new JSON field name conflict handling.

f60f478

This will apply uniformly in both proto2 and proto3, taking into account `json_name` options. See #10750 for more details. PiperOrigin-RevId: 502972769

bithium pushed a commit to bithium/protobuf that referenced this pull request Sep 4, 2023

Add a deprecated message and enum option for restoring legacy json fi…

5a5eb12

…eld validation. See protocolbuffers#10750 for more information. PiperOrigin-RevId: 495354518

Conversation

jhump commented Oct 10, 2022

Uh oh!

mkruskal-google commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhump commented Oct 11, 2022

Uh oh!

mkruskal-google commented Oct 11, 2022

Uh oh!

mkruskal-google commented Oct 11, 2022

Uh oh!

jhump commented Oct 12, 2022

Uh oh!

fowles commented Oct 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkruskal-google commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhump commented Oct 12, 2022

Uh oh!

mkruskal-google commented Oct 12, 2022

Uh oh!

jhump commented Oct 12, 2022

Uh oh!

mkruskal-google commented Oct 12, 2022

Uh oh!

jhump commented Oct 12, 2022

Uh oh!

fowles commented Oct 12, 2022

Uh oh!

mkruskal-google commented Dec 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mkruskal-google commented Oct 11, 2022 •

edited

Loading

mkruskal-google commented Oct 12, 2022 •

edited

Loading