From 2349d3e73180cc1a2f4bcbc160d30372b0ae50a4 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 26 Sep 2026 21:20:16 +0700 Subject: [PATCH] fix(structure): confirm a Structure save once, at the execution gate or the rebuild review, and stay quiet after Cancel --- CHANGELOG.md | 1 + .../QueryExecutionCoordinator.swift | 4 +- .../RowEditingCoordinator+SaveChanges.swift | 2 +- .../Database/DatabaseManager+Schema.swift | 62 ++- TablePro/Core/MCP/MCPAuthPolicy.swift | 2 +- .../SchemaStatementGenerator.swift | 6 +- .../Execution/DefaultExecutionGate.swift | 2 +- .../Execution/ExternalStatementGate.swift | 2 +- .../Execution/OperationDecision.swift | 26 +- .../Providers/OperationConfirming.swift | 17 +- .../Schema/TableRebuildReviewRequest.swift | 20 +- TablePro/Resources/Localizable.xcstrings | 102 ---- .../Main/MainContentCommandActions.swift | 2 +- .../Views/Main/MainContentCoordinator.swift | 2 +- TablePro/Views/Main/MainContentView.swift | 3 + .../StructureColumnReorderHandler.swift | 13 +- .../StructureEditingSession+Apply.swift | 145 ++++-- .../Structure/StructureEditingSession.swift | 5 + .../StructureRebuildPlanRunner.swift | 49 +- .../TableStructureView+ColumnReorder.swift | 29 +- .../Database/SchemaOperationKindTests.swift | 40 ++ .../SchemaStatementGeneratorPluginTests.swift | 36 +- .../Execution/ExecutionGateTests.swift | 39 ++ .../StructureEditingSessionTests.swift | 49 ++ .../StructureSaveConfirmationTests.swift | 447 ++++++++++++++++++ .../Structure/StructureSavePlanTests.swift | 3 +- .../StructureSaveConfirmationUITests.swift | 217 +++++++++ .../StructureTypelessColumnUITests.swift | 10 +- .../Support/SeededSQLiteSession.swift | 26 +- docs/features/safe-mode.mdx | 2 +- docs/features/table-structure.mdx | 4 +- 31 files changed, 1146 insertions(+), 221 deletions(-) create mode 100644 TableProTests/Core/Database/SchemaOperationKindTests.swift create mode 100644 TableProTests/Views/Structure/StructureSaveConfirmationTests.swift create mode 100644 TableProUITests/StructureSaveConfirmationUITests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index fd43cf0c69..318ee6f1b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -196,6 +196,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **New Trigger** offered on a materialized view. - Structure tab refusing every save on a SQLite, libSQL or Cloudflare D1 table with a column that has no declared type. - Structure tab refusing to save a renamed or dropped primary key column. +- Structure saves that could lose data, and table rebuilds, asking twice for one confirmation, and showing an error after Cancel. - Compressed dump named `.GZ` rather than `.gz` reaching the parser still compressed. - **SQL** offered as an import format on MongoDB. - **Save** permanently dim on a Custom provider for an OpenAI-compatible server that wants no API key. diff --git a/TablePro/Core/Coordinators/QueryExecutionCoordinator.swift b/TablePro/Core/Coordinators/QueryExecutionCoordinator.swift index 005da7103f..5587fc14d0 100644 --- a/TablePro/Core/Coordinators/QueryExecutionCoordinator.swift +++ b/TablePro/Core/Coordinators/QueryExecutionCoordinator.swift @@ -86,7 +86,7 @@ final class QueryExecutionCoordinator: ObservableObject { case .needsBatchDriver: break } - case .denied(let reason): + case .denied(let reason, _): parent.tabManager.mutate(at: index) { $0.execution.errorMessage = reason } } } @@ -128,7 +128,7 @@ final class QueryExecutionCoordinator: ObservableObject { switch await ExecutionGateProvider.shared.authorize(request) { case .authorized: executeParameterizedAfterSafeMode(route, parameters: parameters, bypassRowLimit: bypassRowLimit) - case .denied(let reason): + case .denied(let reason, _): parent.tabManager.mutate(tabId: tabId) { $0.execution.errorMessage = reason } } } diff --git a/TablePro/Core/Coordinators/RowEditingCoordinator+SaveChanges.swift b/TablePro/Core/Coordinators/RowEditingCoordinator+SaveChanges.swift index afe307638e..7429a6a08d 100644 --- a/TablePro/Core/Coordinators/RowEditingCoordinator+SaveChanges.swift +++ b/TablePro/Core/Coordinators/RowEditingCoordinator+SaveChanges.swift @@ -133,7 +133,7 @@ extension RowEditingCoordinator { pendingDeletes: &dels, tableOperationOptions: &opts ) - case .denied(let reason): + case .denied(let reason, _): if hasPendingTableOps { restorePendingTableOperations( connectionId: connId, diff --git a/TablePro/Core/Database/DatabaseManager+Schema.swift b/TablePro/Core/Database/DatabaseManager+Schema.swift index 9c50ad1177..b564a0b932 100644 --- a/TablePro/Core/Database/DatabaseManager+Schema.swift +++ b/TablePro/Core/Database/DatabaseManager+Schema.swift @@ -21,32 +21,22 @@ extension DatabaseManager { /// Authorization sits outside the scoped block: it awaits a confirmation sheet and Touch ID, /// and holding the connection's driver gate across a human prompt would freeze every other /// tab on that connection. + /// + /// The gate's sheet is the only confirmation a save gets. A refusal throws + /// `ExecutionGateError.denied` and a Cancel throws `.cancelledByUser`, so the caller can keep + /// the edits staged and stay quiet about a choice the user just made. func executeSchemaChanges( _ statements: [SchemaStatement], databaseType: DatabaseType, - scope: DatabaseScope + scope: DatabaseScope, + gate: any ExecutionGate = ExecutionGateProvider.shared ) async throws { let route = schemaChangeRoute(for: scope) - let combinedSQL = statements.map(\.sql).joined(separator: "\n") - let schemaKind: OperationKind = - QueryClassifier.classifyTier(combinedSQL, databaseType: databaseType) == .destructive - ? .destructiveQuery : .schemaMutation - let authorization = await ExecutionGateProvider.shared.authorize( - OperationRequest( - connectionId: scope.connectionId, - databaseType: databaseType, - sql: combinedSQL, - kind: schemaKind, - caller: .userInterface, - capabilities: .interactiveUser, - operationDescription: String(localized: "Apply Schema Changes") - ) - ) - guard case .authorized = authorization else { - throw DatabaseError.queryFailed( - authorization.deniedReason ?? String(localized: "Schema change was not authorized") - ) + let request = Self.schemaChangeAuthorizationRequest(statements, databaseType: databaseType, scope: scope) + let schemaKind = request.kind + if let denial = await gate.authorize(request).denialError { + throw denial } let executionTimes: [TimeInterval] @@ -110,6 +100,38 @@ extension DatabaseManager { ) } + /// What the gate is asked before a save runs, built apart from the run so a test can put it to + /// the real gate at every Safe Mode level. + nonisolated static func schemaChangeAuthorizationRequest( + _ statements: [SchemaStatement], + databaseType: DatabaseType, + scope: DatabaseScope + ) -> OperationRequest { + let combinedSQL = statements.map(\.sql).joined(separator: "\n") + return OperationRequest( + connectionId: scope.connectionId, + databaseType: databaseType, + sql: combinedSQL, + kind: schemaOperationKind(for: statements, combinedSQL: combinedSQL, databaseType: databaseType), + caller: .userInterface, + capabilities: .interactiveUser, + operationDescription: String(localized: "Apply Schema Changes") + ) + } + + /// Destructive when the text reads that way or when any statement was generated as one. The + /// text alone cannot see that `ALTER COLUMN .. TYPE`, `MODIFY COLUMN .. NOT NULL` or an added + /// `CHECK` can lose or refuse existing rows, and a destructive kind is confirmed at every + /// Safe Mode level, Silent included. + nonisolated static func schemaOperationKind( + for statements: [SchemaStatement], + combinedSQL: String, + databaseType: DatabaseType + ) -> OperationKind { + let destructiveText = QueryClassifier.classifyTier(combinedSQL, databaseType: databaseType) == .destructive + return destructiveText || statements.contains(where: \.isDestructive) ? .destructiveQuery : .schemaMutation + } + /// Run a Create Table draft's statements, on the same isolated route and in the same shape as /// `executeSchemaChanges`. /// diff --git a/TablePro/Core/MCP/MCPAuthPolicy.swift b/TablePro/Core/MCP/MCPAuthPolicy.swift index 3318e824d6..3977eda5d9 100644 --- a/TablePro/Core/MCP/MCPAuthPolicy.swift +++ b/TablePro/Core/MCP/MCPAuthPolicy.swift @@ -330,7 +330,7 @@ public actor MCPAuthPolicy { operationDescription: Self.operationDescription(for: operationLabel) ) ) - if case .denied(let reason) = decision { + if case .denied(let reason, _) = decision { throw DatabaseAccessError.forbidden(reason) } } diff --git a/TablePro/Core/SchemaTracking/SchemaStatementGenerator.swift b/TablePro/Core/SchemaTracking/SchemaStatementGenerator.swift index 891862c155..99d5f728b8 100644 --- a/TablePro/Core/SchemaTracking/SchemaStatementGenerator.swift +++ b/TablePro/Core/SchemaTracking/SchemaStatementGenerator.swift @@ -61,7 +61,11 @@ struct SchemaStatementGenerator { } for stmt in stmts { let sql = stmt.sql.hasSuffix(";") ? stmt.sql : stmt.sql + ";" - statements.append(SchemaStatement(sql: sql, description: stmt.description, isDestructive: stmt.isDestructive)) + statements.append(SchemaStatement( + sql: sql, + description: stmt.description, + isDestructive: stmt.isDestructive || change.requiresDataMigration + )) } } diff --git a/TablePro/Core/Services/Execution/DefaultExecutionGate.swift b/TablePro/Core/Services/Execution/DefaultExecutionGate.swift index e662dd7c93..c539893925 100644 --- a/TablePro/Core/Services/Execution/DefaultExecutionGate.swift +++ b/TablePro/Core/Services/Execution/DefaultExecutionGate.swift @@ -90,7 +90,7 @@ internal actor DefaultExecutionGate: ExecutionGate { ) ) guard confirmed else { - return .denied(reason: String(localized: "Operation cancelled by user")) + return .denied(reason: String(localized: "Operation cancelled by user"), cause: .cancelledByUser) } } diff --git a/TablePro/Core/Services/Execution/ExternalStatementGate.swift b/TablePro/Core/Services/Execution/ExternalStatementGate.swift index 76c33c5a12..132c1e4510 100644 --- a/TablePro/Core/Services/Execution/ExternalStatementGate.swift +++ b/TablePro/Core/Services/Execution/ExternalStatementGate.swift @@ -168,7 +168,7 @@ internal enum ExternalStatementGate { operationDescription: operationDescription ) ) - if case .denied(let reason) = decision { + if case .denied(let reason, _) = decision { throw ExternalStatementGateError.denied(reason) } } diff --git a/TablePro/Core/Services/Execution/OperationDecision.swift b/TablePro/Core/Services/Execution/OperationDecision.swift index 012fb22059..cb97344e83 100644 --- a/TablePro/Core/Services/Execution/OperationDecision.swift +++ b/TablePro/Core/Services/Execution/OperationDecision.swift @@ -21,9 +21,17 @@ internal struct OperationReceipt: Sendable, Equatable { } } +/// Why the gate said no. Only the gate knows whether the person at the keyboard answered Cancel or +/// a rule refused them, and a caller needs that to stay quiet after a Cancel rather than report the +/// user's own choice back to them as a failure. +internal enum OperationDenialCause: Sendable, Equatable { + case policy + case cancelledByUser +} + internal enum OperationDecision: Sendable { case authorized(OperationReceipt) - case denied(reason: String) + case denied(reason: String, cause: OperationDenialCause = .policy) } internal extension OperationDecision { @@ -35,19 +43,31 @@ internal extension OperationDecision { } var deniedReason: String? { - if case .denied(let reason) = self { + if case .denied(let reason, _) = self { return reason } return nil } + + /// What a caller that throws its denial raises, with a Cancel kept apart from a refusal. + var denialError: ExecutionGateError? { + guard case .denied(let reason, let cause) = self else { return nil } + switch cause { + case .policy: + return .denied(reason) + case .cancelledByUser: + return .cancelledByUser(reason) + } + } } internal enum ExecutionGateError: LocalizedError { case denied(String) + case cancelledByUser(String) var errorDescription: String? { switch self { - case .denied(let reason): + case .denied(let reason), .cancelledByUser(let reason): return reason } } diff --git a/TablePro/Core/Services/Execution/Providers/OperationConfirming.swift b/TablePro/Core/Services/Execution/Providers/OperationConfirming.swift index 22ae9dc27c..bb129aaa48 100644 --- a/TablePro/Core/Services/Execution/Providers/OperationConfirming.swift +++ b/TablePro/Core/Services/Execution/Providers/OperationConfirming.swift @@ -34,8 +34,14 @@ internal enum OperationConfirmationPrompt { } internal static func subtitle(of request: OperationConfirmationRequest) -> String { - let connection = request.connectionName?.trimmingCharacters(in: .whitespacesAndNewlines) - guard let client = clientName(for: request.caller) else { + subtitle(connectionName: request.connectionName, caller: request.caller) + } + + /// The line under a confirmation's heading, for a sheet that confirms without going through + /// the gate's own prompt, the table rebuild review among them. + internal static func subtitle(connectionName: String?, caller: OperationCaller) -> String { + let connection = connectionName?.trimmingCharacters(in: .whitespacesAndNewlines) + guard let client = clientName(for: caller) else { guard let connection, !connection.isEmpty else { return String(localized: "Review this before it runs.") } @@ -47,9 +53,12 @@ internal enum OperationConfirmationPrompt { return String(format: String(localized: "%1$@ wants to run this on '%2$@'."), client, connection) } + internal static var destructiveDataWarning: String { + String(localized: "This may permanently modify or delete data and cannot be undone.") + } + internal static func destructiveWarning(of request: OperationConfirmationRequest) -> String? { - guard request.isDestructive else { return nil } - return String(localized: "This may permanently modify or delete data and cannot be undone.") + request.isDestructive ? destructiveDataWarning : nil } /// A rename has no statement to show: the driver builds it from the names, and two engines diff --git a/TablePro/Models/Schema/TableRebuildReviewRequest.swift b/TablePro/Models/Schema/TableRebuildReviewRequest.swift index 4174cda86d..2913cfbc9c 100644 --- a/TablePro/Models/Schema/TableRebuildReviewRequest.swift +++ b/TablePro/Models/Schema/TableRebuildReviewRequest.swift @@ -17,6 +17,8 @@ struct TableRebuildReviewRequest: Identifiable { /// What the confirming button says, which is the only thing that differs between a reorder /// and a constraint change: both recreate the table, and the user is reading the same script. let title: String + /// The operation the gate is told it is authorizing, which the sheet is titled with. + let operationDescription: String let perform: () async -> Void } @@ -33,8 +35,12 @@ struct TableRebuildReviewRequest: Identifiable { let action: Action? + /// A rebuild drops the table it copied, so a sheet that can run it warns the way the gate's own + /// sheet does for a destructive statement, ahead of what the rebuild cannot carry over. var warning: String? { - plan.caveats.isEmpty ? nil : plan.caveats.joined(separator: " ") + let dataWarning = runnableAction.map { _ in OperationConfirmationPrompt.destructiveDataWarning } + let lines = [dataWarning].compactMap { $0 } + plan.caveats + return lines.isEmpty ? nil : lines.joined(separator: " ") } var isRunnable: Bool { plan.isRunnable } @@ -44,4 +50,16 @@ struct TableRebuildReviewRequest: Identifiable { } var scriptStatements: [String] { plan.scriptStatements } + + /// A sheet that can run the script is that script's only confirmation: the run it starts tells + /// the gate so. It therefore reads as the gate's own sheet does, titled with the operation, + /// naming the connection, and showing the script uncut. A preview keeps the preview heading. + var confirmationTitle: String? { runnableAction?.operationDescription } + + var showsStatementsVerbatim: Bool { runnableAction != nil } + + func confirmationSubtitle(connectionName: String?) -> String? { + guard runnableAction != nil else { return nil } + return OperationConfirmationPrompt.subtitle(connectionName: connectionName, caller: .userInterface) + } } diff --git a/TablePro/Resources/Localizable.xcstrings b/TablePro/Resources/Localizable.xcstrings index 55d3752f85..51f3cb4cab 100644 --- a/TablePro/Resources/Localizable.xcstrings +++ b/TablePro/Resources/Localizable.xcstrings @@ -52681,40 +52681,6 @@ } } }, - "Destructive Changes" : { - "localizations" : { - "ko" : { - "stringUnit" : { - "state" : "translated", - "value" : "파괴적 변경" - } - }, - "tr" : { - "stringUnit" : { - "state" : "translated", - "value" : "Yıkıcı Değişiklikler" - } - }, - "vi" : { - "stringUnit" : { - "state" : "translated", - "value" : "Thay đổi có thể mất dữ liệu" - } - }, - "zh-Hans" : { - "stringUnit" : { - "state" : "translated", - "value" : "破坏性更改" - } - }, - "zh-Hant" : { - "stringUnit" : { - "state" : "translated", - "value" : "破壞性變更" - } - } - } - }, "Destructive operations are not permitted for this client" : { "localizations" : { "ko" : { @@ -134534,40 +134500,6 @@ } } }, - "Schema change was not authorized" : { - "localizations" : { - "ko" : { - "stringUnit" : { - "state" : "translated", - "value" : "스키마 변경 권한이 없습니다" - } - }, - "tr" : { - "stringUnit" : { - "state" : "translated", - "value" : "Şema değişikliğine izin verilmedi" - } - }, - "vi" : { - "stringUnit" : { - "state" : "translated", - "value" : "Thay đổi schema không được cho phép" - } - }, - "zh-Hans" : { - "stringUnit" : { - "state" : "translated", - "value" : "Schema 更改未获授权" - } - }, - "zh-Hant" : { - "stringUnit" : { - "state" : "translated", - "value" : "綱要變更未獲授權" - } - } - } - }, "Schema for %@" : { "extractionState" : "stale", "localizations" : { @@ -159300,40 +159232,6 @@ } } }, - "The following changes may cause data loss:\n\n%@\n\nDo you want to proceed?" : { - "localizations" : { - "ko" : { - "stringUnit" : { - "state" : "translated", - "value" : "다음 변경 사항으로 인해 데이터가 손실될 수 있습니다.\n\n%@\n\n계속하시겠습니까?" - } - }, - "tr" : { - "stringUnit" : { - "state" : "translated", - "value" : "Aşağıdaki değişiklikler veri kaybına neden olabilir:\n\n%@\n\nDevam etmek istiyor musunuz?" - } - }, - "vi" : { - "stringUnit" : { - "state" : "translated", - "value" : "Các thay đổi sau có thể gây mất dữ liệu:\n\n%@\n\nBạn có muốn tiếp tục?" - } - }, - "zh-Hans" : { - "stringUnit" : { - "state" : "translated", - "value" : "以下更改可能导致数据丢失：\n\n%@\n\n是否继续？" - } - }, - "zh-Hant" : { - "stringUnit" : { - "state" : "translated", - "value" : "下列變更可能導致資料遺失：\n\n%@\n\n是否要繼續？" - } - } - } - }, "The following plugins were rejected:\n\n%@\n\nPlease update them from the plugin registry." : { "extractionState" : "stale", "localizations" : { diff --git a/TablePro/Views/Main/MainContentCommandActions.swift b/TablePro/Views/Main/MainContentCommandActions.swift index 2f14a10231..c4a10869bd 100644 --- a/TablePro/Views/Main/MainContentCommandActions.swift +++ b/TablePro/Views/Main/MainContentCommandActions.swift @@ -830,7 +830,7 @@ final class MainContentCommandActions: ObservableObject { /// False comes back whenever the work is still staged after the attempt, because the caller /// goes on to close and closing destroys it. User and role changes can only be applied after /// the SQL is reviewed, so Save opens the review sheet and stands the close down; a schema - /// change that Safe Mode refused, that the user cancelled at the destructive prompt, or that + /// change that Safe Mode refused, that the user cancelled at the gate's confirmation, or that /// the server rejected stands it down for the same reason, and so does a file that changed on /// disk, whose conflict sheet is now up, or a Save As the user cancelled. func saveSelectedTabWork() async -> Bool { diff --git a/TablePro/Views/Main/MainContentCoordinator.swift b/TablePro/Views/Main/MainContentCoordinator.swift index 1bd5bd6ace..83e027266d 100644 --- a/TablePro/Views/Main/MainContentCoordinator.swift +++ b/TablePro/Views/Main/MainContentCoordinator.swift @@ -1193,7 +1193,7 @@ final class MainContentCoordinator: ObservableObject { switch decision { case .authorized: executeQueryInternal(sql, isAutoLoad: true, trigger: trigger, viewport: viewport) - case .denied(let reason): + case .denied(let reason, _): traceNavigationAbandoned(tabId: tab.id, outcome: .safeModeDenied) tabManager.mutate(at: index) { $0.execution.errorMessage = reason } } diff --git a/TablePro/Views/Main/MainContentView.swift b/TablePro/Views/Main/MainContentView.swift index 9c226a23a4..b393ed9d48 100644 --- a/TablePro/Views/Main/MainContentView.swift +++ b/TablePro/Views/Main/MainContentView.swift @@ -283,6 +283,9 @@ struct MainContentView: View { isPresented: dismissBinding, statements: request.scriptStatements, databaseType: connection.type, + title: request.confirmationTitle, + subtitle: request.confirmationSubtitle(connectionName: connection.name), + showsStatementsVerbatim: request.showsStatementsVerbatim, warning: request.warning, primaryAction: request.runnableAction.map { action in SQLReviewSheet.PrimaryAction( diff --git a/TablePro/Views/Structure/StructureColumnReorderHandler.swift b/TablePro/Views/Structure/StructureColumnReorderHandler.swift index 9f8ba7366e..235acd1457 100644 --- a/TablePro/Views/Structure/StructureColumnReorderHandler.swift +++ b/TablePro/Views/Structure/StructureColumnReorderHandler.swift @@ -110,11 +110,19 @@ enum StructureColumnReorderHandler { return PreparedReorder(plan: prepared.0, fingerprint: prepared.1, scope: scope) } + static var operationDescription: String { + String(localized: "Reorder Columns") + } + /// Runs a prepared reorder through the shared plan runner. + /// + /// `isConfirmationPreCleared` is true only from the rebuild review's own button, which showed + /// this script; a reorder that runs on the drop leaves the confirmation to the gate. static func execute( _ prepared: PreparedReorder, tableName: String, - databaseType: DatabaseType + databaseType: DatabaseType, + isConfirmationPreCleared: Bool ) async throws { try await StructureRebuildPlanRunner.execute( StructureRebuildPlanRunner.Prepared( @@ -124,7 +132,8 @@ enum StructureColumnReorderHandler { tableName: tableName ), databaseType: databaseType, - operationDescription: String(localized: "Reorder Columns") + operationDescription: operationDescription, + isConfirmationPreCleared: isConfirmationPreCleared ) } } diff --git a/TablePro/Views/Structure/StructureEditingSession+Apply.swift b/TablePro/Views/Structure/StructureEditingSession+Apply.swift index 9a32c9d5b2..836aa0bcb5 100644 --- a/TablePro/Views/Structure/StructureEditingSession+Apply.swift +++ b/TablePro/Views/Structure/StructureEditingSession+Apply.swift @@ -16,8 +16,8 @@ internal enum StructureSaveOutcome: Equatable { /// Nothing was staged. The close may proceed: there is no work to lose. case nothingToApply case applied - /// Safe Mode refused the write, or the user cancelled the destructive-changes prompt. The edits - /// are still staged. + /// Safe Mode refused the write, or the user cancelled at the execution gate's confirmation. The + /// edits are still staged. case refused case failed(String) @@ -29,6 +29,54 @@ internal enum StructureSaveOutcome: Equatable { } } +/// How a save that did not apply ends, read from the error alone so it can be tested without +/// presenting anything. +/// +/// The gate's sheet is the one confirmation a save gets, so Cancel there is the ordinary way to +/// back out, and it ends quietly the way closing any confirmation does. Answering it with an +/// "Error Applying Changes" sheet put a second dialog in front of the user for the choice they had +/// just made. +internal enum StructureApplyFailure: Equatable { + case cancelledByUser + case refused(String) + case failed(String) + + internal init(_ error: any Error) { + guard let gateError = error as? ExecutionGateError else { + self = .failed(error.localizedDescription) + return + } + switch gateError { + case .cancelledByUser: + self = .cancelledByUser + case .denied(let reason): + self = .refused(reason) + } + } + + internal var outcome: StructureSaveOutcome { + switch self { + case .cancelledByUser, .refused: .refused + case .failed(let message): .failed(message) + } + } + + /// What the error sheet says. Nil for a Cancel, which shows nothing. + internal var message: String? { + switch self { + case .cancelledByUser: nil + case .refused(let reason), .failed(let reason): reason + } + } + + /// A refusal and a Cancel both stop at the gate, before the first statement, so neither is a + /// failed operation to report or a reason to reload the catalog. + internal var reportsFailure: Bool { + if case .failed = self { return true } + return false + } +} + internal extension StructureEditingSession { /// Applies this tab's staged ALTERs, with no mounted view required. /// @@ -44,7 +92,7 @@ internal extension StructureEditingSession { let changes = changeManager.getChangesArray() guard !changes.isEmpty else { return .nothingToApply } - /// Asked before Safe Mode and before the destructive prompt, because an incomplete row is + /// Asked before Safe Mode and before the gate's confirmation, because an incomplete row is /// not a change the user meant to make. Without this a foreign key added and never filled /// in reached DDL generation as `ADD CONSTRAINT "" FOREIGN KEY () REFERENCES "" ()`, which /// SQLite refused as an unsupported operation and MySQL sent to the server as a syntax @@ -95,40 +143,24 @@ internal extension StructureEditingSession { /// instead, and a rebuild is never run from a Save press. It is shown in full, with what /// it cannot carry over, and confirmed before anything is dropped. /// - /// Ahead of the destructive-changes prompt, not after it. The review sheet is already - /// that confirmation and shows the exact script rather than a list of descriptions, so - /// asking first would be two dialogs for one decision. The HIG's rule is one alert at a - /// time. + /// The review sheet is that confirmation and shows the exact script, so the run it + /// starts tells the gate it was confirmed rather than stacking the gate's own sheet over + /// it for the same decision. The HIG's rule is one alert at a time. return presentRebuildReview(prepared, startedAt: planStart, coordinator: coordinator) case .alter(let statements): - return await applyAlterStatements(statements, changes: changes, coordinator: coordinator) + return await applyAlterStatements(statements, coordinator: coordinator) } } + /// The execution gate is the one confirmation an `ALTER` save gets. It shows the statements + /// verbatim, applies the connection's Safe Mode level, and confirms a change that can lose data + /// at every level because `SchemaStatementGenerator` marks those statements destructive. The + /// editor used to ask first with a list of descriptions, which made one decision two dialogs, + /// and three with Touch ID. private func applyAlterStatements( _ statements: [SchemaStatement], - changes: [SchemaChange], coordinator: MainContentCoordinator? ) async -> StructureSaveOutcome { - let destructiveChanges = changes.filter(\.requiresDataMigration) - if !destructiveChanges.isEmpty { - let message = String( - format: String(localized: "The following changes may cause data loss:\n\n%@\n\nDo you want to proceed?"), - destructiveChanges.map(\.description).joined(separator: "\n") - ) - let confirmed = await AlertHelper.confirmDestructive( - title: String(localized: "Destructive Changes"), - message: message, - confirmButton: String(localized: "Apply Changes"), - cancelButton: String(localized: "Cancel"), - window: coordinator?.contentWindow - ) - guard confirmed else { return .refused } - } - - /// Started here, not at the top of the function. The destructive-changes prompt sits above - /// this and the user can take as long as they like over it, so a clock started earlier - /// measures their reading time and reports an instant ALTER as having taken a minute. let operationStart = ContinuousClock.Instant.now isApplying = true @@ -136,7 +168,8 @@ internal extension StructureEditingSession { try await DatabaseManager.shared.executeSchemaChanges( statements, databaseType: connection.type, - scope: scope + scope: scope, + gate: executionGate ) changeManager.discardChanges() tabData.markAllStale() @@ -148,16 +181,28 @@ internal extension StructureEditingSession { return .applied } catch { isApplying = false - report(.failed(reason: error.localizedDescription), startedAt: operationStart, coordinator: coordinator) - AlertHelper.showErrorSheet( - title: String(localized: "Error Applying Changes"), - message: error.localizedDescription, - window: coordinator?.contentWindow - ) - return .failed(error.localizedDescription) + let failure = StructureApplyFailure(error) + present(failure, startedAt: operationStart, coordinator: coordinator) + return failure.outcome } } + private func present( + _ failure: StructureApplyFailure, + startedAt: ContinuousClock.Instant, + coordinator: MainContentCoordinator? + ) { + guard let message = failure.message else { return } + if failure.reportsFailure { + report(.failed(reason: message), startedAt: startedAt, coordinator: coordinator) + } + AlertHelper.showErrorSheet( + title: String(localized: "Error Applying Changes"), + message: message, + window: coordinator?.contentWindow + ) + } + /// Hands the rebuild script to the review sheet. /// /// Returns `.refused` because at this point nothing has run and the edits are still staged, @@ -175,6 +220,7 @@ internal extension StructureEditingSession { plan: prepared.plan, action: TableRebuildReviewRequest.Action( title: String(localized: "Apply and Rebuild"), + operationDescription: Self.applyOperationDescription, perform: { [weak coordinator] in await self.runRebuild(prepared, startedAt: operationStart, coordinator: coordinator) } @@ -184,11 +230,18 @@ internal extension StructureEditingSession { return .refused } + private static var applyOperationDescription: String { + String(localized: "Apply Schema Changes") + } + /// Runs a confirmed rebuild and does everything a save owes the rest of the app afterwards. /// /// The table was dropped and recreated, so the grid's rows, the query history and the saved /// column layout all describe a table that no longer exists in that form. The ordinary save /// path does not record history or clear a layout because an `ALTER` leaves both valid. + /// + /// Only the review sheet's own button reaches this, so the gate is told the script was + /// confirmed. Touch ID and the Read-Only refusal still apply. private func runRebuild( _ prepared: StructureRebuildPlanRunner.Prepared, startedAt: ContinuousClock.Instant, @@ -199,19 +252,19 @@ internal extension StructureEditingSession { try await StructureRebuildPlanRunner.execute( prepared, databaseType: connection.type, - operationDescription: String(localized: "Apply Schema Changes") + operationDescription: Self.applyOperationDescription, + isConfirmationPreCleared: true, + gate: executionGate ) } catch { isApplying = false - CatalogChangeService.post( - .changed(CatalogChange(connectionId: connection.id, database: prepared.scope.database, kinds: .tables)) - ) - report(.failed(reason: error.localizedDescription), startedAt: startedAt, coordinator: coordinator) - AlertHelper.showErrorSheet( - title: String(localized: "Error Applying Changes"), - message: error.localizedDescription, - window: coordinator?.contentWindow - ) + let failure = StructureApplyFailure(error) + if failure.reportsFailure { + CatalogChangeService.post( + .changed(CatalogChange(connectionId: connection.id, database: prepared.scope.database, kinds: .tables)) + ) + } + present(failure, startedAt: startedAt, coordinator: coordinator) return } diff --git a/TablePro/Views/Structure/StructureEditingSession.swift b/TablePro/Views/Structure/StructureEditingSession.swift index f5f921104e..0d06bf7e8c 100644 --- a/TablePro/Views/Structure/StructureEditingSession.swift +++ b/TablePro/Views/Structure/StructureEditingSession.swift @@ -121,6 +121,11 @@ internal final class StructureEditingSession: ObservableObject { /// what the save has just re-fetched. @Published internal var lastAppliedAt: Date? + /// What every save from this tab is authorized through. A test supplies one whose prompts + /// answer themselves, because the real gate raises an application-modal alert that nothing on + /// a CI runner can dismiss. + internal var executionGate: any ExecutionGate = ExecutionGateProvider.shared + internal init( identity: String, connection: DatabaseConnection, diff --git a/TablePro/Views/Structure/StructureRebuildPlanRunner.swift b/TablePro/Views/Structure/StructureRebuildPlanRunner.swift index 2e461f7c46..918773035f 100644 --- a/TablePro/Views/Structure/StructureRebuildPlanRunner.swift +++ b/TablePro/Views/Structure/StructureRebuildPlanRunner.swift @@ -65,6 +65,33 @@ enum StructureRebuildPlanRunner { } } + /// What the gate is asked before a plan runs. + /// + /// `isConfirmationPreCleared` is true only from a review sheet that showed this exact script + /// and was answered with its own button. That sheet is the confirmation, and the gate asking + /// again stacked a second sheet over it for the same decision. It clears the confirmation only: + /// Touch ID, the Read-Only refusal and the audit record all still apply. A plan that runs with + /// no review, a metadata-only reorder, passes false and is confirmed by the gate per level. + nonisolated static func authorizationRequest( + plan: PluginColumnReorderPlan, + scope: DatabaseScope, + databaseType: DatabaseType, + operationDescription: String, + isConfirmationPreCleared: Bool + ) -> OperationRequest { + var capabilities = CallerCapabilities.interactiveUser + if isConfirmationPreCleared { capabilities.insert(.confirmationPreCleared) } + return OperationRequest( + connectionId: scope.connectionId, + databaseType: databaseType, + sql: plan.scriptStatements.joined(separator: "\n"), + kind: plan.cost == .tableRebuild ? .destructiveQuery : .schemaMutation, + caller: .userInterface, + capabilities: capabilities, + operationDescription: operationDescription + ) + } + /// Runs a prepared plan, once, on the scope it was planned against. /// /// Authorization happens once for the whole plan, before any statement runs, and deliberately @@ -75,25 +102,25 @@ enum StructureRebuildPlanRunner { static func execute( _ prepared: Prepared, databaseType: DatabaseType, - operationDescription: String + operationDescription: String, + isConfirmationPreCleared: Bool, + gate: any ExecutionGate = ExecutionGateProvider.shared ) async throws { let plan = prepared.plan let scope = prepared.scope let tableName = prepared.tableName - let decision = await ExecutionGateProvider.shared.authorize( - OperationRequest( - connectionId: scope.connectionId, + let decision = await gate.authorize( + authorizationRequest( + plan: plan, + scope: scope, databaseType: databaseType, - sql: plan.scriptStatements.joined(separator: "\n"), - kind: plan.cost == .tableRebuild ? .destructiveQuery : .schemaMutation, - caller: .userInterface, - capabilities: .interactiveUser, - operationDescription: operationDescription + operationDescription: operationDescription, + isConfirmationPreCleared: isConfirmationPreCleared ) ) - guard case .authorized = decision else { - throw DatabaseError.queryFailed(decision.deniedReason ?? String(localized: "Operation not permitted")) + if let denial = decision.denialError { + throw denial } let expectedFingerprint = prepared.fingerprint diff --git a/TablePro/Views/Structure/TableStructureView+ColumnReorder.swift b/TablePro/Views/Structure/TableStructureView+ColumnReorder.swift index 5d6a354371..df7bbc608b 100644 --- a/TablePro/Views/Structure/TableStructureView+ColumnReorder.swift +++ b/TablePro/Views/Structure/TableStructureView+ColumnReorder.swift @@ -49,7 +49,7 @@ extension TableStructureView { switch prepared.plan.cost { case .metadataOnly: try await StructureColumnReorderHandler.execute( - prepared, tableName: tableName, databaseType: connection.type + prepared, tableName: tableName, databaseType: connection.type, isConfirmationPreCleared: false ) await finishColumnReorder(prepared, clearTarget: clearTarget) case .tableRebuild: @@ -75,22 +75,27 @@ extension TableStructureView { tableName: tableName, scope: prepared.scope, plan: prepared.plan, - action: TableRebuildReviewRequest.Action(title: String(localized: "Rebuild Table")) { + action: TableRebuildReviewRequest.Action( + title: String(localized: "Rebuild Table"), + operationDescription: StructureColumnReorderHandler.operationDescription + ) { do { try await StructureColumnReorderHandler.execute( - prepared, tableName: tableName, databaseType: connection.type + prepared, tableName: tableName, databaseType: connection.type, isConfirmationPreCleared: true ) await finishColumnReorder(prepared, clearTarget: clearTarget) } catch { - CatalogChangeService.post( - .changed( - CatalogChange( - connectionId: prepared.scope.connectionId, - database: prepared.scope.database, - kinds: .tables + if StructureApplyFailure(error).reportsFailure { + CatalogChangeService.post( + .changed( + CatalogChange( + connectionId: prepared.scope.connectionId, + database: prepared.scope.database, + kinds: .tables + ) ) ) - ) + } reportColumnReorderFailure(error) } } @@ -129,10 +134,12 @@ extension TableStructureView { ) } + /// A Cancel at the gate's confirmation shows nothing: the user chose it a moment ago. private func reportColumnReorderFailure(_ error: any Error) { + guard let message = StructureApplyFailure(error).message else { return } AlertHelper.showErrorSheet( title: String(localized: "Column Reorder Failed"), - message: error.localizedDescription, + message: message, window: coordinator?.contentWindow ) } diff --git a/TableProTests/Core/Database/SchemaOperationKindTests.swift b/TableProTests/Core/Database/SchemaOperationKindTests.swift new file mode 100644 index 0000000000..bd1f02082a --- /dev/null +++ b/TableProTests/Core/Database/SchemaOperationKindTests.swift @@ -0,0 +1,40 @@ +// +// SchemaOperationKindTests.swift +// TableProTests +// + +import Foundation +@testable import TablePro +import Testing + +struct SchemaOperationKindTests { + private func kind(_ sql: String, destructive: Bool, _ type: DatabaseType) -> OperationKind { + let statement = SchemaStatement(sql: sql, description: "change", isDestructive: destructive) + return DatabaseManager.schemaOperationKind(for: [statement], combinedSQL: sql, databaseType: type) + } + + @Test("A removed MongoDB field is destructive, though its statement is an update") + func removedFieldIsDestructive() { + let sql = #"db.users.updateMany({"legacy": {"$exists": true}}, {"$unset": {"legacy": ""}});"# + #expect(kind(sql, destructive: true, .mongodb) == .destructiveQuery) + } + + @Test("A renamed MongoDB field and the collMod ahead of it stay a schema change") + func renamedFieldIsNotDestructive() { + let rename = #"db.users.updateMany({"a": {"$exists": true}, "b": {"$exists": false}}, {"$rename": {"a": "b"}});"# + let validator = #"db.runCommand({"collMod": "users", "validator": {"$jsonSchema": {}}});"# + #expect(kind(rename, destructive: false, .mongodb) == .schemaMutation) + #expect(kind(validator, destructive: false, .mongodb) == .schemaMutation) + } + + @Test("A SQL DROP COLUMN is destructive from its text, as before") + func sqlDropColumn() { + #expect(kind("ALTER TABLE t DROP COLUMN c;", destructive: false, .postgresql) == .destructiveQuery) + #expect(kind("ALTER TABLE t ADD COLUMN c int;", destructive: false, .postgresql) == .schemaMutation) + } + + @Test("A SQL column type change is destructive, as the Structure tab already marks it") + func sqlTypeChange() { + #expect(kind("ALTER TABLE t ALTER COLUMN c TYPE bigint;", destructive: true, .postgresql) == .destructiveQuery) + } +} diff --git a/TableProTests/Core/SchemaTracking/SchemaStatementGeneratorPluginTests.swift b/TableProTests/Core/SchemaTracking/SchemaStatementGeneratorPluginTests.swift index 12712f672e..777bef3d14 100644 --- a/TableProTests/Core/SchemaTracking/SchemaStatementGeneratorPluginTests.swift +++ b/TableProTests/Core/SchemaTracking/SchemaStatementGeneratorPluginTests.swift @@ -539,7 +539,7 @@ struct SchemaStatementGeneratorPluginTests { @Test("Modify column with type change is destructive") func modifyColumnTypeChangeDestructive() throws { let mock = MockPluginDriver() - mock.modifyColumnHandler = { _, oldCol, newCol in + mock.modifyColumnHandler = { _, _, newCol in "ALTER TABLE users MODIFY COLUMN \(newCol.name) \(newCol.dataType)" } @@ -552,6 +552,40 @@ struct SchemaStatementGeneratorPluginTests { #expect(stmts[0].isDestructive == true) } + /// `MODIFY COLUMN .. NOT NULL` reads as a plain write, so the execution gate only confirms it at + /// Silent because the statement says it can refuse existing rows. + @Test("Making a column NOT NULL is destructive with its type unchanged") + func notNullWithSameTypeIsDestructive() throws { + let mock = MockPluginDriver() + mock.modifyColumnHandler = { _, _, newCol in + "ALTER TABLE users MODIFY COLUMN \(newCol.name) \(newCol.dataType) NOT NULL" + } + + let generator = SchemaStatementGenerator(tableName: "users", pluginDriver: mock) + let oldCol = makeColumn(name: "email", dataType: "VARCHAR(255)", isNullable: true) + let newCol = makeColumn(name: "email", dataType: "VARCHAR(255)", isNullable: false) + let stmts = try generator.generate(changes: [.modifyColumn(old: oldCol, new: newCol)]) + + #expect(stmts.count == 1) + #expect(stmts[0].isDestructive) + } + + @Test("Renaming a column is not destructive") + func renameIsNotDestructive() throws { + let mock = MockPluginDriver() + mock.modifyColumnHandler = { _, oldCol, newCol in + "ALTER TABLE users RENAME COLUMN \(oldCol.name) TO \(newCol.name)" + } + + let generator = SchemaStatementGenerator(tableName: "users", pluginDriver: mock) + let stmts = try generator.generate(changes: [ + .modifyColumn(old: makeColumn(name: "email"), new: makeColumn(name: "contact_email")) + ]) + + #expect(stmts.count == 1) + #expect(!stmts[0].isDestructive) + } + @Test("Add column is not destructive") func addColumnNotDestructive() throws { let mock = MockPluginDriver() diff --git a/TableProTests/Core/Services/Execution/ExecutionGateTests.swift b/TableProTests/Core/Services/Execution/ExecutionGateTests.swift index ce1a0b3aa3..2957031c08 100644 --- a/TableProTests/Core/Services/Execution/ExecutionGateTests.swift +++ b/TableProTests/Core/Services/Execution/ExecutionGateTests.swift @@ -205,6 +205,45 @@ struct ExecutionGateTests { #expect(confirm.callCount == 1) } + @Test("A Cancel at the confirmation is told apart from a refusal") + func cancelCarriesItsCause() async { + let gate = makeGate(level: .silent, confirm: StubConfirming(answer: false), auth: StubAuthenticating(answer: true)) + + let decision = await gate.authorize(makeRequest(sql: "TRUNCATE t", kind: .destructiveQuery)) + + guard case .denied(_, let cause) = decision else { + Issue.record("A Cancel must deny") + return + } + #expect(cause == .cancelledByUser) + guard case .cancelledByUser = decision.denialError else { + Issue.record("A Cancel must throw as a Cancel, got \(String(describing: decision.denialError))") + return + } + } + + @Test("Read-Only and a declined Touch ID are refusals, not a Cancel") + func refusalsCarryThePolicyCause() async { + let readOnly = await makeGate( + level: .readOnly, confirm: StubConfirming(answer: true), auth: StubAuthenticating(answer: true) + ).authorize(makeRequest(sql: "DELETE FROM t WHERE id = 1", kind: .writeQuery)) + let declined = await makeGate( + level: .safeMode, confirm: StubConfirming(answer: true), auth: StubAuthenticating(answer: false) + ).authorize(makeRequest(sql: "DELETE FROM t WHERE id = 1", kind: .writeQuery)) + + for decision in [readOnly, declined] { + guard case .denied(_, let cause) = decision else { + Issue.record("Expected a denial") + continue + } + #expect(cause == .policy) + guard case .denied = decision.denialError else { + Issue.record("A refusal must throw as a refusal, got \(String(describing: decision.denialError))") + continue + } + } + } + @Test("Unqualified DELETE is treated as destructive even when declared a write") func unqualifiedDeleteForcesConfirm() async { let confirm = StubConfirming(answer: true) diff --git a/TableProTests/Views/Structure/StructureEditingSessionTests.swift b/TableProTests/Views/Structure/StructureEditingSessionTests.swift index db992ea301..fdccf9d087 100644 --- a/TableProTests/Views/Structure/StructureEditingSessionTests.swift +++ b/TableProTests/Views/Structure/StructureEditingSessionTests.swift @@ -193,6 +193,55 @@ struct StructureEditingSessionTests { #expect(!session.hasLoaded) } + /// The gate's sheet is the one confirmation a save gets, so Cancel there is how a user backs + /// out. It leaves the edits staged, runs nothing, and presents nothing: answering it with an + /// "Error Applying Changes" sheet was a second dialog for the choice just made, and with no + /// window here any sheet would be an application-modal alert this test host never leaves. + @Test("Cancelling at the gate keeps the edits staged and runs nothing") + func cancelAtTheGateKeepsTheEdits() async throws { + let connection = TestFixtures.makeConnection(database: "testdb") + let sessionDriver = StructureSessionDriver() + var connectionSession = ConnectionSession( + connection: connection, + driver: PluginDriverAdapter(connection: connection, pluginDriver: sessionDriver) + ) + connectionSession.browseDatabase = "testdb" + DatabaseManager.shared.injectSession(connectionSession, for: connection.id) + + let session = Self.makeSession(connection: connection) + let pooledDriver = try await Self.seedPooledDriver(connection, scope: session.scope) + defer { + MetadataConnectionPool.shared.closeAll(connectionId: connection.id) + DatabaseManager.shared.removeSession(for: connection.id) + } + + let confirm = StubConfirming(answer: false) + let auth = StubAuthenticating(answer: true) + session.executionGate = DefaultExecutionGate( + confirming: confirm, + authenticating: auth, + safeModeLevelResolver: { _ in .alert }, + forcesWriteResolver: { _ in false }, + auditLog: ExecutionAuditLog( + fileURL: FileManager.default.temporaryDirectory + .appendingPathComponent("structure-session-audit-\(UUID().uuidString).json") + ) + ) + + Self.stageAColumn(on: session) + let outcome = await session.applyStagedChanges(coordinator: nil) + + #expect(outcome == .refused) + #expect(!outcome.allowsClose) + #expect(confirm.callCount == 1) + #expect(auth.callCount == 0) + #expect(session.changeManager.hasChanges) + #expect(!pooledDriver.executedQueries.contains { $0.contains("ADD COLUMN") }) + #expect(sessionDriver.executedQueries.isEmpty) + #expect(session.appliedVersion == 0) + #expect(!session.isApplying) + } + /// Stands in for the connection the pool would open on the scope. private static func seedPooledDriver( _ connection: DatabaseConnection, diff --git a/TableProTests/Views/Structure/StructureSaveConfirmationTests.swift b/TableProTests/Views/Structure/StructureSaveConfirmationTests.swift new file mode 100644 index 0000000000..8cd198fdb5 --- /dev/null +++ b/TableProTests/Views/Structure/StructureSaveConfirmationTests.swift @@ -0,0 +1,447 @@ +// +// StructureSaveConfirmationTests.swift +// TableProTests +// +// A Structure save is confirmed once. The execution gate's sheet is that confirmation for an +// ALTER save and the rebuild review is that confirmation for a rebuild; the editor used to ask +// first with an alert of its own, which made one decision two dialogs, and three with Touch ID. +// Every case runs the request the app builds through the real gate, with prompts that count. +// + +import Foundation +@testable import TablePro +import TableProPluginKit +import Testing + +private final class MySQLShapedDDLDriver: PluginDatabaseDriver, @unchecked Sendable { + func generateAddColumnSQL(table: String, column: PluginColumnDefinition) -> String? { + "ALTER TABLE `\(table)` ADD COLUMN `\(column.name)` \(column.dataType)" + } + + func generateModifyColumnSQL( + table: String, + oldColumn: PluginColumnDefinition, + newColumn: PluginColumnDefinition + ) -> String? { + let nullability = newColumn.isNullable ? "NULL" : "NOT NULL" + return "ALTER TABLE `\(table)` CHANGE COLUMN `\(oldColumn.name)` `\(newColumn.name)` " + + "\(newColumn.dataType) \(nullability)" + } + + func generateDropColumnSQL(table: String, columnName: String) -> String? { + "ALTER TABLE `\(table)` DROP COLUMN `\(columnName)`" + } + + func generateDropIndexSQL(table: String, indexName: String) -> String? { + "DROP INDEX `\(indexName)` ON `\(table)`" + } + + func generateAddCheckConstraintSQL(table: String, constraint: PluginCheckConstraintDefinition) -> String? { + "ALTER TABLE `\(table)` ADD CONSTRAINT `\(constraint.name)` CHECK (\(constraint.expression))" + } + + func generateModifyPrimaryKeySQL( + table: String, + oldColumns: [String], + newColumns: [String], + constraintName: String? + ) -> [String]? { + [ + "ALTER TABLE `\(table)` DROP PRIMARY KEY", + "ALTER TABLE `\(table)` ADD PRIMARY KEY (\(newColumns.map { "`\($0)`" }.joined(separator: ", ")))" + ] + } + + func connect() async throws {} + func disconnect() {} + func ping() async throws {} + func execute(query: String) async throws -> PluginQueryResult { + PluginQueryResult(columns: [], columnTypeNames: [], rows: [], rowsAffected: 0, executionTime: 0) + } + 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) + } +} + +private struct StructureSave { + let name: String + let change: SchemaChange + let isDestructive: Bool +} + +private struct ServerRejection: LocalizedError { + var errorDescription: String? { "Duplicate column name 'notes'" } +} + +@MainActor +struct StructureSaveConfirmationTests { + private static let scope = DatabaseScope(connectionId: UUID(), database: "shop", schema: nil) + + private static func column( + _ name: String, + type: String = "VARCHAR(255)", + isNullable: Bool = true + ) -> EditableColumnDefinition { + EditableColumnDefinition( + id: UUID(), + name: name, + dataType: type, + isNullable: isNullable, + defaultValue: nil, + autoIncrement: false, + unsigned: false, + comment: nil, + collation: nil, + onUpdate: nil, + charset: nil, + extra: nil, + isPrimaryKey: false + ) + } + + private static var saves: [StructureSave] { + let email = column("email") + return [ + StructureSave(name: "add column", change: .addColumn(column("notes", type: "TEXT")), isDestructive: false), + StructureSave( + name: "rename column", + change: .modifyColumn(old: email, new: column("contact_email")), + isDestructive: false + ), + StructureSave(name: "drop column", change: .deleteColumn(email), isDestructive: true), + StructureSave( + name: "type change", + change: .modifyColumn(old: email, new: column("email", type: "VARCHAR(32)")), + isDestructive: true + ), + StructureSave( + name: "NOT NULL", + change: .modifyColumn(old: email, new: column("email", isNullable: false)), + isDestructive: true + ), + StructureSave( + name: "add check constraint", + change: .addCheckConstraint( + EditableCheckConstraintDefinition( + id: UUID(), name: "chk_email", expression: "email LIKE '%@%'", columns: ["email"], isValidated: true + ) + ), + isDestructive: true + ), + StructureSave( + name: "primary key change", + change: .modifyPrimaryKey(old: ["id"], new: ["id", "tenant_id"]), + isDestructive: true + ), + StructureSave( + name: "drop index", + change: .deleteIndex( + EditableIndexDefinition( + id: UUID(), name: "idx_email", columns: ["email"], type: .btree, + isUnique: false, isPrimary: false, comment: nil + ) + ), + isDestructive: true + ) + ] + } + + private static func gate( + _ level: SafeModeLevel, + confirm: StubConfirming, + auth: StubAuthenticating + ) -> DefaultExecutionGate { + DefaultExecutionGate( + confirming: confirm, + authenticating: auth, + safeModeLevelResolver: { _ in level }, + forcesWriteResolver: { _ in false }, + auditLog: ExecutionAuditLog( + fileURL: FileManager.default.temporaryDirectory + .appendingPathComponent("structure-save-audit-\(UUID().uuidString).json") + ) + ) + } + + private static func asksForConfirmation(_ level: SafeModeLevel, isDestructive: Bool) -> Bool { + switch level { + case .silent: isDestructive + case .alert, .alertFull, .safeMode, .safeModeFull: true + case .readOnly: false + } + } + + private static func asksForAuthentication(_ level: SafeModeLevel) -> Bool { + level == .safeMode || level == .safeModeFull + } + + // MARK: - ALTER saves + + @Test("Every ALTER save asks at most once, and a destructive one asks at every level but Read-Only") + func alterSavesAskOnce() async throws { + let generator = SchemaStatementGenerator(tableName: "users", pluginDriver: MySQLShapedDDLDriver()) + for save in Self.saves { + let statements = try generator.generate(changes: [save.change]) + let request = DatabaseManager.schemaChangeAuthorizationRequest( + statements, databaseType: .mysql, scope: Self.scope + ) + for level in SafeModeLevel.allCases { + let confirm = StubConfirming(answer: true) + let auth = StubAuthenticating(answer: true) + let decision = await Self.gate(level, confirm: confirm, auth: auth).authorize(request) + let label = "\(save.name) at \(level.rawValue)" + + guard level != .readOnly else { + #expect(!decision.isAuthorized, "\(label) must be refused") + #expect(confirm.callCount == 0, "\(label) must be refused without asking") + #expect(auth.callCount == 0, "\(label) must be refused without Touch ID") + continue + } + let asks = Self.asksForConfirmation(level, isDestructive: save.isDestructive) + #expect(decision.isAuthorized, "\(label) must run once answered") + #expect(confirm.callCount == (asks ? 1 : 0), "\(label) asked \(confirm.callCount) times") + #expect(auth.callCount == (Self.asksForAuthentication(level) ? 1 : 0), "\(label) Touch ID") + if asks { + #expect(confirm.lastDestructive == save.isDestructive, "\(label) warning") + } + } + } + } + + /// `CHANGE COLUMN .. NOT NULL`, a type change and an added `CHECK` read as plain writes, so the + /// gate can only see what they risk if the statements say so. A dropped index is destructive by + /// its own `DROP` and needs no mark. + @Test("A change that can lose or refuse existing rows marks every statement it generates") + func dataLossIsStampedOnTheStatements() throws { + let stamped: Set = ["drop column", "type change", "NOT NULL", "add check constraint", "primary key change"] + let generator = SchemaStatementGenerator(tableName: "users", pluginDriver: MySQLShapedDDLDriver()) + for save in Self.saves { + let statements = try generator.generate(changes: [save.change]) + let expected = stamped.contains(save.name) + #expect(!statements.isEmpty) + #expect( + statements.allSatisfy { $0.isDestructive == expected }, + "\(save.name) statements must be marked \(expected)" + ) + } + } + + @Test("A Cancel at the gate's sheet comes back as a Cancel, not a refusal") + func cancelAtTheSheetIsACancel() async throws { + let generator = SchemaStatementGenerator(tableName: "users", pluginDriver: MySQLShapedDDLDriver()) + let statements = try generator.generate(changes: [.deleteColumn(Self.column("email"))]) + let request = DatabaseManager.schemaChangeAuthorizationRequest( + statements, databaseType: .mysql, scope: Self.scope + ) + let decision = await Self.gate( + .silent, + confirm: StubConfirming(answer: false), + auth: StubAuthenticating(answer: true) + ).authorize(request) + + let error = try #require(decision.denialError) + #expect(StructureApplyFailure(error) == .cancelledByUser) + } + + @Test("A declined Touch ID is a refusal that says why, not a quiet Cancel") + func declinedAuthenticationIsARefusal() async throws { + let generator = SchemaStatementGenerator(tableName: "users", pluginDriver: MySQLShapedDDLDriver()) + let statements = try generator.generate(changes: [.deleteColumn(Self.column("email"))]) + let request = DatabaseManager.schemaChangeAuthorizationRequest( + statements, databaseType: .mysql, scope: Self.scope + ) + let decision = await Self.gate( + .safeMode, + confirm: StubConfirming(answer: true), + auth: StubAuthenticating(answer: false) + ).authorize(request) + + let error = try #require(decision.denialError) + let failure = StructureApplyFailure(error) + #expect(failure.outcome == .refused) + #expect(failure.message != nil) + #expect(!failure.reportsFailure) + } + + // MARK: - Rebuilds + + private static func plan(cost: PluginColumnReorderCost) -> PluginColumnReorderPlan { + switch cost { + case .metadataOnly: + PluginColumnReorderPlan( + statements: ["ALTER TABLE `users` MODIFY COLUMN `email` VARCHAR(255) AFTER `id`"], + cost: .metadataOnly, + verifications: [] + ) + default: + PluginColumnReorderPlan( + statements: [ + "CREATE TABLE \"_album_new\" (\"id\" INTEGER PRIMARY KEY, \"artist_id\" INTEGER)", + "INSERT INTO \"_album_new\" SELECT \"id\", \"artist_id\" FROM \"album\"", + "DROP TABLE \"album\"", + "ALTER TABLE \"_album_new\" RENAME TO \"album\"" + ], + isTransactional: true, + cost: .tableRebuild, + verifications: [] + ) + } + } + + @Test("A review that can run the rebuild warns of data loss ahead of its caveats, and a preview does not") + func runnableRebuildReviewWarnsOfDataLoss() { + let action = TableRebuildReviewRequest.Action( + title: "Apply", operationDescription: "Apply Schema Changes", perform: {} + ) + let rebuild = Self.plan(cost: .tableRebuild) + let caveat = "Triggers on album are not recreated." + let withCaveat = PluginColumnReorderPlan( + statements: rebuild.statements, isTransactional: true, cost: .tableRebuild, caveats: [caveat], verifications: [] + ) + let dataWarning = OperationConfirmationPrompt.destructiveDataWarning + + let runnable = TableRebuildReviewRequest(tableName: "album", scope: Self.scope, plan: rebuild, action: action) + #expect(runnable.warning == dataWarning) + + let runnableWithCaveat = TableRebuildReviewRequest( + tableName: "album", scope: Self.scope, plan: withCaveat, action: action + ) + #expect(runnableWithCaveat.warning == "\(dataWarning) \(caveat)") + + let preview = TableRebuildReviewRequest(tableName: "album", scope: Self.scope, plan: rebuild, action: nil) + #expect(preview.warning == nil) + } + + @Test("A rebuild the review sheet confirmed is not confirmed again, and still asks for Touch ID") + func reviewedRebuildIsConfirmedOnce() async { + let request = StructureRebuildPlanRunner.authorizationRequest( + plan: Self.plan(cost: .tableRebuild), + scope: Self.scope, + databaseType: .sqlite, + operationDescription: "Apply Schema Changes", + isConfirmationPreCleared: true + ) + for level in SafeModeLevel.allCases { + let confirm = StubConfirming(answer: false) + let auth = StubAuthenticating(answer: true) + let decision = await Self.gate(level, confirm: confirm, auth: auth).authorize(request) + + #expect(confirm.callCount == 0, "a reviewed rebuild at \(level.rawValue) was confirmed again") + guard level != .readOnly else { + #expect(!decision.isAuthorized, "Read-Only must still refuse a reviewed rebuild") + continue + } + #expect(decision.isAuthorized, "a reviewed rebuild at \(level.rawValue) must run") + #expect(auth.callCount == (Self.asksForAuthentication(level) ? 1 : 0), "Touch ID at \(level.rawValue)") + } + } + + @Test("A reorder that runs on the drop, with no review, is confirmed by the gate at the levels that ask") + func unreviewedReorderIsConfirmedPerLevel() async { + let request = StructureRebuildPlanRunner.authorizationRequest( + plan: Self.plan(cost: .metadataOnly), + scope: Self.scope, + databaseType: .mysql, + operationDescription: "Reorder Columns", + isConfirmationPreCleared: false + ) + for level in SafeModeLevel.allCases where level != .readOnly { + let confirm = StubConfirming(answer: true) + let auth = StubAuthenticating(answer: true) + let decision = await Self.gate(level, confirm: confirm, auth: auth).authorize(request) + + #expect(decision.isAuthorized) + let asks = Self.asksForConfirmation(level, isDestructive: false) + #expect(confirm.callCount == (asks ? 1 : 0), "a reorder at \(level.rawValue) asked \(confirm.callCount) times") + } + } + + @Test("A review that can run its script reads as the confirmation it is") + func runnableReviewIsTheConfirmation() { + let request = TableRebuildReviewRequest( + tableName: "album", + scope: Self.scope, + plan: Self.plan(cost: .tableRebuild), + action: TableRebuildReviewRequest.Action( + title: "Apply and Rebuild", + operationDescription: "Apply Schema Changes", + perform: {} + ) + ) + + #expect(request.confirmationTitle == "Apply Schema Changes") + #expect(request.showsStatementsVerbatim) + #expect(request.confirmationSubtitle(connectionName: "Chinook")?.contains("Chinook") == true) + } + + @Test("A preview, and a script the app will not run, keep the preview heading") + func previewIsNotAConfirmation() { + let preview = TableRebuildReviewRequest( + tableName: "album", + scope: Self.scope, + plan: Self.plan(cost: .tableRebuild), + action: nil + ) + let unrunnable = TableRebuildReviewRequest( + tableName: "album", + scope: Self.scope, + plan: PluginColumnReorderPlan( + statements: Self.plan(cost: .tableRebuild).statements, + cost: .tableRebuild, + isRunnable: false, + verifications: [] + ), + action: TableRebuildReviewRequest.Action( + title: "Apply and Rebuild", + operationDescription: "Apply Schema Changes", + perform: {} + ) + ) + + for request in [preview, unrunnable] { + #expect(request.confirmationTitle == nil) + #expect(!request.showsStatementsVerbatim) + #expect(request.confirmationSubtitle(connectionName: "Chinook") == nil) + } + } + + // MARK: - How a save that did not apply ends + + @Test("A Cancel leaves the edits staged and shows nothing") + func cancelIsQuiet() { + let failure = StructureApplyFailure(ExecutionGateError.cancelledByUser("Operation cancelled by user")) + + #expect(failure == .cancelledByUser) + #expect(failure.outcome == .refused) + #expect(failure.message == nil) + #expect(!failure.reportsFailure) + } + + @Test("A Safe Mode refusal leaves the edits staged and says why, and is not a failed operation") + func refusalExplainsItself() { + let failure = StructureApplyFailure(ExecutionGateError.denied("Safe Mode is read-only")) + + #expect(failure.outcome == .refused) + #expect(failure.message == "Safe Mode is read-only") + #expect(!failure.reportsFailure) + } + + @Test("A statement the server rejected is a failed save") + func serverRejectionIsAFailure() { + let failure = StructureApplyFailure(ServerRejection()) + + #expect(failure.outcome == .failed("Duplicate column name 'notes'")) + #expect(failure.message == "Duplicate column name 'notes'") + #expect(failure.reportsFailure) + } +} diff --git a/TableProTests/Views/Structure/StructureSavePlanTests.swift b/TableProTests/Views/Structure/StructureSavePlanTests.swift index 66cff5102e..16131f5de6 100644 --- a/TableProTests/Views/Structure/StructureSavePlanTests.swift +++ b/TableProTests/Views/Structure/StructureSavePlanTests.swift @@ -375,7 +375,8 @@ struct StructureSavePlanTests { let review = try #require(coordinator.tableRebuildRequest) #expect(review.scriptStatements == preview.scriptStatements) - #expect(review.warning == preview.warning) + let dataWarning = isRunnable ? [OperationConfirmationPrompt.destructiveDataWarning] : [] + #expect(review.warning == (dataWarning + plan.caveats).joined(separator: " ")) #expect((review.runnableAction != nil) == isRunnable) } diff --git a/TableProUITests/StructureSaveConfirmationUITests.swift b/TableProUITests/StructureSaveConfirmationUITests.swift new file mode 100644 index 0000000000..4ebeaedc31 --- /dev/null +++ b/TableProUITests/StructureSaveConfirmationUITests.swift @@ -0,0 +1,217 @@ +// +// StructureSaveConfirmationUITests.swift +// TableProUITests +// + +import XCTest + +/// A Structure save asks once. An `ALTER` save is confirmed by the execution gate's sheet, which +/// shows the statements it runs; a table rebuild is confirmed by its review sheet, which shows the +/// script. The editor used to ask first with an alert of its own and the gate then asked again, and +/// a rebuild's review was followed by the gate's sheet stacked over it, so one decision took two +/// answers, and three with Touch ID. +/// +/// SQLite runs every case here: it drops a column with `ALTER`, and it changes a foreign key or a +/// column's position by rebuilding the table. +final class StructureSaveConfirmationUITests: UITestCase { + func testDroppingAColumnAtAlertAsksOnce() throws { + let (app, database) = try openNotesStructure(connection: "Alerted", safeModeLevel: "alert") + let window = app.windows.firstMatch + + stageColumnDrop(in: app, window: window) + app.typeKey("s", modifierFlags: .command) + + let execute = gateExecuteButton(in: window) + XCTAssertTrue(execute.waitToExist(timeout: 20), "The first sheet must be the gate's, with the statement") + XCTAssertFalse(window.sheets.buttons["Apply Changes"].exists, "The editor must not ask before the gate does") + execute.click() + + XCTAssertTrue( + waitForPredicate(timeout: 30) { + sqliteStrings("SELECT name FROM pragma_table_xinfo('notes')", in: database) == ["body"] + }, + "One answer must be enough for the drop to reach the file" + ) + XCTAssertFalse(window.sheets.firstMatch.exists, "Nothing is left to answer") + } + + func testCancellingTheGateKeepsTheDropStagedAndShowsNothingElse() throws { + let (app, database) = try openNotesStructure(connection: "Cancelled", safeModeLevel: "silent") + let window = app.windows.firstMatch + + stageColumnDrop(in: app, window: window) + app.typeKey("s", modifierFlags: .command) + + let execute = gateExecuteButton(in: window) + XCTAssertTrue(execute.waitToExist(timeout: 20), "Dropping a column must ask once, at the gate") + let cancel = window.sheets.buttons["Cancel"].firstMatch + XCTAssertTrue(cancel.waitToExist(timeout: 5), "The gate's sheet must offer Cancel") + cancel.click() + + XCTAssertTrue( + waitForPredicate(timeout: 10) { !window.sheets.firstMatch.exists }, + "Cancel must close the gate's sheet" + ) + XCTAssertFalse( + waitForPredicate(timeout: 3) { window.sheets.firstMatch.exists }, + "A Cancel is the user's own choice and must not be answered with an error sheet" + ) + XCTAssertEqual( + sqliteStrings("SELECT name FROM pragma_table_xinfo('notes')", in: database), + ["tag", "body"], + "Nothing may run after a Cancel" + ) + + app.typeKey("s", modifierFlags: .command) + XCTAssertTrue( + gateExecuteButton(in: window).waitToExist(timeout: 20), + "The drop must still be staged, so saving again asks again" + ) + window.sheets.buttons["Cancel"].firstMatch.click() + } + + func testRemovingAForeignKeyRunsFromItsReviewWithoutAskingAgain() throws { + let databases = try seedSQLiteSession( + connectionNames: ["Rebuild"], + databaseSQL: """ + CREATE TABLE artist (id INTEGER PRIMARY KEY); + CREATE TABLE album (id INTEGER PRIMARY KEY, artist_id INTEGER REFERENCES artist (id)); + """ + ) + let database = try XCTUnwrap(databases.first) + let app = try launchApp() + let window = app.windows.firstMatch + + let table = objectBrowserRow("album", in: window) + XCTAssertTrue(table.waitToExist(timeout: 60), "The restored connection must list album") + clickAtCenter(table) + showStructure(in: app, window: window) + + let foreignKeys = window.radioGroups["structure-tab-picker"].firstMatch + .radioButtons + .matching(NSPredicate(format: "label BEGINSWITH %@", "Foreign Keys")) + .firstMatch + XCTAssertTrue(foreignKeys.waitToExist(timeout: 20), "SQLite has foreign keys, so the tab must be offered") + foreignKeys.click() + + let grid = window.tables.matching(identifier: "data-grid").firstMatch + XCTAssertTrue(grid.waitToExist(timeout: 30), "The Foreign Keys tab must draw its grid") + XCTAssertTrue( + waitForPredicate(timeout: 30) { + grid.frame.width > 0 && grid.frame.height > 0 && grid.tableRows.count == 1 + }, + "album has one foreign key, so the grid must list one row" + ) + gridPoint(in: grid, of: window, dy: 40).click() + + let remove = window.buttons["structure-footer-remove"].firstMatch + XCTAssertTrue(remove.waitToExist(timeout: 20), "The Foreign Keys tab must offer a remove button") + XCTAssertTrue(waitForPredicate(timeout: 10) { remove.isEnabled }, "Removing the selected key must be offered") + remove.click() + + app.typeKey("s", modifierFlags: .command) + + let apply = window.sheets.buttons["sql-review-execute"].firstMatch + XCTAssertTrue(apply.waitToExist(timeout: 30), "Changing a SQLite foreign key must show the rebuild first") + XCTAssertEqual(apply.label, "Apply and Rebuild") + let sheetText = texts(in: window.sheets.firstMatch) + XCTAssertTrue( + sheetText.contains("Runs on 'Rebuild'"), + "The review is the confirmation, so it must name the connection, got: \(sheetText)" + ) + apply.click() + + XCTAssertTrue( + waitForPredicate(timeout: 30) { + sqliteStrings("SELECT \"table\" FROM pragma_foreign_key_list('album')", in: database).isEmpty + }, + "The review's own button must run the rebuild, with no second sheet to answer" + ) + XCTAssertFalse(gateExecuteButton(in: window).exists, "The gate must not ask again after the review") + } + + func testMovingAColumnRunsFromItsReviewWithoutAskingAgain() throws { + let (app, database) = try openNotesStructure(connection: "Reorder", safeModeLevel: "silent") + let window = app.windows.firstMatch + + let grid = window.tables.matching(identifier: "data-grid").firstMatch + let target = gridPoint(in: grid, of: window, dy: 40) + target.click() + target.rightClick() + let moveDown = app.menuItems["Move Column Down"].firstMatch + XCTAssertTrue(moveDown.waitToExist(timeout: 15), "The first column row must offer Move Column Down") + moveDown.click() + + let rebuild = window.sheets.buttons["sql-review-execute"].firstMatch + XCTAssertTrue(rebuild.waitToExist(timeout: 30), "SQLite moves a column by rebuilding, so it shows the script") + XCTAssertEqual(rebuild.label, "Rebuild Table") + rebuild.click() + + XCTAssertTrue( + waitForPredicate(timeout: 30) { + sqliteStrings("SELECT name FROM pragma_table_xinfo('notes')", in: database) == ["body", "tag"] + }, + "The review's own button must run the rebuild, with no second sheet to answer" + ) + XCTAssertFalse(gateExecuteButton(in: window).exists, "The gate must not ask again after the review") + } + + // MARK: - Helpers + + /// Seeds `notes (tag TEXT, body TEXT)` at `safeModeLevel`, launches, and opens its Columns tab. + private func openNotesStructure( + connection: String, + safeModeLevel: String + ) throws -> (XCUIApplication, URL) { + let databases = try seedSQLiteSession( + connectionNames: [connection], + databaseSQL: "CREATE TABLE notes (tag TEXT, body TEXT); INSERT INTO notes VALUES ('a', 'b');", + safeModeLevel: safeModeLevel + ) + let database = try XCTUnwrap(databases.first) + let app = try launchApp() + let window = app.windows.firstMatch + + let table = objectBrowserRow("notes", in: window) + XCTAssertTrue(table.waitToExist(timeout: 60), "The restored connection must list notes") + clickAtCenter(table) + + showStructure(in: app, window: window) + let grid = window.tables.matching(identifier: "data-grid").firstMatch + XCTAssertTrue(grid.waitToExist(timeout: 30), "The structure editor must draw its column grid") + XCTAssertTrue( + waitForPredicate(timeout: 30) { + grid.frame.width > 0 && grid.frame.height > 0 && grid.tableRows.count == 2 + }, + "notes has two columns, so the grid must list two rows" + ) + return (app, database) + } + + private func stageColumnDrop(in app: XCUIApplication, window: XCUIElement) { + let grid = window.tables.matching(identifier: "data-grid").firstMatch + gridPoint(in: grid, of: window, dy: 40).click() + XCTAssertTrue( + waitForPredicate(timeout: 10) { grid.tableRows.allElementsBoundByIndex.contains { $0.isSelected } }, + "The click must select a column row" + ) + let remove = window.buttons["structure-footer-remove"].firstMatch + XCTAssertTrue(remove.waitToExist(timeout: 20), "The Columns tab must offer a remove button") + XCTAssertTrue(waitForPredicate(timeout: 10) { remove.isEnabled }, "Removing the selected column must be offered") + remove.click() + } + + /// The gate's confirming button. The rebuild review's shares its identifier, so the label is + /// what tells the two apart. + private func gateExecuteButton(in window: XCUIElement) -> XCUIElement { + window.sheets.buttons + .matching(NSPredicate(format: "identifier == %@ AND label == %@", "sql-review-execute", "Execute")) + .firstMatch + } + + private func texts(in element: XCUIElement) -> String { + element.staticTexts.allElementsBoundByIndex + .flatMap { [$0.label, ($0.value as? String) ?? ""] } + .joined(separator: " ") + } +} diff --git a/TableProUITests/StructureTypelessColumnUITests.swift b/TableProUITests/StructureTypelessColumnUITests.swift index 9b05161cf1..c42c00993e 100644 --- a/TableProUITests/StructureTypelessColumnUITests.swift +++ b/TableProUITests/StructureTypelessColumnUITests.swift @@ -58,15 +58,16 @@ final class StructureTypelessColumnUITests: UITestCase { text.contains("must have a name and a data type"), "A column with no type must not stop the save, got: \(text)" ) - let apply = sheet.buttons["Apply Changes"].firstMatch - XCTAssertTrue(apply.waitToExist(timeout: 5), "The drop must be offered for confirmation, got: \(text)") - apply.click() + /// The first sheet is the execution gate's, and it is the only one. The editor used to ask + /// first with an alert of its own, so the same decision took two answers. let execute = window.sheets.buttons["sql-review-execute"].firstMatch XCTAssertTrue( execute.waitToExist(timeout: 20), - "Apply Changes hands the drop to the execution gate, which shows the statement before it runs" + "The first sheet must be the gate's, showing the statement it runs, got: \(text)" ) + XCTAssertEqual(execute.label, "Execute") + XCTAssertFalse(window.sheets.buttons["Apply Changes"].exists, "The editor must not ask before the gate does") let statement = (window.sheets.textViews.firstMatch.value as? String) ?? "" XCTAssertTrue(statement.contains("DROP COLUMN"), "The gate must be showing the drop, got: \(statement)") execute.click() @@ -81,5 +82,6 @@ final class StructureTypelessColumnUITests: UITestCase { waitForPredicate(timeout: 30) { grid.tableRows.count == 1 }, "The grid must list the one column the save kept" ) + XCTAssertFalse(window.sheets.firstMatch.exists, "One answer is the whole confirmation") } } diff --git a/TableProUITests/Support/SeededSQLiteSession.swift b/TableProUITests/Support/SeededSQLiteSession.swift index d4f27812df..b39c869ece 100644 --- a/TableProUITests/Support/SeededSQLiteSession.swift +++ b/TableProUITests/Support/SeededSQLiteSession.swift @@ -12,9 +12,14 @@ import XCTest internal extension UITestCase { /// Each connection gets its own database file built from `databaseSQL`, and `tabsEach` query /// tabs, so nothing restored depends on a table's rows loading. Returns the database files in - /// connection order. + /// connection order. Every connection runs at `safeModeLevel`, Silent unless a test asks. @discardableResult - func seedSQLiteSession(connectionNames: [String], databaseSQL: String, tabsEach: Int = 1) throws -> [URL] { + func seedSQLiteSession( + connectionNames: [String], + databaseSQL: String, + tabsEach: Int = 1, + safeModeLevel: String = "silent" + ) throws -> [URL] { let root = try XCTUnwrap(sandboxRoot, "setUpWithError did not prepare a sandbox") let supportDirectory = root.appendingPathComponent("TablePro", isDirectory: true) let tabStateDirectory = supportDirectory.appendingPathComponent("TabState", isDirectory: true) @@ -27,7 +32,13 @@ internal extension UITestCase { let id = UUID().uuidString let databaseURL = root.appendingPathComponent("restored-\(index).sqlite") makeDatabase(at: databaseURL, sql: databaseSQL) - connections.append(connectionPayload(id: id, name: name, databasePath: databaseURL.path, sortOrder: index)) + connections.append(connectionPayload( + id: id, + name: name, + databasePath: databaseURL.path, + sortOrder: index, + safeModeLevel: safeModeLevel + )) connectionIds.append(id) databaseURLs.append(databaseURL) try writeJSON( @@ -63,7 +74,13 @@ internal extension UITestCase { return values } - private func connectionPayload(id: String, name: String, databasePath: String, sortOrder: Int) -> [String: Any] { + private func connectionPayload( + id: String, + name: String, + databasePath: String, + sortOrder: Int, + safeModeLevel: String + ) -> [String: Any] { [ "id": id, "name": name, @@ -78,6 +95,7 @@ internal extension UITestCase { "sshAuthMethod": "password", "sshPrivateKeyPath": "", "sortOrder": sortOrder, + "safeModeLevel": safeModeLevel, ] } diff --git a/docs/features/safe-mode.mdx b/docs/features/safe-mode.mdx index 91efdc92d6..b91768562c 100644 --- a/docs/features/safe-mode.mdx +++ b/docs/features/safe-mode.mdx @@ -24,7 +24,7 @@ Four things the table cannot carry. The confirmation dialog shows the whole stat Confirmation dialog over a query tab, showing a fourteen line UPDATE with Cancel and Execute Confirmation dialog over a query tab, showing a fourteen line UPDATE with Cancel and Execute - **Silent** is not a free pass: `DROP`, `TRUNCATE`, and a `DELETE` with no `WHERE` still raise the built-in dangerous query warning even there. And **Read-Only** goes past queries to the interface itself, disabling inline cell editing, adding, deleting and duplicating rows, table truncate and drop, and import. + **Silent** is not a free pass: `DROP`, `TRUNCATE`, and a `DELETE` with no `WHERE` still raise the built-in dangerous query warning even there. So does a [Structure](/features/table-structure#saving-changes) save that drops a column, changes a column's type, adds NOT NULL, changes the primary key, or adds or rewrites a check constraint. And **Read-Only** goes past queries to the interface itself, disabling inline cell editing, adding, deleting and duplicating rows, table truncate and drop, and import. ## Connections that are always read-only diff --git a/docs/features/table-structure.mdx b/docs/features/table-structure.mdx index d89ae349ca..cc8fc65c8d 100644 --- a/docs/features/table-structure.mdx +++ b/docs/features/table-structure.mdx @@ -202,9 +202,11 @@ SQLite is the one engine where listing and editing part company. The tab appears [Change Tracking](/features/change-tracking) covers the queue, undo (`Cmd+Z`) and redo (`Cmd+Shift+Z`). A save runs on the tab's own connection, database, and schema, the ones it was opened on, and never moves the sidebar or the toolbar. -- **Save Changes** (`Cmd+S` or the toolbar checkmark) applies the queue. Changes that can lose data, dropping a column, changing a type, adding NOT NULL, changing the primary key, first show a confirmation listing each one. +- **Save Changes** (`Cmd+S` or the toolbar checkmark) applies the queue. - **Preview SQL** (`Cmd+Shift+P`) shows the statements **Save Changes** would run, on the tab's own schema, and runs none of them. A save that recreates the table previews as its rebuild script, with what the rebuild cannot carry over. +A save asks at most once, on a sheet showing the statements it will run. One that removes a column, an index, a key or a check constraint, changes a column's type, adds NOT NULL, changes the primary key, or adds or rewrites a check constraint asks even at the **Silent** [Safe Mode](/features/safe-mode) level. Any other save asks at **Alert** and above. A save that recreates the table asks on its rebuild script instead. At **Safe Mode** and **Safe Mode (Full)**, Touch ID follows the answer. + The queue outlives everything short of an explicit discard: closing the tab, closing the window, quitting, and **Refresh** all ask first. A save that never reaches the server leaves the tab open with its queue intact.