grpc / grpc-swift

The Swift language implementation of gRPC.
Apache License 2.0
1.99k stars 408 forks source link

Channel level interceptors #1148

Open jongarate opened 3 years ago

jongarate commented 3 years ago

Description

The current interceptor implementation sits on top of each service client. This forces the implementation on the client side of the gRPC service to arrange a factory with an interceptor instance per available service method.

When the backend supports multiple services (which is usually the case), forces the development on the client side to define interceptors per-service/per-method, resulting in a lot of boilerplate code and cumbersomeness.

Suggested solution

Just like in Android, it would be helpful to have an option to define interceptors at the Channel level, so that at least basic exchange data can be logged/observed (e.g. Headers, etc...) by hooking to a single point in the whole architecture.

glbrntt commented 3 years ago

This is quite cumbersome at the moment and worth improving.

Adding it at the channel-level has a few difficulties, mainly that it's not easy to resolve which interceptors to use when they exist at the channel level and in the interceptor factory. Should the interceptors from the factory override or be added to those defined on the channel?

Another approach might be to change the generated interceptor factory: it could provide a generic factory method for 'default' interceptors and default the implementation of each factory method to call the default. That way you'd only have to implement one method per service and you retain full control over configuring interceptors on a per-method basis.

jongarate commented 3 years ago

To my view, channel-level interceptors would sit higher architecturally speaking than the method specific ones, which could be further extended in more detail in the method-level ones if found necessary. They should essentially coexist as their scope and purpose is quite different.

Providing a generic factory for default interceptors would certainly be an improvement, and I guess more feasible in the short run. However, it would still require attaching them to each client explicitly, which hurts maintainability in the long run.

VorobyevS commented 3 years ago

For base factory, temp solution. Resolving unnecessary recreations for same requests. U may improve it for your self.

typealias InterceptorEntity = (SwiftProtobuf.Message
                               & SwiftProtobuf._MessageImplementationBase
                               & SwiftProtobuf._ProtoNameProviding)

class GPRCBaseInterceptorFactory {
    private final class InterceptorsLocator {
        private var interceptors = [String: [Any]]()

        func register<T: InterceptorEntity, D: InterceptorEntity>(_ interceptors: [ClientInterceptor<T, D>]) {
            let description = String(describing: T.self) + String(describing: D.self)
            self.interceptors[description] = interceptors
        }

        func resolve<T: InterceptorEntity, D: InterceptorEntity>() -> [ClientInterceptor<T, D>]? {
            let description = String(describing: T.self) + String(describing: D.self)
            return interceptors[description] as? [ClientInterceptor<T, D>]
        }
    }

    //MARK: Internal properties
    private static let locator = InterceptorsLocator()
    private let creators: [InterceptorCreator]

    //MARK: Initialization
    required init(creators: [InterceptorCreator]) {
        self.creators = creators
    }

    func getInterceptors<T: InterceptorEntity, D: InterceptorEntity>() -> [ClientInterceptor<T, D>] {
        guard let interceptors: [ClientInterceptor<T, D>]  = Self.locator.resolve() else {
            let newInterceptors: [ClientInterceptor<T, D>] = creators.map { $0.create() }
            Self.locator.register(newInterceptors)
            return getInterceptors()
        }
        return interceptors
    }
}
jongarate commented 3 years ago

Still, as I mentioned, this requires feeding the interceptor manually for each service's client method, which is what the whole point of discussion is about.

lacasaprivata2 commented 3 years ago

thankfully, there are plenty of pre-existing examples from java-grpc to tonic (rust-grpc) on how to architect this. This would be huge currently have a section at the bottom of each file I copy & past authentication interceptors into. It's actually quite shocking this hasn't been a more popular issue given the universality of authentication.

XabierGoros commented 3 years ago

Indeed it would be very helpful for everyone.

bartekpacia commented 1 year ago

I'm so surprised that this isn't implemented.

Jerry-Carter commented 1 year ago

Just ran across this issue on the server side. In several situations, registering an interceptors at the Channel level is strongly preferred to adding interceptors for every single method. A single registration point will reduce both developer errors and reduces the testing burden.

Three use cases that come to mind:

cassianomonteiro commented 1 year ago

+1

talksik commented 7 months ago

For a newbie in swift, how do I get this working? I was able to do this simply in Dart, but can't find a way to do it with swift.

I read through various related issues in this repo regarding this. In current state, do I have to create the boilerplate for every single method?

Thank you 🙏🏽

Edit: found the partial solution here: https://github.com/grpc/grpc-swift/issues/1567#issuecomment-1425469302. Reduces boilerplate at least a little bit.

glbrntt commented 7 months ago

For a newbie in swift, how do I get this working? I was able to do this simply in Dart, but can't find a way to do it with swift.

I read through various related issues in this repo regarding this. In current state, do I have to create the boilerplate for every single method?

You do, yes. This will be addressed in the next major version.

talksik commented 7 months ago

Thank you @glbrntt !