Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to the protoc code generator to avoid generating deprecated code #3089

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mgodave
Copy link
Contributor

@mgodave mgodave commented Nov 6, 2024

Motivation

The generated ServiceTalk gRPC stubs make use of deprecated calls. This causes a problem when -Werror is used to build the code since it will automatically fail the build. We should allow users who have already migrated their code to prevent the protoc compiler from generating and using deprecated references.

Modifications

Add an option, skipDeprecated, as part of the protoc compiler configuration, to tell the generator to leave out deprecated references.

Result

Example: Greeter.java before/after https://gist.github.com/mgodave/9926d62e4bf9c2d3192d2dbf2299528b

Remove

  • initSerializationProvider, reason: ContentCodec is deprecated
  • isSupportedMessageCodingsEmpty, reason ContentCodec is deprecated
  • Deprecated ServiceFactory constructors
  • ServiceFactory::Builder references to ContentCodec
  • Generated client metadata and associated methods, reason: deprecated

Testing

Manually tested:

  • Add -Werror -Xlint:deprecation and observe the code that makes this compilation fail
  • Ensure that the deprecated calls are indeed removed and that re-application of the above options does not fail the build

@mgodave mgodave marked this pull request as ready for review November 7, 2024 15:10
Copy link
Contributor

@bryce-anderson bryce-anderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Token approval: this looks fine to me but please wait for a more experienced reviewer before merging.

How was it tested? I presume manually, but it would be good to document it in the PR description.

@@ -91,6 +91,28 @@ public final class Main {
* }</pre>
*/
private static final String PRINT_JAVA_DOCS_OPTION = "javaDocs";
/**
* Supports an option to disable generating calls to deprecated libraries.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Supports an option to disable generating calls to deprecated libraries.
* Supports an option to disable generating deprecated API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few other notes regarding docs:

  1. Consider adding a note why this option is useful and what is the risk. Something like:
     * <p>
     * This option can be useful for users who have their javac configured to treat usage of deprecated code as error.
     * However, it may cause a risk of API breaking changes when users upgrade ServiceTalk version and re-generate code
     * from protos.

^ feel free to reword or provide more details.

  1. Please document the new option in servicetalk-grpc-protoc/README.adoc. Would be great to clarify there what is the default value for all existing options.
  2. Consider demonstrating it in servicetalk-examples/grpc/protoc-options, in build.gradle and README.adoc.


Generator(final GenerationContext context, final Map<String, ClassName> messageTypesMap,
final boolean printJavaDocs, SourceCodeInfo sourceCodeInfo) {
final boolean printJavaDocs, final boolean skipDeprecated, SourceCodeInfo sourceCodeInfo) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall approach lgtm, but I added new option to our protoc-options example, run ./gradlew :servicetalk-examples:grpc:servicetalk-examples-grpc-protoc-options:build, and this is what I found:

  1. Client interfaces are empty now:

before:

    public interface GreeterClient extends GrpcClient<BlockingGreeterClient> {
        /**
         * <pre>
         *  Sends a greeting
         * </pre>
         *
         * @param request the request to send to the server.
         * @return a {@link Single} which completes when the response is received from the server.
         */
        Single<HelloReply> sayHello(HelloRequest request);

        /**
         * <pre>
         *  Sends a greeting
         * </pre>
         *
         * @deprecated Use {@link #sayHello(GrpcClientMetadata,HelloRequest)}.
         * @param metadata the metadata associated with this client call.
         * @param request the request to send to the server.
         * @return a {@link Single} which completes when the response is received from the server.
         */
        @Deprecated
        default Single<HelloReply> sayHello(SayHelloMetadata metadata, HelloRequest request) {
            return Single.failed(new UnsupportedOperationException("This method is not implemented by " + getClass() + ". Consider migrating to an alternative method or implement this method if it's required temporarily."));
        }

        /**
         * <pre>
         *  Sends a greeting
         * </pre>
         *
         * @param metadata the metadata associated with this client call.
         * @param request the request to send to the server.
         * @return a {@link Single} which completes when the response is received from the server.
         */
        default Single<HelloReply> sayHello(GrpcClientMetadata metadata, HelloRequest request) {
            return sayHello(new SayHelloMetadata(metadata), request);
        }

        @Override
        default BlockingGreeterClient asBlockingClient() {
            return new GreeterClientToBlockingGreeterClient(this);
        }
    }

    public interface BlockingGreeterClient extends BlockingGrpcClient<GreeterClient> {
        /**
         * <pre>
         *  Sends a greeting
         * </pre>
         *
         * @param request the request from the client.
         * @return the response from the server.
         * @throws Exception if an unexpected application error occurs.
         * @throws io.servicetalk.grpc.api.GrpcStatusException if an expected application exception occurs. Its contents will be serialized and propagated to the peer.
         */
        HelloReply sayHello(HelloRequest request) throws Exception;

        /**
         * <pre>
         *  Sends a greeting
         * </pre>
         *
         * @deprecated Use {@link #sayHello(GrpcClientMetadata,HelloRequest)}.
         * @param metadata the metadata associated with this client call.
         * @param request the request from the client.
         * @return the response from the server.
         * @throws Exception if an unexpected application error occurs.
         * @throws io.servicetalk.grpc.api.GrpcStatusException if an expected application exception occurs. Its contents will be serialized and propagated to the peer.
         */
        @Deprecated
        default HelloReply sayHello(SayHelloMetadata metadata, HelloRequest request) throws
                Exception {
            throw new UnsupportedOperationException("This method is not implemented by " + getClass() + ". Consider migrating to an alternative method or implement this method if it's required temporarily.");
        }

        /**
         * <pre>
         *  Sends a greeting
         * </pre>
         *
         * @param metadata the metadata associated with this client call.
         * @param request the request from the client.
         * @return the response from the server.
         * @throws Exception if an unexpected application error occurs.
         * @throws io.servicetalk.grpc.api.GrpcStatusException if an expected application exception occurs. Its contents will be serialized and propagated to the peer.
         */
        default HelloReply sayHello(GrpcClientMetadata metadata, HelloRequest request) throws
                Exception {
            return sayHello(new SayHelloMetadata(metadata), request);
        }
    }

after:

    public interface GreeterClient extends GrpcClient<BlockingGreeterClient> {
        @Override
        default BlockingGreeterClient asBlockingClient() {
            return new GreeterClientToBlockingGreeterClient(this);
        }
    }

    public interface BlockingGreeterClient extends BlockingGrpcClient<GreeterClient> {
    }

Expectation: they should have 2 methods: sayHello(HelloRequest) and sayHello(GrpcClientMetadata metadata, HelloRequest request). The last one should not have default implementation anymore, bcz that default was added for backward compatibility only.

  1. There are still some fields and methods with @Deprecated annotation:
    @FunctionalInterface
    public interface BlockingSayHelloRpc extends BlockingGrpcService {
        /**
         * @deprecated Use {@link #methodDescriptor}.
         */
        @Deprecated
        String PATH = "/helloworld.Greeter/SayHello";
            @Override
            @Deprecated
            protected GreeterServiceFromRoutes newServiceFromRoutes(
                    final GrpcRoutes.AllGrpcRoutes routes) {
                return new GreeterServiceFromRoutes(routes);
            }

Consider following the same approach for tests. However, existing helloworld.proto used by protoc-options has only one endpoint (aggregated). Only for the purpose of local testing, consider also copying test_service.proto into protoc-options. That will codegen all API variations we have. Would be better to use for skipDeprecated=false|true diff comparison.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants