Simplified HTTPClient.Completion and refactored response handling i…

…mplementation (#1426)

For #695.

The biggest change is that this code at the beginning of `HTTPClient.handle` is now gone:
```swift
var statusCode: HTTPStatusCode = .networkConnectTimeoutError
var jsonObject: [String: Any]?
var httpResponse: HTTPResponse? = HTTPResponse(statusCode: statusCode, jsonObject: jsonObject)
var receivedJSONError: Error?
```

It was hard to trace which paths modified these default values. The new implementation has a mostly linear path (part of it abstracted in smaller methods now) and doesn’t have mutable values.

This is the new `handle` implementation:
```swift
func handle(urlResponse: URLResponse?,
            request: Request,
            urlRequest: URLRequest,
            data: Data?,
            error networkError: Error?) {
    let (response, shouldRetry) = self.parse(
        urlResponse: urlResponse,
        request: request,
        urlRequest: urlRequest,
        data: data,
        error: networkError
    )

    if shouldRetry {
        Logger.debug(Strings.network.retrying_request(httpMethod: request.method.httpMethod,
                                                      path: request.path))

        self.state.modify {
            $0.queuedRequests.insert(request.retriedRequest(), at: 0)
        }
    } else {
        request.completionHandler?(response)
    }

    self.beginNextRequest()
}
```

## All Changes:

- `HTTPClient.perform` no longer has a bunch of mutable variables with forking paths that make it hard to follow. Most of the response handling is now in a new `parse` method. This will simplify the task of continuing to refactor response deserialization.
- `HTTPClient`’s completion abstracts response into `HTTPResponse`
- `UserInfoAttributeParser` is now an `enum`
- `ETagManager` no longer takes an `Error`. The only thing it did with it is to return the default response.
- `HTTPResponse.jsonObject` is no longer `Optional`
- Moved `defaultJsonEncoder` / `defaultJsonDecoder` to `JSONDecoder+Extensions`. They used by `HTTPClient` and others soon.

Sources/FoundationExtensions/JSONDecoder+Extensions.swift

@@ -129,3 +129,25 @@ extension Encodable {
     }
 
 }
+
+extension JSONEncoder {
+
+    static let `default`: JSONEncoder = {
+        let encoder = JSONEncoder()
+        encoder.keyEncodingStrategy = .convertToSnakeCase
+
+        return encoder
+    }()
+
+}
+
+extension JSONDecoder {
+
+    static let `default`: JSONDecoder = {
+        let decoder = JSONDecoder()
+        decoder.keyDecodingStrategy = .convertFromSnakeCase
+
+        return decoder
+    }()
+
+}

Sources/Networking/ETagManager.swift

