RFC: Networking Updates

[designatednerd]

This is the technical outline of a proposal to make major changes to our primary networking interface from the current HTTPNetworkTransport to a RequestChainNetworkTransport which uses a chain of interceptor objects to set up and process the results of network requests.

Note that this is going to be a 🎉 Spectacularly 🎉 breaking change - while very surface level APIs will remain basically the same, if you're doing anything remotely advanced, this will necessitate some changes, but the idea is to break it now so we don't have to break it way worse later.

I would REALLY love feedback on this before I start working towards making this the default option. You can see the code changes in-place in this PR. I will be updating this RFC with feedback as it is received.

Why The Change?

HTTPNetworkTransport allows you to hook into various delegates to accomplish various things. There are several limitations to this approach:

• Users can only do things that are specifically supported by delegates.
• Asynchronous use of delegates without callbacks is basically impossible.
• Any time we want to add a new feature, we need to add a new delegate method and handle it, creating additional complexity.
• There is no flexibility in terms of order of operations, particularly around whether data should be returned to the UI before being written to the cache.

The other major issue driving this update is that the current networking stack is deeply tied to the current cache architecture. This isn't ideal for many reasons, the biggest of which is that the cache likely to change in relation to the Swift Codegen Rewrite.

What is proposed?

The proposed new architecture uses the Interceptor pattern to create a customizable request chain. This means users can hook into the system at any point during the request creation or data processing process.

This also means that the pieces which will need to be swapped out for the Swift Codegen Rewrite are more clearly defined, and less tied to the actual parsing operation.

Finally, this also opens the opportunity for different patterns than we already support, such as writing to the cache after returning data to the UI instead of before, or creating an array of interceptors which hit the network first, then hit the cache if nothing was returned.

New Protocols

• FlexibleDecoder: This is mostly going to be helpful for the Codable implementation down the line, but this will allow anything conforming to Decoder to be used to decode data.

• Parseable: This is a wrapper that allows us to continue to support non-Codable parsing alongside Codable parsing, while keeping us able to constrain and construct things generically. A default implementation for Codable will be provided.

• ApolloInterceptor: This is an interface which allows you to add an asynchronous handler to perform any necessary work, such as fetching credentials and reading or writing from the cache, asynchronously.

  public protocol ApolloInterceptor: class {
  
    /// Called when this interceptor should do its work.
    ///
    /// - Parameters:
    ///   - chain: The chain the interceptor is a part of.
    ///   - request: The request, as far as it has been constructed
    ///   - response: [optional] The response, if received
    ///   - completion: The completion block to fire when data needs to be returned to the UI.
    func interceptAsync<Operation: GraphQLOperation>(
      chain: RequestChain,
      request: HTTPRequest<Operation>,
      response: HTTPResponse<Operation>?,
      completion: @escaping (Result<GraphQLResult<Operation.Data>, Error>) -> Void)
  }

  Default implementations of ApolloInterceptor for both Legacy (ie, non Swift Codegen) networking and Swift Codegen networking will be provided.

• InterceptorProvider This protocol will be used to quickly create a new array of interceptors for a given request:

  public protocol InterceptorProvider {
  
    /// Creates a new array of interceptors when called
    ///
    /// - Parameter operation: The operation to provide interceptors for
    func interceptors<Operation: GraphQLOperation>(for operation: Operation) -> [ApolloInterceptor]
  }

  This design allows for both flexibility (you can return different interceptors for different types of requests, for instance) and isolation (each request will have its own unique set of interceptors, reducing the possibility of different requests stomping on each other).

  Two default interceptor providers are set up:

  • LegacyInterceptorProvider will provide interceptors mimicking the current stack
  • CodableInterceptorProvider will provide interceptors for the forthcoming Swift Codegen Rewrite's network stack.

• ApolloErrorInterceptor will allow you to have additional checks whenever an error is about to be returned. This will be optional to implement, and no default implementation is provided.

  /// Asynchronously handles the receipt of an error at any point in the chain.
  ///
  /// - Parameters:
  ///   - error: The received error
  ///   - chain: The chain the error was received on
  ///   - request: The request, as far as it was constructed
  ///   - response: [optional] The response, if received
  ///   - completion: The completion closure to fire when the operation has completed. Note that if you call `retry` on the chain, you will not want to call the completion block in this method.
  func handleErrorAsync<Operation: GraphQLOperation>(
      error: Error,
      chain: RequestChain,
      request: HTTPRequest<Operation>,
      response: HTTPResponse<Operation>?,
      completion: @escaping (Result<GraphQLResult<Operation.Data>, Error>) -> Void)

