From ef210ceeca8dcc8b41ee4c48fa1d93528962cab5 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 19 Aug 2026 13:26:48 +0700 Subject: [PATCH 1/2] fix(sidebar): run table maintenance against the database on screen --- CHANGELOG.md | 1 + ...ainContentCoordinator+SidebarActions.swift | 23 ++- .../Database/ScopedDriverPinningTests.swift | 158 ++++++++++++++++++ 3 files changed, 181 insertions(+), 1 deletion(-) create mode 100644 TableProTests/Core/Database/ScopedDriverPinningTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 99992142f..e95a47af9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Table maintenance from the object browser (OPTIMIZE, ANALYZE, CHECK, REPAIR, and VACUUM on PostgreSQL) now runs against the database you are looking at. It ran on whichever database the connection happened to be on, which a tab from another database moves for the length of its query, so the command could maintain the wrong copy of a table and still report success. - The inspector button stays at the right end of the toolbar when the inspector is open. It used to slide left and sit against the inspector's inner edge, so the button moved as soon as you used it. - The Tables and Favorites switch in the toolbar keeps following the sidebar after you open Customize Toolbar. It stopped responding until the window was closed and reopened. - Refresh, New Tab, Open Quickly, Export, Database, Results and Dashboard are dimmed in the toolbar's overflow menu when they cannot run. On a narrow window they looked available and did nothing. diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift index d4c222e1d..5c8e36910 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift @@ -214,11 +214,22 @@ extension MainContentCoordinator { activeSheet = .maintenance(operation: operation, tableName: tableName) } + /// Runs against the database the object browser is listing, not against whatever the session + /// driver was last pointed at. + /// + /// A maintenance statement names its table and nothing else, so where it lands is decided + /// entirely by the connection's current database. Executing on the session driver directly left + /// that to chance: a cross-database tab pins the shared handle to its own database for the + /// length of its query and deliberately writes no session state back, so `OPTIMIZE TABLE + /// role_ability` could optimize the copy in another database while the sheet reported success. + /// Every other statement the user owns takes a scoped lease; this one now does too, which also + /// puts it behind the same gate rather than interleaving with a tab's work on one handle. func executeMaintenance(operation: String, tableName: String, options: [String: String]) { guard let driver = DatabaseManager.shared.driver(for: connectionId) else { return } guard let statements = driver.maintenanceStatements( operation: operation, table: tableName, options: options ) else { return } + guard let scope = browseScope else { return } Task { [weak self] in guard let self else { return } @@ -245,8 +256,18 @@ extension MainContentCoordinator { } do { var lastResult: QueryResult? + let route = DatabaseManager.shared.executionRoute(for: scope) for sql in statements { - lastResult = try await driver.execute(query: sql) + /// `.protectedWrite`: a half-applied OPTIMIZE or REPAIR cannot be undone by + /// retrying, so the lease is registered to mark the connection busy and is never + /// reachable by Stop. + lastResult = try await DatabaseManager.shared.withScopedDriver( + scope: scope, + route: route, + cancellation: .protectedWrite + ) { scopedDriver in + try await scopedDriver.execute(query: sql) + } } await AlertHelper.showInfoSheet( title: String(format: String(localized: "%@ completed"), operation), diff --git a/TableProTests/Core/Database/ScopedDriverPinningTests.swift b/TableProTests/Core/Database/ScopedDriverPinningTests.swift new file mode 100644 index 000000000..7af0ee183 --- /dev/null +++ b/TableProTests/Core/Database/ScopedDriverPinningTests.swift @@ -0,0 +1,158 @@ +// +// ScopedDriverPinningTests.swift +// TableProTests +// +// What a scoped lease guarantees, and what running on the session driver directly does not. +// A maintenance statement names its table and nothing else, so the connection's current database +// decides where it lands, and nothing tracks where that is. +// + +import Foundation +@testable import TablePro +import TableProPluginKit +import Testing + +@Suite("Scoped driver pinning", .serialized) +@MainActor +struct ScopedDriverPinningTests { + private static func seed( + browseDatabase: String + ) -> (connection: DatabaseConnection, recorder: CallRecordingPluginDriver) { + let connection = TestFixtures.makeConnection(database: browseDatabase, type: .mysql) + let recorder = CallRecordingPluginDriver() + var session = ConnectionSession( + connection: connection, + driver: PluginDriverAdapter(connection: connection, pluginDriver: recorder) + ) + session.browseDatabase = browseDatabase + session.status = .connected + DatabaseManager.shared.injectSession(session, for: connection.id) + return (connection, recorder) + } + + @Test("A scoped statement moves the connection onto its own database before it runs") + func pinsBeforeRunningTheStatement() async throws { + let (connection, recorder) = Self.seed(browseDatabase: "app") + defer { DatabaseManager.shared.removeSession(for: connection.id) } + + let scope = DatabaseScope(connectionId: connection.id, database: "logs", schema: nil) + try await DatabaseManager.shared.withScopedDriver( + scope: scope, + route: DatabaseManager.shared.executionRoute(for: scope), + cancellation: .protectedWrite + ) { driver in + _ = try await driver.execute(query: "OPTIMIZE TABLE `role_ability`") + } + + #expect(recorder.calls == ["switch:logs", "execute:OPTIMIZE TABLE `role_ability`"]) + } + + /// The half that made an unscoped statement dangerous. A lease moves the shared connection and + /// deliberately writes no session state back, so the browse cursor still says `app` while the + /// handle sits on `logs`. Anything that then runs on the session driver without a scope of its + /// own inherits `logs` and reports success against the wrong database. + @Test("A lease leaves the shared connection on its database and the browse cursor untouched") + func leavesTheConnectionWhereTheLeasePutIt() async throws { + let (connection, recorder) = Self.seed(browseDatabase: "app") + defer { DatabaseManager.shared.removeSession(for: connection.id) } + + let scope = DatabaseScope(connectionId: connection.id, database: "logs", schema: nil) + try await DatabaseManager.shared.withScopedDriver( + scope: scope, + route: DatabaseManager.shared.executionRoute(for: scope), + cancellation: .protectedWrite + ) { driver in + _ = try await driver.execute(query: "SELECT 1") + } + + #expect(recorder.currentDatabase == "logs") + #expect(DatabaseManager.shared.session(for: connection.id)?.resolvedBrowseDatabase == "app") + } + + @Test("A statement scoped to the browsed database is pinned back onto it") + func pinsBackOntoTheBrowsedDatabase() async throws { + let (connection, recorder) = Self.seed(browseDatabase: "app") + defer { DatabaseManager.shared.removeSession(for: connection.id) } + + let foreign = DatabaseScope(connectionId: connection.id, database: "logs", schema: nil) + try await DatabaseManager.shared.withScopedDriver( + scope: foreign, + route: DatabaseManager.shared.executionRoute(for: foreign), + cancellation: .cancellableRead + ) { driver in + _ = try await driver.execute(query: "SELECT 1") + } + + let browsed = try #require(DatabaseManager.shared.browseScope(for: connection.id)) + recorder.reset() + try await DatabaseManager.shared.withScopedDriver( + scope: browsed, + route: DatabaseManager.shared.executionRoute(for: browsed), + cancellation: .protectedWrite + ) { driver in + _ = try await driver.execute(query: "OPTIMIZE TABLE `role_ability`") + } + + #expect(recorder.calls == ["switch:app", "execute:OPTIMIZE TABLE `role_ability`"]) + #expect(recorder.currentDatabase == "app") + } +} + +/// Records the order of the two calls that decide where a statement lands. +private final class CallRecordingPluginDriver: PluginDatabaseDriver, @unchecked Sendable { + private let lock = NSLock() + private var recorded: [String] = [] + private var database: String? + + var calls: [String] { + lock.lock() + defer { lock.unlock() } + return recorded + } + + var currentDatabase: String? { + lock.lock() + defer { lock.unlock() } + return database + } + + func reset() { + lock.lock() + recorded = [] + lock.unlock() + } + + private func record(_ call: String) { + lock.lock() + recorded.append(call) + lock.unlock() + } + + func switchDatabase(to database: String) async throws { + lock.lock() + self.database = database + lock.unlock() + record("switch:\(database)") + } + + func execute(query: String) async throws -> PluginQueryResult { + record("execute:\(query)") + return PluginQueryResult(columns: [], columnTypeNames: [], rows: [], rowsAffected: 0, executionTime: 0) + } + + func connect() async throws {} + func disconnect() {} + func fetchTables(schema: String?) async throws -> [PluginTableInfo] { [] } + func fetchColumns(table: String, schema: String?) async throws -> [PluginColumnInfo] { [] } + func fetchIndexes(table: String, schema: String?) async throws -> [PluginIndexInfo] { [] } + func fetchForeignKeys(table: String, schema: String?) async throws -> [PluginForeignKeyInfo] { [] } + func fetchTableDDL(table: String, schema: String?) async throws -> String { "" } + func fetchViewDefinition(view: String, schema: String?) async throws -> String { "" } + func fetchTableMetadata(table: String, schema: String?) async throws -> PluginTableMetadata { + PluginTableMetadata(tableName: table) + } + func fetchDatabases() async throws -> [String] { [] } + func fetchDatabaseMetadata(_ database: String) async throws -> PluginDatabaseMetadata { + PluginDatabaseMetadata(name: database) + } +} From 645bce0de7ae0519bb733d35bab438bf9cb1441a Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 19 Aug 2026 13:49:21 +0700 Subject: [PATCH 2/2] fix(sidebar): carry the clicked object's database into the maintenance command --- CHANGELOG.md | 2 +- ...ainContentCoordinator+SidebarActions.swift | 29 +++++++-- ...ontentCommandActions+DatabaseObjects.swift | 7 ++- .../Views/Main/MainContentCoordinator.swift | 9 ++- TablePro/Views/Main/MainContentView.swift | 12 +++- ...abaseTreeOutlineCoordinator+Commands.swift | 7 ++- .../Main/MaintenanceSheetIdentityTests.swift | 63 +++++++++++++++++++ 7 files changed, 116 insertions(+), 13 deletions(-) create mode 100644 TableProTests/Views/Main/MaintenanceSheetIdentityTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index e95a47af9..e856f6a8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Table maintenance from the object browser (OPTIMIZE, ANALYZE, CHECK, REPAIR, and VACUUM on PostgreSQL) now runs against the database you are looking at. It ran on whichever database the connection happened to be on, which a tab from another database moves for the length of its query, so the command could maintain the wrong copy of a table and still report success. +- Table maintenance from the object browser (OPTIMIZE, ANALYZE, CHECK, REPAIR, and VACUUM on PostgreSQL) now runs against the database the table you picked lives in. It ran on whichever database the connection happened to be on, which a tab from another database moves for the length of its query, so the command could maintain the same-named table in the wrong database and still report success. - The inspector button stays at the right end of the toolbar when the inspector is open. It used to slide left and sit against the inspector's inner edge, so the button moved as soon as you used it. - The Tables and Favorites switch in the toolbar keeps following the sidebar after you open Customize Toolbar. It stopped responding until the window was closed and reopened. - Refresh, New Tab, Open Quickly, Export, Database, Results and Dashboard are dimmed in the toolbar's overflow menu when they cannot run. On a narrow window they looked available and did nothing. diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift index 5c8e36910..effc4c36e 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift @@ -210,12 +210,18 @@ extension MainContentCoordinator { return driver.supportedMaintenanceOperations() ?? [] } - func showMaintenanceSheet(operation: String, tableName: String) { - activeSheet = .maintenance(operation: operation, tableName: tableName) + func showMaintenanceSheet( + operation: String, + tableName: String, + database: String? = nil, + schema: String? = nil + ) { + activeSheet = .maintenance( + operation: operation, tableName: tableName, database: database, schema: schema + ) } - /// Runs against the database the object browser is listing, not against whatever the session - /// driver was last pointed at. + /// Runs against the database the object it names lives in, on a scoped lease. /// /// A maintenance statement names its table and nothing else, so where it lands is decided /// entirely by the connection's current database. Executing on the session driver directly left @@ -224,12 +230,23 @@ extension MainContentCoordinator { /// role_ability` could optimize the copy in another database while the sheet reported success. /// Every other statement the user owns takes a scoped lease; this one now does too, which also /// puts it behind the same gate rather than interleaving with a tab's work on one handle. - func executeMaintenance(operation: String, tableName: String, options: [String: String]) { + func executeMaintenance( + operation: String, + tableName: String, + options: [String: String], + database: String? = nil, + schema: String? = nil + ) { guard let driver = DatabaseManager.shared.driver(for: connectionId) else { return } guard let statements = driver.maintenanceStatements( operation: operation, table: tableName, options: options ) else { return } - guard let scope = browseScope else { return } + /// The object the user picked names its own database, and only a command that names none + /// falls back to where the browser is pointing. `resolvedScope` is what decides that, so a + /// schema is never carried across a database boundary. + guard let scope = services.databaseManager.resolvedScope( + database: database, schema: schema, for: connectionId + ) ?? browseScope else { return } Task { [weak self] in guard let self else { return } diff --git a/TablePro/Views/Main/MainContentCommandActions+DatabaseObjects.swift b/TablePro/Views/Main/MainContentCommandActions+DatabaseObjects.swift index ef9d21d69..fbe5fb8a5 100644 --- a/TablePro/Views/Main/MainContentCommandActions+DatabaseObjects.swift +++ b/TablePro/Views/Main/MainContentCommandActions+DatabaseObjects.swift @@ -32,9 +32,14 @@ extension MainContentCommandActions { return coordinator?.supportedMaintenanceOperations() ?? [] } + /// The menu acts on the object browser's selection, and `TableInfo` carries a schema but no + /// database, so this names only the schema and the command falls back to the database being + /// browsed. The sidebar's own contextual menu carries the clicked row's database and does not. func runMaintenanceOperation(_ operation: String) { guard let object = selectedObject else { return } - coordinator?.showMaintenanceSheet(operation: operation, tableName: object.name) + coordinator?.showMaintenanceSheet( + operation: operation, tableName: object.name, schema: object.schema + ) } var canCreateDatabase: Bool { diff --git a/TablePro/Views/Main/MainContentCoordinator.swift b/TablePro/Views/Main/MainContentCoordinator.swift index e52d17696..36db7f3dc 100644 --- a/TablePro/Views/Main/MainContentCoordinator.swift +++ b/TablePro/Views/Main/MainContentCoordinator.swift @@ -52,7 +52,11 @@ enum ActiveSheet: Identifiable { case exportQueryResults case backupDatabase case restoreDatabase(fileURL: URL) - case maintenance(operation: String, tableName: String) + /// The object's own database and schema travel with the request. A maintenance statement names + /// its table and nothing else, so acting on wherever the object browser happens to point + /// maintains the same-named table in another database whenever the two have drifted apart. + /// This is the rule the sidebar's other destructive commands already keep by carrying their ref. + case maintenance(operation: String, tableName: String, database: String?, schema: String?) case createDatabase var id: String { @@ -64,7 +68,8 @@ enum ActiveSheet: Identifiable { case .exportQueryResults: "exportQueryResults" case .backupDatabase: "backupDatabase" case .restoreDatabase(let fileURL): "restoreDatabase-\(fileURL.path)" - case .maintenance(let operation, let tableName): "maintenance-\(operation)-\(tableName)" + case .maintenance(let operation, let tableName, let database, let schema): + "maintenance-\(operation)-\(database ?? "")-\(schema ?? "")-\(tableName)" case .createDatabase: "createDatabase" } } diff --git a/TablePro/Views/Main/MainContentView.swift b/TablePro/Views/Main/MainContentView.swift index ecbd86a61..437a180b6 100644 --- a/TablePro/Views/Main/MainContentView.swift +++ b/TablePro/Views/Main/MainContentView.swift @@ -266,12 +266,20 @@ struct MainContentView: View { ?? connection.database, sourceURL: fileURL ) - case .maintenance(let operation, let tableName): + case .maintenance(let operation, let tableName, let database, let schema): MaintenanceSheet( operation: operation, tableName: tableName, databaseType: connection.type, - onExecute: coordinator.executeMaintenance + onExecute: { operation, tableName, options in + coordinator.executeMaintenance( + operation: operation, + tableName: tableName, + options: options, + database: database, + schema: schema + ) + } ) case .sqlPreview: SQLReviewSheet( diff --git a/TablePro/Views/Sidebar/DatabaseTreeOutlineCoordinator+Commands.swift b/TablePro/Views/Sidebar/DatabaseTreeOutlineCoordinator+Commands.swift index f41eed3e7..4aece5323 100644 --- a/TablePro/Views/Sidebar/DatabaseTreeOutlineCoordinator+Commands.swift +++ b/TablePro/Views/Sidebar/DatabaseTreeOutlineCoordinator+Commands.swift @@ -49,7 +49,12 @@ extension DatabaseTreeOutlineCoordinator { } case .maintenance(let operation, let tableName, let ref): activateThen(ref) { [weak self] in - self?.mainCoordinator?.showMaintenanceSheet(operation: operation, tableName: tableName) + self?.mainCoordinator?.showMaintenanceSheet( + operation: operation, + tableName: tableName, + database: ref.database, + schema: ref.schema + ) } case .truncateTables(let names, let ref): activateThen(ref) { [weak self] in diff --git a/TableProTests/Views/Main/MaintenanceSheetIdentityTests.swift b/TableProTests/Views/Main/MaintenanceSheetIdentityTests.swift new file mode 100644 index 000000000..88f9b92b4 --- /dev/null +++ b/TableProTests/Views/Main/MaintenanceSheetIdentityTests.swift @@ -0,0 +1,63 @@ +// +// MaintenanceSheetIdentityTests.swift +// TableProTests +// +// A maintenance request is identified by the object it names, database included. Without the +// database it is the same request in every database that holds a table by that name, which is +// how the command came to run against whichever one the connection happened to be on. +// + +import Foundation +@testable import TablePro +import Testing + +@Suite("Maintenance sheet identity") +struct MaintenanceSheetIdentityTests { + @Test("The same table in two databases is two different requests") + func distinguishesTwoDatabases() { + let online = ActiveSheet.maintenance( + operation: "OPTIMIZE TABLE", tableName: "role_ability", database: "banshi_online", schema: nil + ) + let test = ActiveSheet.maintenance( + operation: "OPTIMIZE TABLE", tableName: "role_ability", database: "banshi_test", schema: nil + ) + + #expect(online.id != test.id) + } + + @Test("The same table in two schemas of one database is two different requests") + func distinguishesTwoSchemas() { + let publicSchema = ActiveSheet.maintenance( + operation: "VACUUM", tableName: "orders", database: "app", schema: "public" + ) + let reporting = ActiveSheet.maintenance( + operation: "VACUUM", tableName: "orders", database: "app", schema: "reporting" + ) + + #expect(publicSchema.id != reporting.id) + } + + @Test("The same object is the same request") + func matchesTheSameObject() { + let first = ActiveSheet.maintenance( + operation: "ANALYZE TABLE", tableName: "orders", database: "app", schema: "public" + ) + let second = ActiveSheet.maintenance( + operation: "ANALYZE TABLE", tableName: "orders", database: "app", schema: "public" + ) + + #expect(first.id == second.id) + } + + @Test("A request that names no database is not the same as one that does") + func distinguishesAnUnnamedDatabase() { + let named = ActiveSheet.maintenance( + operation: "OPTIMIZE TABLE", tableName: "orders", database: "app", schema: nil + ) + let unnamed = ActiveSheet.maintenance( + operation: "OPTIMIZE TABLE", tableName: "orders", database: nil, schema: nil + ) + + #expect(named.id != unnamed.id) + } +}