@@ -39,13 +39,12 @@ class ETagManager {
     }
 
     func httpResultFromCacheOrBackend(with response: HTTPURLResponse,
-                                      jsonObject: [String: Any]?,
-                                      error: Error?,
+                                      jsonObject: [String: Any],
                                       request: URLRequest,
                                       retried: Bool) -> HTTPResponse? {
         let statusCode: HTTPStatusCode = .init(rawValue: response.statusCode)
         let resultFromBackend = HTTPResponse(statusCode: statusCode, jsonObject: jsonObject)
-        guard error == nil else { return resultFromBackend }
+
         let headersInResponse = response.allHeaderFields
 
         let eTagInResponse: String? = headersInResponse[ETagManager.eTagHeaderName] as? String ??

Sources/Networking/HTTPClient.swift

@@ -16,7 +16,7 @@ import Foundation
 class HTTPClient {
 
     typealias RequestHeaders = [String: String]
-    typealias Completion = ((_ statusCode: HTTPStatusCode, _ result: Result<[String: Any], Error>) -> Void)
+    typealias Completion = (Result<HTTPResponse, Error>) -> Void
 
     private let session: URLSession
     internal let systemInfo: SystemInfo
@@ -60,13 +60,15 @@ extension HTTPClient {
 }
 
 private extension HTTPClient {
+
     struct State {
         var queuedRequests: [Request]
         var currentSerialRequest: Request?
 
         static let initial: Self = .init(queuedRequests: [],
                                          currentSerialRequest: nil)
     }
+
 }
 
 private extension HTTPClient {
@@ -160,62 +162,114 @@ private extension HTTPClient {
         self.start(request: request)
     }
 
-    func handle(urlResponse: URLResponse?,
-                request: Request,
-                urlRequest: URLRequest,
-                data: Data?,
-                error networkError: Error?) {
-        var statusCode: HTTPStatusCode = .networkConnectTimeoutError
-        var jsonObject: [String: Any]?
-        var httpResponse: HTTPResponse? = HTTPResponse(statusCode: statusCode, jsonObject: jsonObject)
-        var receivedJSONError: Error?
-
-        if networkError == nil {
-            if let httpURLResponse = urlResponse as? HTTPURLResponse {
-                statusCode = .init(rawValue: httpURLResponse.statusCode)
-                Logger.debug(Strings.network.api_request_completed(request.httpRequest, httpCode: statusCode))
-
-                if statusCode == .notModified || data == nil {
-                    jsonObject = [:]
-                } else if let data = data {
-                    do {
-                        jsonObject = try JSONSerialization.jsonObject(with: data) as? [String: Any]
-                    } catch let jsonError {
-                        Logger.error(Strings.network.parsing_json_error(error: jsonError))
-
-                        let dataAsString = String(data: data, encoding: .utf8) ?? ""
-                        Logger.error(Strings.network.json_data_received(dataString: dataAsString))
-
-                        receivedJSONError = jsonError
-                    }
+    // swiftlint:disable:next function_body_length
+    func parse(urlResponse: URLResponse?,
+               request: Request,
+               urlRequest: URLRequest,
+               data: Data?,
+               error networkError: Error?
+    ) -> (result: Result<HTTPResponse, Error>, shouldRetry: Bool) {
+        if let networkError = networkError {
+            return (
+                result: .failure(networkError)
+                    .mapError { error in
+                        if self.dnsChecker.isBlockedAPIError(networkError),
+                           let blockedError = self.dnsChecker.errorWithBlockedHostFromError(networkError) {
+                            Logger.error(blockedError.description)
+                            return blockedError
+                        } else {
+                            return error
+                        }
+                    },
+                shouldRetry: false
+            )
+        }
+
+        guard let httpURLResponse = urlResponse as? HTTPURLResponse else {
+            return (.failure(ErrorUtils.unexpectedBackendResponseError()), false)
+        }
+
+        func extractBody(_ response: HTTPURLResponse, _ statusCode: HTTPStatusCode) throws -> [String: Any] {
+            if let data = data, statusCode != .notModified {
+                let result: [String: Any]?
+
+                do {
+                    result = try JSONSerialization.jsonObject(with: data) as? [String: Any]
+                } catch let jsonError {
+                    Logger.error(Strings.network.parsing_json_error(error: jsonError))
+
+                    let dataAsString = String(data: data, encoding: .utf8) ?? ""
+                    Logger.error(Strings.network.json_data_received(dataString: dataAsString))
+
+                    throw jsonError
                 }
 
-                httpResponse = self.eTagManager.httpResultFromCacheOrBackend(with: httpURLResponse,
-                                                                             jsonObject: jsonObject,
-                                                                             error: receivedJSONError,
-                                                                             request: urlRequest,
-                                                                             retried: request.retried)
-                if httpResponse == nil {
-                    Logger.debug(Strings.network.retrying_request(httpMethod: request.method.httpMethod,
-                                                                  path: request.path))
-                    self.state.modify {
-                        $0.queuedRequests.insert(request.retriedRequest(), at: 0)
-                    }
+                guard let result = result else {
+                    throw ErrorUtils.unexpectedBackendResponseError()
                 }
+
+                return result
+            } else {
+                // No data if the body was empty, or if status is `.notModified`
+                // since the body will be fetched from the eTag.
+                return [:]
             }
         }
 
-        var networkError = networkError
-        if dnsChecker.isBlockedAPIError(networkError),
-           let blockedError = dnsChecker.errorWithBlockedHostFromError(networkError) {
-            Logger.error(blockedError.description)
-            networkError = blockedError
-        }
+        let statusCode = HTTPStatusCode(rawValue: httpURLResponse.statusCode)
+
+        Logger.debug(Strings.network.api_request_completed(request.httpRequest,
+                                                           httpCode: statusCode))
+
+        let result: Result<HTTPResponse?, Error> = Result { try extractBody(httpURLResponse, statusCode) }
+            .map { HTTPResponse(statusCode: statusCode, jsonObject: $0) }
+            .map {
+                return self.eTagManager.httpResultFromCacheOrBackend(with: httpURLResponse,
+                                                                     jsonObject: $0.jsonObject,
+                                                                     request: urlRequest,
+                                                                     retried: request.retried)
+            }
+
+        let shouldRetry: Bool = {
+            if case .success(.none) = result {
+                return true
+            } else {
+                return false
+            }
+        }()
+
+        return (
+            result: result
+                .flatMap {
+                    $0.map(Result.success)
+                        ?? .failure(ErrorUtils.unexpectedBackendResponseError())
+                },
+            shouldRetry: shouldRetry
+        )
+    }
 
-        if let httpResponse = httpResponse,
-           let completionHandler = request.completionHandler {
-            let error = receivedJSONError ?? networkError
-            completionHandler(httpResponse.statusCode, Result(httpResponse.jsonObject, error))
+    func handle(urlResponse: URLResponse?,
+                request: Request,
+                urlRequest: URLRequest,
+                data: Data?,
+                error networkError: Error?) {
+        let (response, shouldRetry) = self.parse(
+            urlResponse: urlResponse,
+            request: request,
+            urlRequest: urlRequest,
+            data: data,
+            error: networkError
+        )
+
+        if shouldRetry {
+            Logger.debug(Strings.network.retrying_request(httpMethod: request.method.httpMethod,
+                                                          path: request.path))
+
+            self.state.modify {
+                $0.queuedRequests.insert(request.retriedRequest(), at: 0)
+            }
+        } else {
+            request.completionHandler?(response)
         }
 
         self.beginNextRequest()
@@ -244,7 +298,6 @@ private extension HTTPClient {
             Logger.error("Could not create request to \(request.path)")
 
             request.completionHandler?(
-                .invalidRequest,
                 .failure(ErrorUtils.networkError(withUnderlyingError: ErrorUtils.unknownError()))
             )
             return
@@ -304,14 +357,7 @@ extension HTTPRequest.Path {
 private extension Encodable {
 
     func asData() throws -> Data {
-        return try jsonEncoder.encode(self)
+        return try JSONEncoder.default.encode(self)
     }
 
 }
-
-private let jsonEncoder: JSONEncoder = {
-    let encoder = JSONEncoder()
-    encoder.keyEncodingStrategy = .convertToSnakeCase
-
-    return encoder
-}()

Sources/Networking/HTTPResponse.swift

@@ -19,14 +19,14 @@ struct HTTPResponse {
     typealias Body = [String: Any]
 
     let statusCode: HTTPStatusCode
-    let jsonObject: Body?
+    let jsonObject: Body
 
 }
 
 extension HTTPResponse: CustomStringConvertible {
 
     var description: String {
-        "HTTPResponse(statusCode: \(self.statusCode.rawValue), jsonObject: \(self.jsonObject?.description ?? ""))"
+        "HTTPResponse(statusCode: \(self.statusCode.rawValue), jsonObject: \(self.jsonObject.description))"
     }
 
 }

Sources/Networking/IntroEligibilityResponse.swift

@@ -16,8 +16,7 @@ import Foundation
 // All parameters that are required to process the reponse from a GetIntroEligibility API call.
 struct IntroEligibilityResponse {
 
-    let result: Result<[String: Any], Error>
-    let statusCode: HTTPStatusCode
+    let result: Result<HTTPResponse, Error>
     let productIdentifiers: [String]
     let unknownEligibilityClosure: () -> [String: IntroEligibility]
     let completion: IntroEligibilityResponseHandler

Sources/Networking/Operations/CreateAliasOperation.swift

@@ -54,16 +54,14 @@ private extension CreateAliasOperation {
         let request = HTTPRequest(method: .post(Body(newAppUserID: newAppUserID)),
                                   path: .createAlias(appUserID: appUserID))
 
-        httpClient.perform(request, authHeaders: self.authHeaders) { statusCode, result in
+        httpClient.perform(request, authHeaders: self.authHeaders) { response in
             self.aliasCallbackCache.performOnAllItemsAndRemoveFromCache(withCacheable: self) { aliasCallback in
 
                 guard let completion = aliasCallback.completion else {
                     return
                 }
 
-                self.createAliasResponseHandler.handle(statusCode: statusCode,
-                                                       result: result,
-                                                       completion: completion)
+                self.createAliasResponseHandler.handle(response, completion: completion)
             }
 
             completion()

Sources/Networking/Operations/GetCustomerInfoOperation.swift

@@ -60,10 +60,9 @@ private extension GetCustomerInfoOperation {
 
         let request = HTTPRequest(method: .get, path: .getCustomerInfo(appUserID: appUserID))
 
-        httpClient.perform(request, authHeaders: self.authHeaders) { statusCode, result in
+        httpClient.perform(request, authHeaders: self.authHeaders) { response in
             self.customerInfoCallbackCache.performOnAllItemsAndRemoveFromCache(withCacheable: self) { callback in
-                self.customerInfoResponseHandler.handle(customerInfoResponse: result,
-                                                        statusCode: statusCode,
+                self.customerInfoResponseHandler.handle(customerInfoResponse: response,
                                                         completion: callback.completion)
             }
 

Sources/Networking/Operations/GetIntroEligibilityOperation.swift

@@ -86,9 +86,8 @@ private extension GetIntroEligibilityOperation {
                                                      fetchToken: self.receiptData.asFetchToken)),
                                   path: .getIntroEligibility(appUserID: appUserID))
 
-        httpClient.perform(request, authHeaders: self.authHeaders) { statusCode, result in
-            let eligibilityResponse = IntroEligibilityResponse(result: result,
-                                                               statusCode: statusCode,
+        httpClient.perform(request, authHeaders: self.authHeaders) { response in
+            let eligibilityResponse = IntroEligibilityResponse(result: response,
                                                                productIdentifiers: self.productIdentifiers,
                                                                unknownEligibilityClosure: unknownEligibilityClosure,
                                                                completion: self.responseHandler)
@@ -99,9 +98,9 @@ private extension GetIntroEligibilityOperation {
 
     func handleIntroEligibility(response: IntroEligibilityResponse) {
         let result: [String: IntroEligibility] = {
-            var eligibilitiesByProductIdentifier = response.result.value
+            var eligibilitiesByProductIdentifier = response.result.value?.jsonObject
 
-            if !response.statusCode.isSuccessfulResponse || response.result.error != nil {
+            if response.result.value?.statusCode.isSuccessfulResponse != true {
                 eligibilitiesByProductIdentifier = [:]
             }
 

Sources/Networking/Operations/GetOfferingsOperation.swift

@@ -46,14 +46,16 @@ private extension GetOfferingsOperation {
 
         let request = HTTPRequest(method: .get, path: .getOfferings(appUserID: appUserID))
 
-        httpClient.perform(request, authHeaders: self.authHeaders) { statusCode, result in
+        httpClient.perform(request, authHeaders: self.authHeaders) { response in
             defer {
                 completion()
             }
 
-            let parsedResponse: Result<[String: Any], Error> = result
+            let parsedResponse: Result<[String: Any], Error> = response
                 .mapError { ErrorUtils.networkError(withUnderlyingError: $0) }
                 .flatMap { response in
+                    let (statusCode, response) = (response.statusCode, response.jsonObject)
+
                     return statusCode.isSuccessfulResponse
                     ? .success(response)
                     : .failure(

Sources/Networking/Operations/Handling/CustomerInfoResponseHandler.swift

@@ -15,15 +15,10 @@ import Foundation
 
 class CustomerInfoResponseHandler {
 
-    let userInfoAttributeParser: UserInfoAttributeParser
-
-    init(userInfoAttributeParser: UserInfoAttributeParser = UserInfoAttributeParser()) {
-        self.userInfoAttributeParser = userInfoAttributeParser
-    }
+    init() { }
 
     // swiftlint:disable:next function_body_length
-    func handle(customerInfoResponse response: Result<[String: Any], Error>,
-                statusCode: HTTPStatusCode,
+    func handle(customerInfoResponse response: Result<HTTPResponse, Error>,
                 file: String = #fileID,
                 function: String = #function,
                 line: UInt = #line,
@@ -36,6 +31,7 @@ class CustomerInfoResponseHandler {
             ))
 
         case let .success(response):
+            let (statusCode, response) = (response.statusCode, response.jsonObject)
             let isErrorStatusCode = !statusCode.isSuccessfulResponse
 
             var result: Result<CustomerInfo, Error> = {
@@ -57,7 +53,7 @@ class CustomerInfoResponseHandler {
                 }
             }()
 
-            let subscriberAttributesErrorInfo = self.userInfoAttributeParser
+            let subscriberAttributesErrorInfo = UserInfoAttributeParser
                 .attributesUserInfoFromResponse(response: response, statusCode: statusCode)
 
             let hasError = (isErrorStatusCode