Add optional default unimplemented stubs

[xanather]

Thought I should create an issue with corresponding PR I submitted.

Feature Request

Crates

tonic-build

Motivation

Re-implement/undo https://github.com/hyperium/tonic/issues/221 changes as optional feature.

This allows referencing gRPC models without version control which only permit backwards-compatible changes. It allows for setups provided by other programming language gRPC service generators that implement RPC's by default and return UNIMPLEMENTED error code for any methods which aren't explicitly handled.

Proposal

https://github.com/hyperium/tonic/pull/1344

Alternatives

I did try seeing if I can use some macro magic outside the library, but it becomes quite a hack.

[xanather]

Is it possible to get comment on this issue and associated PR? I can expand it via unit tests/any other feedback if the team is open to merging such a feature.

[xanather]

This feature has been discussed on discord. The following is a summary:

1. The linked PR https://github.com/hyperium/tonic/pull/1344 has been simplified to have a single boolean override on tonic_build::configure().generate_default_stubs() which defaults to false.
2. As part of writing tests an issue has been identified within the generated trait associated type (used for streaming requests). The type is purposefully abstract and cannot be used as part of a default implementation of a generated service. (The model generation build process will complain if new streaming based RPCs are added to a gRPC model even if generate_default_stubs is true).

There are several options available to resolve this.

Remove the associated type and elevate the stream so it's Pin and Boxed across the board (i.e. tonic::codegen::BoxStream).

Pros:

• Simplifies the generated traits. Explicit associate types no longer need to be implemented by the user.

Cons:

• Any pre-existing implemented RPC functions using a type which is not identical to BoxStream will break in the following release.
• This is a breaking change regardless when streaming RPC's are used. Any defined associated types as part of impl's will complain that it no longer exists.
• Best practice type erasure is removed.

Only force BoxStream when generate_default_stubs is true.

Pros:

• As this is a opt in new feature there is no breaking change.
• This can also be relaxed without breaking changes when https://github.com/rust-lang/rust/issues/29661 is implemented.

Cons:

• In this scenario the code base is using type erasure on one solution and forcing BoxStream on another solution. However in most cases switching the feature on and off should not be a breaking change as most users and example will be using a type equivalent to BoxStream (as all examples do).

The PR in question has gone with option two to avoid breaking changes.

See comments by @LucioFranco on older issue https://github.com/hyperium/tonic/issues/221 for more details about this problem. I think https://github.com/rust-lang/rust/issues/29661 won't be available for some time and option 2 is a good middle ground and avoids this issue re-appearing again.

[xanather]

@LucioFranco any chance this could get an eye? :)

[LucioFranco]

Sorry for the delay on my response @xanather.

This is a great write up and I think really the first option is the best but my concern is that I don't see this use case motivating enough to make this change. I would like to bundle a big breaking change to how we handle streams to include other ergonomic changes. I feel that maybe this could make sense once AFIT lands on stable which will force a breaking change to code gen'ed traits. I think if we bundled that together with other changes it would be a good candidate for one big breaking release change rather than a few.