From 9a020c4a7b247e8da0c5bc096ea85c54beb86b04 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Wed, 31 Jan 2024 14:20:03 -0800 Subject: [PATCH] Unify behavior of deprecated_legacy_json_field_conflicts across upb and syntax. This disables all checks of json_name for upb and protoc under both proto2 and proto3. This option is deprecated that will be removed in future versions, and is only meant as a temporary solution. This also fixes a latent bug in the calculation of camelcase name in Python/upb. Fixes #12525 PiperOrigin-RevId: 603158026 --- python/descriptor.c | 17 ++++++++++++-- .../pb_unit_tests/descriptor_test_wrapper.py | 5 ---- .../protobuf/compiler/parser_unittest.cc | 23 ++++++++++++++++--- src/google/protobuf/descriptor.cc | 10 ++------ src/google/protobuf/descriptor_unittest.cc | 7 ++---- upb/reflection/message_def.c | 19 +++++++++------ 6 files changed, 51 insertions(+), 30 deletions(-) diff --git a/python/descriptor.c b/python/descriptor.c index 29ba6b3047..7e121c4043 100644 --- a/python/descriptor.c +++ b/python/descriptor.c @@ -983,10 +983,23 @@ static PyObject* PyUpb_FieldDescriptor_GetName(PyUpb_DescriptorBase* self, return PyUnicode_FromString(upb_FieldDef_Name(self->def)); } +static char PyUpb_AsciiIsUpper(char ch) { return ch >= 'A' && ch <= 'Z'; } + +static char PyUpb_AsciiToLower(char ch) { + assert(PyUpb_AsciiIsUpper(ch)); + return ch + ('a' - 'A'); +} + static PyObject* PyUpb_FieldDescriptor_GetCamelCaseName( PyUpb_DescriptorBase* self, void* closure) { - // TODO: Ok to use jsonname here? - return PyUnicode_FromString(upb_FieldDef_JsonName(self->def)); + // Camelcase is equivalent to JSON name except for potentially the first + // character. + const char* name = upb_FieldDef_JsonName(self->def); + size_t size = strlen(name); + return size > 0 && PyUpb_AsciiIsUpper(name[0]) + ? PyUnicode_FromFormat("%c%s", PyUpb_AsciiToLower(name[0]), + name + 1) + : PyUnicode_FromStringAndSize(name, size); } static PyObject* PyUpb_FieldDescriptor_GetJsonName(PyUpb_DescriptorBase* self, diff --git a/python/pb_unit_tests/descriptor_test_wrapper.py b/python/pb_unit_tests/descriptor_test_wrapper.py index 11f47ade94..b2be7cd3b3 100644 --- a/python/pb_unit_tests/descriptor_test_wrapper.py +++ b/python/pb_unit_tests/descriptor_test_wrapper.py @@ -31,11 +31,6 @@ from google.protobuf.internal.descriptor_test import * import unittest -# These fail because they attempt to add fields with conflicting JSON names. -# We don't want to support this going forward. -MakeDescriptorTest.testCamelcaseName.__unittest_expecting_failure__ = True -MakeDescriptorTest.testJsonName.__unittest_expecting_failure__ = True - # We pass this test, but the error message is slightly different. # Our error message is better. NewDescriptorTest.testImmutableCppDescriptor.__unittest_expecting_failure__ = True diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc index 89e04d98c5..ee2f6b4bb0 100644 --- a/src/google/protobuf/compiler/parser_unittest.cc +++ b/src/google/protobuf/compiler/parser_unittest.cc @@ -2307,15 +2307,32 @@ TEST_F(ParserValidationErrorTest, Proto2CustomJsonConflictError) { } TEST_F(ParserValidationErrorTest, Proto3JsonConflictLegacy) { - ExpectHasValidationErrors( + ExpectParsesTo( "syntax = 'proto3';\n" "message TestMessage {\n" " option deprecated_legacy_json_field_conflicts = true;\n" " uint32 fooBar = 1;\n" " uint32 foo_bar = 2;\n" "}\n", - "4:9: The default JSON name of field \"foo_bar\" (\"fooBar\") conflicts " - "with the default JSON name of field \"fooBar\".\n"); + "syntax: 'proto3'\n" + "message_type {\n" + " name: 'TestMessage'\n" + " field {\n" + " label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'fooBar' number: 1\n" + " }\n" + " field {\n" + " label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'foo_bar' number: 2\n" + " }\n" + " options {\n" + " uninterpreted_option {\n" + " name {\n" + " name_part: 'deprecated_legacy_json_field_conflicts'\n" + " is_extension: false\n" + " }\n" + " identifier_value: 'true'\n" + " }\n" + " }\n" + "}\n"); } TEST_F(ParserValidationErrorTest, Proto2JsonConflictLegacy) { diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 0cf7b75772..657cddbeda 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -6186,14 +6186,8 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto, void DescriptorBuilder::CheckFieldJsonNameUniqueness( const DescriptorProto& proto, const Descriptor* result) { std::string message_name = result->full_name(); - if (pool_->deprecated_legacy_json_field_conflicts_ || - IsLegacyJsonFieldConflictEnabled(result->options())) { - if (result->file()->edition() == Edition::EDITION_PROTO3) { - // Only check default JSON names for conflicts in proto3. This is legacy - // behavior that will be removed in a later version. - CheckFieldJsonNameUniqueness(message_name, proto, result, false); - } - } else { + if (!pool_->deprecated_legacy_json_field_conflicts_ && + !IsLegacyJsonFieldConflictEnabled(result->options())) { // Check both with and without taking json_name into consideration. This is // needed for field masks, which don't use json_name. CheckFieldJsonNameUniqueness(message_name, proto, result, false); diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index de25e35476..5ed510b5fb 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -7267,7 +7267,7 @@ TEST_F(ValidationErrorTest, ValidateJsonNameConflictProto2) { // Test that field names that may conflict in JSON is not allowed by protoc. TEST_F(ValidationErrorTest, ValidateJsonNameConflictProto3Legacy) { - BuildFileWithErrors( + BuildFile( "name: 'foo.proto' " "syntax: 'proto3' " "message_type {" @@ -7275,10 +7275,7 @@ TEST_F(ValidationErrorTest, ValidateJsonNameConflictProto3Legacy) { " options { deprecated_legacy_json_field_conflicts: true }" " field { name:'AB' number:1 label:LABEL_OPTIONAL type:TYPE_INT32 }" " field { name:'_a__b_' number:2 label:LABEL_OPTIONAL type:TYPE_INT32 }" - "}", - "foo.proto: Foo: NAME: The default JSON name of field \"_a__b_\" " - "(\"AB\") " - "conflicts with the default JSON name of field \"AB\".\n"); + "}"); } TEST_F(ValidationErrorTest, ValidateJsonNameConflictProto2Legacy) { diff --git a/upb/reflection/message_def.c b/upb/reflection/message_def.c index 01f245b262..de3bc0c77c 100644 --- a/upb/reflection/message_def.c +++ b/upb/reflection/message_def.c @@ -415,7 +415,10 @@ void _upb_MessageDef_InsertField(upb_DefBuilder* ctx, upb_MessageDef* m, _upb_MessageDef_Insert(m, shortname, shortnamelen, field_v, ctx->arena); if (!ok) _upb_DefBuilder_OomErr(ctx); - if (strcmp(shortname, json_name) != 0 && + bool skip_json_conflicts = + UPB_DESC(MessageOptions_deprecated_legacy_json_field_conflicts)( + upb_MessageDef_Options(m)); + if (!skip_json_conflicts && strcmp(shortname, json_name) != 0 && UPB_DESC(FeatureSet_json_format)(m->resolved_features) == UPB_DESC(FeatureSet_ALLOW) && upb_strtable_lookup(&m->ntof, json_name, &v)) { @@ -425,14 +428,16 @@ void _upb_MessageDef_InsertField(upb_DefBuilder* ctx, upb_MessageDef* m, } if (upb_strtable_lookup(&m->jtof, json_name, &v)) { - _upb_DefBuilder_Errf(ctx, "duplicate json_name (%s)", json_name); + if (!skip_json_conflicts) { + _upb_DefBuilder_Errf(ctx, "duplicate json_name (%s)", json_name); + } + } else { + const size_t json_size = strlen(json_name); + ok = upb_strtable_insert(&m->jtof, json_name, json_size, + upb_value_constptr(f), ctx->arena); + if (!ok) _upb_DefBuilder_OomErr(ctx); } - const size_t json_size = strlen(json_name); - ok = upb_strtable_insert(&m->jtof, json_name, json_size, - upb_value_constptr(f), ctx->arena); - if (!ok) _upb_DefBuilder_OomErr(ctx); - if (upb_inttable_lookup(&m->itof, field_number, NULL)) { _upb_DefBuilder_Errf(ctx, "duplicate field number (%u)", field_number); }