Refactor redirect logic to be reusable for async/await #522
Conversation
dnadoba
force-pushed
the
dn-redirect-refactor
branch
2 times, most recently
from
eff9ef4 to
57ddf52
Compare
fix compilation fro Swift 5.3 remove double whitespace save progress
dnadoba
force-pushed
the
dn-redirect-refactor
branch
from
57ddf52 to
2879863
Compare
| } else if responseStatus == .movedPermanently || responseStatus == .found, requestMethod == .POST { | ||
| convertToGet = true | ||
| } |
There was a problem hiding this comment.
I know that this is just the code that existed before. @Lukasa does that make any sense?
There was a problem hiding this comment.
Yup, that's exactly right, see step 12 in this algorithm.
| } | ||
| redirectHandler = RedirectHandler<Delegate.Response>(request: request) { newRequest in | ||
| guard request.redirectState != nil else { return nil } |
There was a problem hiding this comment.
I find this pattern pretty oblique: it's not obvious to me that having no redirect state means "don't follow redirects". Can we use a computed property on request to make it clearer?
There was a problem hiding this comment.
redirectState is now no longer a property on request. It never really belonged there in the first place and is now passed explicitly to the RedirectHandler. I also didn't like that nil means don't follow redirects but now that we explicitly forward it is a bit better.
If we still want to make it more explicit, I think we need to make RedirectState aware of the "don't follow redirects" case and ask it before we create a RedirectHandler.
| do { | ||
| var redirectState = self.request.redirectState | ||
| try redirectState?.redirect(to: redirectURL.absoluteString) |
There was a problem hiding this comment.
Is it acceptable for redirectState to be nil here?
There was a problem hiding this comment.
No, I have removed redirectState from Request and moved it to RedirectHandler itself. We now explicitly forward the RedirectState and don't carry it implicitly through the Request.
| /// Returns nil if the user disallowed redirects, | ||
| /// otherwise an instance of `RedirectState` which respects the user defined settings. | ||
| init?( | ||
| _ configuration: HTTPClient.Configuration.RedirectConfiguration.Configuration, |
There was a problem hiding this comment.
This type name is awful. Is it possible we could give some of these types clearer names? If not, can we use a typealias?
There was a problem hiding this comment.
sadly HTTPClient.Configuration.RedirectConfiguration is public API but we can change the last Configuration to e.g. Mode. In addition, we can add a typealias:
internal typealias RedirectMode = HTTPClient.Configuration.RedirectConfiguration.ModeWDYT?
There was a problem hiding this comment.
I'd like both of those changes, great idea!
| throw HTTPClientError.redirectCycleDetected | ||
| } | ||
|
|
||
| self.count -= 1 |
There was a problem hiding this comment.
Should we store count or just derive it from self.visited?
There was a problem hiding this comment.
self.visited.count counts up, self.count counts down until it hits zero where we know we reached the limit.
Because we need to know when we reach the limit, self.visited wouldn't be enough and we would still need to store the limit. Nevertheless, I think it's clearer to only count up and compare self.visited.count agains the constant limit to deduplicate counting.
- `redirectState` is no longer a property of `HTTPClient.Request`. RedirectHandler now stores this state directly and therefore no longer optional. - we no longer count the number of allowed redirects down. Instead the number of redirects is dervied from `self.visited.count` and we compare it to the maxRedirect to check if we git the limit.
… called `HTTPClient.Configuration.RedirectConfiguration.Mode` only two `Configuration`s left in the type name
Motivation
We want to reuse the current redirect logic, which was mainly located inside the
RedirectHandler, for the upcoming async/await implementation.Changes
HTTPClient.Request.RedirectStateto the top level and a separate fileRedirectHandlerinto methods onHTTPHeadersand methods onRedirectStateRedirectHandler