Fix validation checks of implicit presence.

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
pull/16729/head
Mike Kruskal 7 months ago
parent 61c91874c6
commit bdf6b10e7c
  1. 20
      src/google/protobuf/descriptor.cc
  2. 119
      src/google/protobuf/descriptor_unittest.cc

@ -8074,16 +8074,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) {

@ -9759,7 +9759,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(
@ -9787,6 +9842,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(

Loading…
Cancel
Save