Add support for server side interceptors

[aschleck]

(Hopefully) fixes #527

• I spent a bunch of time trying to minimize the couple of casts, I think ultimately since MethodImpl is polymorphic it's inevitable to need this type of casting.
• I don't love that interceptors are a property of the ConnectRouterOptions, originally I was going to put them on the ConnectRouter. However they cannot be on ConnectRouter because it immediately sets up registered methods and interceptors have to be bound inside createMethodImplSpec (to be explicit, if you register a method and then add an interceptor afterwards the method wouldn't be intercepted.) It would be a larger and relatively pointless refactor to make it all lazy. Putting it on ConnectRouterOptions just means that all interceptors will be set prior to any methods being registered.

Thank you for open sourcing this fantastic set of libraries!

[CLAassistant]

All committers have signed the CLA.

[timostamm]

Whoa, thank you @aschleck 🤩

Right now, we're on a freeze, just about to cut a stable version 1 of connect-es. While we cannot get this merged right away, we definitely appreciate the contribution. We'll want to land this in a feature release post v1, ideally in combination for a solution for #586 (context information is closely related to interceptors), which could be added as a follow-up.

[aschleck]

Thank you for the quick reply @timostamm! It's a bummer to hear I missed the window but totally understand. Provided you all like this code, do you have a sense of what timeline this might be able to get released on? I'm pushing my company to totally adopt this framework because it's obviously the correct choice, but interceptors are a big part of making it nice :)

[srikrsna-buf]

Hey @aschleck! Apologies for the delay and thank you for this.

I am not sure whether you looked at client interceptors, but one nice property they have is that the signature is common for all method kinds. This is a useful property because interceptors usually are used to add generic functionality like (authentication, logging etc...) and having a single signature makes it easier to implement such interceptors. For the use cases where they only target a particular method kind they can always check for that and skip the rest.

Could you modify the design to something that has a common signature? Before you rewrite this (as it is a lot of work) it'd be great if you could write down the design and we can take it from there. Thank you!

[aschleck]

Hi @srikrsna-buf, thank you for taking a look at this. I don't really have time for what you're asking and it seems like a matter of taste (trading off safety for convenience) so I'm going to have to decline, but appreciate this library and wish you folks the best of luck. Please feel free to close this.

If I were to do something generic, what I would personally lean towards is making a createFilterInterceptor(fn: (context: HandlerContext) => boolean) function that returns the expanded interceptor. Then you can leave this design as-is, and ensures you don't do crazy things like return unary responses for streaming RPCs due to a lack of type safety, but also lets you make the common case easy.

[aschleck]

Actually now that I think about it, you can get what you want (ease of use) with a helper method (next: AnyFn, context: HandlerContext) => AnyFn that wraps what I've done if you start with my approach. But you can't get what I want (type safety in the core of connect-es) if you start with your approach.

[srikrsna-buf]

Hi @srikrsna-buf, thank you for taking a look at this. I don't really have time for what you're asking

Thank you for taking the time to respond, I understand.

it seems like a matter of taste (trading off safety for convenience)

Just to be clear it will not be any less type-safe, the client interceptors implementation has to check for the stream property of request/response to access the body. Other fields like headers are always accessible.

There is a possibility of returning the wrong response but at that point, you are explicitly setting the stream property and a bunch of others and not relying on the response returned from the handler, which is very unusual and almost all such cases of short-circuiting the request will do so by throwing an error rather than returning a custom response.

I'll close this, I'll be working on the interceptors next and I'll send a PR once #835 lands. I'm hopeful you'll like the design :)