From 3e1382f247de5e6ee8727130280e34fa01d3c088 Mon Sep 17 00:00:00 2001 From: Yiru Tang Date: Thu, 6 Aug 2020 12:55:21 -0700 Subject: [PATCH] fix: enum value conflict in generated ProtoSchema descriptor. (#469) * chore: regen readme (#464) autosynth cannot find the source of changes triggered by earlier changes in this repository, or by version upgrades to tools such as linters. * fix: Enum value confict in Proto conversion. Solution is to convert enum types always in enclosing struct type. Co-authored-by: Yoshi Automation Bot --- .../v1alpha2/ProtoSchemaConverter.java | 60 +++++++++---------- .../v1alpha2/ProtoSchemaConverterTest.java | 25 ++++---- 2 files changed, 41 insertions(+), 44 deletions(-) diff --git a/google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1alpha2/ProtoSchemaConverter.java b/google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1alpha2/ProtoSchemaConverter.java index 31e5003a3e..a623bffaad 100644 --- a/google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1alpha2/ProtoSchemaConverter.java +++ b/google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1alpha2/ProtoSchemaConverter.java @@ -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 visitedTypes, @@ -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 localEnumTypes = new HashSet(); visitedTypes.add(input.getFullName()); @@ -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( @@ -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(); } diff --git a/google-cloud-bigquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1alpha2/ProtoSchemaConverterTest.java b/google-cloud-bigquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1alpha2/ProtoSchemaConverterTest.java index 9d9ca3dab5..390ed67286 100644 --- a/google-cloud-bigquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1alpha2/ProtoSchemaConverterTest.java +++ b/google-cloud-bigquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1alpha2/ProtoSchemaConverterTest.java @@ -76,7 +76,7 @@ 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" @@ -84,15 +84,18 @@ public void convertSimple() { + " 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()); @@ -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()); } }