diff --git a/CHANGELOG.md b/CHANGELOG.md index 169722096..3b7a598bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,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 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. - A table that exists in two databases can be opened in both. The object browser marked the row for a table you had open in another database, so clicking it did nothing at all: the row was already selected and the click changed nothing. The browser now marks a row only while the tab you are looking at belongs to the database on screen. (#2217) - Server Dashboard, Users & Roles, Query Insights, ER Diagram and a linked SQL file now select the tab they already opened instead of doing nothing when another tab is in front. (#2217) - The empty Favorites sidebar fits its width on macOS 15. The description and the New Favorite, New Folder and Link a Folder buttons ran past both edges of the sidebar and were cut off, so the buttons could not be read or reached. They now wrap and stack inside the sidebar. diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift index d4c222e1d..effc4c36e 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarActions.swift @@ -210,15 +210,43 @@ 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 + ) } - func executeMaintenance(operation: String, tableName: String, options: [String: String]) { + /// 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 + /// 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], + 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 } + /// 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 } @@ -245,8 +273,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/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 71a2bb5b7..db4800923 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 ca35d88f4..0cc809dad 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/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) + } +} 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) + } +}