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

Jump to bottom

protoc: validate custom json_name configuration #10581

Merged
merged 7 commits into from
Sep 22, 2022
Merged

protoc: validate custom json_name configuration #10581

Changes from 1 commit
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
39 changes: 22 additions & 17 deletions src/google/protobuf/descriptor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5494,15 +5494,17 @@ 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);
}

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,
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();
Expand All @@ -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,
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()) {
Expand All @@ -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 =
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);
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;
}
}
Expand Down Expand Up @@ -5974,8 +5980,7 @@ void DescriptorBuilder::CheckEnumValueUniqueness(
const EnumValueDescriptor* value = result->value(i);
std::string stripped =
EnumValueToPascalCase(remover.MaybeRemove(value->name()));
std::pair<absl::flat_hash_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 Down