Skip to content

fix(bigquerystorage): hoist shared enum wrappers to the schema root #14708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mvanhorn wants to merge 1 commit into
base: main
Choose a base branch
from
+100 −1
Open

fix(bigquerystorage): hoist shared enum wrappers to the schema root #14708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...querystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ProtoSchemaConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ private static ProtoSchema convertInternal(
resultField.setTypeName(actualEnumFullName);
} else {
EnumDescriptorProto enumType = inputField.getEnumType().toProto();
resultProto.addNestedType(
rootProtoSchema.addNestedType(
DescriptorProto.newBuilder()
.setName(enclosingTypeName)
.addEnumType(enumType.toBuilder().setName(enumName))
Comment on lines +96 to 99

@gemini-code-assist gemini-code-assist Bot Oct 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Hoisting all enum wrappers to the root schema (rootProtoSchema) can introduce name collisions if the input schema contains multiple distinct enums with the same short name (e.g., Status or Type) defined in different nested messages or packages.

Previously, these wrappers were added to resultProto (the local message descriptor), which kept them isolated within their respective parent message namespaces (e.g., Root.First.Status_E and Root.Second.Status_E). By hoisting them all to rootProtoSchema, they will now all be added directly to the root message with the same name (e.g., Root.Status_E), leading to a DescriptorValidationException due to duplicate nested type names.

To prevent this, consider making the hoisted wrapper name unique (for example, by prefixing it with the path of the enclosing message) or only hoisting enums when they are actually shared across different messages, while ensuring unique naming at the root. It would also be beneficial to add a test case in ProtoSchemaConverterTest.java that exercises multiple same-named enums in different nested messages to ensure they do not collide.

Expand Down
99 changes: 99 additions & 0 deletions ...ystorage/src/test/java/com/google/cloud/bigquery/storage/v1/ProtoSchemaConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,17 @@
package com.google.cloud.bigquery.storage.v1;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;

import com.google.api.gax.rpc.InvalidArgumentException;
import com.google.cloud.bigquery.storage.test.Test.*;
import com.google.protobuf.DescriptorProtos.DescriptorProto;
import com.google.protobuf.DescriptorProtos.EnumDescriptorProto;
import com.google.protobuf.DescriptorProtos.EnumValueDescriptorProto;
import com.google.protobuf.DescriptorProtos.FieldDescriptorProto;
import com.google.protobuf.DescriptorProtos.FieldDescriptorProto.Label;
import com.google.protobuf.DescriptorProtos.FieldDescriptorProto.Type;
import com.google.protobuf.DescriptorProtos.FileDescriptorProto;
import com.google.protobuf.Descriptors;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -170,6 +177,98 @@ void convertRecursiveTopMessage() {
e.getMessage());
}

@Test
void convertSharedEnums() throws Descriptors.DescriptorValidationException {
// Exercise sibling order, a deeper first reference, and root references in either order.
for (int layout = 0; layout < 5; layout++) {
EnumDescriptorProto sharedEnum =
EnumDescriptorProto.newBuilder()
.setName("Shared")
.addValue(EnumValueDescriptorProto.newBuilder().setName("ZERO").setNumber(0))
.addValue(EnumValueDescriptorProto.newBuilder().setName("ONE").setNumber(1))
.build();
FieldDescriptorProto enumField =
FieldDescriptorProto.newBuilder()
.setName("value")
.setNumber(1)
.setLabel(Label.LABEL_OPTIONAL)
.setType(Type.TYPE_ENUM)
.setTypeName(".Shared")
.build();
DescriptorProto first =
DescriptorProto.newBuilder()
.setName("First")
.addField(enumField)
.addField(enumField.toBuilder().setName("duplicate").setNumber(2))
.build();
DescriptorProto second = first.toBuilder().setName("Second").build();
FieldDescriptorProto firstField =
enumField.toBuilder()
.setName("first")
.setType(Type.TYPE_MESSAGE)
.setTypeName(layout == 2 ? ".Deep" : ".First")
.build();
FieldDescriptorProto secondField =
firstField.toBuilder().setName("second").setNumber(2).setTypeName(".Second").build();
DescriptorProto.Builder root = DescriptorProto.newBuilder().setName("Root");
FieldDescriptorProto rootEnumField = enumField.toBuilder().setNumber(3).build();
if (layout == 3) {
root.addField(rootEnumField);
}
root.addField(layout == 1 ? secondField : firstField);
root.addField(layout == 1 ? firstField : secondField);
if (layout == 4) {
root.addField(rootEnumField);
}
// Equal value names in distinct enums must remain isolated by their wrappers.
root.addField(enumField.toBuilder().setName("other").setNumber(4).setTypeName(".Other"));
FileDescriptorProto source =
FileDescriptorProto.newBuilder()
.setName("source.proto")
.addEnumType(sharedEnum)
.addEnumType(sharedEnum.toBuilder().setName("Other"))
.addMessageType(root)
.addMessageType(first)
.addMessageType(second)
.addMessageType(
DescriptorProto.newBuilder()
.setName("Deep")
.addField(firstField.toBuilder().setTypeName(".First")))
.build();
Descriptors.Descriptor input =
Descriptors.FileDescriptor.buildFrom(source, new Descriptors.FileDescriptor[0])
.findMessageTypeByName("Root");
ProtoSchema converted = ProtoSchemaConverter.convert(input);
Descriptors.Descriptor result =
Descriptors.FileDescriptor.buildFrom(
FileDescriptorProto.newBuilder()
.setName("converted.proto")
.addMessageType(converted.getProtoDescriptor())
.build(),
new Descriptors.FileDescriptor[0])
.findMessageTypeByName("Root");
Descriptors.EnumDescriptor resolved =
result.findNestedTypeByName("Shared_E").findEnumTypeByName("Shared");
assertEquals(sharedEnum.getValueList(), resolved.toProto().getValueList());
assertEquals(
2,
result.getNestedTypes().stream().filter(type -> !type.getEnumTypes().isEmpty()).count());
for (String childName : new String[] {"First", "Second"}) {
Descriptors.Descriptor child = result.findNestedTypeByName(childName);
assertEquals(0, child.getNestedTypes().size());
for (Descriptors.FieldDescriptor field : child.getFields()) {
assertSame(resolved, field.getEnumType());
}
}
if (layout >= 3) {
assertSame(resolved, result.findFieldByName("value").getEnumType());
}
Descriptors.EnumDescriptor other = result.findFieldByName("other").getEnumType();
assertSame(result.findNestedTypeByName("Other_E"), other.getContainingType());
assertEquals(sharedEnum.getValueList(), other.toProto().getValueList());
}
}

@Test
void convertDuplicateType() throws Descriptors.DescriptorValidationException {
DuplicateType testProto = DuplicateType.newBuilder().build();
Expand Down
Loading