diff --git a/CHANGELOG.md b/CHANGELOG.md index f99f74dec..38191a6e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- A column resized moments before its table was dropped saving the layout back over the clear. +- A connection left pointing at a database that was dropped. - A dropped table's saved filters, column layout, highlight rules, value formats and label columns coming back on a table recreated with its name. - A dropped database or schema leaving every one of its tables' saved settings behind. - Favorites rows left behind by a dropped table, schema or database. @@ -220,6 +222,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security +- SQL favorites deleted by another Mac's sync tombstoned again from this one, pushing the deletion back at it. - Query history kept for a connection deleted on another Mac, including the statements' own text and its literals. - A locked launch on iPhone and iPad connecting to the last session, and asking to trust a host key, before Face ID was answered. - Code inside a plugin bundle, and its resource envelope, were not verified before the bundle was loaded. diff --git a/TablePro/Core/Services/Query/CatalogEditAdoption.swift b/TablePro/Core/Services/Query/CatalogEditAdoption.swift index 9afd9c1ad..eff5b3591 100644 --- a/TablePro/Core/Services/Query/CatalogEditAdoption.swift +++ b/TablePro/Core/Services/Query/CatalogEditAdoption.swift @@ -42,10 +42,19 @@ struct LoadedBrowseCatalog: Sendable, Equatable { struct CatalogEditAdoption { private let databaseManager: DatabaseManager private let schemaService: SchemaService + private let connectionStorage: ConnectionStorage + private let appSettings: AppSettingsStorage - init(databaseManager: DatabaseManager = .shared, schemaService: SchemaService = .shared) { + init( + databaseManager: DatabaseManager = .shared, + schemaService: SchemaService = .shared, + connectionStorage: ConnectionStorage = .shared, + appSettings: AppSettingsStorage = .shared + ) { self.databaseManager = databaseManager self.schemaService = schemaService + self.connectionStorage = connectionStorage + self.appSettings = appSettings } /// Where the object lives. A reference without a database means the one being browsed, and the @@ -157,6 +166,7 @@ struct CatalogEditAdoption { } guard container.kind == .database else { return } FavoriteDatabasesStorage.shared.removeFavorite(database: database, connectionId: connectionId) + clearSavedConnectionDatabase(named: database, connectionId: connectionId) sidebarState.clearRecentTables(inDatabase: database) var selected = sidebarState.databaseFilterSelected guard selected.remove(database) != nil else { return } @@ -276,11 +286,31 @@ struct CatalogEditAdoption { /// The saved default is what a reconnect and Reopen Last Session both use, so a database renamed /// out from under it leaves the connection opening onto nothing. - private func retargetSavedConnectionDatabase(from oldName: String, to newName: String, connectionId: UUID) { - guard var saved = ConnectionStorage.shared.loadConnections().first(where: { $0.id == connectionId }), + internal func retargetSavedConnectionDatabase(from oldName: String, to newName: String, connectionId: UUID) { + guard var saved = connectionStorage.loadConnections().first(where: { $0.id == connectionId }), saved.database == oldName else { return } saved.database = newName - ConnectionStorage.shared.updateConnection(saved) + connectionStorage.updateConnection(saved) + } + + /// A rename has a new name to point the saved default at. A drop has none, so it is emptied, + /// along with the last database the session remembered. + /// + /// Both, because `selectDatabaseFromLastSession` fires precisely when the saved default is + /// empty: emptying one and leaving the other would turn that action on and point it at the + /// database that was just dropped, so every later connect would try to switch to it and fail. + /// + /// Not for a type that requires a value. Emptying is the repair for an engine that accepts a + /// blank database, which is what the form already allows there, and MySQL connects with no + /// default while MongoDB picks one. A type whose form refuses to save without a value would be + /// left failing its own validation with nothing on screen saying why. + internal func clearSavedConnectionDatabase(named database: String, connectionId: UUID) { + guard var saved = connectionStorage.loadConnections().first(where: { $0.id == connectionId }), + saved.database == database, + !ConnectionDatabaseRequirement.requiresValue(for: saved.type) else { return } + saved.database = "" + connectionStorage.updateConnection(saved) + appSettings.saveLastDatabase(nil, for: connectionId) } private func retargetBrowseCursor(_ connection: DatabaseConnection, from oldName: String, to newName: String) { diff --git a/TablePro/Core/Storage/ColumnLayoutPersister.swift b/TablePro/Core/Storage/ColumnLayoutPersister.swift index c884cb719..97fa333e5 100644 --- a/TablePro/Core/Storage/ColumnLayoutPersister.swift +++ b/TablePro/Core/Storage/ColumnLayoutPersister.swift @@ -209,14 +209,21 @@ final class FileColumnLayoutPersister: ColumnLayoutPersisting, TableScopedSettin syncTracker.markDeleted(.settings, ids: dropping.map(Self.syncCategory(for:))) } - func purgeConnections(_ connectionIds: Set) { - var deletedCategories: [String] = [] + func purgeConnections(_ connectionIds: Set, leavesTombstones: Bool) { + var categories: [String] = [] for connectionId in connectionIds { - deletedCategories += loadEntries(for: connectionId).keys.map(Self.syncCategory(for:)) + categories += loadEntries(for: connectionId).keys.map(Self.syncCategory(for:)) cache[connectionId] = [:] removeFile(for: connectionId) } - syncTracker.markDeleted(.settings, ids: deletedCategories) + /// The dirty marks go either way. A tombstone from a remote delete would push the sender's + /// own deletion back at it, but leaving the ids dirty means the next push looks for entries + /// that are gone and never drains them. + if leavesTombstones { + syncTracker.markDeleted(.settings, ids: categories) + } else { + syncTracker.discardDirty(.settings, ids: categories) + } } func clear(for key: ColumnLayoutTableKey) { diff --git a/TablePro/Core/Storage/ConnectionLocalState.swift b/TablePro/Core/Storage/ConnectionLocalState.swift index f6371f5ce..2ded7b2ac 100644 --- a/TablePro/Core/Storage/ConnectionLocalState.swift +++ b/TablePro/Core/Storage/ConnectionLocalState.swift @@ -45,12 +45,16 @@ internal enum ConnectionLocalState { } for store in tableScopedStores { - store.purgeConnections(connectionIds) + store.purgeConnections(connectionIds, leavesTombstones: origin == .local) } DatabaseTreeFilterStorage.shared.removeFilters(for: connectionIds) RecentlyClosedTabStore.shared.removeEntries(for: connectionIds) WorkspaceRailOrderStore.shared.removeEntries(for: connectionIds) - Task { await purgeAsyncStores(connectionIds, sqlFavorites: sqlFavorites, queryHistory: queryHistory) } + Task { + await purgeAsyncStores( + connectionIds, origin: origin, sqlFavorites: sqlFavorites, queryHistory: queryHistory + ) + } } /// The two stores that can only be reached with `await`, so `purge` fires them and does not @@ -61,19 +65,27 @@ internal enum ConnectionLocalState { /// /// Separate from `purge` so a test can await what `purge` cannot. /// - /// `origin` does not reach here, and the two stores differ on why. Query history is device-local - /// and never synced, so a remote delete should forget this device's copy and has no tombstone to - /// push back. SQL favorites are synced and `removeFavoritesAndFolders` tombstones every record it - /// removes, so a remote delete does push one back at the device that sent it. That predates this - /// helper and is carried unchanged rather than fixed here, because the without-sync counterpart - /// `purgeFavorites` uses for the table favorites does not exist for these. + /// `origin` splits the SQL favorites the way `purgeFavorites` splits the table ones, and for the + /// same reason. `SyncCoordinator.applyRemoteChanges` suppresses the change tracker only for the + /// length of its own synchronous body, and this runs from a `Task` that starts after that body + /// has returned and the suppression has been reset, so a remote delete really did write + /// tombstones and push the sender's own deletion back at it. + /// + /// Query history takes no origin: it is device-local and never synced, so a remote delete + /// should forget this device's copy and has no tombstone to leave either way. internal static func purgeAsyncStores( _ connectionIds: Set, + origin: Origin, sqlFavorites: SQLFavoriteManager = .shared, queryHistory: QueryHistoryManager = .shared ) async { for connectionId in connectionIds { - await sqlFavorites.removeFavoritesAndFolders(for: connectionId) + switch origin { + case .local: + await sqlFavorites.removeFavoritesAndFolders(for: connectionId) + case .remote: + await sqlFavorites.removeFavoritesAndFoldersWithoutSync(for: connectionId) + } if await !queryHistory.deleteEverything(forConnection: connectionId) { logger.error( "Query history for a deleted connection could not be cleared: \(connectionId, privacy: .public)" diff --git a/TablePro/Core/Storage/FilterSettingsStorage.swift b/TablePro/Core/Storage/FilterSettingsStorage.swift index 8ff877f56..f40554ab6 100644 --- a/TablePro/Core/Storage/FilterSettingsStorage.swift +++ b/TablePro/Core/Storage/FilterSettingsStorage.swift @@ -547,7 +547,7 @@ final class FilterSettingsStorage: TableScopedSettingsStore { ) + Self.browseKeySuffix } - func purgeConnections(_ connectionIds: Set) { + func purgeConnections(_ connectionIds: Set, leavesTombstones: Bool) { guard !connectionIds.isEmpty else { return } let encodedPrefixes = connectionIds.map { TableScope.storagePrefix(connectionId: $0) } diff --git a/TablePro/Core/Storage/ForeignKeyLabelColumnStore.swift b/TablePro/Core/Storage/ForeignKeyLabelColumnStore.swift index 849bddd19..ba7e2d68e 100644 --- a/TablePro/Core/Storage/ForeignKeyLabelColumnStore.swift +++ b/TablePro/Core/Storage/ForeignKeyLabelColumnStore.swift @@ -69,7 +69,7 @@ internal final class ForeignKeyLabelColumnStore: TableScopedSettingsStore { ) } - func purgeConnections(_ connectionIds: Set) { + func purgeConnections(_ connectionIds: Set, leavesTombstones: Bool) { for connectionId in connectionIds { store.removeValues(withPrefix: Self.keyPrefix + TableScope.storagePrefix(connectionId: connectionId)) } diff --git a/TablePro/Core/Storage/HighlightRuleStorage.swift b/TablePro/Core/Storage/HighlightRuleStorage.swift index ab00cef8e..21ef25682 100644 --- a/TablePro/Core/Storage/HighlightRuleStorage.swift +++ b/TablePro/Core/Storage/HighlightRuleStorage.swift @@ -94,7 +94,7 @@ final class HighlightRuleStorage: ObservableObject, TableScopedSettingsStore { commit(entries, for: connectionId) } - func purgeConnections(_ connectionIds: Set) { + func purgeConnections(_ connectionIds: Set, leavesTombstones: Bool) { guard !connectionIds.isEmpty else { return } for connectionId in connectionIds { cache[connectionId] = [:] diff --git a/TablePro/Core/Storage/SQLFavoriteManager.swift b/TablePro/Core/Storage/SQLFavoriteManager.swift index 37ffc154f..cf2783e8a 100644 --- a/TablePro/Core/Storage/SQLFavoriteManager.swift +++ b/TablePro/Core/Storage/SQLFavoriteManager.swift @@ -79,6 +79,17 @@ internal final class SQLFavoriteManager: @unchecked Sendable { postUpdateNotification(connectionId: nil) } + /// Used when another device deleted the connection. Marking tombstones here would push its own + /// deletion straight back at it, which is the reason `FavoriteTablesStorage` splits the same + /// way. + func removeFavoritesAndFoldersWithoutSync(for connectionId: UUID) async { + let removed = await storage.deleteFavoritesAndFolders(connectionId: connectionId) + guard !removed.isEmpty else { return } + syncTracker.discardDirty(.favorite, ids: removed.favorites.map(\.uuidString)) + syncTracker.discardDirty(.favoriteFolder, ids: removed.folders.map(\.uuidString)) + postUpdateNotification(connectionId: nil) + } + func pruneOrphaned(activeConnectionIds: Set) async { await storage.pruneOrphaned(retaining: activeConnectionIds) } diff --git a/TablePro/Core/Storage/TableScopedSettingsStore.swift b/TablePro/Core/Storage/TableScopedSettingsStore.swift index f6a207033..22879eabc 100644 --- a/TablePro/Core/Storage/TableScopedSettingsStore.swift +++ b/TablePro/Core/Storage/TableScopedSettingsStore.swift @@ -26,7 +26,10 @@ internal protocol TableScopedSettingsStore: AnyObject { /// disk and could never be named here. A nil schema means the whole database. func dropContainer(connectionId: UUID, database: String, schema: String?) - func purgeConnections(_ connectionIds: Set) + /// Forgets everything these connections saved. `leavesTombstones` is false when another device + /// did the deleting: a synced store must not mark its records deleted there, or it pushes the + /// sender's own deletion back at it. + func purgeConnections(_ connectionIds: Set, leavesTombstones: Bool) } @MainActor diff --git a/TablePro/Core/Storage/ValueDisplayFormatStorage.swift b/TablePro/Core/Storage/ValueDisplayFormatStorage.swift index 8f5c24ad7..287d1b59b 100644 --- a/TablePro/Core/Storage/ValueDisplayFormatStorage.swift +++ b/TablePro/Core/Storage/ValueDisplayFormatStorage.swift @@ -76,7 +76,7 @@ internal final class ValueDisplayFormatStorage: TableScopedSettingsStore { ) } - func purgeConnections(_ connectionIds: Set) { + func purgeConnections(_ connectionIds: Set, leavesTombstones: Bool) { for connectionId in connectionIds { store.removeValues(withPrefix: Self.keyPrefix + TableScope.storagePrefix(connectionId: connectionId)) store.removeValues(withPrefix: Self.legacyKeyPrefix(for: connectionId)) diff --git a/TablePro/Core/Sync/SyncChangeTracker.swift b/TablePro/Core/Sync/SyncChangeTracker.swift index a3d5c58c0..09c7643ee 100644 --- a/TablePro/Core/Sync/SyncChangeTracker.swift +++ b/TablePro/Core/Sync/SyncChangeTracker.swift @@ -61,6 +61,16 @@ final class SyncChangeTracker: Sendable { postChangeNotification() } + /// Forgets that records were waiting to be pushed, without tombstoning them. + /// + /// For records another device already deleted: a tombstone would send its own deletion back at + /// it, but leaving the dirty ids behind is not free either. The next push looks for records + /// that are gone, skips them, and never drains the entries. + func discardDirty(_ type: SyncRecordType, ids: [String]) { + guard !ids.isEmpty else { return } + metadataStorage.removeDirty(ids, type: type) + } + func markDeleted(_ type: SyncRecordType, ids: [String]) { guard !isSuppressed, !ids.isEmpty else { return } metadataStorage.removeDirty(ids, type: type) diff --git a/TablePro/Models/Connection/ConnectionDatabaseRequirement.swift b/TablePro/Models/Connection/ConnectionDatabaseRequirement.swift new file mode 100644 index 000000000..a97ba35b5 --- /dev/null +++ b/TablePro/Models/Connection/ConnectionDatabaseRequirement.swift @@ -0,0 +1,37 @@ +// +// ConnectionDatabaseRequirement.swift +// TablePro +// + +import Foundation +import TableProPluginKit + +/// Whether a connection of this type has to carry a database name to be usable. +/// +/// One owner, because two answers drift. The connection form refuses to save without a value when +/// this is true, and the catalog adoption refuses to empty the field for the same reason: a +/// dropped database leaves nothing to put there, and a type that requires one would be left +/// failing its own validation with nothing on screen saying why. +@MainActor +internal enum ConnectionDatabaseRequirement { + /// Whether the form renders the built-in Database field at all. A driver opts out through + /// `hidesBuiltInDatabase` when it names its container some other way, or has none. + internal static func showsBuiltInField(for type: DatabaseType) -> Bool { + let hidden = PluginMetadataRegistry.shared.snapshot(for: type)?.connection.hidesBuiltInDatabase ?? false + switch PluginManager.shared.connectionMode(for: type) { + case .fileBased: + return false + case .apiOnly: + return PluginManager.shared.supportsDatabaseSwitching(for: type) && !hidden + default: + return !hidden + } + } + + /// Never require a value the form does not render. A file-based connection stores its path in + /// `database` and renders it as the Database File field. + internal static func requiresValue(for type: DatabaseType) -> Bool { + let mode = PluginManager.shared.connectionMode(for: type) + return mode == .fileBased || (mode == .apiOnly && showsBuiltInField(for: type)) + } +} diff --git a/TablePro/Views/ConnectionForm/ViewModels/NetworkPaneViewModel.swift b/TablePro/Views/ConnectionForm/ViewModels/NetworkPaneViewModel.swift index 887dc0830..b2c576947 100644 --- a/TablePro/Views/ConnectionForm/ViewModels/NetworkPaneViewModel.swift +++ b/TablePro/Views/ConnectionForm/ViewModels/NetworkPaneViewModel.swift @@ -78,25 +78,14 @@ final class NetworkPaneViewModel: ObservableObject { .connection.hidesBuiltInDatabase ?? false } - /// Whether the form renders the built-in Database field. A driver opts out through - /// `hidesBuiltInDatabase` when it names its container some other way, or has none. - /// This is deliberately not `requiresAuthentication`: whether a driver needs credentials - /// says nothing about whether it accepts a database name. + /// This is deliberately not `requiresAuthentication`: whether a driver needs credentials says + /// nothing about whether it accepts a database name. var showsBuiltInDatabaseField: Bool { - switch connectionMode { - case .fileBased: - return false - case .apiOnly: - return PluginManager.shared.supportsDatabaseSwitching(for: type) && !hidesBuiltInDatabase - default: - return !hidesBuiltInDatabase - } + ConnectionDatabaseRequirement.showsBuiltInField(for: type) } - /// Never require a value the form does not render. A file-based connection stores its - /// path in `database` and renders it as the Database File field. var requiresDatabaseValue: Bool { - connectionMode == .fileBased || (connectionMode == .apiOnly && showsBuiltInDatabaseField) + ConnectionDatabaseRequirement.requiresValue(for: type) } var validationIssues: [String] { diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+TabClosing.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+TabClosing.swift index 87b300fe6..8ff425baa 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+TabClosing.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+TabClosing.swift @@ -38,6 +38,16 @@ extension MainContentCoordinator { /// Closed Tab, which could only reopen onto an error, and the saved tab set is never cleared, /// because an emptied tab list is not consent to forget it. func closeTabsForRemovedObjects(ids: [UUID]) { + /// Discarded rather than flushed, which is what the user-close path does. The object is + /// gone and the adoption has already cleared its saved settings, so a write from the + /// teardown would put the layout back after the clear. + /// + /// Only when the closed tabs include the selected one, because that is the only tab with a + /// mounted grid: a pending width belongs to it, and dropping an unrelated table would + /// otherwise throw away a resize the reader had just made somewhere else. + if let selectedId = tabManager.selectedTabId, ids.contains(selectedId) { + dataTabDelegate?.tableViewCoordinator?.discardPendingColumnLayoutPersistence() + } for id in ids { guard let tab = tabManager.tabs.first(where: { $0.id == id }) else { continue } releaseResources(of: tab) diff --git a/TablePro/Views/Results/Extensions/DataGridView+Selection.swift b/TablePro/Views/Results/Extensions/DataGridView+Selection.swift index d62f44940..1dca5c58e 100644 --- a/TablePro/Views/Results/Extensions/DataGridView+Selection.swift +++ b/TablePro/Views/Results/Extensions/DataGridView+Selection.swift @@ -43,6 +43,17 @@ extension TableViewCoordinator { } } + /// Throws the pending layout away instead of writing it, for a table that is gone. + /// + /// A drop clears the table's saved layout and then closes its tab, and the teardown runs on a + /// later run-loop turn, so a flush there would write the layout back over the clear and mark it + /// dirty for sync again, waiting for a table recreated with the same name. + func discardPendingColumnLayoutPersistence() { + layoutPersistTask?.cancel() + layoutPersistTask = nil + pendingColumnLayoutPersistence = nil + } + func flushPendingColumnLayoutPersistence() { layoutPersistTask?.cancel() layoutPersistTask = nil diff --git a/TableProTests/Core/Services/Query/SavedConnectionDatabaseAdoptionTests.swift b/TableProTests/Core/Services/Query/SavedConnectionDatabaseAdoptionTests.swift new file mode 100644 index 000000000..b140fd1ad --- /dev/null +++ b/TableProTests/Core/Services/Query/SavedConnectionDatabaseAdoptionTests.swift @@ -0,0 +1,104 @@ +// +// SavedConnectionDatabaseAdoptionTests.swift +// TableProTests +// + +import Foundation +@testable import TablePro +import TableProSyncTransport +import Testing + +/// The connection's own Database field after the database it names is renamed or dropped. A +/// reconnect and Reopen Last Session both read it, so a stale one opens the connection onto +/// nothing every time. +@Suite("Saved connection database adoption") +@MainActor +struct SavedConnectionDatabaseAdoptionTests { + private func makeStorage() -> ConnectionStorage { + let unique = UUID().uuidString + let fileURL = FileManager.default.temporaryDirectory + .appendingPathComponent("tablepro-tests") + .appendingPathComponent("connections_\(unique).json") + try? FileManager.default.createDirectory( + at: fileURL.deletingLastPathComponent(), withIntermediateDirectories: true + ) + guard let defaults = UserDefaults(suiteName: "com.TablePro.tests.SavedDatabase.\(unique)"), + let syncDefaults = UserDefaults(suiteName: "com.TablePro.tests.SavedDatabaseSync.\(unique)") + else { + fatalError("Failed to create isolated test user defaults") + } + return ConnectionStorage( + fileURL: fileURL, + userDefaults: defaults, + syncTracker: SyncChangeTracker(metadataStorage: SyncMetadataStorage(userDefaults: syncDefaults)), + keychain: InMemoryKeychain() + ) + } + + private func seed(_ storage: ConnectionStorage, database: String) -> DatabaseConnection { + let connection = TestFixtures.makeConnection(database: database) + _ = storage.saveConnections([connection]) + return connection + } + + private func savedDatabase(_ storage: ConnectionStorage, _ id: UUID) -> String? { + storage.loadConnections().first { $0.id == id }?.database + } + + @Test("Dropping the database a connection is configured for empties its Database field") + func dropClearsTheSavedDatabase() { + let storage = makeStorage() + let connection = seed(storage, database: "staging") + let adoption = CatalogEditAdoption(connectionStorage: storage) + + adoption.clearSavedConnectionDatabase(named: "staging", connectionId: connection.id) + + #expect(savedDatabase(storage, connection.id) == "") + } + + @Test("Dropping a different database leaves the Database field alone") + func dropOfAnotherDatabaseChangesNothing() { + let storage = makeStorage() + let connection = seed(storage, database: "staging") + let adoption = CatalogEditAdoption(connectionStorage: storage) + + adoption.clearSavedConnectionDatabase(named: "prod", connectionId: connection.id) + + #expect(savedDatabase(storage, connection.id) == "staging") + } + + @Test("A connection that is already blank stays blank") + func blankSavedDatabaseIsUntouched() { + let storage = makeStorage() + let connection = seed(storage, database: "") + let adoption = CatalogEditAdoption(connectionStorage: storage) + + adoption.clearSavedConnectionDatabase(named: "staging", connectionId: connection.id) + + #expect(savedDatabase(storage, connection.id) == "") + } + + /// The rename counterpart has shipped untested since it was written. It is the reason the drop + /// has to do something at all, so it is pinned here beside it. + @Test("Renaming the database a connection is configured for follows it to the new name") + func renameFollowsTheSavedDatabase() { + let storage = makeStorage() + let connection = seed(storage, database: "staging") + let adoption = CatalogEditAdoption(connectionStorage: storage) + + adoption.retargetSavedConnectionDatabase(from: "staging", to: "staging_v2", connectionId: connection.id) + + #expect(savedDatabase(storage, connection.id) == "staging_v2") + } + + @Test("Renaming a different database leaves the Database field alone") + func renameOfAnotherDatabaseChangesNothing() { + let storage = makeStorage() + let connection = seed(storage, database: "staging") + let adoption = CatalogEditAdoption(connectionStorage: storage) + + adoption.retargetSavedConnectionDatabase(from: "prod", to: "prod_v2", connectionId: connection.id) + + #expect(savedDatabase(storage, connection.id) == "staging") + } +} diff --git a/TableProTests/Core/Storage/ColumnLayoutSyncTests.swift b/TableProTests/Core/Storage/ColumnLayoutSyncTests.swift index 6b64a85ab..4c4fb9f23 100644 --- a/TableProTests/Core/Storage/ColumnLayoutSyncTests.swift +++ b/TableProTests/Core/Storage/ColumnLayoutSyncTests.swift @@ -38,6 +38,43 @@ struct ColumnLayoutSyncTests { ColumnLayoutTableKey(connectionId: UUID(), databaseName: "shop", schemaName: "public", tableName: "orders") } + /// The other device already told CloudKit. A tombstone from here would push its deletion back, + /// but the dirty marks still have to go or the next push hunts for entries that are gone. + @Test("A remote connection delete drops the layouts without tombstoning them") + func remotePurgeLeavesNoTombstone() throws { + let (persister, metadata, directory) = try makeTrackedPersister() + defer { try? FileManager.default.removeItem(at: directory) } + let connectionId = UUID() + let key = ColumnLayoutTableKey( + connectionId: connectionId, databaseName: "shop", schemaName: "public", tableName: "orders" + ) + persister.save(layout(["id": 80]), for: key) + + persister.purgeConnections([connectionId], leavesTombstones: false) + + #expect(persister.load(for: key) == nil) + #expect(metadata.tombstones(for: .settings).isEmpty) + #expect(!metadata.dirtyIds(for: .settings).contains(FileColumnLayoutPersister.syncCategory(for: key.storageKey))) + } + + @Test("A local connection delete tombstones the layouts it drops") + func localPurgeTombstones() throws { + let (persister, metadata, directory) = try makeTrackedPersister() + defer { try? FileManager.default.removeItem(at: directory) } + let connectionId = UUID() + let key = ColumnLayoutTableKey( + connectionId: connectionId, databaseName: "shop", schemaName: "public", tableName: "orders" + ) + persister.save(layout(["id": 80]), for: key) + + persister.purgeConnections([connectionId], leavesTombstones: true) + + #expect( + metadata.tombstones(for: .settings) + .contains { $0.id == FileColumnLayoutPersister.syncCategory(for: key.storageKey) } + ) + } + @Test("Dropping a table tombstones its layout so the deletion syncs") func dropTableTombstonesTheLayout() throws { let (persister, metadata, directory) = try makeTrackedPersister() @@ -153,7 +190,7 @@ struct ColumnLayoutSyncTests { persister.save(layout(["id": 90]), for: items) persister.save(layout(["id": 70]), for: kept) - persister.purgeConnections([connectionId]) + persister.purgeConnections([connectionId], leavesTombstones: true) let file = directory.appendingPathComponent("\(connectionId.uuidString).json") #expect(!FileManager.default.fileExists(atPath: file.path)) diff --git a/TableProTests/Core/Storage/ConnectionLocalStatePurgeTests.swift b/TableProTests/Core/Storage/ConnectionLocalStatePurgeTests.swift index 8cdbf08f6..d7b019812 100644 --- a/TableProTests/Core/Storage/ConnectionLocalStatePurgeTests.swift +++ b/TableProTests/Core/Storage/ConnectionLocalStatePurgeTests.swift @@ -6,6 +6,7 @@ import Foundation @testable import TablePro import TableProPluginKit +import TableProSyncTransport import Testing /// The stores a deleted connection leaves behind that can only be reached with `await`, and the @@ -68,7 +69,7 @@ struct ConnectionLocalStatePurgeTests { let deleted = UUID() _ = await storage.record(entry(connectionId: deleted, query: "SELECT secret FROM billing")) - await ConnectionLocalState.purgeAsyncStores([deleted], queryHistory: manager) + await ConnectionLocalState.purgeAsyncStores([deleted], origin: .local, queryHistory: manager) #expect(await recordedQueries(storage, connectionId: deleted).isEmpty) } @@ -81,7 +82,7 @@ struct ConnectionLocalStatePurgeTests { _ = await storage.record(entry(connectionId: deleted, query: "SELECT secret FROM billing")) _ = await storage.record(entry(connectionId: kept, query: "SELECT 1")) - await ConnectionLocalState.purgeAsyncStores([deleted], queryHistory: manager) + await ConnectionLocalState.purgeAsyncStores([deleted], origin: .local, queryHistory: manager) #expect(await recordedQueries(storage, connectionId: deleted).isEmpty) #expect(await recordedQueries(storage, connectionId: kept) == ["SELECT 1"]) @@ -95,12 +96,70 @@ struct ConnectionLocalStatePurgeTests { _ = await storage.record(entry(connectionId: first, query: "SELECT 1")) _ = await storage.record(entry(connectionId: second, query: "SELECT 2")) - await ConnectionLocalState.purgeAsyncStores([first, second], queryHistory: manager) + await ConnectionLocalState.purgeAsyncStores([first, second], origin: .local, queryHistory: manager) #expect(await recordedQueries(storage, connectionId: first).isEmpty) #expect(await recordedQueries(storage, connectionId: second).isEmpty) } + private func makeFavorites() -> (SQLFavoriteManager, SyncMetadataStorage) { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent("tablepro-tests") + .appendingPathComponent("purge_favorites_\(UUID().uuidString).db") + try? FileManager.default.createDirectory( + at: url.deletingLastPathComponent(), withIntermediateDirectories: true + ) + let metadata = SyncMetadataStorage( + userDefaults: UserDefaults(suiteName: "tablepro-purge-\(UUID().uuidString)") ?? .standard, + prefix: "tests.\(UUID().uuidString)" + ) + let manager = SQLFavoriteManager( + storage: SQLFavoriteStorage(databaseURL: url, removeDatabaseOnDeinit: true), + syncTracker: SyncChangeTracker(metadataStorage: metadata) + ) + return (manager, metadata) + } + + private func seedFavorite(_ manager: SQLFavoriteManager, connectionId: UUID) async -> SQLFavorite { + let favorite = SQLFavorite( + name: "Active users", query: "SELECT * FROM users", connectionId: connectionId + ) + _ = await manager.addFavorite(favorite) + return favorite + } + + /// The device that deleted the connection already told CloudKit. A tombstone from here sends + /// its own deletion back at it. + @Test("A remote purge removes the SQL favorites without tombstoning them") + func remotePurgeLeavesNoFavoriteTombstone() async { + let (favorites, metadata) = makeFavorites() + let (history, _) = makeHistory() + let deleted = UUID() + let favorite = await seedFavorite(favorites, connectionId: deleted) + + await ConnectionLocalState.purgeAsyncStores( + [deleted], origin: .remote, sqlFavorites: favorites, queryHistory: history + ) + + #expect(await favorites.fetchFavorites(connectionId: deleted).isEmpty) + #expect(!metadata.tombstones(for: .favorite).contains { $0.id == favorite.id.uuidString }) + } + + @Test("A local purge tombstones the SQL favorites it removes") + func localPurgeTombstonesFavorites() async { + let (favorites, metadata) = makeFavorites() + let (history, _) = makeHistory() + let deleted = UUID() + let favorite = await seedFavorite(favorites, connectionId: deleted) + + await ConnectionLocalState.purgeAsyncStores( + [deleted], origin: .local, sqlFavorites: favorites, queryHistory: history + ) + + #expect(await favorites.fetchFavorites(connectionId: deleted).isEmpty) + #expect(metadata.tombstones(for: .favorite).contains { $0.id == favorite.id.uuidString }) + } + /// A filtered clear deletes from `history` alone, and both snapshot tables keep their own copy /// of the statement: `plan_snapshots.subject_sql` and `raw_plan`. Their `history_id` is /// `ON DELETE SET NULL`, so the plan outlived the history row it came from. @@ -126,7 +185,7 @@ struct ConnectionLocalStatePurgeTests { ) #expect(!(await storage.planSnapshots(matching: identity, excluding: nil, limit: 10)).isEmpty) - await ConnectionLocalState.purgeAsyncStores([deleted], queryHistory: manager) + await ConnectionLocalState.purgeAsyncStores([deleted], origin: .local, queryHistory: manager) #expect((await storage.planSnapshots(matching: identity, excluding: nil, limit: 10)).isEmpty) } @@ -151,7 +210,7 @@ struct ConnectionLocalStatePurgeTests { ) ) - await ConnectionLocalState.purgeAsyncStores([deleted], queryHistory: manager) + await ConnectionLocalState.purgeAsyncStores([deleted], origin: .local, queryHistory: manager) #expect(!(await storage.planSnapshots(matching: keptIdentity, excluding: nil, limit: 10)).isEmpty) } @@ -168,7 +227,11 @@ struct ConnectionLocalStatePurgeTests { @Test("Only ConnectionLocalState clears a deleted connection's async stores") func onlyConnectionLocalStateClearsAsyncStores() throws { let root = try Self.repoRoot().appendingPathComponent("TablePro", isDirectory: true) - let calls = ["removeFavoritesAndFolders(for:", "deleteEverything(forConnection:"] + let calls = [ + "removeFavoritesAndFolders(for:", + "removeFavoritesAndFoldersWithoutSync(for:", + "deleteEverything(forConnection:", + ] var offenders: [String] = [] for url in try Self.swiftSources(under: root) diff --git a/TableProTests/Core/Storage/FilterSettingsStorageTests.swift b/TableProTests/Core/Storage/FilterSettingsStorageTests.swift index eddb79e26..22061aabf 100644 --- a/TableProTests/Core/Storage/FilterSettingsStorageTests.swift +++ b/TableProTests/Core/Storage/FilterSettingsStorageTests.swift @@ -290,7 +290,7 @@ struct FilterSettingsStorageTests { for: "users", connectionId: keptConnection, databaseName: "db", schemaName: nil ) - storage.purgeConnections([deletedConnection]) + storage.purgeConnections([deletedConnection], leavesTombstones: true) storage.waitForPendingDiskWrites() #expect( @@ -317,7 +317,7 @@ struct FilterSettingsStorageTests { ) } - storage.purgeConnections([first, second]) + storage.purgeConnections([first, second], leavesTombstones: true) storage.waitForPendingDiskWrites() #expect(storage.loadLastFilters(for: "users", connectionId: first, databaseName: "db", schemaName: nil).isEmpty) @@ -343,7 +343,7 @@ struct FilterSettingsStorageTests { for: "users", connectionId: connectionId, databaseName: "db", schemaName: nil ) - storage.purgeConnections([connectionId]) + storage.purgeConnections([connectionId], leavesTombstones: true) storage.waitForPendingDiskWrites() let fresh = FilterSettingsStorage(filterStateDirectory: directory, defaults: defaults) diff --git a/TableProTests/Core/Storage/HighlightRuleStorageTests.swift b/TableProTests/Core/Storage/HighlightRuleStorageTests.swift index a04b11d0c..5129a8c2e 100644 --- a/TableProTests/Core/Storage/HighlightRuleStorageTests.swift +++ b/TableProTests/Core/Storage/HighlightRuleStorageTests.swift @@ -142,7 +142,7 @@ struct HighlightRuleStorageTests { let storage = HighlightRuleStorage(storageDirectory: directory) storage.setRules([HighlightRule(columnName: "status", value: "paid")], for: scope(table: "orders")) - storage.purgeConnections([connectionId]) + storage.purgeConnections([connectionId], leavesTombstones: true) #expect(storage.rules(for: scope(table: "orders")).isEmpty) #expect(!FileManager.default.fileExists(atPath: fileURL.path)) @@ -157,7 +157,7 @@ struct HighlightRuleStorageTests { let preserved = directory.appendingPathComponent("\(connectionId.uuidString).unreadable.json") #expect(FileManager.default.fileExists(atPath: preserved.path)) - storage.purgeConnections([connectionId]) + storage.purgeConnections([connectionId], leavesTombstones: true) #expect(!FileManager.default.fileExists(atPath: preserved.path)) } diff --git a/TableProTests/Core/Storage/SQLFavoriteDeletionSyncTests.swift b/TableProTests/Core/Storage/SQLFavoriteDeletionSyncTests.swift index 26f78b9e9..023754f76 100644 --- a/TableProTests/Core/Storage/SQLFavoriteDeletionSyncTests.swift +++ b/TableProTests/Core/Storage/SQLFavoriteDeletionSyncTests.swift @@ -41,6 +41,52 @@ struct SQLFavoriteDeletionSyncTests { Set(metadata.tombstones(for: type).map(\.id)) } + /// When the other device did the deleting, a tombstone here pushes its own deletion straight + /// back at it. The rows still go. + @Test("A remote delete removes the rows and tombstones nothing") + func remoteDeleteLeavesNoTombstone() async { + let connectionId = UUID() + let folder = SQLFavoriteFolder(name: "Reports", connectionId: connectionId) + let favorite = SQLFavorite( + name: "Active users", + query: "SELECT * FROM users", + folderId: folder.id, + connectionId: connectionId + ) + #expect(await manager.addFolder(folder)) + #expect(await manager.addFavorite(favorite)) + + await manager.removeFavoritesAndFoldersWithoutSync(for: connectionId) + + #expect(!tombstonedIds(.favorite).contains(favorite.id.uuidString)) + #expect(!tombstonedIds(.favoriteFolder).contains(folder.id.uuidString)) + #expect(await manager.fetchFavorites(connectionId: connectionId).isEmpty) + } + + /// Without this the id stays dirty for good: the next push looks for a record that is gone, + /// skips it, and nothing ever drains the entry. + @Test("A remote delete drains the dirty marks of what it removed") + func remoteDeleteDrainsDirtyMarks() async { + let connectionId = UUID() + let favorite = SQLFavorite( + name: "Active users", query: "SELECT * FROM users", connectionId: connectionId + ) + #expect(await manager.addFavorite(favorite)) + #expect(metadata.dirtyIds(for: .favorite).contains(favorite.id.uuidString)) + + await manager.removeFavoritesAndFoldersWithoutSync(for: connectionId) + + #expect(!metadata.dirtyIds(for: .favorite).contains(favorite.id.uuidString)) + } + + @Test("A remote delete with nothing to remove tombstones nothing") + func remoteDeleteOfNothingTombstonesNothing() async { + await manager.removeFavoritesAndFoldersWithoutSync(for: UUID()) + + #expect(tombstonedIds(.favorite).isEmpty) + #expect(tombstonedIds(.favoriteFolder).isEmpty) + } + @Test("Deleting a connection tombstones its favorites and its folders") func connectionDeleteTombstonesEverythingItRemoved() async { let connectionId = UUID() diff --git a/TableProTests/Core/Storage/TableScopedSettingsRegistryTests.swift b/TableProTests/Core/Storage/TableScopedSettingsRegistryTests.swift index f190e2d17..0b27bc9fc 100644 --- a/TableProTests/Core/Storage/TableScopedSettingsRegistryTests.swift +++ b/TableProTests/Core/Storage/TableScopedSettingsRegistryTests.swift @@ -19,6 +19,7 @@ struct TableScopedSettingsRegistryTests { } private(set) var purgedConnectionIds: [Set] = [] + private(set) var purgedTombstoneFlags: [Bool] = [] private(set) var droppedTables: [TableScope] = [] private(set) var droppedContainers: [DroppedContainer] = [] @@ -40,8 +41,9 @@ struct TableScopedSettingsRegistryTests { droppedContainers.append(DroppedContainer(connectionId: connectionId, database: database, schema: schema)) } - func purgeConnections(_ connectionIds: Set) { + func purgeConnections(_ connectionIds: Set, leavesTombstones: Bool) { purgedConnectionIds.append(connectionIds) + purgedTombstoneFlags.append(leavesTombstones) } } @@ -62,6 +64,36 @@ struct TableScopedSettingsRegistryTests { #expect(second.purgedConnectionIds == [deleted]) } + /// A synced store must not tombstone what another device already deleted, or it sends that + /// device's own deletion back at it. + @Test("A remote delete purges every store without leaving tombstones") + func remotePurgeLeavesNoTombstones() { + let store = RecordingStore() + + ConnectionLocalState.purge( + connectionIds: [UUID()], + origin: .remote, + tableScopedStores: [store], + queryHistory: Self.isolatedQueryHistory() + ) + + #expect(store.purgedTombstoneFlags == [false]) + } + + @Test("A local delete purges every store and leaves tombstones") + func localPurgeLeavesTombstones() { + let store = RecordingStore() + + ConnectionLocalState.purge( + connectionIds: [UUID()], + origin: .local, + tableScopedStores: [store], + queryHistory: Self.isolatedQueryHistory() + ) + + #expect(store.purgedTombstoneFlags == [true]) + } + @Test("An empty delete purges nothing") func emptyPurgeReachesNoStore() { let store = RecordingStore() @@ -76,30 +108,38 @@ struct TableScopedSettingsRegistryTests { #expect(store.purgedConnectionIds.isEmpty) } - @Test("Every store keyed by a table scope conforms and is registered") - func everyTableScopedStoreIsRegistered() throws { - let root = try Self.repoRoot() - let storageDirectory = root.appendingPathComponent("TablePro/Core/Storage", isDirectory: true) + /// Every type that persists state keyed by a connection and a database must either conform to + /// `TableScopedSettingsStore`, so the rename, drop and purge hooks reach it, or be named here + /// with a reason. + /// + /// The list is the point. The previous version looked for three sentinel type names inside one + /// directory, so `FavoriteTablesStorage`, whose key is its own `FavoriteEntry` struct, was + /// invisible to it, and a dropped table went on leaving its star behind with nothing failing. + /// A store is found by the shape of its key now, so one that invents a fourth key type is still + /// seen, and an exemption is a line in a diff someone has to justify. + private static let exemptStoreTypes: [String: String] = [ + "FavoriteTablesStorage": """ + Its lifecycle turns on the local-or-remote origin that TableScopedSettingsStore does not carry, so ConnectionLocalState and CatalogEditAdoption drive it by name. + """, + "FavoriteDatabasesStorage": """ + Keyed by connection and database with no table, and origin-sensitive like its sibling, so it is driven by name from the same two places. + """, + ] + + @Test("Every store keyed by a connection and a database conforms, or is a named exception") + func everyConnectionScopedStoreIsRegisteredOrExempt() throws { + let app = try Self.repoRoot().appendingPathComponent("TablePro", isDirectory: true) + let sources = try Self.swiftSources(under: app) let registry = try String( - contentsOf: storageDirectory.appendingPathComponent("TableScopedSettingsStore.swift"), + contentsOf: app.appendingPathComponent("Core/Storage/TableScopedSettingsStore.swift"), encoding: .utf8 ) - let keyUsage = try NSRegularExpression(pattern: #"\b(TableScope|CompositeStorageKey|ColumnLayoutTableKey)\b"#) - let classDeclaration = try NSRegularExpression(pattern: #"\bclass\s+([A-Z]\w*)"#) - var storeTypes: Set = [] - for text in try Self.swiftSources(under: storageDirectory) where Self.matches(keyUsage, in: text) { - storeTypes.formUnion(Self.captures(classDeclaration, in: text)) - } - - let conformance = try NSRegularExpression( - pattern: #"\b(?:class|extension)\s+([A-Z]\w*)\s*:[^{]*\bTableScopedSettingsStore\b"# - ) - var conformingTypes: Set = [] - for text in try Self.swiftSources(under: root.appendingPathComponent("TablePro", isDirectory: true)) { - conformingTypes.formUnion(Self.captures(conformance, in: text)) - } + let keyTypes = Set(sources.flatMap(Self.connectionScopedKeyTypes(in:))) + #expect(keyTypes.isSuperset(of: ["TableScope", "FavoriteEntry", "FavoriteDatabaseEntry"])) + let storeTypes = Set(sources.flatMap { Self.storesSpeaking(keyTypes, in: $0) }) + .subtracting(Self.exemptStoreTypes.keys) #expect(storeTypes.isSuperset(of: [ "FilterSettingsStorage", "FileColumnLayoutPersister", @@ -107,10 +147,18 @@ struct TableScopedSettingsRegistryTests { "ValueDisplayFormatStorage", "ForeignKeyLabelColumnStore" ])) - let unconformed = storeTypes.subtracting(conformingTypes) + + let conformance = try NSRegularExpression( + pattern: #"\b(?:class|extension)\s+([A-Z]\w*)\s*:[^{]*\bTableScopedSettingsStore\b"# + ) + let conforming = Set(sources.flatMap { Self.captures(conformance, in: $0) }) + + let unconformed = storeTypes.subtracting(conforming) #expect( unconformed.isEmpty, - "A store keyed by table scope must conform to TableScopedSettingsStore: \(unconformed.sorted())" + """ + A store keyed by a connection and a database conforms to TableScopedSettingsStore, or is listed in exemptStoreTypes with a reason: \(unconformed.sorted()) + """ ) let unregistered = storeTypes.filter { !registry.contains("\($0).shared") } #expect( @@ -119,6 +167,107 @@ struct TableScopedSettingsRegistryTests { ) } + /// An exemption naming a type that no longer exists reads as a decision someone made about code + /// that has since moved, which is worse than no exemption at all. + @Test("Every exemption still names a store the scan finds") + func everyExemptionNamesAStoreTheScanFinds() throws { + let app = try Self.repoRoot().appendingPathComponent("TablePro", isDirectory: true) + let sources = try Self.swiftSources(under: app) + let keyTypes = Set(sources.flatMap(Self.connectionScopedKeyTypes(in:))) + let detected = Set(sources.flatMap { Self.storesSpeaking(keyTypes, in: $0) }) + + let stale = Self.exemptStoreTypes.keys.filter { !detected.contains($0) } + #expect(stale.isEmpty, "An exemption names a type the scan no longer finds: \(stale.sorted())") + } + + /// A key carrying both a connection and a database is what makes its owner's data outlive a + /// rename or a drop of either. The name of the type is deliberately not consulted; the cap on + /// the property count is what keeps a whole model out of the result. + private static func connectionScopedKeyTypes(in text: String) -> [String] { + let lines = text.split(separator: "\n", omittingEmptySubsequences: false).map(String.init) + var found: [String] = [] + for (index, line) in lines.enumerated() { + guard line.contains("{"), let name = Self.declaredStructName(in: line) else { continue } + var depth = line.filter { $0 == "{" }.count - line.filter { $0 == "}" }.count + var properties: [String] = [] + var cursor = index + 1 + while cursor < lines.count, depth > 0 { + let body = lines[cursor] + if depth == 1, let property = Self.declaredPropertyName(in: body) { + properties.append(property) + } + depth += body.filter { $0 == "{" }.count - body.filter { $0 == "}" }.count + cursor += 1 + } + guard properties.count <= 6, + properties.contains("connectionId"), + properties.contains("database") || properties.contains("databaseName") else { continue } + found.append(name) + } + return found + } + + /// A type that takes one of those keys in a function signature and writes something somewhere. + /// + /// A write rather than a mention of a persistence type, because a view model that reads + /// `FileManager` to decide what to draw is not a store, and one of them takes a display target + /// shaped exactly like a storage key. + private static func storesSpeaking(_ keyTypes: Set, in text: String) -> [String] { + let writes = [ + ".set(", "setDataValue(", ".write(to:", "removeObject(forKey:", + "removeValues(withPrefix:", "sqlite3_step", + ] + guard writes.contains(where: text.contains) else { return [] } + let lines = text.split(separator: "\n", omittingEmptySubsequences: false).map(String.init) + let speaks = lines.contains { line in + guard line.contains("func ") else { return false } + return !Self.identifiers(in: line).isDisjoint(with: keyTypes) + } + guard speaks else { return [] } + return lines.compactMap(Self.declaredClassName(in:)) + } + + /// Whole identifiers only. Matching a key type as a substring made the nested `Key` of one + /// cache match every `forKey:` parameter in the app, and the scan reported twenty-two stores + /// that persist nothing table-scoped at all. + private static func identifiers(in line: String) -> Set { + Set( + line.split(whereSeparator: { !($0.isLetter || $0.isNumber || $0 == "_") }) + .map(String.init) + ) + } + + private static func declaredStructName(in line: String) -> String? { + Self.name(after: "struct ", in: line) + } + + private static func declaredClassName(in line: String) -> String? { + guard !line.hasPrefix(" "), !line.hasPrefix("\t") else { return nil } + return Self.name(after: "class ", in: line) + } + + private static func name(after keyword: String, in line: String) -> String? { + guard let range = line.range(of: keyword) else { return nil } + let name = String(line[range.upperBound...].prefix { $0.isLetter || $0.isNumber || $0 == "_" }) + guard let first = name.first, first.isUppercase else { return nil } + return name + } + + private static func declaredPropertyName(in line: String) -> String? { + let trimmed = line.trimmingCharacters(in: .whitespaces) + for keyword in ["let ", "var "] { + guard let range = trimmed.range(of: keyword) else { continue } + let prefix = trimmed[trimmed.startIndex..