[grpc-protoc] Add an option to generate default service methods

[mgodave]

Motivation

When evolving a service definition that has multiple implementations it is ideal not to break these implementations. Therefore it would be nice if the protoc generator was able to generate default implementations on the service interface to ensure implementations always conform.

Changes

Add an option, defaultServiceMethods to the grpc-protoc stub compiler to generate these default implementation. The result is that service interfaces will provide a default, throwing, implementation of all service RPC interfaces that they implement.

test_service.proto: https://gist.github.com/mgodave/d9064c67da9f365de70ef7526c9eea72

[idelpivnitskiy]

New findings:

[bryce-anderson]

Take my review with a grain of salt, but it seems good to me.

[idelpivnitskiy]

skipDeprecated=true case works perfect for defaultServiceMethods=false|true 👍

skipDeprecated=false has a problem: currently it overrides non-deprecated methods that already have a default impl (calling their deprecated variant). The deprecated methods don't. In result, users will still have compilation issues when they add a new "response streaming" method. Expectation is that we add default impl override only for those methods that don't already have a default impl in their parent interface.

Way to reproduce:

public final class BlockingProtocOptionsServer {
    public static void main(String[] args) throws Exception {
        GrpcServers.forPort(8080)
                .listenAndAwait(new TesterSt.BlockingTesterService() { /* noop */ })
                .awaitShutdown();
    }
}

Result:

Class 'Anonymous class derived from BlockingTesterService' must implement abstract method 'testBiDiStream(GrpcServiceContext, BlockingIterable<TestRequest>, GrpcPayloadWriter<TestResponse>)' in 'BlockingTestBiDiStreamRpc'

If I change BlockingTesterService to TesterService, the compilation error goes away. It should be the same behavior for BlockingTesterService.

Current output:

    public interface BlockingTesterService extends GrpcBindableService<TesterService>, BlockingTestRpc, BlockingTestBiDiStreamRpc, BlockingTestResponseStreamRpc, BlockingTestRequestStreamRpc {
        /**
         * Makes a {@link ServiceFactory} bound to this instance implementing {@link BlockingTesterService}
         */
        @Override
        default ServiceFactory bindService() {
            return new ServiceFactory(this);
        }

        @Override
        default Collection<MethodDescriptor<?, ?>> methodDescriptors() {
            return BLOCKING_METHOD_DESCRIPTORS;
        }

        @Override
        default TestResponse test(GrpcServiceContext ctx, TestRequest request) throws Exception {
            throw new GrpcStatusException(new GrpcStatus(GrpcStatusCode.UNIMPLEMENTED, "Method grpc.netty.Tester/test is unimplemented"));
        }

        @Override
        default void testBiDiStream(GrpcServiceContext ctx, BlockingIterable<TestRequest> request,
                BlockingStreamingGrpcServerResponse<TestResponse> response) throws Exception {
            BlockingTestBiDiStreamRpc.super.testBiDiStream(ctx, request, response);
        }

        @Override
        default void testResponseStream(GrpcServiceContext ctx, TestRequest request,
                BlockingStreamingGrpcServerResponse<TestResponse> response) throws Exception {
            BlockingTestResponseStreamRpc.super.testResponseStream(ctx, request, response);
        }

        @Override
        default TestResponse testRequestStream(GrpcServiceContext ctx,
                BlockingIterable<TestRequest> request) throws Exception {
            throw new GrpcStatusException(new GrpcStatus(GrpcStatusCode.UNIMPLEMENTED, "Method grpc.netty.Tester/testRequestStream is unimplemented"));
        }
    }

