diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc index af99450f50692..51efa7d08ba9f 100644 --- a/src/google/protobuf/compiler/command_line_interface_unittest.cc +++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc @@ -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 } @@ -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 } diff --git a/src/google/protobuf/compiler/cpp/generator.cc b/src/google/protobuf/compiler/cpp/generator.cc index 442de6a9f341c..22b696f5a267a 100644 --- a/src/google/protobuf/compiler/cpp/generator.cc +++ b/src/google/protobuf/compiler/cpp/generator.cc @@ -101,6 +101,17 @@ absl::flat_hash_map 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, @@ -356,12 +367,6 @@ 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) { @@ -369,6 +374,37 @@ absl::Status CppGenerator::ValidateFeatures(const FileDescriptor* file) const { 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) { diff --git a/src/google/protobuf/compiler/cpp/generator_unittest.cc b/src/google/protobuf/compiler/cpp/generator_unittest.cc index b5ac6bc1b4287..e96d3a083f8e8 100644 --- a/src/google/protobuf/compiler/cpp/generator_unittest.cc +++ b/src/google/protobuf/compiler/cpp/generator_unittest.cc @@ -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( @@ -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; @@ -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 " @@ -172,6 +171,88 @@ 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 map_field = 1 [ + features.(pb.cpp).utf8_validation = NONE + ]; + map map_field_value = 2 [ + features.(pb.cpp).utf8_validation = VERIFY_PARSE + ]; + map 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 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"); +} + +TEST_F(CppGeneratorTest, Utf8ValidationUnknownValue) { + CreateTempFile("foo.proto", R"schema( + edition = "2023"; + import "google/protobuf/cpp_features.proto"; + + message Foo { + string bar = 1 [ + features.(pb.cpp).utf8_validation = UTF8_VALIDATION_UNKNOWN + ]; + } + )schema"); + + RunProtoc( + "protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir " + "--experimental_editions foo.proto"); + + ExpectErrorSubstring( + "Field Foo.bar has an unknown value for the utf8_validation feature."); +} #ifdef PROTOBUF_FUTURE_REMOVE_WRONG_CTYPE TEST_F(CppGeneratorTest, CtypeOnNoneStringFieldTest) { CreateTempFile("foo.proto", diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc index 31a0c78b822b0..b94358be7cf0c 100644 --- a/src/google/protobuf/compiler/parser_unittest.cc +++ b/src/google/protobuf/compiler/parser_unittest.cc @@ -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 " diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 2b4bfc5b064a2..09b60ad81259e 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -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; } @@ -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; @@ -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 { @@ -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. @@ -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, diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index 51e483cecd2a3..93b038f628c1a 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -63,8 +63,11 @@ #include "absl/strings/str_format.h" #include "absl/strings/string_view.h" #include "absl/strings/substitute.h" +#include "google/protobuf/compiler/parser.h" #include "google/protobuf/cpp_features.pb.h" #include "google/protobuf/descriptor_legacy.h" +#include "google/protobuf/io/tokenizer.h" +#include "google/protobuf/io/zero_copy_stream_impl_lite.h" #include "google/protobuf/test_textproto.h" #include "google/protobuf/unittest.pb.h" #include "google/protobuf/unittest_custom_options.pb.h" @@ -7254,26 +7257,32 @@ TEST_F(FeaturesTest, Proto2Features) { field_presence: EXPLICIT enum_type: CLOSED repeated_field_encoding: EXPANDED - string_field_validation: HINT message_encoding: LENGTH_PREFIXED json_format: LEGACY_BEST_EFFORT - [pb.cpp] { legacy_closed_enum: true })pb")); + [pb.cpp] { + legacy_closed_enum: true + utf8_validation: VERIFY_DLOG + })pb")); EXPECT_THAT(GetFeatures(field), EqualsProto(R"pb( field_presence: EXPLICIT enum_type: CLOSED repeated_field_encoding: EXPANDED - string_field_validation: HINT message_encoding: LENGTH_PREFIXED json_format: LEGACY_BEST_EFFORT - [pb.cpp] { legacy_closed_enum: true })pb")); + [pb.cpp] { + legacy_closed_enum: true + utf8_validation: VERIFY_DLOG + })pb")); EXPECT_THAT(GetFeatures(group), EqualsProto(R"pb( field_presence: EXPLICIT enum_type: CLOSED repeated_field_encoding: EXPANDED - string_field_validation: HINT message_encoding: DELIMITED json_format: LEGACY_BEST_EFFORT - [pb.cpp] { legacy_closed_enum: true })pb")); + [pb.cpp] { + legacy_closed_enum: true + utf8_validation: VERIFY_DLOG + })pb")); EXPECT_TRUE(field->has_presence()); EXPECT_FALSE(field->requires_utf8_validation()); EXPECT_FALSE(field->is_packed()); @@ -7324,16 +7333,22 @@ TEST_F(FeaturesTest, Proto3Features) { field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); EXPECT_THAT(GetFeatures(field), EqualsProto(R"pb( field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); EXPECT_FALSE(field->has_presence()); EXPECT_FALSE(field->requires_utf8_validation()); EXPECT_FALSE(field->is_packed()); @@ -7463,7 +7478,6 @@ TEST_F(FeaturesTest, Edition2023Defaults) { field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { legacy_closed_enum: false utf8_validation: VERIFY_PARSE } @@ -7491,7 +7505,6 @@ TEST_F(FeaturesTest, ClearsOptions) { field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -7754,7 +7767,6 @@ TEST_F(FeaturesTest, NoOptions) { field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -7787,7 +7799,6 @@ TEST_F(FeaturesTest, FileFeatures) { field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -7865,7 +7876,6 @@ TEST_F(FeaturesTest, MessageFeaturesDefault) { field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -7973,7 +7983,6 @@ TEST_F(FeaturesTest, FieldFeaturesDefault) { field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -8211,52 +8220,6 @@ TEST_F(FeaturesTest, MapFieldFeaturesOverride) { validate(value); } -TEST_F(FeaturesTest, MapFieldFeaturesStringValidation) { - constexpr absl::string_view kProtoFile = R"schema( - edition = "2023"; - - message Foo { - map map_field = 1 [ - features.string_field_validation = HINT - ]; - map map_field_value = 2 [ - features.string_field_validation = HINT - ]; - map map_field_key = 3 [ - features.string_field_validation = HINT - ]; - } - )schema"; - io::ArrayInputStream input_stream(kProtoFile.data(), kProtoFile.size()); - SimpleErrorCollector error_collector; - io::Tokenizer tokenizer(&input_stream, &error_collector); - compiler::Parser parser; - parser.RecordErrorsTo(&error_collector); - FileDescriptorProto proto; - ASSERT_TRUE(parser.Parse(&tokenizer, &proto)) - << error_collector.last_error() << "\n" - << kProtoFile; - ASSERT_EQ("", error_collector.last_error()); - proto.set_name("foo.proto"); - - BuildDescriptorMessagesInTestPool(); - const FileDescriptor* file = pool_.BuildFile(proto); - ASSERT_THAT(file, NotNull()); - - auto validate_map_field = [](const FieldDescriptor* field) { - const FieldDescriptor* key = field->message_type()->field(0); - const FieldDescriptor* value = field->message_type()->field(1); - - EXPECT_FALSE(field->requires_utf8_validation()) << field->DebugString(); - EXPECT_FALSE(key->requires_utf8_validation()) << field->DebugString(); - EXPECT_FALSE(value->requires_utf8_validation()) << field->DebugString(); - }; - - validate_map_field(file->message_type(0)->field(0)); - validate_map_field(file->message_type(0)->field(1)); - validate_map_field(file->message_type(0)->field(2)); -} - TEST_F(FeaturesTest, RootExtensionFeaturesOverride) { BuildDescriptorMessagesInTestPool(); BuildFileInTestPool(pb::TestFeatures::descriptor()->file()); @@ -8364,7 +8327,6 @@ TEST_F(FeaturesTest, EnumFeaturesDefault) { field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -8474,7 +8436,6 @@ TEST_F(FeaturesTest, EnumValueFeaturesDefault) { field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -8569,7 +8530,6 @@ TEST_F(FeaturesTest, OneofFeaturesDefault) { field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -8671,7 +8631,6 @@ TEST_F(FeaturesTest, ExtensionRangeFeaturesDefault) { field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -8758,7 +8717,6 @@ TEST_F(FeaturesTest, ServiceFeaturesDefault) { field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -8827,7 +8785,6 @@ TEST_F(FeaturesTest, MethodFeaturesDefault) { field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -8908,9 +8865,11 @@ TEST_F(FeaturesTest, MethodFeaturesOverride) { TEST_F(FeaturesTest, FieldFeatureHelpers) { BuildDescriptorMessagesInTestPool(); + BuildFileInTestPool(pb::CppFeatures::GetDescriptor()->file()); const FileDescriptor* file = BuildFile(R"pb( name: "foo.proto" syntax: "editions" + dependency: "google/protobuf/cpp_features.proto" edition: "2023" message_type { name: "Foo" @@ -8950,14 +8909,22 @@ TEST_F(FeaturesTest, FieldFeatureHelpers) { number: 7 label: LABEL_REPEATED type: TYPE_STRING - options { features { string_field_validation: HINT } } + options { + features { + [pb.cpp] { utf8_validation: VERIFY_DLOG } + } + } } field { name: "utf8_none_field" number: 8 label: LABEL_REPEATED type: TYPE_STRING - options { features { string_field_validation: NONE } } + options { + features { + [pb.cpp] { utf8_validation: NONE } + } + } } } )pb"); @@ -9243,6 +9210,35 @@ TEST_F(FeaturesTest, InvalidFieldRepeatedImplicit) { "implicit field presence.\n"); } +TEST_F(FeaturesTest, InvalidFieldMapImplicit) { + constexpr absl::string_view kProtoFile = R"schema( + edition = "2023"; + + message Foo { + map bar = 1 [ + features.field_presence = IMPLICIT + ]; + } + )schema"; + io::ArrayInputStream input_stream(kProtoFile.data(), kProtoFile.size()); + SimpleErrorCollector error_collector; + io::Tokenizer tokenizer(&input_stream, &error_collector); + compiler::Parser parser; + parser.RecordErrorsTo(&error_collector); + FileDescriptorProto proto; + ASSERT_TRUE(parser.Parse(&tokenizer, &proto)) + << error_collector.last_error() << "\n" + << kProtoFile; + ASSERT_EQ("", error_collector.last_error()); + proto.set_name("foo.proto"); + + BuildDescriptorMessagesInTestPool(); + BuildFileWithErrors( + proto.DebugString(), + "foo.proto: Foo.bar: NAME: Only singular scalar fields can specify " + "implicit field presence.\n"); +} + TEST_F(FeaturesTest, InvalidFieldOneofImplicit) { BuildDescriptorMessagesInTestPool(); BuildFileWithErrors( @@ -9313,95 +9309,6 @@ TEST_F(FeaturesTest, InvalidFieldOneofRequired) { "required field presence.\n"); } -TEST_F(FeaturesTest, InvalidFieldNonStringWithStringValidation) { - BuildDescriptorMessagesInTestPool(); - BuildFileWithErrors( - R"pb( - name: "foo.proto" - syntax: "editions" - edition: "2023" - message_type { - name: "Foo" - field { - name: "bar" - number: 1 - label: LABEL_OPTIONAL - type: TYPE_INT64 - options { features { string_field_validation: MANDATORY } } - } - } - )pb", - "foo.proto: Foo.bar: NAME: Only string fields can specify " - "`string_field_validation`.\n"); -} - -TEST_F(FeaturesTest, InvalidFieldNonStringMapWithStringValidation) { - BuildDescriptorMessagesInTestPool(); - BuildFileWithErrors( - R"pb( - name: "foo.proto" - syntax: "editions" - edition: "2023" - message_type { - name: "Foo" - nested_type { - name: "MapFieldEntry" - field { - name: "key" - number: 1 - label: LABEL_OPTIONAL - type: TYPE_INT32 - options { - uninterpreted_option { - name { name_part: "features" is_extension: false } - name { - name_part: "string_field_validation" - is_extension: false - } - identifier_value: "HINT" - } - } - } - field { - name: "value" - number: 2 - label: LABEL_OPTIONAL - type: TYPE_INT32 - options { - uninterpreted_option { - name { name_part: "features" is_extension: false } - name { - name_part: "string_field_validation" - is_extension: false - } - identifier_value: "HINT" - } - } - } - options { map_entry: true } - } - field { - name: "map_field" - number: 1 - label: LABEL_REPEATED - type_name: "MapFieldEntry" - options { - uninterpreted_option { - name { name_part: "features" is_extension: false } - name { - name_part: "string_field_validation" - is_extension: false - } - identifier_value: "HINT" - } - } - } - } - )pb", - "foo.proto: Foo.map_field: NAME: Only string fields can specify " - "`string_field_validation`.\n"); -} - TEST_F(FeaturesTest, InvalidFieldNonRepeatedWithRepeatedEncoding) { BuildDescriptorMessagesInTestPool(); BuildFileWithErrors( @@ -9519,7 +9426,6 @@ TEST_F(FeaturesTest, UninterpretedOptions) { field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -9647,7 +9553,6 @@ TEST_F(FeaturesTest, RawFeatures) { field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { @@ -9674,7 +9579,6 @@ TEST_F(FeaturesTest, RawFeaturesConflict) { field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { diff --git a/src/google/protobuf/editions/BUILD b/src/google/protobuf/editions/BUILD index 8b31ffda8197a..7add8dcbb2c02 100644 --- a/src/google/protobuf/editions/BUILD +++ b/src/google/protobuf/editions/BUILD @@ -2,6 +2,7 @@ load("@rules_cc//cc:defs.bzl", "cc_proto_library") proto_library( name = "test_messages_proto2_proto", + testonly = True, srcs = ["golden/test_messages_proto2.proto"], strip_import_prefix = "/src", deps = ["//src/google/protobuf:cpp_features_proto"], @@ -9,11 +10,13 @@ proto_library( cc_proto_library( name = "test_messages_proto2_cc_proto", + testonly = True, deps = [":test_messages_proto2_proto"], ) proto_library( name = "test_messages_proto3_proto", + testonly = True, srcs = ["golden/test_messages_proto3.proto"], strip_import_prefix = "/src", deps = [ @@ -29,25 +32,28 @@ proto_library( cc_proto_library( name = "test_messages_proto3_cc_proto", + testonly = True, deps = [":test_messages_proto3_proto"], ) proto_library( - name = "editions_default_features_proto", - srcs = ["proto/editions_default_features.proto"], + name = "test_editions_default_features_proto", + testonly = True, + srcs = ["proto/test_editions_default_features.proto"], strip_import_prefix = "/src", ) cc_proto_library( - name = "editions_default_features_cc_proto", - deps = [":editions_default_features_proto"], + name = "test_editions_default_features_cc_proto", + testonly = True, + deps = [":test_editions_default_features_proto"], ) cc_test( name = "generated_files_test", srcs = ["generated_files_test.cc"], deps = [ - ":editions_default_features_cc_proto", + ":test_editions_default_features_cc_proto", ":test_messages_proto2_cc_proto", ":test_messages_proto3_cc_proto", "//src/google/protobuf", diff --git a/src/google/protobuf/editions/generated_files_test.cc b/src/google/protobuf/editions/generated_files_test.cc index 0f9ab590612e9..ba75cb3c1c2d8 100644 --- a/src/google/protobuf/editions/generated_files_test.cc +++ b/src/google/protobuf/editions/generated_files_test.cc @@ -33,7 +33,7 @@ #include "google/protobuf/descriptor.h" #include "google/protobuf/editions/golden/test_messages_proto2.pb.h" #include "google/protobuf/editions/golden/test_messages_proto3.pb.h" -#include "google/protobuf/editions/proto/editions_default_features.pb.h" +#include "google/protobuf/editions/proto/test_editions_default_features.pb.h" #include "google/protobuf/test_textproto.h" // These tests provide some basic minimal coverage that protos work as expected. @@ -166,7 +166,6 @@ TEST(Generated, EditionDefaults2023InternalFeatures) { field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED - string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED json_format: ALLOW [pb.cpp] { legacy_closed_enum: false utf8_validation: VERIFY_PARSE } diff --git a/src/google/protobuf/editions/golden/compare_cpp_codegen_failure.txt b/src/google/protobuf/editions/golden/compare_cpp_codegen_failure.txt index fa33da63cd842..e1b1075029eaa 100644 --- a/src/google/protobuf/editions/golden/compare_cpp_codegen_failure.txt +++ b/src/google/protobuf/editions/golden/compare_cpp_codegen_failure.txt @@ -35,6 +35,14 @@ total_size += ::_pbi::WireFormatLite::Int32SizePlusOne( [ FAILED ] third_party/protobuf/editions/golden/simple_proto3.pb.cc [ RUN ] third_party/protobuf/editions/golden/simple_proto3.pb.h +@@ @@ + #include "third_party/protobuf/generated_message_util.h" + #include "third_party/protobuf/metadata_lite.h" + #include "third_party/protobuf/message_lite.h" ++#include "third_party/protobuf/cpp_features.pb.h" + // @@protoc_insertion_point(includes) + + // Must be included last. @@ @@ enum : int { kInt32FieldFieldNumber = 1, diff --git a/src/google/protobuf/editions/golden/compare_cpp_codegen_failure.xml b/src/google/protobuf/editions/golden/compare_cpp_codegen_failure.xml index c9668360bb337..384b33ff3cc4b 100644 --- a/src/google/protobuf/editions/golden/compare_cpp_codegen_failure.xml +++ b/src/google/protobuf/editions/golden/compare_cpp_codegen_failure.xml @@ -5,7 +5,7 @@ - + diff --git a/src/google/protobuf/editions/golden/editions_transform_proto2.proto b/src/google/protobuf/editions/golden/editions_transform_proto2.proto index 1320a2be912a1..14fe46623c16d 100644 --- a/src/google/protobuf/editions/golden/editions_transform_proto2.proto +++ b/src/google/protobuf/editions/golden/editions_transform_proto2.proto @@ -32,9 +32,9 @@ edition = "2023"; import "google/protobuf/cpp_features.proto"; option features.enum_type = CLOSED; option features.repeated_field_encoding = EXPANDED; -option features.string_field_validation = HINT; option features.json_format = LEGACY_BEST_EFFORT; option features.(pb.cpp).legacy_closed_enum = true; +option features.(pb.cpp).utf8_validation = VERIFY_DLOG; // This file contains various edge cases we've collected from migrating real // protos in order to lock down the transformations. diff --git a/src/google/protobuf/editions/golden/editions_transform_proto2_lite.proto b/src/google/protobuf/editions/golden/editions_transform_proto2_lite.proto index 404cdde9149bb..37df5c14b9fc7 100644 --- a/src/google/protobuf/editions/golden/editions_transform_proto2_lite.proto +++ b/src/google/protobuf/editions/golden/editions_transform_proto2_lite.proto @@ -32,9 +32,9 @@ edition = "2023"; import "google/protobuf/cpp_features.proto"; option features.enum_type = CLOSED; option features.repeated_field_encoding = EXPANDED; -option features.string_field_validation = HINT; option features.json_format = LEGACY_BEST_EFFORT; option features.(pb.cpp).legacy_closed_enum = true; +option features.(pb.cpp).utf8_validation = VERIFY_DLOG; package protobuf_editions_test; diff --git a/src/google/protobuf/editions/golden/editions_transform_proto2_utf8_disabled.proto b/src/google/protobuf/editions/golden/editions_transform_proto2_utf8_disabled.proto index e58ca31a0dcca..3aa94a6968e4c 100644 --- a/src/google/protobuf/editions/golden/editions_transform_proto2_utf8_disabled.proto +++ b/src/google/protobuf/editions/golden/editions_transform_proto2_utf8_disabled.proto @@ -32,19 +32,19 @@ edition = "2023"; import "google/protobuf/cpp_features.proto"; option features.enum_type = CLOSED; option features.repeated_field_encoding = EXPANDED; -option features.string_field_validation = HINT; option features.json_format = LEGACY_BEST_EFFORT; option features.(pb.cpp).legacy_closed_enum = true; +option features.(pb.cpp).utf8_validation = VERIFY_DLOG; package protobuf_editions_test; message TestMessageUtf8Disabled { - string string_field = 1 [features.string_field_validation = NONE]; - string string_field_utf = 2 [features.string_field_validation = NONE]; - string string_field_noutf = 3 [features.string_field_validation = NONE]; + string string_field = 1 [features.(pb.cpp).utf8_validation = NONE]; + string string_field_utf = 2 [features.(pb.cpp).utf8_validation = NONE]; + string string_field_noutf = 3 [features.(pb.cpp).utf8_validation = NONE]; - map string_map_field = 4 [features.string_field_validation = NONE]; + map string_map_field = 4 [features.(pb.cpp).utf8_validation = NONE]; int32 int_field = 5; } diff --git a/src/google/protobuf/editions/golden/editions_transform_proto3.proto b/src/google/protobuf/editions/golden/editions_transform_proto3.proto index 2310bd796a047..636668a9c5a89 100644 --- a/src/google/protobuf/editions/golden/editions_transform_proto3.proto +++ b/src/google/protobuf/editions/golden/editions_transform_proto3.proto @@ -29,18 +29,20 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. edition = "2023"; +import "google/protobuf/cpp_features.proto"; option features.field_presence = IMPLICIT; +option features.(pb.cpp).utf8_validation = VERIFY_PARSE; package protobuf_editions_test; message TestMessageProto3 { string string_field = 1; string string_field_utf = 2; - string string_field_noutf = 3 [features.string_field_validation = HINT]; + string string_field_noutf = 3 [features.(pb.cpp).utf8_validation = VERIFY_DLOG]; map string_map_field = 4; map string_map_field_utf = 5; - map string_map_field_noutf = 6 [features.string_field_validation = HINT]; + map string_map_field_noutf = 6 [features.(pb.cpp).utf8_validation = VERIFY_DLOG]; repeated int32 int_field = 7; repeated int32 int_field_packed = 8; diff --git a/src/google/protobuf/editions/golden/editions_transform_proto3_utf8_disabled.proto b/src/google/protobuf/editions/golden/editions_transform_proto3_utf8_disabled.proto index 2ef6447bb87f9..35ce495e675ac 100644 --- a/src/google/protobuf/editions/golden/editions_transform_proto3_utf8_disabled.proto +++ b/src/google/protobuf/editions/golden/editions_transform_proto3_utf8_disabled.proto @@ -29,17 +29,19 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. edition = "2023"; +import "google/protobuf/cpp_features.proto"; option features.field_presence = IMPLICIT; +option features.(pb.cpp).utf8_validation = VERIFY_PARSE; package protobuf_editions_test; message TestMessageProto3 { - string string_field = 1 [features.string_field_validation = NONE]; - string string_field_utf = 2 [features.string_field_validation = NONE]; - string string_field_noutf = 3 [features.string_field_validation = NONE]; + string string_field = 1 [features.(pb.cpp).utf8_validation = NONE]; + string string_field_utf = 2 [features.(pb.cpp).utf8_validation = NONE]; + string string_field_noutf = 3 [features.(pb.cpp).utf8_validation = NONE]; - map string_map_field = 4 [features.string_field_validation = NONE]; + map string_map_field = 4 [features.(pb.cpp).utf8_validation = NONE]; repeated int32 int_field = 7; } diff --git a/src/google/protobuf/editions/golden/test_messages_proto2.proto b/src/google/protobuf/editions/golden/test_messages_proto2.proto index f5f59dacf612f..77506e2ac1566 100644 --- a/src/google/protobuf/editions/golden/test_messages_proto2.proto +++ b/src/google/protobuf/editions/golden/test_messages_proto2.proto @@ -39,9 +39,9 @@ edition = "2023"; import "google/protobuf/cpp_features.proto"; option features.enum_type = CLOSED; option features.repeated_field_encoding = EXPANDED; -option features.string_field_validation = HINT; option features.json_format = LEGACY_BEST_EFFORT; option features.(pb.cpp).legacy_closed_enum = true; +option features.(pb.cpp).utf8_validation = VERIFY_DLOG; package protobuf_test_messages.proto2; diff --git a/src/google/protobuf/editions/golden/test_messages_proto3.proto b/src/google/protobuf/editions/golden/test_messages_proto3.proto index ce24bf90b629a..b6aab02b997a0 100644 --- a/src/google/protobuf/editions/golden/test_messages_proto3.proto +++ b/src/google/protobuf/editions/golden/test_messages_proto3.proto @@ -36,7 +36,9 @@ // edition = "2023"; +import "google/protobuf/cpp_features.proto"; option features.field_presence = IMPLICIT; +option features.(pb.cpp).utf8_validation = VERIFY_PARSE; package protobuf_test_messages.proto3; diff --git a/src/google/protobuf/editions/proto/editions_default_features.proto b/src/google/protobuf/editions/proto/test_editions_default_features.proto similarity index 100% rename from src/google/protobuf/editions/proto/editions_default_features.proto rename to src/google/protobuf/editions/proto/test_editions_default_features.proto diff --git a/src/google/protobuf/editions/transform.awk b/src/google/protobuf/editions/transform.awk index 23ead050ee6df..1126f224c57d4 100644 --- a/src/google/protobuf/editions/transform.awk +++ b/src/google/protobuf/editions/transform.awk @@ -78,15 +78,15 @@ function transform_field(field) options[++num_options] = "features.field_presence = LEGACY_REQUIRED" } if (disable_utf8 && match(field_def, /^\s*(string|repeated\s*string|map)/)) { - options[++num_options] = "features.string_field_validation = NONE" + options[++num_options] = "features.(pb.cpp).utf8_validation = NONE" } } if(syntax == 3) { if (disable_utf8 && match(field_def, /^\s*(string|repeated\s*string|map)/)) { - options[++num_options] = "features.string_field_validation = NONE" + options[++num_options] = "features.(pb.cpp).utf8_validation = NONE" } else { - sub(/\/, "features.string_field_validation = HINT", existing_options) + sub(/\/, "features.(pb.cpp).utf8_validation = VERIFY_DLOG", existing_options) } sub(/\/, "features.repeated_field_encoding = EXPANDED", existing_options) existing_options = strip_option("packed = true", existing_options) @@ -121,16 +121,18 @@ function transform_field(field) print "import \"third_party/protobuf/cpp_features.proto\";" print "option features.enum_type = CLOSED;" print "option features.repeated_field_encoding = EXPANDED;" - print "option features.string_field_validation = HINT;" print "option features.json_format = LEGACY_BEST_EFFORT;" print "option features.(pb.cpp).legacy_closed_enum = true;" + print "option features.(pb.cpp).utf8_validation = VERIFY_DLOG;" syntax = 2 next } /syntax = "proto3"/ { print "edition = \"2023\";" + print "import \"third_party/protobuf/cpp_features.proto\";" print "option features.field_presence = IMPLICIT;" + print "option features.(pb.cpp).utf8_validation = VERIFY_PARSE;" syntax = 3 next } diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc index 1387d7e9503e6..3938ab568fc4e 100644 --- a/src/google/protobuf/feature_resolver.cc +++ b/src/google/protobuf/feature_resolver.cc @@ -74,7 +74,8 @@ absl::Status Error(Args... args) { bool IsNonFeatureField(const FieldDescriptor& field) { return field.containing_type() && field.containing_type()->full_name() == "google.protobuf.FeatureSet" && - field.name() == "raw_features"; + (field.name() == "raw_features" || + field.name() == "string_field_validation"); } bool EditionsLessThan(absl::string_view a, absl::string_view b) { @@ -197,6 +198,7 @@ absl::Status ValidateMergedFeatures(const Message& msg) { const Reflection& reflection = *msg.GetReflection(); for (int i = 0; i < descriptor.field_count(); ++i) { const FieldDescriptor& field = *descriptor.field(i); + if (IsNonFeatureField(field)) continue; // Validate enum features. if (field.enum_type() != nullptr) { ABSL_DCHECK(reflection.HasField(msg, &field)); diff --git a/src/google/protobuf/feature_resolver_test.cc b/src/google/protobuf/feature_resolver_test.cc index 5c5ef41c8ffab..d42c491a4e9a4 100644 --- a/src/google/protobuf/feature_resolver_test.cc +++ b/src/google/protobuf/feature_resolver_test.cc @@ -144,7 +144,6 @@ TEST(FeatureResolverTest, DefaultsCore2023) { EXPECT_EQ(merged->field_presence(), FeatureSet::EXPLICIT); EXPECT_EQ(merged->enum_type(), FeatureSet::OPEN); EXPECT_EQ(merged->repeated_field_encoding(), FeatureSet::PACKED); - EXPECT_EQ(merged->string_field_validation(), FeatureSet::MANDATORY); EXPECT_EQ(merged->message_encoding(), FeatureSet::LENGTH_PREFIXED); EXPECT_FALSE(merged->HasExtension(pb::test)); } @@ -156,7 +155,6 @@ TEST(FeatureResolverTest, DefaultsTest2023) { EXPECT_EQ(merged->field_presence(), FeatureSet::EXPLICIT); EXPECT_EQ(merged->enum_type(), FeatureSet::OPEN); EXPECT_EQ(merged->repeated_field_encoding(), FeatureSet::PACKED); - EXPECT_EQ(merged->string_field_validation(), FeatureSet::MANDATORY); EXPECT_EQ(merged->message_encoding(), FeatureSet::LENGTH_PREFIXED); const pb::TestFeatures& ext = merged->GetExtension(pb::test); @@ -185,7 +183,6 @@ TEST(FeatureResolverTest, DefaultsTestMessageExtension) { EXPECT_EQ(merged->field_presence(), FeatureSet::EXPLICIT); EXPECT_EQ(merged->enum_type(), FeatureSet::OPEN); EXPECT_EQ(merged->repeated_field_encoding(), FeatureSet::PACKED); - EXPECT_EQ(merged->string_field_validation(), FeatureSet::MANDATORY); EXPECT_EQ(merged->message_encoding(), FeatureSet::LENGTH_PREFIXED); const pb::TestFeatures& ext = merged->GetExtension(pb::test); @@ -214,7 +211,6 @@ TEST(FeatureResolverTest, DefaultsTestNestedExtension) { EXPECT_EQ(merged->field_presence(), FeatureSet::EXPLICIT); EXPECT_EQ(merged->enum_type(), FeatureSet::OPEN); EXPECT_EQ(merged->repeated_field_encoding(), FeatureSet::PACKED); - EXPECT_EQ(merged->string_field_validation(), FeatureSet::MANDATORY); EXPECT_EQ(merged->message_encoding(), FeatureSet::LENGTH_PREFIXED); const pb::TestFeatures& ext = merged->GetExtension(pb::test); @@ -370,7 +366,6 @@ TEST(FeatureResolverTest, MergeFeaturesChildOverrideCore) { EXPECT_EQ(merged->field_presence(), FeatureSet::IMPLICIT); EXPECT_EQ(merged->enum_type(), FeatureSet::OPEN); EXPECT_EQ(merged->repeated_field_encoding(), FeatureSet::EXPANDED); - EXPECT_EQ(merged->string_field_validation(), FeatureSet::MANDATORY); EXPECT_EQ(merged->message_encoding(), FeatureSet::LENGTH_PREFIXED); } @@ -394,7 +389,6 @@ TEST(FeatureResolverTest, MergeFeaturesChildOverrideComplex) { EXPECT_EQ(merged->field_presence(), FeatureSet::IMPLICIT); EXPECT_EQ(merged->enum_type(), FeatureSet::OPEN); EXPECT_EQ(merged->repeated_field_encoding(), FeatureSet::EXPANDED); - EXPECT_EQ(merged->string_field_validation(), FeatureSet::MANDATORY); EXPECT_EQ(merged->message_encoding(), FeatureSet::LENGTH_PREFIXED); pb::TestFeatures ext = merged->GetExtension(pb::test); @@ -421,7 +415,6 @@ TEST(FeatureResolverTest, MergeFeaturesParentOverrides) { } )pb"); FeatureSet child = ParseTextOrDie(R"pb( - string_field_validation: NONE repeated_field_encoding: PACKED [pb.test] { int_field_feature: 9 @@ -434,7 +427,6 @@ TEST(FeatureResolverTest, MergeFeaturesParentOverrides) { EXPECT_EQ(merged->field_presence(), FeatureSet::IMPLICIT); EXPECT_EQ(merged->enum_type(), FeatureSet::OPEN); EXPECT_EQ(merged->repeated_field_encoding(), FeatureSet::PACKED); - EXPECT_EQ(merged->string_field_validation(), FeatureSet::NONE); EXPECT_EQ(merged->message_encoding(), FeatureSet::LENGTH_PREFIXED); pb::TestFeatures ext = merged->GetExtension(pb::test); @@ -490,18 +482,6 @@ TEST(FeatureResolverTest, MergeFeaturesRepeatedFieldEncodingUnknown) { HasSubstr("REPEATED_FIELD_ENCODING_UNKNOWN")))); } -TEST(FeatureResolverTest, MergeFeaturesStringFieldValidationUnknown) { - absl::StatusOr resolver = SetupFeatureResolver("2023"); - ASSERT_OK(resolver); - FeatureSet child = ParseTextOrDie(R"pb( - string_field_validation: STRING_FIELD_VALIDATION_UNKNOWN - )pb"); - EXPECT_THAT(resolver->MergeFeatures(FeatureSet(), child), - HasError(AllOf( - HasSubstr("field google.protobuf.FeatureSet.string_field_validation"), - HasSubstr("STRING_FIELD_VALIDATION_UNKNOWN")))); -} - TEST(FeatureResolverTest, MergeFeaturesMessageEncodingUnknown) { absl::StatusOr resolver = SetupFeatureResolver("2023"); ASSERT_OK(resolver);