From e5ddc45645871fbe2c6fc089ebe09f72ca727b5e Mon Sep 17 00:00:00 2001 From: Sandy Zhang Date: Thu, 30 May 2024 11:08:35 -0700 Subject: [PATCH 1/3] Reparse unknown features using extension registry containing Java features. This should not be needed for generated code, but may be needed for user calls to the public buildFrom method. FileDescriptorProto should really be parsed with java features in the extension registry (like other extensions), but we can handle this in Java runtime to ease editions adoption. PiperOrigin-RevId: 638715579 --- .../java/com/google/protobuf/Descriptors.java | 15 +++ .../com/google/protobuf/DescriptorsTest.java | 107 +++++++++++++++++- 2 files changed, 121 insertions(+), 1 deletion(-) 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 0eb67eebeda22..adaff5568e15a 100644 --- a/java/core/src/main/java/com/google/protobuf/Descriptors.java +++ b/java/core/src/main/java/com/google/protobuf/Descriptors.java @@ -2785,6 +2785,21 @@ 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())) { + ExtensionRegistry registry = ExtensionRegistry.newInstance(); + registry.add(JavaFeaturesProto.java_); + ByteString bytes = unresolvedFeatures.toByteString(); + try { + unresolvedFeatures = FeatureSet.parseFrom(bytes, registry); + } catch (InvalidProtocolBufferException e) { + throw new DescriptorValidationException( + this, "Failed to parse features with Java feature extension registry.", e); + } + } + if (this.parent != null && unresolvedFeatures.equals(FeatureSet.getDefaultInstance()) && !hasInferredLegacyProtoFeatures()) { 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 2ebdf71e75de3..508344d3bfc18 100644 --- a/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java +++ b/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java @@ -352,7 +352,7 @@ public void testFieldDescriptorDefault() throws Exception { } @Test - public void testFieldDescriptorLegacyEnumFieldTreatedAsClosed() throws Exception { + public void testProto2FieldDescriptorLegacyEnumFieldTreatedAsClosed() throws Exception { // Make an open enum definition. FileDescriptorProto openEnumFile = FileDescriptorProto.newBuilder() @@ -430,6 +430,111 @@ public void testFieldDescriptorLegacyEnumFieldTreatedAsClosed() throws Exception .isTrue(); } + @Test + public void testEditionFieldDescriptorLegacyEnumFieldTreatedAsClosed() throws Exception { + // Make an open enum definition. + FileDescriptorProto openEnumFile = + FileDescriptorProto.newBuilder() + .setName("open_enum.proto") + .setSyntax("editions") + .setEdition(Edition.EDITION_2023) + .addEnumType( + EnumDescriptorProto.newBuilder() + .setName("TestEnumOpen") + .addValue( + EnumValueDescriptorProto.newBuilder() + .setName("TestEnumOpen_VALUE0") + .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("TestClosedEnumField") + .addField( + FieldDescriptorProto.newBuilder() + .setName("int_field") + .setNumber(1) + .setType(FieldDescriptorProto.Type.TYPE_INT32) + .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL) + .build()) + .addField( + FieldDescriptorProto.newBuilder() + .setName("open_enum") + .setNumber(2) + .setType(FieldDescriptorProto.Type.TYPE_ENUM) + .setTypeName("TestEnumOpen") + .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setOptions( + DescriptorProtos.FieldOptions.newBuilder() + .setFeatures( + DescriptorProtos.FeatureSet.newBuilder() + .setExtension( + 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(); + // Ensure Java features are in unknown fields. + editionsClosedEnumFile = + FileDescriptorProto.parseFrom( + editionsClosedEnumFile.toByteString(), ExtensionRegistry.getEmptyRegistry()); + Descriptor editionsClosedMessage = + Descriptors.FileDescriptor.buildFrom( + editionsClosedEnumFile, new FileDescriptor[] {openFileDescriptor}) + .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 public void testFieldDescriptorLegacyEnumFieldTreatedAsOpen() throws Exception { // Make an open enum definition and message that treats enum fields as open. From 2426a02b90d61e6c18b8ffa411e76b1642f47ad6 Mon Sep 17 00:00:00 2001 From: Sandy Zhang Date: Mon, 17 Jun 2024 08:16:24 -0700 Subject: [PATCH 2/3] Reserialize all unresolved features using java features from the generated pool in case of descriptors from the custom pool. This extends previous workaround for java features in unknown fields, to include features extensions that are known but use a mismatched descriptor. This can happen when users bring their own descriptors via buildFrom. PiperOrigin-RevId: 644013578 --- .../java/com/google/protobuf/Descriptors.java | 37 +++-- .../com/google/protobuf/DescriptorsTest.java | 152 +++++++++++------- 2 files changed, 121 insertions(+), 68 deletions(-) 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 adaff5568e15a..569fa26e0e66a 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 508344d3bfc18..5eca109d7789c 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 From c7a006a225e0b94b639a9be694b03c835f4db6d6 Mon Sep 17 00:00:00 2001 From: Sandy Zhang Date: Mon, 17 Jun 2024 11:45:51 -0400 Subject: [PATCH 3/3] Fix checking unknown field set empty which wasn't exposed yet in 27.x --- java/core/src/main/java/com/google/protobuf/Descriptors.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 569fa26e0e66a..877cb081037bd 100644 --- a/java/core/src/main/java/com/google/protobuf/Descriptors.java +++ b/java/core/src/main/java/com/google/protobuf/Descriptors.java @@ -2804,7 +2804,7 @@ void resolveFeatures(FeatureSet unresolvedFeatures) throws DescriptorValidationE } } boolean hasPossibleUnknownJavaFeature = - !unresolvedFeatures.getUnknownFields().isEmpty() + !unresolvedFeatures.getUnknownFields().asMap().isEmpty() && unresolvedFeatures .getUnknownFields() .hasField(JavaFeaturesProto.java_.getNumber());