From 2f7b66765478326964dc50f0d235d200e57634fb Mon Sep 17 00:00:00 2001 From: Ryan Michela Date: Wed, 20 Feb 2019 10:57:23 -0800 Subject: [PATCH 1/3] Correctly handle nested types when java_multiple_files=true --- .../salesforce/jprotoc/ProtoTypeMapTest.java | 17 +++++++++- .../test/proto/nested_multiple_files.proto | 32 +++++++++++++++++++ .../com/salesforce/jprotoc/ProtoTypeMap.java | 11 +++---- 3 files changed, 53 insertions(+), 7 deletions(-) create mode 100644 jprotoc/jprotoc-test/src/test/proto/nested_multiple_files.proto diff --git a/jprotoc/jprotoc-test/src/test/java/com/salesforce/jprotoc/ProtoTypeMapTest.java b/jprotoc/jprotoc-test/src/test/java/com/salesforce/jprotoc/ProtoTypeMapTest.java index caf994b9..b77f3913 100644 --- a/jprotoc/jprotoc-test/src/test/java/com/salesforce/jprotoc/ProtoTypeMapTest.java +++ b/jprotoc/jprotoc-test/src/test/java/com/salesforce/jprotoc/ProtoTypeMapTest.java @@ -1,6 +1,7 @@ package com.salesforce.jprotoc; import com.google.common.io.ByteStreams; +import com.google.protobuf.DescriptorProtos; import com.google.protobuf.ExtensionRegistry; import com.google.protobuf.compiler.PluginProtos; import org.junit.BeforeClass; @@ -9,6 +10,7 @@ import java.io.File; import java.io.FileInputStream; import java.io.IOException; +import java.util.List; import static org.assertj.core.api.Assertions.assertThat; @@ -29,7 +31,8 @@ public static void buildProtoTypeMap() throws IOException { byte[] generatorRequestBytes = ByteStreams.toByteArray(new FileInputStream(new File(dumpPath))); PluginProtos.CodeGeneratorRequest request = PluginProtos.CodeGeneratorRequest.parseFrom( generatorRequestBytes, ExtensionRegistry.newInstance()); - protoTypeMap = ProtoTypeMap.of(request.getProtoFileList()); + List fileProtos = request.getProtoFileList(); + protoTypeMap = ProtoTypeMap.of(fileProtos); } @@ -60,6 +63,18 @@ public void nestedTypeMappings() { assertProtoTypeMapping(".nested.Outer.MiddleBB.Inner", nested.NestedOuterClass.Outer.MiddleBB.Inner.class); } + /** + * Verify that nested proto message types map correctly when {@code option java_multiple_files = true}. + */ + @Test + public void nestedTypeMappingsMultipleFiles() { + assertProtoTypeMapping(".nested_multiple_files.Outer", nested_multiple_files.Outer.class); + assertProtoTypeMapping(".nested_multiple_files.Outer.MiddleAA", nested_multiple_files.Outer.MiddleAA.class); + assertProtoTypeMapping(".nested_multiple_files.Outer.MiddleAA.Inner", nested_multiple_files.Outer.MiddleAA.Inner.class); + assertProtoTypeMapping(".nested_multiple_files.Outer.MiddleBB", nested_multiple_files.Outer.MiddleBB.class); + assertProtoTypeMapping(".nested_multiple_files.Outer.MiddleBB.Inner", nested_multiple_files.Outer.MiddleBB.Inner.class); + } + /** * Verify that types with nested enums sharing the same name as a top-level type don't conflict. */ diff --git a/jprotoc/jprotoc-test/src/test/proto/nested_multiple_files.proto b/jprotoc/jprotoc-test/src/test/proto/nested_multiple_files.proto new file mode 100644 index 00000000..b05ffec6 --- /dev/null +++ b/jprotoc/jprotoc-test/src/test/proto/nested_multiple_files.proto @@ -0,0 +1,32 @@ +syntax = "proto3"; + +package nested_multiple_files; + +option java_multiple_files = true; + +message Outer { // Level 0 + enum FooEnum { + FOO = 0; + BAR = 1; + CHEESE = 2; + } + message MiddleAA { // Level 1 + + message Inner { // Level 2 + int64 ival = 1; + bool booly = 2; + Outer.FooEnum enum = 3; + } + } + message MiddleBB { // Level 1 + message Inner { // Level 2 + int32 ival = 1; + bool booly = 2; + Outer.FooEnum enum = 3; + } + } +} + +service Nested { + rpc doNested (Outer.MiddleAA.Inner) returns (Outer.MiddleBB.Inner) {} +} \ No newline at end of file diff --git a/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java b/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java index 48b4fc9a..a4878167 100644 --- a/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java +++ b/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java @@ -21,6 +21,7 @@ */ public final class ProtoTypeMap { + private static final Joiner DOT_JOINER = Joiner.on('.').skipNulls(); private final ImmutableMap types; private ProtoTypeMap(@Nonnull ImmutableMap types) { @@ -58,7 +59,7 @@ public static ProtoTypeMap of(@Nonnull Collection types.put( protoPackage + "." + e.getName(), - toJavaTypeName(e.getName(), enclosingClassName, javaPackage))); + DOT_JOINER.join(javaPackage, enclosingClassName, e.getName()))); // Identify top-level messages, and nested types fileDescriptor.getMessageTypeList().forEach( @@ -74,15 +75,13 @@ private static void recursivelyAddTypes(ImmutableMap.Builder typ String protoTypeName = protoPackage + "." + m.getName(); types.put( protoTypeName, - toJavaTypeName(m.getName(), enclosingClassName, javaPackage)); + DOT_JOINER.join(javaPackage, enclosingClassName, m.getName())); // Identify any nested Enums m.getEnumTypeList().forEach( e -> types.put( protoPackage + "." + m.getName() + "." + e.getName(), - toJavaTypeName(e.getName(), - enclosingClassName + "." + m.getName(), - javaPackage))); + DOT_JOINER.join(javaPackage, enclosingClassName, m.getName(), e.getName()))); // Recursively identify any nested types m.getNestedTypeList().forEach( @@ -90,7 +89,7 @@ private static void recursivelyAddTypes(ImmutableMap.Builder typ types, n, protoPackage + "." + m.getName(), - enclosingClassName + "." + m.getName(), + DOT_JOINER.join(enclosingClassName, m.getName()), javaPackage)); } From b08298b9cf109c7a9198f3263540da5dd9e8e9f5 Mon Sep 17 00:00:00 2001 From: Ryan Michela Date: Wed, 20 Feb 2019 11:04:05 -0800 Subject: [PATCH 2/3] Remove vestigial toJavaTypeName() method --- .../com/salesforce/jprotoc/ProtoTypeMap.java | 18 -------- .../jprotoc/ProtoTypeUtilsTest.java | 45 ------------------- 2 files changed, 63 deletions(-) delete mode 100644 jprotoc/jprotoc/src/test/java/com/salesforce/jprotoc/ProtoTypeUtilsTest.java diff --git a/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java b/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java index a4878167..f79f3e7b 100644 --- a/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java +++ b/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java @@ -103,24 +103,6 @@ public String toJavaTypeName(@Nonnull String protoTypeName) { return types.get(protoTypeName); } - /** - * Returns the full Java type name based on the given protobuf type parameters. - * - * @param className the protobuf type name - * @param enclosingClassName the optional enclosing class for the given type - * @param javaPackage the proto file's configured java package name - */ - public static String toJavaTypeName( - @Nonnull String className, - @Nullable String enclosingClassName, - @Nullable String javaPackage) { - - Preconditions.checkNotNull(className, "className"); - - Joiner dotJoiner = Joiner.on('.').skipNulls(); - return dotJoiner.join(javaPackage, enclosingClassName, className); - } - private static String getJavaOuterClassname( DescriptorProtos.FileDescriptorProto fileDescriptor, DescriptorProtos.FileOptions fileOptions) { diff --git a/jprotoc/jprotoc/src/test/java/com/salesforce/jprotoc/ProtoTypeUtilsTest.java b/jprotoc/jprotoc/src/test/java/com/salesforce/jprotoc/ProtoTypeUtilsTest.java deleted file mode 100644 index ab86cda9..00000000 --- a/jprotoc/jprotoc/src/test/java/com/salesforce/jprotoc/ProtoTypeUtilsTest.java +++ /dev/null @@ -1,45 +0,0 @@ -/* - * Copyright (c) 2019, Salesforce.com, Inc. - * All rights reserved. - * Licensed under the BSD 3-Clause license. - * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause - */ - -package com.salesforce.jprotoc; - -import static org.apache.commons.lang.RandomStringUtils.randomAlphabetic; -import static org.assertj.core.api.Assertions.assertThat; -import org.junit.Test; - -import java.util.concurrent.ThreadLocalRandom; - -public class ProtoTypeUtilsTest { - - @Test - public void classWithEnclosingClassAndJavaPackage() { - final String className = randomAlphabetic(ThreadLocalRandom.current().nextInt(5, 10)); - final String enclosingClassName = randomAlphabetic(ThreadLocalRandom.current().nextInt(5, 10)); - final String javaPackage = randomAlphabetic(ThreadLocalRandom.current().nextInt(5, 10)); - - assertThat(ProtoTypeMap.toJavaTypeName(className, enclosingClassName, javaPackage)) - .isEqualTo(javaPackage + "." + enclosingClassName + "." + className); - } - - @Test - public void classWithEnclosingClass() { - final String className = randomAlphabetic(ThreadLocalRandom.current().nextInt(5, 10)); - final String enclosingClassName = randomAlphabetic(ThreadLocalRandom.current().nextInt(5, 10)); - - assertThat(ProtoTypeMap.toJavaTypeName(className, enclosingClassName, null)) - .isEqualTo(enclosingClassName + "." + className); - } - - @Test - public void classWithJavaPackage() { - final String className = randomAlphabetic(ThreadLocalRandom.current().nextInt(5, 10)); - final String javaPackage = randomAlphabetic(ThreadLocalRandom.current().nextInt(5, 10)); - - assertThat(ProtoTypeMap.toJavaTypeName(className, null, javaPackage)) - .isEqualTo(javaPackage + "." + className); - } -} From 83ba34f1b961dd9950435ab8a4a3c3832efec60c Mon Sep 17 00:00:00 2001 From: Ryan Michela Date: Wed, 20 Feb 2019 11:10:24 -0800 Subject: [PATCH 3/3] Checkstyle --- .../src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java | 1 - 1 file changed, 1 deletion(-) diff --git a/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java b/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java index f79f3e7b..680a8710 100644 --- a/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java +++ b/jprotoc/jprotoc/src/main/java/com/salesforce/jprotoc/ProtoTypeMap.java @@ -14,7 +14,6 @@ import java.util.Collection; import javax.annotation.Nonnull; -import javax.annotation.Nullable; /** * {@code ProtoTypeMap} maintains a dictionary for looking up Java type names when given proto types.