New Classes

• HTTPRequest This object will hold all the information related to a request before it hits the network, with the toURLRequest() method creating an actual URLRequest based on all the information in the request. This is subclass-able (and will mostly be using subclasses).

  open class HTTPRequest<Operation: GraphQLOperation> {
  
    open var graphQLEndpoint: URL
    open var operation: Operation
    open var contentType: String
    open var additionalHeaders: [String: String]
    open var clientName: String? = nil
    open var clientVersion: String? = nil
    open var retryCount: Int = 0
    public let cachePolicy: CachePolicy
  
    public init(graphQLEndpoint: URL,
                operation: Operation,
                contentType: String,
                additionalHeaders: [String: String],
                cachePolicy: CachePolicy = .default)
                
    open func toURLRequest() throws -> URLRequest
  
    open func addHeader(name: String, value: String)
  }

  • JSONRequest subclass of HTTPRequest will handle creating requests with JSON, which will be the vast majority of requests with operations. This is where handling of auto-persisted queries is also layered in:

  public class JSONRequest<Operation: GraphQLOperation>: HTTPRequest<Operation> {
  
    public let requestCreator: RequestCreator
  
    public let autoPersistQueries: Bool
    public let useGETForQueries: Bool
    public let useGETForPersistedQueryRetry: Bool
    public var isPersistedQueryRetry = false
  
    public let serializationFormat = JSONSerializationFormat.self
  
    public init(operation: Operation,
                graphQLEndpoint: URL,
                additionalHeaders: [String: String] = [:],
                cachePolicy: CachePolicy = .default,
                autoPersistQueries: Bool = false,
                useGETForQueries: Bool = false,
                useGETForPersistedQueryRetry: Bool = false,
                requestCreator: RequestCreator = ApolloRequestCreator())
  }

  • UploadRequest subclass of HTTPRequest will handle multipart file uploads:

  public class UploadRequest<Operation: GraphQLOperation>: HTTPRequest<Operation> {
  
    public let requestCreator: RequestCreator
    public let files: [GraphQLFile]
    public let manualBoundary: String?  
    public let serializationFormat = JSONSerializationFormat.self
  
    public init(graphQLEndpoint: URL,
            operation: Operation,
            additionalHeaders: [String: String] = [:],
            files: [GraphQLFile],
            manualBoundary: String? = nil,
            requestCreator: RequestCreator = ApolloRequestCreator()) 
  }

• HTTPResponse will represent the objects returned and/or parsed from the server:

  /// Designated initializer
  ///
  /// - Parameters:
  ///   - response: The `HTTPURLResponse` received from the server.
  ///   - rawData: The raw, unparsed data received from the server.
  ///   - parsedResponse: [optional] The response parsed into the `ParsedValue` type. Will be nil if not yet parsed, or if parsing failed.
  public class HTTPResponse<Operation: GraphQLOperation> {
    public var httpResponse: HTTPURLResponse
    public var rawData: Data
    public var parsedResponse: GraphQLResult<Operation.Data>?
  }

• ‌RequestChain will handle the interaction with the network for a single operation.

  public class RequestChain: Cancellable {
  
  /// Creates a chain with the given interceptor array
  public init(interceptors: [ApolloInterceptor])
  
  /// Kicks off the request from the beginning of the interceptor array.
  ///
  /// - Parameters:
  ///   - request: The request to send.
  ///   - completion: The completion closure to call when the request has completed.
  public func kickoff<ParsedValue: Parseable, Operation: GraphQLOperation>(
      request: HTTPRequest<Operation>, 
      completion: @escaping (Result<ParsedValue, Error>) -> Void)
  
  /// Proceeds to the next interceptor in the array.
  /// 
  /// - Parameters:
  ///   - request: The in-progress request object
  ///   - response: [optional] The in-progress response object, if received yet
  ///   - completion: The completion closure to call when data has been processed and should be returned to the UI.
  public func proceedAsync<ParsedValue: Parseable, Operation: GraphQLOperation>(
      request: HTTPRequest<Operation>,               
      response: HTTPResponse<ParsedValue>?,
      completion: @escaping (Result<ParsedValue, Error>) -> Void)
  
  /// Cancels the entire chain of interceptors.
  public func cancel()
  
  /// Restarts the request starting from the first inteceptor.
  ///
  /// - Parameters:
  ///   - request: The request to retry
  ///   - completion: The completion closure to call when the request has completed.
  public func retry<ParsedValue: Parseable, Operation: GraphQLOperation>(
      request: HTTPRequest<Operation>,
      completion: @escaping (Result<ParsedValue, Error>) -> Void)
  }

