From ad9d2f41a8bedf825484979c98cca9b5de6541dc Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 2 Sep 2026 11:11:00 +0700 Subject: [PATCH] fix(compare): fence a run on the question it answered rather than the script it replaced Claude-Session: https://claude.ai/code/session_01J6xU4Zx4DRJ5JaxMP437uT --- .../MySQLPluginDriver+BulkMetadata.swift | 10 +- .../MySQLPluginDriver+Routines.swift | 2 +- .../Core/Compare/CompareMetadataService.swift | 45 ++- .../Core/Compare/CompareRunner+Data.swift | 48 ++- TablePro/Core/Compare/CompareRunner.swift | 23 +- .../Compare/CompareSyncProfileStorage.swift | 16 - .../Compare/CompareSyncSession+Editing.swift | 4 +- .../Core/Compare/CompareSyncSession.swift | 100 ++++++- TablePro/Core/Database/DatabaseDriver.swift | 46 --- .../Core/Plugins/PluginDriverAdapter.swift | 16 - .../Views/Compare/CompareOptionsView.swift | 3 - .../Views/Compare/CompareProgressView.swift | 2 +- .../Compare/CompareSyncWindowController.swift | 19 +- .../Compare/CompareWindowContentView.swift | 2 +- .../Core/Compare/CompareRunClaimTests.swift | 275 ++++++++++++++++++ .../TableDefinitionRendererTests.swift | 45 +-- 16 files changed, 478 insertions(+), 178 deletions(-) create mode 100644 TableProTests/Core/Compare/CompareRunClaimTests.swift diff --git a/Plugins/MySQLDriverPlugin/MySQLPluginDriver+BulkMetadata.swift b/Plugins/MySQLDriverPlugin/MySQLPluginDriver+BulkMetadata.swift index 1bbe673d8..074030643 100644 --- a/Plugins/MySQLDriverPlugin/MySQLPluginDriver+BulkMetadata.swift +++ b/Plugins/MySQLDriverPlugin/MySQLPluginDriver+BulkMetadata.swift @@ -77,7 +77,7 @@ extension MySQLPluginDriver { /// `SHOW INDEX` returns text, so both are cast rather than read through a text accessor that /// would depend on how the driver rendered an integer cell. func fetchAllIndexes(schema: String?) async throws -> [String: [PluginIndexInfo]] { - let escapedDb = activeDatabaseName.replacingOccurrences(of: "'", with: "''") + let escapedDb = mysqlEscapeStringLiteral(routineSchema(schema)) let query = """ SELECT TABLE_NAME, INDEX_NAME, COLUMN_NAME, @@ -107,10 +107,12 @@ extension MySQLPluginDriver { var providesBulkTableMetadataFetch: Bool { true } - /// `SHOW TABLE STATUS` with no `WHERE` is the whole schema, in the same column order the - /// per-table read indexes into. + /// `SHOW TABLE STATUS FROM` with no `WHERE` is the whole schema, in the same column order the + /// per-table read indexes into. The database is named rather than inherited from the session, + /// so a caller asking about another one is answered about the one it asked about. func fetchAllTableMetadata(schema: String?) async throws -> [String: PluginTableMetadata] { - let result = try await execute(query: "SHOW TABLE STATUS") + let database = mysqlQuoteIdentifier(routineSchema(schema)) + let result = try await execute(query: "SHOW TABLE STATUS FROM \(database)") var metadata: [String: PluginTableMetadata] = [:] for row in result.rows { guard let name = row[safe: 0]?.asText else { continue } diff --git a/Plugins/MySQLDriverPlugin/MySQLPluginDriver+Routines.swift b/Plugins/MySQLDriverPlugin/MySQLPluginDriver+Routines.swift index 49005733a..597e6daa5 100644 --- a/Plugins/MySQLDriverPlugin/MySQLPluginDriver+Routines.swift +++ b/Plugins/MySQLDriverPlugin/MySQLPluginDriver+Routines.swift @@ -122,7 +122,7 @@ extension MySQLPluginDriver { } } - private func routineSchema(_ schema: String?) -> String { + func routineSchema(_ schema: String?) -> String { guard let schema, !schema.isEmpty else { return activeDatabaseName } return schema } diff --git a/TablePro/Core/Compare/CompareMetadataService.swift b/TablePro/Core/Compare/CompareMetadataService.swift index ef6c4b7fe..1fe39a2db 100644 --- a/TablePro/Core/Compare/CompareMetadataService.swift +++ b/TablePro/Core/Compare/CompareMetadataService.swift @@ -368,11 +368,34 @@ internal struct CompareMetadataService { /// What a whole-schema read produced, or nothing where the driver has no single-query form for /// it and the per-table read still has to run. + /// A whole-schema answer plus the folded spellings of its own keys, indexed once. Every table + /// that is missing from a sparse map asks for the folded fallback, so computing it per lookup + /// made the fallback quadratic in the table count. + private struct FoldedMap: Sendable { + let byName: [String: Value] + let byFoldedName: [String: Value] + + init(_ map: [String: Value]) { + byName = map + var folded: [String: Value] = [:] + var collided: Set = [] + for (key, value) in map { + let name = key.lowercased() + guard !collided.contains(name) else { continue } + if folded.updateValue(value, forKey: name) != nil { + folded.removeValue(forKey: name) + collided.insert(name) + } + } + byFoldedName = folded + } + } + private struct BulkMetadata: Sendable { - var columns: [String: [PluginColumnInfo]]? - var indexes: [String: [PluginIndexInfo]]? - var foreignKeys: [String: [PluginForeignKeyInfo]]? - var tableMetadata: [String: PluginTableMetadata]? + var columns: FoldedMap<[PluginColumnInfo]>? + var indexes: FoldedMap<[PluginIndexInfo]>? + var foreignKeys: FoldedMap<[PluginForeignKeyInfo]>? + var tableMetadata: FoldedMap? /// The folded spellings that name exactly one table in this scope. A folded fallback is /// only safe for those. @@ -396,16 +419,16 @@ internal struct CompareMetadataService { unambiguousFolded = Set(counts.filter { $0.value == 1 }.keys) if plugin.providesBulkColumnFetch { - columns = try? await plugin.fetchAllColumns(schema: schema) + columns = (try? await plugin.fetchAllColumns(schema: schema)).map(FoldedMap.init) } if profile.wantsIndexes, plugin.providesBulkIndexFetch { - indexes = try? await plugin.fetchAllIndexes(schema: schema) + indexes = (try? await plugin.fetchAllIndexes(schema: schema)).map(FoldedMap.init) } if profile.wantsForeignKeys, plugin.providesBulkForeignKeyFetch { - foreignKeys = try? await plugin.fetchAllForeignKeys(schema: schema) + foreignKeys = (try? await plugin.fetchAllForeignKeys(schema: schema)).map(FoldedMap.init) } if profile.wantsTableMetadata, plugin.providesBulkTableMetadataFetch { - tableMetadata = try? await plugin.fetchAllTableMetadata(schema: schema) + tableMetadata = (try? await plugin.fetchAllTableMetadata(schema: schema)).map(FoldedMap.init) } } @@ -427,12 +450,12 @@ internal struct CompareMetadataService { /// entry, and PostgreSQL allows `"Foo"` beside `"foo"`: a folded match there handed one /// table's indexes to the other, and a DROP INDEX generated from that names the index /// alone, so it would have dropped the real one. - func lookup(_ map: [String: Value]?, _ name: String) -> Value? { + func lookup(_ map: FoldedMap?, _ name: String) -> Value? { guard let map else { return nil } - if let exact = map[name] { return exact } + if let exact = map.byName[name] { return exact } let folded = name.lowercased() guard unambiguousFolded.contains(folded) else { return nil } - return map.first { $0.key.lowercased() == folded }?.value + return map.byFoldedName[folded] } } diff --git a/TablePro/Core/Compare/CompareRunner+Data.swift b/TablePro/Core/Compare/CompareRunner+Data.swift index 78a87ac31..16e39e531 100644 --- a/TablePro/Core/Compare/CompareRunner+Data.swift +++ b/TablePro/Core/Compare/CompareRunner+Data.swift @@ -37,18 +37,18 @@ internal extension CompareRunner { do { let context = try resolveContext() if let refusal = try await capabilityRefusal(context) { - guard session.owns(claim) else { return } + guard session.ownsAnswer(claim) else { return } session.errorMessage = refusal return } - let plans = try await buildPlans(context) + let read = try await buildPlans(context) /// The pair may have moved while this was reading. Publishing now would put one /// pair's tables, columns and snapshots behind another pair's Compare. - guard session.owns(claim) else { return } - session.adoptDataPlans(plans) + guard session.ownsAnswer(claim) else { return } + adopt(read) } catch is CancellationError { } catch { - guard session.owns(claim) else { return } + guard session.ownsAnswer(claim) else { return } session.errorMessage = error.localizedDescription } } @@ -64,9 +64,9 @@ internal extension CompareRunner { /// a stale key still merges the two row streams and still addresses the UPDATE and DELETE /// it generates, so one reviewed row's statement can reach every row sharing that value. /// `buildPlans` carries the user's ticks, keys and row exclusions onto the fresh list. - let built = try await buildPlans(context) - guard session.owns(claim) else { throw CancellationError() } - session.adoptDataPlans(built) + let read = try await buildPlans(context) + guard session.ownsAnswer(claim) else { throw CancellationError() } + adopt(read) var plans = session.dataPlans for index in plans.indices where plans[index].isEnabled && plans[index].isComparable { @@ -88,10 +88,9 @@ internal extension CompareRunner { } try Task.checkCancellation() - guard session.owns(claim) else { throw CancellationError() } - session.dataPlans = plans + guard session.ownsAnswer(claim) else { throw CancellationError() } + session.applyComparedSummaries(from: plans) session.hasLoadedDataPlans = true - session.selectedPlanId = plans.first { $0.isEnabled && $0.isComparable }?.id ?? plans.first?.id session.detailPane = .rows session.invalidateScript() @@ -154,12 +153,27 @@ internal extension CompareRunner { // MARK: - Plans - private func buildPlans(_ context: Context) async throws -> [DataComparePlan] { + /// Everything one read of the two sides produced, so the caller publishes all of it behind one + /// ownership check. Writing the snapshots here put one pair's foreign key graph and CREATE + /// TABLE source under another pair's Apply whenever the read outlived the pair it was started + /// for, which is the hazard the claim exists to close. + struct DataPlanRead { + let plans: [DataComparePlan] + let sourceSnapshots: [String: TableStructureSnapshot] + let unreadableTableCount: Int + } + + private func adopt(_ read: DataPlanRead) { + session.sourceSnapshots = read.sourceSnapshots + session.unreadableTableCount = read.unreadableTableCount + session.adoptDataPlans(read.plans) + } + + private func buildPlans(_ context: Context) async throws -> DataPlanRead { let (sourceReads, targetReads) = try await metadataService.bothSideTableReads( context: context, includeViews: false, profile: .data ) try Task.checkCancellation() - session.unreadableTableCount = (sourceReads + targetReads).filter { $0.failure != nil }.count /// Keyed on schema and name, not name alone: two schemas of one database can hold the same /// table, and pairing on the bare name took the shared column set from the wrong @@ -173,7 +187,7 @@ internal extension CompareRunner { session.dataPlans.map { ($0.id, $0) }, uniquingKeysWith: { first, _ in first } ) - session.sourceSnapshots = Dictionary( + let sourceSnapshots = Dictionary( sourceReads.compactMap { $0.snapshot }.map { ($0.qualifiedName, $0) }, uniquingKeysWith: { first, _ in first } ) @@ -205,6 +219,10 @@ internal extension CompareRunner { plan.unavailableReason = DataComparePlan.unavailableReason(for: plan) plans.append(plan) } - return plans.sorted { $0.id.localizedStandardCompare($1.id) == .orderedAscending } + return DataPlanRead( + plans: plans.sorted { $0.id.localizedStandardCompare($1.id) == .orderedAscending }, + sourceSnapshots: sourceSnapshots, + unreadableTableCount: (sourceReads + targetReads).filter { $0.failure != nil }.count + ) } } diff --git a/TablePro/Core/Compare/CompareRunner.swift b/TablePro/Core/Compare/CompareRunner.swift index c0c6e8b74..86d9b3d7d 100644 --- a/TablePro/Core/Compare/CompareRunner.swift +++ b/TablePro/Core/Compare/CompareRunner.swift @@ -42,7 +42,7 @@ internal struct CompareRunner { do { let context = try resolveContext() if let refusal = try await capabilityRefusal(context) { - guard session.owns(claim) else { return } + guard session.ownsAnswer(claim) else { return } session.errorMessage = refusal return } @@ -53,13 +53,11 @@ internal struct CompareRunner { case .data: try await runDataCompare(context, claim: claim) } - guard session.owns(claim) else { return } + guard session.ownsAnswer(claim) else { return } session.informationalMessage = session.crossEngineNotice } catch is CancellationError { - guard session.owns(claim) else { return } - session.informationalMessage = String(localized: "Comparison cancelled.") } catch { - guard session.owns(claim) else { return } + guard session.ownsAnswer(claim) else { return } session.errorMessage = error.localizedDescription } } @@ -90,7 +88,7 @@ internal struct CompareRunner { let claim = session.currentClaim return Task { [session] in - session.activity = .comparing + session.activity = .buildingScript defer { session.activity = .idle } do { let context = try resolveContext() @@ -113,9 +111,9 @@ internal struct CompareRunner { session.statements = built session.detailPane = .script } catch is CancellationError { - guard session.owns(claim) else { return } - session.informationalMessage = String(localized: "Script generation cancelled.") } catch { + /// The full claim, not the answer alone: a build that failed for a selection the + /// user has since changed would blame an object they had just excluded. guard session.owns(claim) else { return } session.errorMessage = error.localizedDescription } @@ -173,6 +171,7 @@ internal struct CompareRunner { /// armed left Apply enabled on a stale plan, one click from running the same /// CREATE/ALTER/DELETE a second time. session.markAppliedAndStale() + } catch is CancellationError { } catch { session.errorMessage = error.localizedDescription } @@ -264,17 +263,13 @@ internal struct CompareRunner { results += try await sourceDefinedResults(context, sourceReads: sourceReads, targetReads: targetReads) try Task.checkCancellation() - guard session.owns(claim) else { throw CancellationError() } + guard session.ownsAnswer(claim) else { throw CancellationError() } let report = CompareReport(results: results) session.sourceSnapshots = sourceByName session.targetSnapshots = targetByName session.report = report - session.actions = [:] - for result in report.comparable where session.pendingSelection.contains(result.id) { - session.actions[result.id] = result.suggestedAction - } - session.pendingSelection = [] + session.adoptActions(for: report) session.invalidateScript() session.selectedObjectId = session.visibleResults.first?.id session.lastAction = .compared(Date(), differences: report.differenceCount) diff --git a/TablePro/Core/Compare/CompareSyncProfileStorage.swift b/TablePro/Core/Compare/CompareSyncProfileStorage.swift index 376d871ca..12ab3219f 100644 --- a/TablePro/Core/Compare/CompareSyncProfileStorage.swift +++ b/TablePro/Core/Compare/CompareSyncProfileStorage.swift @@ -47,17 +47,6 @@ internal struct CompareSyncProfile: Codable, Hashable, Identifiable { self.selectedObjects = selectedObjects } - internal static func storageKey(source: DatabaseScope, target: DatabaseScope, mode: CompareSyncMode) -> String { - "\(key(for: source))|\(key(for: target))|\(mode.rawValue)" - } - - internal var storageKey: String { - Self.storageKey(source: source, target: target, mode: mode) - } - - private static func key(for scope: DatabaseScope) -> String { - "\(scope.connectionId.uuidString)/\(scope.database)/\(scope.schema ?? "")" - } } extension DatabaseScope: Codable { @@ -134,11 +123,6 @@ internal final class CompareSyncProfileStorage { } } - internal func profiles(source: DatabaseScope, target: DatabaseScope, mode: CompareSyncMode) -> [CompareSyncProfile] { - let key = CompareSyncProfile.storageKey(source: source, target: target, mode: mode) - return allProfiles().filter { $0.storageKey == key } - } - internal func save(_ profile: CompareSyncProfile) { var profiles = allProfiles() if let index = profiles.firstIndex(where: { $0.id == profile.id }) { diff --git a/TablePro/Core/Compare/CompareSyncSession+Editing.swift b/TablePro/Core/Compare/CompareSyncSession+Editing.swift index fb07d2a02..48321ec8a 100644 --- a/TablePro/Core/Compare/CompareSyncSession+Editing.swift +++ b/TablePro/Core/Compare/CompareSyncSession+Editing.swift @@ -21,7 +21,7 @@ internal extension CompareSyncSession { dataPlans[index].summary = nil dataPlans[index].excludedRowKeys = [] dataPlans[index].unavailableReason = DataComparePlan.unavailableReason(for: dataPlans[index]) - invalidateScript() + invalidateAnswer() } func toggleKeyColumn(_ column: String, for planId: String) { @@ -73,7 +73,7 @@ internal extension CompareSyncSession { for index in dataPlans.indices { dataPlans[index].summary = nil } - invalidateScript() + invalidateAnswer() } // MARK: - Row inclusion diff --git a/TablePro/Core/Compare/CompareSyncSession.swift b/TablePro/Core/Compare/CompareSyncSession.swift index 45ece35ca..6324d412f 100644 --- a/TablePro/Core/Compare/CompareSyncSession.swift +++ b/TablePro/Core/Compare/CompareSyncSession.swift @@ -19,6 +19,7 @@ internal enum CompareSyncActivity: Equatable { case idle case connecting case comparing + case buildingScript case applying } @@ -118,6 +119,15 @@ internal final class CompareSyncSession { /// open on them, because an ordinary INSERT or ALTER is not a hazard `runRefusalReason` catches. private(set) var scriptRevision = 0 + /// Which question the answers on screen were computed for. + /// + /// A comparison invalidates the script it just made stale, so a run that fenced its own report + /// on `scriptRevision` could never publish one: it had already advanced the revision itself. + /// Ticking a table or excluding a row is the other half of the same distinction. It changes + /// which statements come out of an answer that still stands, so it invalidates the script and + /// must not throw away a comparison that has been streaming rows for minutes. + private(set) var answerRevision = 0 + /// A setup problem, which outlives the comparison it interrupted. `errorMessage` is cleared by /// the next reset, and a reset is exactly what changing the setup does, so a message about the /// setup itself cannot live there: loading a profile whose connection is gone reported the @@ -190,7 +200,7 @@ internal final class CompareSyncSession { switch activity { case .applying: return String(format: String(localized: "Applying to %@…"), target?.qualifiedDescription ?? "") - case .comparing, .connecting: + case .comparing, .connecting, .buildingScript: return String(localized: "Comparing only. Nothing has been written.") case .idle: return idleBannerText @@ -425,6 +435,10 @@ internal final class CompareSyncSession { /// it. `hasWrittenToTarget` deliberately survives, because a write already happened and no /// later comparison makes that untrue. internal func resetComparison() { + /// The setup the work in flight was started for is the one being replaced, so it is stopped + /// here rather than left holding `runTask`. A preload taking that slot from an orphaned run + /// left Stop with nothing to cancel while the run went on reading. + cancelRunningWork() report = nil sourceSnapshots = [:] targetSnapshots = [:] @@ -439,7 +453,7 @@ internal final class CompareSyncSession { informationalMessage = nil lastAction = .none isStaleAfterApply = false - invalidateScript() + invalidateAnswer() setupGeneration &+= 1 /// Every path that resets a comparison is a path that changed the setup: an endpoint, the /// mode, or an option. So this is also where the setup is written down, and reopening the @@ -457,16 +471,24 @@ internal final class CompareSyncSession { /// whatever the setup has become, which is exactly the ownership the fence is meant to check. internal struct RunClaim: Sendable { internal let setup: Int + internal let answer: Int internal let script: Int internal let mode: CompareSyncMode } internal var currentClaim: RunClaim { - RunClaim(setup: setupGeneration, script: scriptRevision, mode: mode) + RunClaim(setup: setupGeneration, answer: answerRevision, script: scriptRevision, mode: mode) } + /// For statements, which describe one setup, one answer and one set of choices. internal func owns(_ claim: RunClaim) -> Bool { - isCurrent(setup: claim.setup, script: claim.script) + ownsAnswer(claim) && claim.script == scriptRevision + } + + /// For a comparison's own results and for anything it reports about itself. A run advances the + /// script revision as it publishes, so it cannot be fenced on the revision it started with. + internal func ownsAnswer(_ claim: RunClaim) -> Bool { + claim.setup == setupGeneration && claim.answer == answerRevision } /// The one place a table list is published, so the tables a saved comparison asked for are @@ -492,6 +514,39 @@ internal final class CompareSyncSession { selectedPlanId = adopted.first { $0.isEnabled && $0.isComparable }?.id ?? adopted.first?.id } + /// The one place a structure report's inclusions are published, so what the user ticked while + /// the comparison ran survives it. The Include controls stay live throughout, and a rebuild + /// that reset them to nothing discarded every choice made during the run. This is the same rule + /// the data side already follows, where a rebuilt plan carries the tick it had. + internal func adoptActions(for report: CompareReport) { + let carried = actions + actions = [:] + for result in report.comparable { + if let action = carried[result.id] { + actions[result.id] = action + } else if pendingSelection.contains(result.id) { + actions[result.id] = result.suggestedAction + } + } + pendingSelection = [] + } + + /// A comparison streams every row of both sides, so the plans it started from can be minutes + /// old by the time it has summaries for them, and the ticks, keys and row exclusions stay live + /// throughout. Writing the run's own array back put every one of those edits behind the list it + /// captured before the first row was read. Only the summary belongs to the run, so only the + /// summary is carried over, and only onto a plan still asking the question the run answered. + internal func applyComparedSummaries(from compared: [DataComparePlan]) { + let byId = Dictionary(compared.map { ($0.id, $0) }, uniquingKeysWith: { first, _ in first }) + for index in dataPlans.indices { + guard let run = byId[dataPlans[index].id], + run.keyColumns == dataPlans[index].keyColumns, + run.columns == dataPlans[index].columns else { continue } + dataPlans[index].summary = run.summary + dataPlans[index].unavailableReason = run.unavailableReason + } + } + /// After a run the report describes a target that has since changed, so it is stale rather than /// wrong: it stays on screen to be read, and every action that would write again is withdrawn /// until the user compares once more. @@ -510,18 +565,47 @@ internal final class CompareSyncSession { scriptRevision &+= 1 } - /// True while both the setup and the choices a piece of work was started for still stand. - internal func isCurrent(setup: Int, script: Int) -> Bool { - setup == setupGeneration && script == scriptRevision + /// For the edits that make a computed answer wrong rather than merely restating which parts of + /// it to apply: a new setup, a new key column, a new set of compared columns. + internal func invalidateAnswer() { + answerRevision &+= 1 + invalidateScript() } - /// Cancelling advances the script revision as well as asking the task to stop, because + /// Cancelling advances both revisions as well as asking the task to stop, because /// `Task.cancel()` is cooperative: a build already inside a driver call finishes and would /// otherwise publish over a comparison the user has stopped. + /// + /// It advances them without clearing what is on screen. Apply cancels the work in flight and + /// then reads the very statements it is about to run, so discarding the script here left every + /// confirmed Apply executing nothing and reporting success. internal func cancelRunningWork() { progress?.cancel() runTask?.cancel() scriptRevision &+= 1 + answerRevision &+= 1 + } + + /// The user's own Stop, which reports itself here rather than waiting for the task to notice. + /// `Task.cancel()` is cooperative and a run already inside a driver call may never observe it, + /// so a message published from the cancellation path is a message that may never arrive; and a + /// run that does observe it can no longer tell the user's Stop from being superseded by the + /// next run, because both reach it the same way. + internal func stopRunningWork() { + let stopped = activity + cancelRunningWork() + switch stopped { + case .idle: + return + case .applying: + informationalMessage = String( + localized: "Sync stopped. Statements that already ran stay applied." + ) + case .buildingScript: + informationalMessage = String(localized: "Script generation cancelled.") + case .comparing, .connecting: + informationalMessage = String(localized: "Comparison cancelled.") + } } internal var isBusy: Bool { diff --git a/TablePro/Core/Database/DatabaseDriver.swift b/TablePro/Core/Database/DatabaseDriver.swift index 48e9d87af..567feffeb 100644 --- a/TablePro/Core/Database/DatabaseDriver.swift +++ b/TablePro/Core/Database/DatabaseDriver.swift @@ -127,28 +127,6 @@ protocol DatabaseDriver: AnyObject, Sendable { /// per table, which is too expensive to run ahead of the user. var providesBulkForeignKeyFetch: Bool { get } - /// Whether `fetchAllColumns` is a single query that reports what per-table `fetchColumns` - /// reports. Both halves matter: a bulk query missing generated columns is not a substitute. - var providesBulkColumnFetch: Bool { get } - - /// Fetch indexes for every table in the current schema in bulk. - /// Default implementation falls back to per-table fetchIndexes. - func fetchAllIndexes(schema: String?) async throws -> [String: [IndexInfo]] - - /// Whether `fetchAllIndexes` is a single query. - var providesBulkIndexFetch: Bool { get } - - /// Fetch table metadata for every table in the current schema in bulk. - /// Default implementation falls back to per-table fetchTableMetadata. - func fetchAllTableMetadata(schema: String?) async throws -> [String: TableMetadata] - - /// Whether `fetchAllTableMetadata` is a single query. - var providesBulkTableMetadataFetch: Bool { get } - - /// Whether `fetchAllTriggers` lists a whole schema's triggers. Its default answers with nothing - /// rather than looping, so a caller has to know before it decides to ask per table. - var providesBulkTriggerFetch: Bool { get } - /// Fetch foreign keys for a specific set of tables. /// Default implementation calls fetchAllForeignKeys and filters, or falls back to per-table. func fetchForeignKeys(forTables tableNames: [String]) async throws -> [String: [ForeignKeyInfo]] @@ -436,30 +414,6 @@ extension DatabaseDriver { } var providesBulkForeignKeyFetch: Bool { false } - var providesBulkColumnFetch: Bool { false } - var providesBulkIndexFetch: Bool { false } - var providesBulkTableMetadataFetch: Bool { false } - var providesBulkTriggerFetch: Bool { false } - - func fetchAllIndexes(schema: String?) async throws -> [String: [IndexInfo]] { - let tables = try await fetchTables() - var result: [String: [IndexInfo]] = [:] - for table in tables { - let indexes = try await fetchIndexes(table: table.name) - if !indexes.isEmpty { result[table.name] = indexes } - } - return result - } - - func fetchAllTableMetadata(schema: String?) async throws -> [String: TableMetadata] { - let tables = try await fetchTables() - var result: [String: TableMetadata] = [:] - for table in tables { - guard let metadata = try? await fetchTableMetadata(tableName: table.name) else { continue } - result[table.name] = metadata - } - return result - } func fetchAllForeignKeys() async throws -> [String: [ForeignKeyInfo]] { let allTables = try await fetchTables() diff --git a/TablePro/Core/Plugins/PluginDriverAdapter.swift b/TablePro/Core/Plugins/PluginDriverAdapter.swift index 60ab0feb9..acb1a05d0 100644 --- a/TablePro/Core/Plugins/PluginDriverAdapter.swift +++ b/TablePro/Core/Plugins/PluginDriverAdapter.swift @@ -516,22 +516,6 @@ final class PluginDriverAdapter: DatabaseDriver, SchemaSwitchable, DatabaseRepor } var providesBulkForeignKeyFetch: Bool { pluginDriver.providesBulkForeignKeyFetch } - var providesBulkColumnFetch: Bool { pluginDriver.providesBulkColumnFetch } - var providesBulkIndexFetch: Bool { pluginDriver.providesBulkIndexFetch } - var providesBulkTableMetadataFetch: Bool { pluginDriver.providesBulkTableMetadataFetch } - var providesBulkTriggerFetch: Bool { pluginDriver.providesBulkTriggerFetch } - - func fetchAllIndexes(schema: String?) async throws -> [String: [IndexInfo]] { - let pluginResult = try await pluginDriver.fetchAllIndexes(schema: schema ?? pluginDriver.currentSchema) - return pluginResult.mapValues { $0.map(Self.mapPluginIndex) } - } - - func fetchAllTableMetadata(schema: String?) async throws -> [String: TableMetadata] { - let pluginResult = try await pluginDriver.fetchAllTableMetadata( - schema: schema ?? pluginDriver.currentSchema - ) - return pluginResult.mapValues(Self.mapPluginTableMetadata) - } func fetchAllForeignKeys() async throws -> [String: [ForeignKeyInfo]] { let pluginResult = try await pluginDriver.fetchAllForeignKeys(schema: pluginDriver.currentSchema) diff --git a/TablePro/Views/Compare/CompareOptionsView.swift b/TablePro/Views/Compare/CompareOptionsView.swift index 1406effa7..357dad37a 100644 --- a/TablePro/Views/Compare/CompareOptionsView.swift +++ b/TablePro/Views/Compare/CompareOptionsView.swift @@ -30,9 +30,6 @@ internal struct CompareOptionsView: View { .onAppear { savedProfiles = session.savedProfiles } - .onChange(of: session.mode) { - savedProfiles = session.savedProfiles - } .onChange(of: session.includedKinds) { session.resetComparison() } diff --git a/TablePro/Views/Compare/CompareProgressView.swift b/TablePro/Views/Compare/CompareProgressView.swift index 7bca5a05c..ae9d6b6cf 100644 --- a/TablePro/Views/Compare/CompareProgressView.swift +++ b/TablePro/Views/Compare/CompareProgressView.swift @@ -30,7 +30,7 @@ internal struct CompareProgressView: View { progressBar .frame(maxWidth: 200) Button("Cancel") { - session.cancelRunningWork() + session.stopRunningWork() } .controlSize(.small) .accessibilityIdentifier("compare.progress.cancel") diff --git a/TablePro/Views/Compare/CompareSyncWindowController.swift b/TablePro/Views/Compare/CompareSyncWindowController.swift index b3884990e..8ce64976f 100644 --- a/TablePro/Views/Compare/CompareSyncWindowController.swift +++ b/TablePro/Views/Compare/CompareSyncWindowController.swift @@ -159,9 +159,13 @@ internal final class CompareSyncWindowController: NSWindowController, private func insertSavedComparisonsItemOnce(into toolbar: NSToolbar) { let defaults = AppStorageEnvironment.shared.defaults guard !defaults.bool(forKey: Self.savedComparisonsInsertedKey) else { return } + if !toolbar.items.contains(where: { $0.itemIdentifier == .compareSaved }) { + toolbar.insertItem(withItemIdentifier: .compareSaved, at: 0) + } + /// Recorded once the item is in the toolbar, not before. Burning the flag ahead of the + /// insert left a window that had not placed the item with no second chance at it. + guard toolbar.items.contains(where: { $0.itemIdentifier == .compareSaved }) else { return } defaults.set(true, forKey: Self.savedComparisonsInsertedKey) - guard !toolbar.items.contains(where: { $0.itemIdentifier == .compareSaved }) else { return } - toolbar.insertItem(withItemIdentifier: .compareSaved, at: 0) } private static let savedComparisonsInsertedKey = "compareSyncToolbarHasSavedComparisonsItem" @@ -478,9 +482,12 @@ internal final class CompareSyncWindowController: NSWindowController, presentApplySheet() } + /// `swapEndpoints` resets on its own, so only the chrome half runs here. Calling the whole + /// funnel reset the session twice for one press, which advanced the setup generation twice and + /// wrote the remembered setup twice. @objc internal func swapEndpoints(_ sender: Any?) { session.swapEndpoints() - endpointsChanged() + adoptChangedEndpoints() } /// Every path that changes an endpoint funnels here, because the toolbar's Source and Target @@ -489,6 +496,10 @@ internal final class CompareSyncWindowController: NSWindowController, /// wrong database as the one about to be written to. private func endpointsChanged() { session.resetComparison() + adoptChangedEndpoints() + } + + private func adoptChangedEndpoints() { session.clearSetupErrorIfResolved() refreshEndpointChrome() } @@ -521,7 +532,7 @@ internal final class CompareSyncWindowController: NSWindowController, } @objc internal func stopComparison(_ sender: Any?) { - session.cancelRunningWork() + session.stopRunningWork() } /// Edit > Find > Find… already owns Command F and routes by nil target, so the Compare window diff --git a/TablePro/Views/Compare/CompareWindowContentView.swift b/TablePro/Views/Compare/CompareWindowContentView.swift index a9cc0f37e..927b19b83 100644 --- a/TablePro/Views/Compare/CompareWindowContentView.swift +++ b/TablePro/Views/Compare/CompareWindowContentView.swift @@ -99,7 +99,7 @@ internal struct CompareStatusStrip: View { private var symbol: String { switch session.activity { case .applying: return "exclamationmark.triangle.fill" - case .comparing, .connecting: return "magnifyingglass" + case .comparing, .connecting, .buildingScript: return "magnifyingglass" case .idle: return session.hasWrittenToTarget ? "checkmark.circle" : "eye" } } diff --git a/TableProTests/Core/Compare/CompareRunClaimTests.swift b/TableProTests/Core/Compare/CompareRunClaimTests.swift new file mode 100644 index 000000000..a4ad46d86 --- /dev/null +++ b/TableProTests/Core/Compare/CompareRunClaimTests.swift @@ -0,0 +1,275 @@ +// +// CompareRunClaimTests.swift +// TableProTests +// +// What a comparison in flight still owns, and what it has to give up. +// +// One revision counter used to answer both "are these statements still the +// user's choices" and "is this answer still the question on screen". A +// comparison advances the first itself as it publishes, so it could never pass +// its own fence: the cross-engine warning and every cancellation message were +// unreachable, and ticking one more table part way through a run threw away +// every summary the run had already computed. +// + +@testable import TablePro +import XCTest + +@MainActor +final class CompareRunClaimTests: XCTestCase { + private let sourceConnection = UUID() + private let targetConnection = UUID() + private var storage: CompareSyncProfileStorage! + private var defaults: UserDefaults! + private let suiteName = "CompareRunClaimTests" + + override func setUp() { + super.setUp() + UserDefaults.standard.removePersistentDomain(forName: suiteName) + defaults = UserDefaults(suiteName: suiteName) + storage = CompareSyncProfileStorage(defaults: defaults) + } + + override func tearDown() { + UserDefaults.standard.removePersistentDomain(forName: suiteName) + storage = nil + defaults = nil + super.tearDown() + } + + // MARK: - The fence + + func testAComparisonStillOwnsItsAnswerAfterInvalidatingTheScriptItReplaced() { + let session = makeSession() + let claim = session.currentClaim + + session.invalidateScript() + + XCTAssertTrue(session.ownsAnswer(claim), "publishing a report is what makes the old script stale") + XCTAssertFalse(session.owns(claim), "the statements it replaced are not the ones it may publish") + } + + func testTickingATableLeavesARunningComparisonItsAnswer() { + let session = makeSession() + session.adoptDataPlans([plan(table: "orders"), plan(table: "users")]) + let claim = session.currentClaim + + session.setPlanEnabled(true, for: "orders") + + XCTAssertTrue(session.ownsAnswer(claim), "which tables to apply is not which question was asked") + } + + func testChangingAKeyColumnTakesTheAnswerFromARunningComparison() { + let session = makeSession() + session.adoptDataPlans([plan(table: "orders")]) + let claim = session.currentClaim + + session.setKeyColumns(["email"], for: "orders") + + XCTAssertFalse(session.ownsAnswer(claim), "a new key is a different question") + } + + func testUnpickingAComparedColumnTakesTheAnswerFromARunningComparison() { + let session = makeSession() + let claim = session.currentClaim + + session.clearDataSummaries() + + XCTAssertFalse(session.ownsAnswer(claim)) + } + + func testChangingTheSetupTakesTheAnswerFromARunningComparison() { + let session = makeSession() + let claim = session.currentClaim + + session.resetComparison() + + XCTAssertFalse(session.ownsAnswer(claim)) + } + + // MARK: - Summaries + + func testAComparisonCarriesItsSummariesOntoTheTicksMadeWhileItRan() { + let session = makeSession() + session.adoptDataPlans([plan(table: "orders"), plan(table: "users")]) + var compared = plan(table: "orders") + compared.summary = summary(insertCount: 3) + + session.setPlanEnabled(true, for: "users") + session.applyComparedSummaries(from: [compared]) + + XCTAssertEqual(session.dataPlans.first { $0.id == "orders" }?.summary?.insertCount, 3) + XCTAssertEqual( + session.dataPlans.first { $0.id == "users" }?.isEnabled, true, + "a tick made during the run is the user's, not the run's to overwrite" + ) + } + + func testASummaryIsNotCarriedOntoAPlanWhoseKeyChangedUnderIt() { + let session = makeSession() + session.adoptDataPlans([plan(table: "orders", keyColumns: ["id"])]) + var compared = plan(table: "orders", keyColumns: ["id"]) + compared.summary = summary(insertCount: 3) + + session.setKeyColumns(["email"], for: "orders") + session.applyComparedSummaries(from: [compared]) + + XCTAssertNil( + session.dataPlans.first?.summary, + "the run answered for a key the plan no longer uses" + ) + } + + func testASummaryIsNotCarriedOntoATableTheRebuiltListNoLongerHolds() { + let session = makeSession() + session.adoptDataPlans([plan(table: "orders")]) + var compared = plan(table: "dropped") + compared.summary = summary(insertCount: 1) + + session.applyComparedSummaries(from: [compared]) + + XCTAssertEqual(session.dataPlans.map(\.id), ["orders"]) + XCTAssertNil(session.dataPlans.first?.summary) + } + + // MARK: - What a cancel may not take with it + + /// Apply cancels the work in flight and then reads the statements it is about to run, so a + /// cancel that discarded the script left every confirmed Apply executing nothing against the + /// target and reporting that it had succeeded. + func testCancellingRunningWorkLeavesTheScriptItIsAboutToApply() { + let session = makeSession() + session.statements = [ + SyncStatement(sql: "DROP TABLE orders", objectName: "orders", summary: "Drop orders") + ] + + session.cancelRunningWork() + + XCTAssertEqual(session.statements.count, 1) + } + + func testCancellingRunningWorkStillTakesOwnershipFromTheRunItStopped() { + let session = makeSession() + let claim = session.currentClaim + + session.cancelRunningWork() + + XCTAssertFalse(session.ownsAnswer(claim)) + XCTAssertFalse(session.owns(claim)) + } + + // MARK: - Structure inclusions + + func testARecompareKeepsWhatTheUserIncludedWhileItRan() { + let session = makeSession() + let report = CompareReport(results: [result(name: "orders"), result(name: "users")]) + session.report = report + session.setIncluded(true, for: result(name: "orders")) + + session.adoptActions(for: report) + + XCTAssertNotEqual(session.action(for: result(name: "orders")), .skip) + } + + func testARecompareDropsAnInclusionForAnObjectItNoLongerFinds() { + let session = makeSession() + session.report = CompareReport(results: [result(name: "dropped")]) + session.setIncluded(true, for: result(name: "dropped")) + + session.adoptActions(for: CompareReport(results: [result(name: "orders")])) + + XCTAssertEqual(session.actions.count, 0) + } + + // MARK: - Stopping + + func testStoppingAComparisonSaysSoWithoutWaitingForTheTaskToNotice() { + let session = makeSession() + session.activity = .comparing + + session.stopRunningWork() + + XCTAssertEqual(session.informationalMessage, "Comparison cancelled.") + } + + func testStoppingAScriptBuildNamesTheScript() { + let session = makeSession() + session.activity = .buildingScript + + session.stopRunningWork() + + XCTAssertEqual(session.informationalMessage, "Script generation cancelled.") + } + + /// A sync writes as it goes, so stopping one is not the same as stopping a read: the statements + /// that already ran stay applied, and the message has to say so. + func testStoppingASyncSaysWhatStaysApplied() { + let session = makeSession() + session.activity = .applying + + session.stopRunningWork() + + XCTAssertEqual(session.informationalMessage, "Sync stopped. Statements that already ran stay applied.") + } + + func testStoppingWithNothingRunningSaysNothing() { + let session = makeSession() + + session.stopRunningWork() + + XCTAssertNil(session.informationalMessage) + } + + func testResettingTheSetupCancelsTheWorkTheOldSetupStarted() { + let session = makeSession() + let task = Task { try? await Task.sleep(for: .seconds(30)) } + session.runTask = task + + session.resetComparison() + + XCTAssertTrue(task.isCancelled, "a preload taking the slot would leave Stop nothing to cancel") + } + + // MARK: - Helpers + + private func makeSession() -> CompareSyncSession { + let connections = [ + connection(id: sourceConnection, name: "prod"), + connection(id: targetConnection, name: "staging") + ] + return CompareSyncSession(profileStorage: storage, connectionsProvider: { connections }) + } + + private func connection(id: UUID, name: String) -> DatabaseConnection { + DatabaseConnection(id: id, name: name, database: name, type: .postgresql) + } + + private func plan(table: String, keyColumns: [String] = ["id"]) -> DataComparePlan { + DataComparePlan( + table: table, + schema: nil, + columns: ["id", "email"], + keyColumns: keyColumns, + isEnabled: false + ) + } + + private func result(name: String) -> CompareObjectResult { + CompareObjectResult( + identity: CompareObjectIdentity(kind: .table, schema: nil, name: name), + status: .onlyInSource + ) + } + + private func summary(insertCount: Int) -> DataDiffSummary { + DataDiffSummary( + insertCount: insertCount, + updateCount: 0, + deleteCount: 0, + identicalCount: 0, + skippedNullKeyCount: 0, + entries: [], + truncatedEntries: false + ) + } +} diff --git a/TableProTests/Core/Compare/TableDefinitionRendererTests.swift b/TableProTests/Core/Compare/TableDefinitionRendererTests.swift index 0264fa13c..66a1f394a 100644 --- a/TableProTests/Core/Compare/TableDefinitionRendererTests.swift +++ b/TableProTests/Core/Compare/TableDefinitionRendererTests.swift @@ -139,50 +139,23 @@ final class CompareSyncProfileStorageTests: XCTestCase { let target = scope(UUID()) storage.save(profile(name: "nightly", source: source, target: target)) - let loaded = storage.profiles(source: source, target: target, mode: .structure) + let loaded = storage.allProfiles() XCTAssertEqual(loaded.count, 1) XCTAssertEqual(loaded[0].name, "nightly") XCTAssertEqual(loaded[0].selectedObjects, ["users"]) } - func testProfilesAreScopedToSourceTargetAndMode() { + /// Every saved comparison is offered, whatever pair the window is on, because loading one is + /// what sets the pair. A list filtered by the pair already on screen could only be reached by + /// doing the work the saved comparison exists to replace. + func testEveryProfileIsListedWhateverTheCurrentPair() { let source = scope(UUID()) let target = scope(UUID()) storage.save(profile(name: "structure", source: source, target: target, mode: .structure)) - storage.save(profile(name: "data", source: source, target: target, mode: .data)) + storage.save(profile(name: "data", source: scope(UUID()), target: scope(UUID()), mode: .data)) - XCTAssertEqual(storage.profiles(source: source, target: target, mode: .structure).map(\.name), ["structure"]) - XCTAssertEqual(storage.profiles(source: source, target: target, mode: .data).map(\.name), ["data"]) - XCTAssertTrue(storage.profiles(source: target, target: source, mode: .structure).isEmpty) - } - - /// Keying on the connection pair alone could not tell two databases on one server apart, so a - /// comparison saved against staging came back for production. - func testTwoDatabasesOnOneConnectionKeepSeparateProfiles() { - let connectionId = UUID() - let staging = scope(connectionId, database: "app_staging") - let production = scope(connectionId, database: "app_prod") - let target = scope(UUID()) - storage.save(profile(name: "staging", source: staging, target: target)) - storage.save(profile(name: "production", source: production, target: target)) - - XCTAssertEqual(storage.profiles(source: staging, target: target, mode: .structure).map(\.name), ["staging"]) - XCTAssertEqual( - storage.profiles(source: production, target: target, mode: .structure).map(\.name), ["production"] - ) - } - - func testTwoSchemasInOneDatabaseKeepSeparateProfiles() { - let connectionId = UUID() - let publicSchema = scope(connectionId, schema: "public") - let salesSchema = scope(connectionId, schema: "sales") - let target = scope(UUID()) - storage.save(profile(name: "public", source: publicSchema, target: target)) - storage.save(profile(name: "sales", source: salesSchema, target: target)) - - XCTAssertEqual(storage.profiles(source: publicSchema, target: target, mode: .structure).map(\.name), ["public"]) - XCTAssertEqual(storage.profiles(source: salesSchema, target: target, mode: .structure).map(\.name), ["sales"]) + XCTAssertEqual(storage.allProfiles().map(\.name).sorted(), ["data", "structure"]) } func testSavingSameProfileIdUpdatesRatherThanDuplicates() { @@ -193,7 +166,7 @@ final class CompareSyncProfileStorageTests: XCTestCase { existing.name = "renamed" storage.save(existing) - let loaded = storage.profiles(source: source, target: target, mode: .structure) + let loaded = storage.allProfiles() XCTAssertEqual(loaded.count, 1) XCTAssertEqual(loaded[0].name, "renamed") @@ -209,6 +182,6 @@ final class CompareSyncProfileStorageTests: XCTestCase { storage.delete(drop) - XCTAssertEqual(storage.profiles(source: source, target: target, mode: .structure).map(\.name), ["keep"]) + XCTAssertEqual(storage.allProfiles().map(\.name), ["keep"]) } }