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

Generated services are not backwards compatible #168

Open
njooma opened this issue Jan 13, 2023 · 2 comments
Open

Generated services are not backwards compatible #168

njooma opened this issue Jan 13, 2023 · 2 comments

Comments

@njooma
Copy link

njooma commented Jan 13, 2023

Additive only changes to protobuf files will result in generated code that is not backwards compatible.

If I add RPC methods to my protobuf files, compiling those protobuf files with grpclib will result in the abstract ...ServiceBase class adding abstract methods for those new RPC methods. Then, my concrete implementations of that service need to be updated with the new methods (even if just to raise a NotImplementedError) to avoid getting a TypeError for not implementing all methods of the abstract class.

This has become an issue because I'm implementing RPC methods defined by an upstream library so I can be compatible with the users of that library. However, I can't/choose not to always implement all the new methods as the upstream maintainers publish them -- I want to pick and choose. But as soon as I recompile the library's protobuf files with grpclib, I now have to update all my implemented services with the new methods.

To solve this, I propose updating the grpclib protoc plugin to generate an additional class Unimplemented...ServiceBase that is a concrete implementation of the abstract ...ServiceBase class, where all methods have default implementations to raise a NotImplementedError. This way, current users of grpclib who rely on receiving a TypeError to know when they haven't fulfilled the abstract class requirements will be able to keep their workflow, and any user who wants to be automatically backwards compatible can instead inherit from the new Unimplemented...ServiceBase and override functions as they implement them.

Another option would be to make the ...ServiceBase class concrete and have default implementations that simply raise NotImplementedErrors (this is what the main grpc library does). But this could break existing workflows.

I can also make a pull request with the changes if you decide that this should be implemented.

@vmagamedov
Copy link
Owner

Is it possible to make that upstream library a dependency of your library, so you can specify concrete versions range you support in your library?

I also was able to come up with this example:

def with_optional_methods(cls: ABCMeta) -> type:
    async def not_implemented(self: Any, stream: Any) -> None:
        raise GRPCError(Status.UNIMPLEMENTED)
    return type(cls.__name__, (cls,), {name: not_implemented
                                       for name in cls.__abstractmethods__})


@with_optional_methods
class Greeter(GreeterBase):

    async def SayHello2(self, stream: Stream[HelloRequest, HelloReply]) -> None:
              ^^^^^^^^^ redacted
        request = await stream.recv_message()
        assert request is not None
        message = f'Hello, {request.name}!'
        await stream.send_message(HelloReply(message=message))


async def main(*, host: str = '127.0.0.1', port: int = 50051) -> None:
    server = Server([Greeter()])  # type: ignore[abstract]
                                  ^^^^^^^^^^^^^^^^^^^^^^^^ added

It is maybe not very beautiful but works in runtime and in mypy. Would this be sufficient in your case?

@njooma
Copy link
Author

njooma commented Jan 17, 2023

The upstream library only publishes protobufs, and they ascribe to the philosophy that additive changes are not breaking since protobufs are supposed to be backwards and forwards compatible. So pinning concrete versions is not as helpful, especially since I sometimes want to support some new protobufs, but not others.

Your example solution works great! Thanks for that.

The reason I suggested creating a specific Unimplemented class is because that's how the official gRPC library functions (it actually only creates one class, and has all the methods default to raising NotImplementedErrors). So for now your solution works, but is there a reason not to implement the additional generated class?

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

No branches or pull requests

2 participants