Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protoc: validate custom json_name configuration #10581

Merged
merged 7 commits into from
Sep 22, 2022
Merged

protoc: validate custom json_name configuration #10581

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c23d0b8
add check for custom JSON name conflicts
jhump Sep 14, 2022
cbd5c84
update existing test expectations and add new tests
jhump Sep 14, 2022
7c57fb0
JSON -> Json
jhump Sep 15, 2022
16627c5
address review feedback wrt absl string functions
jhump Sep 15, 2022
d86340e
apply clang-format changes; change really long pair type to auto
jhump Sep 15, 2022
8e74523
address review feedback
jhump Sep 21, 2022
c7ba055
return struct instead of using out param
jhump Sep 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 82 additions & 2 deletions src/google/protobuf/compiler/parser_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
jhump marked this conversation as resolved.
Show resolved Hide resolved
"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) {
Expand Down
147 changes: 102 additions & 45 deletions src/google/protobuf/descriptor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -3847,8 +3848,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,
Expand All @@ -3860,6 +3859,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);

Expand Down Expand Up @@ -5415,7 +5422,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++) {
Expand Down Expand Up @@ -5480,6 +5490,88 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto,
}
}

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

namespace {
// Helpers for function below

std::string GetJsonName(const FieldDescriptorProto& field, bool use_custom,
jhump marked this conversation as resolved.
Show resolved Hide resolved
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,
jhump marked this conversation as resolved.
Show resolved Hide resolved
jhump marked this conversation as resolved.
Show resolved Hide resolved
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()) {
jhump marked this conversation as resolved.
Show resolved Hide resolved
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 =
jhump marked this conversation as resolved.
Show resolved Hide resolved
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};
name_to_field[lowercase_name] = details;
}
}
}

void DescriptorBuilder::BuildFieldOrExtension(const FieldDescriptorProto& proto,
Descriptor* parent,
FieldDescriptor* result,
Expand Down Expand Up @@ -5883,13 +5975,12 @@ void DescriptorBuilder::CheckEnumValueUniqueness(
// NAME_TYPE_LAST_NAME = 2,
// }
PrefixRemover remover(result->name());
std::map<std::string, const EnumValueDescriptor*> values;
absl::flat_hash_map<std::string, const EnumValueDescriptor*> 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<std::map<std::string, const EnumValueDescriptor*>::iterator, bool>
insert_result = values.insert(std::make_pair(stripped, value));
auto insert_result = values.insert(std::make_pair(stripped, value));
jhump marked this conversation as resolved.
Show resolved Hide resolved
bool inserted = insert_result.second;

// We don't throw the error if the two conflicting symbols are identical, or
Expand All @@ -5900,19 +5991,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);
}
Expand Down Expand Up @@ -6779,20 +6869,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) {
Expand All @@ -6817,25 +6893,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<std::string, const FieldDescriptor*> 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,
Expand Down
35 changes: 16 additions & 19 deletions src/google/protobuf/descriptor_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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'"
Expand All @@ -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'"
Expand All @@ -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'"
Expand All @@ -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).
Expand Down Expand Up @@ -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' "
Expand All @@ -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");
}


Expand Down