Allow adding ServerInterceptors to specific services and methods

[gjcairo]

Motivation

We want to allow users to customise the RPCs a registered interceptor should apply to on the server:

• Intercept all requests
• Intercept requests only meant for specific services
• Intercept requests only meant for specific methods

Modifications

This PR adds a new ServerInterceptorPipelineOperation type that allows users to specify what the target of the interceptor should be.
Existing APIs accepting [any ServerInterceptor] have been kept, but new initialisers taking [ServerInterceptorPipelineOperation] instead have been added.

Result

Users can have more control over to which requests interceptors are applied.

[glbrntt]

Thanks Gus – I think this is broadly the right approach and I like how the user has full control over the ordering of global/service/method specific interceptors. I have a few pieces of higher level feedback:

• I don't think we should propagate the ServerInterceptorTarget right down into the server executor. It couples the surface API (i.e. configuration) with the internals more than is necessary, so rather than pushing that all the way down I think the server should pre-compute the array of interceptors for each method and pass that down to the executor (instead of the array of targets). This also means we can avoid working out which interceptors to apply when each interceptor is called (which is potentially quite expensive).
• I think there's value in making the common case (interceptors apply to all services) as easy as it currently is, so the server should likely have an overload for the interceptors (i.e. one should take [any ServerInterceptor] and apply to everything) and the other variant should all users to customise the setup a bit more.

I think it might also make sense to not think of "targets" but operations on an interceptor pipeline. It could be something like:

let server = GRPCServer(
  ...
  interceptorPipeline: [
    // Add to all
    .add(LoggingInterceptor()),
    // Add to foo service
    .add(AuthInterceptor(), services: [foo]),
    // Add to foo service _and_ the bar method 
    .add(Blah(), services: [foo], methods: [bar]),
    // Add to the bar method
    .add(BlahBlah(), methods: [bar])
  ]
)

This also lets us leave open the door to adding operations that don't add an interceptor for a particular service/method as well. (Maybe "include" is better terminology here than "add"?)

[gjcairo]

Thanks for the review George, you bring up good points. A couple of things:

I don't think we should propagate the ServerInterceptorTarget right down into the server executor.

Yeah, I went a bit back and forth on this one. I had originally precomputed the array of interceptors, but it felt like a lot of unnecessary new arrays being passed around. However I'm not actually sure performance would be affected - I didn't instrument it before making the change. I'm happy to make the change.

I think there's value in making the common case (interceptors apply to all services) as easy as it currently is

Agreed, good idea.

Also noted about naming. I'll give it a bit more thought.

[glbrntt]

Thanks for the review George, you bring up good points. A couple of things:

I don't think we should propagate the ServerInterceptorTarget right down into the server executor.

Yeah, I went a bit back and forth on this one. I had originally precomputed the array of interceptors, but it felt like a lot of unnecessary new arrays being passed around. However I'm not actually sure performance would be affected - I didn't instrument it before making the change. I'm happy to make the change.

It's potentially quite expensive to do the filtering (depends on how many interceptors are registered and the rules for which services/methods they apply to), and this is done each time the RPC is called. It's especially bad if none of the interceptors apply to the RPC being handled!

I think we should precompute and store an array of interceptors for each method registered with the router. That way we avoid repeating the work each time an RPC is executed.

If we do this carefully we can arrange for there to only be one array allocation per unique interceptor pipeline (rather than one array allocation per method).

[glbrntt]

Previously the interceptors were passed to the router when handle is called. This meant that at this point in the code – i.e. when the router is passed to the server – that the router was fully configured and the caller was responsible for configuring it.

In the other API, where the caller passes services and interceptors to the server, the server was responsible for configuring the router.

With the router now accepting interceptors the lines are blurred: it's not clear who is responsible for configuring the router. In this case it's partly the caller and partly the server. I think this distinction should be clearer.

I see a few options:

1. Remove the ability to create the server with a router and interceptors and make the caller responsible for configuring the router entirely. (If the API where they pass it into the server.)
2. Stop storing the interceptors on the router and continue storing them on the server. This would require the server to lookup the interceptors for each RPC and pass them into the router (which is similar to the existing setup).

[gjcairo]

Good point. I honestly think that 1 is the right approach here, since, in the init where the router is passed, we are already assuming it's set up properly (with e.g. each service's methods registered).

[glbrntt]

Great, I'm happy with (1)

[glbrntt]

Does this need to be public?

[gjcairo]

Only if we want to keep RPCRouter/registerInterceptors inlinable.

[glbrntt]

Okay, in that case we should prefix it with an underscore

[glbrntt]

Does this need to be public?

[gjcairo]

Only if we want to keep RPCRouter/registerInterceptors inlinable.

[glbrntt]

Now we have two APIs for configuring the router:

1. The server configures it (caller passes in services and interceptor pipeline)
2. The caller configures the router and passes it to the server

I think we need to make this method public so that users can register interceptors using path (2).

One knock on from this is that we either need to document very clearly that the ordering of registerHandler and registerInterceptors are important, or we force users to init the router with the interceptor pipeline.

[gjcairo]

Good point. I think there's value in letting users register the interceptors manually. I've added docs - let me know if you think they're not enough.

[glbrntt]

Thanks Gus, this looks great. The docs check and formatting checks are failing though so those need fixing up before we can merge this.

[glbrntt]

API break expected:

1 breaking change detected in GRPCCore:
  💔 API breakage: constructor GRPCServer.init(transport:router:interceptors:) has been removed