From 0ccd8459ce0ee2af2693681bca75aa9f781bdb73 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Thu, 9 Jan 2025 13:37:16 -0800 Subject: [PATCH] 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 --- .../google/protobuf/FieldPresenceTest.java | 62 +++++++++++++++++++ .../protobuf/compiler/java/full/enum_field.cc | 20 +++--- 2 files changed, 73 insertions(+), 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 index 9455df0dc08ac..3afdab743422f 100644 --- 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 index 4c747cd07b7fe..14a2b596cacf4 100644 --- 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(