From dbcbfd8f71f6194572e0de7b9d7a82be9f3b0e70 Mon Sep 17 00:00:00 2001 From: Idel Pivnitskiy Date: Wed, 9 Mar 2022 23:02:27 -0600 Subject: [PATCH] Javadoc for generated gRPC classes has warnings and errors (#2139) Motivation: Generated gRPC classes have javadoc with illegal HTML characters `<` and `>` and misses some `@param`. It causes build failures when project compiles with `-Xwerror` flag. Modifications: - Replace parametrized types with non-parametrized for javadoc `@link`; - Add missing `@param`; - Include generated classes into `testJavadoc` task for `servicetalk-grpc-protoc`; Result: Project generates javadoc successfully when run with `-Xwerror` flag. --- servicetalk-grpc-protoc/build.gradle | 14 +++ .../io/servicetalk/grpc/protoc/Generator.java | 116 ++++++++++++++---- .../io/servicetalk/grpc/protoc/Types.java | 5 +- .../io/servicetalk/grpc/protoc/Words.java | 5 +- 4 files changed, 116 insertions(+), 24 deletions(-) diff --git a/servicetalk-grpc-protoc/build.gradle b/servicetalk-grpc-protoc/build.gradle index e9e0ee541d..32dfd065ba 100644 --- a/servicetalk-grpc-protoc/build.gradle +++ b/servicetalk-grpc-protoc/build.gradle @@ -108,3 +108,17 @@ afterEvaluate { clean { delete protobuf.generatedFilesBaseDir } + +task testJavadoc(type: Javadoc) { + dependsOn tasks.compileTestJava + + source = protobuf.generatedFilesBaseDir + classpath = sourceSets.test.compileClasspath + destinationDir = file("$buildDir/tmp/testjavadoc") + options.addBooleanOption("Xdoclint:syntax", true) + options.addBooleanOption("Xdoclint:html", true) + options.addBooleanOption("Xdoclint:accessibility", true) + // Don't use Xdoclint:missing for now because not all methods/types have javadocs. +} + +test.finalizedBy(testJavadoc) diff --git a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java index cfa0f1f4f5..5dc29e6b73 100644 --- a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java +++ b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java @@ -96,6 +96,8 @@ import static io.servicetalk.grpc.protoc.Types.ResponseStreamingClientCall; import static io.servicetalk.grpc.protoc.Types.ResponseStreamingRoute; import static io.servicetalk.grpc.protoc.Types.Route; +import static io.servicetalk.grpc.protoc.Types.RouteExecutionStrategy; +import static io.servicetalk.grpc.protoc.Types.RouteExecutionStrategyFactory; import static io.servicetalk.grpc.protoc.Types.Single; import static io.servicetalk.grpc.protoc.Types.StreamingClientCall; import static io.servicetalk.grpc.protoc.Types.StreamingRoute; @@ -109,6 +111,7 @@ import static io.servicetalk.grpc.protoc.Words.Default; import static io.servicetalk.grpc.protoc.Words.Factory; import static io.servicetalk.grpc.protoc.Words.INSTANCE; +import static io.servicetalk.grpc.protoc.Words.JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT; import static io.servicetalk.grpc.protoc.Words.JAVADOC_DEPRECATED; import static io.servicetalk.grpc.protoc.Words.JAVADOC_PARAM; import static io.servicetalk.grpc.protoc.Words.JAVADOC_RETURN; @@ -343,7 +346,7 @@ private String addServiceRpcInterfaceSpec(final State state, methodDescriptorType, methodDescFieldName, isAsync)); final FieldSpec.Builder pathSpecBuilder = FieldSpec.builder(String.class, RPC_PATH) - .addJavadoc(JAVADOC_DEPRECATED + " Use {@link #$L}." + lineSeparator(), methodDescriptor) + .addJavadoc(JAVADOC_DEPRECATED + "Use {@link #$L}." + lineSeparator(), methodDescriptor) .addAnnotation(Deprecated.class) .addModifiers(PUBLIC, STATIC, FINAL) // redundant, default for interface field .initializer("$S", context.methodPath(state.serviceProto, methodProto)); @@ -466,27 +469,44 @@ private TypeSpec.Builder addServiceFactory(final State state, final TypeSpec.Bui .addType(newServiceFromRoutesClassSpec(serviceFromRoutesClass, state.serviceRpcInterfaces, state.serviceClass)) .addMethod(constructorBuilder() + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) .addModifiers(PUBLIC) .addStatement("this($T.emptyList())", Collections) .build()) .addMethod(constructorBuilder() - .addJavadoc(JAVADOC_DEPRECATED + " Use {@link #$L($T)} and {@link #$L($T)}." + lineSeparator(), - bufferDecoderGroup, BufferDecoderGroup, bufferEncoders, BufferEncoderList) + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) + .addJavadoc(lineSeparator()) + .addJavadoc(JAVADOC_PARAM + supportedMessageCodings + " the set of allowed encodings" + + lineSeparator()) + .addJavadoc(JAVADOC_DEPRECATED + "Use {@link #$L($T)} and {@link #$L($T)}." + lineSeparator(), + bufferDecoderGroup, BufferDecoderGroup, bufferEncoders, Types.List) .addAnnotation(Deprecated.class) .addModifiers(PUBLIC) .addParameter(GrpcSupportedCodings, supportedMessageCodings, FINAL) .addStatement("this.$L = $L", supportedMessageCodings, supportedMessageCodings) .build()) .addMethod(constructorBuilder() + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) + .addJavadoc(lineSeparator()) + .addJavadoc(JAVADOC_PARAM + strategyFactory + + " a factory that creates an execution strategy for different {@link $L#id() id}s" + + lineSeparator(), RouteExecutionStrategy) .addModifiers(PUBLIC) .addParameter(GrpcRouteExecutionStrategyFactory, strategyFactory, FINAL) .addStatement("this($L, $T.emptyList())", strategyFactory, Collections) .build()) .addMethod(constructorBuilder() + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) + .addJavadoc(lineSeparator()) + .addJavadoc(JAVADOC_PARAM + strategyFactory + + " a factory that creates an execution strategy for different {@link $L#id() id}s" + + lineSeparator(), RouteExecutionStrategy) + .addJavadoc(JAVADOC_PARAM + supportedMessageCodings + " the set of allowed encodings" + + lineSeparator()) .addJavadoc(JAVADOC_DEPRECATED + - " Use {@link #$L($T)}, {@link #$L($T)}, and {@link #$L($T)}." + lineSeparator(), - Builder, GrpcRouteExecutionStrategyFactory, bufferDecoderGroup, BufferDecoderGroup, - bufferEncoders, BufferEncoderList) + "Use {@link #$L($T)}, {@link #$L($T)}, and {@link #$L($T)}." + lineSeparator(), + Builder, RouteExecutionStrategyFactory, bufferDecoderGroup, BufferDecoderGroup, + bufferEncoders, Types.List) .addAnnotation(Deprecated.class) .addModifiers(PUBLIC) .addParameter(GrpcRouteExecutionStrategyFactory, strategyFactory, FINAL) @@ -606,16 +626,26 @@ private TypeSpec.Builder addServiceFactory(final State state, final TypeSpec.Bui // Add ServiceFactory constructors for blocking and async services with and without content codings and // execution strategy .addMethod(constructorBuilder() + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) + .addJavadoc(lineSeparator()) + .addJavadoc(JAVADOC_PARAM + service + " a service to handle incoming requests" + + lineSeparator()) .addModifiers(PUBLIC) .addParameter(state.serviceClass, service, FINAL) .addStatement("this(new $T().$L)", builderClass, serviceFactoryBuilderInitChain(state.serviceProto, false)) .build()) .addMethod(constructorBuilder() + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) + .addJavadoc(lineSeparator()) + .addJavadoc(JAVADOC_PARAM + service + " a service to handle incoming requests" + + lineSeparator()) + .addJavadoc(JAVADOC_PARAM + supportedMessageCodings + " the set of allowed encodings" + + lineSeparator()) .addJavadoc(JAVADOC_DEPRECATED + - " Use {@link $L#$L()}, {@link $L#$L($T)}, and {@link $L#$L($T)}." + lineSeparator(), + "Use {@link $L#$L()}, {@link $L#$L($T)}, and {@link $L#$L($T)}." + lineSeparator(), Builder, Builder, Builder, bufferDecoderGroup, BufferDecoderGroup, - Builder, bufferEncoders, BufferEncoderList) + Builder, bufferEncoders, Types.List) .addAnnotation(Deprecated.class) .addModifiers(PUBLIC) .addParameter(state.serviceClass, service, FINAL) @@ -624,6 +654,13 @@ private TypeSpec.Builder addServiceFactory(final State state, final TypeSpec.Bui serviceFactoryBuilderInitChain(state.serviceProto, false)) .build()) .addMethod(constructorBuilder() + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) + .addJavadoc(lineSeparator()) + .addJavadoc(JAVADOC_PARAM + service + " a service to handle incoming requests" + + lineSeparator()) + .addJavadoc(JAVADOC_PARAM + strategyFactory + + " a factory that creates an execution strategy for different {@link $L#id() id}s" + + lineSeparator(), RouteExecutionStrategy) .addModifiers(PUBLIC) .addParameter(state.serviceClass, service, FINAL) .addParameter(GrpcRouteExecutionStrategyFactory, strategyFactory, FINAL) @@ -631,10 +668,19 @@ private TypeSpec.Builder addServiceFactory(final State state, final TypeSpec.Bui serviceFactoryBuilderInitChain(state.serviceProto, false)) .build()) .addMethod(constructorBuilder() + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) + .addJavadoc(lineSeparator()) + .addJavadoc(JAVADOC_PARAM + service + " a service to handle incoming requests" + + lineSeparator()) + .addJavadoc(JAVADOC_PARAM + strategyFactory + + " a factory that creates an execution strategy for different {@link $L#id() id}s" + + lineSeparator(), RouteExecutionStrategy) + .addJavadoc(JAVADOC_PARAM + supportedMessageCodings + " the set of allowed encodings" + + lineSeparator()) .addJavadoc(JAVADOC_DEPRECATED + - " Use {@link $L#$L($T)}, {@link $L#$L($T)}, and {@link $L#$L($T)}." + lineSeparator(), - Builder, Builder, GrpcRouteExecutionStrategyFactory, Builder, bufferDecoderGroup, - BufferDecoderGroup, Builder, bufferEncoders, BufferEncoderList) + "Use {@link $L#$L($T)}, {@link $L#$L($T)}, and {@link $L#$L($T)}." + lineSeparator(), + Builder, Builder, RouteExecutionStrategyFactory, Builder, bufferDecoderGroup, + BufferDecoderGroup, Builder, bufferEncoders, Types.List) .addAnnotation(Deprecated.class) .addModifiers(PUBLIC) .addParameter(state.serviceClass, service, FINAL) @@ -644,16 +690,26 @@ private TypeSpec.Builder addServiceFactory(final State state, final TypeSpec.Bui supportedMessageCodings, serviceFactoryBuilderInitChain(state.serviceProto, false)) .build()) .addMethod(constructorBuilder() + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) + .addJavadoc(lineSeparator()) + .addJavadoc(JAVADOC_PARAM + service + " a service to handle incoming requests" + + lineSeparator()) .addModifiers(PUBLIC) .addParameter(state.blockingServiceClass, service, FINAL) .addStatement("this(new $T().$L)", builderClass, serviceFactoryBuilderInitChain(state.serviceProto, true)) .build()) .addMethod(constructorBuilder() + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) + .addJavadoc(lineSeparator()) + .addJavadoc(JAVADOC_PARAM + service + " a service to handle incoming requests" + + lineSeparator()) + .addJavadoc(JAVADOC_PARAM + supportedMessageCodings + " the set of allowed encodings" + + lineSeparator()) .addJavadoc(JAVADOC_DEPRECATED + - " Use {@link $L#$L()}, {@link $L#$L($T)}, and {@link $L#$L($T)}." + lineSeparator(), + "Use {@link $L#$L()}, {@link $L#$L($T)}, and {@link $L#$L($T)}." + lineSeparator(), Builder, Builder, Builder, bufferDecoderGroup, BufferDecoderGroup, - Builder, bufferEncoders, BufferEncoderList) + Builder, bufferEncoders, Types.List) .addAnnotation(Deprecated.class) .addModifiers(PUBLIC) .addParameter(state.blockingServiceClass, service, FINAL) @@ -662,6 +718,13 @@ supportedMessageCodings, serviceFactoryBuilderInitChain(state.serviceProto, fals serviceFactoryBuilderInitChain(state.serviceProto, true)) .build()) .addMethod(constructorBuilder() + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) + .addJavadoc(lineSeparator()) + .addJavadoc(JAVADOC_PARAM + service + " a service to handle incoming requests" + + lineSeparator()) + .addJavadoc(JAVADOC_PARAM + strategyFactory + + " a factory that creates an execution strategy for different {@link $L#id() id}s" + + lineSeparator(), RouteExecutionStrategy) .addModifiers(PUBLIC) .addParameter(state.blockingServiceClass, service, FINAL) .addParameter(GrpcRouteExecutionStrategyFactory, strategyFactory, FINAL) @@ -669,10 +732,19 @@ supportedMessageCodings, serviceFactoryBuilderInitChain(state.serviceProto, fals serviceFactoryBuilderInitChain(state.serviceProto, true)) .build()) .addMethod(constructorBuilder() + .addJavadoc(JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT) + .addJavadoc(lineSeparator()) + .addJavadoc(JAVADOC_PARAM + service + " a service to handle incoming requests" + + lineSeparator()) + .addJavadoc(JAVADOC_PARAM + strategyFactory + + " a factory that creates an execution strategy for different {@link $L#id() id}s" + + lineSeparator(), RouteExecutionStrategy) + .addJavadoc(JAVADOC_PARAM + supportedMessageCodings + " the set of allowed encodings" + + lineSeparator()) .addJavadoc(JAVADOC_DEPRECATED + - " Use {@link $L#$L($T)}, {@link $L#$L($T)}, and {@link $L#$L($T)}." + lineSeparator(), - Builder, Builder, GrpcRouteExecutionStrategyFactory, Builder, bufferDecoderGroup, - BufferDecoderGroup, Builder, bufferEncoders, BufferEncoderList) + "Use {@link $L#$L($T)}, {@link $L#$L($T)}, and {@link $L#$L($T)}." + lineSeparator(), + Builder, Builder, RouteExecutionStrategyFactory, Builder, bufferDecoderGroup, + BufferDecoderGroup, Builder, bufferEncoders, Types.List) .addAnnotation(Deprecated.class) .addModifiers(PUBLIC) .addParameter(state.blockingServiceClass, service, FINAL) @@ -702,7 +774,7 @@ private TypeSpec.Builder addClientMetadata(final State state, final TypeSpec.Bui final ClassName metaDataClassName = ClassName.bestGuess(name); final TypeSpec classSpec = classBuilder(name) - .addJavadoc(JAVADOC_DEPRECATED + " This class will be removed in the future in favor of direct " + + .addJavadoc(JAVADOC_DEPRECATED + "This class will be removed in the future in favor of direct " + "usage of {@link $T}. Deprecation of {@link $T#path()} renders this type unnecessary." + lineSeparator(), GrpcClientMetadata, GrpcClientMetadata) .addAnnotation(Deprecated.class) @@ -710,7 +782,7 @@ private TypeSpec.Builder addClientMetadata(final State state, final TypeSpec.Bui .superclass(DefaultGrpcClientMetadata) .addField(FieldSpec.builder(metaDataClassName, INSTANCE) .addJavadoc(JAVADOC_DEPRECATED + - " This class will be removed in the future in favor of direct usage of {@link $T}." + "This class will be removed in the future in favor of direct usage of {@link $T}." + lineSeparator(), GrpcClientMetadata) .addAnnotation(Deprecated.class) .addModifiers(PUBLIC, STATIC, FINAL) // redundant, default for interface field @@ -791,10 +863,10 @@ private TypeSpec.Builder addClientInterfaces(final State state, final TypeSpec.B ClassName inClass = messageTypesMap.get(clientMetaData.methodProto.getInputType()); b.addModifiers(ABSTRACT).addParameter(clientMetaData.className, metadata) .addAnnotation(Deprecated.class) - .addJavadoc(JAVADOC_DEPRECATED + " Use {@link #$L($T,$T)}." + lineSeparator(), + .addJavadoc(JAVADOC_DEPRECATED + "Use {@link #$L($T,$T)}." + lineSeparator(), methodName, GrpcClientMetadata, clientMetaData.methodProto.getClientStreaming() ? - ParameterizedTypeName.get(Publisher, inClass) : inClass); + Publisher : inClass); if (printJavaDocs) { extractJavaDocComments(state, methodIndex, b); b.addJavadoc(JAVADOC_PARAM + metadata + @@ -828,9 +900,9 @@ private TypeSpec.Builder addClientInterfaces(final State state, final TypeSpec.B ClassName inClass = messageTypesMap.get(clientMetaData.methodProto.getInputType()); b.addModifiers(ABSTRACT).addParameter(clientMetaData.className, metadata) .addAnnotation(Deprecated.class) - .addJavadoc(JAVADOC_DEPRECATED + " Use {@link #$L($T,$T)}." + lineSeparator(), + .addJavadoc(JAVADOC_DEPRECATED + "Use {@link #$L($T,$T)}." + lineSeparator(), methodName, GrpcClientMetadata, clientMetaData.methodProto.getClientStreaming() ? - ParameterizedTypeName.get(ClassName.get(Iterable.class), inClass) : inClass); + Types.Iterable : inClass); if (printJavaDocs) { extractJavaDocComments(state, methodIndex, b); b.addJavadoc(JAVADOC_PARAM + metadata + diff --git a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Types.java b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Types.java index 601bf760bb..78651df314 100644 --- a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Types.java +++ b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Types.java @@ -37,12 +37,15 @@ final class Types { private static final TypeName Wildcard = WildcardTypeName.subtypeOf(Object.class); static final ClassName List = ClassName.get("java.util", "List"); + static final ClassName Iterable = ClassName.get("java.lang", "Iterable"); static final ClassName Objects = ClassName.get("java.util", "Objects"); static final ClassName Collections = ClassName.get("java.util", "Collections"); static final ClassName Arrays = ClassName.get("java.util", "Arrays"); private static final ClassName Collection = ClassName.get("java.util", "Collection"); - private static final ClassName RouteExecutionStrategyFactory = + static final ClassName RouteExecutionStrategy = + bestGuess(routerApiPkg + ".RouteExecutionStrategy"); + static final ClassName RouteExecutionStrategyFactory = bestGuess(routerApiPkg + ".RouteExecutionStrategyFactory"); static final ClassName BlockingIterable = bestGuess(concurrentPkg + ".BlockingIterable"); diff --git a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Words.java b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Words.java index f80d21592f..618c6f1afd 100644 --- a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Words.java +++ b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Words.java @@ -15,6 +15,8 @@ */ package io.servicetalk.grpc.protoc; +import static java.lang.System.lineSeparator; + final class Words { static final String bind = "bind"; static final String builder = "builder"; @@ -63,10 +65,11 @@ final class Words { static final String RPC_PATH = "PATH"; static final String COMMENT_PRE_TAG = "
";
     static final String COMMENT_POST_TAG = "
"; + static final String JAVADOC_CONSTRUCTOR_DEFAULT_STATEMENT = "Create a new instance." + lineSeparator(); static final String JAVADOC_PARAM = "@param "; static final String JAVADOC_RETURN = "@return "; static final String JAVADOC_THROWS = "@throws "; - static final String JAVADOC_DEPRECATED = "@deprecated"; + static final String JAVADOC_DEPRECATED = "@deprecated "; static final String ASYNC_METHOD_DESCRIPTORS = "ASYNC_METHOD_DESCRIPTORS"; static final String BLOCKING_METHOD_DESCRIPTORS = "BLOCKING_METHOD_DESCRIPTORS";