Skip to content

Commit

Permalink
servicetalk-grpc-protoc name conflict fixes (#2157)
Browse files Browse the repository at this point in the history 
Motivation:
servicetalk-grpc-protoc attempts to detect naming conflicts
and appends numbers to the end of the class names to avoid
the conflict. However in the event `java_outer_classname` isn't
specified and the service type name conflicts with the file name
a conflict is incorrectly detected which leads to forcing a `0`
at the end of the service name.

Detection of implicit class name isn't consistent with
protobuf-java which may result in not generating code in the
same file when `java_multiple_files = false`.

Types (messages, enums) that start with a lower case value
would fail to compile due to CLassName.bestGuess restrictions.

Potential User Changes:
If you are in this scenario you must regenerate code and your
class names will change, requiring code changes.
  • Loading branch information
@Scottmitch
Scottmitch authored Mar 22, 2022
1 parent c27ef5c commit 40c3e3c
Show file tree
Hide file tree
Showing 19 changed files with 675 additions and 79 deletions.
1 change: 1 addition & 0 deletions servicetalk-grpc-protoc/gradle/checkstyle/suppressions.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,5 @@
<suppressions>
<suppress id="ConsolePrint" files="io[\\/]servicetalk[\\/]grpc[\\/]protoc[\\/]Main.java"/>
<suppress checks="UncommentedMain" files="io[\\/]servicetalk[\\/]grpc[\\/]protoc[\\/]Main.java"/>
<suppress checks="LineLength" files="io[\\/]servicetalk[\\/]grpc[\\/]protoc[\\/]FileDescriptor.java"/>
</suppressions>
107 changes: 79 additions & 28 deletions servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/FileDescriptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import com.google.protobuf.DescriptorProtos;
import com.google.protobuf.DescriptorProtos.DescriptorProto;
import com.google.protobuf.DescriptorProtos.EnumDescriptorProto;
import com.google.protobuf.DescriptorProtos.FileDescriptorProto;
import com.google.protobuf.DescriptorProtos.FileOptions;
import com.google.protobuf.DescriptorProtos.MethodDescriptorProto;
Expand Down Expand Up @@ -47,23 +48,24 @@
* A single protoc file for which we will be generating classes
*/
final class FileDescriptor implements GenerationContext {
private static final String GENERATED_BY_COMMENT = "Generated by ServiceTalk proto compiler";

private static final String GENERATED_BY_COMMENT = "Generated by ServiceTalk gRPC protoc plugin";
/**
* Inferred behavior from protobuf-java is that if no suffix is explicitly provided the root file name will have
* this suffix. See
* <a href="https://github.com/protocolbuffers/protobuf/blob/v3.19.4/src/google/protobuf/compiler/java/java_name_resolver.cc#L49">java_name_resolver.cc</a>
*/
private static final String OUTER_CLASS_SUFFIX = "OuterClass";
private final FileDescriptorProto protoFile;
private final String sanitizedProtoFileName;
@Nullable
private final String protoPackageName;
private final boolean deprecated;
private final boolean multipleClassFiles;
@Nullable
private final String javaPackageName;
@Nullable
private final String outerClassName;
@Nullable
private final String typeNameSuffix;
private final List<TypeSpec.Builder> serviceClassBuilders;
@Nullable
private Set<String> reservedJavaTypeName;
private final Set<String> reservedJavaTypeName = new HashSet<>();

/**
* A single protoc file for which we will be generating classes
Expand All @@ -74,22 +76,26 @@ final class FileDescriptor implements GenerationContext {
FileDescriptor(final FileDescriptorProto protoFile,
@Nullable final String typeNameSuffix) {
this.protoFile = protoFile;
sanitizedProtoFileName = sanitizeFileName(protoFile.getName());
final String sanitizedProtoFileName = sanitizeFileName(protoFile.getName());
protoPackageName = protoFile.hasPackage() ? protoFile.getPackage() : null;
this.typeNameSuffix = typeNameSuffix;

if (protoFile.hasOptions()) {
final FileOptions fileOptions = protoFile.getOptions();
deprecated = fileOptions.hasDeprecated() && fileOptions.getDeprecated();
multipleClassFiles = fileOptions.hasJavaMultipleFiles() && fileOptions.getJavaMultipleFiles();
javaPackageName = fileOptions.hasJavaPackage() ? fileOptions.getJavaPackage() : null;
outerClassName = fileOptions.hasJavaOuterClassname() ? fileOptions.getJavaOuterClassname() : null;
javaPackageName = fileOptions.hasJavaPackage() ? fileOptions.getJavaPackage() :
inferJavaPackageName(protoPackageName, sanitizedProtoFileName);
outerClassName = fileOptions.hasJavaOuterClassname() ?
sanitizeClassName(fileOptions.getJavaOuterClassname()) :
inferOuterClassName(sanitizedProtoFileName, protoFile);
} else {
deprecated = false;
multipleClassFiles = false;
javaPackageName = null;
outerClassName = null;
javaPackageName = inferJavaPackageName(protoPackageName, sanitizedProtoFileName);
outerClassName = inferOuterClassName(sanitizedProtoFileName, protoFile);
}
reservedJavaTypeName.add(outerClassName);

serviceClassBuilders = new ArrayList<>(protoFile.getServiceCount());
}
Expand All @@ -115,26 +121,21 @@ DescriptorProtos.SourceCodeInfo sourceCodeInfo() {
}

private void addMessageTypes(final List<DescriptorProto> messageTypes,
@Nullable String parentProtoScope,
@Nullable String parentJavaScope,
final @Nullable String parentProtoScope,
final String parentJavaScope,
final Map<String, ClassName> messageTypesMap) {

messageTypes.forEach(t -> {
final String protoTypeName = (parentProtoScope != null ? parentProtoScope : "") + '.' + t.getName();
final String javaClassName = parentJavaScope + '.' + t.getName();
messageTypesMap.put(protoTypeName, ClassName.bestGuess(javaClassName));
final String protoTypeName = parentProtoScope != null ?
(parentProtoScope + '.' + t.getName()) : '.' + t.getName();
final ClassName className = ClassName.get(parentJavaScope, t.getName());
messageTypesMap.put(protoTypeName, className);

addMessageTypes(t.getNestedTypeList(), protoTypeName, javaClassName, messageTypesMap);
addMessageTypes(t.getNestedTypeList(), protoTypeName, className.canonicalName(), messageTypesMap);
});
}

@Override
public String deconflictJavaTypeName(final String name) {
if (reservedJavaTypeName == null) {
reservedJavaTypeName = new HashSet<>();
reservedJavaTypeName.add(outerJavaClassName());
}

if (reservedJavaTypeName.add(name)) {
return name;
}
Expand Down Expand Up @@ -227,13 +228,63 @@ private static void insertSingleFileContent(final String content, String fileNam
}

private String outerJavaClassName() {
return isNotNullNorEmpty(outerClassName) ? sanitizeClassName(outerClassName) :
sanitizeClassName(sanitizedProtoFileName);
return outerClassName;
}

private String javaPackageName() {
return isNotNullNorEmpty(javaPackageName) ? javaPackageName :
isNotNullNorEmpty(protoPackageName) ? protoPackageName : sanitizeClassName(sanitizedProtoFileName);
return javaPackageName;
}

private static String inferOuterClassName(String sanitizedProtoFileName, FileDescriptorProto protoFile) {
final String sanitizeClassName = sanitizeClassName(sanitizedProtoFileName);
return hasConflictingClassName(sanitizeClassName, protoFile) ?
sanitizeClassName + OUTER_CLASS_SUFFIX : sanitizeClassName;
}

/**
* See <a href="https://github.com/protocolbuffers/protobuf/blob/v3.19.4/src/google/protobuf/compiler/java/java_name_resolver.cc#L192-L214">java_name_resolver.cc</a>.
* @param sanitizedClassName The sanitized classname to check for conflicts.
* @param protoFile The {@link FileDescriptorProto} to search for conflicting names in.
* @return {@code true} if there is a name conflict, {@code false} otherwise.
*/
private static boolean hasConflictingClassName(String sanitizedClassName, FileDescriptorProto protoFile) {
for (EnumDescriptorProto enumDescriptor : protoFile.getEnumTypeList()) {
if (enumDescriptor.getName().equals(sanitizedClassName)) {
return true;
}
}
for (ServiceDescriptorProto serviceDescriptor : protoFile.getServiceList()) {
if (serviceDescriptor.getName().equals(sanitizedClassName)) {
return true;
}
}
for (DescriptorProto typeDescriptor : protoFile.getMessageTypeList()) {
if (hasConflictingClassName(sanitizedClassName, typeDescriptor)) {
return true;
}
}
return false;
}

private static boolean hasConflictingClassName(String sanitizedClassName, DescriptorProto typeDescriptor) {
if (typeDescriptor.getName().equals(sanitizedClassName)) {
return true;
}
for (DescriptorProto nestedTypeDescriptor : typeDescriptor.getNestedTypeList()) {
if (hasConflictingClassName(sanitizedClassName, nestedTypeDescriptor)) {
return true;
}
}
for (EnumDescriptorProto enumDescriptor : typeDescriptor.getEnumTypeList()) {
if (enumDescriptor.getName().equals(sanitizedClassName)) {
return true;
}
}
return false;
}

private static String inferJavaPackageName(@Nullable String protoPackageName, String sanitizedProtoFileName) {
return isNotNullNorEmpty(protoPackageName) ? protoPackageName : sanitizeClassName(sanitizedProtoFileName);
}

private static String sanitizeFileName(final String v) {
Expand Down
94 changes: 48 additions & 46 deletions servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Types.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,78 +44,80 @@ final class Types {
private static final ClassName Collection = ClassName.get("java.util", "Collection");

static final ClassName RouteExecutionStrategy =
bestGuess(routerApiPkg + ".RouteExecutionStrategy");
ClassName.get(routerApiPkg, "RouteExecutionStrategy");
static final ClassName RouteExecutionStrategyFactory =
bestGuess(routerApiPkg + ".RouteExecutionStrategyFactory");
ClassName.get(routerApiPkg, "RouteExecutionStrategyFactory");

static final ClassName BlockingIterable = bestGuess(concurrentPkg + ".BlockingIterable");
static final ClassName BlockingIterable = ClassName.get(concurrentPkg, "BlockingIterable");

static final ClassName AsyncCloseable = bestGuess(concurrentApiPkg + ".AsyncCloseable");
static final ClassName Completable = bestGuess(concurrentApiPkg + ".Completable");
static final ClassName Publisher = bestGuess(concurrentApiPkg + ".Publisher");
static final ClassName Single = bestGuess(concurrentApiPkg + ".Single");
static final ClassName AsyncCloseable = ClassName.get(concurrentApiPkg, "AsyncCloseable");
static final ClassName Completable = ClassName.get(concurrentApiPkg, "Completable");
static final ClassName Publisher = ClassName.get(concurrentApiPkg, "Publisher");
static final ClassName Single = ClassName.get(concurrentApiPkg, "Single");

static final ClassName BlockingGrpcClient = bestGuess(grpcApiPkg + ".BlockingGrpcClient");
static final ClassName BlockingGrpcService = bestGuess(grpcApiPkg + ".BlockingGrpcService");
static final ClassName GrpcClientMetadata = bestGuess(grpcApiPkg + ".GrpcClientMetadata");
static final ClassName DefaultGrpcClientMetadata = bestGuess(grpcApiPkg + ".DefaultGrpcClientMetadata");
static final ClassName GrpcClient = bestGuess(grpcApiPkg + ".GrpcClient");
static final ClassName GrpcClientCallFactory = bestGuess(grpcApiPkg + ".GrpcClientCallFactory");
static final ClassName GrpcClientFactory = bestGuess(grpcApiPkg + ".GrpcClientFactory");
static final ClassName GrpcExecutionContext = bestGuess(grpcApiPkg + ".GrpcExecutionContext");
static final ClassName GrpcExecutionStrategy = bestGuess(grpcApiPkg + ".GrpcExecutionStrategy");
static final ClassName GrpcStatusException = bestGuess(grpcApiPkg + ".GrpcStatusException");
static final ClassName Identity = bestGuess(encodingApiPkg + ".Identity");
static final ClassName BufferDecoderGroup = bestGuess(encodingApiPkg + ".BufferDecoderGroup");
static final ClassName EmptyBufferDecoderGroup = bestGuess(encodingApiPkg + ".EmptyBufferDecoderGroup");
static final ClassName BufferEncoder = bestGuess(encodingApiPkg + ".BufferEncoder");
static final ClassName BlockingGrpcClient = ClassName.get(grpcApiPkg, "BlockingGrpcClient");
static final ClassName BlockingGrpcService = ClassName.get(grpcApiPkg, "BlockingGrpcService");
static final ClassName GrpcClientMetadata = ClassName.get(grpcApiPkg, "GrpcClientMetadata");
static final ClassName DefaultGrpcClientMetadata = ClassName.get(grpcApiPkg, "DefaultGrpcClientMetadata");
static final ClassName GrpcClient = ClassName.get(grpcApiPkg, "GrpcClient");
static final ClassName GrpcClientCallFactory = ClassName.get(grpcApiPkg, "GrpcClientCallFactory");
static final ClassName GrpcClientFactory = ClassName.get(grpcApiPkg, "GrpcClientFactory");
static final ClassName GrpcExecutionContext = ClassName.get(grpcApiPkg, "GrpcExecutionContext");
static final ClassName GrpcExecutionStrategy = ClassName.get(grpcApiPkg, "GrpcExecutionStrategy");
static final ClassName GrpcStatusException = ClassName.get(grpcApiPkg, "GrpcStatusException");
static final ClassName Identity = ClassName.get(encodingApiPkg, "Identity");
static final ClassName BufferDecoderGroup = ClassName.get(encodingApiPkg, "BufferDecoderGroup");
static final ClassName EmptyBufferDecoderGroup = ClassName.get(encodingApiPkg, "EmptyBufferDecoderGroup");
static final ClassName BufferEncoder = ClassName.get(encodingApiPkg, "BufferEncoder");
static final TypeName BufferEncoderList = ParameterizedTypeName.get(List, BufferEncoder);
static final ClassName ContentCodec = bestGuess(encodingApiPkg + ".ContentCodec");
static final ClassName ContentCodec = ClassName.get(encodingApiPkg, "ContentCodec");
static final TypeName GrpcSupportedCodings = ParameterizedTypeName.get(List, ContentCodec);
static final ClassName GrpcPayloadWriter = bestGuess(grpcApiPkg + ".GrpcPayloadWriter");
static final ClassName GrpcRoutes = bestGuess(grpcApiPkg + ".GrpcRoutes");
static final ClassName GrpcSerializationProvider = bestGuess(grpcApiPkg + ".GrpcSerializationProvider");
static final ClassName GrpcBindableService = bestGuess(grpcApiPkg + ".GrpcBindableService");
static final ClassName GrpcService = bestGuess(grpcApiPkg + ".GrpcService");
static final ClassName GrpcServiceContext = bestGuess(grpcApiPkg + ".GrpcServiceContext");
static final ClassName GrpcServiceFactory = bestGuess(grpcApiPkg + ".GrpcServiceFactory");
static final ClassName GrpcMethodDescriptor = bestGuess(grpcApiPkg + ".MethodDescriptor");
static final ClassName GrpcMethodDescriptors = bestGuess(grpcApiPkg + ".MethodDescriptors");
static final ClassName GrpcPayloadWriter = ClassName.get(grpcApiPkg, "GrpcPayloadWriter");
static final ClassName GrpcRoutes = ClassName.get(grpcApiPkg, "GrpcRoutes");
static final ClassName GrpcSerializationProvider = ClassName.get(grpcApiPkg, "GrpcSerializationProvider");
static final ClassName GrpcBindableService = ClassName.get(grpcApiPkg, "GrpcBindableService");
static final ClassName GrpcService = ClassName.get(grpcApiPkg, "GrpcService");
static final ClassName GrpcServiceContext = ClassName.get(grpcApiPkg, "GrpcServiceContext");
static final ClassName GrpcServiceFactory = ClassName.get(grpcApiPkg, "GrpcServiceFactory");
static final ClassName GrpcMethodDescriptor = ClassName.get(grpcApiPkg, "MethodDescriptor");
static final ClassName GrpcMethodDescriptors = ClassName.get(grpcApiPkg, "MethodDescriptors");
static final ParameterizedTypeName GrpcMethodDescriptorCollection = ParameterizedTypeName.get(Collection,
ParameterizedTypeName.get(GrpcMethodDescriptor, Wildcard, Wildcard));

static final ClassName BlockingClientCall = bestGuess(GrpcClientCallFactory + ".BlockingClientCall");
static final ClassName BlockingClientCall = GrpcClientCallFactory.nestedClass("BlockingClientCall");
static final ClassName BlockingRequestStreamingClientCall =
bestGuess(GrpcClientCallFactory + ".BlockingRequestStreamingClientCall");
GrpcClientCallFactory.nestedClass("BlockingRequestStreamingClientCall");
static final ClassName BlockingResponseStreamingClientCall =
bestGuess(GrpcClientCallFactory + ".BlockingResponseStreamingClientCall");
GrpcClientCallFactory.nestedClass("BlockingResponseStreamingClientCall");
static final ClassName BlockingStreamingClientCall =
bestGuess(GrpcClientCallFactory + ".BlockingStreamingClientCall");
static final ClassName ClientCall = bestGuess(GrpcClientCallFactory + ".ClientCall");
GrpcClientCallFactory.nestedClass("BlockingStreamingClientCall");
static final ClassName ClientCall = GrpcClientCallFactory.nestedClass("ClientCall");
static final ClassName RequestStreamingClientCall =
bestGuess(GrpcClientCallFactory + ".RequestStreamingClientCall");
GrpcClientCallFactory.nestedClass("RequestStreamingClientCall");
static final ClassName ResponseStreamingClientCall =
bestGuess(GrpcClientCallFactory + ".ResponseStreamingClientCall");
static final ClassName StreamingClientCall = bestGuess(GrpcClientCallFactory + ".StreamingClientCall");
GrpcClientCallFactory.nestedClass("ResponseStreamingClientCall");
static final ClassName StreamingClientCall = GrpcClientCallFactory.nestedClass("StreamingClientCall");

// Inner protected types need bestGuess to avoid adding imports which aren't visible and causing compile problems.
static final ClassName AllGrpcRoutes = bestGuess(grpcRoutesFqcn + ".AllGrpcRoutes");
static final ClassName RequestStreamingRoute = bestGuess(grpcRoutesFqcn + ".RequestStreamingRoute");
static final ClassName ResponseStreamingRoute = bestGuess(grpcRoutesFqcn + ".ResponseStreamingRoute");
static final ClassName Route = bestGuess(grpcRoutesFqcn + ".Route");
static final ClassName StreamingRoute = bestGuess(grpcRoutesFqcn + ".StreamingRoute");
static final ClassName BlockingRequestStreamingRoute = bestGuess(grpcRoutesFqcn + ".BlockingRequestStreamingRoute");
static final ClassName BlockingResponseStreamingRoute = bestGuess(grpcRoutesFqcn +
".BlockingResponseStreamingRoute");
static final ClassName BlockingRequestStreamingRoute =
bestGuess(grpcRoutesFqcn + ".BlockingRequestStreamingRoute");
static final ClassName BlockingResponseStreamingRoute =
bestGuess(grpcRoutesFqcn + ".BlockingResponseStreamingRoute");
static final ClassName BlockingRoute = bestGuess(grpcRoutesFqcn + ".BlockingRoute");
static final ClassName BlockingStreamingRoute = bestGuess(grpcRoutesFqcn + ".BlockingStreamingRoute");

@Deprecated
static final ClassName ProtoBufSerializationProviderBuilder =
bestGuess(grpcProtobufPkg + ".ProtoBufSerializationProviderBuilder");
static final ClassName ProtobufSerializerFactory = bestGuess(protobufDataPkg + ".ProtobufSerializerFactory");
ClassName.get(grpcProtobufPkg, "ProtoBufSerializationProviderBuilder");
static final ClassName ProtobufSerializerFactory = ClassName.get(protobufDataPkg, "ProtobufSerializerFactory");

static final TypeName GrpcRouteExecutionStrategyFactory = ParameterizedTypeName.get(RouteExecutionStrategyFactory,
GrpcExecutionStrategy);
static final TypeName GrpcRouteExecutionStrategyFactory =
ParameterizedTypeName.get(RouteExecutionStrategyFactory, GrpcExecutionStrategy);

private Types() {
// no instances
Expand Down
Loading

0 comments on commit 40c3e3c

Please sign in to comment.