Expected output:

    public interface BlockingTesterService extends GrpcBindableService<TesterService>, BlockingTestRpc, BlockingTestBiDiStreamRpc, BlockingTestResponseStreamRpc, BlockingTestRequestStreamRpc {
        /**
         * Makes a {@link ServiceFactory} bound to this instance implementing {@link BlockingTesterService}
         */
        @Override
        default ServiceFactory bindService() {
            return new ServiceFactory(this);
        }

        @Override
        default Collection<MethodDescriptor<?, ?>> methodDescriptors() {
            return BLOCKING_METHOD_DESCRIPTORS;
        }

        @Override
        default TestResponse test(GrpcServiceContext ctx, TestRequest request) throws Exception {
            throw new GrpcStatusException(new GrpcStatus(GrpcStatusCode.UNIMPLEMENTED, "Method grpc.netty.Tester/test is unimplemented"));
        }

        @Override
        @Deprecated
        default void testBiDiStream(GrpcServiceContext ctx, BlockingIterable<TestRequest> request,
                                    GrpcPayloadWriter<TestResponse> responseWriter) throws Exception {
            throw new GrpcStatusException(new GrpcStatus(GrpcStatusCode.UNIMPLEMENTED, "Method grpc.netty.Tester/testBiDiStream is unimplemented"));
        }

        @Override
        @Deprecated
        default void testResponseStream(GrpcServiceContext ctx, TestRequest request,
                                        GrpcPayloadWriter<TestResponse> responseWriter) throws Exception {
            throw new GrpcStatusException(new GrpcStatus(GrpcStatusCode.UNIMPLEMENTED, "Method grpc.netty.Tester/testResponseStream is unimplemented"));
        }

        @Override
        default TestResponse testRequestStream(GrpcServiceContext ctx,
                BlockingIterable<TestRequest> request) throws Exception {
            throw new GrpcStatusException(new GrpcStatus(GrpcStatusCode.UNIMPLEMENTED, "Method grpc.netty.Tester/testRequestStream is unimplemented"));
        }
    }

[mgodave]

I see what is going on here. I need to somehow grab a hold of the super interface specs so I can query them for any abstract methods that I can then implement... I have no idea how to do this at the moment...

[idelpivnitskiy]

As replied in another thread, it will be ok to implement all *Rpc methods and don't bother with skipping those that already have a default impl.

[idelpivnitskiy]

Figured it 🎉
I reverted your last 2 commits to go back to the original approach you had and then made the following changes:

Index: servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
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
--- a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java	(revision af1dfe3dc11881a0cf5cd0b18f952d8cda2c5bad)
+++ b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java	(date 1732316010356)
@@ -1650,49 +1650,33 @@
             final List<RpcInterface> interfaces = state.serviceRpcInterfaces.stream()
                     .filter(intf -> intf.blocking == blocking)
                     .collect(toList());
