Refactor redirect logic to be reusable for async/await (#522)

* refactor RedirectHandler
- `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.

* `HTTPClient.Configuration.RedirectConfiguration.Configuration` is now called `HTTPClient.Configuration.RedirectConfiguration.Mode`
only two `Configuration`s left in the type name

* add redirect logger test

Author: dnadoba

Sources/AsyncHTTPClient/HTTPClient.swift

@@ -504,11 +504,43 @@ public class HTTPClient {
     ///     - delegate: Delegate to process response parts.
     ///     - eventLoop: NIO Event Loop preference.
     ///     - deadline: Point in time by which the request must complete.
-    public func execute<Delegate: HTTPClientResponseDelegate>(request: Request,
-                                                              delegate: Delegate,
-                                                              eventLoop eventLoopPreference: EventLoopPreference,
-                                                              deadline: NIODeadline? = nil,
-                                                              logger originalLogger: Logger?) -> Task<Delegate.Response> {
+    ///     - logger: The logger to use for this request.
+    public func execute<Delegate: HTTPClientResponseDelegate>(
+        request: Request,
+        delegate: Delegate,
+        eventLoop eventLoopPreference: EventLoopPreference,
+        deadline: NIODeadline? = nil,
+        logger originalLogger: Logger?
+    ) -> Task<Delegate.Response> {
+        self._execute(
+            request: request,
+            delegate: delegate,
+            eventLoop: eventLoopPreference,
+            deadline: deadline,
+            logger: originalLogger,
+            redirectState: RedirectState(
+                self.configuration.redirectConfiguration.mode,
+                initialURL: request.url.absoluteString
+            )
+        )
+    }
+
+    /// Execute arbitrary HTTP request and handle response processing using provided delegate.
+    ///
+    /// - parameters:
+    ///     - request: HTTP request to execute.
+    ///     - delegate: Delegate to process response parts.
+    ///     - eventLoop: NIO Event Loop preference.
+    ///     - deadline: Point in time by which the request must complete.
+    ///     - logger: The logger to use for this request.
+    func _execute<Delegate: HTTPClientResponseDelegate>(
+        request: Request,
+        delegate: Delegate,
+        eventLoop eventLoopPreference: EventLoopPreference,
+        deadline: NIODeadline? = nil,
+        logger originalLogger: Logger?,
+        redirectState: RedirectState?
+    ) -> Task<Delegate.Response> {
         let logger = (originalLogger ?? HTTPClient.loggingDisabled).attachingRequestInformation(request, requestID: globalRequestID.add(1))
         let taskEL: EventLoop
         switch eventLoopPreference.preference {
@@ -543,22 +575,20 @@ public class HTTPClient {
             return failedTask
         }
 
-        let redirectHandler: RedirectHandler<Delegate.Response>?
-        switch self.configuration.redirectConfiguration.configuration {
-        case .follow(let max, let allowCycles):
-            var request = request
-            if request.redirectState == nil {
-                request.redirectState = .init(count: max, visited: allowCycles ? nil : Set())
+        let redirectHandler: RedirectHandler<Delegate.Response>? = {
+            guard let redirectState = redirectState else { return nil }
+
+            return .init(request: request, redirectState: redirectState) { newRequest, newRedirectState in
+                self._execute(
+                    request: newRequest,
+                    delegate: delegate,
+                    eventLoop: eventLoopPreference,
+                    deadline: deadline,
+                    logger: logger,
+                    redirectState: newRedirectState
+                )
             }
-            redirectHandler = RedirectHandler<Delegate.Response>(request: request) { newRequest in
-                self.execute(request: newRequest,
-                             delegate: delegate,
-                             eventLoop: eventLoopPreference,
-                             deadline: deadline)
-            }
-        case .disallow:
-            redirectHandler = nil
-        }
+        }()
 
         let task = Task<Delegate.Response>(eventLoop: taskEL, logger: logger)
         do {
@@ -804,21 +834,21 @@ extension HTTPClient.Configuration {
 
     /// Specifies redirect processing settings.
     public struct RedirectConfiguration {
-        enum Configuration {
+        enum Mode {
             /// Redirects are not followed.
             case disallow
             /// Redirects are followed with a specified limit.
             case follow(max: Int, allowCycles: Bool)
         }
 
-        var configuration: Configuration
+        var mode: Mode
 
         init() {
-            self.configuration = .follow(max: 5, allowCycles: false)
+            self.mode = .follow(max: 5, allowCycles: false)
         }
 
-        init(configuration: Configuration) {
-            self.configuration = configuration
+        init(configuration: Mode) {
+            self.mode = configuration
         }
 
         /// Redirects are not followed.

Sources/AsyncHTTPClient/HTTPHandler.swift

@@ -109,16 +109,9 @@ extension HTTPClient {
         /// Request-specific TLS configuration, defaults to no request-specific TLS configuration.
         public var tlsConfiguration: TLSConfiguration?
 
-        struct RedirectState {
-            var count: Int
-            var visited: Set<URL>?
-        }
-
         /// Parsed, validated and deconstructed URL.
         let deconstructedURL: DeconstructedURL
 
-        var redirectState: RedirectState?
-
         /// Create HTTP request.
         ///
         /// - parameters:
@@ -190,7 +183,6 @@ extension HTTPClient {
         public init(url: URL, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil, tlsConfiguration: TLSConfiguration?) throws {
             self.deconstructedURL = try DeconstructedURL(url: url)
 
-            self.redirectState = nil
             self.url = url
             self.method = method
             self.headers = headers
@@ -642,87 +634,42 @@ internal struct TaskCancelEvent {}
 
 internal struct RedirectHandler<ResponseType> {
     let request: HTTPClient.Request
-    let execute: (HTTPClient.Request) -> HTTPClient.Task<ResponseType>
-
-    func redirectTarget(status: HTTPResponseStatus, headers: HTTPHeaders) -> URL? {
-        switch status {
-        case .movedPermanently, .found, .seeOther, .notModified, .useProxy, .temporaryRedirect, .permanentRedirect:
-            break
-        default:
-            return nil
-        }
-
-        guard let location = headers.first(name: "Location") else {
-            return nil
-        }
-
-        guard let url = URL(string: location, relativeTo: request.url) else {
-            return nil
-        }
-
-        guard self.request.deconstructedURL.scheme.supportsRedirects(to: url.scheme) else {
-            return nil
-        }
-
-        if url.isFileURL {
-            return nil
-        }
-
-        return url.absoluteURL
+    let redirectState: RedirectState
+    let execute: (HTTPClient.Request, RedirectState) -> HTTPClient.Task<ResponseType>
+
+    func redirectTarget(status: HTTPResponseStatus, responseHeaders: HTTPHeaders) -> URL? {
+        responseHeaders.extractRedirectTarget(
+            status: status,
+            originalURL: self.request.url,
+            originalScheme: self.request.deconstructedURL.scheme
+        )
     }
 
-    func redirect(status: HTTPResponseStatus, to redirectURL: URL, promise: EventLoopPromise<ResponseType>) {
-        var nextState: HTTPClient.Request.RedirectState?
-        if var state = request.redirectState {
-            guard state.count > 0 else {
-                return promise.fail(HTTPClientError.redirectLimitReached)
-            }
-
-            state.count -= 1
-
-            if var visited = state.visited {
-                guard !visited.contains(redirectURL) else {
-                    return promise.fail(HTTPClientError.redirectCycleDetected)
-                }
-
-                visited.insert(redirectURL)
-                state.visited = visited
-            }
-
-            nextState = state
-        }
-
-        let originalRequest = self.request
-
-        var convertToGet = false
-        if status == .seeOther, self.request.method != .HEAD {
-            convertToGet = true
-        } else if status == .movedPermanently || status == .found, self.request.method == .POST {
-            convertToGet = true
-        }
-
-        var method = originalRequest.method
-        var headers = originalRequest.headers
-        var body = originalRequest.body
-
-        if convertToGet {
-            method = .GET
-            body = nil
-            headers.remove(name: "Content-Length")
-            headers.remove(name: "Content-Type")
-        }
-
-        if !originalRequest.url.hasTheSameOrigin(as: redirectURL) {
-            headers.remove(name: "Origin")
-            headers.remove(name: "Cookie")
-            headers.remove(name: "Authorization")
-            headers.remove(name: "Proxy-Authorization")
-        }
-
+    func redirect(
+        status: HTTPResponseStatus,
+        to redirectURL: URL,
+        promise: EventLoopPromise<ResponseType>
+    ) {
         do {
-            var newRequest = try HTTPClient.Request(url: redirectURL, method: method, headers: headers, body: body)
-            newRequest.redirectState = nextState
-            self.execute(newRequest).futureResult.whenComplete { result in
+            var redirectState = self.redirectState
+            try redirectState.redirect(to: redirectURL.absoluteString)
+
+            let (method, headers, body) = transformRequestForRedirect(
+                from: request.url,
+                method: self.request.method,
+                headers: self.request.headers,
+                body: self.request.body,
+                to: redirectURL,
+                status: status
+            )
+
+            let newRequest = try HTTPClient.Request(
+                url: redirectURL,
+                method: method,
+                headers: headers,
+                body: body
+            )
+            self.execute(newRequest, redirectState).futureResult.whenComplete { result in
                 promise.futureResult.eventLoop.execute {
                     promise.completeWith(result)
                 }