Skip to content

Commit

Permalink
Editions: Migrate string_field_validation to a C++ feature
Browse files Browse the repository at this point in the history 
Every language has very different handling of utf8 validation.  Any with proto2/proto3 differences will receive language-specific features for edition zero to better model these subtle differences.

PiperOrigin-RevId: 552625656
  • Loading branch information
@mkruskal-google @copybara-github
mkruskal-google authored and copybara-github committed Aug 15, 2023
1 parent 15863ff commit 22f1d03
Show file tree
Hide file tree
Showing 20 changed files with 226 additions and 236 deletions.
2 changes: 0 additions & 2 deletions src/google/protobuf/compiler/command_line_interface_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1560,7 +1560,6 @@ TEST_F(CommandLineInterfaceTest, Plugin_RuntimeFeatures) {
EqualsProto(R"pb(field_presence: IMPLICIT
enum_type: OPEN
repeated_field_encoding: PACKED
string_field_validation: MANDATORY
message_encoding: LENGTH_PREFIXED
json_format: ALLOW
raw_features { field_presence: IMPLICIT }
Expand All @@ -1577,7 +1576,6 @@ TEST_F(CommandLineInterfaceTest, Plugin_RuntimeFeatures) {
EqualsProto(R"pb(field_presence: IMPLICIT
enum_type: OPEN
repeated_field_encoding: PACKED
string_field_validation: MANDATORY
message_encoding: LENGTH_PREFIXED
json_format: ALLOW
raw_features { field_presence: IMPLICIT }
Expand Down
48 changes: 42 additions & 6 deletions src/google/protobuf/compiler/cpp/generator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,17 @@ absl::flat_hash_map<absl::string_view, std::string> CommonVars(
};
}

static bool IsStringMapType(const FieldDescriptor& field) {
if (!field.is_map()) return false;
for (int i = 0; i < field.message_type()->field_count(); ++i) {
if (field.message_type()->field(i)->type() ==
FieldDescriptor::TYPE_STRING) {
return true;
}
}
return false;
}

} // namespace

bool CppGenerator::Generate(const FileDescriptor* file,
Expand Down Expand Up @@ -356,19 +367,44 @@ absl::Status CppGenerator::ValidateFeatures(const FileDescriptor* file) const {
google::protobuf::internal::VisitDescriptors(*file, [&](const FieldDescriptor& field) {
const FeatureSet& source_features = GetSourceFeatures(field);
const FeatureSet& raw_features = GetSourceRawFeatures(field);
if (raw_features.GetExtension(::pb::cpp).has_legacy_closed_enum() &&
field.cpp_type() != FieldDescriptor::CPPTYPE_ENUM) {
status = absl::FailedPreconditionError(absl::StrCat(
"Field ", field.full_name(),
" specifies the legacy_closed_enum feature but has non-enum type."));
}
if (field.enum_type() != nullptr &&
source_features.GetExtension(::pb::cpp).legacy_closed_enum() &&
source_features.field_presence() == FeatureSet::IMPLICIT) {
status = absl::FailedPreconditionError(
absl::StrCat("Field ", field.full_name(),
" has a closed enum type with implicit presence."));
}
if (source_features.GetExtension(::pb::cpp).utf8_validation() ==
pb::CppFeatures::UTF8_VALIDATION_UNKNOWN) {
status = absl::FailedPreconditionError(absl::StrCat(
"Field ", field.full_name(),
" has an unknown value for the utf8_validation feature. ",
source_features.DebugString(), "\nRawFeatures: ", raw_features));
}

if (field.containing_type() == nullptr ||
!field.containing_type()->options().map_entry()) {
// Skip validation of explicit features on generated map fields. These
// will be blindly propagated from the original map field, and may violate
// a lot of these conditions. Note: we do still validate the
// user-specified map field.
if (raw_features.GetExtension(::pb::cpp).has_legacy_closed_enum() &&
field.cpp_type() != FieldDescriptor::CPPTYPE_ENUM) {
status = absl::FailedPreconditionError(
absl::StrCat("Field ", field.full_name(),
" specifies the legacy_closed_enum feature but has "
"non-enum type."));
}
if (field.type() != FieldDescriptor::TYPE_STRING &&
!IsStringMapType(field) &&
raw_features.GetExtension(::pb::cpp).has_utf8_validation()) {
status = absl::FailedPreconditionError(
absl::StrCat("Field ", field.full_name(),
" specifies the utf8_validation feature but is not of "
"string type."));
}
}

#ifdef PROTOBUF_FUTURE_REMOVE_WRONG_CTYPE
if (field.options().has_ctype()) {
if (field.cpp_type() != FieldDescriptor::CPPTYPE_STRING) {
Expand Down
70 changes: 66 additions & 4 deletions src/google/protobuf/compiler/cpp/generator_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ TEST_F(CppGeneratorTest, BasicError) {
"foo.proto:4:7: Expected \"required\", \"optional\", or \"repeated\"");
}


TEST_F(CppGeneratorTest, LegacyClosedEnumOnNonEnumField) {
CreateTempFile("foo.proto",
R"schema(
Expand Down Expand Up @@ -150,8 +149,7 @@ TEST_F(CppGeneratorTest, LegacyClosedEnumInherited) {
}

TEST_F(CppGeneratorTest, LegacyClosedEnumImplicit) {
CreateTempFile("foo.proto",
R"schema(
CreateTempFile("foo.proto", R"schema(
edition = "2023";
import "google/protobuf/cpp_features.proto";
option features.(pb.cpp).legacy_closed_enum = true;
Expand All @@ -162,7 +160,8 @@ TEST_F(CppGeneratorTest, LegacyClosedEnumImplicit) {
message Foo {
TestEnum bar = 1 [features.field_presence = IMPLICIT];
int32 baz = 2;
})schema");
}
)schema");

RunProtoc(
"protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir "
Expand All @@ -172,6 +171,69 @@ TEST_F(CppGeneratorTest, LegacyClosedEnumImplicit) {
"Field Foo.bar has a closed enum type with implicit presence.");
}

TEST_F(CppGeneratorTest, Utf8ValidationMap) {
CreateTempFile("foo.proto", R"schema(
edition = "2023";
import "google/protobuf/cpp_features.proto";
message Foo {
map<string, string> map_field = 1 [
features.(pb.cpp).utf8_validation = NONE
];
map<int32, string> map_field_value = 2 [
features.(pb.cpp).utf8_validation = VERIFY_PARSE
];
map<string, int32> map_field_key = 3 [
features.(pb.cpp).utf8_validation = VERIFY_DLOG
];
}
)schema");

RunProtoc(
"protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir "
"--experimental_editions foo.proto");

ExpectNoErrors();
}

TEST_F(CppGeneratorTest, Utf8ValidationNonString) {
CreateTempFile("foo.proto", R"schema(
edition = "2023";
import "google/protobuf/cpp_features.proto";
message Foo {
int64 bar = 1 [
features.(pb.cpp).utf8_validation = NONE
];
}
)schema");

RunProtoc(
"protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir "
"--experimental_editions foo.proto");

ExpectErrorSubstring("Field Foo.bar specifies the utf8_validation feature");
}

TEST_F(CppGeneratorTest, Utf8ValidationNonStringMap) {
CreateTempFile("foo.proto", R"schema(
edition = "2023";
import "google/protobuf/cpp_features.proto";
message Foo {
map<int64, int64> bar = 1 [
features.(pb.cpp).utf8_validation = VERIFY_PARSE
];
}
)schema");

RunProtoc(
"protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir "
"--experimental_editions foo.proto");

ExpectErrorSubstring("Field Foo.bar specifies the utf8_validation feature");
}

#ifdef PROTOBUF_FUTURE_REMOVE_WRONG_CTYPE
TEST_F(CppGeneratorTest, CtypeOnNoneStringFieldTest) {
CreateTempFile("foo.proto",
Expand Down
2 changes: 1 addition & 1 deletion src/google/protobuf/compiler/parser_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4243,7 +4243,7 @@ TEST_F(ParseEditionsTest, InvalidMerge) {
string foo = 1 [
default = "hello",
features.field_presence = FIELD_PRESENCE_UNKNOWN,
features.string_field_validation = STRING_FIELD_VALIDATION_UNKNOWN
features.enum_type = ENUM_TYPE_UNKNOWN
];
})schema",
"5:17: Feature field google.protobuf.FeatureSet.field_presence must resolve to a "
Expand Down
27 changes: 7 additions & 20 deletions src/google/protobuf/descriptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1117,10 +1117,11 @@ FeatureSet* CreateProto2DefaultFeatures() {
features->set_field_presence(FeatureSet::EXPLICIT);
features->set_enum_type(FeatureSet::CLOSED);
features->set_repeated_field_encoding(FeatureSet::EXPANDED);
features->set_string_field_validation(FeatureSet::HINT);
features->set_message_encoding(FeatureSet::LENGTH_PREFIXED);
features->set_json_format(FeatureSet::LEGACY_BEST_EFFORT);
features->MutableExtension(pb::cpp)->set_legacy_closed_enum(true);
features->MutableExtension(pb::cpp)->set_utf8_validation(
pb::CppFeatures::VERIFY_DLOG);

return features;
}
Expand All @@ -1145,9 +1146,11 @@ const FeatureSet& GetProto3Features() {
features->set_field_presence(FeatureSet::IMPLICIT);
features->set_enum_type(FeatureSet::OPEN);
features->set_repeated_field_encoding(FeatureSet::PACKED);
features->set_string_field_validation(FeatureSet::MANDATORY);
features->set_message_encoding(FeatureSet::LENGTH_PREFIXED);
features->set_json_format(FeatureSet::ALLOW);
features->MutableExtension(pb::cpp)->set_legacy_closed_enum(false);
features->MutableExtension(pb::cpp)->set_utf8_validation(
pb::CppFeatures::VERIFY_PARSE);
return features;
}();
return *kProto3Features;
Expand Down Expand Up @@ -3817,7 +3820,8 @@ bool FieldDescriptor::is_packed() const {

static bool IsStrictUtf8(const FieldDescriptor* field) {
return internal::InternalFeatureHelper::GetFeatures(*field)
.string_field_validation() == FeatureSet::MANDATORY;
.GetExtension(pb::cpp)
.utf8_validation() == pb::CppFeatures::VERIFY_PARSE;
}

bool FieldDescriptor::requires_utf8_validation() const {
Expand Down Expand Up @@ -7838,17 +7842,6 @@ void DescriptorBuilder::ValidateOptions(const FieldDescriptor* field,

}

static bool IsStringMapType(const FieldDescriptor* field) {
if (!field->is_map()) return false;
for (int i = 0; i < field->message_type()->field_count(); ++i) {
if (field->message_type()->field(i)->type() ==
FieldDescriptor::TYPE_STRING) {
return true;
}
}
return false;
}

void DescriptorBuilder::ValidateFieldFeatures(
const FieldDescriptor* field, const FieldDescriptorProto& proto) {
// Rely on our legacy validation for proto2/proto3 files.
Expand Down Expand Up @@ -7895,12 +7888,6 @@ void DescriptorBuilder::ValidateFieldFeatures(
field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
"Only singular scalar fields can specify required field presence.");
}
if (field->type() != FieldDescriptor::TYPE_STRING &&
!IsStringMapType(field) &&
field->proto_features_->has_string_field_validation()) {
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
"Only string fields can specify `string_field_validation`.");
}
if (!field->is_repeated() &&
field->proto_features_->has_repeated_field_encoding()) {
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
Expand Down
Loading

0 comments on commit 22f1d03

Please sign in to comment.