Skip to content
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.

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

fix: enum value conflict in generated ProtoSchema descriptor. #469

Merged
merged 2 commits into from Aug 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Expand Up @@ -38,7 +38,7 @@ If you are using Maven without BOM, add this to your dependencies:
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-bigquerystorage</artifactId>
<version>1.2.1</version>
<version>1.3.1</version>
</dependency>

```
Expand Down
Expand Up @@ -30,6 +30,10 @@
// protobuf::DescriptorProto
// that can be reconstructed by the backend.
public class ProtoSchemaConverter {
private static String getNameFromFullName(String fullName) {
return fullName.replace('.', '_');
}

private static ProtoSchema convertInternal(
Descriptor input,
Set<String> visitedTypes,
Expand All @@ -40,8 +44,8 @@ private static ProtoSchema convertInternal(
if (rootProtoSchema == null) {
rootProtoSchema = resultProto;
}
String protoName = input.getFullName();
protoName = protoName.replace('.', '_');
String protoFullName = input.getFullName();
String protoName = getNameFromFullName(protoFullName);
resultProto.setName(protoName);
Set<String> localEnumTypes = new HashSet<String>();
visitedTypes.add(input.getFullName());
Expand All @@ -51,9 +55,9 @@ private static ProtoSchema convertInternal(
if (inputField.getType() == FieldDescriptor.Type.GROUP
|| inputField.getType() == FieldDescriptor.Type.MESSAGE) {
String msgFullName = inputField.getMessageType().getFullName();
msgFullName = msgFullName.replace('.', '_');
String msgName = getNameFromFullName(msgFullName);
if (structTypes.contains(msgFullName)) {
resultField.setTypeName(msgFullName);
resultField.setTypeName(msgName);
} else {
if (visitedTypes.contains(msgFullName)) {
throw new InvalidArgumentException(
Expand All @@ -76,42 +80,32 @@ private static ProtoSchema convertInternal(
rootProtoSchema.getNestedType(rootProtoSchema.getNestedTypeCount() - 1).getName());
}
}

if (inputField.getType() == FieldDescriptor.Type.ENUM) {
// For enums, in order to avoid value conflict, we will always define
// a enclosing struct called enum_full_name_E that includes the actual
// enum.
String enumFullName = inputField.getEnumType().getFullName();
// If the enum is defined within the current message, we don't want to
// pull it out to the top since then the same enum values will not be
// allowed if the value collides with other enums.
if (enumFullName.startsWith(input.getFullName())) {
String enumName = inputField.getEnumType().getName();
if (localEnumTypes.contains(enumName)) {
resultField.setTypeName(enumName);
} else {
resultProto.addEnumType(inputField.getEnumType().toProto());
resultField.setTypeName(enumName);
localEnumTypes.add(enumName);
}
String enclosingTypeName = getNameFromFullName(enumFullName) + "_E";
String enumName = inputField.getEnumType().getName();
String actualEnumFullName = enclosingTypeName + "." + enumName;
if (enumTypes.contains(enumFullName)) {
resultField.setTypeName(actualEnumFullName);
} else {
// If the enum is defined elsewhere, then redefine it at the top
// message scope. There is a problem that different enum values might
// be OK when living under its original scope, but when they all live
// in top scope, their values cannot collide. Say if thers A.Color with
// RED and B.Color with RED, if they are redefined here, the RED will
// collide with each other and thus not allowed.
enumFullName = enumFullName.replace('.', '_');
if (enumTypes.contains(enumFullName)) {
resultField.setTypeName(enumFullName);
} else {
EnumDescriptorProto enumType =
inputField.getEnumType().toProto().toBuilder().setName(enumFullName).build();
resultProto.addEnumType(enumType);
resultField.setTypeName(enumFullName);
enumTypes.add(enumFullName);
}
EnumDescriptorProto enumType = inputField.getEnumType().toProto();
resultProto.addNestedType(
DescriptorProto.newBuilder()
.setName(enclosingTypeName)
.addEnumType(enumType.toBuilder().setName(enumName))
.build());
resultField.setTypeName(actualEnumFullName);
enumTypes.add(enumFullName);
}
}
resultProto.addField(resultField);
}
structTypes.add(protoName);
structTypes.add(protoFullName);

return ProtoSchema.newBuilder().setProtoDescriptor(resultProto.build()).build();
}

Expand Down
Expand Up @@ -76,23 +76,26 @@ public void convertSimple() {
+ " number: 8\n"
+ " label: LABEL_OPTIONAL\n"
+ " type: TYPE_ENUM\n"
+ " type_name: \"com_google_cloud_bigquery_storage_test_TestEnum\"\n"
+ " type_name: \"com_google_cloud_bigquery_storage_test_TestEnum_E.TestEnum\"\n"
+ "}\n"
+ "field {\n"
+ " name: \"string_value\"\n"
+ " number: 9\n"
+ " label: LABEL_REQUIRED\n"
+ " type: TYPE_STRING\n"
+ "}\n"
+ "enum_type {\n"
+ " name: \"com_google_cloud_bigquery_storage_test_TestEnum\"\n"
+ " value {\n"
+ " name: \"TestEnum0\"\n"
+ " number: 0\n"
+ " }\n"
+ " value {\n"
+ " name: \"TestEnum1\"\n"
+ " number: 1\n"
+ "nested_type {\n"
+ " name: \"com_google_cloud_bigquery_storage_test_TestEnum_E\"\n"
+ " enum_type {\n"
+ " name: \"TestEnum\"\n"
+ " value {\n"
+ " name: \"TestEnum0\"\n"
+ " number: 0\n"
+ " }\n"
+ " value {\n"
+ " name: \"TestEnum1\"\n"
+ " number: 1\n"
+ " }\n"
+ " }\n"
+ "}\n",
protoSchema.getProtoDescriptor().toString());
Expand Down Expand Up @@ -150,7 +153,7 @@ public void convertRecursive() {
Assert.fail("No exception raised");
} catch (InvalidArgumentException e) {
Assert.assertEquals(
"Recursive type is not supported:com.google.cloud.bigquery.storage.test.ContainsRecursive",
"Recursive type is not supported:com.google.cloud.bigquery.storage.test.RecursiveType",
e.getMessage());
}
}
Expand Down
2 changes: 1 addition & 1 deletion synth.metadata
Expand Up @@ -11,7 +11,7 @@
"git": {
"name": ".",
"remote": "https://github.com/googleapis/java-bigquerystorage.git",
"sha": "5cfefd325d2e68318e8ac6e7e0a98e63837ec194"
"sha": "36b368a14bb7d45f92af23bc0dd6fe08ad79b085"
}
},
{
Expand Down