diff --git a/Modules/Sources/NetworkingCore/ApplicationPassword/RequestProcessor.swift b/Modules/Sources/NetworkingCore/ApplicationPassword/RequestProcessor.swift index bc1dd0ce434..7243191216e 100644 --- a/Modules/Sources/NetworkingCore/ApplicationPassword/RequestProcessor.swift +++ b/Modules/Sources/NetworkingCore/ApplicationPassword/RequestProcessor.swift @@ -1,13 +1,8 @@ import Alamofire import Foundation -enum AppPasswordFailureReason { - case notSupported - case unknown -} - protocol RequestProcessorDelegate: AnyObject { - func didFailToAuthenticateRequestWithAppPassword(siteID: Int64, reason: AppPasswordFailureReason) + func didFailToAuthenticateRequestWithAppPassword(siteID: Int64) } /// Authenticates and retries requests @@ -92,7 +87,7 @@ private extension RequestProcessor { isAuthenticating = false completeRequests(false) if let currentSiteID { - notifyFailure(error, for: currentSiteID) + notifyFailureIfNeeded(error, for: currentSiteID) } } } @@ -120,22 +115,24 @@ private extension RequestProcessor { } } - func notifyFailure(_ error: Error, for siteID: Int64) { - let reason: AppPasswordFailureReason = { + func notifyFailureIfNeeded(_ error: Error, for siteID: Int64) { + let appPasswordNotSupported: Bool = { switch error { case NetworkError.notFound: - return .notSupported + return true case let networkError as NetworkError: if let code = networkError.errorCode, AppPasswordConstants.disabledCodes.contains(code) { - return .notSupported + return true } - return .unknown + return false default: - return .unknown + return false } }() - delegate?.didFailToAuthenticateRequestWithAppPassword(siteID: siteID, reason: reason) + if appPasswordNotSupported { + delegate?.didFailToAuthenticateRequestWithAppPassword(siteID: siteID) + } } func shouldRetry(_ error: Error) -> Bool { diff --git a/Modules/Sources/NetworkingCore/Network/AlamofireNetwork.swift b/Modules/Sources/NetworkingCore/Network/AlamofireNetwork.swift index f3bb15379aa..0f0095f774e 100644 --- a/Modules/Sources/NetworkingCore/Network/AlamofireNetwork.swift +++ b/Modules/Sources/NetworkingCore/Network/AlamofireNetwork.swift @@ -49,11 +49,8 @@ public class AlamofireNetwork: Network { private var subscription: AnyCancellable? - /// Keeps track of failure counts for each site when switching to direct requests - private var appPasswordFailures: [Int64: Int] = [:] - - /// Keeps track of retried requests when direct requests fail - private var retriedJetpackRequests: [RetriedJetpackRequest] = [] + /// Thread-safe error handler for failure tracking and retry logic + private let errorHandler: AlamofireNetworkErrorHandler /// Public Initializer /// @@ -71,6 +68,7 @@ public class AlamofireNetwork: Network { self.credentials = credentials self.selectedSite = selectedSite self.userDefaults = userDefaults + self.errorHandler = AlamofireNetworkErrorHandler(credentials: credentials, userDefaults: userDefaults) self.requestConverter = { let siteAddress: String? = { switch credentials { @@ -123,7 +121,7 @@ public class AlamofireNetwork: Network { alamofireSession.request(convertedRequest) .validateIfRestRequest(for: convertedRequest) .responseData { [weak self] response in - self?.handleFailureForDirectRequestIfNeeded( + self?.errorHandler.handleFailureForDirectRequestIfNeeded( originalRequest: request, convertedRequest: convertedRequest, failure: response.networkingError, @@ -151,7 +149,7 @@ public class AlamofireNetwork: Network { alamofireSession.request(convertedRequest) .validateIfRestRequest(for: convertedRequest) .responseData { [weak self] response in - self?.handleFailureForDirectRequestIfNeeded( + self?.errorHandler.handleFailureForDirectRequestIfNeeded( originalRequest: request, convertedRequest: convertedRequest, failure: response.networkingError, @@ -176,7 +174,7 @@ public class AlamofireNetwork: Network { let response = await sessionRequest.serializingData().response let failure = response.networkingError - if shouldRetryJetpackRequest( + if errorHandler.shouldRetryJetpackRequest( originalRequest: request, convertedRequest: convertedRequest, failure: failure @@ -184,7 +182,7 @@ public class AlamofireNetwork: Network { return try await responseDataAndHeaders(for: request) } - flagSiteAsUnsupportedForAppPasswordIfNeeded(originalRequest: request, failure: failure) + errorHandler.flagSiteAsUnsupportedForAppPasswordIfNeeded(originalRequest: request, failure: failure) if let error = response.networkingError { throw error @@ -212,7 +210,7 @@ public class AlamofireNetwork: Network { .request(convertedRequest) .validateIfRestRequest(for: convertedRequest) .responseData { [weak self] response in - self?.handleFailureForDirectRequestIfNeeded( + self?.errorHandler.handleFailureForDirectRequestIfNeeded( originalRequest: request, convertedRequest: convertedRequest, failure: response.networkingError, @@ -240,7 +238,7 @@ public class AlamofireNetwork: Network { alamofireSession .upload(multipartFormData: multipartFormData, with: convertedRequest) .responseData { [weak self] response in - self?.handleFailureForDirectRequestIfNeeded( + self?.errorHandler.handleFailureForDirectRequestIfNeeded( originalRequest: request, convertedRequest: convertedRequest, failure: response.networkingError, @@ -284,7 +282,7 @@ private extension AlamofireNetwork { network: self )) requestAuthenticator.delegate = self - appPasswordFailures.removeValue(forKey: site.siteID) // reset failure count + errorHandler.resetFailureCount(for: site.siteID) // reset failure count } } } @@ -293,127 +291,22 @@ private extension AlamofireNetwork { // private extension AlamofireNetwork { func convertRequestIfNeeded(_ request: URLRequestConvertible) -> URLRequestConvertible { - let isRetried = retriedJetpackRequests.contains { retriedRequest in - let urlRequest = try? request.asURLRequest() - let currentItem = try? retriedRequest.request.asURLRequest() - return currentItem == urlRequest - } - if isRetried { + if errorHandler.isRequestRetried(request) { return request // do not convert } return requestConverter.convert(request) } - /// Checks if the specified request and error are eligible for retrying as Jetpack request. - /// If yes, enqueue the original request to the retried list before returning. - /// - func shouldRetryJetpackRequest(originalRequest: URLRequestConvertible, - convertedRequest: URLRequestConvertible, - failure: Error?) -> Bool { - if let request = originalRequest as? JetpackRequest, - convertedRequest is RESTRequest, - case .some(.wpcom) = credentials, - let failure = failure as? NetworkError { - let retriedRequest = RetriedJetpackRequest(request: request, error: failure) - retriedJetpackRequests.append(retriedRequest) - return true - } - return false - } - - /// Determines if the site has issue with application password based on the original request. - /// - func flagSiteAsUnsupportedForAppPasswordIfNeeded(originalRequest: URLRequestConvertible, - failure: Error?) { - let retriedRequestIndex = retriedJetpackRequests.firstIndex { retriedRequest in - let urlRequest = try? originalRequest.asURLRequest() - let retriedRequest = try? retriedRequest.request.asURLRequest() - return urlRequest == retriedRequest - } - guard let index = retriedRequestIndex else { return } - - if failure == nil { - let siteID = retriedJetpackRequests[index].request.siteID - let originalFailure = retriedJetpackRequests[index].error - switch originalFailure { - case .unacceptableStatusCode(statusCode: 401, _), - .unacceptableStatusCode(statusCode: 403, _), - .unacceptableStatusCode(statusCode: 429, _): - flagSiteAsUnsupported(for: siteID) - default: - if let code = originalFailure.errorCode, AppPasswordConstants.disabledCodes.contains(code) { - flagSiteAsUnsupported(for: siteID) - } else { - incrementFailureCount(for: siteID) - } - } - } - - // remove retried request from list - retriedJetpackRequests.remove(at: index) - } - - func handleFailureForDirectRequestIfNeeded(originalRequest: URLRequestConvertible, - convertedRequest: URLRequestConvertible, - failure: Error?, - onRetry: @escaping () -> Void, - onCompletion: @escaping () -> Void) { - if shouldRetryJetpackRequest(originalRequest: originalRequest, - convertedRequest: convertedRequest, - failure: failure) { - onRetry() - } else { - flagSiteAsUnsupportedForAppPasswordIfNeeded(originalRequest: originalRequest, failure: failure) - onCompletion() - } - } - - /// Helper type to keep track of retried requests with accompanied error - /// - struct RetriedJetpackRequest { - let request: JetpackRequest - let error: NetworkError - } } // MARK: `RequestProcessorDelegate` conformance // extension AlamofireNetwork: RequestProcessorDelegate { - func didFailToAuthenticateRequestWithAppPassword(siteID: Int64, reason: AppPasswordFailureReason) { - switch reason { - case .notSupported: - flagSiteAsUnsupported(for: siteID) - case .unknown: - incrementFailureCount(for: siteID) - } - } - - func flagSiteAsUnsupported(for siteID: Int64) { - let currentList = userDefaults.applicationPasswordUnsupportedList - userDefaults.applicationPasswordUnsupportedList = currentList + [siteID] - } - - func incrementFailureCount(for siteID: Int64) { - let currentFailureCount = appPasswordFailures[siteID] ?? 0 - let updatedCount = currentFailureCount + 1 - if updatedCount == AppPasswordConstants.requestFailureThreshold { - let currentList = userDefaults.applicationPasswordUnsupportedList - userDefaults.applicationPasswordUnsupportedList = currentList + [siteID] - } - appPasswordFailures[siteID] = updatedCount + func didFailToAuthenticateRequestWithAppPassword(siteID: Int64) { + errorHandler.flagSiteAsUnsupported(for: siteID) } } -// MARK: - Constants for direct request error handling -enum AppPasswordConstants { - // flag site as disabled after threshold is reached - static let requestFailureThreshold = 10 - static let disabledCodes = [ - "application_passwords_disabled", - "application_passwords_disabled_for_user", - "incorrect_password" - ] -} private extension DataRequest { /// Validates only for `RESTRequest` @@ -450,6 +343,10 @@ extension Alamofire.DataResponse { return error } + if case .some(AFError.requestAdaptationFailed) = error?.asAFError { + return error + } + return response.flatMap { response in NetworkError(responseData: data, statusCode: response.statusCode) diff --git a/Modules/Sources/NetworkingCore/Network/AlamofireNetworkErrorHandler.swift b/Modules/Sources/NetworkingCore/Network/AlamofireNetworkErrorHandler.swift new file mode 100644 index 00000000000..fa5a842c95d --- /dev/null +++ b/Modules/Sources/NetworkingCore/Network/AlamofireNetworkErrorHandler.swift @@ -0,0 +1,169 @@ +import Foundation +import Alamofire + +/// Thread-safe handler for network error tracking and retry logic +final class AlamofireNetworkErrorHandler { + private let queue = DispatchQueue(label: "com.networkingcore.errorhandler", attributes: .concurrent) + private let userDefaults: UserDefaults + private let credentials: Credentials? + + private var _appPasswordFailures: [Int64: Int] = [:] + private var _retriedJetpackRequests: [RetriedJetpackRequest] = [] + + init(credentials: Credentials?, userDefaults: UserDefaults = .standard) { + self.credentials = credentials + self.userDefaults = userDefaults + } + + // MARK: - Thread-safe property access + + private var appPasswordFailures: [Int64: Int] { + get { + queue.sync { _appPasswordFailures } + } + set { + queue.sync(flags: .barrier) { [weak self] in + self?._appPasswordFailures = newValue + } + } + } + + private var retriedJetpackRequests: [RetriedJetpackRequest] { + get { + queue.sync { _retriedJetpackRequests } + } + set { + queue.sync(flags: .barrier) { [weak self] in + self?._retriedJetpackRequests = newValue + } + } + } + + // MARK: - Public interface + + func resetFailureCount(for siteID: Int64) { + appPasswordFailures.removeValue(forKey: siteID) + } + + func shouldRetryJetpackRequest(originalRequest: URLRequestConvertible, + convertedRequest: URLRequestConvertible, + failure: Error?) -> Bool { + guard let error = failure, + let request = originalRequest as? JetpackRequest, + convertedRequest is RESTRequest, + case .some(.wpcom) = self.credentials else { + return false + } + + let isExpectedError: Bool = { + switch error { + case AFError.requestAdaptationFailed: + return true + case _ as NetworkError: + return true + default: + return false + } + }() + + if isExpectedError { + let retriedRequest = RetriedJetpackRequest(request: request, error: error) + retriedJetpackRequests.append(retriedRequest) + return true + } + return false + } + + func flagSiteAsUnsupportedForAppPasswordIfNeeded( + originalRequest: URLRequestConvertible, + failure: Error? + ) { + let retriedRequestIndex = retriedJetpackRequests.firstIndex { retriedRequest in + let urlRequest = try? originalRequest.asURLRequest() + let retriedRequest = try? retriedRequest.request.asURLRequest() + return urlRequest == retriedRequest + } + + guard let index = retriedRequestIndex else { return } + + let retriedRequest = retriedJetpackRequests[index] + + if failure == nil { + let siteID = retriedRequest.request.siteID + let originalFailure = retriedRequest.error + switch originalFailure { + case NetworkError.unacceptableStatusCode(statusCode: 401, _), + NetworkError.unacceptableStatusCode(statusCode: 403, _), + NetworkError.unacceptableStatusCode(statusCode: 429, _): + flagSiteAsUnsupported(for: siteID) + default: + if let networkError = originalFailure as? NetworkError, + let code = networkError.errorCode, + AppPasswordConstants.disabledCodes.contains(code) { + flagSiteAsUnsupported(for: siteID) + } else { + incrementFailureCount(for: siteID) + } + } + } + + retriedJetpackRequests.remove(at: index) + } + + func handleFailureForDirectRequestIfNeeded(originalRequest: URLRequestConvertible, + convertedRequest: URLRequestConvertible, + failure: Error?, + onRetry: @escaping () -> Void, + onCompletion: @escaping () -> Void) { + if shouldRetryJetpackRequest(originalRequest: originalRequest, + convertedRequest: convertedRequest, + failure: failure) { + onRetry() + } else { + flagSiteAsUnsupportedForAppPasswordIfNeeded(originalRequest: originalRequest, failure: failure) + onCompletion() + } + } + + func isRequestRetried(_ request: URLRequestConvertible) -> Bool { + retriedJetpackRequests.contains { retriedRequest in + let urlRequest = try? request.asURLRequest() + let currentItem = try? retriedRequest.request.asURLRequest() + return currentItem == urlRequest + } + } + + func flagSiteAsUnsupported(for siteID: Int64) { + queue.sync(flags: .barrier) { + let currentList = userDefaults.applicationPasswordUnsupportedList + userDefaults.applicationPasswordUnsupportedList = currentList + [siteID] + } + } + + // MARK: - Private methods + + private func incrementFailureCount(for siteID: Int64) { + let currentFailureCount = appPasswordFailures[siteID] ?? 0 + let updatedCount = currentFailureCount + 1 + if updatedCount == AppPasswordConstants.requestFailureThreshold { + flagSiteAsUnsupported(for: siteID) + } + appPasswordFailures[siteID] = updatedCount + } +} + +/// Helper type to keep track of retried requests with accompanied error +struct RetriedJetpackRequest { + let request: JetpackRequest + let error: Error +} + +// MARK: - Constants for direct request error handling +enum AppPasswordConstants { + static let requestFailureThreshold = 10 + static let disabledCodes = [ + "application_passwords_disabled", + "application_passwords_disabled_for_user", + "incorrect_password" + ] +} diff --git a/Modules/Tests/NetworkingTests/ApplicationPassword/RequestProcessorTests.swift b/Modules/Tests/NetworkingTests/ApplicationPassword/RequestProcessorTests.swift index bdc5000e018..c731194fdd7 100644 --- a/Modules/Tests/NetworkingTests/ApplicationPassword/RequestProcessorTests.swift +++ b/Modules/Tests/NetworkingTests/ApplicationPassword/RequestProcessorTests.swift @@ -322,8 +322,7 @@ final class RequestProcessorTests: XCTestCase { // Then XCTAssertFalse(shouldRetry.retryRequired) waitUntil { - mockDelegate.didFailToAuthenticateRequestForSiteID == 123 && - mockDelegate.didFailToAuthenticateRequestWithReason == .notSupported + mockDelegate.didFailToAuthenticateRequestForSiteID == 123 } } @@ -355,8 +354,7 @@ final class RequestProcessorTests: XCTestCase { // Then XCTAssertFalse(shouldRetry.retryRequired) waitUntil { - mockDelegate.didFailToAuthenticateRequestForSiteID == 123 && - mockDelegate.didFailToAuthenticateRequestWithReason == .notSupported + mockDelegate.didFailToAuthenticateRequestForSiteID == 123 } } @@ -388,8 +386,7 @@ final class RequestProcessorTests: XCTestCase { // Then XCTAssertFalse(shouldRetry.retryRequired) waitUntil { - mockDelegate.didFailToAuthenticateRequestForSiteID == 123 && - mockDelegate.didFailToAuthenticateRequestWithReason == .notSupported + mockDelegate.didFailToAuthenticateRequestForSiteID == 123 } } @@ -419,10 +416,6 @@ final class RequestProcessorTests: XCTestCase { // Then XCTAssertFalse(shouldRetry.retryRequired) XCTAssertFalse(mockRequestAuthenticator.deleteApplicationPasswordCalled) - waitUntil { - mockDelegate.didFailToAuthenticateRequestForSiteID == 123 && - mockDelegate.didFailToAuthenticateRequestWithReason == .unknown - } } func test_request_not_retried_when_password_deletion_fails_after_409_error_with_wpcom_authentication() throws { @@ -531,10 +524,8 @@ private class MockRequest: Alamofire.DataRequest, @unchecked Sendable { private class MockRequestProcessorDelegate: RequestProcessorDelegate { private(set) var didFailToAuthenticateRequestForSiteID: Int64? - private(set) var didFailToAuthenticateRequestWithReason: AppPasswordFailureReason? - func didFailToAuthenticateRequestWithAppPassword(siteID: Int64, reason: AppPasswordFailureReason) { + func didFailToAuthenticateRequestWithAppPassword(siteID: Int64) { didFailToAuthenticateRequestForSiteID = siteID - didFailToAuthenticateRequestWithReason = reason } } diff --git a/Modules/Tests/NetworkingTests/Network/AlamofireNetworkErrorHandlerTests.swift b/Modules/Tests/NetworkingTests/Network/AlamofireNetworkErrorHandlerTests.swift new file mode 100644 index 00000000000..8ad98b95c50 --- /dev/null +++ b/Modules/Tests/NetworkingTests/Network/AlamofireNetworkErrorHandlerTests.swift @@ -0,0 +1,440 @@ +import XCTest +import Alamofire +@testable import NetworkingCore + +/// Tests for AlamofireNetworkErrorHandler to verify thread safety and functionality +final class AlamofireNetworkErrorHandlerTests: XCTestCase { + private var userDefaults: UserDefaults! + private var errorHandler: AlamofireNetworkErrorHandler! + + override func setUp() { + super.setUp() + userDefaults = UserDefaults(suiteName: UUID().uuidString) + errorHandler = AlamofireNetworkErrorHandler(credentials: createWPComCredentials(), userDefaults: userDefaults) + } + + override func tearDown() { + userDefaults = nil + errorHandler = nil + super.tearDown() + } + + // MARK: - Basic Functionality Tests + + func test_resetFailureCount_removes_failure_count_for_site() { + // Given + let siteID: Int64 = 123 + + // Simulate adding some failures first + simulateFailureCount(5, for: siteID) + + // When + errorHandler.resetFailureCount(for: siteID) + + // Then - should not flag site as unsupported even after more failures + simulateFailureCount(5, for: siteID) // Would normally reach threshold + XCTAssertFalse(userDefaults.applicationPasswordUnsupportedList.contains(siteID)) + } + + func test_shouldRetryJetpackRequest_returns_false_for_nil_credentials() { + // Given + let errorHandlerWithNilCredentials = AlamofireNetworkErrorHandler(credentials: nil, userDefaults: userDefaults) + let jetpackRequest = createJetpackRequest(siteID: 123) + let restRequest = createRESTRequest() + let error = createNetworkError() + + // When + let shouldRetry = errorHandlerWithNilCredentials.shouldRetryJetpackRequest( + originalRequest: jetpackRequest, + convertedRequest: restRequest, + failure: error + ) + + // Then + XCTAssertFalse(shouldRetry) + } + + func test_shouldRetryJetpackRequest_returns_false_for_non_wpcom_credentials() { + // Given + let wporgCredentials = Credentials.wporg(username: "user", password: "pass", siteAddress: "https://example.com") + let errorHandlerWithWporg = AlamofireNetworkErrorHandler(credentials: wporgCredentials, userDefaults: userDefaults) + let jetpackRequest = createJetpackRequest(siteID: 123) + let restRequest = createRESTRequest() + let error = createNetworkError() + + // When + let shouldRetry = errorHandlerWithWporg.shouldRetryJetpackRequest( + originalRequest: jetpackRequest, + convertedRequest: restRequest, + failure: error + ) + + // Then + XCTAssertFalse(shouldRetry) + } + + func test_shouldRetryJetpackRequest_returns_true_for_expected_errors() { + // Given + let jetpackRequest = createJetpackRequest(siteID: 123) + let restRequest = createRESTRequest() + + let testCases: [Error] = [ + AFError.requestAdaptationFailed(error: NSError(domain: "test", code: 1)), + createNetworkError() + ] + + for error in testCases { + // When + let shouldRetry = errorHandler.shouldRetryJetpackRequest( + originalRequest: jetpackRequest, + convertedRequest: restRequest, + failure: error + ) + + // Then + XCTAssertTrue(shouldRetry, "Should retry for error: \(error)") + } + } + + func test_shouldRetryJetpackRequest_returns_false_for_unexpected_errors() { + // Given + let jetpackRequest = createJetpackRequest(siteID: 123) + let restRequest = createRESTRequest() + let unexpectedError = NSError(domain: "UnexpectedDomain", code: 999) + + // When + let shouldRetry = errorHandler.shouldRetryJetpackRequest( + originalRequest: jetpackRequest, + convertedRequest: restRequest, + failure: unexpectedError + ) + + // Then + XCTAssertFalse(shouldRetry) + } + + func test_flagSiteAsUnsupported_adds_site_to_unsupported_list() { + // Given + let siteID: Int64 = 456 + XCTAssertTrue(userDefaults.applicationPasswordUnsupportedList.isEmpty) + + // When + errorHandler.flagSiteAsUnsupported(for: siteID) + + // Then + XCTAssertEqual(userDefaults.applicationPasswordUnsupportedList, [siteID]) + } + + func test_flagSiteAsUnsupported_appends_to_existing_list() { + // Given + let existingSiteID: Int64 = 789 + let newSiteID: Int64 = 456 + userDefaults.applicationPasswordUnsupportedList = [existingSiteID] + + // When + errorHandler.flagSiteAsUnsupported(for: newSiteID) + + // Then + XCTAssertEqual(userDefaults.applicationPasswordUnsupportedList, [existingSiteID, newSiteID]) + } + + // MARK: - Thread Safety Tests + + func test_concurrent_resetFailureCount_operations_are_thread_safe() { + let expectation = XCTestExpectation(description: "All reset operations complete") + let operationCount = 3 + let siteIDs = Array(1...3).map { Int64($0) } + + expectation.expectedFulfillmentCount = operationCount + + // When - perform many concurrent reset operations + for i in 0.. Credentials { + return Credentials.wpcom(username: "test", authToken: "token", siteAddress: "https://example.com") + } + + func createJetpackRequest(siteID: Int64) -> JetpackRequest { + return JetpackRequest( + wooApiVersion: .mark3, + method: .get, + siteID: siteID, + path: "test", + availableAsRESTRequest: true + ) + } + + func createRESTRequest() -> RESTRequest { + return RESTRequest( + siteURL: "https://example.com", + wooApiVersion: .mark3, + method: .get, + path: "test" + ) + } + + func createNetworkError() -> NetworkError { + return NetworkError.unacceptableStatusCode(statusCode: 500, response: Data()) + } + + func simulateFailureCount(_ count: Int, for siteID: Int64) { + // Simulate multiple failures to reach the count + for _ in 0..