From 4e4a06ef8f72d53141d0abf707a71794bc71925a Mon Sep 17 00:00:00 2001 From: David Date: Tue, 26 Nov 2024 14:37:49 -0800 Subject: [PATCH] Expose status code in retriable error and add more precise logging (#3284) * Expose status code in retriable error and add more precise logging * Bubble up errorMessage from connectionError * Comments * Fix tests * Fix tests * Fix lint * Remove comment --------- Co-authored-by: crow --- .../Engine/AutomationPreparer.swift | 2 +- .../AirshipCore/Source/DeferredResolver.swift | 16 +++++++------- .../Tests/DeferredResolverTest.swift | 22 ++++++++++++------- .../Tests/TestDeferredResolver.swift | 4 ++-- .../Source/DeferredFlagResolver.swift | 14 +++++++++--- .../Source/FeatureFlagManager.swift | 18 ++++++++------- .../FeatureFlagDeferredResolverTest.swift | 12 +++++----- .../Tests/FeatureFlagManagerTest.swift | 18 +++++++-------- 8 files changed, 61 insertions(+), 45 deletions(-) diff --git a/Airship/AirshipAutomation/Source/Automation/Engine/AutomationPreparer.swift b/Airship/AirshipAutomation/Source/Automation/Engine/AutomationPreparer.swift index 40d00f1fd..c43cc7a8c 100644 --- a/Airship/AirshipAutomation/Source/Automation/Engine/AutomationPreparer.swift +++ b/Airship/AirshipAutomation/Source/Automation/Engine/AutomationPreparer.swift @@ -302,7 +302,7 @@ struct AutomationPreparer: AutomationPreparerProtocol { case .notFound: await self.remoteDataAccess.notifyOutdated(schedule: schedule) return .success(result: .invalidate) - case .retriableError(retryAfter: let retryAfter): + case .retriableError(retryAfter: let retryAfter, statusCode: _): if let retryAfter { return .retryAfter(retryAfter) } else { diff --git a/Airship/AirshipCore/Source/DeferredResolver.swift b/Airship/AirshipCore/Source/DeferredResolver.swift index d05481ac1..1cd55b241 100644 --- a/Airship/AirshipCore/Source/DeferredResolver.swift +++ b/Airship/AirshipCore/Source/DeferredResolver.swift @@ -8,7 +8,7 @@ public enum AirshipDeferredResult: Sendable, Equatable { case timedOut case outOfDate case notFound - case retriableError(retryAfter: TimeInterval? = nil) + case retriableError(retryAfter: TimeInterval? = nil, statusCode: Int? = nil) } /// NOTE: For internal use only. :nodoc: @@ -147,7 +147,7 @@ final class AirshipDeferredResolver : AirshipDeferredResolverProtocol { case 200: do { guard let body = result.result else { - return .retriableError() + return .retriableError(statusCode: result.statusCode) } let parsed = try await resultParser(body) @@ -155,8 +155,8 @@ final class AirshipDeferredResolver : AirshipDeferredResolverProtocol { return .success(parsed) } catch { - AirshipLogger.error("Failed ot parse deferred body \(error)") - return .retriableError() + AirshipLogger.error("Failed to parse deferred body \(error) with status code: \(result.statusCode)") + return .retriableError(statusCode: result.statusCode) } case 404: return .notFound case 409: return .outOfDate @@ -164,13 +164,13 @@ final class AirshipDeferredResolver : AirshipDeferredResolverProtocol { if let location = result.locationHeader { locationMap.value[url] = location } - return .retriableError(retryAfter: result.retryAfter) + return .retriableError(retryAfter: result.retryAfter, statusCode: result.statusCode) case 307: if let location = result.locationHeader { locationMap.value[url] = location if let retry = result.retryAfter, retry > 0 { - return .retriableError(retryAfter: retry) + return .retriableError(retryAfter: retry, statusCode: result.statusCode) } if (allowRetry) { @@ -186,9 +186,9 @@ final class AirshipDeferredResolver : AirshipDeferredResolverProtocol { ) } } - return .retriableError(retryAfter: result.retryAfter ?? 0) + return .retriableError(retryAfter: result.retryAfter ?? 0, statusCode: result.statusCode) default: - return .retriableError() + return .retriableError(statusCode: result.statusCode) } } } diff --git a/Airship/AirshipCore/Tests/DeferredResolverTest.swift b/Airship/AirshipCore/Tests/DeferredResolverTest.swift index 4b9e0bba8..5b7ae3dba 100644 --- a/Airship/AirshipCore/Tests/DeferredResolverTest.swift +++ b/Airship/AirshipCore/Tests/DeferredResolverTest.swift @@ -129,16 +129,18 @@ final class DeferredResolverTest: XCTestCase { notificationOptIn: true ) + let statusCode: Int = 200 + let body = "some body".data(using: .utf8) self.client.onResolve = { _, _, _, _, _, _ in - return AirshipHTTPResponse(result: body, statusCode: 200, headers: [:]) + return AirshipHTTPResponse(result: body, statusCode: statusCode, headers: [:]) } let result: AirshipDeferredResult = await resolver.resolve(request: request) { data in throw AirshipErrors.error("parse error") } - XCTAssertEqual(result, .retriableError()) + XCTAssertEqual(result, .retriableError(statusCode: statusCode)) } @@ -210,16 +212,17 @@ final class DeferredResolverTest: XCTestCase { notificationOptIn: true ) + let statusCode: Int = 429 let body = "some body".data(using: .utf8)! self.client.onResolve = { _, _, _, _, _, _ in - return AirshipHTTPResponse(result: body, statusCode: 429, headers: ["Location": self.altExampleURL.absoluteString]) + return AirshipHTTPResponse(result: body, statusCode: statusCode, headers: ["Location": self.altExampleURL.absoluteString]) } let result: AirshipDeferredResult = await resolver.resolve(request: request) { data in return data } - XCTAssertEqual(result, .retriableError()) + XCTAssertEqual(result, .retriableError(statusCode: statusCode)) self.client.onResolve = { url, _, _, _, _, _ in XCTAssertEqual(url, self.altExampleURL) @@ -242,16 +245,17 @@ final class DeferredResolverTest: XCTestCase { notificationOptIn: true ) + let statusCode: Int = 429 let body = "some body".data(using: .utf8)! self.client.onResolve = { _, _, _, _, _, _ in - return AirshipHTTPResponse(result: body, statusCode: 429, headers: ["Retry-After": "100.0"]) + return AirshipHTTPResponse(result: body, statusCode: statusCode, headers: ["Retry-After": "100.0"]) } let result: AirshipDeferredResult = await resolver.resolve(request: request) { data in return data } - XCTAssertEqual(result, .retriableError(retryAfter: 100.0)) + XCTAssertEqual(result, .retriableError(retryAfter: 100.0, statusCode: statusCode)) } func testResolve307() async throws { @@ -287,11 +291,13 @@ final class DeferredResolverTest: XCTestCase { ) let body = "some body".data(using: .utf8)! + + let statusCode: Int = 307 self.client.onResolve = { url, _, _, _, _, _ in if (url == self.exampleURL) { return AirshipHTTPResponse( result: nil, - statusCode: 307, + statusCode: statusCode, headers: [ "Location": self.altExampleURL.absoluteString, "Retry-After": "20.0" @@ -306,7 +312,7 @@ final class DeferredResolverTest: XCTestCase { return data } - XCTAssertEqual(result, .retriableError(retryAfter: 20.0)) + XCTAssertEqual(result, .retriableError(retryAfter: 20.0, statusCode: statusCode)) let anotherResult: AirshipDeferredResult = await resolver.resolve(request: request) { data in return data diff --git a/Airship/AirshipCore/Tests/TestDeferredResolver.swift b/Airship/AirshipCore/Tests/TestDeferredResolver.swift index 925540eae..de5b3b3d1 100644 --- a/Airship/AirshipCore/Tests/TestDeferredResolver.swift +++ b/Airship/AirshipCore/Tests/TestDeferredResolver.swift @@ -18,12 +18,12 @@ final class TestDeferredResolver: AirshipDeferredResolverProtocol, @unchecked Se let value = try await resultParser(data) return .success(value) } catch { - return .retriableError() + return .retriableError(statusCode: 200) } case .timedOut: return .timedOut case .outOfDate: return .outOfDate case .notFound: return .notFound - case .retriableError(retryAfter: let retryAfter): return .retriableError(retryAfter: retryAfter) + case .retriableError(retryAfter: let retryAfter, statusCode: let statusCode): return .retriableError(retryAfter: retryAfter, statusCode:statusCode) } } } diff --git a/Airship/AirshipFeatureFlags/Source/DeferredFlagResolver.swift b/Airship/AirshipFeatureFlags/Source/DeferredFlagResolver.swift index f48a328ec..351a79958 100644 --- a/Airship/AirshipFeatureFlags/Source/DeferredFlagResolver.swift +++ b/Airship/AirshipFeatureFlags/Source/DeferredFlagResolver.swift @@ -103,18 +103,26 @@ actor FeatureFlagDeferredResolver: FeatureFlagDeferredResolverProtocol { case .notFound: return .notFound - case .retriableError(let retryAfter): + case .retriableError(let retryAfter, let statusCode): let backoff = retryAfter ?? FeatureFlagDeferredResolver.defaultBackoff guard allowRetry, backoff <= FeatureFlagDeferredResolver.immediateBackoffRetryCutoff else { backOffDates[requestID] = self.date.now.advanced(by: backoff) - throw FeatureFlagEvaluationError.connectionError + if let statusCode = statusCode { + throw FeatureFlagEvaluationError.connectionError(errorMessage: "Failed to resolve flag. Status code: \(statusCode)") + } + + throw FeatureFlagEvaluationError.connectionError(errorMessage: "Failed to resolve flag.") } if (backoff > 0) { + AirshipLogger.debug(statusCode == nil ? "Backing off deferred flag request \(requestID) for \(backoff) seconds" : "Backing off deferred flag request \(requestID) for \(backoff) seconds with status code: \(statusCode!)") + try await self.taskSleeper.sleep(timeInterval: backoff) } + AirshipLogger.error(statusCode == nil ? "Retrying deferred flag request \(requestID)" : "Retrying deferred flag request \(requestID) with status code: \(statusCode!)") + return try await self.fetchFlag( request: request, requestID: requestID, @@ -126,7 +134,7 @@ actor FeatureFlagDeferredResolver: FeatureFlagDeferredResolverProtocol { throw FeatureFlagEvaluationError.outOfDate default: - throw FeatureFlagEvaluationError.connectionError + throw FeatureFlagEvaluationError.connectionError(errorMessage: "Failed to resolve flag.") } } } diff --git a/Airship/AirshipFeatureFlags/Source/FeatureFlagManager.swift b/Airship/AirshipFeatureFlags/Source/FeatureFlagManager.swift index 193d1daf0..8ff7ef0e9 100644 --- a/Airship/AirshipFeatureFlags/Source/FeatureFlagManager.swift +++ b/Airship/AirshipFeatureFlags/Source/FeatureFlagManager.swift @@ -9,15 +9,17 @@ import AirshipCore /// Feature flag errors -public enum FeatureFlagError: Error { - /// Failed to fetch the data for the feature flag. The app should retry on this error. +public enum FeatureFlagError: Error, Equatable { case failedToFetchData + case staleData + case outOfDate + case connectionError(errorMessage: String) } -enum FeatureFlagEvaluationError: Error { - case connectionError +enum FeatureFlagEvaluationError: Error, Equatable { case outOfDate case staleNotAllowed + case connectionError(errorMessage: String) } /// Airship feature flag manager @@ -92,8 +94,8 @@ public final class FeatureFlagManager: Sendable { ) } catch { switch (error) { - case FeatureFlagEvaluationError.connectionError: - throw FeatureFlagError.failedToFetchData + case FeatureFlagEvaluationError.connectionError(let errorMessage): + throw FeatureFlagError.connectionError(errorMessage: errorMessage) case FeatureFlagEvaluationError.outOfDate: await self.remoteDataAccess.notifyOutdated( remoteDateInfo: remoteDataFeatureFlagInfo.remoteDataInfo @@ -103,14 +105,14 @@ public final class FeatureFlagManager: Sendable { await self.remoteDataAccess.waitForRefresh() return try await self.flag(name: name, allowRefresh: false) } - throw FeatureFlagError.failedToFetchData + throw FeatureFlagError.outOfDate case FeatureFlagEvaluationError.staleNotAllowed: if (allowRefresh) { await self.remoteDataAccess.waitForRefresh() return try await self.flag(name: name, allowRefresh: false) } - throw FeatureFlagError.failedToFetchData + throw FeatureFlagError.staleData default: AirshipLogger.error("Unexpected error \(error)") throw FeatureFlagError.failedToFetchData diff --git a/Airship/AirshipFeatureFlags/Tests/FeatureFlagDeferredResolverTest.swift b/Airship/AirshipFeatureFlags/Tests/FeatureFlagDeferredResolverTest.swift index 256e3e2ca..c5aee29ee 100644 --- a/Airship/AirshipFeatureFlags/Tests/FeatureFlagDeferredResolverTest.swift +++ b/Airship/AirshipFeatureFlags/Tests/FeatureFlagDeferredResolverTest.swift @@ -167,7 +167,7 @@ final class FeatureFlagDeferredResolverTest: XCTestCase { ) XCTFail() } catch { - XCTAssertEqual(FeatureFlagEvaluationError.connectionError, error as! FeatureFlagEvaluationError) + XCTAssertEqual(FeatureFlagEvaluationError.connectionError(errorMessage: "Failed to resolve flag."), error as! FeatureFlagEvaluationError) } await fulfillment(of: [expectation]) @@ -177,7 +177,7 @@ final class FeatureFlagDeferredResolverTest: XCTestCase { let expectation = expectation(description: "flag resolved") self.deferredResolver.onData = { _ in expectation.fulfill() - return .retriableError() + return .retriableError(statusCode: nil) } do { @@ -187,7 +187,7 @@ final class FeatureFlagDeferredResolverTest: XCTestCase { ) XCTFail() } catch { - XCTAssertEqual(FeatureFlagEvaluationError.connectionError, error as! FeatureFlagEvaluationError) + XCTAssertEqual(FeatureFlagEvaluationError.connectionError(errorMessage: "Failed to resolve flag."), error as! FeatureFlagEvaluationError) } await fulfillment(of: [expectation]) @@ -210,7 +210,7 @@ final class FeatureFlagDeferredResolverTest: XCTestCase { ) XCTFail() } catch { - XCTAssertEqual(FeatureFlagEvaluationError.connectionError, error as! FeatureFlagEvaluationError) + XCTAssertEqual(FeatureFlagEvaluationError.connectionError(errorMessage: "Failed to resolve flag."), error as! FeatureFlagEvaluationError) } await fulfillment(of: [expectation]) @@ -232,7 +232,7 @@ final class FeatureFlagDeferredResolverTest: XCTestCase { ) XCTFail() } catch { - XCTAssertEqual(FeatureFlagEvaluationError.connectionError, error as! FeatureFlagEvaluationError) + XCTAssertEqual(FeatureFlagEvaluationError.connectionError(errorMessage: "Failed to resolve flag."), error as! FeatureFlagEvaluationError) } await fulfillment(of: [expecation]) @@ -371,7 +371,7 @@ fileprivate final class TestDeferredResolver: AirshipDeferredResolverProtocol, @ case .timedOut: return .timedOut case .outOfDate: return .outOfDate case .notFound: return .notFound - case .retriableError(retryAfter: let retryAfter): return .retriableError(retryAfter: retryAfter) + case .retriableError(retryAfter: let retryAfter, statusCode: let statusCode): return .retriableError(retryAfter: retryAfter, statusCode: statusCode) @unknown default: fatalError() } diff --git a/Airship/AirshipFeatureFlags/Tests/FeatureFlagManagerTest.swift b/Airship/AirshipFeatureFlags/Tests/FeatureFlagManagerTest.swift index 6217aaac9..7c19e8b86 100644 --- a/Airship/AirshipFeatureFlags/Tests/FeatureFlagManagerTest.swift +++ b/Airship/AirshipFeatureFlags/Tests/FeatureFlagManagerTest.swift @@ -838,10 +838,10 @@ final class AirshipFeatureFlagsTest: XCTestCase { do { let _ = try await featureFlagManager.flag(name: "foo") XCTFail("Should throw") - } catch FeatureFlagError.failedToFetchData { + } catch FeatureFlagError.staleData { // No-op } catch { - XCTFail("Should throw failedToFetchData") + XCTFail("Should throw staleData") } } @@ -881,10 +881,10 @@ final class AirshipFeatureFlagsTest: XCTestCase { do { let _ = try await featureFlagManager.flag(name: "foo") XCTFail("Should throw") - } catch FeatureFlagError.failedToFetchData { + }catch FeatureFlagError.staleData { // No-op } catch { - XCTFail("Should throw failedToFetchData") + XCTFail("Should throw staleData") } } @@ -894,10 +894,10 @@ final class AirshipFeatureFlagsTest: XCTestCase { do { let _ = try await featureFlagManager.flag(name: "foo") XCTFail("Should throw") - } catch FeatureFlagError.failedToFetchData { + } catch FeatureFlagError.outOfDate { // No-op } catch { - XCTFail("Should throw failedToFetchData") + XCTFail("Should throw outOfDate") } } @@ -1223,7 +1223,7 @@ final class AirshipFeatureFlagsTest: XCTestCase { do { _ = try await featureFlagManager.flag(name: "foo") } catch { - XCTAssertEqual(error as! FeatureFlagError, FeatureFlagError.failedToFetchData) + XCTAssertEqual(error as! FeatureFlagError, FeatureFlagError.outOfDate) } XCTAssertEqual(remoteDataAccess.lastOutdatedRemoteInfo, self.remoteDataAccess.remoteDataInfo) @@ -1259,13 +1259,13 @@ final class AirshipFeatureFlagsTest: XCTestCase { } await self.deferredResolver.setOnResolve { _, _ in - throw FeatureFlagEvaluationError.connectionError + throw FeatureFlagEvaluationError.connectionError(errorMessage: "Failed to resolve flag.") } do { _ = try await featureFlagManager.flag(name: "foo") } catch { - XCTAssertEqual(error as! FeatureFlagError, FeatureFlagError.failedToFetchData) + XCTAssertEqual(error as! FeatureFlagError, FeatureFlagError.connectionError(errorMessage: "Failed to resolve flag.")) } XCTAssertNil(remoteDataAccess.lastOutdatedRemoteInfo)