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

Feature request: Provide a way to make sure that their code does not break when services add new rpcs. #2318

Closed
hsaliak opened this issue Sep 20, 2018 · 7 comments
Assignees
Labels
P2 Type: Feature New features or improvements in behavior

Comments

@hsaliak
Copy link

hsaliak commented Sep 20, 2018

When a gRPC service is defined in a proto a corresponding interface is generated. A user is expected to implement this interface.
When the proto is updated to add a new API method, the interface generated changes.
Any struct that implements that interface is no longer in conformance. Therefore, the code breaks.

I would expect the structs to be forward compatible.

This can potentially be solved by the code generating a dummy struct that implements the interface. Users can then embed that struct and only implement the methods they care about, effectively overriding the default methods.

@ahmetb
Copy link

ahmetb commented Sep 20, 2018

(Context: This came up in #2313.)

https://github.com/grpc/grpc-go/tree/master/health/grpc_health_v1 package just made a change (because health.proto in grpc/grpc was updated to add a new rpc).

grpc_health_v1 package in grpc-go v1.14.0 had:

type HealthServer interface {
	Check(context.Context, *HealthCheckRequest) (*HealthCheckResponse, error)
}

so I implemented this interface in my code.

Then I updated my grpc-go dependency to v1.15.0 which isn't supposed to break (per semver). But v1.15.0 introduced a new method:

type HealthServer interface {
	Check(context.Context, *HealthCheckRequest) (*HealthCheckResponse, error)
	Watch(*HealthCheckRequest, Health_WatchServer) error
}

as expected, having not implementing this Watch method yet, this broke all my programs using grpc-go.

@dfawley
Copy link
Member

dfawley commented Sep 20, 2018

Your proposal works, and might be the best short-term solution.

Two downsides: 1. compile-time type safety is effectively lost (if the user typos a method name, everything still compiles), and 2. we can't force users to do this, and users that don't will break when new RPCs are added to services.

An alternative would be to support registering methods one-by-one and recommend using that instead of service-level registration. The implementation of pb.RegisterFooService could optionally be changed to accept an interface{} and register the methods that are supported by the parameter (this obviously also loses type safety), or we could still provide the unimplemented service as above.

@dfawley dfawley added P1 Type: Feature New features or improvements in behavior labels Sep 27, 2018
@dsymonds
Copy link
Contributor

dsymonds commented Oct 8, 2018

The solution inside Google is that the stubby codegen creates an UnimplementedFooService concrete type that implements the interface with methods that return an unimplemented error. Servers that wish to avoid breaking when the Foo service is extended can embed that type in their own implementation, and new methods will thus transparently work (by returning that unimplemented error). That's effectively what the OP suggests, and it seems to work well enough in practice.

@dfawley dfawley added P2 and removed P1 labels Nov 5, 2018
@prannayk
Copy link
Contributor

If this is still open, I would like to get assigned to it. @dsymonds solution seems doable.

@dfawley
Copy link
Member

dfawley commented Jan 14, 2019

@prannayk - Sounds good to me, thanks! The grpc code gen lives in the proto repo - let me know if you need any help with that, and please cc me on any PRs you send there.

@srikrsna
Copy link

I've made a separate proto plugin that solves this: https://github.com/srikrsna/protoc-gen-defaults

@dfawley
Copy link
Member

dfawley commented Mar 28, 2019

Fixed by golang/protobuf#785

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P2 Type: Feature New features or improvements in behavior
Projects
None yet
Development

No branches or pull requests

6 participants