diff --git a/Sources/SQLiteData/CloudKit/Internal/MockCloudContainer.swift b/Sources/SQLiteData/CloudKit/Internal/MockCloudContainer.swift index f0421c82..bb043eda 100644 --- a/Sources/SQLiteData/CloudKit/Internal/MockCloudContainer.swift +++ b/Sources/SQLiteData/CloudKit/Internal/MockCloudContainer.swift @@ -48,7 +48,7 @@ : sharedCloudDatabase let rootRecord: CKRecord? = database.storage.withValue { - $0[share.recordID.zoneID]?.values.first { record in + $0[share.recordID.zoneID]?.records.values.first { record in record.share?.recordID == share.recordID } } diff --git a/Sources/SQLiteData/CloudKit/Internal/MockCloudDatabase.swift b/Sources/SQLiteData/CloudKit/Internal/MockCloudDatabase.swift index a3a12db5..c8138bf0 100644 --- a/Sources/SQLiteData/CloudKit/Internal/MockCloudDatabase.swift +++ b/Sources/SQLiteData/CloudKit/Internal/MockCloudDatabase.swift @@ -4,11 +4,10 @@ @available(iOS 17, macOS 14, tvOS 17, watchOS 10, *) package final class MockCloudDatabase: CloudDatabase { - package let storage = LockIsolated<[CKRecordZone.ID: [CKRecord.ID: CKRecord]]>([:]) + package let storage = LockIsolated<[CKRecordZone.ID: Zone]>([:]) let assets = LockIsolated<[AssetID: Data]>([:]) package let databaseScope: CKDatabase.Scope let _container = IsolatedWeakVar() - let dataManager = Dependency(\.dataManager) struct AssetID: Hashable { @@ -16,6 +15,11 @@ let key: String } + package struct Zone { + package var zone: CKRecordZone + package var records: [CKRecord.ID: CKRecord] = [:] + } + package init(databaseScope: CKDatabase.Scope) { self.databaseScope = databaseScope } @@ -34,7 +38,7 @@ else { throw ckError(forAccountStatus: accountStatus) } guard let zone = storage[recordID.zoneID] else { throw CKError(.zoneNotFound) } - guard let record = zone[recordID] + guard let record = zone.records[recordID] else { throw CKError(.unknownItem) } guard let record = record.copy() as? CKRecord else { fatalError("Could not copy CKRecord.") } @@ -81,6 +85,7 @@ else { throw ckError(forAccountStatus: accountStatus) } return storage.withValue { storage in + let previousStorage = storage var saveResults: [CKRecord.ID: Result] = [:] var deleteResults: [CKRecord.ID: Result] = [:] @@ -91,7 +96,8 @@ let isSavingRootRecord = recordsToSave.contains(where: { $0.share?.recordID == share.recordID }) - let shareWasPreviouslySaved = storage[share.recordID.zoneID]?[share.recordID] != nil + let shareWasPreviouslySaved = + storage[share.recordID.zoneID]?.records[share.recordID] != nil guard shareWasPreviouslySaved || isSavingRootRecord else { saveResults[recordToSave.recordID] = .failure(CKError(.invalidArguments)) @@ -114,12 +120,14 @@ continue } - let existingRecord = storage[recordToSave.recordID.zoneID]?[recordToSave.recordID] + let existingRecord = storage[recordToSave.recordID.zoneID]?.records[ + recordToSave.recordID + ] func saveRecordToDatabase() { let hasReferenceViolation = recordToSave.parent.map { parent in - storage[parent.recordID.zoneID]?[parent.recordID] == nil + storage[parent.recordID.zoneID]?.records[parent.recordID] == nil && !recordsToSave.contains { $0.recordID == parent.recordID } } ?? false @@ -132,10 +140,12 @@ func root(of record: CKRecord) -> CKRecord { guard let parent = record.parent else { return record } - return (storage[parent.recordID.zoneID]?[parent.recordID]).map(root) ?? record + return (storage[parent.recordID.zoneID]?.records[parent.recordID]).map( + root + ) ?? record } func share(for rootRecord: CKRecord) -> CKShare? { - for (_, record) in storage[rootRecord.recordID.zoneID] ?? [:] { + for (_, record) in storage[rootRecord.recordID.zoneID]?.records ?? [:] { guard record.recordID == rootRecord.share?.recordID else { continue } return record as? CKShare @@ -169,7 +179,7 @@ } // TODO: This should merge copy's values to more accurately reflect reality - storage[recordToSave.recordID.zoneID]?[recordToSave.recordID] = copy + storage[recordToSave.recordID.zoneID]?.records[recordToSave.recordID] = copy saveResults[recordToSave.recordID] = .success(copy) } @@ -228,7 +238,7 @@ continue } let hasReferenceViolation = !Set( - storage[recordIDToDelete.zoneID]?.values + storage[recordIDToDelete.zoneID]?.records.values .compactMap { $0.parent?.recordID == recordIDToDelete ? $0.recordID : nil } ?? [] ) @@ -240,8 +250,8 @@ deleteResults[recordIDToDelete] = .failure(CKError(.referenceViolation)) continue } - let recordToDelete = storage[recordIDToDelete.zoneID]?[recordIDToDelete] - storage[recordIDToDelete.zoneID]?[recordIDToDelete] = nil + let recordToDelete = storage[recordIDToDelete.zoneID]?.records[recordIDToDelete] + storage[recordIDToDelete.zoneID]?.records[recordIDToDelete] = nil deleteResults[recordIDToDelete] = .success(()) // NB: If deleting a share that the current user owns, delete the shared records and all @@ -251,14 +261,14 @@ shareToDelete.recordID.zoneID.ownerName == CKCurrentUserDefaultName { func deleteRecords(referencing recordID: CKRecord.ID) { - for recordToDelete in (storage[recordIDToDelete.zoneID] ?? [:]).values { + for recordToDelete in (storage[recordIDToDelete.zoneID]?.records ?? [:]).values { guard recordToDelete.share?.recordID == recordID || recordToDelete.parent?.recordID == recordID else { continue } - storage[recordIDToDelete.zoneID]?[recordToDelete.recordID] = nil + storage[recordIDToDelete.zoneID]?.records[recordToDelete.recordID] = nil deleteRecords(referencing: recordToDelete.recordID) } } @@ -266,6 +276,41 @@ } } + guard atomically + else { + return (saveResults: saveResults, deleteResults: deleteResults) + } + + let affectedZones = Set( + recordsToSave.map(\.recordID.zoneID) + recordIDsToDelete.map(\.zoneID) + ) + for zoneID in affectedZones { + let saveResultsInZone = saveResults.filter { recordID, _ in recordID.zoneID == zoneID } + let deleteResultsInZone = deleteResults.filter { recordID, _ in + recordID.zoneID == zoneID + } + let saveSuccessRecordIDs = saveResultsInZone.compactMap { recordID, result in + (try? result.get()) == nil ? nil : recordID + } + let deleteSuccessRecordIDs = deleteResultsInZone.compactMap { recordID, result in + (try? result.get()) == nil ? nil : recordID + } + guard + saveSuccessRecordIDs.count != saveResultsInZone.count + || deleteSuccessRecordIDs.count != deleteResultsInZone.count + else { + continue + } + // Every successful save and deletion becomes a '.batchRequestFailed'. + for saveSuccessRecordID in saveSuccessRecordIDs { + saveResults[saveSuccessRecordID] = .failure(CKError(.batchRequestFailed)) + } + for deleteSuccessRecordID in deleteSuccessRecordIDs { + deleteResults[deleteSuccessRecordID] = .failure(CKError(.batchRequestFailed)) + } + // All storage changes are reverted in zone. + storage[zoneID]?.records = previousStorage[zoneID]?.records ?? [:] + } return (saveResults: saveResults, deleteResults: deleteResults) } } @@ -286,7 +331,8 @@ var deleteResults: [CKRecordZone.ID: Result] = [:] for recordZoneToSave in recordZonesToSave { - storage[recordZoneToSave.zoneID] = storage[recordZoneToSave.zoneID] ?? [:] + storage[recordZoneToSave.zoneID] = + storage[recordZoneToSave.zoneID] ?? Zone(zone: recordZoneToSave) saveResults[recordZoneToSave.zoneID] = .success(recordZoneToSave) } diff --git a/Sources/SQLiteData/CloudKit/Internal/MockSyncEngine.swift b/Sources/SQLiteData/CloudKit/Internal/MockSyncEngine.swift index 4f65532f..336cb6c1 100644 --- a/Sources/SQLiteData/CloudKit/Internal/MockSyncEngine.swift +++ b/Sources/SQLiteData/CloudKit/Internal/MockSyncEngine.swift @@ -48,7 +48,7 @@ } records = zoneIDs.reduce(into: [CKRecord]()) { accum, zoneID in accum += database.storage.withValue { - ($0[zoneID]?.values).map { Array($0) } ?? [] + ($0[zoneID]?.records.values).map { Array($0) } ?? [] } } await parentSyncEngine.handleEvent( @@ -177,6 +177,7 @@ package func processPendingRecordZoneChanges( options: CKSyncEngine.SendChangesOptions = CKSyncEngine.SendChangesOptions(), scope: CKDatabase.Scope, + forceAtomicByZone: Bool? = nil, fileID: StaticString = #fileID, filePath: StaticString = #filePath, line: UInt = #line, @@ -208,7 +209,7 @@ return } - let batch = await nextRecordZoneChangeBatch( + var batch = await nextRecordZoneChangeBatch( reason: .scheduled, options: options, syncEngine: { @@ -224,6 +225,9 @@ } }() ) + if let forceAtomicByZone { + batch?.atomicByZone = forceAtomicByZone + } guard let batch else { return } @@ -231,7 +235,7 @@ saving: batch.recordsToSave, deleting: batch.recordIDsToDelete, savePolicy: .ifServerRecordUnchanged, - atomically: true + atomically: batch.atomicByZone ) var savedRecords: [CKRecord] = [] @@ -263,9 +267,15 @@ syncEngine.state.remove( pendingRecordZoneChanges: savedRecords.map { .saveRecord($0.recordID) } ) + syncEngine.state.remove( + pendingRecordZoneChanges: failedRecordSaves.map { .saveRecord($0.record.recordID) } + ) syncEngine.state.remove( pendingRecordZoneChanges: deletedRecordIDs.map { .deleteRecord($0) } ) + syncEngine.state.remove( + pendingRecordZoneChanges: failedRecordDeletes.keys.map { .deleteRecord($0) } + ) await syncEngine.parentSyncEngine .handleEvent( diff --git a/Sources/SQLiteData/CloudKit/SyncEngine.swift b/Sources/SQLiteData/CloudKit/SyncEngine.swift index 72595053..f4b97a0b 100644 --- a/Sources/SQLiteData/CloudKit/SyncEngine.swift +++ b/Sources/SQLiteData/CloudKit/SyncEngine.swift @@ -912,8 +912,7 @@ parentForeignKey: parentForeignKey, defaultZone: defaultZone, privateTables: privateTables - ) - { + ) { try trigger.execute(db) } } @@ -929,7 +928,7 @@ defaultZone: defaultZone, privateTables: privateTables ) - .reversed() { + .reversed() { try trigger.drop().execute(db) } } @@ -1696,8 +1695,12 @@ try await open(table) } + case .batchRequestFailed: + newPendingRecordZoneChanges.append(.saveRecord(failedRecord.recordID)) + break + case .networkFailure, .networkUnavailable, .zoneBusy, .serviceUnavailable, - .notAuthenticated, .operationCancelled, .batchRequestFailed, + .notAuthenticated, .operationCancelled, .internalError, .partialFailure, .badContainer, .requestRateLimited, .missingEntitlement, .invalidArguments, .resultsTruncated, .assetFileNotFound, .assetFileModified, .incompatibleVersion, .constraintViolation, .changeTokenExpired, @@ -1719,15 +1722,32 @@ try await userDatabase.write { db in var enqueuedUnsyncedRecordID = false for (failedRecordID, error) in failedRecordDeletes { - guard - error.code == .referenceViolation - else { continue } - try UnsyncedRecordID.insert(or: .ignore) { - UnsyncedRecordID(recordID: failedRecordID) + switch error.code { + case .referenceViolation: + enqueuedUnsyncedRecordID = true + try UnsyncedRecordID.insert(or: .ignore) { + UnsyncedRecordID(recordID: failedRecordID) + } + .execute(db) + syncEngine.state.remove(pendingRecordZoneChanges: [.deleteRecord(failedRecordID)]) + break + case .batchRequestFailed: + syncEngine.state.add(pendingRecordZoneChanges: [.deleteRecord(failedRecordID)]) + break + case .networkFailure, .networkUnavailable, .zoneBusy, .serviceUnavailable, + .notAuthenticated, .operationCancelled, .internalError, .partialFailure, + .badContainer, .requestRateLimited, .missingEntitlement, .invalidArguments, + .resultsTruncated, .assetFileNotFound, .assetFileModified, .incompatibleVersion, + .constraintViolation, .changeTokenExpired, .badDatabase, .quotaExceeded, + .limitExceeded, .userDeletedZone, .tooManyParticipants, .alreadyShared, + .managedAccountRestricted, .participantMayNeedVerification, .serverResponseLost, + .assetNotAvailable, .accountTemporarilyUnavailable, .permissionFailure, + .unknownItem, .serverRecordChanged, .serverRejectedRequest, .zoneNotFound, + .participantAlreadyInvited: + break + @unknown default: + break } - .execute(db) - syncEngine.state.remove(pendingRecordZoneChanges: [.deleteRecord(failedRecordID)]) - enqueuedUnsyncedRecordID = true } return enqueuedUnsyncedRecordID } diff --git a/Tests/SQLiteDataTests/CloudKitTests/AtomicTests.swift b/Tests/SQLiteDataTests/CloudKitTests/AtomicTests.swift new file mode 100644 index 00000000..b1e54c4a --- /dev/null +++ b/Tests/SQLiteDataTests/CloudKitTests/AtomicTests.swift @@ -0,0 +1,202 @@ +#if canImport(CloudKit) + import CloudKit + import ConcurrencyExtras + import CustomDump + import InlineSnapshotTesting + import OrderedCollections + import SQLiteData + import SQLiteDataTestSupport + import SnapshotTestingCustomDump + import Testing + + extension BaseCloudKitTests { + @MainActor + final class AtomicTests: BaseCloudKitTests, @unchecked Sendable { + @available(iOS 17, macOS 14, tvOS 17, watchOS 10, *) + @Test func editConflictAndNewRecord() async throws { + try await userDatabase.userWrite { db in + try db.seed { + RemindersList(id: 1, title: "Personal") + } + } + try await syncEngine.processPendingRecordZoneChanges(scope: .private) + + let remindersListRecord = try syncEngine.private.database.record( + for: RemindersList.recordID(for: 1) + ) + remindersListRecord.setValue("My stuff", forKey: "title", at: 1) + let (saveResults, _) = try syncEngine.private.database.modifyRecords(saving: [remindersListRecord]) + #expect(saveResults.values.allSatisfy { $0.error == nil }) + + try await withDependencies { + $0.currentTime.now = 2 + } operation: { + try await userDatabase.userWrite { db in + try RemindersList.find(1).update { $0.title = "Stuff" }.execute(db) + try RemindersList.insert { RemindersList(id: 2, title: "Business") }.execute(db) + } + } + try await syncEngine.processPendingRecordZoneChanges( + scope: .private, + forceAtomicByZone: true + ) + + assertInlineSnapshot(of: container, as: .customDump) { + """ + MockCloudContainer( + privateCloudDatabase: MockCloudDatabase( + databaseScope: .private, + storage: [ + [0]: CKRecord( + recordID: CKRecord.ID(1:remindersLists/zone/__defaultOwner__), + recordType: "remindersLists", + parent: nil, + share: nil, + id: 1, + title: "My stuff" + ) + ] + ), + sharedCloudDatabase: MockCloudDatabase( + databaseScope: .shared, + storage: [] + ) + ) + """ + } + + try await syncEngine.processPendingRecordZoneChanges( + scope: .private, + forceAtomicByZone: true + ) + + assertInlineSnapshot(of: container, as: .customDump) { + """ + MockCloudContainer( + privateCloudDatabase: MockCloudDatabase( + databaseScope: .private, + storage: [ + [0]: CKRecord( + recordID: CKRecord.ID(1:remindersLists/zone/__defaultOwner__), + recordType: "remindersLists", + parent: nil, + share: nil, + id: 1, + title: "Stuff" + ), + [1]: CKRecord( + recordID: CKRecord.ID(2:remindersLists/zone/__defaultOwner__), + recordType: "remindersLists", + parent: nil, + share: nil, + id: 2, + title: "Business" + ) + ] + ), + sharedCloudDatabase: MockCloudDatabase( + databaseScope: .shared, + storage: [] + ) + ) + """ + } + } + + @available(iOS 17, macOS 14, tvOS 17, watchOS 10, *) + @Test func editConflictAndDeleteRecord() async throws { + try await userDatabase.userWrite { db in + try db.seed { + RemindersList(id: 1, title: "Personal") + RemindersList(id: 2, title: "Business") + } + } + try await syncEngine.processPendingRecordZoneChanges( + scope: .private, + forceAtomicByZone: true + ) + + let remindersListRecord = try syncEngine.private.database.record( + for: RemindersList.recordID(for: 1) + ) + remindersListRecord.setValue("My stuff", forKey: "title", at: 1) + let (saveResults, _) = try syncEngine.private.database.modifyRecords(saving: [remindersListRecord]) + #expect(saveResults.values.allSatisfy { $0.error == nil }) + + try await withDependencies { + $0.currentTime.now = 2 + } operation: { + try await userDatabase.userWrite { db in + try RemindersList.find(1).update { $0.title = "Stuff" }.execute(db) + try RemindersList.find(2).delete().execute(db) + } + } + try await syncEngine.processPendingRecordZoneChanges( + scope: .private, + forceAtomicByZone: true + ) + + assertInlineSnapshot(of: container, as: .customDump) { + """ + MockCloudContainer( + privateCloudDatabase: MockCloudDatabase( + databaseScope: .private, + storage: [ + [0]: CKRecord( + recordID: CKRecord.ID(1:remindersLists/zone/__defaultOwner__), + recordType: "remindersLists", + parent: nil, + share: nil, + id: 1, + title: "My stuff" + ), + [1]: CKRecord( + recordID: CKRecord.ID(2:remindersLists/zone/__defaultOwner__), + recordType: "remindersLists", + parent: nil, + share: nil, + id: 2, + title: "Business" + ) + ] + ), + sharedCloudDatabase: MockCloudDatabase( + databaseScope: .shared, + storage: [] + ) + ) + """ + } + + try await syncEngine.processPendingRecordZoneChanges( + scope: .private, + forceAtomicByZone: true + ) + + assertInlineSnapshot(of: container, as: .customDump) { + """ + MockCloudContainer( + privateCloudDatabase: MockCloudDatabase( + databaseScope: .private, + storage: [ + [0]: CKRecord( + recordID: CKRecord.ID(1:remindersLists/zone/__defaultOwner__), + recordType: "remindersLists", + parent: nil, + share: nil, + id: 1, + title: "Stuff" + ) + ] + ), + sharedCloudDatabase: MockCloudDatabase( + databaseScope: .shared, + storage: [] + ) + ) + """ + } + } + } + } +#endif diff --git a/Tests/SQLiteDataTests/CloudKitTests/ForeignKeyConstraintTests.swift b/Tests/SQLiteDataTests/CloudKitTests/ForeignKeyConstraintTests.swift index d9369f95..bf6d011c 100644 --- a/Tests/SQLiteDataTests/CloudKitTests/ForeignKeyConstraintTests.swift +++ b/Tests/SQLiteDataTests/CloudKitTests/ForeignKeyConstraintTests.swift @@ -668,7 +668,7 @@ """ } assertInlineSnapshot( - of: syncEngine.private.database.storage[syncEngine.defaultZone.zoneID]?[ + of: syncEngine.private.database.storage[syncEngine.defaultZone.zoneID]?.records[ Reminder.recordID(for: 1) ], as: .customDump @@ -763,7 +763,7 @@ """ } assertInlineSnapshot( - of: syncEngine.private.database.storage[syncEngine.defaultZone.zoneID]?[ + of: syncEngine.private.database.storage[syncEngine.defaultZone.zoneID]?.records[ Reminder.recordID(for: 1) ], as: .customDump diff --git a/Tests/SQLiteDataTests/CloudKitTests/MockCloudDatabaseTests.swift b/Tests/SQLiteDataTests/CloudKitTests/MockCloudDatabaseTests.swift index ac6db0e9..245121d5 100644 --- a/Tests/SQLiteDataTests/CloudKitTests/MockCloudDatabaseTests.swift +++ b/Tests/SQLiteDataTests/CloudKitTests/MockCloudDatabaseTests.swift @@ -605,6 +605,46 @@ """ } } + + @Test func batchRequestFailed() async throws { + let record1ID = CKRecord.ID(recordName: "1") + let record2ID = CKRecord.ID(recordName: "2") + + do { + let record1 = CKRecord(recordType: "record1", recordID: record1ID) + let record2 = CKRecord(recordType: "record2", recordID: record2ID) + let (saveResults, _) = try syncEngine.private.database.modifyRecords(saving: [ + record1, record2, + ]) + #expect(saveResults.values.count(where: { (try? $0.get()) != nil }) == 2) + } + + let freshRecord2 = try syncEngine.private.database.record(for: record2ID) + do { + let freshRecord1 = try syncEngine.private.database.record(for: record1ID) + freshRecord1["isOn"] = true + freshRecord2["isOn"] = true + let (saveResults, _) = try syncEngine.private.database.modifyRecords( + saving: [freshRecord1, freshRecord2] + ) + #expect(saveResults.values.count(where: { (try? $0.get()) != nil }) == 2) + } + + do { + let freshRecord1 = try syncEngine.private.database.record(for: record1ID) + freshRecord1["isOn"] = true + freshRecord2["isOn"] = false + let (saveResults, _) = try syncEngine.private.database.modifyRecords( + saving: [freshRecord1, freshRecord2] + ) + #expect( + saveResults.compactMapValues { ($0.error as? CKError)?.code } == [ + record1ID: .batchRequestFailed, + record2ID: .serverRecordChanged + ] + ) + } + } } } #endif diff --git a/Tests/SQLiteDataTests/Internal/CloudKit+CustomDump.swift b/Tests/SQLiteDataTests/Internal/CloudKit+CustomDump.swift index 3514158e..5874e5d6 100644 --- a/Tests/SQLiteDataTests/Internal/CloudKit+CustomDump.swift +++ b/Tests/SQLiteDataTests/Internal/CloudKit+CustomDump.swift @@ -239,7 +239,7 @@ "databaseScope": databaseScope, "storage": storage .value - .flatMap { _, value in value.values } + .flatMap { _, value in value.records.values } .sorted { ($0.recordType, $0.recordID.recordName) < ($1.recordType, $1.recordID.recordName) }, diff --git a/Tests/SQLiteDataTests/Internal/CloudKitTestHelpers.swift b/Tests/SQLiteDataTests/Internal/CloudKitTestHelpers.swift index 4b22c99a..207fd4c8 100644 --- a/Tests/SQLiteDataTests/Internal/CloudKitTestHelpers.swift +++ b/Tests/SQLiteDataTests/Internal/CloudKitTestHelpers.swift @@ -65,7 +65,8 @@ extension SyncEngine { func modifyRecords( scope: CKDatabase.Scope, saving recordsToSave: [CKRecord] = [], - deleting recordIDsToDelete: [CKRecord.ID] = [] + deleting recordIDsToDelete: [CKRecord.ID] = [], + atomically: Bool = true ) throws -> ModifyRecordsCallback< ( saveResults: [CKRecord.ID: Result], @@ -75,7 +76,7 @@ extension SyncEngine { let syncEngine = syncEngine(for: scope) let recordsToDeleteByID = Dictionary( grouping: syncEngine.database.storage.withValue { storage in - recordIDsToDelete.compactMap { recordID in storage[recordID.zoneID]?[recordID] } + recordIDsToDelete.compactMap { recordID in storage[recordID.zoneID]?.records[recordID] } }, by: \.recordID ) @@ -83,7 +84,8 @@ extension SyncEngine { let (saveResults, deleteResults) = try syncEngine.database.modifyRecords( saving: recordsToSave, - deleting: recordIDsToDelete + deleting: recordIDsToDelete, + atomically: atomically ) return ModifyRecordsCallback { diff --git a/Tests/SQLiteDataTests/Internal/ResultExtensions.swift b/Tests/SQLiteDataTests/Internal/ResultExtensions.swift new file mode 100644 index 00000000..31c785eb --- /dev/null +++ b/Tests/SQLiteDataTests/Internal/ResultExtensions.swift @@ -0,0 +1,10 @@ +extension Result { + var error: Failure? { + do { + _ = try get() + return nil + } catch { + return error + } + } +}