diff --git a/conformance/failure_list_cpp.txt b/conformance/failure_list_cpp.txt index d7ffba72740a..f17e03bfa14b 100644 --- a/conformance/failure_list_cpp.txt +++ b/conformance/failure_list_cpp.txt @@ -14,8 +14,6 @@ Recommended.Editions_Proto2.JsonInput.FieldNameDuplicateDifferentCasing1 Recommended.Editions_Proto2.JsonInput.FieldNameDuplicateDifferentCasing2 # Should have failed to parse, but didn't. Recommended.Editions_Proto2.JsonInput.FieldNameExtension.Validator # Expected JSON payload but got type 1 Recommended.Editions_Proto2.JsonInput.FieldNameNotQuoted # Should have failed to parse, but didn't. -Recommended.Editions_Proto2.JsonInput.IgnoreUnknownEnumStringValueInMapPart.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key2]: FOO -Recommended.Editions_Proto2.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key]: FOO Recommended.Editions_Proto2.JsonInput.MapFieldValueIsNull # Should have failed to parse, but didn't. Recommended.Editions_Proto2.JsonInput.RepeatedFieldMessageElementIsNull # Should have failed to parse, but didn't. Recommended.Editions_Proto2.JsonInput.RepeatedFieldPrimitiveElementIsNull # Should have failed to parse, but didn't. @@ -41,8 +39,6 @@ Recommended.Editions_Proto3.JsonInput.FieldNameDuplicate Recommended.Editions_Proto3.JsonInput.FieldNameDuplicateDifferentCasing1 # Should have failed to parse, but didn't. Recommended.Editions_Proto3.JsonInput.FieldNameDuplicateDifferentCasing2 # Should have failed to parse, but didn't. Recommended.Editions_Proto3.JsonInput.FieldNameNotQuoted # Should have failed to parse, but didn't. -Recommended.Editions_Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapPart.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key2]: FOO -Recommended.Editions_Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key]: FOO Recommended.Editions_Proto3.JsonInput.MapFieldValueIsNull # Should have failed to parse, but didn't. Recommended.Editions_Proto3.JsonInput.RepeatedFieldMessageElementIsNull # Should have failed to parse, but didn't. Recommended.Editions_Proto3.JsonInput.RepeatedFieldPrimitiveElementIsNull # Should have failed to parse, but didn't. @@ -65,8 +61,6 @@ Recommended.Proto2.JsonInput.FieldNameDuplicateDifferentCasing1 Recommended.Proto2.JsonInput.FieldNameDuplicateDifferentCasing2 # Should have failed to parse, but didn't. Recommended.Proto2.JsonInput.FieldNameExtension.Validator # Expected JSON payload but got type 1 Recommended.Proto2.JsonInput.FieldNameNotQuoted # Should have failed to parse, but didn't. -Recommended.Proto2.JsonInput.IgnoreUnknownEnumStringValueInMapPart.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key2]: FOO -Recommended.Proto2.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key]: FOO Recommended.Proto2.JsonInput.MapFieldValueIsNull # Should have failed to parse, but didn't. Recommended.Proto2.JsonInput.RepeatedFieldMessageElementIsNull # Should have failed to parse, but didn't. Recommended.Proto2.JsonInput.RepeatedFieldPrimitiveElementIsNull # Should have failed to parse, but didn't. @@ -92,8 +86,6 @@ Recommended.Proto3.JsonInput.FieldNameDuplicate Recommended.Proto3.JsonInput.FieldNameDuplicateDifferentCasing1 # Should have failed to parse, but didn't. Recommended.Proto3.JsonInput.FieldNameDuplicateDifferentCasing2 # Should have failed to parse, but didn't. Recommended.Proto3.JsonInput.FieldNameNotQuoted # Should have failed to parse, but didn't. -Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapPart.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key2]: FOO -Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key]: FOO Recommended.Proto3.JsonInput.MapFieldValueIsNull # Should have failed to parse, but didn't. Recommended.Proto3.JsonInput.RepeatedFieldMessageElementIsNull # Should have failed to parse, but didn't. Recommended.Proto3.JsonInput.RepeatedFieldPrimitiveElementIsNull # Should have failed to parse, but didn't. diff --git a/src/google/protobuf/json/internal/parser.cc b/src/google/protobuf/json/internal/parser.cc index 4bdc83e49314..fb5dd17483f9 100644 --- a/src/google/protobuf/json/internal/parser.cc +++ b/src/google/protobuf/json/internal/parser.cc @@ -691,12 +691,105 @@ absl::Status ParseMapKey(const Desc& type, Msg& entry, return absl::OkStatus(); } +// Parses map entry when value is an enum name (string) and ignoring unknown +// fields flag is set. +// Specification says that in this case, if the enum name is unknown, we should +// skip the map entry. +// For example {"key1": "UNKNOWN_ENUM_VALUE"} should parse into an empty map if +// the ignore_unknown_fields is set. +// +// The default implementation in ParseMapEntry will first allocate a new map +// entry message (by calling Traits::NewMsg) and then consume the value from +// the lexer. +// +// In this special case, we must do it the other way around: +// - first consume the value from the lexer, +// - check that this is a known enum value, +// - and only then proceed with allocating a new map entry message. +// +// Note that in both cases the key is already consumed and passed in through +// 'LocationWith& key' param. +template +absl::Status ParseMapEntryWithEnumStringValueWhenIgnoringUnknownFields( + JsonLexer& lex, Field map_field, Msg& parent_msg, + LocationWith& key) { + // Parse the enum value from string, advancing the lexer. + absl::StatusOr> enum_value; + (void)Traits::WithFieldType( + map_field, [&lex, &enum_value](const Desc& map_entry_desc) { + enum_value = ParseEnum(lex, Traits::ValueField(map_entry_desc)); + return absl::OkStatus(); + }); + RETURN_IF_ERROR(enum_value.status()); + + // If enum value is unknown, we'll stop here and avoid allocating a new map + // entry message. + if (!enum_value->has_value()) { + return absl::OkStatus(); + } + + // If the enum value is known, output the map entry with already parsed + // value. + return Traits::NewMsg( + map_field, parent_msg, + [&](const Desc& type, Msg& entry) -> absl::Status { + RETURN_IF_ERROR(ParseMapKey(lex.options(), type, entry, key)); + + // This is different from the default implementation in 'ParseMapEntry' + // since we already parsed this value (we must not attempt to advance + // the lexer to parse the value here again). + Traits::SetEnum(Traits::ValueField(type), entry, + enum_value->value_or(0)); + + return absl::OkStatus(); + }); +} + +// ParseMapEntry implementation has a special case if: +// * 'map_field' is a map of enums, and +// * value that follows in 'lex' is a string (enum name), and +// * and ignore_unknown_fields is set. +// This function does not advance the lexer. +template +absl::StatusOr MapEntryNeedsEnumStringValueSpecialCase( + JsonLexer& lex, Field map_field) { + // Check if the map_field is a map of enums. + bool is_map_of_enums = false; + (void)Traits::WithFieldType( + map_field, [&is_map_of_enums](const Desc& desc) { + is_map_of_enums = Traits::FieldType(Traits::ValueField(desc)) == + FieldDescriptor::TYPE_ENUM; + return absl::OkStatus(); + }); + if (!is_map_of_enums) { + return false; + } + + // Check that the value is a string (instead of a number). + absl::StatusOr kind = lex.PeekKind(); + RETURN_IF_ERROR(kind.status()); + if (*kind != JsonLexer::kStr) { + return false; + } + + // We need the special case only if we are ignoring unknown fields. + return lex.options().ignore_unknown_fields; +} + // Parses one map entry for 'map_field' in 'parent_msg' with already consumed // 'key'. template absl::Status ParseMapEntry(JsonLexer& lex, Field map_field, Msg& parent_msg, LocationWith& key) { + auto needs_special_case = + MapEntryNeedsEnumStringValueSpecialCase(lex, map_field); + RETURN_IF_ERROR(needs_special_case.status()); + if (*needs_special_case) { + return ParseMapEntryWithEnumStringValueWhenIgnoringUnknownFields( + lex, map_field, parent_msg, key); + } + return Traits::NewMsg( map_field, parent_msg, [&](const Desc& type, Msg& entry) -> absl::Status { diff --git a/src/google/protobuf/json/json_test.cc b/src/google/protobuf/json/json_test.cc index e398af6de2be..3b0224e20425 100644 --- a/src/google/protobuf/json/json_test.cc +++ b/src/google/protobuf/json/json_test.cc @@ -562,15 +562,9 @@ TEST_P(JsonTest, ParseMapWithEnumValuesProto2WithUnknownFields) { })json"; ASSERT_OK(ToProto(message, input_json, options)); - // With ignore_unknown_fields set, the unknown enum string value is accepted - // but coerced to 0-th enum value. This behavior fails the conformance test - // 'IgnoreUnknownEnumStringValueInMap' and it will be fixed in - // https://github.com/protocolbuffers/protobuf/pull/16479. - EXPECT_EQ(message.enum_map().size(), 5); - EXPECT_EQ(message.enum_map().contains("key2"), true); - EXPECT_EQ(message.enum_map().contains("key4"), true); - EXPECT_EQ(message.enum_map().at("key2"), 0); - EXPECT_EQ(message.enum_map().at("key4"), 0); + EXPECT_EQ(message.enum_map().size(), 3); + EXPECT_EQ(message.enum_map().contains("key2"), false); + EXPECT_EQ(message.enum_map().contains("key4"), false); } TEST_P(JsonTest, ParseMapWithEnumValuesProto3WithUnknownFields) { @@ -588,15 +582,9 @@ TEST_P(JsonTest, ParseMapWithEnumValuesProto3WithUnknownFields) { })json"; ASSERT_OK(ToProto(message, input_json, options)); - // With ignore_unknown_fields set, the unknown enum string value is accepted - // but coerced to 0-th enum value. This behavior fails the conformance test - // 'IgnoreUnknownEnumStringValueInMap' and it will be fixed in - // https://github.com/protocolbuffers/protobuf/pull/16479. - EXPECT_EQ(message.map().size(), 5); - EXPECT_EQ(message.map().contains("key2"), true); - EXPECT_EQ(message.map().contains("key4"), true); - EXPECT_EQ(message.map().at("key2"), 0); - EXPECT_EQ(message.map().at("key4"), 0); + EXPECT_EQ(message.map().size(), 3); + EXPECT_EQ(message.map().contains("key2"), false); + EXPECT_EQ(message.map().contains("key4"), false); } TEST_P(JsonTest, RepeatedMapKey) {