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

Failing URLRequest not available to RequestRetrier #1785

Closed
rastersize opened this Issue Nov 16, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@rastersize
Contributor

rastersize commented Nov 16, 2016

I’m in the process of updating to 4.1.0 to adopt the new behavior of the request retried and adapter introduced with #1682. Unfortunately I ran into some troubles. The intention was to make sure we refresh the access token before making any requests if it has expired. So I made the our RequestAdapter throw a missingAccessToken if the token is expired. That’s when the troubles started.

For context; we have a setup which allows different parts of the app to provide authorizers to the network layer for specific hosts. As such I need to check which host a request was for in order to tell the associated authorizer to refresh its access token.

When the adapter throws the error for a request that hasn’t touched the actual network we can’t actually refresh the token. Because the Request object given the RequestRetrier returns nil for both the request and task properties. As such I can’t check which host the request is going to try and reach. However the Request object does in fact have a reference to the URLRequest via the original data task. That property is internal to Alamofire.

To work around this I decided to extend our missingAccessToken error case with the host, ala:

public enum AuthorizerError: Error {
    case missingAccessToken(host: String)
}

This doesn’t work either though since AdaptError is also internal 😄 As such I can’t check if the Error object given to the retrier is of that type and then extract the underlying error. The same is true for the underlyingAdaptError category on Error so I’m unable to use that.

As such I’m stuck with not knowing anything about the request that failed. Or am I missing something obvious?


To remedy this I’d like to propose a few solutions:

  • Change so that the request property “always1” returns the URLRequest associated with it. This could be accomplished
    • Change so that Request.task returns originalTask as a URLSessionTask if the delegate.task property returns nil.
    • Change so that Request.urlRequest returns originalTask’s originalRequest value if task?.originalRequest is nil.
  • Change AdaptError and its error property to be public.
  • Change Error.underlyingAdaptError to be public.

Obviously several of these changes could made at the same time to support different use cases. Which one do you prefer? Or is there some other solution you’d rather have? I’m open to submitting a PR.


  1. I.e. when there is a URLRequest, obviously not for certain types of requests which doesn’t have one.
@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Nov 17, 2016

Member

Good observation @rastersize. You have some really good suggestions here and I agree that all your assumptions are correct. I'm actually working through essentially the same exercise you are right now for my company's network stack. I have a few different ideas here on what you can do.

Solution

First off, I think the correct solution here is to override the Request request property in the subclasses by doing something like this:

class DataRequest: Request {
    open override var request: URLRequest? {
        let taskRequest = super.request

        guard taskRequest == nil else { return taskRequest }
        guard let originalTask = originalTask as? Requestable else { return nil }

        return originalTask.urlRequest
    }
}

This will give you access to the upcoming urlRequest when the task is actually created which should solve your problem. I'll try and get that put together today into a PR for you to try it out.

Other Approaches

Different Session Managers

What I'd also suggest you think about is using a different SessionManager and Adapter / Retrier per authentication system. That way you don't actually have to inspect the incoming request if it's coming into your Retrier. You just know that it needs to be refreshed since that request is paired with your retrier.

Pass the Host through the Error

If you don't want to split them up like that (that's how I'm doing it in our library), then you will need to pass the host through with your missingAccessToken (maybe consider expiredAccessToken?) error like you posted. That will certainly work for now until I can get the request properties overridden.

Would that be something you'd like to take on and put in a PR for? I'm pretty swamped right now and might struggle to get the changes made today. I'd really appreciate if you could take a stab at this if that would be possible. We'll need to override the Data, Download and Upload Request properties and then write some tests to verify they work as expected.

Cheers. 🍻

Member

cnoon commented Nov 17, 2016

Good observation @rastersize. You have some really good suggestions here and I agree that all your assumptions are correct. I'm actually working through essentially the same exercise you are right now for my company's network stack. I have a few different ideas here on what you can do.

Solution

First off, I think the correct solution here is to override the Request request property in the subclasses by doing something like this:

class DataRequest: Request {
    open override var request: URLRequest? {
        let taskRequest = super.request

        guard taskRequest == nil else { return taskRequest }
        guard let originalTask = originalTask as? Requestable else { return nil }

        return originalTask.urlRequest
    }
}

This will give you access to the upcoming urlRequest when the task is actually created which should solve your problem. I'll try and get that put together today into a PR for you to try it out.

Other Approaches

Different Session Managers

What I'd also suggest you think about is using a different SessionManager and Adapter / Retrier per authentication system. That way you don't actually have to inspect the incoming request if it's coming into your Retrier. You just know that it needs to be refreshed since that request is paired with your retrier.

Pass the Host through the Error

If you don't want to split them up like that (that's how I'm doing it in our library), then you will need to pass the host through with your missingAccessToken (maybe consider expiredAccessToken?) error like you posted. That will certainly work for now until I can get the request properties overridden.

Would that be something you'd like to take on and put in a PR for? I'm pretty swamped right now and might struggle to get the changes made today. I'd really appreciate if you could take a stab at this if that would be possible. We'll need to override the Data, Download and Upload Request properties and then write some tests to verify they work as expected.

Cheers. 🍻

@cnoon cnoon self-assigned this Nov 17, 2016

@cnoon cnoon added this to the 4.2.0 milestone Nov 17, 2016

@rastersize

This comment has been minimized.

Show comment
Hide comment
@rastersize

rastersize Nov 17, 2016

Contributor

@cnoon I agree that the solution under the “Solution” heading is the correct one. I’ll try find some time this week to send a PR, also quite swamped with stuff to do 😅

Contributor

rastersize commented Nov 17, 2016

@cnoon I agree that the solution under the “Solution” heading is the correct one. I’ll try find some time this week to send a PR, also quite swamped with stuff to do 😅

rastersize added a commit to rastersize/Alamofire that referenced this issue Nov 18, 2016

Fix URLRequest not always returned when available
- In certain cases the `request` property would return `nil` even
  though the `Request` had a `URLRequest` object. This change makes
  sure we return any `URLRequest` object we might have.
- Adds some tests to verify that catches the original problem (Alamofire#1785).
- Resolves Alamofire#1785.
@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Nov 20, 2016

Member

Closing this issue out since PR #1792 has been submitted. Please direct all further comments to #1792.

Cheers. 🍻

Member

cnoon commented Nov 20, 2016

Closing this issue out since PR #1792 has been submitted. Please direct all further comments to #1792.

Cheers. 🍻

@cnoon cnoon closed this Nov 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment