Skip to content

Commit

Permalink
Fail building descriptors if ctype is used for fields other than st…
Browse files Browse the repository at this point in the history
…ring or bytes.

Instead of silently ignoring `[ctype = XXX]` for non string or bytes fields,
this CL starts failing to build descriptors to call out the issue. This may
cause failures to existing proto schemas but fixing them should be straightforward.

PiperOrigin-RevId: 602441330
  • Loading branch information
protobuf-github-bot authored and Copybara-Service committed Jan 29, 2024
1 parent 0d91515 commit 403e207
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 1 deletion.
23 changes: 23 additions & 0 deletions src/google/protobuf/descriptor.cc
Expand Up @@ -7735,6 +7735,29 @@ void DescriptorBuilder::ValidateOptions(const FieldDescriptor* field,

ValidateFieldFeatures(field, proto);

// The following check is temporarily OSS only till we fix all affected
// google3 TAP tests.
if (field->options().has_ctype()) {
if (field->cpp_type() != FieldDescriptor::CPPTYPE_STRING) {
AddError(
field->full_name(), proto, DescriptorPool::ErrorCollector::TYPE,
absl::StrFormat(
"Field %s specifies ctype, but is not a string nor bytes field.",
field->full_name())
.c_str());
}
if (field->options().ctype() == FieldOptions::CORD) {
if (field->is_extension()) {
AddError(field->full_name(), proto,
DescriptorPool::ErrorCollector::TYPE,
absl::StrFormat("Extension %s specifies ctype=CORD which is "
"not supported for extensions.",
field->full_name())
.c_str());
}
}
}

// Only message type fields may be lazy.
if (field->options().lazy() || field->options().unverified_lazy()) {
if (field->type() != FieldDescriptor::TYPE_MESSAGE) {
Expand Down
22 changes: 21 additions & 1 deletion src/google/protobuf/descriptor_unittest.cc
Expand Up @@ -2953,6 +2953,26 @@ TEST_F(MiscTest, DefaultValues) {
EXPECT_EQ(enum_value_a, message->field(22)->default_value_enum());
}

TEST_F(MiscTest, InvalidFieldOptions) {
FileDescriptorProto file_proto;
file_proto.set_name("foo.proto");

DescriptorProto* message_proto = AddMessage(&file_proto, "TestMessage");
AddField(message_proto, "foo", 1, FieldDescriptorProto::LABEL_OPTIONAL,
FieldDescriptorProto::TYPE_INT32);
FieldDescriptorProto* bar_proto =
AddField(message_proto, "bar", 2, FieldDescriptorProto::LABEL_OPTIONAL,
FieldDescriptorProto::TYPE_INT32);

FieldOptions* options = bar_proto->mutable_options();
options->set_ctype(FieldOptions::CORD);

// Expects it to fail as int32 fields cannot have ctype.
DescriptorPool pool;
const FileDescriptor* file = pool.BuildFile(file_proto);
EXPECT_EQ(file, nullptr);
}

TEST_F(MiscTest, FieldOptions) {
// Try setting field options.

Expand All @@ -2964,7 +2984,7 @@ TEST_F(MiscTest, FieldOptions) {
FieldDescriptorProto::TYPE_INT32);
FieldDescriptorProto* bar_proto =
AddField(message_proto, "bar", 2, FieldDescriptorProto::LABEL_OPTIONAL,
FieldDescriptorProto::TYPE_INT32);
FieldDescriptorProto::TYPE_BYTES);

FieldOptions* options = bar_proto->mutable_options();
options->set_ctype(FieldOptions::CORD);
Expand Down

0 comments on commit 403e207

Please sign in to comment.