From c23d0b87c50cb9655c230d637c5430f25d76cbe4 Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Wed, 14 Sep 2022 14:04:12 -0400 Subject: [PATCH 1/7] 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 --- src/google/protobuf/descriptor.cc | 116 +++++++++++++++++++++--------- 1 file changed, 81 insertions(+), 35 deletions(-) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index ddebcf618415..43d7492b9b80 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -3847,8 +3847,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, @@ -3860,6 +3858,14 @@ class DescriptorBuilder { const ServiceDescriptor* parent, MethodDescriptor* result, internal::FlatAllocator& alloc); + void CheckFieldJSONNameUniqueness(const DescriptorProto& proto, + const Descriptor* result); + void CheckFieldJSONNameUniqueness(std::string message_name, + const DescriptorProto& message, + bool is_proto2, bool use_custom_names); + void CheckEnumValueUniqueness(const EnumDescriptorProto& proto, + const EnumDescriptor* result); + void LogUnusedDependency(const FileDescriptorProto& proto, const FileDescriptor* result); @@ -5415,7 +5421,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++) { @@ -5480,6 +5489,75 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto, } } +std::string GetJSONName(const FieldDescriptorProto& field, bool use_custom, bool* was_custom) { + if (use_custom && field.has_json_name()) { + *was_custom = true; + return field.json_name(); + } + *was_custom = false; + return ToJsonName(field.name()); +} + +void DescriptorBuilder::CheckFieldJSONNameUniqueness( + const DescriptorProto& proto, const Descriptor* result) { + bool is_proto2 = result->file()->syntax() == FileDescriptor::SYNTAX_PROTO2; + 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, is_proto2, false); + CheckFieldJSONNameUniqueness(message_name, proto, is_proto2, true); +} + +struct JSONNameDetails { + const FieldDescriptorProto* field; + std::string orig_name; + bool is_custom; +}; + +void DescriptorBuilder::CheckFieldJSONNameUniqueness( + std::string message_name,const DescriptorProto& message, bool is_proto2, bool use_custom_names) { + + std::map name_to_field; + for (int i = 0; i < message.field_size(); ++i) { + bool is_custom; + std::string name = GetJSONName(message.field(i), use_custom_names, &is_custom); + std::string lowercase_name = absl::AsciiStrToLower(name); + auto existing = name_to_field.find(lowercase_name); + if (existing != name_to_field.end()) { + auto match = existing->second; + if (use_custom_names && !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; + } + std::string this_type = is_custom ? "custom" : "default"; + std::string 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 = name == match.orig_name ? "" : " (\"" + match.orig_name + "\")"; + std::string error_message = + "The " + this_type + " JSON name of field \"" + message.field(i).name() + + "\" (\"" + name + "\") conflicts with the " + existing_type + " JSON name of field \"" + + match.field->name() + "\"" + name_suffix + "."; + + bool involves_default = !is_custom || !match.is_custom; + if (is_proto2 && involves_default) { + AddWarning(message_name, message.field(i), + DescriptorPool::ErrorCollector::NAME, error_message); + } else { + if (involves_default) { + error_message += " This is not allowed in proto3."; + } + AddError(message_name, message.field(i), + DescriptorPool::ErrorCollector::NAME, error_message); + } + } else { + struct JSONNameDetails details = { &message.field(i), name, is_custom }; + name_to_field[lowercase_name] = details; + } + } +} + void DescriptorBuilder::BuildFieldOrExtension(const FieldDescriptorProto& proto, Descriptor* parent, FieldDescriptor* result, @@ -5913,6 +5991,7 @@ void DescriptorBuilder::CheckEnumValueUniqueness( AddWarning(value->full_name(), proto.value(i), DescriptorPool::ErrorCollector::NAME, error_message); } else { + error_message += " This is not allowed in proto3."; AddError(value->full_name(), proto.value(i), DescriptorPool::ErrorCollector::NAME, error_message); } @@ -6779,20 +6858,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) { @@ -6817,25 +6882,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, From cbd5c84b6b8b07e01ee63ade2c4c2f76dfd3ee2c Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Wed, 14 Sep 2022 18:06:31 -0400 Subject: [PATCH 2/7] update existing test expectations and add new tests --- .../protobuf/compiler/parser_unittest.cc | 84 ++++++++++++++++++- src/google/protobuf/descriptor.cc | 5 +- src/google/protobuf/descriptor_unittest.cc | 35 ++++---- 3 files changed, 100 insertions(+), 24 deletions(-) diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc index 3b3245191659..568a6997273e 100644 --- a/src/google/protobuf/compiler/parser_unittest.cc +++ b/src/google/protobuf/compiler/parser_unittest.cc @@ -1977,8 +1977,88 @@ TEST_F(ParserValidationErrorTest, Proto3JsonConflictError) { " 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"); + "3:9: 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( + "syntax = 'proto2';\n" + "message TestMessage {\n" + " optional uint32 foo = 1;\n" + " optional uint32 Foo = 2;\n" + "}\n", + + "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" + " }" + "}" + ); +} + +TEST_F(ParserValidationErrorTest, Proto3CustomJsonConflictWithDefaultError) { + ExpectHasValidationErrors( + "syntax = 'proto3';\n" + "message TestMessage {\n" + " uint32 foo = 1 [json_name='bar'];\n" + " uint32 bar = 2;\n" + "}\n", + "3:9: 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( + "syntax = 'proto2';\n" + "message TestMessage {\n" + " optional uint32 foo = 1 [json_name='bar'];\n" + " optional uint32 bar = 2;\n" + "}\n", + + "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" + " }" + "}" + ); +} + +TEST_F(ParserValidationErrorTest, Proto3CustomJsonConflictError) { + ExpectHasValidationErrors( + "syntax = 'proto3';\n" + "message TestMessage {\n" + " uint32 foo = 1 [json_name='baz'];\n" + " uint32 bar = 2 [json_name='baz'];\n" + "}\n", + "3:9: The custom JSON name of field \"bar\" (\"baz\") conflicts " + "with the custom JSON name of field \"foo\".\n"); +} + +TEST_F(ParserValidationErrorTest, Proto2CustomJsonConflictError) { + ExpectHasValidationErrors( + "syntax = 'proto2';\n" + "message TestMessage {\n" + " optional uint32 foo = 1 [json_name='baz'];\n" + " optional uint32 bar = 2 [json_name='baz'];\n" + "}\n", + // fails in proto2 also: can't explicitly configure bad custom JSON names + "3:18: 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 43d7492b9b80..ad5eb8db2c48 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -5982,9 +5982,8 @@ void DescriptorBuilder::CheckEnumValueUniqueness( "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."; + "(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) { diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index a2900454f646..cc05addf979e 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -6411,9 +6411,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( @@ -6439,9 +6438,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'" @@ -6453,9 +6451,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'" @@ -6467,9 +6464,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'" @@ -6481,9 +6477,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). @@ -6828,8 +6823,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' " @@ -6839,8 +6835,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"); } From 7c57fb08e6e27bc72a4bfdf0832d4737ed0ab3bb Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Thu, 15 Sep 2022 14:26:23 -0400 Subject: [PATCH 3/7] JSON -> Json --- src/google/protobuf/descriptor.cc | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index ad5eb8db2c48..308381982fe8 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -3858,9 +3858,9 @@ class DescriptorBuilder { const ServiceDescriptor* parent, MethodDescriptor* result, internal::FlatAllocator& alloc); - void CheckFieldJSONNameUniqueness(const DescriptorProto& proto, + void CheckFieldJsonNameUniqueness(const DescriptorProto& proto, const Descriptor* result); - void CheckFieldJSONNameUniqueness(std::string message_name, + void CheckFieldJsonNameUniqueness(std::string message_name, const DescriptorProto& message, bool is_proto2, bool use_custom_names); void CheckEnumValueUniqueness(const EnumDescriptorProto& proto, @@ -5421,7 +5421,7 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto, } } - CheckFieldJSONNameUniqueness(proto, result); + CheckFieldJsonNameUniqueness(proto, result); // Check that fields aren't using reserved names or numbers and that they // aren't using extension numbers. @@ -5489,7 +5489,7 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto, } } -std::string GetJSONName(const FieldDescriptorProto& field, bool use_custom, bool* was_custom) { +std::string GetJsonName(const FieldDescriptorProto& field, bool use_custom, bool* was_custom) { if (use_custom && field.has_json_name()) { *was_custom = true; return field.json_name(); @@ -5498,28 +5498,28 @@ std::string GetJSONName(const FieldDescriptorProto& field, bool use_custom, bool return ToJsonName(field.name()); } -void DescriptorBuilder::CheckFieldJSONNameUniqueness( +void DescriptorBuilder::CheckFieldJsonNameUniqueness( const DescriptorProto& proto, const Descriptor* result) { bool is_proto2 = result->file()->syntax() == FileDescriptor::SYNTAX_PROTO2; 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, is_proto2, false); - CheckFieldJSONNameUniqueness(message_name, proto, is_proto2, true); + CheckFieldJsonNameUniqueness(message_name, proto, is_proto2, false); + CheckFieldJsonNameUniqueness(message_name, proto, is_proto2, true); } -struct JSONNameDetails { +struct JsonNameDetails { const FieldDescriptorProto* field; std::string orig_name; bool is_custom; }; -void DescriptorBuilder::CheckFieldJSONNameUniqueness( +void DescriptorBuilder::CheckFieldJsonNameUniqueness( std::string message_name,const DescriptorProto& message, bool is_proto2, bool use_custom_names) { - std::map name_to_field; + std::map name_to_field; for (int i = 0; i < message.field_size(); ++i) { bool is_custom; - std::string name = GetJSONName(message.field(i), use_custom_names, &is_custom); + std::string name = GetJsonName(message.field(i), use_custom_names, &is_custom); std::string lowercase_name = absl::AsciiStrToLower(name); auto existing = name_to_field.find(lowercase_name); if (existing != name_to_field.end()) { @@ -5552,7 +5552,7 @@ void DescriptorBuilder::CheckFieldJSONNameUniqueness( DescriptorPool::ErrorCollector::NAME, error_message); } } else { - struct JSONNameDetails details = { &message.field(i), name, is_custom }; + JsonNameDetails details = { &message.field(i), name, is_custom }; name_to_field[lowercase_name] = details; } } From 16627c5f41c46c78fc35b4b8d8c43ff6597c382e Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Thu, 15 Sep 2022 15:04:20 -0400 Subject: [PATCH 4/7] address review feedback wrt absl string functions also moves helpers into anonymous namespace --- src/google/protobuf/descriptor.cc | 73 +++++++++++++++++-------------- 1 file changed, 40 insertions(+), 33 deletions(-) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 308381982fe8..3bdbf5c6ef4b 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -58,6 +58,7 @@ #include "absl/strings/ascii.h" #include "absl/strings/escaping.h" #include "absl/strings/str_cat.h" +#include "absl/strings/str_format.h" #include "google/protobuf/stubs/stringprintf.h" #include "absl/strings/str_join.h" #include "absl/strings/str_split.h" @@ -5489,15 +5490,6 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto, } } -std::string GetJsonName(const FieldDescriptorProto& field, bool use_custom, bool* was_custom) { - if (use_custom && field.has_json_name()) { - *was_custom = true; - return field.json_name(); - } - *was_custom = false; - return ToJsonName(field.name()); -} - void DescriptorBuilder::CheckFieldJsonNameUniqueness( const DescriptorProto& proto, const Descriptor* result) { bool is_proto2 = result->file()->syntax() == FileDescriptor::SYNTAX_PROTO2; @@ -5507,52 +5499,68 @@ void DescriptorBuilder::CheckFieldJsonNameUniqueness( CheckFieldJsonNameUniqueness(message_name, proto, is_proto2, true); } +namespace { +// Helpers for function below + +std::string GetJsonName(const FieldDescriptorProto& field, bool use_custom, bool* was_custom) { + if (use_custom && field.has_json_name()) { + *was_custom = true; + return field.json_name(); + } + *was_custom = false; + return ToJsonName(field.name()); +} + struct JsonNameDetails { const FieldDescriptorProto* field; std::string orig_name; bool is_custom; }; +} // namespace + void DescriptorBuilder::CheckFieldJsonNameUniqueness( std::string message_name,const DescriptorProto& message, bool is_proto2, bool use_custom_names) { - std::map name_to_field; - for (int i = 0; i < message.field_size(); ++i) { + absl::flat_hash_map name_to_field; + for (const FieldDescriptorProto& field : message.field()) { bool is_custom; - std::string name = GetJsonName(message.field(i), use_custom_names, &is_custom); + std::string name = GetJsonName(field, use_custom_names, &is_custom); std::string lowercase_name = absl::AsciiStrToLower(name); auto existing = name_to_field.find(lowercase_name); if (existing != name_to_field.end()) { - auto match = existing->second; + JsonNameDetails& match = existing->second; if (use_custom_names && !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; } - std::string this_type = is_custom ? "custom" : "default"; - std::string existing_type = match.is_custom ? "custom" : "default"; + absl::string_view this_type = 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 = name == match.orig_name ? "" : " (\"" + match.orig_name + "\")"; - std::string error_message = - "The " + this_type + " JSON name of field \"" + message.field(i).name() + - "\" (\"" + name + "\") conflicts with the " + existing_type + " JSON name of field \"" + - match.field->name() + "\"" + name_suffix + "."; + absl::string_view name_suffix = ""; + if (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(), name, existing_type, match.field->name(), name_suffix); bool involves_default = !is_custom || !match.is_custom; if (is_proto2 && involves_default) { - AddWarning(message_name, message.field(i), + AddWarning(message_name, field, DescriptorPool::ErrorCollector::NAME, error_message); } else { if (involves_default) { - error_message += " This is not allowed in proto3."; + absl::StrAppend(&error_message, " This is not allowed in proto3."); } - AddError(message_name, message.field(i), + AddError(message_name, field, DescriptorPool::ErrorCollector::NAME, error_message); } } else { - JsonNameDetails details = { &message.field(i), name, is_custom }; + JsonNameDetails details = { &field, name, is_custom }; name_to_field[lowercase_name] = details; } } @@ -5961,12 +5969,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> + std::pair::iterator, bool> insert_result = values.insert(std::make_pair(stripped, value)); bool inserted = insert_result.second; @@ -5978,19 +5986,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). " - "(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 { - error_message += " This is not allowed in proto3."; + absl::StrAppend(&error_message, " This is not allowed in proto3."); AddError(value->full_name(), proto.value(i), DescriptorPool::ErrorCollector::NAME, error_message); } From d86340e37d0591d8e3ac237315d9fb8120b365f1 Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Thu, 15 Sep 2022 15:09:18 -0400 Subject: [PATCH 5/7] apply clang-format changes; change really long pair type to auto --- src/google/protobuf/descriptor.cc | 39 +++++++++++++++++-------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 3bdbf5c6ef4b..393be38294ef 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -5494,7 +5494,8 @@ void DescriptorBuilder::CheckFieldJsonNameUniqueness( const DescriptorProto& proto, const Descriptor* result) { bool is_proto2 = result->file()->syntax() == FileDescriptor::SYNTAX_PROTO2; std::string message_name = result->full_name(); - // two passes: one looking only at default JSON names, and one that considers custom JSON names + // two passes: one looking only at default JSON names, and one that considers + // custom JSON names CheckFieldJsonNameUniqueness(message_name, proto, is_proto2, false); CheckFieldJsonNameUniqueness(message_name, proto, is_proto2, true); } @@ -5502,7 +5503,8 @@ void DescriptorBuilder::CheckFieldJsonNameUniqueness( namespace { // Helpers for function below -std::string GetJsonName(const FieldDescriptorProto& field, bool use_custom, bool* was_custom) { +std::string GetJsonName(const FieldDescriptorProto& field, bool use_custom, + bool* was_custom) { if (use_custom && field.has_json_name()) { *was_custom = true; return field.json_name(); @@ -5520,7 +5522,8 @@ struct JsonNameDetails { } // namespace void DescriptorBuilder::CheckFieldJsonNameUniqueness( - std::string message_name,const DescriptorProto& message, bool is_proto2, bool use_custom_names) { + std::string message_name, const DescriptorProto& message, bool is_proto2, + bool use_custom_names) { absl::flat_hash_map name_to_field; for (const FieldDescriptorProto& field : message.field()) { @@ -5532,35 +5535,38 @@ void DescriptorBuilder::CheckFieldJsonNameUniqueness( JsonNameDetails& match = existing->second; if (use_custom_names && !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). + // 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 = 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. + // 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 (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(), name, existing_type, match.field->name(), name_suffix); + 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(), name, existing_type, + match.field->name(), name_suffix); bool involves_default = !is_custom || !match.is_custom; if (is_proto2 && involves_default) { - AddWarning(message_name, field, - DescriptorPool::ErrorCollector::NAME, error_message); + 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); + AddError(message_name, field, DescriptorPool::ErrorCollector::NAME, + error_message); } } else { - JsonNameDetails details = { &field, name, is_custom }; + JsonNameDetails details = {&field, name, is_custom}; name_to_field[lowercase_name] = details; } } @@ -5974,8 +5980,7 @@ void DescriptorBuilder::CheckEnumValueUniqueness( 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 From 8e74523ff6b5639ec988679634ca800fc927cda0 Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Wed, 21 Sep 2022 12:33:24 -0400 Subject: [PATCH 6/7] address review feedback --- .../protobuf/compiler/parser_unittest.cc | 126 ++++++++++-------- src/google/protobuf/descriptor.cc | 85 ++++++------ 2 files changed, 113 insertions(+), 98 deletions(-) diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc index 568a6997273e..e51b6e326036 100644 --- a/src/google/protobuf/compiler/parser_unittest.cc +++ b/src/google/protobuf/compiler/parser_unittest.cc @@ -1972,12 +1972,14 @@ 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 default JSON name of field \"Foo\" (\"Foo\") conflicts " + 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"); } @@ -1985,33 +1987,38 @@ TEST_F(ParserValidationErrorTest, Proto3JsonConflictError) { TEST_F(ParserValidationErrorTest, Proto2JsonConflictError) { // conflicts with default JSON names are not errors in proto2 ExpectParsesTo( - "syntax = 'proto2';\n" - "message TestMessage {\n" - " optional uint32 foo = 1;\n" - " optional uint32 Foo = 2;\n" - "}\n", - - "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" - " }" - "}" + 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( - "syntax = 'proto3';\n" - "message TestMessage {\n" - " uint32 foo = 1 [json_name='bar'];\n" - " uint32 bar = 2;\n" - "}\n", - "3:9: The default JSON name of field \"bar\" (\"bar\") conflicts " + 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"); } @@ -2019,45 +2026,52 @@ TEST_F(ParserValidationErrorTest, Proto3CustomJsonConflictWithDefaultError) { TEST_F(ParserValidationErrorTest, Proto2CustomJsonConflictWithDefaultError) { // conflicts with default JSON names are not errors in proto2 ExpectParsesTo( - "syntax = 'proto2';\n" - "message TestMessage {\n" - " optional uint32 foo = 1 [json_name='bar'];\n" - " optional uint32 bar = 2;\n" - "}\n", - - "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" - " }" - "}" + 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( - "syntax = 'proto3';\n" - "message TestMessage {\n" - " uint32 foo = 1 [json_name='baz'];\n" - " uint32 bar = 2 [json_name='baz'];\n" - "}\n", - "3:9: The custom JSON name of field \"bar\" (\"baz\") conflicts " + 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( - "syntax = 'proto2';\n" - "message TestMessage {\n" - " optional uint32 foo = 1 [json_name='baz'];\n" - " optional uint32 bar = 2 [json_name='baz'];\n" - "}\n", + 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 - "3:18: The custom JSON name of field \"bar\" (\"baz\") conflicts " + "4:24: The custom JSON name of field \"bar\" (\"baz\") conflicts " "with the custom JSON name of field \"foo\".\n"); } diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 393be38294ef..4dfdc287f591 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -3861,9 +3861,10 @@ class DescriptorBuilder { void CheckFieldJsonNameUniqueness(const DescriptorProto& proto, const Descriptor* result); - void CheckFieldJsonNameUniqueness(std::string message_name, + void CheckFieldJsonNameUniqueness(const std::string& message_name, const DescriptorProto& message, - bool is_proto2, bool use_custom_names); + FileDescriptor::Syntax syntax, + bool use_custom_names); void CheckEnumValueUniqueness(const EnumDescriptorProto& proto, const EnumDescriptor* result); @@ -5492,12 +5493,12 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto, void DescriptorBuilder::CheckFieldJsonNameUniqueness( const DescriptorProto& proto, const Descriptor* result) { - bool is_proto2 = result->file()->syntax() == FileDescriptor::SYNTAX_PROTO2; + 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, is_proto2, false); - CheckFieldJsonNameUniqueness(message_name, proto, is_proto2, true); + CheckFieldJsonNameUniqueness(message_name, proto, syntax, false); + CheckFieldJsonNameUniqueness(message_name, proto, syntax, true); } namespace { @@ -5522,8 +5523,8 @@ struct JsonNameDetails { } // namespace void DescriptorBuilder::CheckFieldJsonNameUniqueness( - std::string message_name, const DescriptorProto& message, bool is_proto2, - bool use_custom_names) { + 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()) { @@ -5531,43 +5532,43 @@ void DescriptorBuilder::CheckFieldJsonNameUniqueness( std::string name = GetJsonName(field, use_custom_names, &is_custom); std::string lowercase_name = absl::AsciiStrToLower(name); auto existing = name_to_field.find(lowercase_name); - if (existing != name_to_field.end()) { - JsonNameDetails& match = existing->second; - if (use_custom_names && !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 = 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 (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(), name, existing_type, - match.field->name(), name_suffix); - - bool involves_default = !is_custom || !match.is_custom; - if (is_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); - } - } else { + if (existing == name_to_field.end()) { JsonNameDetails details = {&field, name, is_custom}; name_to_field[lowercase_name] = details; + continue; + } + JsonNameDetails& match = existing->second; + if (use_custom_names && !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 = 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 (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(), name, existing_type, + match.field->name(), name_suffix); + + bool involves_default = !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); } } } From c7ba0553f511b00519ade94b976480efb37e85b2 Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Wed, 21 Sep 2022 12:54:20 -0400 Subject: [PATCH 7/7] return struct instead of using out param --- src/google/protobuf/descriptor.cc | 34 ++++++++++++++----------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 4dfdc287f591..43ea11042439 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -5504,22 +5504,20 @@ void DescriptorBuilder::CheckFieldJsonNameUniqueness( namespace { // Helpers for function below -std::string GetJsonName(const FieldDescriptorProto& field, bool use_custom, - bool* was_custom) { - if (use_custom && field.has_json_name()) { - *was_custom = true; - return field.json_name(); - } - *was_custom = false; - return ToJsonName(field.name()); -} - 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( @@ -5528,38 +5526,36 @@ void DescriptorBuilder::CheckFieldJsonNameUniqueness( absl::flat_hash_map name_to_field; for (const FieldDescriptorProto& field : message.field()) { - bool is_custom; - std::string name = GetJsonName(field, use_custom_names, &is_custom); - std::string lowercase_name = absl::AsciiStrToLower(name); + 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()) { - JsonNameDetails details = {&field, name, is_custom}; name_to_field[lowercase_name] = details; continue; } JsonNameDetails& match = existing->second; - if (use_custom_names && !is_custom && !match.is_custom) { + 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 = is_custom ? "custom" : "default"; + 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 (name != match.orig_name) { + 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(), name, existing_type, + this_type, field.name(), details.orig_name, existing_type, match.field->name(), name_suffix); - bool involves_default = !is_custom || !match.is_custom; + 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);