From d58d47111726d31e868bf78e4d388cb378c3a03b Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Mon, 26 Sep 2022 12:17:19 -0700 Subject: [PATCH] Revert "Merge pull request #10581 from jhump/jh/custom-json-name-validation" (#10657) This reverts commit d3b538930144f399da0d7e64a35b05d600789f63, reversing changes made to bcd175578f4647a5fa81eecee1d3ecc320e72517. --- .../protobuf/compiler/parser_unittest.cc | 108 +------------ src/google/protobuf/descriptor.cc | 143 ++++++------------ src/google/protobuf/descriptor_unittest.cc | 35 +++-- 3 files changed, 71 insertions(+), 215 deletions(-) diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc index 2722873089f5..8260721acb5d 100644 --- a/src/google/protobuf/compiler/parser_unittest.cc +++ b/src/google/protobuf/compiler/parser_unittest.cc @@ -2069,107 +2069,13 @@ TEST_F(ParserValidationErrorTest, Proto3Default) { TEST_F(ParserValidationErrorTest, Proto3JsonConflictError) { ExpectHasValidationErrors( - R"pb( - syntax = 'proto3'; - message TestMessage { - uint32 foo = 1; - uint32 Foo = 2; - } - )pb", - "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"); -} - -TEST_F(ParserValidationErrorTest, Proto2JsonConflictError) { - // conflicts with default JSON names are not errors in proto2 - ExpectParsesTo( - R"pb( - syntax = "proto2"; - message TestMessage { - optional uint32 foo = 1; - optional uint32 Foo = 2; - } - )pb", - R"pb( - syntax: 'proto2' - message_type { - name: 'TestMessage' - field { - label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'foo' number: 1 - } - field { - label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'Foo' number: 2 - } - } - )pb" - ); -} - -TEST_F(ParserValidationErrorTest, Proto3CustomJsonConflictWithDefaultError) { - ExpectHasValidationErrors( - R"pb( - syntax = 'proto3'; - message TestMessage { - uint32 foo = 1 [json_name='bar']; - uint32 bar = 2; - } - )pb", - "4:15: The default JSON name of field \"bar\" (\"bar\") conflicts " - "with the custom JSON name of field \"foo\". " - "This is not allowed in proto3.\n"); -} - -TEST_F(ParserValidationErrorTest, Proto2CustomJsonConflictWithDefaultError) { - // conflicts with default JSON names are not errors in proto2 - ExpectParsesTo( - R"pb( - syntax = 'proto2'; - message TestMessage { - optional uint32 foo = 1 [json_name='bar']; - optional uint32 bar = 2; - } - )pb", - R"pb( - syntax: 'proto2' - message_type { - name: 'TestMessage' - field { - label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'foo' number: 1 json_name: 'bar' - } - field { - label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'bar' number: 2 - } - } - )pb" - ); -} - -TEST_F(ParserValidationErrorTest, Proto3CustomJsonConflictError) { - ExpectHasValidationErrors( - R"pb( - syntax = 'proto3'; - message TestMessage { - uint32 foo = 1 [json_name='baz']; - uint32 bar = 2 [json_name='baz']; - } - )pb", - "4:15: The custom JSON name of field \"bar\" (\"baz\") conflicts " - "with the custom JSON name of field \"foo\".\n"); -} - -TEST_F(ParserValidationErrorTest, Proto2CustomJsonConflictError) { - ExpectHasValidationErrors( - R"pb( - syntax = 'proto2'; - message TestMessage { - optional uint32 foo = 1 [json_name='baz']; - optional uint32 bar = 2 [json_name='baz']; - } - )pb", - // fails in proto2 also: can't explicitly configure bad custom JSON names - "4:24: The custom JSON name of field \"bar\" (\"baz\") conflicts " - "with the custom JSON name of field \"foo\".\n"); + "syntax = 'proto3';\n" + "message TestMessage {\n" + " uint32 foo = 1;\n" + " uint32 Foo = 2;\n" + "}\n", + "3:9: The JSON camel-case name of field \"Foo\" conflicts with field " + "\"foo\". This is not allowed in proto3.\n"); } TEST_F(ParserValidationErrorTest, EnumNameError) { diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 0f3c6326bf1b..54822bd8ccd6 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -3855,6 +3855,8 @@ class DescriptorBuilder { internal::FlatAllocator& alloc); void BuildOneof(const OneofDescriptorProto& proto, Descriptor* parent, OneofDescriptor* result, internal::FlatAllocator& alloc); + void CheckEnumValueUniqueness(const EnumDescriptorProto& proto, + const EnumDescriptor* result); void BuildEnum(const EnumDescriptorProto& proto, const Descriptor* parent, EnumDescriptor* result, internal::FlatAllocator& alloc); void BuildEnumValue(const EnumValueDescriptorProto& proto, @@ -3866,15 +3868,6 @@ class DescriptorBuilder { const ServiceDescriptor* parent, MethodDescriptor* result, internal::FlatAllocator& alloc); - void CheckFieldJsonNameUniqueness(const DescriptorProto& proto, - const Descriptor* result); - void CheckFieldJsonNameUniqueness(const std::string& message_name, - const DescriptorProto& message, - FileDescriptor::Syntax syntax, - bool use_custom_names); - void CheckEnumValueUniqueness(const EnumDescriptorProto& proto, - const EnumDescriptor* result); - void LogUnusedDependency(const FileDescriptorProto& proto, const FileDescriptor* result); @@ -5430,10 +5423,7 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto, } } - CheckFieldJsonNameUniqueness(proto, result); - // Check that fields aren't using reserved names or numbers and that they - // aren't using extension numbers. for (int i = 0; i < result->field_count(); i++) { const FieldDescriptor* field = result->field(i); for (int j = 0; j < result->extension_range_count(); j++) { @@ -5498,84 +5488,6 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto, } } -void DescriptorBuilder::CheckFieldJsonNameUniqueness( - const DescriptorProto& proto, const Descriptor* result) { - FileDescriptor::Syntax syntax = result->file()->syntax(); - std::string message_name = result->full_name(); - // two passes: one looking only at default JSON names, and one that considers - // custom JSON names - CheckFieldJsonNameUniqueness(message_name, proto, syntax, false); - CheckFieldJsonNameUniqueness(message_name, proto, syntax, true); -} - -namespace { -// Helpers for function below - -struct JsonNameDetails { - const FieldDescriptorProto* field; - std::string orig_name; - bool is_custom; -}; - -JsonNameDetails GetJsonNameDetails(const FieldDescriptorProto* field, bool use_custom) { - if (use_custom && field->has_json_name()) { - return {field, field->json_name(), true}; - } - return {field, ToJsonName(field->name()), false}; -} - - -} // namespace - -void DescriptorBuilder::CheckFieldJsonNameUniqueness( - const std::string& message_name, const DescriptorProto& message, - FileDescriptor::Syntax syntax, bool use_custom_names) { - - absl::flat_hash_map name_to_field; - for (const FieldDescriptorProto& field : message.field()) { - JsonNameDetails details = GetJsonNameDetails(&field, use_custom_names); - std::string lowercase_name = absl::AsciiStrToLower(details.orig_name); - auto existing = name_to_field.find(lowercase_name); - if (existing == name_to_field.end()) { - name_to_field[lowercase_name] = details; - continue; - } - JsonNameDetails& match = existing->second; - if (use_custom_names && !details.is_custom && !match.is_custom) { - // if this pass is considering custom JSON names, but neither of the - // names involved in the conflict are custom, don't bother with a - // message. That will have been reported from other pass (non-custom - // JSON names). - continue; - } - absl::string_view this_type = details.is_custom ? "custom" : "default"; - absl::string_view existing_type = match.is_custom ? "custom" : "default"; - // If the matched name differs (which it can only differ in case), include - // it in the error message, for maximum clarity to user. - std::string name_suffix; - if (details.orig_name != match.orig_name) { - name_suffix = absl::StrCat(" (\"", match.orig_name, "\")"); - } - std::string error_message = - absl::StrFormat("The %s JSON name of field \"%s\" (\"%s\") conflicts " - "with the %s JSON name of field \"%s\"%s.", - this_type, field.name(), details.orig_name, existing_type, - match.field->name(), name_suffix); - - bool involves_default = !details.is_custom || !match.is_custom; - if (syntax == FileDescriptor::SYNTAX_PROTO2 && involves_default) { - AddWarning(message_name, field, DescriptorPool::ErrorCollector::NAME, - error_message); - } else { - if (involves_default) { - absl::StrAppend(&error_message, " This is not allowed in proto3."); - } - AddError(message_name, field, DescriptorPool::ErrorCollector::NAME, - error_message); - } - } -} - void DescriptorBuilder::BuildFieldOrExtension(const FieldDescriptorProto& proto, Descriptor* parent, FieldDescriptor* result, @@ -5979,12 +5891,13 @@ void DescriptorBuilder::CheckEnumValueUniqueness( // NAME_TYPE_LAST_NAME = 2, // } PrefixRemover remover(result->name()); - absl::flat_hash_map values; + std::map values; for (int i = 0; i < result->value_count(); i++) { const EnumValueDescriptor* value = result->value(i); std::string stripped = EnumValueToPascalCase(remover.MaybeRemove(value->name())); - auto insert_result = values.insert(std::make_pair(stripped, value)); + std::pair::iterator, bool> + insert_result = values.insert(std::make_pair(stripped, value)); bool inserted = insert_result.second; // We don't throw the error if the two conflicting symbols are identical, or @@ -5995,18 +5908,19 @@ void DescriptorBuilder::CheckEnumValueUniqueness( // stripping should de-dup the labels in this case). if (!inserted && insert_result.first->second->name() != value->name() && insert_result.first->second->number() != value->number()) { - std::string error_message = absl::StrFormat( - "Enum name %s has the same name as %s if you ignore case and strip " - "out the enum name prefix (if any). (If you are using allow_alias, " - "please assign the same numeric value to both enums.)", - value->name(), values[stripped]->name()); + std::string error_message = + "Enum name " + value->name() + " has the same name as " + + values[stripped]->name() + + " if you ignore case and strip out the enum name prefix (if any). " + "This is error-prone and can lead to undefined behavior. " + "Please avoid doing this. If you are using allow_alias, please " + "assign the same numeric value to both enums."; // There are proto2 enums out there with conflicting names, so to preserve // compatibility we issue only a warning for proto2. if (result->file()->syntax() == FileDescriptor::SYNTAX_PROTO2) { AddWarning(value->full_name(), proto.value(i), DescriptorPool::ErrorCollector::NAME, error_message); } else { - absl::StrAppend(&error_message, " This is not allowed in proto3."); AddError(value->full_name(), proto.value(i), DescriptorPool::ErrorCollector::NAME, error_message); } @@ -6873,6 +6787,20 @@ void DescriptorBuilder::ValidateProto3(FileDescriptor* file, } } +static std::string ToLowercaseWithoutUnderscores(const std::string& name) { + std::string result; + for (char character : name) { + if (character != '_') { + if (character >= 'A' && character <= 'Z') { + result.push_back(character - 'A' + 'a'); + } else { + result.push_back(character); + } + } + } + return result; +} + void DescriptorBuilder::ValidateProto3Message(Descriptor* message, const DescriptorProto& proto) { for (int i = 0; i < message->nested_type_count(); ++i) { @@ -6897,6 +6825,25 @@ void DescriptorBuilder::ValidateProto3Message(Descriptor* message, AddError(message->full_name(), proto, DescriptorPool::ErrorCollector::NAME, "MessageSet is not supported in proto3."); } + + // In proto3, we reject field names if they conflict in camelCase. + // Note that we currently enforce a stricter rule: Field names must be + // unique after being converted to lowercase with underscores removed. + std::map name_to_field; + for (int i = 0; i < message->field_count(); ++i) { + std::string lowercase_name = + ToLowercaseWithoutUnderscores(message->field(i)->name()); + if (name_to_field.find(lowercase_name) != name_to_field.end()) { + AddError(message->full_name(), proto.field(i), + DescriptorPool::ErrorCollector::NAME, + "The JSON camel-case name of field \"" + + message->field(i)->name() + "\" conflicts with field \"" + + name_to_field[lowercase_name]->name() + "\". This is not " + + "allowed in proto3."); + } else { + name_to_field[lowercase_name] = message->field(i); + } + } } void DescriptorBuilder::ValidateProto3Field(FieldDescriptor* field, diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index 54ab6fbd658d..d14557cc9b08 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -6478,8 +6478,9 @@ TEST_F(ValidationErrorTest, EnumValuesConflictWithDifferentCasing) { "}", "foo.proto: bar: NAME: Enum name bar has the same name as BAR " "if you ignore case and strip out the enum name prefix (if any). " - "(If you are using allow_alias, please assign the same numeric " - "value to both enums.) This is not allowed in proto3.\n"); + "This is error-prone and can lead to undefined behavior. " + "Please avoid doing this. If you are using allow_alias, please assign " + "the same numeric value to both enums.\n"); // Not an error because both enums are mapped to the same value. BuildFile( @@ -6505,8 +6506,9 @@ TEST_F(ValidationErrorTest, EnumValuesConflictWhenPrefixesStripped) { "}", "foo.proto: BAZ: NAME: Enum name BAZ has the same name as FOO_ENUM_BAZ " "if you ignore case and strip out the enum name prefix (if any). " - "(If you are using allow_alias, please assign the same numeric value " - "to both enums.) This is not allowed in proto3.\n"); + "This is error-prone and can lead to undefined behavior. " + "Please avoid doing this. If you are using allow_alias, please assign " + "the same numeric value to both enums.\n"); BuildFileWithErrors( "syntax: 'proto3'" @@ -6518,8 +6520,9 @@ TEST_F(ValidationErrorTest, EnumValuesConflictWhenPrefixesStripped) { "}", "foo.proto: BAZ: NAME: Enum name BAZ has the same name as FOOENUM_BAZ " "if you ignore case and strip out the enum name prefix (if any). " - "(If you are using allow_alias, please assign the same numeric value " - "to both enums.) This is not allowed in proto3.\n"); + "This is error-prone and can lead to undefined behavior. " + "Please avoid doing this. If you are using allow_alias, please assign " + "the same numeric value to both enums.\n"); BuildFileWithErrors( "syntax: 'proto3'" @@ -6531,8 +6534,9 @@ TEST_F(ValidationErrorTest, EnumValuesConflictWhenPrefixesStripped) { "}", "foo.proto: BAR__BAZ: NAME: Enum name BAR__BAZ has the same name as " "FOO_ENUM_BAR_BAZ if you ignore case and strip out the enum name prefix " - "(if any). (If you are using allow_alias, please assign the same numeric " - "value to both enums.) This is not allowed in proto3.\n"); + "(if any). This is error-prone and can lead to undefined behavior. " + "Please avoid doing this. If you are using allow_alias, please assign " + "the same numeric value to both enums.\n"); BuildFileWithErrors( "syntax: 'proto3'" @@ -6544,8 +6548,9 @@ TEST_F(ValidationErrorTest, EnumValuesConflictWhenPrefixesStripped) { "}", "foo.proto: BAR_BAZ: NAME: Enum name BAR_BAZ has the same name as " "FOO_ENUM__BAR_BAZ if you ignore case and strip out the enum name prefix " - "(if any). (If you are using allow_alias, please assign the same numeric " - "value to both enums.) This is not allowed in proto3.\n"); + "(if any). This is error-prone and can lead to undefined behavior. " + "Please avoid doing this. If you are using allow_alias, please assign " + "the same numeric value to both enums.\n"); // This isn't an error because the underscore will cause the PascalCase to // differ by case (BarBaz vs. Barbaz). @@ -6890,9 +6895,8 @@ TEST_F(ValidationErrorTest, ValidateProto3JsonName) { " field { name:'name' number:1 label:LABEL_OPTIONAL type:TYPE_INT32 }" " field { name:'Name' number:2 label:LABEL_OPTIONAL type:TYPE_INT32 }" "}", - "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"); + "foo.proto: Foo: NAME: The JSON camel-case name of field \"Name\" " + "conflicts with field \"name\". This is not allowed in proto3.\n"); // Underscores are ignored. BuildFileWithErrors( "name: 'foo.proto' " @@ -6902,9 +6906,8 @@ TEST_F(ValidationErrorTest, ValidateProto3JsonName) { " 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\" (\"ab\"). This is not " - "allowed in proto3.\n"); + "foo.proto: Foo: NAME: The JSON camel-case name of field \"_a__b_\" " + "conflicts with field \"ab\". This is not allowed in proto3.\n"); }