Feature/transport prepare request delegate

[johntmcintosh]

@martijnwalraven @MrAlek , I'm still in the process of adding the additional delegate's that we've discussed in Slack related to #37, but wanted to go ahead and push up what I have for the first delegate to give you both a chance to give any initial feedback on how things are shaping up.

This PR adds a HTTPNetworkTransportDelegate to HTTPNetworkTransport which has a single function for asynchronous preparation of the request that is about to be sent out. To support this, the transport send function creates a URLRequestOperation which wraps the URLSessionTask, rather than directly creating the session task inside the transport.

Relatedly, I've pushed up an additional commit to my fork that adds a hook for inspecting/preprocessing a network response before it's parsed by Apollo. In my use-case this would be beneficial for allowing us to inject some network logging of the raw requests and responses, but seems like something others would find beneficial as well. I decided to wait on adding that one to this branch so you could take a look at this in isolation first, but you can also view the diff for that commit at this link above.

[apollo-cla]

@johntmcintosh: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[martijnwalraven]

The added delegate method for inspecting/preprocessing seems a bit much without a clear use case for the more advanced signature. For response logging, something like this seems sufficient:

func networkTransport(_ networkTransport: HTTPNetworkTransport, request: URLRequest, didReceive response: URLResponse, data: Data)

Alternatively, maybe it would make sense to expose URLSessionTaskMetrics? That would give you access to both the request and the response, as well as detailed timings.

[martijnwalraven]

We also need to align this with #223 (comment). You previously mentioned a need for passing in context with operations. Maybe this is too magical, but we could add a context: inout Context parameter to every delegate method. That would allow us to keep track of arbitrary information all the way from the fetch(query:) (or perform(mutation:) or subscribe(to:)) to the delivery of the result.

That would allow you to do something like (@knutaa):

func networkTransport(_ networkTransport: HTTPNetworkTransport, context: inout Context, request: URLRequest, didReceive response: HTTPURLResponse, data: Data) {
  context["requestId"] = response.allHeaderFields["Request-Id"]
}

(We actually already pass through a context at least part of the way, see here). It is marked as UnsafeMutableRawPointer because we only use it to associate a result with a request (to avoid triggering watcher updates for requests it has initiated itself).

[AnandWalvekar]

@martijnwalraven I think we can defer passing the context parameter. The most needed feature for the client would be

1. Read Authentication token being sent in header of login response which can be addressed by
   func networkTransport(_ networkTransport: HTTPNetworkTransport, didComplete response: HTTPURLResponse, for request: URLRequest)
2. Add Authentication token for validation to all other requests
   func networkTransport(_ networkTransport: HTTPNetworkTransport, prepareRequest request: URLRequest, completionHandler: @escaping (URLRequest) -> Void)

Please let me know if I am missing any case which requires context as mandatory param

[johntmcintosh]

@martijnwalraven yep, I was starting from the more complex version just to be as accommodating as possible, but I'm perfectly fine with us dropping back to a simplified version. What's your thought on exposing error objects that might be returned from URLSession through the delegate vs only triggering the delegate on successful responses? For my use-case, it's convenient to be able to separate out logging of network-specific errors from logging for GraphQL errors (for example, request timed out error from URLSession vs parsing error from GraphQL). If we do support breaking that out, the options are probably either:

func networkTransport(_ networkTransport: HTTPNetworkTransport, request: URLRequest, didReceive response: URLResponse?, data: Data?, error: Error?)

or

func networkTransport(_ networkTransport: HTTPNetworkTransport, request: URLRequest, didReceive response: URLResponse, data: Data)
func networkTransport(_ networkTransport: HTTPNetworkTransport, request: URLRequest, didFail error: Error)

What do you think about either of those approaches?

To avoid this particular PR getting too unweildy, I've been thinking about the context parameter as something that we'd do in a subsequent PR on top of this one, as long as we're comfortable that whatever we start with here lends itself to adding the context afterwards. That approach work for you?

[martijnwalraven]

I agree, I assumed we would still have the didFailWithError error:retryHandler: that you mentioned on Slack before. That also seems the most consistent with platform APIs.

So I think the delegate would then look like:

public protocol HTTPNetworkTransportDelegate: class {
  func networkTransport(_ networkTransport: HTTPNetworkTransport, prepareRequest request: URLRequest, completionHandler: @escaping (URLRequest) -> Void)
  func networkTransport(_ networkTransport: HTTPNetworkTransport, request: URLRequest, didReceive response: URLResponse, data: Data)
  func networkTransport(_ networkTransport: HTTPNetworkTransport, request: URLRequest, didFailWithError error: Error, retryHandler: @escaping (_ shouldRetry: Bool) -> Void)
}