-            for (int i = 0; i < interfaces.size(); ++i) {
-                final RpcInterface rpcInterface = interfaces.get(i);
+            for (final RpcInterface rpcInterface : interfaces) {
                 final MethodDescriptorProto methodProto = rpcInterface.methodProto;
                 final ClassName inClass = messageTypesMap.get(methodProto.getInputType());
                 final ClassName outClass = messageTypesMap.get(methodProto.getOutputType());
                 final String methodName = sanitizeIdentifier(methodProto.getName(), true);
                 final String methodPath = context.methodPath(state.serviceProto, methodProto).substring(1);
-                final int methodIndex = i;
                 final MethodSpec methodSpec = newRpcMethodSpec(inClass, outClass, methodName,
                         methodProto.getClientStreaming(),
                         methodProto.getServerStreaming(),
-                        !blocking ? EnumSet.of(INTERFACE) : EnumSet.of(INTERFACE, BLOCKING, SERVER_RESPONSE),
-                        printJavaDocs, (__, spec) -> {
+                        !blocking ? EnumSet.of(INTERFACE) : (skipDeprecated ?
+                                EnumSet.of(INTERFACE, BLOCKING, SERVER_RESPONSE) : EnumSet.of(INTERFACE, BLOCKING)),
+                        false, (__, spec) -> {
+                            spec.addAnnotation(Override.class);
+                            spec.addModifiers(DEFAULT).addParameter(GrpcServiceContext, ctx);
                             final String errorMessage = "\"Method " + methodPath + " is unimplemented\"";
-                            spec.addModifiers(DEFAULT).addParameter(GrpcServiceContext, ctx);
-                            spec.addAnnotation(Override.class);
                             if (!blocking) {
                                 final ClassName returnType = methodProto.getServerStreaming() ? Publisher : Single;
                                 spec.addStatement("return $T.failed(new $T(new $T($T.UNIMPLEMENTED, $L)))",
                                         returnType, GrpcStatusException, GrpcStatus, GrpcStatusCode, errorMessage);
                             } else {
-                                if (!skipDeprecated && methodProto.getServerStreaming()) {
-                                    spec.addStatement("$T.super.$L(ctx, request, response)",
-                                            rpcInterface.className, methodName);
-                                } else {
-                                    spec.addStatement("throw new $T(new $T($T.UNIMPLEMENTED, $L))",
-                                            GrpcStatusException, GrpcStatus, GrpcStatusCode, errorMessage);
-                                }
+                                spec.addStatement("throw new $T(new $T($T.UNIMPLEMENTED, $L))",
+                                        GrpcStatusException, GrpcStatus, GrpcStatusCode, errorMessage);
                             }
-                            if (printJavaDocs) {
-                                extractJavaDocComments(state, methodIndex, spec);
-                                spec.addJavadoc(JAVADOC_PARAM + ctx +
-                                        " context associated with this service and request." + lineSeparator());
-                            }
                             return spec;
                         });
 
-                final boolean isDeprecated = methodSpec.annotations.contains(
-                        AnnotationSpec.builder(Deprecated.class).build());
-
-                if (!isDeprecated || !skipDeprecated) {
-                    interfaceSpecBuilder.addMethod(methodSpec);
-                }
+                interfaceSpecBuilder.addMethod(methodSpec);
             }
         }
 

Now all default methods look as expected. We just need to add tests to cover them all.

[idelpivnitskiy]

We should always throw UNIMPLEMENTED exception instead of calling super.

Instead of

        @Override
        default void testBiDiStream(GrpcServiceContext ctx, BlockingIterable<TestRequest> request,
                BlockingStreamingGrpcServerResponse<TestResponse> response) throws Exception {
            BlockingTestBiDiStreamRpc.super.testBiDiStream(ctx, request, response);
        }

        @Override
        default void testResponseStream(GrpcServiceContext ctx, TestRequest request,
                BlockingStreamingGrpcServerResponse<TestResponse> response) throws Exception {
            BlockingTestResponseStreamRpc.super.testResponseStream(ctx, request, response);
        }

We should generate:

        @Override
        @Deprecated
        default void testBiDiStream(GrpcServiceContext ctx, BlockingIterable<TestRequest> request,
                                    GrpcPayloadWriter<TestResponse> responseWriter) throws Exception {
            throw new GrpcStatusException(new GrpcStatus(GrpcStatusCode.UNIMPLEMENTED, "Method grpc.netty.Tester/testBiDiStream is unimplemented"));
        }

        @Override
        @Deprecated
        default void testResponseStream(GrpcServiceContext ctx, TestRequest request,
                                        GrpcPayloadWriter<TestResponse> responseWriter) throws Exception {
            throw new GrpcStatusException(new GrpcStatus(GrpcStatusCode.UNIMPLEMENTED, "Method grpc.netty.Tester/testResponseStream is unimplemented"));
        }

[mgodave]

I would argue that this is not broken, it works as it should with the combination of flags. I definitely see your point about not redefining a default method when the base interface already has one. That is potentially easier said than done with javapoet.

[idelpivnitskiy]

It will be ok to override both variants (deprecated and not) of testBiDiStream and testResponseStream and always throw UNIMPLEMENTED (no call to super). Then users will be able to override only non-deprecated variant and won't see any compilation problems asking to implement the deprecated one.

[idelpivnitskiy]

Instead of adding a new unimplementedServerCall in the proto, consider adding service parameter for serviceTalkServerBlocking, serviceTalkServer, and grpcJavaServer. There you can pass an unimplemented service like new BlockingCompatService() { }. That way you can validate all 4 API flavors in every test method. Problems highlighted in above messages will be automatically validated.

[mgodave]

Always throwing will still break the default behavior, especially in cases where the user has implemented the GrpcResponseWriter variant.

[idelpivnitskiy]

Looks like a reply to a wrong thread, but you are right. We should still skip those that already have a default.

[idelpivnitskiy]

For grpcJavaToServiceTalkUnimplementedService and serviceTalkToServiceTalkUnimplementedService consider making them @ParameterizedTest to also run with serviceTalkServer(..., new CompatService() { })