diff --git a/java/core/src/main/java/com/google/protobuf/Descriptors.java b/java/core/src/main/java/com/google/protobuf/Descriptors.java index adaff5568e15..569fa26e0e66 100644 --- a/java/core/src/main/java/com/google/protobuf/Descriptors.java +++ b/java/core/src/main/java/com/google/protobuf/Descriptors.java @@ -2785,10 +2785,30 @@ private GenericDescriptor() {} public abstract FileDescriptor getFile(); void resolveFeatures(FeatureSet unresolvedFeatures) throws DescriptorValidationException { - // Unknown java features may be passed by users via public buildFrom but should not occur from - // generated code. - if (!unresolvedFeatures.getUnknownFields().isEmpty() - && unresolvedFeatures.getUnknownFields().hasField(JavaFeaturesProto.java_.getNumber())) { + if (this.parent != null + && unresolvedFeatures.equals(FeatureSet.getDefaultInstance()) + && !hasInferredLegacyProtoFeatures()) { + this.features = this.parent.features; + validateFeatures(); + return; + } + + // Java features from a custom pool (i.e. buildFrom) may end up in unknown fields or + // use a different descriptor from the generated pool used by the Java runtime. + boolean hasPossibleCustomJavaFeature = false; + for (FieldDescriptor f : unresolvedFeatures.getExtensionFields().keySet()) { + if (f.getNumber() == JavaFeaturesProto.java_.getNumber() + && f != JavaFeaturesProto.java_.getDescriptor()) { + hasPossibleCustomJavaFeature = true; + continue; + } + } + boolean hasPossibleUnknownJavaFeature = + !unresolvedFeatures.getUnknownFields().isEmpty() + && unresolvedFeatures + .getUnknownFields() + .hasField(JavaFeaturesProto.java_.getNumber()); + if (hasPossibleCustomJavaFeature || hasPossibleUnknownJavaFeature) { ExtensionRegistry registry = ExtensionRegistry.newInstance(); registry.add(JavaFeaturesProto.java_); ByteString bytes = unresolvedFeatures.toByteString(); @@ -2799,14 +2819,7 @@ void resolveFeatures(FeatureSet unresolvedFeatures) throws DescriptorValidationE this, "Failed to parse features with Java feature extension registry.", e); } } - - if (this.parent != null - && unresolvedFeatures.equals(FeatureSet.getDefaultInstance()) - && !hasInferredLegacyProtoFeatures()) { - this.features = this.parent.features; - validateFeatures(); - return; - } + FeatureSet.Builder features; if (this.parent == null) { Edition edition = getFile().getEdition(); diff --git a/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java b/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java index 508344d3bfc1..5eca109d7789 100644 --- a/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java +++ b/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java @@ -352,8 +352,9 @@ public void testFieldDescriptorDefault() throws Exception { } @Test - public void testProto2FieldDescriptorLegacyEnumFieldTreatedAsClosed() throws Exception { - // Make an open enum definition. + public void testFieldDescriptorLegacyEnumFieldTreatedAsOpen() throws Exception { + // Make an open enum definition and message that treats enum fields as open. + FileDescriptorProto openEnumFile = FileDescriptorProto.newBuilder() .setName("open_enum.proto") @@ -367,30 +368,9 @@ public void testProto2FieldDescriptorLegacyEnumFieldTreatedAsClosed() throws Exc .setNumber(0) .build()) .build()) - .build(); - FileDescriptor openFileDescriptor = - Descriptors.FileDescriptor.buildFrom(openEnumFile, new FileDescriptor[0]); - EnumDescriptor openEnum = openFileDescriptor.getEnumTypes().get(0); - assertThat(openEnum.isClosed()).isFalse(); - - // Create a message that treats enum fields as closed. - FileDescriptorProto closedEnumFile = - FileDescriptorProto.newBuilder() - .setName("closed_enum_field.proto") - .addDependency("open_enum.proto") - .setSyntax("proto2") - .addEnumType( - EnumDescriptorProto.newBuilder() - .setName("TestEnum") - .addValue( - EnumValueDescriptorProto.newBuilder() - .setName("TestEnum_VALUE0") - .setNumber(0) - .build()) - .build()) .addMessageType( DescriptorProto.newBuilder() - .setName("TestClosedEnumField") + .setName("TestOpenEnumField") .addField( FieldDescriptorProto.newBuilder() .setName("int_field") @@ -406,32 +386,21 @@ public void testProto2FieldDescriptorLegacyEnumFieldTreatedAsClosed() throws Exc .setTypeName("TestEnumOpen") .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL) .build()) - .addField( - FieldDescriptorProto.newBuilder() - .setName("closed_enum") - .setNumber(3) - .setType(FieldDescriptorProto.Type.TYPE_ENUM) - .setTypeName("TestEnum") - .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL) - .build()) .build()) .build(); - Descriptor closedMessage = - Descriptors.FileDescriptor.buildFrom( - closedEnumFile, new FileDescriptor[] {openFileDescriptor}) - .getMessageTypes() - .get(0); - assertThat(closedMessage.findFieldByName("int_field").legacyEnumFieldTreatedAsClosed()) + FileDescriptor openEnumFileDescriptor = + Descriptors.FileDescriptor.buildFrom(openEnumFile, new FileDescriptor[0]); + Descriptor openMessage = openEnumFileDescriptor.getMessageTypes().get(0); + EnumDescriptor openEnum = openEnumFileDescriptor.findEnumTypeByName("TestEnumOpen"); + assertThat(openEnum.isClosed()).isFalse(); + assertThat(openMessage.findFieldByName("int_field").legacyEnumFieldTreatedAsClosed()) + .isFalse(); + assertThat(openMessage.findFieldByName("open_enum").legacyEnumFieldTreatedAsClosed()) .isFalse(); - - assertThat(closedMessage.findFieldByName("closed_enum").legacyEnumFieldTreatedAsClosed()) - .isTrue(); - assertThat(closedMessage.findFieldByName("open_enum").legacyEnumFieldTreatedAsClosed()) - .isTrue(); } @Test - public void testEditionFieldDescriptorLegacyEnumFieldTreatedAsClosed() throws Exception { + public void testEditionFieldDescriptorLegacyEnumFieldTreatedAsClosedUnknown() throws Exception { // Make an open enum definition. FileDescriptorProto openEnumFile = FileDescriptorProto.newBuilder() @@ -536,12 +505,19 @@ public void testEditionFieldDescriptorLegacyEnumFieldTreatedAsClosed() throws Ex } @Test - public void testFieldDescriptorLegacyEnumFieldTreatedAsOpen() throws Exception { - // Make an open enum definition and message that treats enum fields as open. + public void testEditionFieldDescriptorLegacyEnumFieldTreatedAsClosedCustomPool() + throws Exception { + + FileDescriptor javaFeaturesDescriptor = + Descriptors.FileDescriptor.buildFrom( + JavaFeaturesProto.getDescriptor().toProto(), + new FileDescriptor[] {DescriptorProtos.getDescriptor()}); + // Make an open enum definition. FileDescriptorProto openEnumFile = FileDescriptorProto.newBuilder() .setName("open_enum.proto") - .setSyntax("proto3") + .setSyntax("editions") + .setEdition(Edition.EDITION_2023) .addEnumType( EnumDescriptorProto.newBuilder() .setName("TestEnumOpen") @@ -551,9 +527,38 @@ public void testFieldDescriptorLegacyEnumFieldTreatedAsOpen() throws Exception { .setNumber(0) .build()) .build()) + .build(); + FileDescriptor openFileDescriptor = + Descriptors.FileDescriptor.buildFrom(openEnumFile, new FileDescriptor[0]); + EnumDescriptor openEnum = openFileDescriptor.getEnumTypes().get(0); + assertThat(openEnum.isClosed()).isFalse(); + + // Create a message that treats enum fields as closed. + FileDescriptorProto editionsClosedEnumFile = + FileDescriptorProto.newBuilder() + .setName("editions_closed_enum_field.proto") + .addDependency("open_enum.proto") + .setSyntax("editions") + .setEdition(Edition.EDITION_2023) + .setOptions( + FileOptions.newBuilder() + .setFeatures( + DescriptorProtos.FeatureSet.newBuilder() + .setEnumType(DescriptorProtos.FeatureSet.EnumType.CLOSED) + .build()) + .build()) + .addEnumType( + EnumDescriptorProto.newBuilder() + .setName("TestEnum") + .addValue( + EnumValueDescriptorProto.newBuilder() + .setName("TestEnum_VALUE0") + .setNumber(0) + .build()) + .build()) .addMessageType( DescriptorProto.newBuilder() - .setName("TestOpenEnumField") + .setName("TestClosedEnumField") .addField( FieldDescriptorProto.newBuilder() .setName("int_field") @@ -568,18 +573,53 @@ public void testFieldDescriptorLegacyEnumFieldTreatedAsOpen() throws Exception { .setType(FieldDescriptorProto.Type.TYPE_ENUM) .setTypeName("TestEnumOpen") .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setOptions( + DescriptorProtos.FieldOptions.newBuilder() + .setFeatures( + DescriptorProtos.FeatureSet.newBuilder() + .setExtension( + // Extension cannot be directly set using custom + // descriptor, so set using generated for now. + JavaFeaturesProto.java_, + JavaFeaturesProto.JavaFeatures.newBuilder() + .setLegacyClosedEnum(true) + .build()) + .build()) + .build()) + .build()) + .addField( + FieldDescriptorProto.newBuilder() + .setName("closed_enum") + .setNumber(3) + .setType(FieldDescriptorProto.Type.TYPE_ENUM) + .setTypeName("TestEnum") + .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL) .build()) .build()) .build(); - FileDescriptor openEnumFileDescriptor = - Descriptors.FileDescriptor.buildFrom(openEnumFile, new FileDescriptor[0]); - Descriptor openMessage = openEnumFileDescriptor.getMessageTypes().get(0); - EnumDescriptor openEnum = openEnumFileDescriptor.findEnumTypeByName("TestEnumOpen"); - assertThat(openEnum.isClosed()).isFalse(); - assertThat(openMessage.findFieldByName("int_field").legacyEnumFieldTreatedAsClosed()) - .isFalse(); - assertThat(openMessage.findFieldByName("open_enum").legacyEnumFieldTreatedAsClosed()) + // Reparse using custom java features descriptor. + ExtensionRegistry registry = ExtensionRegistry.newInstance(); + registry.add( + javaFeaturesDescriptor.getExtensions().get(0), + DynamicMessage.getDefaultInstance( + javaFeaturesDescriptor.getExtensions().get(0).getMessageType())); + editionsClosedEnumFile = + FileDescriptorProto.parseFrom(editionsClosedEnumFile.toByteString(), registry); + Descriptor editionsClosedMessage = + Descriptors.FileDescriptor.buildFrom( + editionsClosedEnumFile, + new FileDescriptor[] {openFileDescriptor, javaFeaturesDescriptor}) + .getMessageTypes() + .get(0); + assertThat( + editionsClosedMessage.findFieldByName("int_field").legacyEnumFieldTreatedAsClosed()) .isFalse(); + assertThat( + editionsClosedMessage.findFieldByName("closed_enum").legacyEnumFieldTreatedAsClosed()) + .isTrue(); + assertThat( + editionsClosedMessage.findFieldByName("open_enum").legacyEnumFieldTreatedAsClosed()) + .isTrue(); } @Test