• RequestChainNetworkTransport provides an implementation of NetworkTransport which uses an InterceptorProvider to create a request chain for each request.

  public class RequestChainNetworkTransport: NetworkTransport {
  
  public init(interceptorProvider: InterceptorProvider,
    endpointURL: URL,
    additionalHeaders: [String: String] = [:],
    autoPersistQueries: Bool = false,
    cachePolicy: CachePolicy = .default,
    requestCreator: RequestCreator = ApolloRequestCreator(),
    useGETForQueries: Bool = false,
    useGETForPersistedQueryRetry: Bool = false)
  }

Changes to existing Protocols and Classes

• ApolloStore will no longer require a GraphQLQuery explicitly for fetching data from the store. It will instead return an error if the GraphQLOperationType is not .query. This change is necessary to avoid going down an enormous rabbit hole with generics since GraphQLOperation has an associated type.

• The NetworkTransport protocol will get a new method to be implemented:

    func sendForResult<Operation: GraphQLOperation>(operation: Operation,
                                                    completionHandler: @escaping (Result<GraphQLResult<Operation.Data>, Error>) -> Void) -> Cancellable

  This will avoid the double-wrapping of GraphQLResponse around the GraphQLResult so that only the GraphQLResult is actually returned. The send method will eventually be deprecated and removed.

• ApolloClient will get a new sendForResult method which calls into the sendForResult method added to NetworkTransport.

How will this work in practice?

Instantiating a new legacy client manually will look like this:

lazy var legacyClient: ApolloClient = {
    let url = URL(string: "http://localhost:8080/graphql")!
    
    let store = ApolloStore(cache: InMemoryNormalizedCache())
    let provider = LegacyInterceptorProvider(store: store)
    let transport = RequestChainNetworkTransport(interceptorProvider: provider, endpointURL: url)
    
    return ApolloClient(networkTransport: transport)
}()

Ideally I'll be able to transparently swap out the existing HTTPNetworkTransport for this so that this would be the under-the-hood setup on ApolloClient, but this may involve a transition period.

Calls to the client will look like this:

legacyClient.fetchForResult(query: HeroNameQuery()) { result in
    switch result {
    case .success(let graphQLResult):
        print(graphQLResult.data?.hero?.name ?? "Name not found")
    case .failure(let error):
        print("Unexpected error: \(error)")
    }
}

Note that this is VERY similar to how they look on the surface at the moment, which is intentional.

[TizianoCoroneo]

Overall this looks great. I only have 2 small questions:

• Do you think it is possible to add an ErrorType: Error generic type to the signature of handleErrorAsync in the ApolloErrorInterceptor? I would love to be able to return typed errors, and this generic parameter could simply default to Error itself if not specified.
• Do you have any plan to add an official ApolloCombine module?

[designatednerd]

1. I'll have to mess around with this and see how it works - my recollection is that it would cause much more complex typing issues, but I'll see what we can do.
2. Eventually, but it's going to be after all the codegen rewrites. There is a community Combine package if you're looking for something immediately

[eliperkins]

Instantiating a new legacy client

Heh, this was kinda funny to me.

---

public let requestCreator: RequestCreator

Should the request creator live on all requests? Said another way, what's the rationale behind only allowing JSONRequests to have this property?

Do you think it is possible to add an ErrorType: Error generic type to the signature of handleErrorAsync

+1 to this, but definitely not critical for the context of this RFC. I feel like typed errors could be it's own whole RFC/PR too!

ApolloStore will no longer require a GraphQLQuery explicitly for fetching data from the store. It will instead return an error if the GraphQLOperationType is not .query. This change is necessary to avoid going down an enormous rabbit hole with generics since GraphQLOperation has an associated type.

I'm a little bit fuzzy on what this change is. If not a GraphQLQuery, what will the input for data fetching from the store be? Can you clarify for me?

---

All told, this seems solid! One of our big use cases in the GitHub app for the current delegate implementation of this is to logout on HTTP 401 status codes, so it seems like that should make this a little simpler (just need an interceptor to throw an error when those responses come back).

