From e0a1da0868c942d71ced980655b09cdbaaa9ead2 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Thu, 17 Nov 2022 13:24:25 -0800 Subject: [PATCH] Correctly discard unknown enum name strings in an array when parsing a repeated field. This was a regression in the new parser. PiperOrigin-RevId: 489292231 --- conformance/failure_list_cpp.txt | 1 - .../json/internal/descriptor_traits.h | 18 ++++++- src/google/protobuf/json/internal/parser.cc | 2 +- src/google/protobuf/json/json_test.cc | 48 ++++++++++++------- 4 files changed, 49 insertions(+), 20 deletions(-) diff --git a/conformance/failure_list_cpp.txt b/conformance/failure_list_cpp.txt index 5af3dfc300..78a50f0e57 100644 --- a/conformance/failure_list_cpp.txt +++ b/conformance/failure_list_cpp.txt @@ -18,7 +18,6 @@ Recommended.Proto3.JsonInput.FieldNameDuplicateDifferentCasing1 Recommended.Proto3.JsonInput.FieldNameDuplicateDifferentCasing2 Recommended.Proto3.JsonInput.FieldNameNotQuoted Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput -Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput Recommended.Proto3.JsonInput.MapFieldValueIsNull Recommended.Proto3.JsonInput.RepeatedFieldMessageElementIsNull Recommended.Proto3.JsonInput.RepeatedFieldPrimitiveElementIsNull diff --git a/src/google/protobuf/json/internal/descriptor_traits.h b/src/google/protobuf/json/internal/descriptor_traits.h index 2d08f3a927..fb309e9eb4 100644 --- a/src/google/protobuf/json/internal/descriptor_traits.h +++ b/src/google/protobuf/json/internal/descriptor_traits.h @@ -267,6 +267,10 @@ struct Proto2Descriptor { static bool IsOptional(Field f) { return f->has_presence(); } + static bool IsImplicitPresence(Field f) { + return !f->is_repeated() && !f->has_presence(); + } + static bool IsExtension(Field f) { return f->is_extension(); } static bool IsOneof(Field f) { return f->containing_oneof() != nullptr; } @@ -449,8 +453,20 @@ struct Proto3Type { } static bool IsOptional(Field f) { + // Implicit presence requires this weird check: in proto3, everything is + // implicit presence, except for things that are members of oneofs, + // which is how proto3 optional is represented. + if (f->parent().proto().syntax() == google::protobuf::SYNTAX_PROTO3) { + return f->proto().oneof_index() != 0; + } + return f->proto().cardinality() == - google::protobuf::Field::CARDINALITY_OPTIONAL; + google::protobuf::Field::CARDINALITY_OPTIONAL || + google::protobuf::Field::CARDINALITY_REQUIRED; + } + + static bool IsImplicitPresence(Field f) { + return !IsRepeated(f) && !IsOptional(f); } static bool IsExtension(Field f) { return false; } diff --git a/src/google/protobuf/json/internal/parser.cc b/src/google/protobuf/json/internal/parser.cc index a7706f682b..7b55b70746 100644 --- a/src/google/protobuf/json/internal/parser.cc +++ b/src/google/protobuf/json/internal/parser.cc @@ -525,7 +525,7 @@ absl::Status ParseSingular(JsonLexer& lex, Field field, absl::StatusOr> x = ParseEnum(lex, field); RETURN_IF_ERROR(x.status()); - if (x->has_value() || !Traits::IsOptional(field)) { + if (x->has_value() || Traits::IsImplicitPresence(field)) { Traits::SetEnum(field, msg, x->value_or(0)); } break; diff --git a/src/google/protobuf/json/json_test.cc b/src/google/protobuf/json/json_test.cc index e94e6f3df6..3e0ad836d7 100644 --- a/src/google/protobuf/json/json_test.cc +++ b/src/google/protobuf/json/json_test.cc @@ -273,20 +273,20 @@ TEST_P(JsonTest, TestDefaultValues) { R"("defaultForeignEnum":"FOREIGN_BAR","defaultImportEnum":"IMPORT_BAR",)" R"("defaultStringPiece":"abc","defaultCord":"123"})")); - EXPECT_THAT( - ToJson(protobuf_unittest::TestExtremeDefaultValues(), options), - IsOkAndHolds( - R"({"escapedBytes":"XDAwMFwwMDFcMDA3XDAxMFwwMTRcblxyXHRcMDEzXFxcJ1wiXDM3Ng==")" - R"(,"largeUint32":4294967295,"largeUint64":"18446744073709551615",)" - R"("smallInt32":-2147483647,"smallInt64":"-9223372036854775807",)" - R"("utf8String":"ሴ","zeroFloat":0,"oneFloat":1,"smallFloat":1.5,)" - R"("negativeOneFloat":-1,"negativeFloat":-1.5,"largeFloat":2e+08,)" - R"("smallNegativeFloat":-8e-28,"infDouble":0,"negInfDouble":0,)" - R"("nanDouble":0,"infFloat":0,"negInfFloat":0,"nanFloat":0,)" - R"("cppTrigraph":"? ? ?? ?? ??? ??/ ??-","reallySmallInt32":-2147483648)" - R"(,"reallySmallInt64":"-9223372036854775808","stringWithZero":"hel\u0000lo")" - R"(,"bytesWithZero":"d29yXDAwMGxk","stringPieceWithZero":"ab\u0000c")" - R"(,"cordWithZero":"12\u00003","replacementString":"${unknown}"})")); + EXPECT_THAT( + ToJson(protobuf_unittest::TestExtremeDefaultValues(), options), + IsOkAndHolds( + R"({"escapedBytes":"XDAwMFwwMDFcMDA3XDAxMFwwMTRcblxyXHRcMDEzXFxcJ1wiXDM3Ng==")" + R"(,"largeUint32":4294967295,"largeUint64":"18446744073709551615",)" + R"("smallInt32":-2147483647,"smallInt64":"-9223372036854775807",)" + R"("utf8String":"ሴ","zeroFloat":0,"oneFloat":1,"smallFloat":1.5,)" + R"("negativeOneFloat":-1,"negativeFloat":-1.5,"largeFloat":2e+08,)" + R"("smallNegativeFloat":-8e-28,"infDouble":0,"negInfDouble":0,)" + R"("nanDouble":0,"infFloat":0,"negInfFloat":0,"nanFloat":0,)" + R"("cppTrigraph":"? ? ?? ?? ??? ??/ ??-","reallySmallInt32":-2147483648)" + R"(,"reallySmallInt64":"-9223372036854775808","stringWithZero":"hel\u0000lo")" + R"(,"bytesWithZero":"d29yXDAwMGxk","stringPieceWithZero":"ab\u0000c")" + R"(,"cordWithZero":"12\u00003","replacementString":"${unknown}"})")); } TEST_P(JsonTest, TestPreserveProtoFieldNames) { @@ -972,6 +972,21 @@ TEST_P(JsonTest, TestParsingUnknownEnumsProto3FromArray) { StatusIs(absl::StatusCode::kInvalidArgument)); } +TEST_P(JsonTest, TestParsingRepeatedUnknownEnums) { + absl::string_view input = R"json({ + "repeated_enum_value": ["FOO", "BAZ", "BAR"] + })json"; + + EXPECT_THAT(ToProto(input), + StatusIs(absl::StatusCode::kInvalidArgument)); + + ParseOptions options; + options.ignore_unknown_fields = true; + auto m = ToProto(input, options); + ASSERT_OK(m); + EXPECT_THAT(m->repeated_enum_value(), ElementsAre(proto3::FOO, proto3::BAR)); +} + TEST_P(JsonTest, TestParsingEnumCaseSensitive) { TestMessage m; m.set_enum_value(proto3::FOO); @@ -1278,9 +1293,8 @@ TEST_P(JsonTest, FieldOrder) { resolver_.get(), "type.googleapis.com/proto3.TestMessage", "\x18\x03\xb0\x01\x02\x08\x01\xb0\x01\x02", &out); ASSERT_OK(s); - EXPECT_EQ( - out, - R"({"boolValue":true,"int64Value":"3","repeatedInt32Value":[2,2]})"); + EXPECT_EQ( + out, R"({"boolValue":true,"int64Value":"3","repeatedInt32Value":[2,2]})"); } // JSON values get special treatment when it comes to pre-existing values in