Skip to content

Commit

Permalink
Reserialize all unresolved features using java features from the gene…
Browse files Browse the repository at this point in the history 
…rated 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
  • Loading branch information
@zhangskz @copybara-github
zhangskz authored and copybara-github committed Jun 17, 2024
1 parent 40ffd46 commit 415a147
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 68 deletions.
37 changes: 25 additions & 12 deletions java/core/src/main/java/com/google/protobuf/Descriptors.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down
152 changes: 96 additions & 56 deletions java/core/src/test/java/com/google/protobuf/DescriptorsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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()
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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
Expand Down

0 comments on commit 415a147

Please sign in to comment.