From 04e37c2f46e3cfa275b142fdf6b7c507edb78d08 Mon Sep 17 00:00:00 2001 From: Noah Durell Date: Sat, 23 Mar 2024 12:54:23 -0400 Subject: [PATCH 1/8] use retry after instead of calculating delay --- .../APIRequestErrorHandling.swift | 4 ++-- Sources/KlaviyoSwift/KlaviyoAPI.swift | 5 ++-- .../APIRequestErrorHandlingTests.swift | 23 +++++++++++++++++-- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/Sources/KlaviyoSwift/APIRequestErrorHandling.swift b/Sources/KlaviyoSwift/APIRequestErrorHandling.swift index 9b20cd59..f055e050 100644 --- a/Sources/KlaviyoSwift/APIRequestErrorHandling.swift +++ b/Sources/KlaviyoSwift/APIRequestErrorHandling.swift @@ -100,7 +100,7 @@ func handleRequestError( environment.emitDeveloperWarning("Invalid data supplied for request. Skipping.") return .deQueueCompletedResults(request) - case .rateLimitError: + case let .rateLimitError(retryAfter): var requestRetryCount = 0 var totalRetryCount = 0 var nextBackoff = 0 @@ -112,7 +112,7 @@ func handleRequestError( case let .retryWithBackoff(requestCount, totalCount, _): requestRetryCount = requestCount + 1 totalRetryCount = totalCount + 1 - nextBackoff = getDelaySeconds(for: totalRetryCount) + nextBackoff = retryAfter ?? getDelaySeconds(for: totalRetryCount) } return .requestFailed( request, .retryWithBackoff( diff --git a/Sources/KlaviyoSwift/KlaviyoAPI.swift b/Sources/KlaviyoSwift/KlaviyoAPI.swift index 17f09ea1..570e9e32 100644 --- a/Sources/KlaviyoSwift/KlaviyoAPI.swift +++ b/Sources/KlaviyoSwift/KlaviyoAPI.swift @@ -22,7 +22,7 @@ struct KlaviyoAPI { enum KlaviyoAPIError: Error { case httpError(Int, Data) - case rateLimitError + case rateLimitError(Int?) case missingOrInvalidResponse(URLResponse?) case networkError(Error) case internalError(String) @@ -70,7 +70,8 @@ struct KlaviyoAPI { if httpResponse.statusCode == 429 { requestRateLimited(request) - return .failure(KlaviyoAPIError.rateLimitError) + let retryAfter = httpResponse.allHeaderFields["Retry-After"] as? Int + return .failure(KlaviyoAPIError.rateLimitError(retryAfter)) } guard 200..<300 ~= httpResponse.statusCode else { diff --git a/Tests/KlaviyoSwiftTests/APIRequestErrorHandlingTests.swift b/Tests/KlaviyoSwiftTests/APIRequestErrorHandlingTests.swift index dcadff38..445abc85 100644 --- a/Tests/KlaviyoSwiftTests/APIRequestErrorHandlingTests.swift +++ b/Tests/KlaviyoSwiftTests/APIRequestErrorHandlingTests.swift @@ -254,7 +254,7 @@ class APIRequestErrorHandlingTests: XCTestCase { initialState.requestsInFlight = [request] let store = TestStore(initialState: initialState, reducer: KlaviyoReducer()) - environment.analytics.klaviyoAPI.send = { _ in .failure(.rateLimitError) } + environment.analytics.klaviyoAPI.send = { _ in .failure(.rateLimitError(nil)) } _ = await store.send(.sendRequest) @@ -273,7 +273,7 @@ class APIRequestErrorHandlingTests: XCTestCase { initialState.requestsInFlight = [request] let store = TestStore(initialState: initialState, reducer: KlaviyoReducer()) - environment.analytics.klaviyoAPI.send = { _ in .failure(.rateLimitError) } + environment.analytics.klaviyoAPI.send = { _ in .failure(.rateLimitError(nil)) } _ = await store.send(.sendRequest) @@ -285,6 +285,25 @@ class APIRequestErrorHandlingTests: XCTestCase { } } + func testRetryWithRetryAfter() async throws { + var initialState = INITIALIZED_TEST_STATE() + initialState.retryInfo = .retryWithBackoff(requestCount: 3, totalRetryCount: 3, currentBackoff: 4) + let request = initialState.buildProfileRequest(apiKey: initialState.apiKey!, anonymousId: initialState.anonymousId!) + initialState.requestsInFlight = [request] + let store = TestStore(initialState: initialState, reducer: KlaviyoReducer()) + + environment.analytics.klaviyoAPI.send = { _ in .failure(.rateLimitError(20)) } + + _ = await store.send(.sendRequest) + + await store.receive(.requestFailed(request, .retryWithBackoff(requestCount: 4, totalRetryCount: 4, currentBackoff: 20)), timeout: TIMEOUT_NANOSECONDS) { + $0.flushing = false + $0.queue = [request] + $0.requestsInFlight = [] + $0.retryInfo = .retryWithBackoff(requestCount: 4, totalRetryCount: 4, currentBackoff: 20) + } + } + // MARK: - Missing or invalid response func testMissingOrInvalidResponse() async throws { From ebde9a6bf94e7cf92a98bd0d3d314366c5fd8cc4 Mon Sep 17 00:00:00 2001 From: Ajay Subramanya Date: Fri, 29 Mar 2024 14:22:59 -0500 Subject: [PATCH 2/8] added empty check for email, phone and external id --- Sources/KlaviyoSwift/KlaviyoState.swift | 11 ++++++---- .../StateManagementEdgeCaseTests.swift | 21 +++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/Sources/KlaviyoSwift/KlaviyoState.swift b/Sources/KlaviyoSwift/KlaviyoState.swift index 5791c23d..5e03431d 100644 --- a/Sources/KlaviyoSwift/KlaviyoState.swift +++ b/Sources/KlaviyoSwift/KlaviyoState.swift @@ -120,25 +120,28 @@ struct KlaviyoState: Equatable, Codable { } mutating func updateEmail(email: String) { - guard email != self.email else { + if email.isEmpty || email == self.email { return } + self.email = email enqueueProfileOrTokenRequest() } mutating func updateExternalId(externalId: String) { - guard externalId != self.externalId else { + if externalId.isEmpty || externalId == self.externalId { return } + self.externalId = externalId enqueueProfileOrTokenRequest() } mutating func updatePhoneNumber(phoneNumber: String) { - guard phoneNumber != self.phoneNumber else { + if phoneNumber.isEmpty || phoneNumber == self.phoneNumber { return } + self.phoneNumber = phoneNumber enqueueProfileOrTokenRequest() } @@ -190,7 +193,7 @@ struct KlaviyoState: Equatable, Codable { } var attributes = profile.data.attributes var location = profile.data.attributes.location ?? .init() - var properties = profile.data.attributes.properties.value as? [String: Any] ?? [:] + let properties = profile.data.attributes.properties.value as? [String: Any] ?? [:] let updatedProfile = Profile.updateProfileWithProperties(dict: pendingProfile) if let firstName = updatedProfile.firstName { diff --git a/Tests/KlaviyoSwiftTests/StateManagementEdgeCaseTests.swift b/Tests/KlaviyoSwiftTests/StateManagementEdgeCaseTests.swift index 137d5911..fd316df0 100644 --- a/Tests/KlaviyoSwiftTests/StateManagementEdgeCaseTests.swift +++ b/Tests/KlaviyoSwiftTests/StateManagementEdgeCaseTests.swift @@ -144,6 +144,13 @@ class StateManagementEdgeCaseTests: XCTestCase { } } + func testSetEmptyEmail() async throws { + let initialState = INITIALIZED_TEST_STATE() + let store = TestStore(initialState: initialState, reducer: KlaviyoReducer()) + + _ = await store.send(.setEmail("")) + } + // MARK: - Set External Id func testSetExternalIdUninitialized() async throws { @@ -173,6 +180,13 @@ class StateManagementEdgeCaseTests: XCTestCase { } } + func testSetEmptyExternalId() async throws { + let initialState = INITIALIZED_TEST_STATE() + let store = TestStore(initialState: initialState, reducer: KlaviyoReducer()) + + _ = await store.send(.setExternalId("")) + } + // MARK: - Set Phone number func testSetPhoneNumberUninitialized() async throws { @@ -201,6 +215,13 @@ class StateManagementEdgeCaseTests: XCTestCase { } } + func testSetEmptyPhoneNumber() async throws { + let initialState = INITIALIZED_TEST_STATE() + let store = TestStore(initialState: initialState, reducer: KlaviyoReducer()) + + _ = await store.send(.setPhoneNumber("")) + } + // MARK: - Set Push Token func testSetPushTokenUninitialized() async throws { From e7326bf317a5dda5672d95ad6c7b608f1b29605d Mon Sep 17 00:00:00 2001 From: Ajay Subramanya Date: Mon, 1 Apr 2024 21:51:39 -0500 Subject: [PATCH 3/8] updated to filter out empty email, phone and external id when making token or profile request --- Sources/KlaviyoSwift/KlaviyoState.swift | 14 +++++++--- Sources/KlaviyoSwift/StateManagement.swift | 19 +++++++++++-- .../StateManagementEdgeCaseTests.swift | 27 +++++++++++++++++++ 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/Sources/KlaviyoSwift/KlaviyoState.swift b/Sources/KlaviyoSwift/KlaviyoState.swift index 5e03431d..1558df33 100644 --- a/Sources/KlaviyoSwift/KlaviyoState.swift +++ b/Sources/KlaviyoSwift/KlaviyoState.swift @@ -182,9 +182,17 @@ struct KlaviyoState: Equatable, Codable { } mutating func updateStateWithProfile(profile: Profile) { - email = profile.email ?? email - phoneNumber = profile.phoneNumber ?? phoneNumber - externalId = profile.externalId ?? externalId + if let profileEmail = profile.email, !profileEmail.isEmpty, profileEmail != email { + email = profileEmail + } + + if let profilePhoneNumber = profile.phoneNumber, !profilePhoneNumber.isEmpty, profilePhoneNumber != phoneNumber { + phoneNumber = profilePhoneNumber + } + + if let profileExternalId = profile.externalId, !profileExternalId.isEmpty, profileExternalId != externalId { + externalId = profileExternalId + } } mutating func updateRequestAndStateWithPendingProfile(profile: CreateProfilePayload) -> CreateProfilePayload { diff --git a/Sources/KlaviyoSwift/StateManagement.swift b/Sources/KlaviyoSwift/StateManagement.swift index cf576b6d..a4e94ee0 100644 --- a/Sources/KlaviyoSwift/StateManagement.swift +++ b/Sources/KlaviyoSwift/StateManagement.swift @@ -416,13 +416,15 @@ struct KlaviyoReducer: ReducerProtocol { pushToken: tokenData.pushToken, enablement: tokenData.pushEnablement.rawValue, background: tokenData.pushBackground.rawValue, - profile: profile, + profile: profile.profile(from: state), anonymousId: anonymousId) )) } else { request = KlaviyoAPI.KlaviyoRequest( apiKey: apiKey, - endpoint: .createProfile(.init(data: .init(profile: profile, anonymousId: anonymousId)))) + endpoint: .createProfile( + .init(data: .init(profile: profile.profile(from: state), anonymousId: anonymousId)) + )) } state.enqueueRequest(request: request) @@ -485,3 +487,16 @@ extension Event { uniqueId: uniqueId) } } + +extension Profile { + fileprivate func profile(from state: KlaviyoState) -> Profile { + Profile( + email: state.email, + phoneNumber: state.phoneNumber, + externalId: state.externalId, + firstName: firstName, + image: image, + location: location, + properties: properties) + } +} diff --git a/Tests/KlaviyoSwiftTests/StateManagementEdgeCaseTests.swift b/Tests/KlaviyoSwiftTests/StateManagementEdgeCaseTests.swift index fd316df0..35c53af4 100644 --- a/Tests/KlaviyoSwiftTests/StateManagementEdgeCaseTests.swift +++ b/Tests/KlaviyoSwiftTests/StateManagementEdgeCaseTests.swift @@ -354,4 +354,31 @@ class StateManagementEdgeCaseTests: XCTestCase { _ = await store.send(.enqueueProfile(profile)) await fulfillment(of: [expection]) } + + func testSetProfileWithEmptyStringIdentifiers() async throws { + let initialState = KlaviyoState( + apiKey: TEST_API_KEY, + email: "foo@bar.com", + anonymousId: environment.analytics.uuid().uuidString, + phoneNumber: "99999999", + externalId: "12345", + pushTokenData: .init(pushToken: "blob_token", + pushEnablement: .authorized, + pushBackground: .available, + deviceData: .init(context: environment.analytics.appContextInfo())), + queue: [], + requestsInFlight: [], + initalizationState: .initialized, + flushing: true) + + let store = TestStore(initialState: initialState, reducer: KlaviyoReducer()) + + _ = await store.send(.enqueueProfile(Profile(email: "", phoneNumber: "", externalId: ""))) { + $0.email = nil // since we reset state + $0.phoneNumber = nil // since we reset state + $0.externalId = nil // since we reset state + $0.enqueueProfileOrTokenRequest() + $0.pushTokenData = nil + } + } } From e323a92fcbd0de53ef183b02fb5e61f6270f2f10 Mon Sep 17 00:00:00 2001 From: Ajay Subramanya Date: Wed, 3 Apr 2024 11:29:34 -0500 Subject: [PATCH 4/8] added dev logging --- Sources/KlaviyoSwift/KlaviyoState.swift | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/Sources/KlaviyoSwift/KlaviyoState.swift b/Sources/KlaviyoSwift/KlaviyoState.swift index 1558df33..b900b18f 100644 --- a/Sources/KlaviyoSwift/KlaviyoState.swift +++ b/Sources/KlaviyoSwift/KlaviyoState.swift @@ -121,6 +121,7 @@ struct KlaviyoState: Equatable, Codable { mutating func updateEmail(email: String) { if email.isEmpty || email == self.email { + logDevWarning(for: "email") return } @@ -130,6 +131,7 @@ struct KlaviyoState: Equatable, Codable { mutating func updateExternalId(externalId: String) { if externalId.isEmpty || externalId == self.externalId { + logDevWarning(for: "externalId") return } @@ -139,6 +141,7 @@ struct KlaviyoState: Equatable, Codable { mutating func updatePhoneNumber(phoneNumber: String) { if phoneNumber.isEmpty || phoneNumber == self.phoneNumber { + logDevWarning(for: "phone number") return } @@ -184,14 +187,20 @@ struct KlaviyoState: Equatable, Codable { mutating func updateStateWithProfile(profile: Profile) { if let profileEmail = profile.email, !profileEmail.isEmpty, profileEmail != email { email = profileEmail + } else { + logDevWarning(for: "email") } if let profilePhoneNumber = profile.phoneNumber, !profilePhoneNumber.isEmpty, profilePhoneNumber != phoneNumber { phoneNumber = profilePhoneNumber + } else { + logDevWarning(for: "phone number") } if let profileExternalId = profile.externalId, !profileExternalId.isEmpty, profileExternalId != externalId { externalId = profileExternalId + } else { + logDevWarning(for: "external id") } } @@ -384,6 +393,14 @@ private func removeStateFile(at file: URL) { } } +private func logDevWarning(for identifier: String) { + environment.emitDeveloperWarning(""" + \(identifier) is either empty or same as what is already set earlier. + The SDK will ignore this change, please use resetProfile for + resetting profile identifiers + """) +} + /// Loads SDK state from disk /// - Parameter apiKey: the API key that uniquely identiifies the company /// - Returns: an instance of the `KlaviyoState` From 1986a38f8b3b87b7c00540b12d3b9994e94e6e8d Mon Sep 17 00:00:00 2001 From: Ajay Subramanya Date: Wed, 3 Apr 2024 12:51:40 -0500 Subject: [PATCH 5/8] some refactor --- Sources/KlaviyoSwift/KlaviyoState.swift | 53 ++++++++++++------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/Sources/KlaviyoSwift/KlaviyoState.swift b/Sources/KlaviyoSwift/KlaviyoState.swift index b900b18f..8eba10ee 100644 --- a/Sources/KlaviyoSwift/KlaviyoState.swift +++ b/Sources/KlaviyoSwift/KlaviyoState.swift @@ -120,33 +120,24 @@ struct KlaviyoState: Equatable, Codable { } mutating func updateEmail(email: String) { - if email.isEmpty || email == self.email { - logDevWarning(for: "email") - return + if email.isNotEmptyOrSame(as: self.email, identifier: "email") { + self.email = email + enqueueProfileOrTokenRequest() } - - self.email = email - enqueueProfileOrTokenRequest() } mutating func updateExternalId(externalId: String) { - if externalId.isEmpty || externalId == self.externalId { - logDevWarning(for: "externalId") - return + if externalId.isNotEmptyOrSame(as: self.externalId, identifier: "external Id") { + self.externalId = externalId + enqueueProfileOrTokenRequest() } - - self.externalId = externalId - enqueueProfileOrTokenRequest() } mutating func updatePhoneNumber(phoneNumber: String) { - if phoneNumber.isEmpty || phoneNumber == self.phoneNumber { - logDevWarning(for: "phone number") - return + if phoneNumber.isNotEmptyOrSame(as: self.phoneNumber, identifier: "phone number") { + self.phoneNumber = phoneNumber + enqueueProfileOrTokenRequest() } - - self.phoneNumber = phoneNumber - enqueueProfileOrTokenRequest() } mutating func enqueueProfileOrTokenRequest() { @@ -185,22 +176,19 @@ struct KlaviyoState: Equatable, Codable { } mutating func updateStateWithProfile(profile: Profile) { - if let profileEmail = profile.email, !profileEmail.isEmpty, profileEmail != email { + if let profileEmail = profile.email, + profileEmail.isNotEmptyOrSame(as: self.email, identifier: "email") { email = profileEmail - } else { - logDevWarning(for: "email") } - if let profilePhoneNumber = profile.phoneNumber, !profilePhoneNumber.isEmpty, profilePhoneNumber != phoneNumber { + if let profilePhoneNumber = profile.phoneNumber, + profilePhoneNumber.isNotEmptyOrSame(as: self.phoneNumber, identifier: "phone number") { phoneNumber = profilePhoneNumber - } else { - logDevWarning(for: "phone number") } - if let profileExternalId = profile.externalId, !profileExternalId.isEmpty, profileExternalId != externalId { + if let profileExternalId = profile.externalId, + profileExternalId.isNotEmptyOrSame(as: self.externalId, identifier: "external id") { externalId = profileExternalId - } else { - logDevWarning(for: "external id") } } @@ -515,3 +503,14 @@ extension Profile { return profile } } + +extension String { + fileprivate func isNotEmptyOrSame(as state: String?, identifier: String) -> Bool { + let incoming = self + if incoming.isEmpty || incoming == state { + logDevWarning(for: identifier) + } + + return !incoming.isEmpty && incoming != state + } +} From 9b688f3fc71002b67df8ebc962bee999ec30c440 Mon Sep 17 00:00:00 2001 From: Ajay Subramanya Date: Wed, 3 Apr 2024 16:34:03 -0500 Subject: [PATCH 6/8] not logging when an idenfier is nil during set profile --- Sources/KlaviyoSwift/KlaviyoState.swift | 30 +++++++++++++++---------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/Sources/KlaviyoSwift/KlaviyoState.swift b/Sources/KlaviyoSwift/KlaviyoState.swift index b900b18f..56db5975 100644 --- a/Sources/KlaviyoSwift/KlaviyoState.swift +++ b/Sources/KlaviyoSwift/KlaviyoState.swift @@ -185,22 +185,28 @@ struct KlaviyoState: Equatable, Codable { } mutating func updateStateWithProfile(profile: Profile) { - if let profileEmail = profile.email, !profileEmail.isEmpty, profileEmail != email { - email = profileEmail - } else { - logDevWarning(for: "email") + if let profileEmail = profile.email { + if !profileEmail.isEmpty, profileEmail != email { + email = profileEmail + } else { + logDevWarning(for: "email") + } } - if let profilePhoneNumber = profile.phoneNumber, !profilePhoneNumber.isEmpty, profilePhoneNumber != phoneNumber { - phoneNumber = profilePhoneNumber - } else { - logDevWarning(for: "phone number") + if let profilePhoneNumber = profile.phoneNumber { + if !profilePhoneNumber.isEmpty, profilePhoneNumber != phoneNumber { + phoneNumber = profilePhoneNumber + } else { + logDevWarning(for: "phone number") + } } - if let profileExternalId = profile.externalId, !profileExternalId.isEmpty, profileExternalId != externalId { - externalId = profileExternalId - } else { - logDevWarning(for: "external id") + if let profileExternalId = profile.externalId { + if !profileExternalId.isEmpty, profileExternalId != externalId { + externalId = profileExternalId + } else { + logDevWarning(for: "external id") + } } } From e757f6670802b42aa55383abda7dc5a36f6fd912 Mon Sep 17 00:00:00 2001 From: Ajay Subramanya Date: Mon, 8 Apr 2024 21:49:14 -0500 Subject: [PATCH 7/8] added jitter to retry after --- Sources/KlaviyoSwift/APIRequestErrorHandling.swift | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Sources/KlaviyoSwift/APIRequestErrorHandling.swift b/Sources/KlaviyoSwift/APIRequestErrorHandling.swift index f055e050..8a465c64 100644 --- a/Sources/KlaviyoSwift/APIRequestErrorHandling.swift +++ b/Sources/KlaviyoSwift/APIRequestErrorHandling.swift @@ -33,10 +33,9 @@ enum InvalidField: Equatable { } } -private func getDelaySeconds(for count: Int) -> Int { - let delay = Int(pow(2.0, Double(count))) +private func addJitter(to value: Int) -> Int { let jitter = environment.randomInt() - return min(delay + jitter, ErrorHandlingConstants.maxBackoff) + return value + jitter } private func parseError(_ data: Data) -> [InvalidField]? { @@ -112,7 +111,9 @@ func handleRequestError( case let .retryWithBackoff(requestCount, totalCount, _): requestRetryCount = requestCount + 1 totalRetryCount = totalCount + 1 - nextBackoff = retryAfter ?? getDelaySeconds(for: totalRetryCount) + let exponentialBackOff = Int(pow(2.0, Double(totalRetryCount))) + + nextBackoff = addJitter(to: retryAfter ?? exponentialBackOff) } return .requestFailed( request, .retryWithBackoff( From 5cd4a32066b9b0e267db1d3a9bfdbb90e157ccbb Mon Sep 17 00:00:00 2001 From: Ajay Subramanya Date: Tue, 9 Apr 2024 11:38:41 -0500 Subject: [PATCH 8/8] using value for instead of all headers --- Sources/KlaviyoSwift/KlaviyoAPI.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/KlaviyoSwift/KlaviyoAPI.swift b/Sources/KlaviyoSwift/KlaviyoAPI.swift index 570e9e32..e61bb31c 100644 --- a/Sources/KlaviyoSwift/KlaviyoAPI.swift +++ b/Sources/KlaviyoSwift/KlaviyoAPI.swift @@ -70,7 +70,7 @@ struct KlaviyoAPI { if httpResponse.statusCode == 429 { requestRateLimited(request) - let retryAfter = httpResponse.allHeaderFields["Retry-After"] as? Int + let retryAfter = Int(httpResponse.value(forHTTPHeaderField: "Retry-After") ?? "0") return .failure(KlaviyoAPIError.rateLimitError(retryAfter)) }