Chiebot-Mirror
/
protobuf
8
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

address review feedback

Browse Source
pull/10581/head
Josh Humphries 3 years ago
parent d86340e37d
commit 8e74523ff6
2 changed files with 113 additions and 98 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Unified View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 126
      src/google/protobuf/compiler/parser_unittest.cc
  2. 85
      src/google/protobuf/descriptor.cc

126
src/google/protobuf/compiler/parser_unittest.cc
Unescape View File

@ -1972,12 +1972,14 @@ TEST_F(ParserValidationErrorTest, Proto3Default) {
TEST_F(ParserValidationErrorTest, Proto3JsonConflictError) { TEST_F(ParserValidationErrorTest, Proto3JsonConflictError) {
ExpectHasValidationErrors( ExpectHasValidationErrors(
"syntax = 'proto3';\n" R"pb(
"message TestMessage {\n" syntax = 'proto3';
" uint32 foo = 1;\n" message TestMessage {
" uint32 Foo = 2;\n" uint32 foo = 1;
"}\n", uint32 Foo = 2;
"3:9: The default JSON name of field \"Foo\" (\"Foo\") conflicts " }
)pb",
"4:15: The default JSON name of field \"Foo\" (\"Foo\") conflicts "
"with the default JSON name of field \"foo\" (\"foo\"). " "with the default JSON name of field \"foo\" (\"foo\"). "
"This is not allowed in proto3.\n"); "This is not allowed in proto3.\n");
} }
@ -1985,33 +1987,38 @@ TEST_F(ParserValidationErrorTest, Proto3JsonConflictError) {
TEST_F(ParserValidationErrorTest, Proto2JsonConflictError) { TEST_F(ParserValidationErrorTest, Proto2JsonConflictError) {
// conflicts with default JSON names are not errors in proto2 // conflicts with default JSON names are not errors in proto2
ExpectParsesTo( ExpectParsesTo(
"syntax = 'proto2';\n" R"pb(
"message TestMessage {\n" syntax = "proto2";
" optional uint32 foo = 1;\n" message TestMessage {
" optional uint32 Foo = 2;\n" optional uint32 foo = 1;
"}\n", optional uint32 Foo = 2;
}
"syntax: 'proto2'" )pb",
"message_type {" R"pb(
" name: 'TestMessage'" syntax: 'proto2'
" field {" message_type {
" label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'foo' number: 1" name: 'TestMessage'
" }" field {
" field {" label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'foo' number: 1
" label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'Foo' number: 2" }
" }" field {
"}" label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'Foo' number: 2
}
}
)pb"
); );
} }
TEST_F(ParserValidationErrorTest, Proto3CustomJsonConflictWithDefaultError) { TEST_F(ParserValidationErrorTest, Proto3CustomJsonConflictWithDefaultError) {
ExpectHasValidationErrors( ExpectHasValidationErrors(
"syntax = 'proto3';\n" R"pb(
"message TestMessage {\n" syntax = 'proto3';
" uint32 foo = 1 [json_name='bar'];\n" message TestMessage {
" uint32 bar = 2;\n" uint32 foo = 1 [json_name='bar'];
"}\n", uint32 bar = 2;
"3:9: The default JSON name of field \"bar\" (\"bar\") conflicts " }
)pb",
"4:15: The default JSON name of field \"bar\" (\"bar\") conflicts "
"with the custom JSON name of field \"foo\". " "with the custom JSON name of field \"foo\". "
"This is not allowed in proto3.\n"); "This is not allowed in proto3.\n");
} }
@ -2019,45 +2026,52 @@ TEST_F(ParserValidationErrorTest, Proto3CustomJsonConflictWithDefaultError) {
TEST_F(ParserValidationErrorTest, Proto2CustomJsonConflictWithDefaultError) { TEST_F(ParserValidationErrorTest, Proto2CustomJsonConflictWithDefaultError) {
// conflicts with default JSON names are not errors in proto2 // conflicts with default JSON names are not errors in proto2
ExpectParsesTo( ExpectParsesTo(
"syntax = 'proto2';\n" R"pb(
"message TestMessage {\n" syntax = 'proto2';
" optional uint32 foo = 1 [json_name='bar'];\n" message TestMessage {
" optional uint32 bar = 2;\n" optional uint32 foo = 1 [json_name='bar'];
"}\n", optional uint32 bar = 2;
}
"syntax: 'proto2'" )pb",
"message_type {" R"pb(
" name: 'TestMessage'" syntax: 'proto2'
" field {" message_type {
" label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'foo' number: 1 json_name: 'bar'" name: 'TestMessage'
" }" field {
" field {" label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'foo' number: 1 json_name: 'bar'
" label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'bar' number: 2" }
" }" field {
"}" label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'bar' number: 2
}
}
)pb"
); );
} }
TEST_F(ParserValidationErrorTest, Proto3CustomJsonConflictError) { TEST_F(ParserValidationErrorTest, Proto3CustomJsonConflictError) {
ExpectHasValidationErrors( ExpectHasValidationErrors(
"syntax = 'proto3';\n" R"pb(
"message TestMessage {\n" syntax = 'proto3';
" uint32 foo = 1 [json_name='baz'];\n" message TestMessage {
" uint32 bar = 2 [json_name='baz'];\n" uint32 foo = 1 [json_name='baz'];
"}\n", uint32 bar = 2 [json_name='baz'];
"3:9: The custom JSON name of field \"bar\" (\"baz\") conflicts " }
)pb",
"4:15: The custom JSON name of field \"bar\" (\"baz\") conflicts "
"with the custom JSON name of field \"foo\".\n"); "with the custom JSON name of field \"foo\".\n");
} }
TEST_F(ParserValidationErrorTest, Proto2CustomJsonConflictError) { TEST_F(ParserValidationErrorTest, Proto2CustomJsonConflictError) {
ExpectHasValidationErrors( ExpectHasValidationErrors(
"syntax = 'proto2';\n" R"pb(
"message TestMessage {\n" syntax = 'proto2';
" optional uint32 foo = 1 [json_name='baz'];\n" message TestMessage {
" optional uint32 bar = 2 [json_name='baz'];\n" optional uint32 foo = 1 [json_name='baz'];
"}\n", optional uint32 bar = 2 [json_name='baz'];
}
)pb",
// fails in proto2 also: can't explicitly configure bad custom JSON names // 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"); "with the custom JSON name of field \"foo\".\n");
} }

85
src/google/protobuf/descriptor.cc
Unescape View File

@ -3861,9 +3861,10 @@ class DescriptorBuilder {
void CheckFieldJsonNameUniqueness(const DescriptorProto& proto, void CheckFieldJsonNameUniqueness(const DescriptorProto& proto,
const Descriptor* result); const Descriptor* result);
void CheckFieldJsonNameUniqueness(std::string message_name, void CheckFieldJsonNameUniqueness(const std::string& message_name,
const DescriptorProto& message, const DescriptorProto& message,
bool is_proto2, bool use_custom_names); FileDescriptor::Syntax syntax,
bool use_custom_names);
void CheckEnumValueUniqueness(const EnumDescriptorProto& proto, void CheckEnumValueUniqueness(const EnumDescriptorProto& proto,
const EnumDescriptor* result); const EnumDescriptor* result);
@ -5492,12 +5493,12 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto,
void DescriptorBuilder::CheckFieldJsonNameUniqueness( void DescriptorBuilder::CheckFieldJsonNameUniqueness(
const DescriptorProto& proto, const Descriptor* result) { 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(); std::string message_name = result->full_name();
// two passes: one looking only at default JSON names, and one that considers // two passes: one looking only at default JSON names, and one that considers
// custom JSON names // custom JSON names
CheckFieldJsonNameUniqueness(message_name, proto, is_proto2, false); CheckFieldJsonNameUniqueness(message_name, proto, syntax, false);
CheckFieldJsonNameUniqueness(message_name, proto, is_proto2, true); CheckFieldJsonNameUniqueness(message_name, proto, syntax, true);
} }
namespace { namespace {
@ -5522,8 +5523,8 @@ struct JsonNameDetails {
} // namespace } // namespace
void DescriptorBuilder::CheckFieldJsonNameUniqueness( void DescriptorBuilder::CheckFieldJsonNameUniqueness(
std::string message_name, const DescriptorProto& message, bool is_proto2, const std::string& message_name, const DescriptorProto& message,
bool use_custom_names) { FileDescriptor::Syntax syntax, bool use_custom_names) {
absl::flat_hash_map<std::string, JsonNameDetails> name_to_field; absl::flat_hash_map<std::string, JsonNameDetails> name_to_field;
for (const FieldDescriptorProto& field : message.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 name = GetJsonName(field, use_custom_names, &is_custom);
std::string lowercase_name = absl::AsciiStrToLower(name); std::string lowercase_name = absl::AsciiStrToLower(name);
auto existing = name_to_field.find(lowercase_name); auto existing = name_to_field.find(lowercase_name);
if (existing != name_to_field.end()) { 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 {
JsonNameDetails details = {&field, name, is_custom}; JsonNameDetails details = {&field, name, is_custom};
name_to_field[lowercase_name] = details; 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);
} }
} }
} }

Write Preview
Loading…
Cancel
Save