From 5856d09be44bec9dc5644cf1ed294f044052e458 Mon Sep 17 00:00:00 2001 From: Greg Orzell Date: Mon, 18 Feb 2019 16:23:05 +0100 Subject: [PATCH 1/3] Add tests to the TypeMapper and better super the various proto options that exist for Java. --- .../flit/protoc/gen/server/TypeMapper.java | 95 +++++--- .../protoc/gen/server/TypeMapperTest.java | 207 ++++++++++++++++++ 2 files changed, 268 insertions(+), 34 deletions(-) create mode 100644 plugin/src/test/java/com/flit/protoc/gen/server/TypeMapperTest.java diff --git a/plugin/src/main/java/com/flit/protoc/gen/server/TypeMapper.java b/plugin/src/main/java/com/flit/protoc/gen/server/TypeMapper.java index 837332d..cb12e41 100644 --- a/plugin/src/main/java/com/flit/protoc/gen/server/TypeMapper.java +++ b/plugin/src/main/java/com/flit/protoc/gen/server/TypeMapper.java @@ -2,7 +2,6 @@ import com.google.protobuf.DescriptorProtos; import com.squareup.javapoet.ClassName; - import java.io.File; import java.util.HashMap; import java.util.List; @@ -10,7 +9,7 @@ public class TypeMapper { - // holds the qualified class name to the short class reference + // Maps the protobuf package and name to the fully qualified name of the generated Java class. private final Map mapping = new HashMap<>(); public TypeMapper() { @@ -22,7 +21,8 @@ public TypeMapper(List files) { public void add(DescriptorProtos.FileDescriptorProto proto) { proto.getMessageTypeList().forEach(m -> { - mapping.put("." + proto.getPackage() + "." + m.getName(), getClassname(proto) + "." + m.getName()); + mapping.put("." + proto.getPackage() + "." + m.getName(), + getOuterClassOrPackageName(proto) + "." + m.getName()); }); } @@ -30,49 +30,76 @@ public ClassName get(String protobufFqcn) { return ClassName.bestGuess(mapping.get(protobufFqcn)); } - public static String getClassname(DescriptorProtos.FileDescriptorProto proto) { - String clazz = proto.getOptions().getJavaOuterClassname(); + /** + * Determine where message or service in a given proto file will be generated. Depending on the + * java specific options in the spec, this could be either inside of an outer class, or at the top + * level of the package. + */ + public static String getOuterClassOrPackageName(DescriptorProtos.FileDescriptorProto proto) { + // If no 'java_package' option is provided, the protoc compiler will default to the protobuf + // package name. + String packageName = proto.getOptions().hasJavaPackage() ? + proto.getOptions().getJavaPackage() : proto.getPackage(); + + // If this option is enabled protoc will generate a class for each message/service at the top + // level of the given package space. Because message name is appended in the add method, this + // should just return the package in that case. If there are collisions protoc should give a + // warning/error. + if (proto.getOptions().getJavaMultipleFiles()) { + return packageName; + } - if (clazz == null || clazz.isEmpty()) { + // If an outer class name is provided it should be used, otherwise we need to infer one based + // on the same rules the protoc compiler uses. + String outerClass = proto.getOptions().hasJavaOuterClassname() ? + proto.getOptions().getJavaOuterClassname() : outerClassNameFromProtoName(proto); - String basename = new File(proto.getName()).getName(); - char[] classname = basename.substring(0, basename.lastIndexOf('.')).toCharArray(); - StringBuilder sb = new StringBuilder(); + if (outerClass.isEmpty()) { + throw new IllegalArgumentException("'option java_outer_classname' cannot be set to \"\"."); + } - char previous = '_'; - for (char c : classname) { - if (c == '_') { - previous = c; - continue; - } + String fqName = String.join(".", packageName, outerClass); - if (previous == '_') { - sb.append(Character.toUpperCase(c)); - } else { - sb.append(c); - } + // check to see if there are any messages with this same class name as per java proto specs + // note that we also check the services too as the protoc compiler does that as well. + for (DescriptorProtos.DescriptorProto type : proto.getMessageTypeList()) { + if (type.getName().equals(outerClass)) { + return fqName + "OuterClass"; + } + } - previous = c; + for (DescriptorProtos.ServiceDescriptorProto service : proto.getServiceList()) { + if (service.getName().equals(outerClass)) { + return fqName + "OuterClass"; } + } - clazz = sb.toString(); + return fqName; + } + + private static String outerClassNameFromProtoName(DescriptorProtos.FileDescriptorProto proto) { + String basename = new File(proto.getName()).getName(); + char[] classname = basename.substring(0, basename.lastIndexOf('.')).toCharArray(); + StringBuilder sb = new StringBuilder(); - // check to see if there are any messages with this same class name as per java proto specs - // note that we also check the services too as the protoc compiler does that as well - for (DescriptorProtos.DescriptorProto type : proto.getMessageTypeList()) { - if (type.getName().equals(clazz)) { - return clazz + "OuterClass"; - } + char previous = '_'; + for (char c : classname) { + if (c == '_') { + previous = c; + continue; } - for (DescriptorProtos.ServiceDescriptorProto service : proto.getServiceList()) { - if (service.getName().equals(clazz)) { - return clazz + "OuterClass"; - } + if (previous == '_') { + sb.append(Character.toUpperCase(c)); + } else { + sb.append(c); } + + previous = c; } - return String.join(".", proto.getOptions().getJavaPackage(), clazz); - } + String clazz = sb.toString(); + return clazz; + } } diff --git a/plugin/src/test/java/com/flit/protoc/gen/server/TypeMapperTest.java b/plugin/src/test/java/com/flit/protoc/gen/server/TypeMapperTest.java new file mode 100644 index 0000000..43bfbac --- /dev/null +++ b/plugin/src/test/java/com/flit/protoc/gen/server/TypeMapperTest.java @@ -0,0 +1,207 @@ +package com.flit.protoc.gen.server; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import com.google.protobuf.DescriptorProtos; +import com.squareup.javapoet.ClassName; +import org.junit.Test; + +public class TypeMapperTest { + + private static final String PROTO_PACKAGE = "flit.test"; + private static final String JAVA_PACKAGE = "com.flit.test"; + + private static final DescriptorProtos.DescriptorProto MAP_MESSAGE = DescriptorProtos.DescriptorProto + .newBuilder() + .setName("Map") + .build(); + + private static final DescriptorProtos.DescriptorProto MAPPER_MESSAGE = DescriptorProtos.DescriptorProto + .newBuilder() + .setName("Mapper") + .build(); + + @Test + public void protoPackage() { + DescriptorProtos.FileOptions options = DescriptorProtos.FileOptions.newBuilder() + .setJavaMultipleFiles(false) + .setJavaOuterClassname("Mapper") + .build(); + + DescriptorProtos.FileDescriptorProto proto = DescriptorProtos.FileDescriptorProto.newBuilder() + .setPackage(PROTO_PACKAGE) + .setName("mapper.proto") + .setOptions(options) + .addMessageType(MAP_MESSAGE) + .build(); + + TypeMapper mapper = new TypeMapper(); + mapper.add(proto); + + ClassName result = mapper.get(".flit.test.Map"); + assertEquals(PROTO_PACKAGE, result.packageName()); + assertEquals("Mapper", result.enclosingClassName().simpleName()); + assertEquals("Map", result.simpleName()); + } + + @Test + public void protoPackageNameCollision() { + DescriptorProtos.FileOptions options = DescriptorProtos.FileOptions.newBuilder() + .setJavaMultipleFiles(false) + .build(); + + DescriptorProtos.FileDescriptorProto proto = DescriptorProtos.FileDescriptorProto.newBuilder() + .setPackage(PROTO_PACKAGE) + .setName("mapper.proto") + .setOptions(options) + .addMessageType(MAPPER_MESSAGE) + .build(); + + TypeMapper mapper = new TypeMapper(); + mapper.add(proto); + + ClassName result = mapper.get(".flit.test.Mapper"); + assertEquals(PROTO_PACKAGE, result.packageName()); + assertEquals("MapperOuterClass", result.enclosingClassName().simpleName()); + assertEquals("Mapper", result.simpleName()); + } + + @Test + public void protoPackageWithOuterClass() { + DescriptorProtos.FileOptions options = DescriptorProtos.FileOptions.newBuilder() + .setJavaMultipleFiles(false) + .setJavaOuterClassname("Mapper") + .build(); + + DescriptorProtos.FileDescriptorProto proto = DescriptorProtos.FileDescriptorProto.newBuilder() + .setPackage(PROTO_PACKAGE) + .setName("mapper.proto") + .setOptions(options) + .addMessageType(MAP_MESSAGE) + .build(); + + TypeMapper mapper = new TypeMapper(); + mapper.add(proto); + + ClassName result = mapper.get(".flit.test.Map"); + assertEquals(PROTO_PACKAGE, result.packageName()); + assertEquals("Mapper", result.enclosingClassName().simpleName()); + assertEquals("Map", result.simpleName()); + } + + @Test + public void javaPackage() { + DescriptorProtos.FileOptions options = DescriptorProtos.FileOptions.newBuilder() + .setJavaMultipleFiles(false) + .setJavaPackage(JAVA_PACKAGE) + .build(); + + DescriptorProtos.FileDescriptorProto proto = DescriptorProtos.FileDescriptorProto.newBuilder() + .setPackage(PROTO_PACKAGE) + .setName("mapper.proto") + .setOptions(options) + .addMessageType(MAP_MESSAGE) + .build(); + + TypeMapper mapper = new TypeMapper(); + mapper.add(proto); + + ClassName result = mapper.get(".flit.test.Map"); + assertEquals(JAVA_PACKAGE, result.packageName()); + assertEquals("Mapper", result.enclosingClassName().simpleName()); + assertEquals("Map", result.simpleName()); + } + + @Test + public void javaPackageNameCollision() { + DescriptorProtos.FileOptions options = DescriptorProtos.FileOptions.newBuilder() + .setJavaMultipleFiles(false) + .setJavaPackage(JAVA_PACKAGE) + .build(); + + DescriptorProtos.FileDescriptorProto proto = DescriptorProtos.FileDescriptorProto.newBuilder() + .setPackage(PROTO_PACKAGE) + .setName("mapper.proto") + .setOptions(options) + .addMessageType(MAPPER_MESSAGE) + .build(); + + TypeMapper mapper = new TypeMapper(); + mapper.add(proto); + + ClassName result = mapper.get(".flit.test.Mapper"); + assertEquals(JAVA_PACKAGE, result.packageName()); + assertEquals("MapperOuterClass", result.enclosingClassName().simpleName()); + assertEquals("Mapper", result.simpleName()); + } + + @Test + public void javaPackageWithOuterClass() { + DescriptorProtos.FileOptions options = DescriptorProtos.FileOptions.newBuilder() + .setJavaMultipleFiles(false) + .setJavaOuterClassname("Mapper") + .setJavaPackage(JAVA_PACKAGE) + .build(); + + DescriptorProtos.FileDescriptorProto proto = DescriptorProtos.FileDescriptorProto.newBuilder() + .setPackage(PROTO_PACKAGE) + .setName("mapper.proto") + .setOptions(options) + .addMessageType(MAP_MESSAGE) + .build(); + + TypeMapper mapper = new TypeMapper(); + mapper.add(proto); + + ClassName result = mapper.get(".flit.test.Map"); + assertEquals(JAVA_PACKAGE, result.packageName()); + assertEquals("Mapper", result.enclosingClassName().simpleName()); + assertEquals("Map", result.simpleName()); + } + + @Test(expected = IllegalArgumentException.class) + public void javaPackageWithOuterClassEmpty() { + DescriptorProtos.FileOptions options = DescriptorProtos.FileOptions.newBuilder() + .setJavaMultipleFiles(false) + .setJavaOuterClassname("") + .setJavaPackage(JAVA_PACKAGE) + .build(); + + DescriptorProtos.FileDescriptorProto proto = DescriptorProtos.FileDescriptorProto.newBuilder() + .setPackage(PROTO_PACKAGE) + .setName("mapper.proto") + .setOptions(options) + .addMessageType(MAP_MESSAGE) + .build(); + + TypeMapper mapper = new TypeMapper(); + mapper.add(proto); + + mapper.get(".flit.test.Map"); + } + + @Test + public void javaPackageWithOuterClassMultiFile() { + DescriptorProtos.FileOptions options = DescriptorProtos.FileOptions.newBuilder() + .setJavaMultipleFiles(true) + .setJavaOuterClassname("Mapper") + .setJavaPackage(JAVA_PACKAGE) + .build(); + + DescriptorProtos.FileDescriptorProto proto = DescriptorProtos.FileDescriptorProto.newBuilder() + .setPackage(PROTO_PACKAGE) + .setName("mapper.proto") + .setOptions(options) + .addMessageType(MAP_MESSAGE) + .build(); + + TypeMapper mapper = new TypeMapper(); + mapper.add(proto); + + ClassName result = mapper.get(".flit.test.Map"); + assertEquals(JAVA_PACKAGE, result.packageName()); + assertNull(result.enclosingClassName()); + assertEquals("Map", result.simpleName()); + } +} From bbc0d50f4bdb6bb44e3caaa44af413aacb8694dc Mon Sep 17 00:00:00 2001 From: Greg Orzell Date: Tue, 19 Feb 2019 09:26:25 +0100 Subject: [PATCH 2/3] Add/bump version numbers where appropriate. --- plugin/gradle.properties | 2 +- runtime/core/gradle.properties | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 runtime/core/gradle.properties diff --git a/plugin/gradle.properties b/plugin/gradle.properties index beb72cc..de652db 100644 --- a/plugin/gradle.properties +++ b/plugin/gradle.properties @@ -1 +1 @@ -version=1.0.0 \ No newline at end of file +version=1.1.0 diff --git a/runtime/core/gradle.properties b/runtime/core/gradle.properties new file mode 100644 index 0000000..beb72cc --- /dev/null +++ b/runtime/core/gradle.properties @@ -0,0 +1 @@ +version=1.0.0 \ No newline at end of file From 9e355426547c585256c3923bed4216d70f5c8ad6 Mon Sep 17 00:00:00 2001 From: Greg Orzell Date: Mon, 4 Mar 2019 09:39:52 +0100 Subject: [PATCH 3/3] Fix bug in test that lead to duplicate behavior in two tests. --- .../src/test/java/com/flit/protoc/gen/server/TypeMapperTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/plugin/src/test/java/com/flit/protoc/gen/server/TypeMapperTest.java b/plugin/src/test/java/com/flit/protoc/gen/server/TypeMapperTest.java index 43bfbac..aedd91a 100644 --- a/plugin/src/test/java/com/flit/protoc/gen/server/TypeMapperTest.java +++ b/plugin/src/test/java/com/flit/protoc/gen/server/TypeMapperTest.java @@ -26,7 +26,6 @@ public class TypeMapperTest { public void protoPackage() { DescriptorProtos.FileOptions options = DescriptorProtos.FileOptions.newBuilder() .setJavaMultipleFiles(false) - .setJavaOuterClassname("Mapper") .build(); DescriptorProtos.FileDescriptorProto proto = DescriptorProtos.FileDescriptorProto.newBuilder()