From 2d0a3941bea71d8bbe31a3e2da10bd1ce09f9836 Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Wed, 26 Jul 2023 15:57:08 +0200 Subject: [PATCH 1/9] Revert "Merge pull request #38 from spotify/sent-property" This reverts commit 8e727ccb66b274149f8f909069ba58a97fe286a0, reversing changes made to 640f8d3c4dd6ac24215771acad8c4995a240750e. --- .../Apply/FlagApplierWithRetries.swift | 13 +++++- .../Cache/CacheDataActor.swift | 3 ++ .../Cache/CacheDataInteractor.swift | 7 +++- .../Cache/Models/ApplyEvent.swift | 11 +++++ .../Cache/Models/CacheData.swift | 21 +++++++--- .../Cache/Models/FlagApply.swift | 4 +- .../ConfidenceFeatureProvider.swift | 12 +++--- .../CacheDataTests.swift | 41 +++++++++++++++++-- .../FlagApplierWithRetriesTest.swift | 41 ++++++++++++++++++- 9 files changed, 132 insertions(+), 21 deletions(-) create mode 100644 Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift diff --git a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift index adb53ff7..e74cebfb 100644 --- a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift +++ b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift @@ -45,6 +45,7 @@ final class FlagApplierWithRetries: FlagApplier { self.write(resolveToken: resolveToken, name: flagName, applyTime: applyTime) return } + self.setEventSent(resolveToken: resolveToken, name: flagName) self.triggerBatch() } } @@ -65,11 +66,21 @@ final class FlagApplierWithRetries: FlagApplier { return } // Remove events from storage that were successfully sent + // Set 'sent' property of apply events to true self.remove(resolveToken: resolveEvent.resolveToken) + resolveEvent.events.forEach { applyEvent in + self.setEventSent(resolveToken: resolveEvent.resolveToken, name: applyEvent.name) + } } } } + private func setEventSent(resolveToken: String, name: String) { + Task { + await self.cacheDataInteractor.setEventSent(resolveToken: resolveToken, name: name) + } + } + private func write(resolveToken: String, name: String, applyTime: Date) { guard var storedData = try? storage.load(defaultValue: CacheData.empty()) else { return @@ -94,7 +105,7 @@ final class FlagApplierWithRetries: FlagApplier { let applyFlagRequestItems = items.map { applyEvent in AppliedFlagRequestItem( flag: applyEvent.name, - applyTime: applyEvent.applyTime + applyTime: applyEvent.applyEvent.applyTime ) } let request = ApplyFlagsRequest( diff --git a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift index 1cf7f496..c17cf544 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift @@ -17,4 +17,7 @@ protocol CacheDataActor: Actor { /// Removes single apply event from the cache. func applyEventExists(resolveToken: String, name: String) -> Bool + + /// Sets Flag Apply Event `sent` propery to `true`. + func setEventSent(resolveToken: String, name: String) } diff --git a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift index 06d8e239..4103f9b9 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift @@ -36,10 +36,13 @@ final actor CacheDataInteractor: CacheDataActor { cache.applyEventExists(resolveToken: resolveToken, name: name) } + func setEventSent(resolveToken: String, name: String) { + cache.setEventSent(resolveToken: resolveToken, name: name) + } + private func loadCacheFromStorage() { guard let storedData = try? storage.load(defaultValue: cache), - storedData.isEmpty == false - else { + storedData.isEmpty == false else { return } self.cache = storedData diff --git a/Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift b/Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift new file mode 100644 index 00000000..b19d8630 --- /dev/null +++ b/Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift @@ -0,0 +1,11 @@ +import Foundation + +struct ApplyEvent: Codable { + let applyTime: Date + var sent: Bool + + init(applyTime: Date, sent: Bool = false) { + self.applyTime = applyTime + self.sent = sent + } +} diff --git a/Sources/ConfidenceProvider/Cache/Models/CacheData.swift b/Sources/ConfidenceProvider/Cache/Models/CacheData.swift index c629a57a..aa8e5ff2 100644 --- a/Sources/ConfidenceProvider/Cache/Models/CacheData.swift +++ b/Sources/ConfidenceProvider/Cache/Models/CacheData.swift @@ -1,8 +1,10 @@ import Foundation -/// `CacheData` represents object that encapsulates exposure events for evaluated flags. -/// It holds information related to apply event i.e. resolve token, flag name, timestamp . -/// This object is used for tracking exposure events, i.e. by storing them on disk. +/** + `CacheData` represents object that encapsulates exposure events for evaluated flags. +It holds information related to apply event i.e. resolve token, flag name, timestamp . + This object is used for tracking exposure events, i.e. by storing them on disk. + */ struct CacheData: Codable { var resolveEvents: [ResolveApply] @@ -33,6 +35,16 @@ struct CacheData: Codable { return resolveTokenIndex != nil } + mutating func setEventSent(resolveToken: String, name: String, sent: Bool = true) { + let flagEventIndexes = flagEventIndex(resolveToken: resolveToken, name: name) + guard let resolveIndex = flagEventIndexes.resolveEventIndex, + let flagIndex = flagEventIndexes.flagEventIndex else { + return + } + + resolveEvents[resolveIndex].events[flagIndex].applyEvent.sent = sent + } + mutating func add(resolveToken: String, flagName: String, applyTime: Date) { let resolveEventIndex = resolveEventIndex(resolveToken: resolveToken) @@ -93,8 +105,7 @@ struct CacheData: Codable { func flagEvent(resolveToken: String, name: String) -> FlagApply? { guard let resolveTokenIndex = resolveEventIndex(resolveToken: resolveToken), - let flagEventIndex = applyEventIndex(resolveToken: resolveToken, name: name) - else { + let flagEventIndex = applyEventIndex(resolveToken: resolveToken, name: name) else { return nil } diff --git a/Sources/ConfidenceProvider/Cache/Models/FlagApply.swift b/Sources/ConfidenceProvider/Cache/Models/FlagApply.swift index 7ea9696e..c5c26c57 100644 --- a/Sources/ConfidenceProvider/Cache/Models/FlagApply.swift +++ b/Sources/ConfidenceProvider/Cache/Models/FlagApply.swift @@ -2,10 +2,10 @@ import Foundation struct FlagApply: Codable { let name: String - var applyTime: Date + var applyEvent: ApplyEvent init(name: String, applyTime: Date) { self.name = name - self.applyTime = applyTime + self.applyEvent = ApplyEvent(applyTime: applyTime) } } diff --git a/Sources/ConfidenceProvider/ConfidenceFeatureProvider.swift b/Sources/ConfidenceProvider/ConfidenceFeatureProvider.swift index 3cdedf0b..102a9c42 100644 --- a/Sources/ConfidenceProvider/ConfidenceFeatureProvider.swift +++ b/Sources/ConfidenceProvider/ConfidenceFeatureProvider.swift @@ -471,13 +471,11 @@ extension ConfidenceFeatureProvider { /// Creates the `ConfidenceFeatureProvider` according to the settings specified in the builder. public func build() -> ConfidenceFeatureProvider { - let flagApplier = - flagApplier - ?? FlagApplierWithRetries( - httpClient: NetworkClient(region: options.region), - storage: DefaultStorage(filePath: "applier.flags.cache"), - options: options - ) + let flagApplier = flagApplier ?? FlagApplierWithRetries( + httpClient: NetworkClient(region: options.region), + storage: DefaultStorage(filePath: "applier.flags.cache"), + options: options + ) let client = RemoteConfidenceClient( options: options, diff --git a/Tests/ConfidenceProviderTests/CacheDataTests.swift b/Tests/ConfidenceProviderTests/CacheDataTests.swift index a4cfd839..4c72f177 100644 --- a/Tests/ConfidenceProviderTests/CacheDataTests.swift +++ b/Tests/ConfidenceProviderTests/CacheDataTests.swift @@ -20,7 +20,7 @@ final class CacheDataTests: XCTestCase { let resolveEvent = try XCTUnwrap(cacheData.resolveEvents.first) XCTAssertEqual(resolveEvent.resolveToken, "token1") XCTAssertEqual(resolveEvent.events.count, 1) - XCTAssertEqual(resolveEvent.events.first?.applyTime, applyTime) + XCTAssertEqual(resolveEvent.events.first?.applyEvent.applyTime, applyTime) } func testCacheData_addEvent_emptyFlagEvents() throws { @@ -38,7 +38,7 @@ final class CacheDataTests: XCTestCase { let resolveEvent = try XCTUnwrap(cacheData.resolveEvents.first) XCTAssertEqual(resolveEvent.resolveToken, "token1") XCTAssertEqual(resolveEvent.events.count, 1) - XCTAssertEqual(resolveEvent.events.first?.applyTime, applyTime) + XCTAssertEqual(resolveEvent.events.first?.applyEvent.applyTime, applyTime) } func testCacheData_addEvent_prefilled() throws { @@ -66,7 +66,7 @@ final class CacheDataTests: XCTestCase { // Then apply record is not overriden let applyEvent = try XCTUnwrap(cacheData.resolveEvents.first?.events.first) - XCTAssertEqual(applyEvent.applyTime, applyTime) + XCTAssertEqual(applyEvent.applyEvent.applyTime, applyTime) } func testCacheData_addEvent_multipleTokens() throws { @@ -111,6 +111,41 @@ final class CacheDataTests: XCTestCase { XCTAssertEqual(cacheData.resolveEvents.isEmpty, true) } + func testCacheData_eventExist_isEventSent() throws { + // Given prefilled cached data + // and all apply events has sent property set to false + var cacheData = try CacheDataUtility.prefilledCacheData() + let resolve = try XCTUnwrap(cacheData.resolveEvents.first) + let sentEvents = resolve.events.filter { $0.applyEvent.sent == true } + XCTAssertEqual(sentEvents.count, 0) + + // When set event sent is called for item that exists in cache + cacheData.setEventSent(resolveToken: "token0", name: "prefilled2") + let resolveAfterUpdate = try XCTUnwrap(cacheData.resolveEvents.first) + + // Then apply event sent property has been set to true + let sentEventsAfterUpdate = resolveAfterUpdate.events.filter { $0.applyEvent.sent == true } + XCTAssertEqual(sentEventsAfterUpdate.count, 1) + XCTAssertEqual(sentEventsAfterUpdate.first?.name, "prefilled2") + } + + func testCacheData_eventDoesNotExist_isEventSent() throws { + // Given prefilled cached data + // and all apply events has sent property set to false + var cacheData = try CacheDataUtility.prefilledCacheData() + let resolve = try XCTUnwrap(cacheData.resolveEvents.first) + let sentEvents = resolve.events.filter { $0.applyEvent.sent == true } + XCTAssertEqual(sentEvents.count, 0) + + // When set event sent is called for item that does not exists in cache + cacheData.setEventSent(resolveToken: "token0", name: "prefilled45") + let resolveAfterUpdate = try XCTUnwrap(cacheData.resolveEvents.first) + + // Then apply event sent property has not been changed + let sentEventsAfterUpdate = resolveAfterUpdate.events.filter { $0.applyEvent.sent == true } + XCTAssertEqual(sentEventsAfterUpdate.count, 0) + } + func testCacheData_isEmpty() { // Given empty cached data let cacheData = CacheData.empty() diff --git a/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift b/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift index 5b7a3bc4..77aeb6b5 100644 --- a/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift +++ b/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift @@ -162,6 +162,43 @@ class FlagApplierWithRetriesTest: XCTestCase { XCTAssertEqual(request3.flags.first?.flag, "flags/flag1") } + func testApply_multipleApplyCalls_sentSet() async throws { + // Given flag applier with http client that is offline + let cacheDataInteractor = CacheDataInteractor(storage: storage) + let httpClient = HttpClientMock(testMode: .error) + let expectation = XCTestExpectation(description: "Waiting for batch trigger") + expectation.expectedFulfillmentCount = 3 + httpClient.expectation = expectation + + let applier = FlagApplierWithRetries( + httpClient: httpClient, + storage: storage, + options: options, + cacheDataInteractor: cacheDataInteractor, + triggerBatch: false + ) + + // When first apply call is issued + // And http client request fails with .invalidResponse + await applier.apply(flagName: "flag1", resolveToken: "token1") + + // And second apply call is issued + // With test mode .success + httpClient.testMode = .success + await applier.apply(flagName: "flag2", resolveToken: "token1") + wait(for: [expectation], timeout: 1) + + Task { + // Then both requests are marked as sent in cache data + let cacheData = await cacheDataInteractor.cache + let flagEvent1 = cacheData.flagEvent(resolveToken: "token1", name: "flag1") + let flagEvent2 = cacheData.flagEvent(resolveToken: "token1", name: "flag2") + + XCTAssertEqual(flagEvent1?.applyEvent.sent, true) + XCTAssertEqual(flagEvent2?.applyEvent.sent, true) + } + } + func testApply_previoslyStoredData_cleanAfterSending() async throws { // Given storage that has previously stored data (100 records, same token) let prefilledStorage = StorageMock() @@ -226,6 +263,7 @@ class FlagApplierWithRetriesTest: XCTestCase { await applier.apply(flagName: "flag2", resolveToken: "token1") await applier.apply(flagName: "flag3", resolveToken: "token1") + // Then 1 resolve event record is written to disk let storedData = try XCTUnwrap(storage.load(defaultValue: CacheData.empty())) let data = try XCTUnwrap(storedData.resolveEvents.first { $0.resolveToken == "token1" }) @@ -295,7 +333,8 @@ class FlagApplierWithRetriesTest: XCTestCase { let newResolveEvent = try XCTUnwrap(storedData.resolveEvents.first { $0.resolveToken == "token0" }) XCTAssertEqual(newResolveEvent.events.count, 1) XCTAssertEqual(newResolveEvent.events[0].name, "flag1") - XCTAssertEqual(newResolveEvent.events[0].applyTime, Date(timeIntervalSince1970: 1000)) + XCTAssertEqual(newResolveEvent.events[0].applyEvent.applyTime, Date(timeIntervalSince1970: 1000)) + XCTAssertEqual(newResolveEvent.events[0].applyEvent.sent, false) } func testApplyOffline_previoslyStoredData_100records() async throws { From d249e5aa20d58fbf9ab2129453feb0209adea2cd Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Wed, 26 Jul 2023 16:44:50 +0200 Subject: [PATCH 2/9] WIP FlagApplier and CacheData changes --- .../Apply/FlagApplierWithRetries.swift | 52 ++++++++----------- .../Cache/CacheDataActor.swift | 6 ++- .../Cache/CacheDataInteractor.swift | 11 +++- .../FlagApplierWithRetriesTest.swift | 6 +-- .../Helpers/CacheDataInteractorMock.swift | 12 ++++- 5 files changed, 49 insertions(+), 38 deletions(-) diff --git a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift index e74cebfb..85d3e77e 100644 --- a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift +++ b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift @@ -24,7 +24,9 @@ final class FlagApplierWithRetries: FlagApplier { self.cacheDataInteractor = cacheDataInteractor ?? CacheDataInteractor(storage: storage) if triggerBatch { - self.triggerBatch() + Task { + await self.triggerBatch() + } } } @@ -34,43 +36,47 @@ final class FlagApplierWithRetries: FlagApplier { guard eventExists == false else { // If record is found in the cache, early return (de-duplication). // Triggerring batch apply in case if there are any unsent events stored - triggerBatch() + await triggerBatch() return } - await cacheDataInteractor.add(resolveToken: resolveToken, flagName: flagName, applyTime: applyTime) + let data = await cacheDataInteractor.add(resolveToken: resolveToken, flagName: flagName, applyTime: applyTime) + self.writeToFile(data: data) let flagApply = FlagApply(name: flagName, applyTime: applyTime) executeApply(resolveToken: resolveToken, items: [flagApply]) { success in guard success else { - self.write(resolveToken: resolveToken, name: flagName, applyTime: applyTime) return } - self.setEventSent(resolveToken: resolveToken, name: flagName) - self.triggerBatch() + Task { + await self.setEventSent(resolveToken: resolveToken, name: flagName) + await self.triggerBatch() + } } } // MARK: private - private func triggerBatch() { - guard let storedData = try? storage.load(defaultValue: CacheData.empty()), storedData.isEmpty == false else { - return - } - - storedData.resolveEvents.forEach { resolveEvent in + private func triggerBatch() async { + let cacheData = await cacheDataInteractor.cache + cacheData.resolveEvents.forEach { resolveEvent in + let appliesToSend = resolveEvent.events.filter { entry in + return entry.applyEvent.sent == false + } executeApply( resolveToken: resolveEvent.resolveToken, - items: resolveEvent.events + items: appliesToSend ) { success in guard success else { return } - // Remove events from storage that were successfully sent // Set 'sent' property of apply events to true - self.remove(resolveToken: resolveEvent.resolveToken) resolveEvent.events.forEach { applyEvent in self.setEventSent(resolveToken: resolveEvent.resolveToken, name: applyEvent.name) } + Task { + let data = await self.cacheDataInteractor.cache + self.writeToFile(data: data) + } } } } @@ -81,20 +87,8 @@ final class FlagApplierWithRetries: FlagApplier { } } - private func write(resolveToken: String, name: String, applyTime: Date) { - guard var storedData = try? storage.load(defaultValue: CacheData.empty()) else { - return - } - storedData.add(resolveToken: resolveToken, flagName: name, applyTime: applyTime) - try? storage.save(data: storedData) - } - - private func remove(resolveToken: String) { - guard var storedData = try? storage.load(defaultValue: CacheData.empty()) else { - return - } - storedData.remove(resolveToken: resolveToken) - try? storage.save(data: storedData) + private func writeToFile(data: CacheData) { + try? storage.save(data: data) } private func executeApply( diff --git a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift index c17cf544..861e13b6 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift @@ -6,7 +6,7 @@ protocol CacheDataActor: Actor { var cache: CacheData { get } /// Adds single data entry to the cache. - func add(resolveToken: String, flagName: String, applyTime: Date) + func add(resolveToken: String, flagName: String, applyTime: Date) -> CacheData /// Removes data from the cache. /// - Note: This method removes all flag apply entries from cache for given resolve token. @@ -19,5 +19,7 @@ protocol CacheDataActor: Actor { func applyEventExists(resolveToken: String, name: String) -> Bool /// Sets Flag Apply Event `sent` propery to `true`. - func setEventSent(resolveToken: String, name: String) + func setEventSent(resolveToken: String, name: String) -> CacheData + + func setEventSent(resolveToken: String) -> CacheData } diff --git a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift index 4103f9b9..d306c771 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift @@ -12,7 +12,7 @@ final actor CacheDataInteractor: CacheDataActor { } } - func add(resolveToken: String, flagName: String, applyTime: Date) { + func add(resolveToken: String, flagName: String, applyTime: Date) -> CacheData { if cache.isEmpty == false { cache.add(resolveToken: resolveToken, flagName: flagName, applyTime: applyTime) } else { @@ -22,6 +22,7 @@ final actor CacheDataInteractor: CacheDataActor { applyTime: applyTime ) } + return cache } func remove(resolveToken: String, flagName: String) { @@ -36,8 +37,14 @@ final actor CacheDataInteractor: CacheDataActor { cache.applyEventExists(resolveToken: resolveToken, name: name) } - func setEventSent(resolveToken: String, name: String) { + func setEventSent(resolveToken: String, name: String) -> CacheData { cache.setEventSent(resolveToken: resolveToken, name: name) + return cache + } + + func setEventSent(resolveToken: String) -> CacheData { + // TODO implement + return cache } private func loadCacheFromStorage() { diff --git a/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift b/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift index 77aeb6b5..dbf996c1 100644 --- a/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift +++ b/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift @@ -153,13 +153,13 @@ class FlagApplierWithRetriesTest: XCTestCase { let request1 = try XCTUnwrap(httpClient.data?[0] as? ApplyFlagsRequest) let request2 = try XCTUnwrap(httpClient.data?[1] as? ApplyFlagsRequest) - let request3 = try XCTUnwrap(httpClient.data?[2] as? ApplyFlagsRequest) +// let request3 = try XCTUnwrap(httpClient.data?[2] as? ApplyFlagsRequest) XCTAssertEqual(request1.flags.count, 1) XCTAssertEqual(request1.flags.first?.flag, "flags/flag1") XCTAssertEqual(request2.flags.count, 1) XCTAssertEqual(request2.flags.first?.flag, "flags/flag2") - XCTAssertEqual(request3.flags.count, 1) - XCTAssertEqual(request3.flags.first?.flag, "flags/flag1") +// XCTAssertEqual(request3.flags.count, 1) +// XCTAssertEqual(request3.flags.first?.flag, "flags/flag1") } func testApply_multipleApplyCalls_sentSet() async throws { diff --git a/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift b/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift index a8e39c3b..3238c198 100644 --- a/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift +++ b/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift @@ -3,9 +3,15 @@ import Foundation @testable import ConfidenceProvider final actor CacheDataInteractorMock: CacheDataActor { + func setEventSent(resolveToken: String) -> ConfidenceProvider.CacheData { + return cache + } + var cache = CacheData.empty() - func add(resolveToken: String, flagName: String, applyTime: Date) {} + func add(resolveToken: String, flagName: String, applyTime: Date) -> ConfidenceProvider.CacheData { + return cache + } func remove(resolveToken: String, flagName: String) {} @@ -15,5 +21,7 @@ final actor CacheDataInteractorMock: CacheDataActor { return false } - func setEventSent(resolveToken: String, name: String) {} + func setEventSent(resolveToken: String, name: String) -> ConfidenceProvider.CacheData { + return cache + } } From 043c99c9c0363bfb33637ec63a7be36f7f94211a Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Thu, 27 Jul 2023 15:32:34 +0200 Subject: [PATCH 3/9] setEvents for entire resolveToken --- .../Apply/FlagApplierWithRetries.swift | 14 +++----------- .../ConfidenceProvider/Cache/CacheDataActor.swift | 3 ++- .../Cache/CacheDataInteractor.swift | 2 +- .../Cache/Models/CacheData.swift | 10 ++++++++++ 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift index 85d3e77e..f4b2c726 100644 --- a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift +++ b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift @@ -48,7 +48,7 @@ final class FlagApplierWithRetries: FlagApplier { return } Task { - await self.setEventSent(resolveToken: resolveToken, name: flagName) + let _ = await self.cacheDataInteractor.setEventSent(resolveToken: resolveToken, name: flagName) await self.triggerBatch() } } @@ -70,23 +70,15 @@ final class FlagApplierWithRetries: FlagApplier { return } // Set 'sent' property of apply events to true - resolveEvent.events.forEach { applyEvent in - self.setEventSent(resolveToken: resolveEvent.resolveToken, name: applyEvent.name) - } + Task { - let data = await self.cacheDataInteractor.cache + let data = await self.cacheDataInteractor.setEventSent(resolveToken: resolveEvent.resolveToken) self.writeToFile(data: data) } } } } - private func setEventSent(resolveToken: String, name: String) { - Task { - await self.cacheDataInteractor.setEventSent(resolveToken: resolveToken, name: name) - } - } - private func writeToFile(data: CacheData) { try? storage.save(data: data) } diff --git a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift index 861e13b6..aef3b849 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift @@ -18,8 +18,9 @@ protocol CacheDataActor: Actor { /// Removes single apply event from the cache. func applyEventExists(resolveToken: String, name: String) -> Bool - /// Sets Flag Apply Event `sent` propery to `true`. + /// Sets Flag Apply Event `sent` property to `true`. func setEventSent(resolveToken: String, name: String) -> CacheData + /// Sets Resolve Apply Event `sent` property to `true`. func setEventSent(resolveToken: String) -> CacheData } diff --git a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift index d306c771..be699989 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift @@ -43,7 +43,7 @@ final actor CacheDataInteractor: CacheDataActor { } func setEventSent(resolveToken: String) -> CacheData { - // TODO implement + cache.setEventSent(resolveToken: resolveToken) return cache } diff --git a/Sources/ConfidenceProvider/Cache/Models/CacheData.swift b/Sources/ConfidenceProvider/Cache/Models/CacheData.swift index aa8e5ff2..aaf29448 100644 --- a/Sources/ConfidenceProvider/Cache/Models/CacheData.swift +++ b/Sources/ConfidenceProvider/Cache/Models/CacheData.swift @@ -45,6 +45,16 @@ struct CacheData: Codable { resolveEvents[resolveIndex].events[flagIndex].applyEvent.sent = sent } + mutating func setEventSent(resolveToken: String, sent: Bool = true) { + guard let resolveIndex = resolveEventIndex(resolveToken: resolveToken) else { + return + } + + for i in 0...resolveEvents[resolveIndex].events.count-1 { + resolveEvents[resolveIndex].events[i].applyEvent.sent = sent + } + } + mutating func add(resolveToken: String, flagName: String, applyTime: Date) { let resolveEventIndex = resolveEventIndex(resolveToken: resolveToken) From 102bb67fb2a8bd521283a48a10bb03d02b26632b Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Thu, 27 Jul 2023 15:39:48 +0200 Subject: [PATCH 4/9] Make Actor operations more atomic --- .../ConfidenceProvider/Apply/FlagApplierWithRetries.swift | 5 ++--- Sources/ConfidenceProvider/Cache/CacheDataActor.swift | 2 +- .../ConfidenceProvider/Cache/CacheDataInteractor.swift | 8 +++++--- Sources/ConfidenceProvider/Cache/Models/CacheData.swift | 5 ++++- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift index f4b2c726..e7d7b55b 100644 --- a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift +++ b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift @@ -32,15 +32,14 @@ final class FlagApplierWithRetries: FlagApplier { public func apply(flagName: String, resolveToken: String) async { let applyTime = Date.backport.now - let eventExists = await cacheDataInteractor.applyEventExists(resolveToken: resolveToken, name: flagName) - guard eventExists == false else { + let (data, added) = await cacheDataInteractor.add(resolveToken: resolveToken, flagName: flagName, applyTime: applyTime) + guard added == true else { // If record is found in the cache, early return (de-duplication). // Triggerring batch apply in case if there are any unsent events stored await triggerBatch() return } - let data = await cacheDataInteractor.add(resolveToken: resolveToken, flagName: flagName, applyTime: applyTime) self.writeToFile(data: data) let flagApply = FlagApply(name: flagName, applyTime: applyTime) executeApply(resolveToken: resolveToken, items: [flagApply]) { success in diff --git a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift index aef3b849..6736a8c4 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift @@ -6,7 +6,7 @@ protocol CacheDataActor: Actor { var cache: CacheData { get } /// Adds single data entry to the cache. - func add(resolveToken: String, flagName: String, applyTime: Date) -> CacheData + func add(resolveToken: String, flagName: String, applyTime: Date) -> (CacheData, Bool) /// Removes data from the cache. /// - Note: This method removes all flag apply entries from cache for given resolve token. diff --git a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift index be699989..e4a9e19f 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift @@ -12,17 +12,19 @@ final actor CacheDataInteractor: CacheDataActor { } } - func add(resolveToken: String, flagName: String, applyTime: Date) -> CacheData { + func add(resolveToken: String, flagName: String, applyTime: Date) -> (CacheData, Bool) { if cache.isEmpty == false { - cache.add(resolveToken: resolveToken, flagName: flagName, applyTime: applyTime) + let added = cache.add(resolveToken: resolveToken, flagName: flagName, applyTime: applyTime) + return (cache, added) } else { cache = CacheData( resolveToken: resolveToken, flagName: flagName, applyTime: applyTime ) + return (cache, true) } - return cache + } func remove(resolveToken: String, flagName: String) { diff --git a/Sources/ConfidenceProvider/Cache/Models/CacheData.swift b/Sources/ConfidenceProvider/Cache/Models/CacheData.swift index aaf29448..eac6ba63 100644 --- a/Sources/ConfidenceProvider/Cache/Models/CacheData.swift +++ b/Sources/ConfidenceProvider/Cache/Models/CacheData.swift @@ -55,7 +55,7 @@ struct CacheData: Codable { } } - mutating func add(resolveToken: String, flagName: String, applyTime: Date) { + mutating func add(resolveToken: String, flagName: String, applyTime: Date) -> Bool { let resolveEventIndex = resolveEventIndex(resolveToken: resolveToken) if let resolveEventIndex { @@ -68,12 +68,15 @@ struct CacheData: Codable { // No flag apply event for given resolve token, adding new record let flagEvent = FlagApply(name: flagName, applyTime: applyTime) resolveEvents[resolveEventIndex].events.append(flagEvent) + return true } } else { // No resolve event for given resolve token, adding new record let event = ResolveApply(resolveToken: resolveToken, flagName: flagName, applyTime: applyTime) resolveEvents.append(event) + return true } + return false } mutating func remove(resolveToken: String) { From cb77156d6bcc2eae00f1ff8948179fc9fc324337 Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Thu, 27 Jul 2023 15:46:07 +0200 Subject: [PATCH 5/9] Make sure loading data from storage is protected by Actor --- .../ConfidenceProvider/Apply/FlagApplierWithRetries.swift | 3 +++ Sources/ConfidenceProvider/Cache/CacheDataActor.swift | 3 +++ .../ConfidenceProvider/Cache/CacheDataInteractor.swift | 2 +- .../Helpers/CacheDataInteractorMock.swift | 8 ++++++-- 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift index e7d7b55b..e8e9a82f 100644 --- a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift +++ b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift @@ -21,6 +21,9 @@ final class FlagApplierWithRetries: FlagApplier { self.storage = storage self.httpClient = httpClient self.options = options + + // Read/Write operations to/from the file are not protected by the cacheDataActor + // It's important to not write the file before this operation is completed (reading from file) self.cacheDataInteractor = cacheDataInteractor ?? CacheDataInteractor(storage: storage) if triggerBatch { diff --git a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift index 6736a8c4..a3d4da26 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift @@ -23,4 +23,7 @@ protocol CacheDataActor: Actor { /// Sets Resolve Apply Event `sent` property to `true`. func setEventSent(resolveToken: String) -> CacheData + + /// Loads data from storage, might be an expensive operation + func loadCacheFromStorage() } diff --git a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift index e4a9e19f..f7629e8d 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift @@ -49,7 +49,7 @@ final actor CacheDataInteractor: CacheDataActor { return cache } - private func loadCacheFromStorage() { + func loadCacheFromStorage() { guard let storedData = try? storage.load(defaultValue: cache), storedData.isEmpty == false else { return diff --git a/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift b/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift index 3238c198..6ac0336d 100644 --- a/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift +++ b/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift @@ -9,8 +9,8 @@ final actor CacheDataInteractorMock: CacheDataActor { var cache = CacheData.empty() - func add(resolveToken: String, flagName: String, applyTime: Date) -> ConfidenceProvider.CacheData { - return cache + func add(resolveToken: String, flagName: String, applyTime: Date) -> (ConfidenceProvider.CacheData, Bool) { + return (cache, true) } func remove(resolveToken: String, flagName: String) {} @@ -24,4 +24,8 @@ final actor CacheDataInteractorMock: CacheDataActor { func setEventSent(resolveToken: String, name: String) -> ConfidenceProvider.CacheData { return cache } + + func loadCacheFromStorage() { + return + } } From 147fb21da2a732cd77c0bf32eed80ca537f9ad2f Mon Sep 17 00:00:00 2001 From: Alina Andersone Date: Mon, 7 Aug 2023 08:45:22 +0200 Subject: [PATCH 6/9] Introduced apply event status to move implementation closer to android --- .../Apply/FlagApplierWithRetries.swift | 55 ++++-- .../Cache/CacheDataActor.swift | 10 +- .../Cache/CacheDataInteractor.swift | 30 ++- .../Cache/Models/ApplyEvent.swift | 6 +- .../Cache/Models/ApplyEventStatus.swift | 7 + .../Cache/Models/CacheData.swift | 8 +- .../Cache/Models/ResolveApply.swift | 4 + .../CacheDataTests.swift | 12 +- .../FlagApplierWithRetriesTest.swift | 176 ++++++++++-------- .../Helpers/CacheDataInteractorMock.swift | 12 +- 10 files changed, 188 insertions(+), 132 deletions(-) create mode 100644 Sources/ConfidenceProvider/Cache/Models/ApplyEventStatus.swift diff --git a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift index e8e9a82f..72fc8057 100644 --- a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift +++ b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift @@ -5,6 +5,8 @@ import os typealias ApplyFlagHTTPResponse = HttpClientResponse typealias ApplyFlagResult = Result +// swiftlint:disable type_body_length +// swiftlint:disable file_length final class FlagApplierWithRetries: FlagApplier { private let storage: Storage private let httpClient: HttpClient @@ -27,7 +29,7 @@ final class FlagApplierWithRetries: FlagApplier { self.cacheDataInteractor = cacheDataInteractor ?? CacheDataInteractor(storage: storage) if triggerBatch { - Task { + Task(priority: .utility) { await self.triggerBatch() } } @@ -35,7 +37,11 @@ final class FlagApplierWithRetries: FlagApplier { public func apply(flagName: String, resolveToken: String) async { let applyTime = Date.backport.now - let (data, added) = await cacheDataInteractor.add(resolveToken: resolveToken, flagName: flagName, applyTime: applyTime) + let (data, added) = await cacheDataInteractor.add( + resolveToken: resolveToken, + flagName: flagName, + applyTime: applyTime + ) guard added == true else { // If record is found in the cache, early return (de-duplication). // Triggerring batch apply in case if there are any unsent events stored @@ -44,38 +50,51 @@ final class FlagApplierWithRetries: FlagApplier { } self.writeToFile(data: data) - let flagApply = FlagApply(name: flagName, applyTime: applyTime) - executeApply(resolveToken: resolveToken, items: [flagApply]) { success in - guard success else { - return - } - Task { - let _ = await self.cacheDataInteractor.setEventSent(resolveToken: resolveToken, name: flagName) - await self.triggerBatch() - } - } + await triggerBatch() } // MARK: private private func triggerBatch() async { - let cacheData = await cacheDataInteractor.cache - cacheData.resolveEvents.forEach { resolveEvent in + async let cacheData = await cacheDataInteractor.cache + await cacheData.resolveEvents.forEach { resolveEvent in let appliesToSend = resolveEvent.events.filter { entry in - return entry.applyEvent.sent == false + return entry.applyEvent.status == .created } + guard appliesToSend.isEmpty == false else { + return + } + self.writeStatus(resolveToken: resolveEvent.resolveToken, events: appliesToSend, status: .sending) executeApply( resolveToken: resolveEvent.resolveToken, items: appliesToSend ) { success in guard success else { + self.writeStatus(resolveToken: resolveEvent.resolveToken, events: appliesToSend, status: .created) return } // Set 'sent' property of apply events to true + self.writeStatus(resolveToken: resolveEvent.resolveToken, events: appliesToSend, status: .sent) + } + } + } - Task { - let data = await self.cacheDataInteractor.setEventSent(resolveToken: resolveEvent.resolveToken) - self.writeToFile(data: data) + private func writeStatus(resolveToken: String, events: [FlagApply], status: ApplyEventStatus) { + let lastIndex = events.count - 1 + events.enumerated().forEach { (index, event) in + Task(priority: .medium) { + var data = await self.cacheDataInteractor.setEventStatus( + resolveToken: resolveToken, + name: event.name, + status: status + ) + + if index == lastIndex { + let unsentFlagApplies = data.resolveEvents.filter { + $0.isSent == false + } + data.resolveEvents = unsentFlagApplies + try? self.storage.save(data: data) } } } diff --git a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift index a3d4da26..427c6d9f 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataActor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataActor.swift @@ -18,12 +18,10 @@ protocol CacheDataActor: Actor { /// Removes single apply event from the cache. func applyEventExists(resolveToken: String, name: String) -> Bool - /// Sets Flag Apply Event `sent` property to `true`. - func setEventSent(resolveToken: String, name: String) -> CacheData + /// Sets Flag Apply Event `status`. + func setEventStatus(resolveToken: String, name: String, status: ApplyEventStatus) -> CacheData - /// Sets Resolve Apply Event `sent` property to `true`. - func setEventSent(resolveToken: String) -> CacheData + /// Sets Resolve Apply Event `status` property. + func setEventStatus(resolveToken: String, status: ApplyEventStatus) -> CacheData - /// Loads data from storage, might be an expensive operation - func loadCacheFromStorage() } diff --git a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift index f7629e8d..25eee8bf 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift @@ -7,7 +7,7 @@ final actor CacheDataInteractor: CacheDataActor { init(storage: Storage) { self.storage = storage - Task { + Task(priority: .high) { await loadCacheFromStorage() } } @@ -24,7 +24,6 @@ final actor CacheDataInteractor: CacheDataActor { ) return (cache, true) } - } func remove(resolveToken: String, flagName: String) { @@ -39,21 +38,32 @@ final actor CacheDataInteractor: CacheDataActor { cache.applyEventExists(resolveToken: resolveToken, name: name) } - func setEventSent(resolveToken: String, name: String) -> CacheData { - cache.setEventSent(resolveToken: resolveToken, name: name) + func setEventStatus(resolveToken: String, name: String, status: ApplyEventStatus) -> CacheData { + cache.setEventStatus(resolveToken: resolveToken, name: name, status: status) return cache } - func setEventSent(resolveToken: String) -> CacheData { - cache.setEventSent(resolveToken: resolveToken) + func setEventStatus(resolveToken: String, status: ApplyEventStatus) -> CacheData { + cache.setEventStatus(resolveToken: resolveToken, status: status) return cache } - func loadCacheFromStorage() { - guard let storedData = try? storage.load(defaultValue: cache), - storedData.isEmpty == false else { + private func loadCacheFromStorage() { + guard let storedData = try? storage.load(defaultValue: cache), storedData.isEmpty == false else { return } - self.cache = storedData + if self.cache.isEmpty { + self.cache = storedData + } else { + storedData.resolveEvents.forEach { resolveEvent in + resolveEvent.events.forEach { flagApplyEvent in + _ = self.cache.add( + resolveToken: resolveEvent.resolveToken, + flagName: flagApplyEvent.name, + applyTime: flagApplyEvent.applyEvent.applyTime + ) + } + } + } } } diff --git a/Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift b/Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift index b19d8630..6d37d168 100644 --- a/Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift +++ b/Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift @@ -2,10 +2,10 @@ import Foundation struct ApplyEvent: Codable { let applyTime: Date - var sent: Bool + var status: ApplyEventStatus - init(applyTime: Date, sent: Bool = false) { + init(applyTime: Date, status: ApplyEventStatus = .created) { self.applyTime = applyTime - self.sent = sent + self.status = status } } diff --git a/Sources/ConfidenceProvider/Cache/Models/ApplyEventStatus.swift b/Sources/ConfidenceProvider/Cache/Models/ApplyEventStatus.swift new file mode 100644 index 00000000..ec6087ae --- /dev/null +++ b/Sources/ConfidenceProvider/Cache/Models/ApplyEventStatus.swift @@ -0,0 +1,7 @@ +import Foundation + +enum ApplyEventStatus: Codable { + case created + case sending + case sent +} diff --git a/Sources/ConfidenceProvider/Cache/Models/CacheData.swift b/Sources/ConfidenceProvider/Cache/Models/CacheData.swift index eac6ba63..222f087a 100644 --- a/Sources/ConfidenceProvider/Cache/Models/CacheData.swift +++ b/Sources/ConfidenceProvider/Cache/Models/CacheData.swift @@ -35,23 +35,23 @@ struct CacheData: Codable { return resolveTokenIndex != nil } - mutating func setEventSent(resolveToken: String, name: String, sent: Bool = true) { + mutating func setEventStatus(resolveToken: String, name: String, status: ApplyEventStatus = .sent) { let flagEventIndexes = flagEventIndex(resolveToken: resolveToken, name: name) guard let resolveIndex = flagEventIndexes.resolveEventIndex, let flagIndex = flagEventIndexes.flagEventIndex else { return } - resolveEvents[resolveIndex].events[flagIndex].applyEvent.sent = sent + resolveEvents[resolveIndex].events[flagIndex].applyEvent.status = status } - mutating func setEventSent(resolveToken: String, sent: Bool = true) { + mutating func setEventStatus(resolveToken: String, status: ApplyEventStatus = .sent) { guard let resolveIndex = resolveEventIndex(resolveToken: resolveToken) else { return } for i in 0...resolveEvents[resolveIndex].events.count-1 { - resolveEvents[resolveIndex].events[i].applyEvent.sent = sent + resolveEvents[resolveIndex].events[i].applyEvent.status = status } } diff --git a/Sources/ConfidenceProvider/Cache/Models/ResolveApply.swift b/Sources/ConfidenceProvider/Cache/Models/ResolveApply.swift index df4f8140..858cde75 100644 --- a/Sources/ConfidenceProvider/Cache/Models/ResolveApply.swift +++ b/Sources/ConfidenceProvider/Cache/Models/ResolveApply.swift @@ -8,6 +8,10 @@ struct ResolveApply: Codable { resolveToken.isEmpty || events.isEmpty } + var isSent: Bool { + events.allSatisfy { $0.applyEvent.status == .sent } + } + init(resolveToken: String, flagName: String, applyTime: Date) { self.resolveToken = resolveToken self.events = [FlagApply(name: flagName, applyTime: applyTime)] diff --git a/Tests/ConfidenceProviderTests/CacheDataTests.swift b/Tests/ConfidenceProviderTests/CacheDataTests.swift index 4c72f177..82aee772 100644 --- a/Tests/ConfidenceProviderTests/CacheDataTests.swift +++ b/Tests/ConfidenceProviderTests/CacheDataTests.swift @@ -116,15 +116,15 @@ final class CacheDataTests: XCTestCase { // and all apply events has sent property set to false var cacheData = try CacheDataUtility.prefilledCacheData() let resolve = try XCTUnwrap(cacheData.resolveEvents.first) - let sentEvents = resolve.events.filter { $0.applyEvent.sent == true } + let sentEvents = resolve.events.filter { $0.applyEvent.status == .sent } XCTAssertEqual(sentEvents.count, 0) // When set event sent is called for item that exists in cache - cacheData.setEventSent(resolveToken: "token0", name: "prefilled2") + cacheData.setEventStatus(resolveToken: "token0", name: "prefilled2") let resolveAfterUpdate = try XCTUnwrap(cacheData.resolveEvents.first) // Then apply event sent property has been set to true - let sentEventsAfterUpdate = resolveAfterUpdate.events.filter { $0.applyEvent.sent == true } + let sentEventsAfterUpdate = resolveAfterUpdate.events.filter { $0.applyEvent.status == .sent } XCTAssertEqual(sentEventsAfterUpdate.count, 1) XCTAssertEqual(sentEventsAfterUpdate.first?.name, "prefilled2") } @@ -134,15 +134,15 @@ final class CacheDataTests: XCTestCase { // and all apply events has sent property set to false var cacheData = try CacheDataUtility.prefilledCacheData() let resolve = try XCTUnwrap(cacheData.resolveEvents.first) - let sentEvents = resolve.events.filter { $0.applyEvent.sent == true } + let sentEvents = resolve.events.filter { $0.applyEvent.status == .sent } XCTAssertEqual(sentEvents.count, 0) // When set event sent is called for item that does not exists in cache - cacheData.setEventSent(resolveToken: "token0", name: "prefilled45") + cacheData.setEventStatus(resolveToken: "token0", name: "prefilled45") let resolveAfterUpdate = try XCTUnwrap(cacheData.resolveEvents.first) // Then apply event sent property has not been changed - let sentEventsAfterUpdate = resolveAfterUpdate.events.filter { $0.applyEvent.sent == true } + let sentEventsAfterUpdate = resolveAfterUpdate.events.filter { $0.applyEvent.status == .sent } XCTAssertEqual(sentEventsAfterUpdate.count, 0) } diff --git a/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift b/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift index dbf996c1..05c63125 100644 --- a/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift +++ b/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift @@ -49,8 +49,13 @@ class FlagApplierWithRetriesTest: XCTestCase { func testApply_differentFlags() async { // Given flag applier + let cacheDataInteractor = CacheDataInteractor(storage: storage) let applier = FlagApplierWithRetries( - httpClient: httpClient, storage: storage, options: options, triggerBatch: false + httpClient: httpClient, + storage: storage, + options: options, + cacheDataInteractor: cacheDataInteractor, + triggerBatch: false ) // When 3 apply calls are issued with different flag names @@ -58,22 +63,44 @@ class FlagApplierWithRetriesTest: XCTestCase { await applier.apply(flagName: "flag2", resolveToken: "token1") await applier.apply(flagName: "flag3", resolveToken: "token1") - // Then http client sends only 3 post requests + let cacheData = await cacheDataInteractor.cache + + // Then http client sends 3 post requests XCTAssertEqual(httpClient.postCallCounter, 3) + XCTAssertEqual(cacheData.resolveEvents.count, 1) + XCTAssertEqual(cacheData.resolveEvents[0].events.count, 3) } - func testApply_doesNotstoreOnDisk() async throws { + func testApply_doesNotStoreOnDisk() async throws { // Given flag applier + let cacheDataInteractor = CacheDataInteractor(storage: storage) let applier = FlagApplierWithRetries( - httpClient: httpClient, storage: storage, options: options, triggerBatch: false + httpClient: httpClient, + storage: storage, + options: options, + cacheDataInteractor: cacheDataInteractor, + triggerBatch: false ) + let networkExpectation = self.expectation(description: "Waiting for network call to complete") + networkExpectation.expectedFulfillmentCount = 3 + httpClient.expectation = networkExpectation + + // When 3 apply calls are issued with different flag names await applier.apply(flagName: "flag1", resolveToken: "token1") await applier.apply(flagName: "flag2", resolveToken: "token1") await applier.apply(flagName: "flag3", resolveToken: "token1") + await waitForExpectations(timeout: 1.0) + // Then cache data is not stored on to the disk + // But stored in the local cache as sent + let cacheData = await cacheDataInteractor.cache + XCTAssertEqual(cacheData.resolveEvents.count, 1) + XCTAssertEqual(cacheData.resolveEvents[0].events.count, 3) + XCTAssertTrue(cacheData.resolveEvents[0].events.allSatisfy { $0.applyEvent.status == .sent }) + let storedData = try XCTUnwrap(storage.load(defaultValue: CacheData.empty())) XCTAssertEqual(storedData.resolveEvents.count, 0) } @@ -101,20 +128,22 @@ class FlagApplierWithRetriesTest: XCTestCase { let prefilledCache = try CacheDataUtility.prefilledCacheData(applyEventCount: 100) try prefilledStorage.save(data: prefilledCache) - let expectation = XCTestExpectation() + let expectation = self.expectation(description: "Waiting for network call to complete") httpClient.expectation = expectation + let storageExpectation = self.expectation(description: "Waiting for storage expectation to be completed") + storageExpectation.expectedFulfillmentCount = 2 + prefilledStorage.saveExpectation = storageExpectation + + // When flag applier is initialised - let task = Task { - _ = FlagApplierWithRetries( - httpClient: httpClient, - storage: prefilledStorage, - options: options - ) - } - await task.value + _ = FlagApplierWithRetries( + httpClient: httpClient, + storage: prefilledStorage, + options: options + ) - wait(for: [expectation], timeout: 5) + await waitForExpectations(timeout: 5.0) // Then http client sends apply flags batch request, containing 100 records let request = try XCTUnwrap(httpClient.data?.first as? ApplyFlagsRequest) @@ -125,9 +154,9 @@ class FlagApplierWithRetriesTest: XCTestCase { func testApply_multipleApplyCalls_batchTriggered() async throws { // Given flag applier with http client that is offline let httpClient = HttpClientMock(testMode: .error) - let expectation = XCTestExpectation(description: "Waiting for batch trigger") - expectation.expectedFulfillmentCount = 3 - httpClient.expectation = expectation + let networkExpectation = self.expectation(description: "Waiting for batch trigger") + networkExpectation.expectedFulfillmentCount = 2 + httpClient.expectation = networkExpectation let applier = FlagApplierWithRetries( httpClient: httpClient, @@ -145,33 +174,35 @@ class FlagApplierWithRetriesTest: XCTestCase { httpClient.testMode = .success await applier.apply(flagName: "flag2", resolveToken: "token1") - wait(for: [expectation], timeout: 1) + await waitForExpectations(timeout: 1.0) - // Then 3 post calls are issued (one offline, one single apply, one batch request) - XCTAssertEqual(httpClient.postCallCounter, 3) - XCTAssertEqual(httpClient.data?.count, 3) + // Then 3 post calls are issued (one offline, one batch apply containing 2 reconrds) + XCTAssertEqual(httpClient.postCallCounter, 2) + XCTAssertEqual(httpClient.data?.count, 2) let request1 = try XCTUnwrap(httpClient.data?[0] as? ApplyFlagsRequest) let request2 = try XCTUnwrap(httpClient.data?[1] as? ApplyFlagsRequest) -// let request3 = try XCTUnwrap(httpClient.data?[2] as? ApplyFlagsRequest) XCTAssertEqual(request1.flags.count, 1) XCTAssertEqual(request1.flags.first?.flag, "flags/flag1") - XCTAssertEqual(request2.flags.count, 1) - XCTAssertEqual(request2.flags.first?.flag, "flags/flag2") -// XCTAssertEqual(request3.flags.count, 1) -// XCTAssertEqual(request3.flags.first?.flag, "flags/flag1") + XCTAssertEqual(request2.flags.count, 2) + XCTAssertEqual(request2.flags.first?.flag, "flags/flag1") + XCTAssertEqual(request2.flags.last?.flag, "flags/flag2") } func testApply_multipleApplyCalls_sentSet() async throws { // Given flag applier with http client that is offline let cacheDataInteractor = CacheDataInteractor(storage: storage) - let httpClient = HttpClientMock(testMode: .error) - let expectation = XCTestExpectation(description: "Waiting for batch trigger") - expectation.expectedFulfillmentCount = 3 - httpClient.expectation = expectation + let offlineClient = HttpClientMock(testMode: .error) + let networkExpectation = self.expectation(description: "Waiting for network call to complete") + networkExpectation.expectedFulfillmentCount = 2 + offlineClient.expectation = networkExpectation + + let storageExpectation = self.expectation(description: "Waiting for storage expectation to be completed") + storageExpectation.expectedFulfillmentCount = 6 + storage.saveExpectation = storageExpectation let applier = FlagApplierWithRetries( - httpClient: httpClient, + httpClient: offlineClient, storage: storage, options: options, cacheDataInteractor: cacheDataInteractor, @@ -184,42 +215,41 @@ class FlagApplierWithRetriesTest: XCTestCase { // And second apply call is issued // With test mode .success - httpClient.testMode = .success + offlineClient.testMode = .success await applier.apply(flagName: "flag2", resolveToken: "token1") - wait(for: [expectation], timeout: 1) + await waitForExpectations(timeout: 1.0) - Task { - // Then both requests are marked as sent in cache data - let cacheData = await cacheDataInteractor.cache - let flagEvent1 = cacheData.flagEvent(resolveToken: "token1", name: "flag1") - let flagEvent2 = cacheData.flagEvent(resolveToken: "token1", name: "flag2") + // Then both requests are marked as sent in cache data + let cacheData = await cacheDataInteractor.cache + let flagEvent1 = cacheData.flagEvent(resolveToken: "token1", name: "flag1") + let flagEvent2 = cacheData.flagEvent(resolveToken: "token1", name: "flag2") - XCTAssertEqual(flagEvent1?.applyEvent.sent, true) - XCTAssertEqual(flagEvent2?.applyEvent.sent, true) - } + XCTAssertEqual(flagEvent1?.applyEvent.status, .sent) + XCTAssertEqual(flagEvent2?.applyEvent.status, .sent) } func testApply_previoslyStoredData_cleanAfterSending() async throws { // Given storage that has previously stored data (100 records, same token) let prefilledStorage = StorageMock() - let prefilledCache = try CacheDataUtility.prefilledCacheData(applyEventCount: 100) + let prefilledCache = try CacheDataUtility.prefilledCacheData(applyEventCount: 10) try prefilledStorage.save(data: prefilledCache) - let expectation = XCTestExpectation(description: "Waiting for batch trigger") - prefilledStorage.saveExpectation = expectation + let storageExpectation = self.expectation(description: "Waiting for storage expectation to be completed") + storageExpectation.expectedFulfillmentCount = 2 + prefilledStorage.saveExpectation = storageExpectation + + let networkExpectation = self.expectation(description: "Waiting for networkRequest to be completed") + httpClient.expectation = networkExpectation // When flag applier is initialised // And apply flags batch request is successful - let task = Task { - _ = FlagApplierWithRetries( - httpClient: httpClient, - storage: prefilledStorage, - options: options - ) - } - await task.value + _ = FlagApplierWithRetries( + httpClient: httpClient, + storage: prefilledStorage, + options: options + ) - wait(for: [expectation], timeout: 5) + await waitForExpectations(timeout: 1.0) // Then storage has been cleaned let storedData = try prefilledStorage.load(defaultValue: CacheData.empty()) @@ -311,9 +341,13 @@ class FlagApplierWithRetriesTest: XCTestCase { // Given flag applier set up with offline http client // And storage that has previously stored 1 record let offlineClient = HttpClientMock(testMode: .error) - let prefilledStorage = StorageMock() let data = CacheData(resolveToken: "token0", flagName: "flag1", applyTime: Date(timeIntervalSince1970: 1000)) - try prefilledStorage.save(data: data) + let prefilledStorage = try StorageMock(data: data) + + let networkExpectation = self.expectation(description: "Waiting for networkRequest to be completed") + networkExpectation.expectedFulfillmentCount = 2 + offlineClient.expectation = networkExpectation + let applier = FlagApplierWithRetries( httpClient: offlineClient, storage: prefilledStorage, @@ -325,6 +359,8 @@ class FlagApplierWithRetriesTest: XCTestCase { // And http client request fails with .invalidResponse await applier.apply(flagName: "flag1", resolveToken: "token1") + await waitForExpectations(timeout: 5.0) + // Then 2 resolve event records are stored on disk let storedData: CacheData = try XCTUnwrap(prefilledStorage.load(defaultValue: CacheData.empty())) XCTAssertEqual(storedData.resolveEvents.count, 2) @@ -334,36 +370,17 @@ class FlagApplierWithRetriesTest: XCTestCase { XCTAssertEqual(newResolveEvent.events.count, 1) XCTAssertEqual(newResolveEvent.events[0].name, "flag1") XCTAssertEqual(newResolveEvent.events[0].applyEvent.applyTime, Date(timeIntervalSince1970: 1000)) - XCTAssertEqual(newResolveEvent.events[0].applyEvent.sent, false) - } - - func testApplyOffline_previoslyStoredData_100records() async throws { - // Given flag applier set up with offline http client - // And storage that has previously stored 100 records with different tokens - let offlineClient = HttpClientMock(testMode: .error) - let prefilledStorage = StorageMock() - let prefilledCache = try CacheDataUtility.prefilledCacheData(resolveEventCount: 100) - try prefilledStorage.save(data: prefilledCache) - let applier = FlagApplierWithRetries( - httpClient: offlineClient, - storage: prefilledStorage, - options: options, - triggerBatch: false - ) - - // When apply call is issued with another token - // And http client request fails with .invalidResponse - await applier.apply(flagName: "flag1", resolveToken: "token1") - - // Then 100 resolve event records are stored on disk - let storedData: CacheData = try XCTUnwrap(prefilledStorage.load(defaultValue: CacheData.empty())) - XCTAssertEqual(storedData.resolveEvents.count, 100) + XCTAssertEqual(newResolveEvent.events[0].applyEvent.status, .created) } func testApplyOffline_100applyCalls_sameToken() async throws { // Given flag applier set up with offline http client // And storage that has previously stored 100 records with same token let offlineClient = HttpClientMock(testMode: .error) + let networkExpectation = self.expectation(description: "Waiting for networkRequest to be completed") + networkExpectation.expectedFulfillmentCount = 100 + offlineClient.expectation = networkExpectation + let prefilledStorage = StorageMock() let prefilledCache = try CacheDataUtility.prefilledCacheData(applyEventCount: 100) try prefilledStorage.save(data: prefilledCache) @@ -377,6 +394,7 @@ class FlagApplierWithRetriesTest: XCTestCase { // When 100 apply calls are issued // And all http client requests fails with .invalidResponse await hundredApplyCalls(applier: applier, sameToken: true) + await waitForExpectations(timeout: 1.0) // Then 1 resolve event record is stored on disk // And 200 flag event records are stored on disk diff --git a/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift b/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift index 6ac0336d..2d7902bb 100644 --- a/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift +++ b/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift @@ -3,10 +3,6 @@ import Foundation @testable import ConfidenceProvider final actor CacheDataInteractorMock: CacheDataActor { - func setEventSent(resolveToken: String) -> ConfidenceProvider.CacheData { - return cache - } - var cache = CacheData.empty() func add(resolveToken: String, flagName: String, applyTime: Date) -> (ConfidenceProvider.CacheData, Bool) { @@ -21,8 +17,12 @@ final actor CacheDataInteractorMock: CacheDataActor { return false } - func setEventSent(resolveToken: String, name: String) -> ConfidenceProvider.CacheData { - return cache + func setEventStatus(resolveToken: String, name: String, status: ApplyEventStatus) -> CacheData { + cache + } + + func setEventStatus(resolveToken: String, status: ApplyEventStatus) -> CacheData { + cache } func loadCacheFromStorage() { From e561b18afc16d54d3b10d8087d3dae33ea8f0836 Mon Sep 17 00:00:00 2001 From: Alina Andersone Date: Mon, 7 Aug 2023 09:23:11 +0200 Subject: [PATCH 7/9] Removed storage from interactor to avoid unpredictable test results --- .../Apply/FlagApplierWithRetries.swift | 3 +- .../Cache/CacheDataInteractor.swift | 28 ++-------------- .../CacheDataInteractorTests.swift | 9 ++---- .../FlagApplierWithRetriesTest.swift | 32 ++++++++++++++++--- .../Helpers/CacheDataInteractorMock.swift | 4 --- 5 files changed, 35 insertions(+), 41 deletions(-) diff --git a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift index 72fc8057..763815e7 100644 --- a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift +++ b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift @@ -26,7 +26,8 @@ final class FlagApplierWithRetries: FlagApplier { // Read/Write operations to/from the file are not protected by the cacheDataActor // It's important to not write the file before this operation is completed (reading from file) - self.cacheDataInteractor = cacheDataInteractor ?? CacheDataInteractor(storage: storage) + let storedData = try? storage.load(defaultValue: CacheData.empty()) + self.cacheDataInteractor = cacheDataInteractor ?? CacheDataInteractor(cacheData: storedData ?? .empty()) if triggerBatch { Task(priority: .utility) { diff --git a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift index 25eee8bf..f082a2fb 100644 --- a/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift +++ b/Sources/ConfidenceProvider/Cache/CacheDataInteractor.swift @@ -1,15 +1,10 @@ import Foundation final actor CacheDataInteractor: CacheDataActor { - private let storage: Storage var cache = CacheData.empty() - init(storage: Storage) { - self.storage = storage - - Task(priority: .high) { - await loadCacheFromStorage() - } + init(cacheData: CacheData) { + cache = cacheData } func add(resolveToken: String, flagName: String, applyTime: Date) -> (CacheData, Bool) { @@ -47,23 +42,4 @@ final actor CacheDataInteractor: CacheDataActor { cache.setEventStatus(resolveToken: resolveToken, status: status) return cache } - - private func loadCacheFromStorage() { - guard let storedData = try? storage.load(defaultValue: cache), storedData.isEmpty == false else { - return - } - if self.cache.isEmpty { - self.cache = storedData - } else { - storedData.resolveEvents.forEach { resolveEvent in - resolveEvent.events.forEach { flagApplyEvent in - _ = self.cache.add( - resolveToken: resolveEvent.resolveToken, - flagName: flagApplyEvent.name, - applyTime: flagApplyEvent.applyEvent.applyTime - ) - } - } - } - } } diff --git a/Tests/ConfidenceProviderTests/CacheDataInteractorTests.swift b/Tests/ConfidenceProviderTests/CacheDataInteractorTests.swift index 48ea7a65..ab6a1249 100644 --- a/Tests/ConfidenceProviderTests/CacheDataInteractorTests.swift +++ b/Tests/ConfidenceProviderTests/CacheDataInteractorTests.swift @@ -11,10 +11,9 @@ final class CacheDataInteractorTests: XCTestCase { resolveEventCount: 10, applyEventCount: 20 ) - let prefilledStorage = try StorageMock(data: prefilledCache) // When cache data interactor is initialised - let cacheDataInteractor = CacheDataInteractor(storage: prefilledStorage) + let cacheDataInteractor = CacheDataInteractor(cacheData: prefilledCache) // Then cache data is loaded from storage Task { @@ -27,8 +26,7 @@ final class CacheDataInteractorTests: XCTestCase { func testCacheDataInteractor_addEventToEmptyCache() async throws { // Given cache data interactor with no previously stored data - let storage = StorageMock() - let cacheDataInteractor = CacheDataInteractor(storage: storage) + let cacheDataInteractor = CacheDataInteractor(cacheData: .empty()) Task { let cache = await cacheDataInteractor.cache XCTAssertEqual(cache.resolveEvents.count, 0) @@ -47,8 +45,7 @@ final class CacheDataInteractorTests: XCTestCase { func testCacheDataInteractor_addEventToPreFilledCache() async throws { // Given cache data interactor with previously stored data (1 resolve token and 2 apply event) let prefilledCacheData = try CacheDataUtility.prefilledCacheData(applyEventCount: 2) - let prefilledStorage = try StorageMock(data: prefilledCacheData) - let cacheDataInteractor = CacheDataInteractor(storage: prefilledStorage) + let cacheDataInteractor = CacheDataInteractor(cacheData: prefilledCacheData) Task { // When cache data add method is called diff --git a/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift b/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift index 05c63125..b35b2837 100644 --- a/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift +++ b/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift @@ -49,7 +49,7 @@ class FlagApplierWithRetriesTest: XCTestCase { func testApply_differentFlags() async { // Given flag applier - let cacheDataInteractor = CacheDataInteractor(storage: storage) + let cacheDataInteractor = CacheDataInteractor(cacheData: .empty()) let applier = FlagApplierWithRetries( httpClient: httpClient, storage: storage, @@ -73,7 +73,7 @@ class FlagApplierWithRetriesTest: XCTestCase { func testApply_doesNotStoreOnDisk() async throws { // Given flag applier - let cacheDataInteractor = CacheDataInteractor(storage: storage) + let cacheDataInteractor = CacheDataInteractor(cacheData: .empty()) let applier = FlagApplierWithRetries( httpClient: httpClient, storage: storage, @@ -191,7 +191,7 @@ class FlagApplierWithRetriesTest: XCTestCase { func testApply_multipleApplyCalls_sentSet() async throws { // Given flag applier with http client that is offline - let cacheDataInteractor = CacheDataInteractor(storage: storage) + let cacheDataInteractor = CacheDataInteractor(cacheData: .empty()) let offlineClient = HttpClientMock(testMode: .error) let networkExpectation = self.expectation(description: "Waiting for network call to complete") networkExpectation.expectedFulfillmentCount = 2 @@ -231,7 +231,7 @@ class FlagApplierWithRetriesTest: XCTestCase { func testApply_previoslyStoredData_cleanAfterSending() async throws { // Given storage that has previously stored data (100 records, same token) let prefilledStorage = StorageMock() - let prefilledCache = try CacheDataUtility.prefilledCacheData(applyEventCount: 10) + let prefilledCache = try CacheDataUtility.prefilledCacheData(applyEventCount: 100) try prefilledStorage.save(data: prefilledCache) let storageExpectation = self.expectation(description: "Waiting for storage expectation to be completed") @@ -373,6 +373,30 @@ class FlagApplierWithRetriesTest: XCTestCase { XCTAssertEqual(newResolveEvent.events[0].applyEvent.status, .created) } + + func testApplyOffline_previoslyStoredData_100records() async throws { + // Given flag applier set up with offline http client + // And storage that has previously stored 100 records with different tokens + let offlineClient = HttpClientMock(testMode: .error) + let prefilledStorage = StorageMock() + let prefilledCache = try CacheDataUtility.prefilledCacheData(resolveEventCount: 100) + try prefilledStorage.save(data: prefilledCache) + let applier = FlagApplierWithRetries( + httpClient: offlineClient, + storage: prefilledStorage, + options: options, + triggerBatch: false + ) + + // When apply call is issued with another token + // And http client request fails with .invalidResponse + await applier.apply(flagName: "flag1", resolveToken: "token1") + + // Then 100 resolve event records are stored on disk + let storedData: CacheData = try XCTUnwrap(prefilledStorage.load(defaultValue: CacheData.empty())) + XCTAssertEqual(storedData.resolveEvents.count, 100) + } + func testApplyOffline_100applyCalls_sameToken() async throws { // Given flag applier set up with offline http client // And storage that has previously stored 100 records with same token diff --git a/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift b/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift index 2d7902bb..9d4216e1 100644 --- a/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift +++ b/Tests/ConfidenceProviderTests/Helpers/CacheDataInteractorMock.swift @@ -24,8 +24,4 @@ final actor CacheDataInteractorMock: CacheDataActor { func setEventStatus(resolveToken: String, status: ApplyEventStatus) -> CacheData { cache } - - func loadCacheFromStorage() { - return - } } From 832b5206d57b263fcbcd7d507d53dbc101a70420 Mon Sep 17 00:00:00 2001 From: Alina Andersone Date: Wed, 9 Aug 2023 09:17:40 +0200 Subject: [PATCH 8/9] Added chunking capabilities to trigger batch network request --- .../Apply/FlagApplierWithRetries.swift | 28 +++--- .../Utils/Array+Chunks.swift | 9 ++ .../FlagApplierWithRetriesTest.swift | 85 +++++++++++++++++-- .../Helpers/HttpClientMock.swift | 7 ++ 4 files changed, 109 insertions(+), 20 deletions(-) create mode 100644 Sources/ConfidenceProvider/Utils/Array+Chunks.swift diff --git a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift index 763815e7..b09c1811 100644 --- a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift +++ b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift @@ -24,8 +24,6 @@ final class FlagApplierWithRetries: FlagApplier { self.httpClient = httpClient self.options = options - // Read/Write operations to/from the file are not protected by the cacheDataActor - // It's important to not write the file before this operation is completed (reading from file) let storedData = try? storage.load(defaultValue: CacheData.empty()) self.cacheDataInteractor = cacheDataInteractor ?? CacheDataInteractor(cacheData: storedData ?? .empty()) @@ -61,21 +59,25 @@ final class FlagApplierWithRetries: FlagApplier { await cacheData.resolveEvents.forEach { resolveEvent in let appliesToSend = resolveEvent.events.filter { entry in return entry.applyEvent.status == .created - } + }.chunk(size: 20) + guard appliesToSend.isEmpty == false else { return } - self.writeStatus(resolveToken: resolveEvent.resolveToken, events: appliesToSend, status: .sending) - executeApply( - resolveToken: resolveEvent.resolveToken, - items: appliesToSend - ) { success in - guard success else { - self.writeStatus(resolveToken: resolveEvent.resolveToken, events: appliesToSend, status: .created) - return + + appliesToSend.forEach { chunk in + self.writeStatus(resolveToken: resolveEvent.resolveToken, events: chunk, status: .sending) + executeApply( + resolveToken: resolveEvent.resolveToken, + items: chunk + ) { success in + guard success else { + self.writeStatus(resolveToken: resolveEvent.resolveToken, events: chunk, status: .created) + return + } + // Set 'sent' property of apply events to true + self.writeStatus(resolveToken: resolveEvent.resolveToken, events: chunk, status: .sent) } - // Set 'sent' property of apply events to true - self.writeStatus(resolveToken: resolveEvent.resolveToken, events: appliesToSend, status: .sent) } } } diff --git a/Sources/ConfidenceProvider/Utils/Array+Chunks.swift b/Sources/ConfidenceProvider/Utils/Array+Chunks.swift new file mode 100644 index 00000000..fd0e9f64 --- /dev/null +++ b/Sources/ConfidenceProvider/Utils/Array+Chunks.swift @@ -0,0 +1,9 @@ +import Foundation + +extension Array { + func chunk(size: Int) -> [[Element]] { + return stride(from: 0, to: count, by: size).map { + Array(self[$0 ..< Swift.min($0 + size, count)]) + } + } +} diff --git a/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift b/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift index b35b2837..30445209 100644 --- a/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift +++ b/Tests/ConfidenceProviderTests/FlagApplierWithRetriesTest.swift @@ -129,10 +129,11 @@ class FlagApplierWithRetriesTest: XCTestCase { try prefilledStorage.save(data: prefilledCache) let expectation = self.expectation(description: "Waiting for network call to complete") + expectation.expectedFulfillmentCount = 5 httpClient.expectation = expectation let storageExpectation = self.expectation(description: "Waiting for storage expectation to be completed") - storageExpectation.expectedFulfillmentCount = 2 + storageExpectation.expectedFulfillmentCount = 10 prefilledStorage.saveExpectation = storageExpectation @@ -145,10 +146,48 @@ class FlagApplierWithRetriesTest: XCTestCase { await waitForExpectations(timeout: 5.0) - // Then http client sends apply flags batch request, containing 100 records + // Then http client sends 5 apply flag batch request, containing 20 records each let request = try XCTUnwrap(httpClient.data?.first as? ApplyFlagsRequest) - XCTAssertEqual(httpClient.postCallCounter, 1) - XCTAssertEqual(request.flags.count, 100) + XCTAssertEqual(httpClient.postCallCounter, 5) + XCTAssertEqual(request.flags.count, 20) + } + + func testApply_previoslyStoredData_partialFailure() async throws { + // Given storage that has previously stored data (100 records, same token) + let partiallyFailingHttpClient = HttpClientMock(testMode: .failFirstChunk) + let prefilledStorage = StorageMock() + let prefilledCache = try CacheDataUtility.prefilledCacheData(applyEventCount: 100) + try prefilledStorage.save(data: prefilledCache) + + let expectation = self.expectation(description: "Waiting for network call to complete") + expectation.expectedFulfillmentCount = 5 + partiallyFailingHttpClient.expectation = expectation + + let storageExpectation = self.expectation(description: "Waiting for storage expectation to be completed") + storageExpectation.expectedFulfillmentCount = 10 + prefilledStorage.saveExpectation = storageExpectation + + + // When flag applier is initialised + _ = FlagApplierWithRetries( + httpClient: partiallyFailingHttpClient, + storage: prefilledStorage, + options: options + ) + + await waitForExpectations(timeout: 5.0) + + // Then http client sends 5 apply flags batch request, containing 20 records each + let request = try XCTUnwrap(partiallyFailingHttpClient.data?.first as? ApplyFlagsRequest) + XCTAssertEqual(partiallyFailingHttpClient.postCallCounter, 5) + XCTAssertEqual(request.flags.count, 20) + + // And storage has 20 failed events saved + let storedData = try prefilledStorage.load(defaultValue: CacheData.empty()) + XCTAssertEqual(storedData.resolveEvents.count, 1) + + let unsent = try XCTUnwrap(storedData.resolveEvents.first?.events.filter { $0.applyEvent.status == .created }) + XCTAssertEqual(unsent.count, 20) } func testApply_multipleApplyCalls_batchTriggered() async throws { @@ -235,10 +274,11 @@ class FlagApplierWithRetriesTest: XCTestCase { try prefilledStorage.save(data: prefilledCache) let storageExpectation = self.expectation(description: "Waiting for storage expectation to be completed") - storageExpectation.expectedFulfillmentCount = 2 + storageExpectation.expectedFulfillmentCount = 10 prefilledStorage.saveExpectation = storageExpectation let networkExpectation = self.expectation(description: "Waiting for networkRequest to be completed") + networkExpectation.expectedFulfillmentCount = 5 httpClient.expectation = networkExpectation // When flag applier is initialised @@ -253,7 +293,34 @@ class FlagApplierWithRetriesTest: XCTestCase { // Then storage has been cleaned let storedData = try prefilledStorage.load(defaultValue: CacheData.empty()) - XCTAssertEqual(httpClient.postCallCounter, 1) + XCTAssertEqual(httpClient.postCallCounter, 5) + XCTAssertEqual(storedData.resolveEvents.count, 0) + } + + func testApply_100applyCalls_sameToken() async throws { + // Given flag applier set up with offline http client + // And storage that has previously stored 100 records with same token + let networkExpectation = self.expectation(description: "Waiting for networkRequest to be completed") + networkExpectation.expectedFulfillmentCount = 105 + httpClient.expectation = networkExpectation + + let prefilledStorage = StorageMock() + let prefilledCache = try CacheDataUtility.prefilledCacheData(applyEventCount: 100) + try prefilledStorage.save(data: prefilledCache) + let applier = FlagApplierWithRetries( + httpClient: httpClient, + storage: prefilledStorage, + options: options, + triggerBatch: false + ) + + // When 100 apply calls are issued + // And all http client requests fails with .invalidResponse + await hundredApplyCalls(applier: applier, sameToken: true) + await waitForExpectations(timeout: 1.0) + + // Then strored data is empty + let storedData: CacheData = try XCTUnwrap(prefilledStorage.load(defaultValue: CacheData.empty())) XCTAssertEqual(storedData.resolveEvents.count, 0) } @@ -402,7 +469,11 @@ class FlagApplierWithRetriesTest: XCTestCase { // And storage that has previously stored 100 records with same token let offlineClient = HttpClientMock(testMode: .error) let networkExpectation = self.expectation(description: "Waiting for networkRequest to be completed") - networkExpectation.expectedFulfillmentCount = 100 + + // Since we don't fail other requests when one request is failing + // This setup gives us 800 network calls + // Every request is split in 6 to 10 batches (from 101 - 200 apply events) + networkExpectation.expectedFulfillmentCount = 800 offlineClient.expectation = networkExpectation let prefilledStorage = StorageMock() diff --git a/Tests/ConfidenceProviderTests/Helpers/HttpClientMock.swift b/Tests/ConfidenceProviderTests/Helpers/HttpClientMock.swift index b972e2e1..a9ccb5c8 100644 --- a/Tests/ConfidenceProviderTests/Helpers/HttpClientMock.swift +++ b/Tests/ConfidenceProviderTests/Helpers/HttpClientMock.swift @@ -11,6 +11,7 @@ final class HttpClientMock: HttpClient { enum TestMode { case success + case failFirstChunk case error } @@ -56,6 +57,12 @@ final class HttpClientMock: HttpClient { switch testMode { case .success: return HttpClientResponse(response: HTTPURLResponse()) + case .failFirstChunk: + if postCallCounter == 1 { + throw HttpClientError.invalidResponse + } else { + return HttpClientResponse(response: HTTPURLResponse()) + } case .error: throw HttpClientError.invalidResponse } From f0ad926a038c4b5fcbe56f04e4c52b0f79e11316 Mon Sep 17 00:00:00 2001 From: Alina Andersone Date: Fri, 11 Aug 2023 14:39:37 +0200 Subject: [PATCH 9/9] Remove unnecessary nested data structure [ApplyEvent]. --- .../Apply/FlagApplierWithRetries.swift | 8 +++----- .../Cache/Models/ApplyEvent.swift | 11 ----------- .../Cache/Models/CacheData.swift | 14 ++++++-------- .../Cache/Models/FlagApply.swift | 8 +++++--- .../Cache/Models/ResolveApply.swift | 2 +- Tests/ConfidenceProviderTests/CacheDataTests.swift | 14 +++++++------- .../FlagApplierWithRetriesTest.swift | 12 ++++++------ 7 files changed, 28 insertions(+), 41 deletions(-) delete mode 100644 Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift diff --git a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift index b09c1811..55b3ddb6 100644 --- a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift +++ b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift @@ -5,8 +5,6 @@ import os typealias ApplyFlagHTTPResponse = HttpClientResponse typealias ApplyFlagResult = Result -// swiftlint:disable type_body_length -// swiftlint:disable file_length final class FlagApplierWithRetries: FlagApplier { private let storage: Storage private let httpClient: HttpClient @@ -58,7 +56,7 @@ final class FlagApplierWithRetries: FlagApplier { async let cacheData = await cacheDataInteractor.cache await cacheData.resolveEvents.forEach { resolveEvent in let appliesToSend = resolveEvent.events.filter { entry in - return entry.applyEvent.status == .created + return entry.status == .created }.chunk(size: 20) guard appliesToSend.isEmpty == false else { @@ -84,7 +82,7 @@ final class FlagApplierWithRetries: FlagApplier { private func writeStatus(resolveToken: String, events: [FlagApply], status: ApplyEventStatus) { let lastIndex = events.count - 1 - events.enumerated().forEach { (index, event) in + events.enumerated().forEach { index, event in Task(priority: .medium) { var data = await self.cacheDataInteractor.setEventStatus( resolveToken: resolveToken, @@ -115,7 +113,7 @@ final class FlagApplierWithRetries: FlagApplier { let applyFlagRequestItems = items.map { applyEvent in AppliedFlagRequestItem( flag: applyEvent.name, - applyTime: applyEvent.applyEvent.applyTime + applyTime: applyEvent.applyTime ) } let request = ApplyFlagsRequest( diff --git a/Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift b/Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift deleted file mode 100644 index 6d37d168..00000000 --- a/Sources/ConfidenceProvider/Cache/Models/ApplyEvent.swift +++ /dev/null @@ -1,11 +0,0 @@ -import Foundation - -struct ApplyEvent: Codable { - let applyTime: Date - var status: ApplyEventStatus - - init(applyTime: Date, status: ApplyEventStatus = .created) { - self.applyTime = applyTime - self.status = status - } -} diff --git a/Sources/ConfidenceProvider/Cache/Models/CacheData.swift b/Sources/ConfidenceProvider/Cache/Models/CacheData.swift index 222f087a..e911925c 100644 --- a/Sources/ConfidenceProvider/Cache/Models/CacheData.swift +++ b/Sources/ConfidenceProvider/Cache/Models/CacheData.swift @@ -1,10 +1,8 @@ import Foundation -/** - `CacheData` represents object that encapsulates exposure events for evaluated flags. -It holds information related to apply event i.e. resolve token, flag name, timestamp . - This object is used for tracking exposure events, i.e. by storing them on disk. - */ +/// `CacheData` represents object that encapsulates exposure events for evaluated flags. +/// It holds information related to apply event i.e. resolve token, flag name, timestamp . +/// This object is used for tracking exposure events, i.e. by storing them on disk. struct CacheData: Codable { var resolveEvents: [ResolveApply] @@ -42,7 +40,7 @@ struct CacheData: Codable { return } - resolveEvents[resolveIndex].events[flagIndex].applyEvent.status = status + resolveEvents[resolveIndex].events[flagIndex].status = status } mutating func setEventStatus(resolveToken: String, status: ApplyEventStatus = .sent) { @@ -50,8 +48,8 @@ struct CacheData: Codable { return } - for i in 0...resolveEvents[resolveIndex].events.count-1 { - resolveEvents[resolveIndex].events[i].applyEvent.status = status + for i in 0..