diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc index 15f2dc4b6949..f02c23bec22e 100644 --- a/src/google/protobuf/compiler/parser_unittest.cc +++ b/src/google/protobuf/compiler/parser_unittest.cc @@ -2069,13 +2069,107 @@ TEST_F(ParserValidationErrorTest, Proto3Default) { TEST_F(ParserValidationErrorTest, Proto3JsonConflictError) { ExpectHasValidationErrors( - "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"); + 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"); } TEST_F(ParserValidationErrorTest, EnumNameError) { diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 9048cc6b1b75..dfe89c70733f 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -3856,8 +3856,6 @@ 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, @@ -3869,6 +3867,15 @@ 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); @@ -5424,7 +5431,10 @@ 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++) { @@ -5489,6 +5499,84 @@ 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. + absl::string_view 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, @@ -5892,13 +5980,12 @@ void DescriptorBuilder::CheckEnumValueUniqueness( // NAME_TYPE_LAST_NAME = 2, // } PrefixRemover remover(result->name()); - std::map values; + absl::flat_hash_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())); - std::pair::iterator, bool> - insert_result = values.insert(std::make_pair(stripped, value)); + auto 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 @@ -5909,19 +5996,18 @@ 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 = - "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."; + 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()); // 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); } @@ -6788,20 +6874,6 @@ 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) { @@ -6826,25 +6898,6 @@ 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 ef99f8089a81..aa02305a0eb8 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -6478,9 +6478,8 @@ 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). " - "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"); + "(If you are using allow_alias, please assign the same numeric " + "value to both enums.) This is not allowed in proto3.\n"); // Not an error because both enums are mapped to the same value. BuildFile( @@ -6506,9 +6505,8 @@ 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). " - "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"); + "(If you are using allow_alias, please assign the same numeric value " + "to both enums.) This is not allowed in proto3.\n"); BuildFileWithErrors( "syntax: 'proto3'" @@ -6520,9 +6518,8 @@ 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). " - "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"); + "(If you are using allow_alias, please assign the same numeric value " + "to both enums.) This is not allowed in proto3.\n"); BuildFileWithErrors( "syntax: 'proto3'" @@ -6534,9 +6531,8 @@ 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). 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"); + "(if any). (If you are using allow_alias, please assign the same numeric " + "value to both enums.) This is not allowed in proto3.\n"); BuildFileWithErrors( "syntax: 'proto3'" @@ -6548,9 +6544,8 @@ 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). 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"); + "(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 isn't an error because the underscore will cause the PascalCase to // differ by case (BarBaz vs. Barbaz). @@ -6895,8 +6890,9 @@ 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 JSON camel-case name of field \"Name\" " - "conflicts with field \"name\". This is not allowed in proto3.\n"); + "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"); // Underscores are ignored. BuildFileWithErrors( "name: 'foo.proto' " @@ -6906,8 +6902,9 @@ 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 JSON camel-case name of field \"_a__b_\" " - "conflicts with field \"ab\". This is not allowed in proto3.\n"); + "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"); }