From b6f5bc90627ca5b7673c8f1e9598e41dbbf5c994 Mon Sep 17 00:00:00 2001 From: Sebastian Villena <97059974+ruisebas@users.noreply.github.com> Date: Tue, 24 Oct 2023 13:48:39 -0400 Subject: [PATCH] fix(Analytics): Handling certain auth errors as retryable errors (#3322) --- .../Support/Utils/AnalyticsErrorHelper.swift | 2 + .../Analytics/EventRecorder.swift | 64 +++++++-- .../EventRecorderTests.swift | 134 ++++++++++++++++++ .../Mocks/MockAnalyticsEventStorage.swift | 3 + .../Mocks/MockPinpointClient.swift | 5 +- 5 files changed, 195 insertions(+), 13 deletions(-) diff --git a/AmplifyPlugins/Analytics/Sources/AWSPinpointAnalyticsPlugin/Support/Utils/AnalyticsErrorHelper.swift b/AmplifyPlugins/Analytics/Sources/AWSPinpointAnalyticsPlugin/Support/Utils/AnalyticsErrorHelper.swift index 684289ca1b..da6a74fe7b 100644 --- a/AmplifyPlugins/Analytics/Sources/AWSPinpointAnalyticsPlugin/Support/Utils/AnalyticsErrorHelper.swift +++ b/AmplifyPlugins/Analytics/Sources/AWSPinpointAnalyticsPlugin/Support/Utils/AnalyticsErrorHelper.swift @@ -14,6 +14,8 @@ enum AnalyticsErrorHelper { switch error { case let error as AnalyticsErrorConvertible: return error.analyticsError + case let error as AuthError: + return .configuration(error.errorDescription, error.recoverySuggestion, error) default: return getDefaultError(error as NSError) } diff --git a/AmplifyPlugins/Internal/Sources/InternalAWSPinpoint/Analytics/EventRecorder.swift b/AmplifyPlugins/Internal/Sources/InternalAWSPinpoint/Analytics/EventRecorder.swift index 5c2675cdbe..21ee1cc88e 100644 --- a/AmplifyPlugins/Internal/Sources/InternalAWSPinpoint/Analytics/EventRecorder.swift +++ b/AmplifyPlugins/Internal/Sources/InternalAWSPinpoint/Analytics/EventRecorder.swift @@ -6,6 +6,7 @@ // import Amplify +import AWSCognitoAuthPlugin import AWSPinpoint import ClientRuntime import enum AwsCommonRuntimeKit.CommonRunTimeError @@ -186,25 +187,61 @@ class EventRecorder: AnalyticsEventRecording { } catch let analyticsError as AnalyticsError { // This is a known error explicitly thrown inside the do/catch block, so just rethrow it so it can be handled by the consumer throw analyticsError + } catch let authError as AuthError { + // This means all events were rejected due to an Auth error + log.error("Unable to submit \(pinpointEvents.count) events. Error: \(authError.errorDescription). \(authError.recoverySuggestion)") + switch authError { + case .signedOut, + .sessionExpired: + // Session Expired and Signed Out errors should be retried indefinitely, so we won't update the database + log.verbose("Events will be retried") + case .service: + if case .invalidAccountTypeException = authError.underlyingError as? AWSCognitoAuthError { + // Unsupported Guest Access errors should be retried indefinitely, so we won't update the database + log.verbose("Events will be retried") + } else { + fallthrough + } + default: + if let underlyingError = authError.underlyingError { + // Handle the underlying error accordingly + handleError(underlyingError, for: pinpointEvents) + } else { + // Otherwise just mark all events as dirty + log.verbose("Events will be discarded") + markEventsAsDirty(pinpointEvents) + } + } + + // Rethrow the original error so it can be handled by the consumer + throw authError } catch { // This means all events were rejected - if isConnectivityError(error) { - // Connectivity errors should be retried indefinitely, so we won't update the database - log.error("Unable to submit \(pinpointEvents.count) events. Error: \(AWSPinpointErrorConstants.deviceOffline.errorDescription)") - } else if isErrorRetryable(error) { - // For retryable errors, increment the events retry count - log.error("Unable to submit \(pinpointEvents.count) events. Error: \(errorDescription(error)).") - incrementRetryCounter(for: pinpointEvents) - } else { - // For remaining errors, mark events as dirty - log.error("Server rejected the submission of \(pinpointEvents.count) events. Error: \(errorDescription(error)).") - markEventsAsDirty(pinpointEvents) - } + log.error("Unable to submit \(pinpointEvents.count) events. Error: \(errorDescription(error)).") + handleError(error, for: pinpointEvents) // Rethrow the original error so it can be handled by the consumer throw error } } + + private func handleError(_ error: Error, for pinpointEvents: [PinpointEvent]) { + if isConnectivityError(error) { + // Connectivity errors should be retried indefinitely, so we won't update the database + log.verbose("Events will be retried") + return + } + + if isErrorRetryable(error) { + // For retryable errors, increment the events retry count + log.verbose("Events' retry count will be increased") + incrementRetryCounter(for: pinpointEvents) + } else { + // For remaining errors, mark events as dirty + log.verbose("Events will be discarded") + markEventsAsDirty(pinpointEvents) + } + } private func isErrorRetryable(_ error: Error) -> Bool { guard case let modeledError as ModeledError = error else { @@ -214,6 +251,9 @@ class EventRecorder: AnalyticsEventRecording { } private func errorDescription(_ error: Error) -> String { + if isConnectivityError(error) { + return AWSPinpointErrorConstants.deviceOffline.errorDescription + } switch error { case let error as ModeledErrorDescribable: return error.errorDescription diff --git a/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/EventRecorderTests.swift b/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/EventRecorderTests.swift index 04452948b9..004860b037 100644 --- a/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/EventRecorderTests.swift +++ b/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/EventRecorderTests.swift @@ -7,7 +7,9 @@ import XCTest import AWSPinpoint +import AwsCommonRuntimeKit @testable import Amplify +import ClientRuntime @_spi(InternalAWSPinpoint) @testable import InternalAWSPinpoint class EventRecorderTests: XCTestCase { @@ -26,6 +28,13 @@ class EventRecorderTests: XCTestCase { XCTFail("Failed to setup EventRecorderTests") } } + + override func tearDown() { + pinpointClient = nil + endpointClient = nil + storage = nil + recorder = nil + } /// - Given: a event recorder /// - When: instance is constructed @@ -56,4 +65,129 @@ class EventRecorderTests: XCTestCase { XCTAssertEqual(event, storage.events[0]) XCTAssertEqual(storage.checkDiskSizeCallCount, 2) } + + /// - Given: a event recorder with events saved in the local storage + /// - When: submitAllEvents is invoked and successful + /// - Then: the events are removed from the local storage + func testSubmitAllEvents_withSuccess_shouldRemoveEventsFromStorage() async throws { + Amplify.Logging.logLevel = .verbose + let session = PinpointSession(sessionId: "1", startTime: Date(), stopTime: nil) + storage.events = [ + .init(id: "1", eventType: "eventType1", eventDate: Date(), session: session), + .init(id: "2", eventType: "eventType2", eventDate: Date(), session: session) + ] + + pinpointClient.putEventsResult = .success(.init(eventsResponse: .init(results: [ + "endpointId": PinpointClientTypes.ItemResponse( + endpointItemResponse: .init(message: "Accepted", statusCode: 202), + eventsItemResponse: [ + "1": .init(message: "Accepted", statusCode: 202), + "2": .init(message: "Accepted", statusCode: 202) + ] + ) + ]))) + let events = try await recorder.submitAllEvents() + + XCTAssertEqual(events.count, 2) + XCTAssertEqual(pinpointClient.putEventsCount, 1) + XCTAssertTrue(storage.events.isEmpty) + XCTAssertEqual(storage.deleteEventCallCount, 2) + } + + /// - Given: a event recorder with events saved in the local storage + /// - When: submitAllEvents is invoked and fails with a non-retryable error + /// - Then: the events are marked as dirty + func testSubmitAllEvents_withRetryableError_shouldSetEventsAsDirty() async throws { + Amplify.Logging.logLevel = .verbose + let session = PinpointSession(sessionId: "1", startTime: Date(), stopTime: nil) + let event1 = PinpointEvent(id: "1", eventType: "eventType1", eventDate: Date(), session: session) + let event2 = PinpointEvent(id: "2", eventType: "eventType2", eventDate: Date(), session: session) + storage.events = [ event1, event2 ] + pinpointClient.putEventsResult = .failure(NonRetryableError()) + do { + let events = try await recorder.submitAllEvents() + XCTFail("Expected error") + } catch { + XCTAssertEqual(pinpointClient.putEventsCount, 1) + XCTAssertEqual(storage.events.count, 2) + XCTAssertEqual(storage.deleteEventCallCount, 0) + XCTAssertEqual(storage.eventRetryDictionary.count, 0) + XCTAssertEqual(storage.dirtyEventDictionary.count, 2) + XCTAssertEqual(storage.dirtyEventDictionary["1"], 1) + XCTAssertEqual(storage.dirtyEventDictionary["2"], 1) + } + } + + /// - Given: a event recorder with events saved in the local storage + /// - When: submitAllEvents is invoked and fails with a retryable error + /// - Then: the events' retry count is increased + func testSubmitAllEvents_withRetryableError_shouldIncreaseRetryCount() async throws { + Amplify.Logging.logLevel = .verbose + let session = PinpointSession(sessionId: "1", startTime: Date(), stopTime: nil) + let event1 = PinpointEvent(id: "1", eventType: "eventType1", eventDate: Date(), session: session) + let event2 = PinpointEvent(id: "2", eventType: "eventType2", eventDate: Date(), session: session) + storage.events = [ event1, event2 ] + pinpointClient.putEventsResult = .failure(RetryableError()) + do { + let events = try await recorder.submitAllEvents() + XCTFail("Expected error") + } catch { + XCTAssertEqual(pinpointClient.putEventsCount, 1) + XCTAssertEqual(storage.events.count, 2) + XCTAssertEqual(storage.deleteEventCallCount, 0) + XCTAssertEqual(storage.eventRetryDictionary.count, 2) + XCTAssertEqual(storage.eventRetryDictionary["1"], 1) + XCTAssertEqual(storage.eventRetryDictionary["2"], 1) + XCTAssertEqual(storage.dirtyEventDictionary.count, 0) + } + } + + /// - Given: a event recorder with events saved in the local storage + /// - When: submitAllEvents is invoked and fails with a connectivity error + /// - Then: the events are not removed from the local storage + func testSubmitAllEvents_withConnectivityError_shouldNotIncreaseRetryCount_andNotSetEventsAsDirty() async throws { + Amplify.Logging.logLevel = .verbose + let session = PinpointSession(sessionId: "1", startTime: Date(), stopTime: nil) + let event1 = PinpointEvent(id: "1", eventType: "eventType1", eventDate: Date(), session: session) + let event2 = PinpointEvent(id: "2", eventType: "eventType2", eventDate: Date(), session: session) + storage.events = [ event1, event2 ] + pinpointClient.putEventsResult = .failure(ConnectivityError()) + do { + let events = try await recorder.submitAllEvents() + XCTFail("Expected error") + } catch { + XCTAssertEqual(pinpointClient.putEventsCount, 1) + XCTAssertEqual(storage.events.count, 2) + XCTAssertEqual(storage.deleteEventCallCount, 0) + XCTAssertEqual(storage.eventRetryDictionary.count, 0) + XCTAssertEqual(storage.dirtyEventDictionary.count, 0) + } + } +} + +private struct RetryableError: Error, ModeledError { + static var typeName = "RetriableError" + static var fault = ErrorFault.client + static var isRetryable = true + static var isThrottling = false +} + +private struct NonRetryableError: Error, ModeledError { + static var typeName = "RetriableError" + static var fault = ErrorFault.client + static var isRetryable = false + static var isThrottling = false +} + +private class ConnectivityError: NSError { + init() { + super.init( + domain: "ConnectivityError", + code: NSURLErrorNotConnectedToInternet + ) + } + + required init?(coder: NSCoder) { + super.init(coder: coder) + } } diff --git a/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockAnalyticsEventStorage.swift b/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockAnalyticsEventStorage.swift index 3b65a2de58..97d0a99d72 100644 --- a/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockAnalyticsEventStorage.swift +++ b/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockAnalyticsEventStorage.swift @@ -9,6 +9,7 @@ class MockAnalyticsEventStorage: AnalyticsEventStorage { var deletedEvent: String = "" + var deleteEventCallCount = 0 var deleteDirtyEventCallCount = 0 var initializeStorageCallCount = 0 var deleteOldestEventCallCount = 0 @@ -22,6 +23,8 @@ class MockAnalyticsEventStorage: AnalyticsEventStorage { func deleteEvent(eventId: String) throws { deletedEvent = eventId + deleteEventCallCount += 1 + events.removeAll { $0.id == eventId } } func deleteDirtyEvents() throws { diff --git a/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockPinpointClient.swift b/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockPinpointClient.swift index 9abb1ccb88..1069db5214 100644 --- a/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockPinpointClient.swift +++ b/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockPinpointClient.swift @@ -365,8 +365,11 @@ class MockPinpointClient: PinpointClientProtocol { fatalError("Not supported") } + var putEventsCount = 0 + var putEventsResult: Result = .failure(CancellationError()) func putEvents(input: PutEventsInput) async throws -> PutEventsOutputResponse { - fatalError("Not supported") + putEventsCount += 1 + return try putEventsResult.get() } func putEventStream(input: PutEventStreamInput) async throws -> PutEventStreamOutputResponse {