I think it's fine to set aside context for now and break up the PR, but we probably want to make a decision on that before the next release because adding a context parameter to the protocol later would be a breaking change.

[johntmcintosh]

@martijnwalraven, yep, sounds good on the context. I'm onboard with defaulting to saying that we plan to add context. I know that would be very valuable for our team, and I'm up for continuing to work through that with you after we wrap up this one. For now, four questions for you:

1. I had been thinking about the didFailWithError: retryHandler : version as one that would be called both from a network failure and a parsing/apollo failure. In my logging example, I would be wanting a way to distinguish between the two, which is what was making me think about including in a didReceive:data:error: delegate, because that one was more clearly only errors that came back from the network call itself. The API does read a bit strangely though if there's an error parameter on both functions. I'm good with just doing the didFailWithError: retryHandler : for errors (and using the error type to distinguish between network and apollo errors if we return both), but are you thinking that will be called for all errors that would be returned from apollo, or just for network errors?

2. Less of a question, but more an FYI, I'm still planning on adding the SSL Pinning delegate that we discussed in slack. There just haven't been many questions to sort through about that one.

3. The URLSessionTaskMetrics idea is interesting--that's one I haven't come across that object before. The only catch is that I don't think we don't get access to the actual response data from the URLResponse in the task metrics, so we'd still need to expose data separately in the API (which is fine), and we'd need to pull from two different URLSession callbacks to have both the data and metrics needed for our one delegate call. Sticking with response:data: is a simpler implementation, but metrics:data: will be more flexible for consumers of the API. I don't necessarily have a strong opinion on which side of that to fall on. What's your perspective on which is better for the library?

4. Do you have any structural feedback based on the single delegate call that's present in this PR currently? If fundamentally it looks like the right approach, I'll build on top of it for getting the others in place. I'd like to sort out any structural changes with just the one delegate rather than after doing the others.

Thanks!

[martijnwalraven]

@johntmcintosh Sorry, I've been meaning to respond, but got preoccupied with other stuff. I think the code looks great!

1. My idea would be that we'd want the ability to retry on both network errors and on requests that return an error response. So those should both result in a call to didFailWithError:retryHandler:.
2. Yep, that seems useful.
3. For response logging, didReceive:data:error: is probably the easier option for now. Looked at it like that, I actually think it may make sense to expose error there as well. Without it, you'd need additional logging in didFailWithError:retryHandler: for the error case.

[martijnwalraven]

@AnandWalvekar I don't think you'd want to use a delegate method to read the authentication token for the login response. In my mind, authentication would be a separate non-GraphQL call that wouldn't be handled by HTTPNetworkTransport

[johntmcintosh]

Thanks @martijnwalraven! Both work and personal life have gotten a bit hectic for me as well. We have a temporary workaround in place in our app right now, but are planning to swap to this implementation once we're all set. I'll do my best to have a solid pass on these ready by the end of the week.

[MaxDesiatov]

I'm not quite sure if a transport delegate API would add significant benefit, as right now it's pretty to easy to get the same behaviour with a custom transport. I've just published a package that implements a custom transport with request and response logging (with debug curl CLI output as well): https://github.com/maxdesiatov/ApolloAlamofire

[knutaa]

It is very useful to have the ApolloAlamofire to show that it is quite possible to handle customer specific needs with a custom transport.

At the same time, I believe it will be valid to have the transport delegate API as an OOTB capability with minimal dependencies.

Looks like the proposal is quite ready to be merged with prepareRequest even if we at this time do not have available code for didReceive and didFailWithError (but would have been good ...)

[johntmcintosh]

Hey all - my apologies for not getting this over the finish line more quickly. I've had a few surprises come up in my personal life recently which have kept me from having the time I expected to do this on the schedule I was originally expecting. I am targeting next week for some dedicated time to get those other delegates in place.

[RyanCodes]

@johntmcintosh Any idea when these will be done? Thanks!

[wow-such-amazing]

Any updates? 🙂

[ppateljb]

When can this be merged? This is a much needed feature for a lot of us. @johntmcintosh thanks!

[designatednerd]

Hey @johntmcintosh - Thank you for doing this, and sorry for the long radio silence on our end!

Unfortunately since so much has changed since this PR was opened, I'm going to close this in favor of the work happening in #602 to implement this feature. Please feel free to chime in there if you have thoughts or questions, and open issues on anything you feel we may be leaving out.