Holler if you want some feedback directly on #1341 as well!

[designatednerd]

Instantiating a new legacy client

Heh, this was kinda funny to me.

Ha, that's what I get for working on this doc for so long, I completely glossed that over 🙃

public let requestCreator: RequestCreator

Should the request creator live on all requests? Said another way, what's the rationale behind only allowing JSONRequests to have this property?

My thought that was only HTTPRequest subclasses that actually need to use it should have access to it - JSONRequest and a forthcoming UploadRequest would be the places I'm thinking. At this point both of those subclasses would need it, but I don't know that it makes sense to tie that to the base class at this time. Would be interesting to hear your thoughts on that.

ApolloStore will no longer require a GraphQLQuery explicitly for fetching data from the store. It will instead return an error if the GraphQLOperationType is not .query. This change is necessary to avoid going down an enormous rabbit hole with generics since GraphQLOperation has an associated type.

I'm a little bit fuzzy on what this change is. If not a GraphQLQuery, what will the input for data fetching from the store be? Can you clarify for me?

Basically, we have the GraphQLOperation protocol, with an associated type of Data, and its three sub-protocols, GraphQLQuery, GraphQLMutation, and GraphQLSubscription. Previously, we were limiting what operations could read from the cache to GraphQLQuery, since in theory that's the only place getting something from the cache would matter.

HTTPRequest only requires a specification of GraphQLOperation, but when making a call into the cache, we can't use the Blah is GraphQLQuery method of figuring out whether a GraphQLOperation is a query because of the associatedType on GraphQLOperation.

I tried about 15 different workarounds for this and this is the only one that wasn't monstrously over-complicated - everything else involved some mild-to-completely bizarre type erasure strategies. I'm not totally against those, but I think in this case the benefits of going that way were vastly outweighed just changing the gating of this a bit.

[designatednerd]

and if you have specific feedback on #1341 I'd love to hear it - just be aware that there's definitely a lot that's WIP (thus the TODOs)

[designatednerd]

OK, I've done some poking around on the request for a typed error parameter, and I don't think it's going to be doable at the interceptor level.

When I tried to add this in, any existing Error I tried to return would cause a "Cannot convert value" build failure:

It'd make the interceptors a lot harder to keep independent if they all had to have exactly the same type of error. It also wouldn't be possible for me to have default implementations return errors without having TypedError conform to some other protocol that allows me to return an underlying arbitrary error. Ultimately, I think that adds too much complexity for general use cases.

I think if you want to write your own typed wrapper that takes whatever's returned and feeds it into something with an underlying error that's easier to switch on, that could work. But I think requiring everything have to be the same type at a protocol level is going to lead to a hell of a lot more confusion than it solves.

Would love to hear feedback on this finding.

[benjamn]

One of the things that's nice about the ApolloLink abstraction used for the web version of Apollo Client is that the link chain is more like a linked list, rather than an array, with each link deciding how to forward the request on to the rest of the chain (and how to process the response), without needing an overarching RequestChain to manage the list of links.

Concretely, this kind of approach would probably mean the InterceptorProvider would return a single ApolloInterceptor (analogous to ApolloLink), representing the head of the linked list, rather than a [ApolloInterceptor] array. The last interceptor in the chain (the tail of the list) would be responsible for performing the actual HTTP request, in most cases—but not always!

Sometimes you want to terminate the chain with something that makes an HTTP request, but other times the terminal link might provide its own data (for example, mock data during tests). Sometimes you might want to split a request between multiple downstream links, or choose among several servers that can handle different kinds of requests (or load-balance the same type of request between multiple servers, entirely on the client). It's hard to represent branching structures like that as an array, because it's no longer really a list, but a dynamic tree. But if each interceptor gets to make its own decisions about how it passes requests to the rest of the chain (and how it handles the responses), the branching can be hidden as an implementation detail, with each interceptor abstracting over everything downstream from it.

I don't know enough Swift to anticipate specific ways in which this approach might be tricky, but it seems like it should be possible to give the interceptor chain more of a recursive, potentially tree-like structure. That's worked pretty well on the web, in the sense that I haven't had to worry very much about the ApolloLink system, even as I've changed large portions of the rest of the library.

For reference, here's an overview of ApolloLink concepts that I've found useful in the past: https://www.apollographql.com/docs/link/overview/

[designatednerd]

I think one huge, huge difference is that things like cancellation, retry, and thread management can be handled through javascript's Observable reactive system.

