Chiebot-Mirror
/
protobuf
mirror of https://github.com/protocolbuffers/protobuf.git
8
0
Fork 0
Code Issues Projects Releases Wiki Activity

Revert "Merge pull request #10581 from jhump/jh/custom-json-name-validation" (#10657)

Browse Source 
This reverts commit d3b5389301, reversing
changes made to bcd175578f.
pull/10662/head
Mike Kruskal 2 years ago committed by GitHub
parent 1e24391438
commit d58d471117
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 71 additions and 215 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 108
      src/google/protobuf/compiler/parser_unittest.cc
  2. 143
      src/google/protobuf/descriptor.cc
  3. 35
      src/google/protobuf/descriptor_unittest.cc

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

@ -2069,107 +2069,13 @@ TEST_F(ParserValidationErrorTest, Proto3Default) {
TEST_F(ParserValidationErrorTest, Proto3JsonConflictError) {
ExpectHasValidationErrors(
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");
"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");
}
TEST_F(ParserValidationErrorTest, EnumNameError) {

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

@ -3855,6 +3855,8 @@ 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,
@ -3866,15 +3868,6 @@ 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);
@ -5430,10 +5423,7 @@ 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++) {
@ -5498,84 +5488,6 @@ 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<std::string, JsonNameDetails> 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.
std::string 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,
@ -5979,12 +5891,13 @@ void DescriptorBuilder::CheckEnumValueUniqueness(
// NAME_TYPE_LAST_NAME = 2,
// }
PrefixRemover remover(result->name());
absl::flat_hash_map<std::string, const EnumValueDescriptor*> values;
std::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()));
auto insert_result = values.insert(std::make_pair(stripped, value));
std::pair<std::map<std::string, const EnumValueDescriptor*>::iterator, bool>
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
@ -5995,18 +5908,19 @@ 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 = 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());
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.";
// 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);
}
@ -6873,6 +6787,20 @@ 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) {
@ -6897,6 +6825,25 @@ 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,

35
src/google/protobuf/descriptor_unittest.cc
Unescape View File

@ -6478,8 +6478,9 @@ 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). "
"(If you are using allow_alias, please assign the same numeric "
"value to both enums.) This is not allowed in proto3.\n");
"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");
// Not an error because both enums are mapped to the same value.
BuildFile(
@ -6505,8 +6506,9 @@ 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). "
"(If you are using allow_alias, please assign the same numeric value "
"to both enums.) This is not allowed in proto3.\n");
"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");
BuildFileWithErrors(
"syntax: 'proto3'"
@ -6518,8 +6520,9 @@ 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). "
"(If you are using allow_alias, please assign the same numeric value "
"to both enums.) This is not allowed in proto3.\n");
"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");
BuildFileWithErrors(
"syntax: 'proto3'"
@ -6531,8 +6534,9 @@ 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). (If you are using allow_alias, please assign the same numeric "
"value to both enums.) This is not allowed in proto3.\n");
"(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");
BuildFileWithErrors(
"syntax: 'proto3'"
@ -6544,8 +6548,9 @@ 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). (If you are using allow_alias, please assign the same numeric "
"value to both enums.) This is not allowed in proto3.\n");
"(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");
// This isn't an error because the underscore will cause the PascalCase to
// differ by case (BarBaz vs. Barbaz).
@ -6890,9 +6895,8 @@ 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 default JSON name of field \"Name\" (\"Name\") "
"conflicts with the default JSON name of field \"name\" (\"name\"). This is "
"not allowed in proto3.\n");
"foo.proto: Foo: NAME: The JSON camel-case name of field \"Name\" "
"conflicts with field \"name\". This is not allowed in proto3.\n");
// Underscores are ignored.
BuildFileWithErrors(
"name: 'foo.proto' "
@ -6902,9 +6906,8 @@ 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 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");
"foo.proto: Foo: NAME: The JSON camel-case name of field \"_a__b_\" "
"conflicts with field \"ab\". This is not allowed in proto3.\n");
}

Write Preview
Loading…
Cancel
Save