Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Header configuration + Fix GET with null parameter #602

Merged
merged 11 commits into from Jul 3, 2019

Conversation

Projects
None yet
6 participants
@designatednerd
Copy link
Contributor

commented Jul 1, 2019

I know this has been a request for some time, but this PR contains the ability to change parameters on the URL request before it gets sent out, the ability to have the raw Data?/URLResponse/Error? response from a URLSessionTask so you can grab headers and/or log things as you want, and the ability to retry asynchronously (for example, after re-authenticating).

This work also uncovered some weirdness with sending GET requests with null parameters for optional variables, so I broke out the GET url constructor, simplified it, and tested it.

import Foundation

struct GraphQLGETTransformer {

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Jul 1, 2019

Contributor

Breaking this out and testing it is a great improvement. Just wondering why you decided to make this a struct instead of a top-level function. Were you thinking of extending this with additional methods or multi-stage building in the future?

This comment has been minimized.

Copy link
@designatednerd

designatednerd Jul 2, 2019

Author Contributor

I prefer to make things named structs/classes/enums instead of free-floating functions. This is partly to make it easier to break things into additional methods in the future, but it's mostly for clarity of where the hell something is coming from.

@@ -1,11 +1,32 @@
import Foundation

public protocol HTTPNetworkTransportDelegate: class {

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Jul 1, 2019

Contributor

I've blocked this way too long and I want to leave it to you to decide what to do, but in the past I've seen it as a hard requirement that our solution should also support retries on error and token refresh, which probably requires at least some callbacks to be asynchronous. See here for the way Alamofire supports this for example. Maybe it would be enough to make shouldSend asynchronous?

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Jul 1, 2019

Contributor

Here are some other previous ideas for delegate methods: #257 (comment)

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Jul 1, 2019

Contributor

There was also this idea about propagating custom attributes from client API methods in a context argument: #257 (comment)

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Jul 1, 2019

Contributor

Not saying you should do anything with this, I know I've probably made it too complicated in the past :)

This comment has been minimized.

Copy link
@RolandasRazma

RolandasRazma Jul 2, 2019

Contributor

retries on token error and refresh is supper useful, we have that in our code. Helps with wrong dates on client and token format changes on server

This comment has been minimized.

Copy link
@designatednerd

designatednerd Jul 2, 2019

Author Contributor

I based a bunch of this on your suggestions here.

What am I missing about Context that wouldn't be covered by just altering the Request object?

I'm gonna take a look at making response handling a separate delegate, since not everyone who needs request handling needs response handling, and optional delegate methods require all sorts of objc nonsense.

I'll admit that I'm kiiiind of reluctant to do custom retry logic due to the additional complexity, but I'll see if I can make that work.

This comment has been minimized.

Copy link
@designatednerd

designatednerd Jul 2, 2019

Author Contributor

Response handling and retry delegates have now been added.

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Jul 3, 2019

Contributor

Oh, I missed this before, but to answer your question about Context: this would allow you to pass in additional data from ApolloClient methods like fetch (so per request, at the point of use), as opposed to doing it from the delegate. Probably not needed right now, but for context :)

@designatednerd designatednerd force-pushed the update/get-as-option branch from d2059ab to 6a89b50 Jul 2, 2019

@designatednerd designatednerd force-pushed the add/header-configurator branch 2 times, most recently from 6c3cec8 to 5a6cff0 Jul 2, 2019

@designatednerd

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@martijnwalraven Added a couple more delegates to allow retrying and/or handling the raw request + response

case serializedBodyMessageError
case serializedQueryParamsMessageError

public var errorDescription: String? {
switch self {
case .cancelledByDeveloper:
return "The request was cancelled by the developer using the HTTPNetworkTransportPreflightDelegate."

This comment has been minimized.

Copy link
@koenpunt

koenpunt Jul 2, 2019

Not sure if this should mention "the developer", because then you could attribute almost all implementation to "the developer".

Maybe something like cancelledByPreflight with "The request was cancelled by the the HTTPNetworkTransportPreflightDelegate" is more suitable?

This comment has been minimized.

Copy link
@designatednerd

designatednerd Jul 2, 2019

Author Contributor

Fair point

@designatednerd designatednerd force-pushed the add/header-configurator branch 2 times, most recently from d0ecc33 to 0f39e66 Jul 2, 2019

@designatednerd designatednerd changed the base branch from update/get-as-option to master Jul 2, 2019

@designatednerd designatednerd force-pushed the add/header-configurator branch from 0f39e66 to 4b5af8d Jul 2, 2019

@designatednerd

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

This is now out of Fake Draft Because You Can't Turn An Open PR Into A Draft PR status and is ready for review 😃

@@ -1,10 +1,13 @@
/// An error which has occurred during the serialization of a request.
public enum GraphQLHTTPRequestError: Error, LocalizedError {
case cancelledByDeveloper

This comment has been minimized.

Copy link
@koenpunt

koenpunt Jul 2, 2019

I think the enum case should be changed too, for the same reason the errorDescription did.

@koenpunt

This comment has been minimized.

Copy link

commented Jul 2, 2019

Great to see some activity on this repo again, thanks!

/// - response: [Optional] Any response received when the error was generated
/// - retryHandler: A closure indicating whether the operation should be retried. Asyncrhonous to allow for re-authentication or other async operations to complete.
func networkTransport(_ networkTransport: HTTPNetworkTransport,
receievedError error: Error,

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Jul 2, 2019

Contributor

Small typo, should be receivedError.

This comment has been minimized.

Copy link
@designatednerd

designatednerd Jul 2, 2019

Author Contributor

🤦‍♀️If i spell received right on the first try ONCE IN MY LIFE I will be happy.

/// A network transport that uses HTTP POST requests to send GraphQL operations to a server, and that uses `URLSession` as the networking implementation.
public class HTTPNetworkTransport: NetworkTransport {
let url: URL
let session: URLSession
let serializationFormat = JSONSerializationFormat.self
let useGETForQueries: Bool
let preflightDelegate: HTTPNetworkTransportPreflightDelegate?

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Jul 2, 2019

Contributor

I'm a little worried about a proliferation of delegate properties here. I can see the benefit of keeping these separate, but it may also be worth simplifying this to a single delegate property that can conform to any of these delegate protocols (similar to how URLSessions delegate can conform to URLSessionDelegate, URLSessionTaskDelegate, etc.).

This comment has been minimized.

Copy link
@designatednerd

designatednerd Jul 2, 2019

Author Contributor

The problem is that URLSessionDelegate et al are Objective-C protocols, so they can have optional methods. If you only want to implement method foo(), then that's all you have to implement.

Our Swift protocols can't unless we mark the protocol @objc, which has a ton of other knock-on effects we don't want. This means if you're implementing a single protocol, you have to implement ALL of its methods even if you don't need or even want them.

I agree that having to have things conforming to multiple protocols is annoying, but I think the separation of concerns (and the ability to avoid needing to implement EVERYTHING just to get one thing) is worth it.

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Jul 3, 2019

Contributor

Ah, I'm ok with having different delegate protocols, but what I meant was that we could still have only one delegate property. I believe that is similar to what URLSession's delegate does, so your delegate has to conform to URLSessionDelegate and optionally to URLSessionTaskDelegate etc.

This comment has been minimized.

Copy link
@designatednerd

designatednerd Jul 3, 2019

Author Contributor

URLSessionDelegate is the base protocol which inherits from NSObjectProtocol - URLSessionTaskDelegate et al inherit from `URLSessionDelegate. Since these are all Obj-C, they can have optional methods, which pure Swift protocols can't.

We could set up some kind of inheritance with our pure Swift protocols to allow passing the highest level as a single delegate, but the user would still need to implement all protocols in the inheritance tree, which again may not be what they need or want.

This comment has been minimized.

Copy link
@designatednerd

designatednerd Jul 3, 2019

Author Contributor

AHA. Talking through with Martijn I realized there's a way around this: An empty base protocol. Implementing!

/// - data: [optional] Any data received. Passed through from `URLSession`.
/// - response: [optional] Any response received. Passed through from `URLSession`.
/// - error: [optional] Any error received. Passed through from `URLSession`.
func networkTransport(_ networkTransport: HTTPNetworkTransport,

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Jul 2, 2019

Contributor

For consistency with platform methods maybe something like networkTransport:didCompleteTaskForRequest:withData:response:error? So didComplete instead of completed, and I'm not sure whether the raw part is necessary? Alternatively, we could leave out the task altogether, and make it something like networkTransport:didCompleteRequest:withData:response:error.

This comment has been minimized.

Copy link
@designatednerd

designatednerd Jul 2, 2019

Author Contributor

Agree on didComplete vs completed, will update.

The Raw is to indicate that this is unprocessed data (and that the consumer shouldn't necessarily be trying to process it themselves). I started with didCompleteRequest but I went to the bit with Raw because I didn't want it to be confused by users with the actual completion closure.

@designatednerd designatednerd changed the title WIP: Header configuration + Fix GET with null parameter Header configuration + Fix GET with null parameter Jul 3, 2019

@designatednerd designatednerd force-pushed the add/header-configurator branch from 230dec6 to 89cfced Jul 3, 2019

@designatednerd designatednerd merged commit fa1c88e into master Jul 3, 2019

4 checks passed

CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
netlify/apollo-ios-docs/deploy-preview Deploy preview ready!
Details

@designatednerd designatednerd deleted the add/header-configurator branch Jul 3, 2019

@naknut

This comment has been minimized.

Copy link

commented Jul 4, 2019

Hi @designatednerd thanks so much for fixing this, I've been looking forward to the header reconfiguration for a long time. Is there any documentation on how to actually do reconfiguration of headers? I tried looking through the code in this PR but I feel a little lost.

@designatednerd

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Nothing formal yet, but you can can see how we're altering the request if you look at the tests' implementation of willSend. Does that help?

@naknut

This comment has been minimized.

Copy link

commented Jul 4, 2019

Yes I was looking through that but couldn’t really figure it out. But I’ll have an other look!

@designatednerd

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Basically since request.allHTTPHeaderFields can't just have stuff appended to it, you copy the existing header fields, insert whatever you need, and re-assign the copy that now includes your additional fields to the request.

Since request is an inout parameter, you don't need to return anything, it just modifies the variable being used directly. Does that help?

@alouanemed

This comment has been minimized.

Copy link

commented Jul 5, 2019

Awesome work there @designatednerd, I wonder when we can see this shipped to the next update? Thanks!

@designatednerd

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

Hoping for early next week!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.