Fix c++ IgnoreUnknownEnumStringValueInMap conformance tests

PiperOrigin-RevId: 713980229
protocolbuffers · Jan 10, 2025 · 001b674 · 001b674
1 parent c2df2c9
commit 001b674
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 20 deletions.
diff --git 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
@@ -704,12 +704,63 @@ absl::Status ParseArray(JsonLexer& lex, Field<Traits> field, Msg<Traits>& 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 <typename Traits>
+absl::Status ParseMapOfEnumsEntry(JsonLexer& lex, Field<Traits> map_field,
+                                  Msg<Traits>& parent_msg,
+                                  LocationWith<MaybeOwnedString>& key) {
+  // Parse the enum value from string, advancing the lexer.
+  absl::optional<int32_t> enum_value;
+  RETURN_IF_ERROR(Traits::WithFieldType(
+      map_field, [&lex, &enum_value](const Desc<Traits>& map_entry_desc) {
+        ASSIGN_OR_RETURN(
+            enum_value,
+            ParseEnum<Traits>(lex, Traits::ValueField(map_entry_desc)));
+        return absl::OkStatus();
+      }));
+
+  if (enum_value.has_value()) {
+    return Traits::NewMsg(
+        map_field, parent_msg,
+        [&](const Desc<Traits>& type, Msg<Traits>& entry) -> absl::Status {
+          RETURN_IF_ERROR(ParseMapKey<Traits>(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 <typename Traits>
 absl::Status ParseMapEntry(JsonLexer& lex, Field<Traits> map_field,
                            Msg<Traits>& parent_msg,
                            LocationWith<MaybeOwnedString>& key) {
+  bool is_map_of_enums = false;
+  RETURN_IF_ERROR(Traits::WithFieldType(
+      map_field, [&is_map_of_enums](const Desc<Traits>& 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<Traits>(lex, map_field, parent_msg, key);
+  }
+
   return Traits::NewMsg(
       map_field, parent_msg,
       [&](const Desc<Traits>& type, Msg<Traits>& entry) -> absl::Status {

diff --git 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
@@ -45,8 +45,9 @@ absl::Status DoAssignOrReturn(T& lhs, absl::StatusOr<T> 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