From cbb9b74eaae6c6253cffd722086a707967675358 Mon Sep 17 00:00:00 2001 From: Gaston Thea Date: Thu, 18 Dec 2025 12:29:52 -0300 Subject: [PATCH 1/6] New methods for transactions --- .github/workflows/sonar.yml | 5 +-- Split.xcodeproj/project.pbxproj | 4 ++ Split/Storage/CoreDataHelper.swift | 18 ++++++++ .../Storage/GeneralInfo/GeneralInfoDao.swift | 15 +++++++ Split/Storage/Splits/SplitDao.swift | 35 +++++++++++++++ .../Fake/Storage/CoreDataHelperStub.swift | 43 +++++++++++++++++++ .../Fake/Storage/GeneralInfoDaoStub.swift | 4 ++ 7 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 SplitTests/Fake/Storage/CoreDataHelperStub.swift diff --git a/.github/workflows/sonar.yml b/.github/workflows/sonar.yml index 6bab58320..14c7321e2 100644 --- a/.github/workflows/sonar.yml +++ b/.github/workflows/sonar.yml @@ -4,10 +4,9 @@ on: pull_request: branches: - "**" - - merge: + push: branches: - - "**" + - master # Cancel in-progress runs when a new workflow with the same group is triggered concurrency: diff --git a/Split.xcodeproj/project.pbxproj b/Split.xcodeproj/project.pbxproj index efe4edcdb..ef3bf17f6 100644 --- a/Split.xcodeproj/project.pbxproj +++ b/Split.xcodeproj/project.pbxproj @@ -1096,6 +1096,7 @@ C53207E92EF44A2100418BB1 /* PersistenceBreakerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207E82EF44A2100418BB1 /* PersistenceBreakerTests.swift */; }; C53207EB2EF44A2F00418BB1 /* PersistenceBreaker.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207EA2EF44A2F00418BB1 /* PersistenceBreaker.swift */; }; C53207EC2EF44A2F00418BB1 /* PersistenceBreaker.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207EA2EF44A2F00418BB1 /* PersistenceBreaker.swift */; }; + C53207EE2EF452C000418BB1 /* CoreDataHelperStub.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207ED2EF452C000418BB1 /* CoreDataHelperStub.swift */; }; C539CAB62D88C1F10050C732 /* RuleBasedSegment.swift in Sources */ = {isa = PBXBuildFile; fileRef = C539CAB52D88C1F10050C732 /* RuleBasedSegment.swift */; }; C539CABE2D88C7410050C732 /* PersistentRuleBasedSegmentsStorage.swift in Sources */ = {isa = PBXBuildFile; fileRef = C539CABC2D88C7410050C732 /* PersistentRuleBasedSegmentsStorage.swift */; }; C539CABF2D88C7410050C732 /* RuleBasedSegmentsStorage.swift in Sources */ = {isa = PBXBuildFile; fileRef = C539CABD2D88C7410050C732 /* RuleBasedSegmentsStorage.swift */; }; @@ -2006,6 +2007,7 @@ C52C572A2EEB41450064D049 /* EncryptionKeyValidationTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EncryptionKeyValidationTest.swift; sourceTree = ""; }; C53207E82EF44A2100418BB1 /* PersistenceBreakerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PersistenceBreakerTests.swift; sourceTree = ""; }; C53207EA2EF44A2F00418BB1 /* PersistenceBreaker.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PersistenceBreaker.swift; sourceTree = ""; }; + C53207ED2EF452C000418BB1 /* CoreDataHelperStub.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CoreDataHelperStub.swift; sourceTree = ""; }; C539CAB52D88C1F10050C732 /* RuleBasedSegment.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RuleBasedSegment.swift; sourceTree = ""; }; C539CABC2D88C7410050C732 /* PersistentRuleBasedSegmentsStorage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PersistentRuleBasedSegmentsStorage.swift; sourceTree = ""; }; C539CABD2D88C7410050C732 /* RuleBasedSegmentsStorage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RuleBasedSegmentsStorage.swift; sourceTree = ""; }; @@ -2727,6 +2729,7 @@ 5905D4E6255B23C8006DA3B1 /* Storage */ = { isa = PBXGroup; children = ( + C53207ED2EF452C000418BB1 /* CoreDataHelperStub.swift */, C539CAC22D88C7570050C732 /* PersistentRuleBasedSegmentsStorageStub.swift */, C539CAC32D88C7570050C732 /* RuleBasedSegmentsStorageStub.swift */, C5A501D82D88A7E900206F82 /* RuleBasedSegmentDaoStub.swift */, @@ -4819,6 +4822,7 @@ 5982D938219F57BE00230F44 /* FileHelper.swift in Sources */, 95B180272763DA0E002DC9DF /* HttpTelemetryConfigRecorderTest.swift in Sources */, C5977C202BF29F5B003E293A /* EqualToSemverMatcherTest.swift in Sources */, + C53207EE2EF452C000418BB1 /* CoreDataHelperStub.swift in Sources */, C5A7D5532DD672CF0081D190 /* RuleBasedSegmentsIntegrationTest.swift in Sources */, 59ED408424EAB8C900EF7B09 /* PushNotificationManagerTest.swift in Sources */, 95F7BC292C46A4C800C5F2E4 /* SecurityHelper.swift in Sources */, diff --git a/Split/Storage/CoreDataHelper.swift b/Split/Storage/CoreDataHelper.swift index d996f87e1..713b8e9fc 100644 --- a/Split/Storage/CoreDataHelper.swift +++ b/Split/Storage/CoreDataHelper.swift @@ -64,6 +64,24 @@ class CoreDataHelper { } } + /// Save with error handling. Throws errors to caller + /// Used for transactional operations that need to handle persistence failures + func saveWithErrorHandling() throws { + var thrownError: Error? + managedObjectContext.performAndWait { + do { + if self.managedObjectContext.hasChanges { + try self.managedObjectContext.save() + } + } catch { + thrownError = error + } + } + if let error = thrownError { + throw error + } + } + func generateId() -> String { return UUID().uuidString } diff --git a/Split/Storage/GeneralInfo/GeneralInfoDao.swift b/Split/Storage/GeneralInfo/GeneralInfoDao.swift index 9753311bf..097fcd494 100644 --- a/Split/Storage/GeneralInfo/GeneralInfoDao.swift +++ b/Split/Storage/GeneralInfo/GeneralInfoDao.swift @@ -28,6 +28,10 @@ protocol GeneralInfoDao { func stringValue(info: GeneralInfo) -> String? func longValue(info: GeneralInfo) -> Int64? func delete(info: GeneralInfo) + + /// Synchronous update for use in transactions. + /// Caller must call coreDataHelper.saveWithErrorHandling() after all ops + func transactionalUpdate(info: GeneralInfo, longValue: Int64) } class CoreDataGeneralInfoDao: BaseCoreDataDao, GeneralInfoDao { @@ -86,6 +90,17 @@ class CoreDataGeneralInfoDao: BaseCoreDataDao, GeneralInfoDao { } } + /// Synchronous update that does NOT save. Caller must save. For use in transactions + func transactionalUpdate(info: GeneralInfo, longValue: Int64) { + if let obj = get(for: info) ?? coreDataHelper.create(entity: .generalInfo) as? GeneralInfoEntity { + obj.name = info.rawValue + obj.stringValue = "" + obj.longValue = longValue + obj.updatedAt = Date().unixTimestamp() + // Not saving. Caller will save the entire transaction + } + } + private func update(info: GeneralInfo, stringValue: String?, longValue: Int64?) { if let obj = get(for: info) ?? coreDataHelper.create(entity: .generalInfo) as? GeneralInfoEntity { obj.name = info.rawValue diff --git a/Split/Storage/Splits/SplitDao.swift b/Split/Storage/Splits/SplitDao.swift index cae62e01a..bb2403be1 100644 --- a/Split/Storage/Splits/SplitDao.swift +++ b/Split/Storage/Splits/SplitDao.swift @@ -16,6 +16,14 @@ protocol SplitDao { func delete(_ splits: [String]) func deleteAll() func syncInsertOrUpdate(split: Split) + + /// Synchronous insert/update for use in transactions + /// Caller must call coreDataHelper.saveWithErrorHandling() + func transactionalInsertOrUpdate(splits: [Split]) throws + + /// Synchronous delete for use in transactions + /// Caller must call coreDataHelper.saveWithErrorHandling() + func transactionalDelete(_ splitNames: [String]) } class CoreDataSplitDao: BaseCoreDataDao, SplitDao { @@ -112,6 +120,33 @@ class CoreDataSplitDao: BaseCoreDataDao, SplitDao { } } + /// Synchronous insert/update that does NOT save + func transactionalInsertOrUpdate(splits: [Split]) throws { + let parsed = self.encoder.encode(splits) + for (name, json) in parsed { + if let obj = self.getBy(name: name) ?? self.coreDataHelper.create(entity: .split) as? SplitEntity { + obj.name = name + obj.body = json + obj.updatedAt = Date.now() + // Do NOT save here. Caller should save the entire transaction + } + } + } + + /// Synchronous delete that does NOT save + func transactionalDelete(_ splitNames: [String]) { + if splitNames.count == 0 { + return + } + + var names = splitNames + if let cipher = self.cipher { + names = splitNames.map { cipher.encrypt($0) ?? $0 } + } + self.coreDataHelper.delete(entity: .split, by: "name", values: names) + // Do NOT save here. Caller should save the entire transaction + } + private func insertOrUpdate(_ split: Split) { if let splitName = cipher?.encrypt(split.name) ?? split.name, let obj = self.getBy(name: splitName) ?? self.coreDataHelper.create(entity: .split) as? SplitEntity { diff --git a/SplitTests/Fake/Storage/CoreDataHelperStub.swift b/SplitTests/Fake/Storage/CoreDataHelperStub.swift new file mode 100644 index 000000000..0dd736f53 --- /dev/null +++ b/SplitTests/Fake/Storage/CoreDataHelperStub.swift @@ -0,0 +1,43 @@ +// +// CoreDataHelperStub.swift +// SplitTests +// + +import Foundation +import CoreData +@testable import Split + +class CoreDataHelperStub: CoreDataHelper { + + var shouldFailOnSave = false + var saveError: Error = NSError(domain: "TestCoreData", code: 500, userInfo: [NSLocalizedDescriptionKey: "Simulated save failure"]) + + init() { + let model = NSManagedObjectModel() + let coordinator = NSPersistentStoreCoordinator(managedObjectModel: model) + let context = NSManagedObjectContext(concurrencyType: .privateQueueConcurrencyType) + context.persistentStoreCoordinator = coordinator + + super.init(managedObjectContext: context, persistentCoordinator: coordinator) + } + + override func performAndWait(_ operation: () -> Void) { + operation() + } + + override func perform(_ operation: @escaping () -> Void) { + operation() + } + + override func save() { + // No-op for stubs + } + + override func saveWithErrorHandling() throws { + if shouldFailOnSave { + throw saveError + } + // Success + } +} + diff --git a/SplitTests/Fake/Storage/GeneralInfoDaoStub.swift b/SplitTests/Fake/Storage/GeneralInfoDaoStub.swift index 90db7dcd2..2ba3611ae 100644 --- a/SplitTests/Fake/Storage/GeneralInfoDaoStub.swift +++ b/SplitTests/Fake/Storage/GeneralInfoDaoStub.swift @@ -34,4 +34,8 @@ class GeneralInfoDaoStub: GeneralInfoDao { updatedString.removeValue(forKey: info.rawValue) updatedLong.removeValue(forKey: info.rawValue) } + + func transactionalUpdate(info: GeneralInfo, longValue: Int64) { + updatedLong[info.rawValue] = longValue + } } From 57cce4351b2e67b2501a1cd5884115a28d5b007c Mon Sep 17 00:00:00 2001 From: Gaston Thea Date: Thu, 18 Dec 2025 12:36:47 -0300 Subject: [PATCH 2/6] CoreDataHelper test --- Split.xcodeproj/project.pbxproj | 4 ++ SplitTests/Fake/Storage/SplitDaoStub.swift | 8 +++ .../Fake/Storage/SplitDatabaseStub.swift | 8 ++- SplitTests/Storage/CoreDataHelperTests.swift | 69 +++++++++++++++++++ 4 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 SplitTests/Storage/CoreDataHelperTests.swift diff --git a/Split.xcodeproj/project.pbxproj b/Split.xcodeproj/project.pbxproj index ef3bf17f6..1adce743b 100644 --- a/Split.xcodeproj/project.pbxproj +++ b/Split.xcodeproj/project.pbxproj @@ -1097,6 +1097,7 @@ C53207EB2EF44A2F00418BB1 /* PersistenceBreaker.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207EA2EF44A2F00418BB1 /* PersistenceBreaker.swift */; }; C53207EC2EF44A2F00418BB1 /* PersistenceBreaker.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207EA2EF44A2F00418BB1 /* PersistenceBreaker.swift */; }; C53207EE2EF452C000418BB1 /* CoreDataHelperStub.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207ED2EF452C000418BB1 /* CoreDataHelperStub.swift */; }; + C53207F12EF456AF00418BB1 /* CoreDataHelperTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207F02EF456AF00418BB1 /* CoreDataHelperTests.swift */; }; C539CAB62D88C1F10050C732 /* RuleBasedSegment.swift in Sources */ = {isa = PBXBuildFile; fileRef = C539CAB52D88C1F10050C732 /* RuleBasedSegment.swift */; }; C539CABE2D88C7410050C732 /* PersistentRuleBasedSegmentsStorage.swift in Sources */ = {isa = PBXBuildFile; fileRef = C539CABC2D88C7410050C732 /* PersistentRuleBasedSegmentsStorage.swift */; }; C539CABF2D88C7410050C732 /* RuleBasedSegmentsStorage.swift in Sources */ = {isa = PBXBuildFile; fileRef = C539CABD2D88C7410050C732 /* RuleBasedSegmentsStorage.swift */; }; @@ -2008,6 +2009,7 @@ C53207E82EF44A2100418BB1 /* PersistenceBreakerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PersistenceBreakerTests.swift; sourceTree = ""; }; C53207EA2EF44A2F00418BB1 /* PersistenceBreaker.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PersistenceBreaker.swift; sourceTree = ""; }; C53207ED2EF452C000418BB1 /* CoreDataHelperStub.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CoreDataHelperStub.swift; sourceTree = ""; }; + C53207F02EF456AF00418BB1 /* CoreDataHelperTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CoreDataHelperTests.swift; sourceTree = ""; }; C539CAB52D88C1F10050C732 /* RuleBasedSegment.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RuleBasedSegment.swift; sourceTree = ""; }; C539CABC2D88C7410050C732 /* PersistentRuleBasedSegmentsStorage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PersistentRuleBasedSegmentsStorage.swift; sourceTree = ""; }; C539CABD2D88C7410050C732 /* RuleBasedSegmentsStorage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RuleBasedSegmentsStorage.swift; sourceTree = ""; }; @@ -2685,6 +2687,7 @@ 5905D4E3255B2373006DA3B1 /* Storage */ = { isa = PBXGroup; children = ( + C53207F02EF456AF00418BB1 /* CoreDataHelperTests.swift */, C53207E82EF44A2100418BB1 /* PersistenceBreakerTests.swift */, C52C572A2EEB41450064D049 /* EncryptionKeyValidationTest.swift */, 599407DF22403BE9003B85CC /* SplitsStorageTrafficTypesTests.swift */, @@ -4634,6 +4637,7 @@ 952E26752833FF3F0015D633 /* UniqueKeyDaoStub.swift in Sources */, 59ED408F24F06EC100EF7B09 /* TimersManagerTest.swift in Sources */, 59D84BE7221AE775003DA248 /* LocalhostManagerTests.swift in Sources */, + C53207F12EF456AF00418BB1 /* CoreDataHelperTests.swift in Sources */, C539CAE22D9477770050C732 /* PropertyValidatorStub.swift in Sources */, 955B596C2816BC0C00D105CD /* MultiClientEvaluationTest.swift in Sources */, 59FB7C35220329B900ECC96A /* SplitFactoryBuilderTests.swift in Sources */, diff --git a/SplitTests/Fake/Storage/SplitDaoStub.swift b/SplitTests/Fake/Storage/SplitDaoStub.swift index 7d6bd0aaf..b0c32f131 100644 --- a/SplitTests/Fake/Storage/SplitDaoStub.swift +++ b/SplitTests/Fake/Storage/SplitDaoStub.swift @@ -38,4 +38,12 @@ class SplitDaoStub: SplitDao { func deleteAll() { deleteAllCalled = true } + + func transactionalInsertOrUpdate(splits: [Split]) throws { + insertedSplits = splits + } + + func transactionalDelete(_ splitNames: [String]) { + deletedSplits = splitNames + } } diff --git a/SplitTests/Fake/Storage/SplitDatabaseStub.swift b/SplitTests/Fake/Storage/SplitDatabaseStub.swift index 6f1eab591..193e47f6f 100644 --- a/SplitTests/Fake/Storage/SplitDatabaseStub.swift +++ b/SplitTests/Fake/Storage/SplitDatabaseStub.swift @@ -37,7 +37,7 @@ struct CoreDataDaoProviderMock: DaoProvider { var ruleBasedSegmentDao: RuleBasedSegmentDao = RuleBasedSegmentDaoStub() } -class SplitDatabaseStub: SplitDatabase { +class SplitDatabaseStub: SplitDatabase, TestSplitDatabase { var splitDao: SplitDao var mySegmentsDao: MySegmentsDao @@ -51,7 +51,10 @@ class SplitDatabaseStub: SplitDatabase { var uniqueKeyDao: UniqueKeyDao var ruleBasedSegmentDao: RuleBasedSegmentDao - init(daoProvider: DaoProvider) { + // TestSplitDatabase conformance + var coreDataHelper: CoreDataHelper + + init(daoProvider: DaoProvider, coreDataHelper: CoreDataHelper? = nil) { self.eventDao = daoProvider.eventDao self.impressionDao = daoProvider.impressionDao self.impressionsCountDao = daoProvider.impressionsCountDao @@ -63,5 +66,6 @@ class SplitDatabaseStub: SplitDatabase { self.uniqueKeyDao = daoProvider.uniqueKeyDao self.hashedImpressionDao = daoProvider.hashedImpressionDao self.ruleBasedSegmentDao = daoProvider.ruleBasedSegmentDao + self.coreDataHelper = coreDataHelper ?? CoreDataHelperStub() } } diff --git a/SplitTests/Storage/CoreDataHelperTests.swift b/SplitTests/Storage/CoreDataHelperTests.swift new file mode 100644 index 000000000..ff6ae6c20 --- /dev/null +++ b/SplitTests/Storage/CoreDataHelperTests.swift @@ -0,0 +1,69 @@ +// +// CoreDataHelperTests.swift +// SplitTests +// + +import Foundation +import XCTest +import CoreData +@testable import Split + +class CoreDataHelperTests: XCTestCase { + + var coreDataHelper: CoreDataHelper! + + override func setUp() { + let queue = DispatchQueue(label: "coredata helper test") + coreDataHelper = IntegrationCoreDataHelper.get(databaseName: "test", dispatchQueue: queue) + } + + func testSaveWithErrorHandlingSucceedsWhenChangesExist() { + coreDataHelper.performAndWait { + _ = self.coreDataHelper.create(entity: .generalInfo) + } + + XCTAssertNoThrow(try coreDataHelper.saveWithErrorHandling()) + } + + func testSaveWithErrorHandlingSucceedsWhenNoChanges() { + XCTAssertNoThrow(try coreDataHelper.saveWithErrorHandling()) + } + + func testSaveWithErrorHandlingPersistsData() { + coreDataHelper.performAndWait { + if let entity = self.coreDataHelper.create(entity: .generalInfo) as? GeneralInfoEntity { + entity.name = GeneralInfo.splitsChangeNumber.rawValue + entity.longValue = 12345 + } + } + + try? coreDataHelper.saveWithErrorHandling() + + let fetched = coreDataHelper.fetch(entity: .generalInfo) + XCTAssertEqual(1, fetched.count) + + if let entity = fetched.first as? GeneralInfoEntity { + XCTAssertEqual(12345, entity.longValue) + } else { + XCTFail("Expected GeneralInfoEntity") + } + } + + func testSaveWithErrorHandlingThrowsOnInvalidContext() { + let invalidContext = NSManagedObjectContext(concurrencyType: .privateQueueConcurrencyType) + let invalidHelper = CoreDataHelper( + managedObjectContext: invalidContext, + persistentCoordinator: NSPersistentStoreCoordinator(managedObjectModel: NSManagedObjectModel()) + ) + + invalidContext.performAndWait { + let entity = NSEntityDescription() + entity.name = "TestEntity" + entity.managedObjectClassName = NSStringFromClass(NSManagedObject.self) + } + + // Context without persistent store should not throw when there are no changes + XCTAssertNoThrow(try invalidHelper.saveWithErrorHandling()) + } +} + From 977a3cdc01ed3bbde69fcf23904af29f83e93688 Mon Sep 17 00:00:00 2001 From: Gaston Thea Date: Thu, 18 Dec 2025 12:51:37 -0300 Subject: [PATCH 3/6] Add missing tests --- SplitTests/Storage/CoreDataHelperTests.swift | 17 ++++- SplitTests/Storage/GeneralInfoDaoTests.swift | 41 ++++++++++ SplitTests/Storage/SplitDaoTest.swift | 79 ++++++++++++++++++++ 3 files changed, 136 insertions(+), 1 deletion(-) diff --git a/SplitTests/Storage/CoreDataHelperTests.swift b/SplitTests/Storage/CoreDataHelperTests.swift index ff6ae6c20..c2c6bae8e 100644 --- a/SplitTests/Storage/CoreDataHelperTests.swift +++ b/SplitTests/Storage/CoreDataHelperTests.swift @@ -19,12 +19,27 @@ class CoreDataHelperTests: XCTestCase { func testSaveWithErrorHandlingSucceedsWhenChangesExist() { coreDataHelper.performAndWait { - _ = self.coreDataHelper.create(entity: .generalInfo) + if let entity = self.coreDataHelper.create(entity: .generalInfo) as? GeneralInfoEntity { + entity.name = "test_info" + entity.stringValue = "test_value" + } } XCTAssertNoThrow(try coreDataHelper.saveWithErrorHandling()) } + func testSaveWithErrorHandlingThrowsOnValidationError() { + coreDataHelper.performAndWait { + // Create entity without required 'name' field to trigger validation error + _ = self.coreDataHelper.create(entity: .generalInfo) + } + + XCTAssertThrowsError(try coreDataHelper.saveWithErrorHandling()) { error in + let nsError = error as NSError + XCTAssertEqual(NSValidationMissingMandatoryPropertyError, nsError.code) + } + } + func testSaveWithErrorHandlingSucceedsWhenNoChanges() { XCTAssertNoThrow(try coreDataHelper.saveWithErrorHandling()) } diff --git a/SplitTests/Storage/GeneralInfoDaoTests.swift b/SplitTests/Storage/GeneralInfoDaoTests.swift index 017c8daba..a22f8662e 100644 --- a/SplitTests/Storage/GeneralInfoDaoTests.swift +++ b/SplitTests/Storage/GeneralInfoDaoTests.swift @@ -63,6 +63,47 @@ class GeneralInfoDaoTest: XCTestCase { XCTAssertEqual(data, segmentsInUse) } + + func testTransactionalUpdateDoesNotSaveUntilCallerSaves() { + guard let coreDataDao = generalInfoDao as? CoreDataGeneralInfoDao else { + XCTFail("Expected CoreDataGeneralInfoDao") + return + } + + coreDataDao.coreDataHelper.performAndWait { + coreDataDao.transactionalUpdate(info: .splitsChangeNumber, longValue: 999) + } + + // Value should be in context but we need to save to persist + coreDataDao.coreDataHelper.save() + + let savedValue = generalInfoDao.longValue(info: .splitsChangeNumber) + XCTAssertEqual(999, savedValue) + } + + func testTransactionalUpdateUpdatesExistingValue() { + guard let coreDataDao = generalInfoDao as? CoreDataGeneralInfoDao else { + XCTFail("Expected CoreDataGeneralInfoDao") + return + } + + // Create initial value + generalInfoDao.update(info: .splitsUpdateTimestamp, longValue: 100) + + // Wait for async save to complete + let exp = expectation(description: "wait for save") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { exp.fulfill() } + wait(for: [exp], timeout: 1.0) + + // Transactionally update + coreDataDao.coreDataHelper.performAndWait { + coreDataDao.transactionalUpdate(info: .splitsUpdateTimestamp, longValue: 200) + } + coreDataDao.coreDataHelper.save() + + let updatedValue = generalInfoDao.longValue(info: .splitsUpdateTimestamp) + XCTAssertEqual(200, updatedValue) + } override func tearDown() { } diff --git a/SplitTests/Storage/SplitDaoTest.swift b/SplitTests/Storage/SplitDaoTest.swift index 3ca848235..9926c8e0b 100644 --- a/SplitTests/Storage/SplitDaoTest.swift +++ b/SplitTests/Storage/SplitDaoTest.swift @@ -154,6 +154,85 @@ class SplitDaoTest: XCTestCase { return (name: name, body: body) } + func testTransactionalInsertOrUpdateDoesNotSaveUntilCallerSaves() { + guard let coreDataDao = splitDao as? CoreDataSplitDao else { + XCTFail("Expected CoreDataSplitDao") + return + } + + let newSplits = [newSplit(name: "transactional_split_1", trafficType: "user")] + + coreDataDao.coreDataHelper.performAndWait { + try? coreDataDao.transactionalInsertOrUpdate(splits: newSplits) + } + + // Save manually + coreDataDao.coreDataHelper.save() + + let allSplits = splitDao.getAll() + let found = allSplits.first { $0.name == "transactional_split_1" } + + XCTAssertNotNil(found) + XCTAssertEqual("user", found?.trafficTypeName) + } + + func testTransactionalInsertOrUpdateUpdatesExisting() { + guard let coreDataDao = splitDao as? CoreDataSplitDao else { + XCTFail("Expected CoreDataSplitDao") + return + } + + // feat_0 was created in setUp + let updatedSplit = newSplit(name: "feat_0", trafficType: "updated_type") + + coreDataDao.coreDataHelper.performAndWait { + try? coreDataDao.transactionalInsertOrUpdate(splits: [updatedSplit]) + } + coreDataDao.coreDataHelper.save() + + let allSplits = splitDao.getAll() + let found = allSplits.first { $0.name == "feat_0" } + + XCTAssertNotNil(found) + XCTAssertEqual("updated_type", found?.trafficTypeName) + } + + func testTransactionalDeleteDoesNotSaveUntilCallerSaves() { + guard let coreDataDao = splitDao as? CoreDataSplitDao else { + XCTFail("Expected CoreDataSplitDao") + return + } + + let beforeCount = splitDao.getAll().count + + coreDataDao.coreDataHelper.performAndWait { + coreDataDao.transactionalDelete(["feat_0", "feat_1"]) + } + coreDataDao.coreDataHelper.save() + + let afterCount = splitDao.getAll().count + + XCTAssertEqual(beforeCount - 2, afterCount) + } + + func testTransactionalDeleteWithEmptyArrayDoesNothing() { + guard let coreDataDao = splitDao as? CoreDataSplitDao else { + XCTFail("Expected CoreDataSplitDao") + return + } + + let beforeCount = splitDao.getAll().count + + coreDataDao.coreDataHelper.performAndWait { + coreDataDao.transactionalDelete([]) + } + coreDataDao.coreDataHelper.save() + + let afterCount = splitDao.getAll().count + + XCTAssertEqual(beforeCount, afterCount) + } + private func createSplits() -> [Split] { return SplitTestHelper.createSplits(namePrefix: "feat_", count: 10) } From af9710f81588077189535ba3e566604dca8ec3f9 Mon Sep 17 00:00:00 2001 From: Gaston Thea Date: Thu, 18 Dec 2025 13:25:51 -0300 Subject: [PATCH 4/6] Integrate transaction & persistence breaker in SDK --- Split.xcodeproj/project.pbxproj | 8 + Split/Api/SplitDatabaseHelper.swift | 17 +- Split/Network/Sync/SyncCommons.swift | 1 + .../Splits/PersistentSplitsStorage.swift | 34 +++- Split/Storage/Splits/SplitDao.swift | 4 +- Split/Storage/Splits/SplitsStorage.swift | 39 ++++- .../Storage/PersistentSplitsStorageStub.swift | 2 +- SplitTests/Fake/Storage/SplitDaoStub.swift | 2 +- SplitTests/Helpers/TestingHelper.swift | 3 +- .../PersistentSplitsStorageTests.swift | 2 +- ...stentSplitsStorageTransactionalTests.swift | 106 +++++++++++ SplitTests/Storage/SplitDaoTest.swift | 4 +- ...itsPersistenceBreakerIntegrationTest.swift | 164 ++++++++++++++++++ SplitTests/Storage/SplitsStorageTests.swift | 8 +- .../SplitsStorageTrafficTypesTests.swift | 3 +- .../FeatureFlagsSynchronizerTest.swift | 3 +- .../Streaming/ImpressionsTrackerTest.swift | 3 +- SplitTests/Streaming/SynchronizerTest.swift | 3 +- SplitTests/TreatmentManagerTest.swift | 3 +- 19 files changed, 373 insertions(+), 36 deletions(-) create mode 100644 SplitTests/Storage/PersistentSplitsStorageTransactionalTests.swift create mode 100644 SplitTests/Storage/SplitsPersistenceBreakerIntegrationTest.swift diff --git a/Split.xcodeproj/project.pbxproj b/Split.xcodeproj/project.pbxproj index 1adce743b..6aab8e1a9 100644 --- a/Split.xcodeproj/project.pbxproj +++ b/Split.xcodeproj/project.pbxproj @@ -1098,6 +1098,8 @@ C53207EC2EF44A2F00418BB1 /* PersistenceBreaker.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207EA2EF44A2F00418BB1 /* PersistenceBreaker.swift */; }; C53207EE2EF452C000418BB1 /* CoreDataHelperStub.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207ED2EF452C000418BB1 /* CoreDataHelperStub.swift */; }; C53207F12EF456AF00418BB1 /* CoreDataHelperTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207F02EF456AF00418BB1 /* CoreDataHelperTests.swift */; }; + C53207F42EF45BC600418BB1 /* PersistentSplitsStorageTransactionalTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207F22EF45BC600418BB1 /* PersistentSplitsStorageTransactionalTests.swift */; }; + C53207F52EF45BC600418BB1 /* SplitsPersistenceBreakerIntegrationTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = C53207F32EF45BC600418BB1 /* SplitsPersistenceBreakerIntegrationTest.swift */; }; C539CAB62D88C1F10050C732 /* RuleBasedSegment.swift in Sources */ = {isa = PBXBuildFile; fileRef = C539CAB52D88C1F10050C732 /* RuleBasedSegment.swift */; }; C539CABE2D88C7410050C732 /* PersistentRuleBasedSegmentsStorage.swift in Sources */ = {isa = PBXBuildFile; fileRef = C539CABC2D88C7410050C732 /* PersistentRuleBasedSegmentsStorage.swift */; }; C539CABF2D88C7410050C732 /* RuleBasedSegmentsStorage.swift in Sources */ = {isa = PBXBuildFile; fileRef = C539CABD2D88C7410050C732 /* RuleBasedSegmentsStorage.swift */; }; @@ -2010,6 +2012,8 @@ C53207EA2EF44A2F00418BB1 /* PersistenceBreaker.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PersistenceBreaker.swift; sourceTree = ""; }; C53207ED2EF452C000418BB1 /* CoreDataHelperStub.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CoreDataHelperStub.swift; sourceTree = ""; }; C53207F02EF456AF00418BB1 /* CoreDataHelperTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CoreDataHelperTests.swift; sourceTree = ""; }; + C53207F22EF45BC600418BB1 /* PersistentSplitsStorageTransactionalTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PersistentSplitsStorageTransactionalTests.swift; sourceTree = ""; }; + C53207F32EF45BC600418BB1 /* SplitsPersistenceBreakerIntegrationTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SplitsPersistenceBreakerIntegrationTest.swift; sourceTree = ""; }; C539CAB52D88C1F10050C732 /* RuleBasedSegment.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RuleBasedSegment.swift; sourceTree = ""; }; C539CABC2D88C7410050C732 /* PersistentRuleBasedSegmentsStorage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PersistentRuleBasedSegmentsStorage.swift; sourceTree = ""; }; C539CABD2D88C7410050C732 /* RuleBasedSegmentsStorage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RuleBasedSegmentsStorage.swift; sourceTree = ""; }; @@ -2687,6 +2691,8 @@ 5905D4E3255B2373006DA3B1 /* Storage */ = { isa = PBXGroup; children = ( + C53207F22EF45BC600418BB1 /* PersistentSplitsStorageTransactionalTests.swift */, + C53207F32EF45BC600418BB1 /* SplitsPersistenceBreakerIntegrationTest.swift */, C53207F02EF456AF00418BB1 /* CoreDataHelperTests.swift */, C53207E82EF44A2100418BB1 /* PersistenceBreakerTests.swift */, C52C572A2EEB41450064D049 /* EncryptionKeyValidationTest.swift */, @@ -4776,6 +4782,8 @@ 592C6AC4211B718E002D120C /* RegexTest.swift in Sources */, 9500D9922C56F9BA00383593 /* HostDomainFilterTests.swift in Sources */, 95F3F0282590D81B00084AF8 /* ImpressionsRecorderWorkerTests.swift in Sources */, + C53207F42EF45BC600418BB1 /* PersistentSplitsStorageTransactionalTests.swift in Sources */, + C53207F52EF45BC600418BB1 /* SplitsPersistenceBreakerIntegrationTest.swift in Sources */, 955E12312BFBEA8600AE6D10 /* HashedImpressionDaoTest.swift in Sources */, 95342E992A4C71300045B201 /* FeatureFlagsPayloadDecoderMock.swift in Sources */, 9577A8472683B8C800D92AE1 /* HttpImpressionsCountRecorderTests.swift in Sources */, diff --git a/Split/Api/SplitDatabaseHelper.swift b/Split/Api/SplitDatabaseHelper.swift index 48f4b5dd8..ff7eb7452 100644 --- a/Split/Api/SplitDatabaseHelper.swift +++ b/Split/Api/SplitDatabaseHelper.swift @@ -91,7 +91,13 @@ struct SplitDatabaseHelper { DefaultFlagSetsCache(setsInFilter: splitClientConfig.bySetsFilter()?.values.asSet()) let persistentSplitsStorage = DefaultPersistentSplitsStorage(database: splitDatabase) let generalInfoStorage = openGeneralInfoStorage(database: splitDatabase) - let splitsStorage = openSplitsStorage(database: splitDatabase, flagSetsCache: flagSetsCache, generalInfoStorage: generalInfoStorage) + + // Create shared persistence breaker for targeting rules (splits + RBS) + let targetingRulesPersistenceBreaker = DefaultPersistenceBreaker() + let splitsStorage = openSplitsStorage(database: splitDatabase, + flagSetsCache: flagSetsCache, + generalInfoStorage: generalInfoStorage, + persistenceBreaker: targetingRulesPersistenceBreaker) let persistentImpressionsStorage = openPersistentImpressionsStorage(database: splitDatabase) let impressionsStorage = openImpressionsStorage(persistentStorage: persistentImpressionsStorage) @@ -139,7 +145,8 @@ struct SplitDatabaseHelper { hashedImpressionsStorage: hashedImpressionsStorage, generalInfoStorage: generalInfoStorage, ruleBasedSegmentsStorage: ruleBasedSegmentsStorage, - persistentRuleBasedSegmentsStorage: persistentRuleBasedSegmentsStorage) + persistentRuleBasedSegmentsStorage: persistentRuleBasedSegmentsStorage, + targetingRulesPersistenceBreaker: targetingRulesPersistenceBreaker) } static func openDatabase(dataFolderName: String, @@ -158,9 +165,11 @@ struct SplitDatabaseHelper { } static func openSplitsStorage(database: SplitDatabase, - flagSetsCache: FlagSetsCache, generalInfoStorage: GeneralInfoStorage) -> SplitsStorage { + flagSetsCache: FlagSetsCache, generalInfoStorage: GeneralInfoStorage, + persistenceBreaker: PersistenceBreaker) -> SplitsStorage { return DefaultSplitsStorage(persistentSplitsStorage: openPersistentSplitsStorage(database: database), - flagSetsCache: flagSetsCache, GeneralInfoStorage: generalInfoStorage) + flagSetsCache: flagSetsCache, generalInfoStorage: generalInfoStorage, + persistenceBreaker: persistenceBreaker) } static func openPersistentMySegmentsStorage(database: SplitDatabase) -> PersistentMySegmentsStorage { diff --git a/Split/Network/Sync/SyncCommons.swift b/Split/Network/Sync/SyncCommons.swift index fb3313ae9..e8df6c3a0 100644 --- a/Split/Network/Sync/SyncCommons.swift +++ b/Split/Network/Sync/SyncCommons.swift @@ -26,6 +26,7 @@ struct SplitStorageContainer { let generalInfoStorage: GeneralInfoStorage let ruleBasedSegmentsStorage: RuleBasedSegmentsStorage let persistentRuleBasedSegmentsStorage: PersistentRuleBasedSegmentsStorage + let targetingRulesPersistenceBreaker: PersistenceBreaker } protocol ImpressionLogger { diff --git a/Split/Storage/Splits/PersistentSplitsStorage.swift b/Split/Storage/Splits/PersistentSplitsStorage.swift index b082b5825..88a122f12 100644 --- a/Split/Storage/Splits/PersistentSplitsStorage.swift +++ b/Split/Storage/Splits/PersistentSplitsStorage.swift @@ -9,7 +9,7 @@ import Foundation protocol PersistentSplitsStorage { - func update(splitChange: ProcessedSplitChange) + func update(splitChange: ProcessedSplitChange, onFailure: ((Error) -> Void)?) func update(split: Split) func update(bySetsFilter: SplitFilter?) func getBySetsFilter() -> SplitFilter? @@ -25,17 +25,39 @@ class DefaultPersistentSplitsStorage: PersistentSplitsStorage { private let splitDao: SplitDao private let generalInfoDao: GeneralInfoDao + private let coreDataHelper: CoreDataHelper init(database: SplitDatabase) { self.splitDao = database.splitDao self.generalInfoDao = database.generalInfoDao + if let testDb = database as? TestSplitDatabase { + self.coreDataHelper = testDb.coreDataHelper + } else { + fatalError("Database must provide CoreDataHelper for transactional operations") + } } - func update(splitChange: ProcessedSplitChange) { - splitDao.insertOrUpdate(splits: splitChange.activeSplits) - splitDao.delete(splitChange.archivedSplits.compactMap { return $0.name }) - generalInfoDao.update(info: .splitsChangeNumber, longValue: splitChange.changeNumber) - generalInfoDao.update(info: .splitsUpdateTimestamp, longValue: splitChange.updateTimestamp) + func update(splitChange: ProcessedSplitChange, onFailure: ((Error) -> Void)? = nil) { + // Execute transactionally: all operations must succeed or all must fail + coreDataHelper.performAndWait { [weak self] in + guard let self = self else { return } + + do { + // All operations within this block happen in the same CoreData context + self.splitDao.transactionalInsertOrUpdate(splits: splitChange.activeSplits) + let archivedNames = splitChange.archivedSplits.compactMap { $0.name } + self.splitDao.transactionalDelete(archivedNames) + + self.generalInfoDao.transactionalUpdate(info: .splitsChangeNumber, longValue: splitChange.changeNumber) + self.generalInfoDao.transactionalUpdate(info: .splitsUpdateTimestamp, longValue: splitChange.updateTimestamp) + + // Save everything as one transaction + try self.coreDataHelper.saveWithErrorHandling() + } catch { + Logger.e("Transactional flags update failed: \(error.localizedDescription)") + onFailure?(error) + } + } } func update(split: Split) { diff --git a/Split/Storage/Splits/SplitDao.swift b/Split/Storage/Splits/SplitDao.swift index bb2403be1..137f9b2af 100644 --- a/Split/Storage/Splits/SplitDao.swift +++ b/Split/Storage/Splits/SplitDao.swift @@ -19,7 +19,7 @@ protocol SplitDao { /// Synchronous insert/update for use in transactions /// Caller must call coreDataHelper.saveWithErrorHandling() - func transactionalInsertOrUpdate(splits: [Split]) throws + func transactionalInsertOrUpdate(splits: [Split]) /// Synchronous delete for use in transactions /// Caller must call coreDataHelper.saveWithErrorHandling() @@ -121,7 +121,7 @@ class CoreDataSplitDao: BaseCoreDataDao, SplitDao { } /// Synchronous insert/update that does NOT save - func transactionalInsertOrUpdate(splits: [Split]) throws { + func transactionalInsertOrUpdate(splits: [Split]) { let parsed = self.encoder.encode(splits) for (name, json) in parsed { if let obj = self.getBy(name: name) ?? self.coreDataHelper.create(entity: .split) as? SplitEntity { diff --git a/Split/Storage/Splits/SplitsStorage.swift b/Split/Storage/Splits/SplitsStorage.swift index 732edbec4..e822a0901 100644 --- a/Split/Storage/Splits/SplitsStorage.swift +++ b/Split/Storage/Splits/SplitsStorage.swift @@ -36,18 +36,21 @@ class DefaultSplitsStorage: SplitsStorage { private var trafficTypes: SynchronizedDictionary private let flagSetsCache: FlagSetsCache private let generalInfoStorage: GeneralInfoStorage + private let persistenceBreaker: PersistenceBreaker private(set) var changeNumber: Int64 = -1 private(set) var updateTimestamp: Int64 = -1 init(persistentSplitsStorage: PersistentSplitsStorage, flagSetsCache: FlagSetsCache, - GeneralInfoStorage: GeneralInfoStorage) { + generalInfoStorage: GeneralInfoStorage, + persistenceBreaker: PersistenceBreaker) { self.persistentStorage = persistentSplitsStorage self.inMemorySplits = SynchronizedDictionary() self.trafficTypes = SynchronizedDictionary() self.flagSetsCache = flagSetsCache - self.generalInfoStorage = GeneralInfoStorage + self.generalInfoStorage = generalInfoStorage + self.persistenceBreaker = persistenceBreaker } func loadLocal() { @@ -86,26 +89,39 @@ class DefaultSplitsStorage: SplitsStorage { func update(splitChange: ProcessedSplitChange) -> Bool { - // Process + // Process in-memory updates (always happens) let updated = processUpdated(splits: splitChange.activeSplits, active: true) let removed = processUpdated(splits: splitChange.archivedSplits, active: false) - // Update + // Update in-memory metadata (always happens) changeNumber = splitChange.changeNumber updateTimestamp = splitChange.updateTimestamp - persistentStorage.update(splitChange: splitChange) + + // Attempt persistence only if breaker allows + if persistenceBreaker.isPersistenceEnabled { + persistentStorage.update(splitChange: splitChange, onFailure: { [weak self] _ in + // On first failure, disable persistence for remainder of session + self?.persistenceBreaker.disable() + }) + } return updated || removed } func update(bySetsFilter filter: SplitFilter?) { - self.persistentStorage.update(bySetsFilter: filter) + // Only call persistence if breaker allows + if persistenceBreaker.isPersistenceEnabled { + self.persistentStorage.update(bySetsFilter: filter) + } } func updateWithoutChecks(split: Split) { if let splitName = split.name?.lowercased() { inMemorySplits.setValue(split, forKey: splitName) - persistentStorage.update(split: split) + // Only call persistence if breaker allows + if persistenceBreaker.isPersistenceEnabled { + persistentStorage.update(split: split) + } } } @@ -116,7 +132,10 @@ class DefaultSplitsStorage: SplitsStorage { func clear() { inMemorySplits.removeAll() changeNumber = -1 - persistentStorage.clear() + // Only call persistence if breaker allows + if persistenceBreaker.isPersistenceEnabled { + persistentStorage.clear() + } } func getCount() -> Int { @@ -296,7 +315,9 @@ class BackgroundSyncSplitsStorage: SyncSplitsStorage { } func update(splitChange: ProcessedSplitChange) -> Bool { - persistentStorage.update(splitChange: splitChange) + // Background sync doesn't use the breaker - it just persists directly + // If persistence fails, it will be logged but won't trigger breaker + persistentStorage.update(splitChange: splitChange, onFailure: nil) return true } diff --git a/SplitTests/Fake/Storage/PersistentSplitsStorageStub.swift b/SplitTests/Fake/Storage/PersistentSplitsStorageStub.swift index a26e1bf6b..5c4334a78 100644 --- a/SplitTests/Fake/Storage/PersistentSplitsStorageStub.swift +++ b/SplitTests/Fake/Storage/PersistentSplitsStorageStub.swift @@ -43,7 +43,7 @@ class PersistentSplitsStorageStub: PersistentSplitsStorage { self.init(delegate: nil) } - func update(splitChange: ProcessedSplitChange) { + func update(splitChange: ProcessedSplitChange, onFailure: ((Error) -> Void)? = nil) { processedSplitChange = splitChange changeNumber = splitChange.changeNumber updateCalled = true diff --git a/SplitTests/Fake/Storage/SplitDaoStub.swift b/SplitTests/Fake/Storage/SplitDaoStub.swift index b0c32f131..0078f3fb9 100644 --- a/SplitTests/Fake/Storage/SplitDaoStub.swift +++ b/SplitTests/Fake/Storage/SplitDaoStub.swift @@ -39,7 +39,7 @@ class SplitDaoStub: SplitDao { deleteAllCalled = true } - func transactionalInsertOrUpdate(splits: [Split]) throws { + func transactionalInsertOrUpdate(splits: [Split]) { insertedSplits = splits } diff --git a/SplitTests/Helpers/TestingHelper.swift b/SplitTests/Helpers/TestingHelper.swift index c908d1cd7..e377ccfc4 100644 --- a/SplitTests/Helpers/TestingHelper.swift +++ b/SplitTests/Helpers/TestingHelper.swift @@ -198,7 +198,8 @@ struct TestingHelper { hashedImpressionsStorage: HashedImpressionsStorageMock(), generalInfoStorage: GeneralInfoStorageMock(), ruleBasedSegmentsStorage: RuleBasedSegmentsStorageStub(), - persistentRuleBasedSegmentsStorage: PersistentRuleBasedSegmentsStorageStub()) + persistentRuleBasedSegmentsStorage: PersistentRuleBasedSegmentsStorageStub(), + targetingRulesPersistenceBreaker: DefaultPersistenceBreaker()) } static func createApiFacade() -> SplitApiFacade { diff --git a/SplitTests/Storage/PersistentSplitsStorageTests.swift b/SplitTests/Storage/PersistentSplitsStorageTests.swift index 48c17dbda..cc5eccb98 100644 --- a/SplitTests/Storage/PersistentSplitsStorageTests.swift +++ b/SplitTests/Storage/PersistentSplitsStorageTests.swift @@ -31,7 +31,7 @@ class PersistentSplitsStorageTest: XCTestCase { let archivedSplits = [newSplit(name: "ar1", trafficType: "t2", status: .archived), newSplit(name: "ar2", trafficType: "t2", status: .archived)] let change = ProcessedSplitChange(activeSplits: activeSplits, archivedSplits: archivedSplits, changeNumber: 100, updateTimestamp: 200) - splitsStorage.update(splitChange: change) + splitsStorage.update(splitChange: change, onFailure: nil) XCTAssertEqual(3, splitDao.insertedSplits.count) XCTAssertEqual(2, splitDao.deletedSplits?.count) diff --git a/SplitTests/Storage/PersistentSplitsStorageTransactionalTests.swift b/SplitTests/Storage/PersistentSplitsStorageTransactionalTests.swift new file mode 100644 index 000000000..76856bb7f --- /dev/null +++ b/SplitTests/Storage/PersistentSplitsStorageTransactionalTests.swift @@ -0,0 +1,106 @@ +// +// PersistentSplitsStorageTransactionalTests.swift +// SplitTests +// + +import Foundation +import XCTest +@testable import Split + +class PersistentSplitsStorageTransactionalTests: XCTestCase { + + var splitsStorage: PersistentSplitsStorage! + var splitDao: SplitDaoStub! + var generalInfoDao: GeneralInfoDaoStub! + var coreDataHelperStub: CoreDataHelperStub! + + override func setUp() { + splitDao = SplitDaoStub() + generalInfoDao = GeneralInfoDaoStub() + coreDataHelperStub = CoreDataHelperStub() + var daoProvider = CoreDataDaoProviderMock() + daoProvider.splitDao = splitDao + daoProvider.generalInfoDao = generalInfoDao + splitsStorage = DefaultPersistentSplitsStorage( + database: SplitDatabaseStub(daoProvider: daoProvider, coreDataHelper: coreDataHelperStub) + ) + } + + func testSuccessDoesNotInvokeFailureCallback() { + let change = ProcessedSplitChange( + activeSplits: [createSplit(name: "split1")], + archivedSplits: [], + changeNumber: 100, + updateTimestamp: 1000 + ) + + var failureWasReported = false + + splitsStorage.update(splitChange: change, onFailure: { _ in + failureWasReported = true + }) + + XCTAssertFalse(failureWasReported, "With working stubs, no failure should occur") + } + + func testFailureCallbackIsInvokedOnSaveError() { + coreDataHelperStub.shouldFailOnSave = true + + let change = ProcessedSplitChange( + activeSplits: [createSplit(name: "split1")], + archivedSplits: [], + changeNumber: 100, + updateTimestamp: 1000 + ) + + var failureWasReported = false + var reportedError: Error? + + splitsStorage.update(splitChange: change, onFailure: { error in + failureWasReported = true + reportedError = error + }) + + XCTAssertTrue(failureWasReported, "Failure callback should be invoked on save error") + XCTAssertNotNil(reportedError, "Error should be reported") + } + + func testNilFailureCallbackIsHandled() { + let change = ProcessedSplitChange( + activeSplits: [createSplit(name: "split1")], + archivedSplits: [], + changeNumber: 100, + updateTimestamp: 1000 + ) + + splitsStorage.update(splitChange: change, onFailure: nil) + + XCTAssertEqual(1, splitDao.insertedSplits.count) + XCTAssertEqual(100, generalInfoDao.longValue(info: .splitsChangeNumber)) + } + + func testSplitsAndGeneralInfoAreUpdatedTogether() { + let activeSplits = [createSplit(name: "s1"), createSplit(name: "s2")] + let archivedSplits = [createSplit(name: "s3")] + let change = ProcessedSplitChange( + activeSplits: activeSplits, + archivedSplits: archivedSplits, + changeNumber: 200, + updateTimestamp: 2000 + ) + + splitsStorage.update(splitChange: change, onFailure: nil) + + XCTAssertEqual(2, splitDao.insertedSplits.count, "Active splits should be inserted") + XCTAssertEqual(1, splitDao.deletedSplits?.count, "Archived splits should be deleted") + XCTAssertEqual(200, generalInfoDao.longValue(info: .splitsChangeNumber), "ChangeNumber should be updated") + XCTAssertEqual(2000, generalInfoDao.longValue(info: .splitsUpdateTimestamp), "UpdateTimestamp should be updated") + } + + private func createSplit(name: String, trafficType: String = "user") -> Split { + let split = SplitTestHelper.newSplit(name: name, trafficType: trafficType) + split.status = .active + return split + } +} + diff --git a/SplitTests/Storage/SplitDaoTest.swift b/SplitTests/Storage/SplitDaoTest.swift index 9926c8e0b..9ae5a324d 100644 --- a/SplitTests/Storage/SplitDaoTest.swift +++ b/SplitTests/Storage/SplitDaoTest.swift @@ -163,7 +163,7 @@ class SplitDaoTest: XCTestCase { let newSplits = [newSplit(name: "transactional_split_1", trafficType: "user")] coreDataDao.coreDataHelper.performAndWait { - try? coreDataDao.transactionalInsertOrUpdate(splits: newSplits) + coreDataDao.transactionalInsertOrUpdate(splits: newSplits) } // Save manually @@ -186,7 +186,7 @@ class SplitDaoTest: XCTestCase { let updatedSplit = newSplit(name: "feat_0", trafficType: "updated_type") coreDataDao.coreDataHelper.performAndWait { - try? coreDataDao.transactionalInsertOrUpdate(splits: [updatedSplit]) + coreDataDao.transactionalInsertOrUpdate(splits: [updatedSplit]) } coreDataDao.coreDataHelper.save() diff --git a/SplitTests/Storage/SplitsPersistenceBreakerIntegrationTest.swift b/SplitTests/Storage/SplitsPersistenceBreakerIntegrationTest.swift new file mode 100644 index 000000000..c509f9f50 --- /dev/null +++ b/SplitTests/Storage/SplitsPersistenceBreakerIntegrationTest.swift @@ -0,0 +1,164 @@ +// +// SplitsPersistenceBreakerIntegrationTest.swift +// SplitTests +// + +import Foundation +import XCTest +@testable import Split + +class SplitsPersistenceBreakerIntegrationTest: XCTestCase { + + var splitsStorage: DefaultSplitsStorage! + var persistentStorage: FailingPersistentSplitsStorage! + var flagSetsCache: FlagSetsCacheMock! + var generalInfoStorage: GeneralInfoStorageMock! + var persistenceBreaker: DefaultPersistenceBreaker! + + override func setUp() { + super.setUp() + persistentStorage = FailingPersistentSplitsStorage() + flagSetsCache = FlagSetsCacheMock() + generalInfoStorage = GeneralInfoStorageMock() + persistenceBreaker = DefaultPersistenceBreaker() + + splitsStorage = DefaultSplitsStorage( + persistentSplitsStorage: persistentStorage, + flagSetsCache: flagSetsCache, + generalInfoStorage: generalInfoStorage, + persistenceBreaker: persistenceBreaker + ) + } + + func testFirstPersistenceFailureDisablesFurtherPersistence() { + persistentStorage.shouldFail = true + persistentStorage.failOnCallNumber = 1 + + let change1 = ProcessedSplitChange( + activeSplits: [createSplit(name: "split1")], + archivedSplits: [], + changeNumber: 100, + updateTimestamp: 1000 + ) + + let change2 = ProcessedSplitChange( + activeSplits: [createSplit(name: "split2")], + archivedSplits: [], + changeNumber: 200, + updateTimestamp: 2000 + ) + + _ = splitsStorage.update(splitChange: change1) + + XCTAssertEqual(1, persistentStorage.updateCallCount, "First update should attempt persistence") + + _ = splitsStorage.update(splitChange: change2) + + XCTAssertEqual(1, persistentStorage.updateCallCount, + "After first failure, no further persistence calls should occur") + } + + func testInMemorySplitsStillWorkAfterPersistenceDisabled() { + persistentStorage.shouldFail = true + persistentStorage.failOnCallNumber = 1 + + let change1 = ProcessedSplitChange( + activeSplits: [createSplit(name: "split1")], + archivedSplits: [], + changeNumber: 100, + updateTimestamp: 1000 + ) + + let change2 = ProcessedSplitChange( + activeSplits: [createSplit(name: "split2")], + archivedSplits: [], + changeNumber: 200, + updateTimestamp: 2000 + ) + + _ = splitsStorage.update(splitChange: change1) + _ = splitsStorage.update(splitChange: change2) + + XCTAssertNotNil(splitsStorage.get(name: "split1"), + "First split should be in memory despite persistence failure") + XCTAssertNotNil(splitsStorage.get(name: "split2"), + "Second split should be in memory (persistence skipped)") + + XCTAssertEqual(200, splitsStorage.changeNumber, + "In-memory change number should advance even when persistence disabled") + XCTAssertEqual(2000, splitsStorage.updateTimestamp, + "In-memory timestamp should advance even when persistence disabled") + } + + private func createSplit(name: String, trafficType: String = "user") -> Split { + let split = SplitTestHelper.newSplit(name: name, trafficType: trafficType) + split.status = .active + return split + } +} + +class FailingPersistentSplitsStorage: PersistentSplitsStorage { + + var shouldFail = false + var failOnCallNumber: Int = 1 + var updateCallCount = 0 + var failureReported = false + + private var snapshot = SplitsSnapshot(changeNumber: -1, splits: [], updateTimestamp: -1) + + func update(splitChange: ProcessedSplitChange, onFailure: ((Error) -> Void)? = nil) { + updateCallCount += 1 + + if shouldFail && updateCallCount == failOnCallNumber { + // Simulate a CoreData save() failure + let error = NSError(domain: "TestCoreData", code: 1001, userInfo: [NSLocalizedDescriptionKey: "Simulated CoreData save failure"]) + failureReported = true + onFailure?(error) + return + } + + // Normal success path (not failing) + snapshot = SplitsSnapshot( + changeNumber: splitChange.changeNumber, + splits: splitChange.activeSplits, + updateTimestamp: splitChange.updateTimestamp + ) + } + + func update(split: Split) { + // No-op for this test + } + + func update(bySetsFilter: SplitFilter?) { + // No-op for this test + } + + func getBySetsFilter() -> SplitFilter? { + return nil + } + + func getSplitsSnapshot() -> SplitsSnapshot { + return snapshot + } + + func getChangeNumber() -> Int64 { + return snapshot.changeNumber + } + + func getUpdateTimestamp() -> Int64 { + return snapshot.updateTimestamp + } + + func getAll() -> [Split] { + return snapshot.splits + } + + func delete(splitNames: [String]) { + // No-op for this test + } + + func clear() { + snapshot = SplitsSnapshot(changeNumber: -1, splits: [], updateTimestamp: -1) + } +} + diff --git a/SplitTests/Storage/SplitsStorageTests.swift b/SplitTests/Storage/SplitsStorageTests.swift index 508e31396..335a970d3 100644 --- a/SplitTests/Storage/SplitsStorageTests.swift +++ b/SplitTests/Storage/SplitsStorageTests.swift @@ -25,7 +25,7 @@ class SplitsStorageTest: XCTestCase { override func setUp() { persistentStorage = PersistentSplitsStorageStub() flagSetsCache = FlagSetsCacheMock() - splitsStorage = DefaultSplitsStorage(persistentSplitsStorage: persistentStorage, flagSetsCache: flagSetsCache, GeneralInfoStorage: generalInfoStorage) + splitsStorage = DefaultSplitsStorage(persistentSplitsStorage: persistentStorage, flagSetsCache: flagSetsCache, generalInfoStorage: generalInfoStorage, persistenceBreaker: DefaultPersistenceBreaker()) } func testNoLocalLoaded() { @@ -40,7 +40,7 @@ class SplitsStorageTest: XCTestCase { } func testLazyParsing() { - noLoadedStorage = DefaultSplitsStorage(persistentSplitsStorage: createPersistentStorageStub(isParsed: false), flagSetsCache: FlagSetsCacheMock(), GeneralInfoStorage: generalInfoStorage) + noLoadedStorage = DefaultSplitsStorage(persistentSplitsStorage: createPersistentStorageStub(isParsed: false), flagSetsCache: FlagSetsCacheMock(), generalInfoStorage: generalInfoStorage, persistenceBreaker: DefaultPersistenceBreaker()) noLoadedStorage?.loadLocal() @@ -278,7 +278,7 @@ class SplitsStorageTest: XCTestCase { let flagSetsCache = FlagSetsCacheMock() flagSetsCache.setsInFilter = ["set1", "set2", "set3"] - splitsStorage = DefaultSplitsStorage(persistentSplitsStorage: persistentStorage, flagSetsCache: flagSetsCache, GeneralInfoStorage: GeneralInfoStorageMock()) + splitsStorage = DefaultSplitsStorage(persistentSplitsStorage: persistentStorage, flagSetsCache: flagSetsCache, generalInfoStorage: GeneralInfoStorageMock(), persistenceBreaker: DefaultPersistenceBreaker()) persistentStorage.snapshot = getTestSnapshot(count: 3, sets: [ ["set1", "set2"], ["set1"], @@ -472,7 +472,7 @@ private class MockPersistentSplitsSegmentsStorage: PersistentSplitsStorage { self.segmentsInUse = segmentsInUse } - func update(splitChange: ProcessedSplitChange) { + func update(splitChange: ProcessedSplitChange, onFailure: ((Error) -> Void)?) { // No-op for the mock } diff --git a/SplitTests/Storage/SplitsStorageTrafficTypesTests.swift b/SplitTests/Storage/SplitsStorageTrafficTypesTests.swift index 39228a116..bc316e242 100644 --- a/SplitTests/Storage/SplitsStorageTrafficTypesTests.swift +++ b/SplitTests/Storage/SplitsStorageTrafficTypesTests.swift @@ -27,7 +27,8 @@ class SplitsStorageTrafficTypesTests: XCTestCase { flagSetsCache = FlagSetsCacheMock() persistent.snapshot = SplitsSnapshot(changeNumber: 1, splits: splits, updateTimestamp: 100) - splitsStorage = DefaultSplitsStorage(persistentSplitsStorage: persistent, flagSetsCache: flagSetsCache, GeneralInfoStorage: generalInfoStorage) + splitsStorage = DefaultSplitsStorage(persistentSplitsStorage: persistent, flagSetsCache: flagSetsCache, generalInfoStorage: generalInfoStorage, + persistenceBreaker: DefaultPersistenceBreaker()) splitsStorage.loadLocal() } diff --git a/SplitTests/Streaming/FeatureFlagsSynchronizerTest.swift b/SplitTests/Streaming/FeatureFlagsSynchronizerTest.swift index ab794172c..e06d5b629 100644 --- a/SplitTests/Streaming/FeatureFlagsSynchronizerTest.swift +++ b/SplitTests/Streaming/FeatureFlagsSynchronizerTest.swift @@ -79,7 +79,8 @@ class FeatureFlagsSynchronizerTest: XCTestCase { hashedImpressionsStorage: HashedImpressionsStorageMock(), generalInfoStorage: self.generalInfoStorage!, ruleBasedSegmentsStorage: ruleBasedSegmentsStorage, - persistentRuleBasedSegmentsStorage: PersistentRuleBasedSegmentsStorageStub()) + persistentRuleBasedSegmentsStorage: PersistentRuleBasedSegmentsStorageStub(), + targetingRulesPersistenceBreaker: DefaultPersistenceBreaker()) splitConfig = SplitClientConfig() splitConfig.syncEnabled = syncEnabled diff --git a/SplitTests/Streaming/ImpressionsTrackerTest.swift b/SplitTests/Streaming/ImpressionsTrackerTest.swift index f4010affc..a9e543387 100644 --- a/SplitTests/Streaming/ImpressionsTrackerTest.swift +++ b/SplitTests/Streaming/ImpressionsTrackerTest.swift @@ -361,7 +361,8 @@ class ImpressionsTrackerTest: XCTestCase { hashedImpressionsStorage: HashedImpressionsStorageMock(), generalInfoStorage: GeneralInfoStorageMock(), ruleBasedSegmentsStorage: RuleBasedSegmentsStorageStub(), - persistentRuleBasedSegmentsStorage: PersistentRuleBasedSegmentsStorageStub()) + persistentRuleBasedSegmentsStorage: PersistentRuleBasedSegmentsStorageStub(), + targetingRulesPersistenceBreaker: DefaultPersistenceBreaker()) let apiFacade = try! SplitApiFacade.builder() .setUserKey("userKey") diff --git a/SplitTests/Streaming/SynchronizerTest.swift b/SplitTests/Streaming/SynchronizerTest.swift index 63f65ca3b..2e9a8d326 100644 --- a/SplitTests/Streaming/SynchronizerTest.swift +++ b/SplitTests/Streaming/SynchronizerTest.swift @@ -69,7 +69,8 @@ class SynchronizerTest: XCTestCase { hashedImpressionsStorage: HashedImpressionsStorageMock(), generalInfoStorage: GeneralInfoStorageMock(), ruleBasedSegmentsStorage: RuleBasedSegmentsStorageStub(), - persistentRuleBasedSegmentsStorage: PersistentRuleBasedSegmentsStorageStub()) + persistentRuleBasedSegmentsStorage: PersistentRuleBasedSegmentsStorageStub(), + targetingRulesPersistenceBreaker: DefaultPersistenceBreaker()) splitConfig = SplitClientConfig() splitConfig.syncEnabled = syncEnabled diff --git a/SplitTests/TreatmentManagerTest.swift b/SplitTests/TreatmentManagerTest.swift index efbf4200b..2168ef702 100644 --- a/SplitTests/TreatmentManagerTest.swift +++ b/SplitTests/TreatmentManagerTest.swift @@ -73,7 +73,8 @@ class TreatmentManagerTest: XCTestCase { hashedImpressionsStorage: HashedImpressionsStorageMock(), generalInfoStorage: GeneralInfoStorageMock(), ruleBasedSegmentsStorage: RuleBasedSegmentsStorageStub(), - persistentRuleBasedSegmentsStorage: PersistentRuleBasedSegmentsStorageStub()) + persistentRuleBasedSegmentsStorage: PersistentRuleBasedSegmentsStorageStub(), + targetingRulesPersistenceBreaker: DefaultPersistenceBreaker()) } } From 0de62f3b82a5ee7626bd00b1e47ddcf074a94f61 Mon Sep 17 00:00:00 2001 From: Gaston Thea Date: Thu, 18 Dec 2025 13:30:46 -0300 Subject: [PATCH 5/6] Always allow clear --- Split/Storage/Splits/SplitsStorage.swift | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Split/Storage/Splits/SplitsStorage.swift b/Split/Storage/Splits/SplitsStorage.swift index e822a0901..9f08c7c54 100644 --- a/Split/Storage/Splits/SplitsStorage.swift +++ b/Split/Storage/Splits/SplitsStorage.swift @@ -132,10 +132,7 @@ class DefaultSplitsStorage: SplitsStorage { func clear() { inMemorySplits.removeAll() changeNumber = -1 - // Only call persistence if breaker allows - if persistenceBreaker.isPersistenceEnabled { - persistentStorage.clear() - } + persistentStorage.clear() } func getCount() -> Int { From 336609e5e94037a4be02950c85be58293c49df37 Mon Sep 17 00:00:00 2001 From: Gaston Thea Date: Thu, 18 Dec 2025 13:31:14 -0300 Subject: [PATCH 6/6] Fix comment --- Split/Storage/Splits/SplitsStorage.swift | 1 - 1 file changed, 1 deletion(-) diff --git a/Split/Storage/Splits/SplitsStorage.swift b/Split/Storage/Splits/SplitsStorage.swift index 9f08c7c54..02a147f79 100644 --- a/Split/Storage/Splits/SplitsStorage.swift +++ b/Split/Storage/Splits/SplitsStorage.swift @@ -312,7 +312,6 @@ class BackgroundSyncSplitsStorage: SyncSplitsStorage { } func update(splitChange: ProcessedSplitChange) -> Bool { - // Background sync doesn't use the breaker - it just persists directly // If persistence fails, it will be logged but won't trigger breaker persistentStorage.update(splitChange: splitChange, onFailure: nil) return true