Skip to content

Commit

Permalink
address review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@jhump @mkruskal-google
jhump authored and mkruskal-google committed Nov 1, 2022
1 parent 6d5f2fc commit 81b5cbe
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 98 deletions.
126 changes: 70 additions & 56 deletions src/google/protobuf/compiler/parser_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2069,92 +2069,106 @@ 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");
}

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");
}

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");
}

Expand Down
85 changes: 43 additions & 42 deletions src/google/protobuf/descriptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3867,9 +3867,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);

Expand Down Expand Up @@ -5498,12 +5499,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 {
Expand All @@ -5528,52 +5529,52 @@ 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<std::string, JsonNameDetails> 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);
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);
}
}
}
Expand Down

0 comments on commit 81b5cbe

Please sign in to comment.