Fix mishandling of open enum + explicit presence fields.

The java generator currently uses the enum-typed getters in mergeFrom for explicit presence fields, even for open enums. This results in buggy behavior where unknown enum values can trigger runtime exceptions during merges. It is reproducible in proto3 by using the `optional` keyword, but is more noticeable in edition 2023 due to the feature defaults. PiperOrigin-RevId: 713780284
protocolbuffers · Jan 9, 2025 · 0ccd845 · 0ccd845
1 parent b339b5c
commit 0ccd845
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 9 deletions.
diff --git a/java/core/src/test/java/com/google/protobuf/FieldPresenceTest.java b/java/core/src/test/java/com/google/protobuf/FieldPresenceTest.java
@@ -102,6 +102,7 @@ public void testHasMethodForProto3Optional() throws Exception {
     assertThat(TestProto3Optional.getDefaultInstance().hasOptionalBool()).isFalse();
     assertThat(TestProto3Optional.getDefaultInstance().hasOptionalString()).isFalse();
     assertThat(TestProto3Optional.getDefaultInstance().hasOptionalBytes()).isFalse();
+    assertThat(TestProto3Optional.getDefaultInstance().hasOptionalNestedEnum()).isFalse();
 
     TestProto3Optional.Builder builder = TestProto3Optional.newBuilder().setOptionalInt32(0);
     assertThat(builder.hasOptionalInt32()).isTrue();
@@ -125,6 +126,67 @@ public void testHasMethodForProto3Optional() throws Exception {
     assertThat(proto.toBuilder().hasOptionalInt32()).isTrue();
   }
 
+  @Test
+  public void testMergeFrom_unknownExplicitOpenEnum() throws Exception {
+    TestProto3Optional.Builder builder =
+        TestProto3Optional.newBuilder().setOptionalNestedEnumValue(100);
+
+    TestProto3Optional.Builder mergedBuilder =
+        TestProto3Optional.newBuilder()
+            .setOptionalNestedEnum(TestProto3Optional.NestedEnum.FOO)
+            .mergeFrom(builder.build());
+
+    assertThat(builder.hasOptionalNestedEnum()).isTrue();
+    assertThat(builder.build().getOptionalNestedEnumValue()).isEqualTo(100);
+    assertThat(mergedBuilder.hasOptionalNestedEnum()).isTrue();
+    assertThat(mergedBuilder.getOptionalNestedEnumValue()).isEqualTo(100);
+  }
+
+  @Test
+  public void testParseFrom_unknownExplicitOpenEnum() throws Exception {
+    TestProto3Optional.Builder builder =
+        TestProto3Optional.newBuilder().setOptionalNestedEnumValue(100);
+
+    TestProto3Optional parsedProto =
+        TestProto3Optional.parseFrom(
+            builder.build().toByteArray(), ExtensionRegistry.getEmptyRegistry());
+
+    assertThat(parsedProto.hasOptionalNestedEnum()).isTrue();
+    assertThat(parsedProto.getOptionalNestedEnumValue()).isEqualTo(100);
+  }
+
+  @Test
+  public void testMergeFrom_defaultExplicitOpenEnum() throws Exception {
+    TestProto3Optional.Builder builder =
+        TestProto3Optional.newBuilder().setOptionalNestedEnumValue(0);
+
+    TestProto3Optional.Builder otherBuilder =
+        TestProto3Optional.newBuilder()
+            .setOptionalNestedEnum(TestProto3Optional.NestedEnum.FOO)
+            .mergeFrom(builder.build());
+
+    assertThat(builder.hasOptionalNestedEnum()).isTrue();
+    assertThat(builder.build().getOptionalNestedEnum())
+        .isEqualTo(TestProto3Optional.NestedEnum.UNSPECIFIED);
+    assertThat(otherBuilder.hasOptionalNestedEnum()).isTrue();
+    assertThat(otherBuilder.getOptionalNestedEnum())
+        .isEqualTo(TestProto3Optional.NestedEnum.UNSPECIFIED);
+  }
+
+  @Test
+  public void testParseFrom_defaultExplicitOpenEnum() throws Exception {
+    TestProto3Optional.Builder builder =
+        TestProto3Optional.newBuilder().setOptionalNestedEnumValue(0);
+
+    TestProto3Optional parsedProto =
+        TestProto3Optional.parseFrom(
+            builder.build().toByteArray(), ExtensionRegistry.getEmptyRegistry());
+
+    assertThat(parsedProto.hasOptionalNestedEnum()).isTrue();
+    assertThat(parsedProto.getOptionalNestedEnum())
+        .isEqualTo(TestProto3Optional.NestedEnum.UNSPECIFIED);
+  }
+
   private static void assertProto3OptionalReflection(String name) throws Exception {
     FieldDescriptor fieldDescriptor = TestProto3Optional.getDescriptor().findFieldByName(name);
     OneofDescriptor oneofDescriptor = fieldDescriptor.getContainingOneof();

diff --git a/src/google/protobuf/compiler/java/full/enum_field.cc b/src/google/protobuf/compiler/java/full/enum_field.cc
@@ -277,19 +277,21 @@ void ImmutableEnumFieldGenerator::GenerateBuilderClearCode(
 void ImmutableEnumFieldGenerator::GenerateMergingCode(
     io::Printer* printer) const {
   if (descriptor_->has_presence()) {
-    printer->Print(variables_,
-                   "if (other.has$capitalized_name$()) {\n"
-                   "  set$capitalized_name$(other.get$capitalized_name$());\n"
-                   "}\n");
-  } else if (SupportUnknownEnumValue(descriptor_)) {
+    printer->Print(variables_, "if (other.has$capitalized_name$()) {\n");
+  } else {
+    printer->Print(variables_, "if (other.$name$_ != $default_number$) {\n");
+  }
+  printer->Indent();
+  if (SupportUnknownEnumValue(descriptor_)) {
     printer->Print(
         variables_,
-        "if (other.$name$_ != $default_number$) {\n"
-        "  set$capitalized_name$Value(other.get$capitalized_name$Value());\n"
-        "}\n");
+        "set$capitalized_name$Value(other.get$capitalized_name$Value());\n");
   } else {
-    ABSL_LOG(FATAL) << "Can't reach here.";
+    printer->Print(variables_,
+                   "set$capitalized_name$(other.get$capitalized_name$());\n");
   }
+  printer->Outdent();
+  printer->Print("}\n");
 }
 
 void ImmutableEnumFieldGenerator::GenerateBuildingCode(