Fix TextFormat.Parser to appropriately handle unknown values for op…

…en enums PiperOrigin-RevId: 661229108
protocolbuffers · Aug 9, 2024 · df1aad4 · df1aad4
1 parent 903c3f1
commit df1aad4
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 15 deletions.
diff --git a/conformance/text_format_conformance_suite.cc b/conformance/text_format_conformance_suite.cc
@@ -147,12 +147,14 @@ TextFormatConformanceTestSuiteImpl<MessageType>::
   } else {
     if (MessageType::GetDescriptor()->name() == "TestAllTypesProto2") {
       RunGroupTests();
+      RunClosedEnumTests();
     }
     if (MessageType::GetDescriptor()->name() == "TestAllTypesEdition2023") {
       RunDelimitedTests();
     }
     if (MessageType::GetDescriptor()->name() == "TestAllTypesProto3") {
       RunAnyTests();
+      RunOpenEnumTests();
       // TODO Run these over proto2 also.
       RunAllTests();
     }
@@ -865,5 +867,29 @@ void TextFormatConformanceTestSuiteImpl<MessageType>::
       RECOMMENDED, input, expected);
 }
 
+template <typename MessageType>
+void TextFormatConformanceTestSuiteImpl<MessageType>::RunOpenEnumTests() {
+  RunValidTextFormatTest("ClosedEnumFieldByNumber", REQUIRED,
+                         R"(
+        optional_nested_enum: 1
+        )");
+  RunValidTextFormatTest("ClosedEnumFieldWithUnknownNumber", REQUIRED,
+                         R"(
+        optional_nested_enum: 42
+        )");
+}
+
+template <typename MessageType>
+void TextFormatConformanceTestSuiteImpl<MessageType>::RunClosedEnumTests() {
+  RunValidTextFormatTest("ClosedEnumFieldByNumber", REQUIRED,
+                         R"(
+        optional_nested_enum: 1
+        )");
+  ExpectParseFailure("ClosedEnumFieldWithUnknownNumber", REQUIRED,
+                     R"(
+        optional_nested_enum: 42
+        )");
+}
+
 }  // namespace protobuf
 }  // namespace google
diff --git a/conformance/text_format_conformance_suite.h b/conformance/text_format_conformance_suite.h
@@ -52,6 +52,8 @@ class TextFormatConformanceTestSuiteImpl {
   void RunDelimitedTests();
   void RunGroupTests();
   void RunAnyTests();
+  void RunOpenEnumTests();
+  void RunClosedEnumTests();
 
   void RunTextFormatPerformanceTests();
   void RunValidTextFormatTest(const std::string& test_name,

diff --git a/java/core/src/main/java/com/google/protobuf/TextFormat.java b/java/core/src/main/java/com/google/protobuf/TextFormat.java
@@ -534,7 +534,12 @@ private void printFieldValue(
           break;
 
         case ENUM:
-          generator.print(((EnumValueDescriptor) value).getName());
+          if (((EnumValueDescriptor) value).getIndex() == -1) {
+            // Unknown enum value, print the number instead of the name.
+            generator.print(Integer.toString(((EnumValueDescriptor) value).getNumber()));
+          } else {
+            generator.print(((EnumValueDescriptor) value).getName());
+          }
           break;
 
         case MESSAGE:
@@ -2195,7 +2200,10 @@ private void consumeFieldValue(
 
             if (tokenizer.lookingAtInteger()) {
               final int number = tokenizer.consumeInt32();
-              value = enumType.findValueByNumber(number);
+              value =
+                  enumType.isClosed()
+                      ? enumType.findValueByNumber(number)
+                      : enumType.findValueByNumberCreatingIfUnknown(number);
               if (value == null) {
                 String unknownValueMsg =
                     "Enum type \""

diff --git a/java/core/src/test/java/com/google/protobuf/UnknownEnumValueTest.java b/java/core/src/test/java/com/google/protobuf/UnknownEnumValueTest.java
@@ -19,7 +19,6 @@
 import com.google.protobuf.Proto2UnknownEnumValuesTestProto.Proto2EnumMessageWithEnumSubset;
 import com.google.protobuf.Proto2UnknownEnumValuesTestProto.Proto2TestEnum;
 import com.google.protobuf.Proto2UnknownEnumValuesTestProto.Proto2TestEnumSubset;
-import com.google.protobuf.TextFormat.ParseException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -221,7 +220,7 @@ public void testUnknownEnumValueWithDynamicMessage() throws Exception {
   }
 
   @Test
-  public void testUnknownEnumValuesInTextFormat() {
+  public void testUnknownEnumValuesInTextFormat() throws Exception {
     TestAllTypes.Builder builder = TestAllTypes.newBuilder();
     builder.setOptionalNestedEnumValue(4321);
     builder.addRepeatedNestedEnumValue(5432);
@@ -232,18 +231,13 @@ public void testUnknownEnumValuesInTextFormat() {
     String textData = TextFormat.printer().printToString(message);
     assertThat(textData)
         .isEqualTo(
-            "optional_nested_enum: UNKNOWN_ENUM_VALUE_NestedEnum_4321\n"
-                + "repeated_nested_enum: UNKNOWN_ENUM_VALUE_NestedEnum_5432\n"
-                + "packed_nested_enum: UNKNOWN_ENUM_VALUE_NestedEnum_6543\n");
+            "optional_nested_enum: 4321\n"
+                + "repeated_nested_enum: 5432\n"
+                + "packed_nested_enum: 6543\n");
 
-    // Parsing unknown enum values will fail just like parsing other kinds of
-    // unknown fields.
-    try {
-      TextFormat.merge(textData, builder);
-      assertWithMessage("Expected exception").fail();
-    } catch (ParseException e) {
-      // expected.
-    }
+    builder.clear();
+    TextFormat.merge(textData, builder);
+    assertThat(message.equals(builder.build())).isTrue();
   }
 
   @Test