Skip to content

Commit

Permalink
Fix validation checks of implicit presence.
Browse files Browse the repository at this point in the history
Instead of checking the resolved features, we should really be checking the has_presence helper.  Repeated fields, oneofs, and extensions can trigger these conditions when they inherit IMPLICIT, even though it's ignored.

Closes #16664

PiperOrigin-RevId: 630206208
  • Loading branch information
mkruskal-google authored and copybara-github committed May 2, 2024
1 parent 0dbd99a commit d6c2833
Show file tree
Hide file tree
Showing 2 changed files with 128 additions and 11 deletions.
20 changes: 10 additions & 10 deletions src/google/protobuf/descriptor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8084,16 +8084,16 @@ void DescriptorBuilder::ValidateFieldFeatures(
}

// Validate fully resolved features.
if (field->has_default_value() &&
field->features().field_presence() == FeatureSet::IMPLICIT) {
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
"Implicit presence fields can't specify defaults.");
}
if (field->enum_type() != nullptr &&
field->enum_type()->features().enum_type() != FeatureSet::OPEN &&
field->features().field_presence() == FeatureSet::IMPLICIT) {
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
"Implicit presence enum fields must always be open.");
if (!field->is_repeated() && !field->has_presence()) {
if (field->has_default_value()) {
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
"Implicit presence fields can't specify defaults.");
}
if (field->enum_type() != nullptr &&
field->enum_type()->features().enum_type() != FeatureSet::OPEN) {
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
"Implicit presence enum fields must always be open.");
}
}
if (field->is_extension() &&
field->features().field_presence() == FeatureSet::LEGACY_REQUIRED) {
Expand Down
119 changes: 118 additions & 1 deletion src/google/protobuf/descriptor_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9830,7 +9830,62 @@ TEST_F(FeaturesTest, InvalidFieldImplicitDefault) {
"defaults.\n");
}

TEST_F(FeaturesTest, InvalidFieldImplicitOpen) {
TEST_F(FeaturesTest, ValidExtensionFieldImplicitDefault) {
BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = BuildFile(
R"pb(
name: "foo.proto"
syntax: "editions"
edition: EDITION_2023
options { features { field_presence: IMPLICIT } }
message_type {
name: "Foo"
extension_range { start: 1 end: 100 }
}
extension {
name: "bar"
number: 1
label: LABEL_OPTIONAL
type: TYPE_STRING
default_value: "Hello world"
extendee: "Foo"
}
)pb");
ASSERT_THAT(file, NotNull());

EXPECT_TRUE(file->extension(0)->has_presence());
EXPECT_EQ(file->extension(0)->default_value_string(), "Hello world");
}

TEST_F(FeaturesTest, ValidOneofFieldImplicitDefault) {
BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = BuildFile(
R"pb(
name: "foo.proto"
syntax: "editions"
edition: EDITION_2023
options { features { field_presence: IMPLICIT } }
message_type {
name: "Foo"
field {
name: "bar"
number: 1
label: LABEL_OPTIONAL
type: TYPE_STRING
default_value: "Hello world"
oneof_index: 0
}
oneof_decl { name: "_foo" }
}
)pb");
ASSERT_THAT(file, NotNull());

EXPECT_TRUE(file->message_type(0)->field(0)->has_presence());
EXPECT_EQ(file->message_type(0)->field(0)->default_value_string(),
"Hello world");
}

TEST_F(FeaturesTest, InvalidFieldImplicitClosed) {
BuildDescriptorMessagesInTestPool();
BuildFileWithErrors(
R"pb(
Expand Down Expand Up @@ -9858,6 +9913,68 @@ TEST_F(FeaturesTest, InvalidFieldImplicitOpen) {
"be open.\n");
}

TEST_F(FeaturesTest, ValidRepeatedFieldImplicitClosed) {
BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = BuildFile(
R"pb(
name: "foo.proto"
syntax: "editions"
edition: EDITION_2023
options { features { field_presence: IMPLICIT } }
message_type {
name: "Foo"
field {
name: "bar"
number: 1
label: LABEL_REPEATED
type: TYPE_ENUM
type_name: "Enum"
}
}
enum_type {
name: "Enum"
value { name: "BAR" number: 0 }
options { features { enum_type: CLOSED } }
}
)pb");
ASSERT_THAT(file, NotNull());

EXPECT_FALSE(file->message_type(0)->field(0)->has_presence());
EXPECT_TRUE(file->enum_type(0)->is_closed());
}

TEST_F(FeaturesTest, ValidOneofFieldImplicitClosed) {
BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = BuildFile(
R"pb(
name: "foo.proto"
syntax: "editions"
edition: EDITION_2023
options { features { field_presence: IMPLICIT } }
message_type {
name: "Foo"
field {
name: "bar"
number: 1
label: LABEL_OPTIONAL
type: TYPE_ENUM
type_name: "Enum"
oneof_index: 0
}
oneof_decl { name: "_foo" }
}
enum_type {
name: "Enum"
value { name: "BAR" number: 0 }
options { features { enum_type: CLOSED } }
}
)pb");
ASSERT_THAT(file, NotNull());

EXPECT_TRUE(file->message_type(0)->field(0)->has_presence());
EXPECT_TRUE(file->enum_type(0)->is_closed());
}

TEST_F(FeaturesTest, InvalidFieldRequiredExtension) {
BuildDescriptorMessagesInTestPool();
BuildFileWithErrors(
Expand Down

0 comments on commit d6c2833

Please sign in to comment.