-
Notifications
You must be signed in to change notification settings - Fork 113
Fix 'limitExceeded' errors related to FK constraint failures. #359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bf01f0e
8dc739e
96360ff
e060702
c2ac6f2
40aa5f2
0e3e2c1
281ec18
c7c420e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ | |
| package let tables: [any SynchronizableTable] | ||
| package let privateTables: [any SynchronizableTable] | ||
| let tablesByName: [String: any SynchronizableTable] | ||
| private let tablesByOrder: [String: Int] | ||
| package let tablesByOrder: [String: Int] | ||
| let foreignKeysByTableName: [String: [ForeignKey]] | ||
| package let syncEngines = LockIsolated<SyncEngines>(SyncEngines()) | ||
| package let defaultZone: CKRecordZone | ||
|
|
@@ -217,7 +217,7 @@ | |
| tables: [any SynchronizableTable], | ||
| privateTables: [any SynchronizableTable] = [] | ||
| ) throws { | ||
| let allTables = Set((tables + privateTables).map(HashableSynchronizedTable.init)) | ||
| let allTables = OrderedSet((tables + privateTables).map(HashableSynchronizedTable.init)) | ||
| .map(\.type) | ||
| self.tables = allTables | ||
| self.privateTables = privateTables | ||
|
|
@@ -1419,11 +1419,11 @@ | |
| ) async { | ||
| let deletedRecordIDsByRecordType = OrderedDictionary( | ||
| grouping: deletions.sorted { lhs, rhs in | ||
| guard | ||
| let lhsIndex = tablesByOrder[lhs.recordType], | ||
| let rhsIndex = tablesByOrder[rhs.recordType] | ||
| else { return true } | ||
| return lhsIndex > rhsIndex | ||
| topologicallyAscending( | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a bunch of places we do this sorting logic, and I needed to do it again, so I added a helper for it. |
||
| lhsTableName: lhs.recordType, | ||
| rhsTableName: rhs.recordType, | ||
| rootFirst: false | ||
| ) | ||
| }, | ||
| by: \.recordType | ||
| ) | ||
|
|
@@ -1490,32 +1490,43 @@ | |
| .execute(db) | ||
| } | ||
| } | ||
| let results = try await syncEngine.database.records(for: Array(unsyncedRecordIDs)) | ||
| let batchSize = 150 | ||
| let orderedUnsyncedRecordIDs = unsyncedRecordIDs.sorted { | ||
|
Comment on lines
+1493
to
+1494
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now invoking |
||
| topologicallyAscending( | ||
| lhsTableName: $0.tableName, | ||
| rhsTableName: $1.tableName, | ||
| rootFirst: true | ||
| ) | ||
| } | ||
| var unsyncedRecords: [CKRecord] = [] | ||
| for (recordID, result) in results { | ||
| switch result { | ||
| case .success(let record): | ||
| unsyncedRecords.append(record) | ||
| case .failure(let error as CKError) where error.code == .unknownItem: | ||
| try await userDatabase.write { db in | ||
| try UnsyncedRecordID.find(recordID).delete().execute(db) | ||
| for start in stride(from: 0, to: orderedUnsyncedRecordIDs.count, by: batchSize) { | ||
| let recordIDsBatch = orderedUnsyncedRecordIDs | ||
| .dropFirst(start) | ||
| .prefix(batchSize) | ||
| let results = try await syncEngine.database.records(for: Array(recordIDsBatch)) | ||
| for (recordID, result) in results { | ||
| switch result { | ||
| case .success(let record): | ||
| unsyncedRecords.append(record) | ||
| case .failure(let error as CKError) where error.code == .unknownItem: | ||
| try await userDatabase.write { db in | ||
| try UnsyncedRecordID.find(recordID).delete().execute(db) | ||
| } | ||
| case .failure: | ||
| continue | ||
| } | ||
| case .failure: | ||
| continue | ||
| } | ||
| } | ||
| return unsyncedRecords | ||
| } | ||
| ?? [CKRecord]() | ||
|
|
||
| let modifications = (modifications + unsyncedRecords).sorted { lhs, rhs in | ||
| guard | ||
| let lhsRecordType = lhs.recordID.tableName, | ||
| let lhsIndex = tablesByOrder[lhsRecordType], | ||
| let rhsRecordType = rhs.recordID.tableName, | ||
| let rhsIndex = tablesByOrder[rhsRecordType] | ||
| else { return true } | ||
| return lhsIndex < rhsIndex | ||
| topologicallyAscending( | ||
| lhsTableName: lhs.recordType, | ||
| rhsTableName: rhs.recordType, | ||
| rootFirst: true | ||
| ) | ||
| } | ||
|
|
||
| enum ShareOrReference { | ||
|
|
@@ -1563,6 +1574,27 @@ | |
| } | ||
| } | ||
|
|
||
| private func topologicallyAscending( | ||
| lhsTableName: String?, | ||
| rhsTableName: String?, | ||
| rootFirst: Bool | ||
| ) -> Bool { | ||
| switch (lhsTableName, rhsTableName) { | ||
| case (nil, nil), (nil, _): | ||
| return false | ||
| case (_, nil): | ||
| return true | ||
| case let (.some(lhs), .some(rhs)): | ||
| let lhsIndex = tablesByOrder[lhs] ?? (rootFirst ? .max : .min) | ||
| let rhsIndex = tablesByOrder[rhs] ?? (rootFirst ? .max : .min) | ||
| guard lhsIndex != rhsIndex | ||
| else { | ||
| return lhs < rhs | ||
| } | ||
| return rootFirst ? lhsIndex < rhsIndex : lhsIndex > rhsIndex | ||
| } | ||
| } | ||
|
|
||
| package func handleSentRecordZoneChanges( | ||
| savedRecords: [CKRecord] = [], | ||
| failedRecordSaves: [(record: CKRecord, error: CKError)] = [], | ||
|
|
@@ -2292,10 +2324,12 @@ | |
| tablesByName: [String: any SynchronizableTable] | ||
| ) throws -> [String: Int] { | ||
| let tableDependencies = try userDatabase.read { db in | ||
| var dependencies: [HashableSynchronizedTable: [any SynchronizableTable]] = [:] | ||
| var dependencies: OrderedDictionary<HashableSynchronizedTable, [any SynchronizableTable]> = [:] | ||
| for table in tables { | ||
| func open<T>(_: some SynchronizableTable<T>) throws -> [String] { | ||
| try PragmaForeignKeyList<T>.select(\.table) | ||
| try PragmaForeignKeyList<T> | ||
| .order(by: \.table) | ||
| .select(\.table) | ||
| .fetchAll(db) | ||
| } | ||
| let toTables = try open(table) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -869,6 +869,70 @@ | |
| """ | ||
| } | ||
| } | ||
|
|
||
| /* | ||
| * Create a parent record in CloudKit database but do not sync to client. | ||
| * Create many child records in CloudKit database and **do** sync to client. | ||
| * Sync parent record to client. | ||
| * => Cached unsaved child records should be batched so as to not run into 'limitExceeded' | ||
| errors | ||
| */ | ||
| @available(iOS 17, macOS 14, tvOS 17, watchOS 10, *) | ||
| @Test func batchAssociations() async throws { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test failed before the fixes in this PR because it creates 500 reminders and syncs them to the client before the client receives the reminders list. That lead to the limit exceeded error and the system couldn't recover from that. |
||
|
|
||
| let remindersListRecord = CKRecord( | ||
| recordType: RemindersList.tableName, | ||
| recordID: RemindersList.recordID(for: 1) | ||
| ) | ||
| remindersListRecord.setValue(1, forKey: "id", at: now) | ||
| remindersListRecord.setValue("Personal", forKey: "title", at: now) | ||
| let remindersListModification = try syncEngine.modifyRecords( | ||
| scope: .private, | ||
| saving: [remindersListRecord] | ||
| ) | ||
|
|
||
| let reminderRecords = (1...500).map { index in | ||
| let reminderRecord = CKRecord( | ||
| recordType: Reminder.tableName, | ||
| recordID: Reminder.recordID(for: index) | ||
| ) | ||
| reminderRecord.setValue(index, forKey: "id", at: now) | ||
| reminderRecord.setValue("Reminder #\(index)", forKey: "title", at: now) | ||
| reminderRecord.setValue(1, forKey: "remindersListID", at: now) | ||
| reminderRecord.parent = CKRecord.Reference( | ||
| record: remindersListRecord, | ||
| action: .none | ||
| ) | ||
| return reminderRecord | ||
| } | ||
|
|
||
| try await syncEngine.modifyRecords( | ||
| scope: .private, | ||
| saving: Array(reminderRecords[0...100]) | ||
| ).notify() | ||
| try await syncEngine.modifyRecords( | ||
| scope: .private, | ||
| saving: Array(reminderRecords[101...200]) | ||
| ).notify() | ||
| try await syncEngine.modifyRecords( | ||
| scope: .private, | ||
| saving: Array(reminderRecords[201...300]) | ||
| ).notify() | ||
| try await syncEngine.modifyRecords( | ||
| scope: .private, | ||
| saving: Array(reminderRecords[301...400]) | ||
| ).notify() | ||
| try await syncEngine.modifyRecords( | ||
| scope: .private, | ||
| saving: Array(reminderRecords[401...499]) | ||
| ).notify() | ||
| await remindersListModification.notify() | ||
|
|
||
| try await userDatabase.read { db in | ||
| try #expect(RemindersList.fetchCount(db) == 1) | ||
| try #expect(Reminder.fetchCount(db) == 500) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| #if canImport(CloudKit) | ||
| import CloudKit | ||
| import SQLiteData | ||
| import Testing | ||
|
|
||
| extension BaseCloudKitTests { | ||
| @MainActor | ||
| final class TopologicalTableSortingTests: BaseCloudKitTests, @unchecked Sendable { | ||
| @available(iOS 17, macOS 14, tvOS 17, watchOS 10, *) | ||
| @Test func tablesByOrder() async throws { | ||
| #expect( | ||
| syncEngine.tablesByOrder == [ | ||
| "remindersLists": 0, | ||
| "reminders": 1, | ||
| "remindersListAssets": 2, | ||
| "tags": 3, | ||
| "reminderTags": 4, | ||
| "parents": 5, | ||
| "childWithOnDeleteSetNulls": 6, | ||
| "childWithOnDeleteSetDefaults": 7, | ||
| "modelAs": 8, | ||
| "modelBs": 9, | ||
| "modelCs": 10, | ||
| "remindersListPrivates": 11, | ||
| ] | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to the core of this PR, but I did want to write a test on our topological sorting of tables, and so needed the order of things to be stable.