feat(tonic): expose Interceptor trait

[yotamofek]

Motivation

Found myself wanting to be able to explicitly type a wrapped client (or server, for that matter), i.e.:

struct ClientWrapper {
    client: Client<InterceptedService<Channel, MyInterceptor>>
}

This is currently impossible (until impling the Fn traits becomes stable, which will take a while).

Solution

Expose an Interceptor trait that is implemented for the function signatures of interceptors, but can also be implemented by downstream crates on custom types.

I think a semver change can be avoided by not renaming interceptor_fn -> interceptor_layer, but IMHO the name change is worthwhile.

[davidpdrsn]

I'm thinking going straight to a tower Service would be better for making a nameable type. Not sure the mental overhead of a new Service-like trait is worth it.

Perhaps a function pointer like InterceptedService<S, fn(...) -> ...> could work as well.

[yotamofek]

I did that, but found myself having to copy+paste this part of tonic, which is less than ideal. BTW, since regular functions (with the right sig) implement this trait anyways, I don't really see it as any more mental overhead than already exists.

(Thanks for the quick response, BTW!)

[davidpdrsn]

I don't really see it as any more mental overhead than already exists.

You're right that it's just one more trait but in my experience tower/services/middleware is already hard to grasp for some users. This trait does introduce another step you have to wrap your head around.

I think if you use the types from the http crate you shouldn't have to duplicate anything. There is an example here https://github.com/hyperium/tonic/blob/master/examples/src/tower/server.rs

[yotamofek]

I think if you use the types from the http crate you shouldn't have to duplicate anything. There is an example here https://github.com/hyperium/tonic/blob/master/examples/src/tower/server.rs

Correct me if I'm wrong, but I tried that and there's no way to access the MetadataMap from the hyper::Request param passed in, without doing the to_parts->transform->for_parts thing that the interceptor does.

[davidpdrsn]

The headers on the request is actually the same as the metadata. Tonic maps between the types internally. Same goes for the extensions.

[davidpdrsn]

And you can use Status::to_http to reject a request with a status code.

[yotamofek]

I got around my original issue by implementing the tower::Service trait directly, like you suggested. I think still it's still worthwhile to allow implementing named Interceptors. Their original purpose, if I understood correctly, was to allow tonic's downstream consumers to implement simple middleware without the having to wrap their heads around the tower ecosystem (which, I agree with you, requires a lot of head-wrapping as is).

My opinion is that the small addition this PR adds to the complexity factor of understanding Interceptors, is worth the trade-off if it allows even a few more users (and I reckon that more people will be hitting this issue soon) to not have to "learn" tower.

But I completely understand your reasoning and please feel free to close this pull request if I you're still not convinced ;)

[davidpdrsn]

@LucioFranco what do you think?

[yotamofek]

For what it's worth, this is the code that I could've avoided writing if this PR was accepted: https://github.com/etcdv3/etcd-client/pull/21/files#diff-212eed958dda431dc3bf15be7424ef556201e53143bbe8b19bb80e19e22e5cd9

It's not a disaster, but I generally will always prefer to delete existing code rather than adding new code :)

And regardless - thanks for all your hard work!

[davidpdrsn]

You might be able to use SetRequestHeader from tower-http do to that as well https://docs.rs/tower-http/0.1.1/tower_http/set_header/request/index.html at least it looks quite similar.

[LucioFranco]

I'm not opposed to exposing a trait, we can always remove it if we find its wrong down the line and tonic is still a ways away from 1.0

[yotamofek]

@LucioFranco rebased to resolve conflicts and force pushed

[davidpdrsn]

Alright cool. Then lets move forward with this 😊

Given that we recently shipped a breaking release (0.5) I doubt we're going to make another breaking release anytime soon. So it would be cool to find a way to add this without breaking changes. @yotamofek what do you think?

[yotamofek]

Given that we recently shipped a breaking release (0.5) I doubt we're going to make another breaking release anytime soon. So it would be cool to find a way to add this without breaking changes. @yotamofek what do you think?

If I rename interceptor_layer back to interceptor_fn, I think it should make this a non-breaking change. Because any FnMut that satisfied the original bound will also satisfy F: tonic::service::Interceptor, this means that downstream crates shouldn't* have to change anything in order to build successfully against a version of tonic with this PR merged it.

*unless there's some very contrived scenario which I can't think of that will cause breakage?

[yotamofek]

Another small thought:

The whole reason I opened this PR was to allow myself (as a downstream user of tonic) to upgrade to 0.5 with less breakage than I ended up having to adapt to. I think it's safe to assume that the removal of the Interceptor struct that was present in 0.4 affected others, too. So maybe shipping a new breaking version (i.e. 0.6) that is actually somewhat less "broken" compared to 0.4 isn't so bad?

I can say that as a consumer, I would probably not mind jumping two minor versions at a time if it meant an easier upgrade path. But this is really just my $0.02, and I'll be happy to try to make this PR be consistent with the 0.5 API surface.

[davidpdrsn]

I think we should at least explore what a non-breaking version of what this might look like before we decide to ship another breaking release.

I don't think shipping two breaking release within less than two weeks is ideal, unless something is broken which I don't think is the case here 🤷

[yotamofek]

@davidpdrsn
This should now be, to the best of my knowledge, a non-breaking change. 😁

[davidpdrsn]

I'm going on vacation for a few weeks. Will check this out when I'm done. @LucioFranco feel free to take over if you have the time.

[yotamofek]

Enjoy your time off, thanks for all the effort so far!

[LucioFranco]

I think this should work, I can't find a situation where this would be a breaking change now that you've added the deprecated.