That just isn't doable without either using Combine (which would require dropping everything below iOS 12, and is unfortunately a non-starter with a number of our larger users) or adding some kind of Reactive library as a dependency (which I am loathe to do because of the massive number of dependency conflicts it could introduce).

However, I think you're right that splitting out methods for request setup vs response handling is a good idea - this could at least help reduce the number of things that need to be optional on HTTPResponse.

Will futz with this tomorrow.

[benjamn]

@designatednerd I'm glad you brought that up!

While I agree that Observables are an easier pill to swallow in JavaScript, and they do figure prominently in the ApolloLink API, I specifically do not think it's necessary to replicate their behavior here.

I should also mention that Observable is not implemented natively in JS, so you still need some sort of library, and (if I'm being honest) the API doesn't fully deliver on any of those benefits you mentioned, so I think it's fair to say the acceptance of Observable in JS (compared to the resistance to Combine in iOS) is a matter of culture/taste/evangelism.

On the topic of cancellation, one approach that does work is to pass some sort of context object explicitly down through the chain (and back up), so that each link/interceptor can check whether the request has been cancelled at points where aborting would be safe. Observables don't provide any useful notion of context, even if you wanted it, but you can design a pipeline/interceptor/chain system like this to provide a strongly typed context object everywhere it's needed, I believe.

On the topic of retrying, the RetryLink subclass of ApolloLink has been successful in part because the retry logic can be hidden behind the same abstraction that any other ApolloLink provides. From the perspective of links earlier in the chain, a request that succeeded after several retries looks exactly like one that succeeded on the first try, except that it might have taken a bit longer. If retrying was something you could implement in a perfectly generic way for all interceptors, it might make sense to hoist it to a higher layer of the system (maybe into RequestChain), but in practice retrying tends to be sensitive to application concerns (different logic for different queries, even), so I think it makes sense to push it down into a part of the system that can be customized by application developers.

I think we can agree it's important for interceptors to be able to perform any kind of async work as an implementation detail, which requires a uniformly asynchronous API. Both Observable and Combine provide that kind of API, but they are both probably overkill, or at the very least they would need to earn their way into a system like this. I believe you that Combine is not worth it for iOS, but then again I'm not sure Observable is totally defensible for JS applications, either.

[TizianoCoroneo]

When I tried to add this in, any existing Error I tried to return would cause a "Cannot convert value" build failure:

from this error it looks like you changed the signature of handleErrorAsync to:

func handleErrorAsync<ParsedValue: Parseable, Operation: GraphQLOperation, TypedError: Error>(
    error: TypedError,
    chain: RequestChain,
    request: HTTPRequest<Operation>,
    response: HTTPResponse<ParsedValue>,
    completion: @escaping (Result<ParsedValue, TypedError>) -> Void)

while I was thinking something like:

func handleErrorAsync<ParsedValue: Parseable, Operation: GraphQLOperation, TypedError: Error>(
    error: Error, // keep this one untyped so that you can pass anything you need
    chain: RequestChain,
    request: HTTPRequest<Operation>,
    response: HTTPResponse<ParsedValue>,
    completion: @escaping (Result<ParsedValue, TypedError>) -> Void) // but allow users to wrap the internal errors in their own custom types

so that you can give a default implementation that does not wrap the error type:

extension ApolloErrorInterceptor where TypedError == Error {
// default interceptor with a untyped completionHandler

sorry I wasn't clear with the previous message. Thank you for fiddling with this!

+1 to this, but definitely not critical for the context of this RFC. I feel like typed errors could be it's own whole RFC/PR too!

@designatednerd would you prefer to move this conversation in a separate issue?

[designatednerd]

@TizianoCoroneo Yeah if I left it with Error as the incoming error, I got that same Cannot convert value problem in the method in question - I'll take a look at your suggestion later today though

[designatednerd]

@benjamn I think one thing that's helpful context to understand is that the basis for this architecture is the ApolloInterceptor type in Android. This itself is based on OkHttp library's Interceptor type that's used very widely throughout the Android ecosystem for networking.

In OkHttp, there's a chain that takes an array of interceptors. Each interceptor in the array needs to call proceed before it can keep going, and then each one calls intercept to handle the result. There's a few issues with that architecture as-is (mostly around things being implicitly asynchronous instead of explicitly asynchronous), which are generally dealt with by ApolloInterceptor's changes to the architecture.

Still futzing around, but I thought that'd be useful context.