diff --git a/conformance/failure_list_cpp.txt b/conformance/failure_list_cpp.txt index 7c52e23d96c8d..99fd552e9d7b5 100644 --- a/conformance/failure_list_cpp.txt +++ b/conformance/failure_list_cpp.txt @@ -14,8 +14,6 @@ Recommended.*.JsonInput.FieldNameDuplicateDifferentCasing1 Recommended.*.JsonInput.FieldNameDuplicateDifferentCasing2 # Should have failed to parse, but didn't. Recommended.*.JsonInput.FieldNameExtension.Validator # Expected JSON payload but got type 1 Recommended.*.JsonInput.FieldNameNotQuoted # Should have failed to parse, but didn't. -Recommended.*.JsonInput.IgnoreUnknownEnumStringValueInMapPart.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key2]: FOO -Recommended.*.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key]: FOO Recommended.*.JsonInput.MapFieldValueIsNull # Should have failed to parse, but didn't. Recommended.*.JsonInput.RepeatedFieldMessageElementIsNull # Should have failed to parse, but didn't. Recommended.*.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 1db28924d0eb5..98232252a7396 100644 --- a/src/google/protobuf/json/internal/parser.cc +++ b/src/google/protobuf/json/internal/parser.cc @@ -704,12 +704,63 @@ absl::Status ParseArray(JsonLexer& lex, Field field, Msg& msg) { }); } +// Parses one map entry for 'map_field' of type map<*, enum> in 'parent_msg' +// with already consumed 'key'. +// The implementation is careful not to invoke Traits::NewMsg (which emits a new +// message) if the enum value is unknown but we can recover from it. +// This is tested by ParseMapWithEnumValuesProto{2,3}WithUnknownFields tests in +// kReflective codec mode. +template +absl::Status ParseMapOfEnumsEntry(JsonLexer& lex, Field map_field, + Msg& parent_msg, + LocationWith& key) { + // Parse the enum value from string, advancing the lexer. + absl::optional enum_value; + RETURN_IF_ERROR(Traits::WithFieldType( + map_field, [&lex, &enum_value](const Desc& map_entry_desc) { + ASSIGN_OR_RETURN( + enum_value, + ParseEnum(lex, Traits::ValueField(map_entry_desc))); + return absl::OkStatus(); + })); + + if (enum_value.has_value()) { + return Traits::NewMsg( + map_field, parent_msg, + [&](const Desc& type, Msg& entry) -> absl::Status { + RETURN_IF_ERROR(ParseMapKey(type, entry, key)); + Traits::SetEnum(Traits::ValueField(type), entry, *enum_value); + return absl::OkStatus(); + }); + } else { + // If we don't have enum value here, it means that it was OK to ignore it + // due to ignore_unknown_fields flag, otherwise ParseEnum call would fail + // above with "unknown enum value: " invalid argument error. + ABSL_DCHECK(lex.options().ignore_unknown_fields); + return absl::OkStatus(); + } +} + // 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) { + bool is_map_of_enums = false; + RETURN_IF_ERROR(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(); + })); + + // Special case for map<*, enum> due to handling of unknown enum values. + // See comments above ParseMapOfEnumsEntry for details. + if (is_map_of_enums) { + return ParseMapOfEnumsEntry(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 b13fe597aff28..2104496dd9646 100644 --- a/src/google/protobuf/json/json_test.cc +++ b/src/google/protobuf/json/json_test.cc @@ -585,14 +585,9 @@ TEST_P(JsonTest, ParseMapWithEnumValuesProto2WithUnknownFields) { 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 will be fixed in a follow-up. - 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) { @@ -611,14 +606,9 @@ TEST_P(JsonTest, ParseMapWithEnumValuesProto3WithUnknownFields) { 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 will be fixed in a follow-up. - 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, ParseMap) { diff --git a/src/google/protobuf/stubs/status_macros.h b/src/google/protobuf/stubs/status_macros.h index 5995e3894dca5..93d6b8b005b31 100644 --- a/src/google/protobuf/stubs/status_macros.h +++ b/src/google/protobuf/stubs/status_macros.h @@ -45,8 +45,9 @@ absl::Status DoAssignOrReturn(T& lhs, absl::StatusOr result) { return result.status(); } -#define ASSIGN_OR_RETURN_IMPL(status, lhs, rexpr) \ - absl::Status status = DoAssignOrReturn(lhs, (rexpr)); \ +#define ASSIGN_OR_RETURN_IMPL(status, lhs, rexpr) \ + absl::Status status = \ + ::google::protobuf::util::DoAssignOrReturn(lhs, (rexpr)); \ if (ABSL_PREDICT_FALSE(!status.ok())) return status; // Executes an expression that returns a util::StatusOr, extracting its value