From 45b6d395ef9fb61c9530fd9bf3cafaace1574818 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 4 Nov 2026 01:56:07 +0700 Subject: [PATCH 1/4] fix(app): keep sheet commands from replacing an active dialog --- ...itViewController+DatabaseMenuActions.swift | 27 +- ...SplitViewController+DebugMenuActions.swift | 5 +- ...inSplitViewController+MenuValidation.swift | 58 +++- .../MainWindowToolbar+Context.swift | 29 +- .../Toolbar/ToolbarContextResolver.swift | 90 ++---- apple/TablePro/Models/UI/ToolbarContext.swift | 53 ++-- .../MainContentCoordinator+Rewind.swift | 9 +- .../MainContentCoordinator+RowImport.swift | 4 +- .../MainContentCoordinator+SheetGate.swift | 26 ++ ...ContentCommandActions+DataDictionary.swift | 1 + ...ontentCommandActions+DatabaseObjects.swift | 35 ++- .../MainContentCommandActions+Documents.swift | 4 +- .../MainContentCommandActions+Structure.swift | 1 + .../Main/MainContentCommandActions.swift | 15 + .../Views/Main/SheetCommandGateTests.swift | 270 ++++++++++++++++++ 15 files changed, 455 insertions(+), 172 deletions(-) create mode 100644 apple/TablePro/Views/Main/Extensions/MainContentCoordinator+SheetGate.swift create mode 100644 apple/TableProTests/Views/Main/SheetCommandGateTests.swift diff --git a/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+DatabaseMenuActions.swift b/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+DatabaseMenuActions.swift index dbeb02d0b8..ce3c3232e6 100644 --- a/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+DatabaseMenuActions.swift +++ b/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+DatabaseMenuActions.swift @@ -12,13 +12,7 @@ extension MainSplitViewController { openConnectionSwitcher() } - /// Presented by the window, not by the selected connection's actions like everything else in - /// this file. The switcher lists every connection the app has open and every one the user has - /// saved, and it reads none of that from a session, so nothing about it belongs to the - /// connection on screen. That connection going away is exactly when a user reaches for it, and - /// routing it through `commandActions` made it do nothing at that moment: `releaseSession` - /// nils the actions, so Switch Connection and Control-Command-C were dead over the very pane - /// telling the user to reconnect or pick another connection. + // The window owns switching because a disconnected workspace has no command actions. func openConnectionSwitcher() { view.window?.makeFirstResponder(nil) switcherPresenter.present( @@ -47,8 +41,7 @@ extension MainSplitViewController { return coordinator.canMoveTabToConnection(tabId) } - /// The source is read now, not at pick time: the pick can arrive after the window has - /// switched to another connection. + // Capture the source now because the selected workspace can change before the pick. func presentMoveTabPicker(tabId: UUID) { guard let source = workspace(holding: tabId), let coordinator = source.sessionState?.coordinator, @@ -91,20 +84,20 @@ extension MainSplitViewController { commandActions?.openDatabaseSwitcher() } - /// The full chooser for the inner scope, which the checked Schema submenu beside it cannot - /// replace: only the popover searches, favourites, drops and exports. + // The full chooser stays useful when the schema submenu cannot fit every entry. @objc func openSchemaSwitcher(_ sender: Any?) { commandActions?.openScopeSwitcher(.schema) } @objc func createSchema(_ sender: Any?) { - commandActions?.coordinator?.createSchema(database: nil) + guard let coordinator = commandActions?.coordinator, coordinator.admitsSheetCommand() else { return } + coordinator.createSchema(database: nil) } - /// Edits the schema the connection is browsing. The sidebar's own item edits the row that was - /// clicked; this one is the route for a sidebar shape that draws no schema row at all. + // The browsed schema, for a sidebar shape that draws no schema row to right-click. @objc func editCurrentSchema(_ sender: Any?) { guard let coordinator = commandActions?.coordinator, + coordinator.admitsSheetCommand(), let schema = coordinator.toolbarState.currentSchema ?? DatabaseManager.shared.session(for: coordinator.connection.id)?.browseSchema else { return } @@ -119,16 +112,14 @@ extension MainSplitViewController { ) } - /// Reached through the workspace's coordinator rather than `commandActions`, which exists only - /// once the browse content has appeared. A window opened straight into Agent mode never shows - /// that content, so its Safe Mode list had no checkmark and no entry in it did anything. + // Agent mode may never mount browse command actions, so use the workspace coordinator. @objc func setSafeModeLevel(_ sender: Any?) { guard let raw = (sender as? NSMenuItem)?.representedObject as? String, let level = SafeModeLevel(rawValue: raw) else { return } workspaces.selected?.sessionState?.coordinator.setSafeModeLevel(level) } - /// What the Safe Mode list offers for the connection on screen, or nil with no session behind it. + // What the Safe Mode list offers for the connection on screen, or nil with no session behind it. var safeModeStatus: SafeModeStatus? { workspaces.selected?.sessionState?.coordinator.safeModeStatus } diff --git a/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+DebugMenuActions.swift b/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+DebugMenuActions.swift index db9fe8340e..728f162130 100644 --- a/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+DebugMenuActions.swift +++ b/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+DebugMenuActions.swift @@ -23,7 +23,10 @@ extension MainSplitViewController { ] @objc func chooseRoutineToDebug(_ sender: Any?) { - guard let coordinator = routineDebugCoordinator, coordinator.canChooseRoutineToDebug() else { return } + guard let coordinator = routineDebugCoordinator, + coordinator.admitsSheetCommand(), + coordinator.canChooseRoutineToDebug() + else { return } coordinator.chooseRoutineToDebug() } diff --git a/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+MenuValidation.swift b/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+MenuValidation.swift index 83e9aad90d..3083a75b24 100644 --- a/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+MenuValidation.swift +++ b/apple/TablePro/Core/Services/Infrastructure/MainSplitViewController+MenuValidation.swift @@ -138,6 +138,8 @@ struct MenuValidationContext: Equatable { // Filled only when a Debug menu item is validated; the menu exists only while the module is on. var routineDebug = RoutineDebugMenuState() var canOpenLiveMessages = false + var canDisconnect = false + var isPresentingSheet = false // The validation and the window's handler both read it, so a lit Use Selection for Find never lands on an editor with no selection. var usesGridSelectionForFind: Bool { @@ -149,6 +151,7 @@ extension MainSplitViewController: NSMenuItemValidation { // Nil: not this window's call. An unhandled selector stays lit (Clear Selection did); MenuValidationCoverageTests catches it. static func resolvedEnablement(_ selector: Selector, context: MenuValidationContext) -> Bool? { guard let answer = ownEnablement(selector, context: context) else { return nil } + if context.isPresentingSheet, sheetCommandSelectors.contains(selector) { return false } guard context.isAgentMode else { return answer } return answer && agentModeSelectors.contains(selector) } @@ -202,6 +205,8 @@ extension MainSplitViewController: NSMenuItemValidation { return context.isConnected case #selector(closeConnection(_:)): return context.hasSelectedWorkspace + case #selector(requestDisconnect): + return context.canDisconnect // Not `isConnected`: the switcher needs nothing from the session and is how the user leaves a connection that stopped working. case #selector(switchConnection(_:)): return context.hasSelectedWorkspace @@ -461,6 +466,7 @@ extension MainSplitViewController: NSMenuItemValidation { static let agentModeSelectors: Set = [ #selector(switchConnection(_:)), #selector(closeConnection(_:)), + #selector(requestDisconnect), #selector(setSafeModeLevel(_:)), #selector(releaseFileLock(_:)), #selector(openContainerSwitcher(_:)), @@ -489,6 +495,49 @@ extension MainSplitViewController: NSMenuItemValidation { #selector(redo(_:)), ] + // Each presents a sheet on this window; a window with a sheet attached takes no second one. + static let sheetCommandSelectors: Set = [ + #selector(saveDocument(_:)), + #selector(saveDocumentAs(_:)), + #selector(discardChanges(_:)), + #selector(exportTables(_:)), + #selector(exportQueryResults(_:)), + #selector(exportDataDictionary(_:)), + #selector(importData(_:)), + #selector(importDataFormat(_:)), + #selector(backupDatabase(_:)), + #selector(restoreDatabase(_:)), + #selector(restorePreviousValues(_:)), + #selector(insertDocument(_:)), + #selector(editDocument(_:)), + #selector(previewSQL(_:)), + #selector(saveAsFavorite(_:)), + #selector(createNewDatabase(_:)), + #selector(createSchema(_:)), + #selector(editCurrentSchema(_:)), + #selector(copyObjectsToDatabase(_:)), + #selector(duplicateCurrentDatabase(_:)), + #selector(searchDatabase(_:)), + #selector(editObjectComment(_:)), + #selector(setSequenceNextValue(_:)), + #selector(editSequence(_:)), + #selector(runMaintenanceOperation(_:)), + #selector(buildJoinQuery(_:)), + #selector(chooseRoutineToDebug(_:)), + #selector(refreshMaterializedView(_:)), + #selector(releaseFileLock(_:)), + #selector(truncateTable(_:)), + #selector(delete(_:)), + #selector(closeConnection(_:)), + #selector(requestDisconnect), + #selector(closeOtherTabs(_:)), + #selector(closeTabsForOtherContainers(_:)), + #selector(closeAllTabs(_:)), + #selector(closeAgentSession(_:)), + #selector(deleteAgentSession(_:)), + #selector(clearAIConversations(_:)), + ] + // A command this window does not own stays enabled, which keeps `performClose:` and the system's own items working. static func isEnabled(_ selector: Selector, context: MenuValidationContext) -> Bool { resolvedEnablement(selector, context: context) ?? true @@ -597,7 +646,9 @@ extension MainSplitViewController: NSMenuItemValidation { canFocusAssistant: canFocusAssistant, canMoveTabToConnection: canMoveSelectedTabToConnection, hasOtherWindowTabs: hasOtherWindowTabs, - canToggleWorkspaceRail: canToggleWorkspaceRail + canToggleWorkspaceRail: canToggleWorkspaceRail, + canDisconnect: canDisconnect, + isPresentingSheet: isViewLoaded && view.window?.attachedSheet != nil ) } return MenuValidationContext( @@ -701,7 +752,9 @@ extension MainSplitViewController: NSMenuItemValidation { canFavoriteActiveDatabase: actions.canFavoriteActiveDatabase, hasDatabaseFilter: actions.hasDatabaseFilter, canFilterSchemas: actions.canFilterSchemas, - hasSchemaFilter: actions.hasSchemaFilter + hasSchemaFilter: actions.hasSchemaFilter, + canDisconnect: canDisconnect, + isPresentingSheet: actions.coordinator?.isPresentingSheet ?? false ) } @@ -714,7 +767,6 @@ extension MainSplitViewController: NSMenuItemValidation { if action == #selector(toggleAssistant(_:)) { return canToggleAssistant } if action == #selector(setResultView(_:)) { return canShowResultView(menuItem) } if action == #selector(setSafeModeLevel(_:)) { return canChooseSafeModeLevel(menuItem) } - if action == #selector(requestDisconnect) { return canDisconnect } if action == #selector(retryConnection) { return canReconnect } if let zoom = FontSizeZoom.editor.enablement(action, typography: AppSettingsManager.shared.typography) { return zoom diff --git a/apple/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Context.swift b/apple/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Context.swift index 904d5d0c84..cfda0d7407 100644 --- a/apple/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Context.swift +++ b/apple/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Context.swift @@ -7,31 +7,17 @@ import AppKit import TableProPluginKit internal extension MainWindowToolbar { - /// The window's own controller, not the connection's coordinator. - /// - /// A workspace that is still connecting, or disconnected, has no `MainContentCoordinator`, so - /// reaching the split controller through one made the window's own commands inert in exactly - /// the state a user reaches for them. + // A disconnected workspace has no coordinator, but its window commands must still work. var host: MainSplitViewController? { windowController ?? coordinator?.splitViewController } - /// The engine the window is showing, read from the workspace's connection record and never from - /// the coordinator. - /// - /// The coordinator goes with the session, so a capability read through it would take the - /// container capsule out of the titlebar and put it back on every dropped connection, which is - /// the titlebar moving on something transient. + // Read the workspace so a dropped session cannot hide the database control. private var databaseType: DatabaseType? { host?.workspaces.selected?.connection?.type ?? coordinator?.connection.type } - /// The eight slow-moving facts, read on their own and never through a whole context. - /// - /// This is what every tab-manager publish pays, and typing publishes one per keystroke, so it - /// reads only what the key holds: the selected tab, the window's mode, four locked reads of the - /// plugin metadata registry and a switch over the engine for the dashboard. Everything - /// transient, and every question that walks the coordinator, is left to `currentContext()`. + // Keep transient state out of the key so typing cannot reshape the toolbar. func currentVisibilityKey() -> ToolbarContext.VisibilityKey { let tab = coordinator?.tabManager.selectedTab let databaseType = self.databaseType @@ -51,11 +37,7 @@ internal extension MainWindowToolbar { ) } - /// Everything the two resolvers are allowed to know, read once per question. - /// - /// `isConnected` is the session's own liveness, which counts a reconnect in progress as up. - /// The health monitor writes `.connecting` on every attempt of a backoff while the window keeps - /// showing the session's tabs and rows, and dimming the row for that would be noise. + // `isConnected` counts a reconnect in progress as up: a backoff writes `.connecting` while the tabs and rows stay on screen. func currentContext() -> ToolbarContext { let host = self.host let state = coordinator?.toolbarState @@ -76,7 +58,8 @@ internal extension MainWindowToolbar { debugCommands: debugger.map { controller in Set(PluginDebugCommand.hostCommands.filter { controller.canPerform($0) }) } ?? [], - canStopDebugging: debugger?.canStop ?? false + canStopDebugging: debugger?.canStop ?? false, + isPresentingSheet: coordinator?.isPresentingSheet ?? false ) } } diff --git a/apple/TablePro/Core/Services/Infrastructure/Toolbar/ToolbarContextResolver.swift b/apple/TablePro/Core/Services/Infrastructure/Toolbar/ToolbarContextResolver.swift index 500b7b548f..24cfb06a8b 100644 --- a/apple/TablePro/Core/Services/Infrastructure/Toolbar/ToolbarContextResolver.swift +++ b/apple/TablePro/Core/Services/Infrastructure/Toolbar/ToolbarContextResolver.swift @@ -6,37 +6,9 @@ import AppKit import TableProPluginKit -/// Which of the connection window's toolbar items a context shows, and which of them answer. -/// -/// Two questions with deliberately different inputs, and keeping them apart is what stops the -/// titlebar reflowing while the user types. -/// -/// `visibility(for:)` and `commitVerb(for:)` take `ToolbarContext.VisibilityKey` or a part of it and -/// nothing else, so the type says what the shape may depend on: the tab kind, the results mode, the -/// content mode and the driver's capabilities. Those change on a tab switch, a mode switch or a -/// connection switch and at no other time, which are the three moments a native app's toolbar is -/// expected to change shape. `isEnabled` carries everything transient, so a staged edit, a running -/// query or a reconnect backoff dims a control and never moves or relabels one. -/// -/// Version-free on purpose. `NSToolbarItem.isHidden` is macOS 15, and the caller is what decides -/// whether to apply the set or fall back to dimming; the answer itself does not depend on the OS. -/// -/// Exhaustive over `TabType` with no `default:` arm. CLAUDE.md's "every switch needs `default:`" -/// rule is about `DatabaseType`, which is an open string-based struct; `TabType` is closed, and a -/// ninth kind must not compile without choosing what its toolbar shows. +// Transient state only dims controls; it never changes the toolbar layout. internal enum ToolbarContextResolver { - /// The identifiers the app may take out of the titlebar, which is exactly the set it puts there. - /// - /// Derived rather than listed, so the rule cannot drift from the toolbar: an item is hideable - /// because the app placed it, and anything else in `NSToolbar.items` was dragged in from the - /// customization palette by the user. That item is opt-in, so it stays where they put it in every - /// context and only dims. Measured on macOS 27 with two palette items inserted into a live - /// toolbar, a pass that hid through this filter wrote neither of them and moved neither. - /// - /// The spaces and tracking separators are in it, and deliberately so. Filtering the standard - /// identifiers out by their `NSToolbar` prefix would also declare the sidebar and inspector - /// toggles unhideable, and a space only ever receives `isHidden = false`, which AppKit returns - /// from early at about 2.5ns a write. + // Keep items the user added from the customization palette visible. internal static let hideableIdentifiers = Set(MainWindowToolbar.defaultItemIdentifiers) // The debug controls belong to their module, so all of them, the palette's steps included, may leave the titlebar @@ -45,24 +17,14 @@ internal enum ToolbarContextResolver { showsDebugItems ? hideableIdentifiers.union(MainWindowToolbar.debugItemIdentifiers) : hideableIdentifiers } - /// What this context takes out of the titlebar, already confined to what may be taken. - /// - /// The confinement lives here rather than at the call site, so it is a property of the answer - /// and a second caller cannot skip it. + // Constrain hiding here so callers cannot remove user-added items. internal static func visibility(for key: ToolbarContext.VisibilityKey) -> ToolbarVisibility { ToolbarVisibility( hidden: contextualHidden(key).intersection(hideableItems(showsDebugItems: key.showsDebugItems)) ) } - /// The commit control's label: the verb its tab commits with. - /// - /// Keyed on the tab kind and never on what is staged. The staged change moves with every edit: - /// a Create Table draft is `.createTable` only while it validates, so a label read from it - /// flipped between Save Changes and Create Table as the user typed, and with labels shown each - /// flip changed the item's width and reflowed the titlebar. The kind moves on a tab switch, - /// when the item set may change anyway. Nothing staged still leaves the control dim, which is - /// `isEnabled`'s answer. + // Use the tab kind so editing cannot change the label width. internal static func commitVerb(for tabKind: TabType?) -> String { switch tabKind { case .createTable: @@ -78,8 +40,7 @@ internal enum ToolbarContextResolver { private static func contextualHidden(_ key: ToolbarContext.VisibilityKey) -> Set { var hidden: Set = [] - /// A file-based engine has one database and it is the file already named beside it, so the - /// second capsule has never been clickable on SQLite or DuckDB. + // File engines already name their sole database in the connection control. if key.isFileBased || !key.supportsContainerSwitching { hidden.insert(MainWindowToolbar.database) } @@ -91,8 +52,7 @@ internal enum ToolbarContextResolver { switch key.contentMode { case .agent: - /// No grid and no object browser are on screen, and the commit control's gate is frozen - /// because the browse content tree is not mounted to write it. + // Agent mode mounts no browse views to update these controls. hidden.insert(MainWindowToolbar.refresh) hidden.insert(MainWindowToolbar.saveChanges) return hidden @@ -106,12 +66,12 @@ internal enum ToolbarContextResolver { guard let tabKind = key.tabKind else { return [] } switch tabKind { case .createTable: - /// A definition that is not on the server yet has nothing to reload. + // A definition that is not on the server yet has nothing to reload. return [MainWindowToolbar.refresh] case .objectSource where key.stagesObjectChanges: return [] case .serverDashboard, .insights, .tableHealth, .objectSource, .versionHistory, .tableListing: - /// None of these can stage a change, so the commit control could only ever be dim. + // None of these can stage a change, so the commit control could only ever be dim. return [MainWindowToolbar.saveChanges] case .routineDebug, .liveMessages: // A debug run or a live subscription has nothing to reload or commit; its own controls take that place. @@ -138,40 +98,40 @@ internal enum ToolbarContextResolver { MainWindowToolbar.navigateForward, ] - /// Whether an item answers in this context. - /// - /// Every command the toolbar vends has an arm. The Back and Forward group has none because it is - /// a container: it carries no action, so AppKit never asks for it, and each of its two subitems - /// answers for itself. The `default:` returns false rather than true because the old - /// unconditional arm is what left Query History live and inert over a window that had never - /// connected, and left every identifier nobody had thought about enabled. + // The same commands as the menu bar's `sheetCommandSelectors`: a window with a sheet attached takes no second one. + private static let sheetCommandIdentifiers: Set = [ + MainWindowToolbar.saveChanges, + MainWindowToolbar.previewSQL, + MainWindowToolbar.exportTables, + MainWindowToolbar.importTables, + MainWindowToolbar.restorePreviousValues, + ] + + // The `default:` is false: an unconditional true left Query History live and inert over a window that never connected. internal static func isEnabled( _ identifier: NSToolbarItem.Identifier, context: ToolbarContext ) -> Bool { if context.contentMode == .agent, browseContentIdentifiers.contains(identifier) { return false } + if context.isPresentingSheet, sheetCommandIdentifiers.contains(identifier) { return false } switch identifier { case MainWindowToolbar.connection: - /// Switch Connection is the window's command, so it answers before a session exists. - /// It is the route back from a connection that failed. + // Switch Connection is the window's command, so it answers before a session exists. return true case MainWindowToolbar.database: return context.isConnected && !context.isFileBased && context.supportsContainerSwitching case MainWindowToolbar.actions: - /// The menu gates its own entries through the responder chain, so this answers only for - /// the window having something to be about at all. A closing window opens no menu. + // Each Actions menu entry validates itself through the responder chain. return context.hasSelectedWorkspace && context.pane != .empty case MainWindowToolbar.refresh: return context.isConnected case MainWindowToolbar.saveChanges: return context.pendingChange != nil && context.isConnected && !context.blocksAllWrites case MainWindowToolbar.safeMode: - /// Safe Mode is what stands between a stray keystroke and a live table, so it answers - /// for as long as the session does. A window with no session has nothing to write it to. + // Safe Mode stays available for as long as the session does. return context.isConnected case MainWindowToolbar.inspector: - /// Not `isConnected`. A connection that drops with the pane open must still be able to - /// close it, which is the state the old rule disabled on the app's minimum OS. + // A disconnected window must still let the user close its inspector. return context.canToggleTrailingPane case MainWindowToolbar.addRow: return context.isConnected && context.canAddRow @@ -184,9 +144,7 @@ internal enum ToolbarContextResolver { case MainWindowToolbar.previewSQL: return context.isConnected && context.hasDataPendingChanges case MainWindowToolbar.results: - /// The results pane belongs to the query editor. The shipped rule was `!isTableTab`, - /// which enabled it on the five kinds that have no results pane at all and then wrote a - /// collapse flag with no tab-kind guard behind it. + // Only query tabs have a results pane to toggle. return context.isConnected && context.tabKind == .query case MainWindowToolbar.history: return context.isConnected diff --git a/apple/TablePro/Models/UI/ToolbarContext.swift b/apple/TablePro/Models/UI/ToolbarContext.swift index f4267409e4..c056534fa1 100644 --- a/apple/TablePro/Models/UI/ToolbarContext.swift +++ b/apple/TablePro/Models/UI/ToolbarContext.swift @@ -6,38 +6,22 @@ import Foundation import TableProPluginKit -/// Everything the connection window's toolbar is allowed to know about what the window is showing. -/// -/// The toolbar used to carry one tab-shaped fact, `isTableTab`, and no content mode at all, so a -/// single list of identifiers was vended for every tab kind, every pane and both content modes and -/// the only lever anyone had was dimming. Every feature that arrived then had to buy a permanent -/// slot in the titlebar. -/// -/// Nothing global is read inside this struct or inside the resolvers that take it. The toolbar -/// builds it once per validation pass and once per Actions menu open and passes it down, so the -/// resolvers stay pure and testable with no host app and no session. -/// -/// Every field is one a resolver reads. A fact nothing reads is a fact every pass pays to look up. +// A value snapshot keeps toolbar validation independent of the live session. internal struct ToolbarContext: Equatable { - /// What the window's detail pane is drawing. `nil` when no tab is selected, which is a real - /// state on a window that has just opened. + // A newly opened window can have no selected tab. internal let tabKind: TabType? internal let resultsMode: ResultsViewMode? internal let contentMode: ConnectionWorkspaceContentMode internal let pane: ConnectionWindowPane - /// True whenever the session is alive, which includes a query in flight. A running query is not - /// a reason to disable Refresh, and the menu bar derives its own answer from the window phase - /// rather than from execution. + // A running query does not make the session unavailable. internal let isConnected: Bool - /// A connection is on screen, whether or not it has finished connecting. + // A connection is on screen, whether or not it has finished connecting. internal let hasSelectedWorkspace: Bool internal let canToggleTrailingPane: Bool internal let pendingChange: PendingChangeKind? - /// Not a projection of `pendingChange`. The two are computed from different inputs: a dirty - /// query file raises the commit control and leaves this false, which is exactly the distinction - /// Preview SQL is gated on. + // Dirty query files enable Save but have no data edits to preview. internal let hasDataPendingChanges: Bool internal let blocksAllWrites: Bool @@ -53,7 +37,7 @@ internal struct ToolbarContext: Equatable { internal let isAIEnabled: Bool - /// A type's tab queues changes the commit control saves; every other object tab only reads. + // A type's tab queues changes the commit control saves; every other object tab only reads. internal let stagesObjectChanges: Bool // Whether the Routine Debugger module puts its controls in the toolbar at all. @@ -61,17 +45,9 @@ internal struct ToolbarContext: Equatable { // What the selected debug tab can do now; empty and false everywhere else. internal let debugCommands: Set internal let canStopDebugging: Bool + internal let isPresentingSheet: Bool - /// The subset of the context that may move an item in or out of the titlebar, or change what - /// one says. - /// - /// This is the whole anti-reflow rule in one type. `isHidden` and the commit control's label are - /// written only from these fields, so the titlebar can change shape on a tab switch, a mode - /// switch or a connection switch and on nothing else; everything transient rides `isEnabled` - /// instead. The toolbar computes this from its eight inputs directly rather than from a whole - /// context, so a keystroke costs the selected-tab lookup, four locked reads of the plugin - /// metadata registry, a switch over the engine for the dashboard and one comparison, and writes - /// nothing. + // Only these stable facts may change the toolbar layout or labels. internal struct VisibilityKey: Equatable { internal let tabKind: TabType? internal let resultsMode: ResultsViewMode? @@ -123,7 +99,8 @@ internal struct ToolbarContext: Equatable { stagesObjectChanges: Bool = false, showsDebugItems: Bool = false, debugCommands: Set = [], - canStopDebugging: Bool = false + canStopDebugging: Bool = false, + isPresentingSheet: Bool = false ) { self.tabKind = tabKind self.resultsMode = resultsMode @@ -148,10 +125,10 @@ internal struct ToolbarContext: Equatable { self.showsDebugItems = showsDebugItems self.debugCommands = debugCommands self.canStopDebugging = canStopDebugging + self.isPresentingSheet = isPresentingSheet } - /// The whole context over a key the caller already computed, so the eight slow-moving facts - /// are read once per context rather than once for the key and again for the context. + // Over a key the caller already computed, so the slow-moving facts are read once per context. internal init( key: VisibilityKey, pane: ConnectionWindowPane, @@ -166,7 +143,8 @@ internal struct ToolbarContext: Equatable { canNavigateBack: Bool, canNavigateForward: Bool, debugCommands: Set = [], - canStopDebugging: Bool = false + canStopDebugging: Bool = false, + isPresentingSheet: Bool = false ) { self.init( tabKind: key.tabKind, @@ -191,7 +169,8 @@ internal struct ToolbarContext: Equatable { stagesObjectChanges: key.stagesObjectChanges, showsDebugItems: key.showsDebugItems, debugCommands: debugCommands, - canStopDebugging: canStopDebugging + canStopDebugging: canStopDebugging, + isPresentingSheet: isPresentingSheet ) } } diff --git a/apple/TablePro/Views/Main/Extensions/MainContentCoordinator+Rewind.swift b/apple/TablePro/Views/Main/Extensions/MainContentCoordinator+Rewind.swift index 40d7e43014..b6b4cf4dad 100644 --- a/apple/TablePro/Views/Main/Extensions/MainContentCoordinator+Rewind.swift +++ b/apple/TablePro/Views/Main/Extensions/MainContentCoordinator+Rewind.swift @@ -49,7 +49,7 @@ extension MainContentCoordinator { } func rewindLastSave() { - guard presentUpsellUnlessLicensed() else { return } + guard admitsSheetCommand(), presentUpsellUnlessLicensed() else { return } guard canRewindSelectedTab, let target = selectedRewindTarget else { return } Task { [weak self] in @@ -61,6 +61,7 @@ extension MainContentCoordinator { table: target.table, limit: 1 ) + guard admitsSheetCommand() else { return } // Enabled from the last publish; the save can be pruned or unreadable by the time this reads it. guard let record = records.first else { AlertHelper.showInfoSheet( @@ -80,7 +81,7 @@ extension MainContentCoordinator { // The record's own target decides where it runs: a rewind planned against the window's scope // would write a same-named table in the wrong database. func rewindSave(historyId: UUID) { - guard presentUpsellUnlessLicensed(), canRestoreSave(historyId: historyId) else { return } + guard admitsSheetCommand(), presentUpsellUnlessLicensed(), canRestoreSave(historyId: historyId) else { return } Task { [weak self] in guard let self else { return } let manager = services.queryHistoryManager @@ -88,6 +89,7 @@ extension MainContentCoordinator { guard let recordId = await manager.rewindSnapshotId(forHistoryId: historyId), let record = await manager.rewindSnapshot(id: recordId) else { + guard admitsSheetCommand() else { return } AlertHelper.showInfoSheet( title: String(localized: "Nothing to Restore"), message: String(localized: "The values this save replaced are no longer being kept."), @@ -121,6 +123,7 @@ extension MainContentCoordinator { } private func presentRewind(for record: RewindRecord, scope: DatabaseScope) async { + guard admitsSheetCommand() else { return } if let refusal = record.blanketRefusal { AlertHelper.showInfoSheet( title: String(localized: "This Save Cannot Be Restored"), @@ -140,10 +143,12 @@ extension MainContentCoordinator { factory: rewindStatementFactory(for: record), browseBuilder: services.databaseManager.driver(for: connectionId)?.queryBuildingPluginDriver ) + guard admitsSheetCommand() else { return } rewindPlan = plan activeSheet = .rewind } catch { Self.rewindLogger.error("Could not plan a rewind: \(error.publicLogShape, privacy: .public)") + guard admitsSheetCommand() else { return } AlertHelper.showErrorSheet( title: String(localized: "Could Not Prepare the Restore"), message: error.localizedDescription, diff --git a/apple/TablePro/Views/Main/Extensions/MainContentCoordinator+RowImport.swift b/apple/TablePro/Views/Main/Extensions/MainContentCoordinator+RowImport.swift index abfc0a5604..db9a0364db 100644 --- a/apple/TablePro/Views/Main/Extensions/MainContentCoordinator+RowImport.swift +++ b/apple/TablePro/Views/Main/Extensions/MainContentCoordinator+RowImport.swift @@ -24,7 +24,7 @@ internal extension MainContentCoordinator { databaseType: connection.type, connectionName: connection.name, safeModeLevel: safeModeLevel, - isPresentingSheet: activeSheet != nil || workspaceWindow?.attachedSheet != nil, + isPresentingSheet: isPresentingSheet, lookup: importFormatLookup ) } @@ -94,7 +94,7 @@ internal extension MainContentCoordinator { releaseImportFile() } - private var workspaceWindow: NSWindow? { + var workspaceWindow: NSWindow? { guard let splitViewController, splitViewController.isViewLoaded else { return contentWindow } return splitViewController.view.window ?? contentWindow } diff --git a/apple/TablePro/Views/Main/Extensions/MainContentCoordinator+SheetGate.swift b/apple/TablePro/Views/Main/Extensions/MainContentCoordinator+SheetGate.swift new file mode 100644 index 0000000000..8ed4c020c7 --- /dev/null +++ b/apple/TablePro/Views/Main/Extensions/MainContentCoordinator+SheetGate.swift @@ -0,0 +1,26 @@ +// +// MainContentCoordinator+SheetGate.swift +// TablePro +// + +import AppKit +import os + +internal extension MainContentCoordinator { + nonisolated private static var sheetGateLogger: Logger { + Logger(subsystem: "com.TablePro", category: "SheetGate") + } + + // The window sees every presenter (AppKit, other SwiftUI sheets, another hosted connection); `activeSheet` sees the turn before SwiftUI attaches it. + var isPresentingSheet: Bool { + activeSheet != nil || favoriteDialogQuery != nil || fileConflictRequest != nil + || workspaceWindow?.attachedSheet != nil + } + + // `.sheet(item:)` swaps the sheet on screen, so a job it runs would go on with no progress sheet. + func admitsSheetCommand(_ command: String = #function) -> Bool { + guard isPresentingSheet else { return true } + Self.sheetGateLogger.info("Refused \(command, privacy: .public): the window has a sheet attached") + return false + } +} diff --git a/apple/TablePro/Views/Main/MainContentCommandActions+DataDictionary.swift b/apple/TablePro/Views/Main/MainContentCommandActions+DataDictionary.swift index 1a68ba6029..1c37b8c799 100644 --- a/apple/TablePro/Views/Main/MainContentCommandActions+DataDictionary.swift +++ b/apple/TablePro/Views/Main/MainContentCommandActions+DataDictionary.swift @@ -7,6 +7,7 @@ import Foundation extension MainContentCommandActions { func exportDataDictionary() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.openExportDataDictionaryDialog() } diff --git a/apple/TablePro/Views/Main/MainContentCommandActions+DatabaseObjects.swift b/apple/TablePro/Views/Main/MainContentCommandActions+DatabaseObjects.swift index ad393b6be1..0d75618a6a 100644 --- a/apple/TablePro/Views/Main/MainContentCommandActions+DatabaseObjects.swift +++ b/apple/TablePro/Views/Main/MainContentCommandActions+DatabaseObjects.swift @@ -6,9 +6,7 @@ import Foundation import TableProPluginKit -/// Commands that act on the object selected in the sidebar. The sidebar's own -/// context menu reaches the same coordinator methods, so the menu bar is a second -/// path to them rather than a second implementation. +// Menu commands use the selected object; contextual menus carry their clicked row. extension MainContentCommandActions { var canShowTableStructure: Bool { selectedObject != nil @@ -60,6 +58,7 @@ extension MainContentCommandActions { } func refreshMaterializedView() { + guard coordinator?.admitsSheetCommand() == true else { return } guard let ref = selectedObjectRef, canRefreshMaterializedView else { return } coordinator?.refreshMaterializedView(ref) } @@ -73,7 +72,8 @@ extension MainContentCommandActions { } func editObjectComment() { - guard let ref = selectedObjectRef, canEditObjectComment else { return } + guard coordinator?.admitsSheetCommand() == true, let ref = selectedObjectRef, canEditObjectComment + else { return } coordinator?.editComment(of: ref) } @@ -86,7 +86,8 @@ extension MainContentCommandActions { } func setSequenceNextValue() { - guard let ref = selectedObjectRef, canSetSequenceNextValue else { return } + guard coordinator?.admitsSheetCommand() == true, let ref = selectedObjectRef, canSetSequenceNextValue + else { return } coordinator?.setSequenceNextValue(of: ref) } @@ -99,7 +100,7 @@ extension MainContentCommandActions { } func editSequence() { - guard let ref = selectedObjectRef, canEditSequence else { return } + guard coordinator?.admitsSheetCommand() == true, let ref = selectedObjectRef, canEditSequence else { return } coordinator?.editSequence(ref) } @@ -108,8 +109,7 @@ extension MainContentCommandActions { return .of(DatabaseManager.shared.driver(for: connectionId)) } - /// Narrowed to the selected object's kind, the same rule the sidebar's submenu follows. Asking - /// only whether something was selected is how the menu bar offered `REINDEX` on a view. + // Match the sidebar submenu by checking the selected object kind. var maintenanceOperations: [PluginMaintenanceOperation] { guard let object = selectedObject else { return [] } return TableOperationEligibility.maintenanceOperations( @@ -118,11 +118,9 @@ extension MainContentCommandActions { ) } - /// 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. + // Names only the schema: `TableInfo` carries no database, so the command falls back to the one being browsed. func runMaintenanceOperation(_ operation: PluginMaintenanceOperation) { - guard let object = selectedObject else { return } + guard coordinator?.admitsSheetCommand() == true, let object = selectedObject else { return } coordinator?.showMaintenanceSheet( operation: operation, tableName: object.name, schema: object.schema ) @@ -157,11 +155,11 @@ extension MainContentCommandActions { } func createDatabase() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.createDatabase() } - /// The menu-bar mirrors of the sidebar's own commands. With no clicked row to carry, both act - /// on the database being browsed and preselect everything in it. + // Without a clicked row, these commands use the browsed scope. var canCopyObjects: Bool { guard let coordinator else { return false } return isConnected && ObjectCopyEligibility.supportsCopying( @@ -181,10 +179,12 @@ extension MainContentCommandActions { } func copyObjectsToAnotherDatabase() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.openCopyObjects(mode: .copyTo, database: nil, schema: nil, preselection: .wholeScope) } func duplicateCurrentDatabase() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.openCopyObjects(mode: .duplicateDatabase, database: nil, schema: nil, preselection: .wholeScope) } @@ -193,17 +193,14 @@ extension MainContentCommandActions { } func searchDatabase() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.openDatabaseSearch(database: nil, schema: nil) } } @MainActor internal extension MainContentCoordinator { - /// Opens Copy To or Duplicate Database on the row that was right-clicked. - /// - /// The database and the schema travel with the request rather than being read from the browsing - /// state, for the reason every other sidebar command carries its ref: the tree can show a - /// database the session is not currently on, and copying from the wrong one is silent. + // Opens Copy To or Duplicate Database on the row that was right-clicked. func openCopyObjects( mode: ObjectCopyMode, database: String?, diff --git a/apple/TablePro/Views/Main/MainContentCommandActions+Documents.swift b/apple/TablePro/Views/Main/MainContentCommandActions+Documents.swift index f97b9b60c1..7580864a3e 100644 --- a/apple/TablePro/Views/Main/MainContentCommandActions+Documents.swift +++ b/apple/TablePro/Views/Main/MainContentCommandActions+Documents.swift @@ -7,6 +7,7 @@ import Foundation extension MainContentCommandActions { func insertDocument() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.presentInsertDocument() } @@ -15,7 +16,8 @@ extension MainContentCommandActions { } func editDocument() { - guard let rowID = singleSelectedDataGridRowID, + guard coordinator?.admitsSheetCommand() == true, + let rowID = singleSelectedDataGridRowID, let locator = coordinator?.documentLocator(for: rowID) else { return } coordinator?.presentEditDocument(locator: locator) } diff --git a/apple/TablePro/Views/Main/MainContentCommandActions+Structure.swift b/apple/TablePro/Views/Main/MainContentCommandActions+Structure.swift index d6fb86f73c..e64c645fed 100644 --- a/apple/TablePro/Views/Main/MainContentCommandActions+Structure.swift +++ b/apple/TablePro/Views/Main/MainContentCommandActions+Structure.swift @@ -22,6 +22,7 @@ extension MainContentCommandActions { var canDiscardChanges: Bool { discardTarget != nil } func discardChanges() { + guard coordinator?.admitsSheetCommand() == true else { return } switch discardTarget { case .structure(let session): session.discardChangesAfterConfirming() diff --git a/apple/TablePro/Views/Main/MainContentCommandActions.swift b/apple/TablePro/Views/Main/MainContentCommandActions.swift index 14d86bb65c..cbd24a5d90 100644 --- a/apple/TablePro/Views/Main/MainContentCommandActions.swift +++ b/apple/TablePro/Views/Main/MainContentCommandActions.swift @@ -217,6 +217,7 @@ final class MainContentCommandActions: ObservableObject { } func deleteSelectedRows() { + guard coordinator?.admitsSheetCommand() == true else { return } // A selected definition that cannot be removed refuses here; Delete never reaches the sidebar's tables. if selectsSchemaDefinition { removeSchemaRows() @@ -500,6 +501,7 @@ final class MainContentCommandActions: ObservableObject { } func buildJoinQuery() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.openJoinBuilder(seededWith: selectedTables.wrappedValue) } @@ -818,6 +820,7 @@ final class MainContentCommandActions: ObservableObject { } func truncateTables() { + guard coordinator?.admitsSheetCommand() == true else { return } guard canTruncateSelectedTables else { return } coordinator?.sidebarViewModel?.batchToggleTruncate() } @@ -874,10 +877,12 @@ final class MainContentCommandActions: ObservableObject { } func restorePreviousValues() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.rewindLastSave() } func saveChanges() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.commitActiveGridEdit() if isUsersRolesTab { coordinator?.usersRolesActions?.reviewAndApply() @@ -924,6 +929,7 @@ final class MainContentCommandActions: ObservableObject { } func saveFileAs() { + guard coordinator?.admitsSheetCommand() == true else { return } Task { await saveFileAsAwaiting() } } @@ -980,18 +986,22 @@ final class MainContentCommandActions: ObservableObject { } func exportTables() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.openExportDialog() } func exportQueryResults() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.openExportQueryResultsDialog() } func importData() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.openImportPanel() } func importTables(formatId: String) { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.openImportDialog(formatId: formatId) } @@ -1000,6 +1010,7 @@ final class MainContentCommandActions: ObservableObject { } func backupDatabase() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.activeSheet = .backupDatabase(databases: []) } @@ -1014,12 +1025,14 @@ final class MainContentCommandActions: ObservableObject { var supportsRestore: Bool { supportsBackup } func restoreDatabase() { + guard coordinator?.admitsSheetCommand() == true else { return } Task { @MainActor [weak self] in await self?.presentRestoreSourcePicker() } } private func presentRestoreSourcePicker() async { + guard coordinator?.admitsSheetCommand() == true else { return } let panel = NSOpenPanel() panel.canChooseFiles = true panel.canChooseDirectories = false @@ -1066,6 +1079,7 @@ final class MainContentCommandActions: ObservableObject { } func saveAsFavorite() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.saveCurrentQueryAsFavorite() } @@ -1075,6 +1089,7 @@ final class MainContentCommandActions: ObservableObject { } func previewSQL() { + guard coordinator?.admitsSheetCommand() == true else { return } coordinator?.handlePreviewSQL( pendingTruncates: pendingTruncates.wrappedValue, pendingDeletes: pendingDeletes.wrappedValue, diff --git a/apple/TableProTests/Views/Main/SheetCommandGateTests.swift b/apple/TableProTests/Views/Main/SheetCommandGateTests.swift new file mode 100644 index 0000000000..ddce82033d --- /dev/null +++ b/apple/TableProTests/Views/Main/SheetCommandGateTests.swift @@ -0,0 +1,270 @@ +// +// SheetCommandGateTests.swift +// TableProTests +// + +import AppKit +import SwiftUI +@testable import TablePro +import Testing + +// A stubbed sheet, so the case puts nothing on screen. +private final class SheetHostWindow: NSWindow { + var stubbedSheet: NSWindow? + override var attachedSheet: NSWindow? { stubbedSheet } +} + +@MainActor +struct SheetCommandGateTests { + private static func makeWindow() -> NSWindow { + let window = NSWindow(contentRect: .zero, styleMask: [.titled], backing: .buffered, defer: true) + window.isReleasedWhenClosed = false + return window + } + + private static func makeHostWindow(withSheet: Bool) -> SheetHostWindow { + let window = SheetHostWindow(contentRect: .zero, styleMask: [.titled], backing: .buffered, defer: true) + window.isReleasedWhenClosed = false + window.stubbedSheet = withSheet ? makeWindow() : nil + return window + } + + private static func makeActions(for fixture: CoordinatorFixture) -> MainContentCommandActions { + MainContentCommandActions( + coordinator: fixture.coordinator, + connection: fixture.connection, + selectedTables: .constant([]), + pendingTruncates: .constant([]), + pendingDeletes: .constant([]), + tableOperationOptions: .constant([:]), + trailingPaneState: TrailingPaneState() + ) + } + + // Every surface below lights these commands, so only the sheet decides. + private static func menuContext(isPresentingSheet: Bool) -> MenuValidationContext { + var context = MenuValidationContext() + context.hasSelectedWorkspace = true + context.isConnected = true + context.hasPendingChanges = true + context.hasDataPendingChanges = true + context.hasImportFormats = true + context.canRestorePreviousValues = true + context.hasQueryText = true + context.canDisconnect = true + context.isPresentingSheet = isPresentingSheet + return context + } + + private static func toolbarContext(isPresentingSheet: Bool) -> ToolbarContext { + ToolbarContext( + tabKind: .table, + pane: .content, + isConnected: true, + hasSelectedWorkspace: true, + pendingChange: .data, + hasDataPendingChanges: true, + canRestorePreviousValues: true, + supportsImport: true, + isPresentingSheet: isPresentingSheet + ) + } + + private static let menuAndToolbarTwins: [(selector: Selector, item: NSToolbarItem.Identifier)] = [ + (#selector(MainSplitViewController.saveDocument(_:)), MainWindowToolbar.saveChanges), + (#selector(MainSplitViewController.previewSQL(_:)), MainWindowToolbar.previewSQL), + (#selector(MainSplitViewController.exportTables(_:)), MainWindowToolbar.exportTables), + (#selector(MainSplitViewController.importData(_:)), MainWindowToolbar.importTables), + (#selector(MainSplitViewController.restorePreviousValues(_:)), MainWindowToolbar.restorePreviousValues), + ] + + // MARK: - The predicate + + @Test("A sheet the coordinator named counts before SwiftUI attaches it") + func namedSheetCounts() { + let fixture = CoordinatorFixture() + #expect(!fixture.coordinator.isPresentingSheet) + + fixture.coordinator.activeSheet = .exportDialog + #expect(fixture.coordinator.isPresentingSheet) + + fixture.coordinator.activeSheet = nil + #expect(!fixture.coordinator.isPresentingSheet) + } + + @Test("A sheet any presenter attached to the window counts, with no sheet named") + func attachedSheetCounts() { + let fixture = CoordinatorFixture() + let window = Self.makeHostWindow(withSheet: true) + fixture.coordinator.contentWindow = window + #expect(fixture.coordinator.activeSheet == nil) + #expect(fixture.coordinator.isPresentingSheet) + #expect(!fixture.coordinator.admitsSheetCommand()) + + window.stubbedSheet = nil + #expect(!fixture.coordinator.isPresentingSheet) + #expect(fixture.coordinator.admitsSheetCommand()) + } + + @Test("A pending favorite sheet blocks export before AppKit attaches it") + func favoriteSheetCounts() { + let fixture = CoordinatorFixture() + let actions = Self.makeActions(for: fixture) + fixture.coordinator.favoriteDialogQuery = FavoriteDialogQuery(query: "SELECT 1") + + #expect(fixture.coordinator.isPresentingSheet) + actions.exportTables() + #expect(fixture.coordinator.activeSheet == nil) + + fixture.coordinator.favoriteDialogQuery = nil + #expect(!fixture.coordinator.isPresentingSheet) + } + + @Test("A pending file conflict sheet blocks export before AppKit attaches it") + func fileConflictSheetCounts() { + let fixture = CoordinatorFixture() + let actions = Self.makeActions(for: fixture) + fixture.coordinator.fileConflictRequest = MainContentCoordinator.FileConflictRequest( + tabId: UUID(), url: URL(fileURLWithPath: "/query.sql"), mineContent: "SELECT 1", diskContent: "SELECT 2" + ) + + #expect(fixture.coordinator.isPresentingSheet) + actions.exportTables() + #expect(fixture.coordinator.activeSheet == nil) + + fixture.coordinator.fileConflictRequest = nil + #expect(!fixture.coordinator.isPresentingSheet) + } + + @Test("Restore Previous Values reached without validation keeps the export sheet") + func restoreRefusesOverExport() { + let fixture = CoordinatorFixture() + let actions = Self.makeActions(for: fixture) + fixture.coordinator.activeSheet = .exportDialog + + actions.restorePreviousValues() + fixture.coordinator.rewindLastSave() + fixture.coordinator.rewindSave(historyId: UUID()) + + #expect(fixture.coordinator.activeSheet?.id == ActiveSheet.exportDialog.id) + #expect(fixture.coordinator.rewindPlan == nil) + } + + // MARK: - Menu bar + + @Test("Restore Previous Values dims while the window has a sheet and lights without one") + func restorePreviousValuesMenuItem() { + let selector = #selector(MainSplitViewController.restorePreviousValues(_:)) + #expect(MainSplitViewController.isEnabled(selector, context: Self.menuContext(isPresentingSheet: false))) + #expect(!MainSplitViewController.isEnabled(selector, context: Self.menuContext(isPresentingSheet: true))) + } + + @Test("Export Tables dims while the window has a sheet and lights without one") + func exportMenuItem() { + let selector = #selector(MainSplitViewController.exportTables(_:)) + #expect(MainSplitViewController.isEnabled(selector, context: Self.menuContext(isPresentingSheet: false))) + #expect(!MainSplitViewController.isEnabled(selector, context: Self.menuContext(isPresentingSheet: true))) + } + + @Test("Disconnect dims under a sheet in Browse and Agent mode") + func disconnectMenuItem() { + let selector = #selector(MainSplitViewController.requestDisconnect) + for agentMode in [false, true] { + var context = Self.menuContext(isPresentingSheet: false) + context.isAgentMode = agentMode + #expect(MainSplitViewController.isEnabled(selector, context: context)) + context.isPresentingSheet = true + #expect(!MainSplitViewController.isEnabled(selector, context: context)) + } + } + + @Test("Every sheet command is one the window validates, so a sheet dims each of them") + func everySheetCommandDims() { + let context = Self.menuContext(isPresentingSheet: true) + for selector in MainSplitViewController.sheetCommandSelectors { + #expect( + MainSplitViewController.resolvedEnablement(selector, context: context) == false, + "\(NSStringFromSelector(selector)) must dim under a sheet" + ) + } + } + + @Test("A command that presents no sheet keeps working under one") + func commandsWithoutASheetStayLit() { + let context = Self.menuContext(isPresentingSheet: true) + let unaffected = [ + #selector(MainSplitViewController.refreshDatabase(_:)), + #selector(MainSplitViewController.executeQuery(_:)), + #selector(MainSplitViewController.newEditorTab(_:)), + #selector(MainSplitViewController.openQuickSwitcher(_:)), + ] + for selector in unaffected { + #expect( + MainSplitViewController.isEnabled(selector, context: context), + "\(NSStringFromSelector(selector)) presents no sheet and must stay lit" + ) + } + } + + // MARK: - Toolbar + + @Test("The toolbar's Restore Previous Values and Export dim under a sheet; Refresh does not") + func toolbarItems() { + let lit = Self.toolbarContext(isPresentingSheet: false) + let covered = Self.toolbarContext(isPresentingSheet: true) + for item in [MainWindowToolbar.restorePreviousValues, MainWindowToolbar.exportTables] { + #expect(ToolbarContextResolver.isEnabled(item, context: lit), "\(item.rawValue) must light without a sheet") + #expect(!ToolbarContextResolver.isEnabled(item, context: covered), "\(item.rawValue) must dim under a sheet") + } + #expect(ToolbarContextResolver.isEnabled(MainWindowToolbar.refresh, context: covered)) + } + + @Test("A toolbar item and its menu twin answer the same under a sheet") + func menuAndToolbarAgree() { + for twin in Self.menuAndToolbarTwins { + for isPresentingSheet in [false, true] { + let menu = MainSplitViewController.isEnabled( + twin.selector, + context: Self.menuContext(isPresentingSheet: isPresentingSheet) + ) + let toolbar = ToolbarContextResolver.isEnabled( + twin.item, + context: Self.toolbarContext(isPresentingSheet: isPresentingSheet) + ) + #expect(menu == !isPresentingSheet, "\(NSStringFromSelector(twin.selector)) answered \(menu)") + #expect(toolbar == menu, "\(twin.item.rawValue) answered \(toolbar), its menu twin \(menu)") + } + } + } + + // MARK: - The command itself + + @Test("Export reached with no validation keeps the sheet on screen instead of swapping it") + func exportRefusesOverANamedSheet() { + let fixture = CoordinatorFixture() + let actions = Self.makeActions(for: fixture) + + fixture.coordinator.activeSheet = .rewind + actions.exportTables() + #expect(fixture.coordinator.activeSheet?.id == ActiveSheet.rewind.id) + + fixture.coordinator.activeSheet = nil + actions.exportTables() + #expect(fixture.coordinator.activeSheet?.id == ActiveSheet.exportDialog.id) + } + + @Test("Export refuses over a sheet another presenter attached") + func exportRefusesOverAnAttachedSheet() { + let fixture = CoordinatorFixture() + let actions = Self.makeActions(for: fixture) + let window = Self.makeHostWindow(withSheet: true) + fixture.coordinator.contentWindow = window + + actions.exportTables() + #expect(fixture.coordinator.activeSheet == nil) + + window.stubbedSheet = nil + actions.exportTables() + #expect(fixture.coordinator.activeSheet?.id == ActiveSheet.exportDialog.id) + } +} From 73ef2560da59bde5720e9a75318bfc5d17f691b0 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 4 Nov 2026 02:51:48 +0700 Subject: [PATCH 2/4] fix(import): pause writes before choosing a stop outcome --- .../RowImportRunnerTests.swift | 31 + .../SQLImportTransactionTests.swift | 29 +- .../SQLImportPlugin/SQLImportFailure.swift | 10 +- .../SQLImportPlugin/SQLImportPlugin.swift | 14 +- .../TableProPluginKit/RowImportRunner.swift | 19 +- .../Core/Plugins/ImportDataSinkAdapter.swift | 128 +-- .../TablePro/Core/Plugins/ImportSinkRun.swift | 294 +++++++ .../Core/Services/Export/ImportLedger.swift | 315 ++++++++ .../Core/Services/Export/ImportRun.swift | 305 ++++++++ .../Core/Services/Export/ImportService.swift | 346 +++++--- .../Services/Export/RowImportBatchRun.swift | 14 +- .../Services/Export/RowImportTableMaker.swift | 110 ++- .../TablePro/Resources/Localizable.xcstrings | 123 +++ .../Components/TransferResultAlert.swift | 123 ++- .../TablePro/Views/Import/ImportDialog.swift | 26 +- .../Views/Import/ImportProgressView.swift | 47 +- .../Views/Import/RowImportBatchSheet.swift | 32 +- .../Views/Import/RowImportSheet.swift | 86 +- .../Core/Services/ImportRunTests.swift | 211 +++++ .../Services/ImportServiceStopTests.swift | 737 ++++++++++++++++++ .../Views/ImportCancelledTextTests.swift | 10 +- 21 files changed, 2579 insertions(+), 431 deletions(-) create mode 100644 apple/TablePro/Core/Plugins/ImportSinkRun.swift create mode 100644 apple/TablePro/Core/Services/Export/ImportLedger.swift create mode 100644 apple/TablePro/Core/Services/Export/ImportRun.swift create mode 100644 apple/TableProTests/Core/Services/ImportRunTests.swift create mode 100644 apple/TableProTests/Core/Services/ImportServiceStopTests.swift diff --git a/apple/Packages/TableProCore/Tests/TableProPluginKitTests/RowImportRunnerTests.swift b/apple/Packages/TableProCore/Tests/TableProPluginKitTests/RowImportRunnerTests.swift index 31349bea1e..9044c60a35 100644 --- a/apple/Packages/TableProCore/Tests/TableProPluginKitTests/RowImportRunnerTests.swift +++ b/apple/Packages/TableProCore/Tests/TableProPluginKitTests/RowImportRunnerTests.swift @@ -197,3 +197,34 @@ struct RowImportRunnerTests { #expect(sink.calls == ["insertRows(1)"]) } } + + +extension RowImportRunnerTests { + @Test("Skip and Continue propagates cancellation from the sink", arguments: [false, true]) + func skipCancellation(taskCancellation: Bool) async { + let sink = MockImportSink() + sink.onInsertRow = { _ in + if taskCancellation { throw CancellationError() } + throw PluginImportCancellationError() + } + do { + _ = try await RowImportRunner.run(configuration: configuration(.skipAndContinue), sink: sink, + progress: makeProgress(), nextBatch: provider([[entry(1), entry(2)]])) + Issue.record("Cancellation must leave the runner") + } catch { + #expect(error is PluginImportCancellationError || error is CancellationError) + } + #expect(sink.calls == ["insertRow"]) + } + + @Test("Task cancellation in a batch never applies Stop and Commit error policy") + func batchTaskCancellation() async { + let sink = MockImportSink() + sink.onInsertRows = { _ in throw CancellationError() } + await #expect(throws: CancellationError.self) { + try await RowImportRunner.run(configuration: configuration(.stopAndCommit), sink: sink, + progress: makeProgress(), nextBatch: provider([[entry(1)]])) + } + #expect(sink.calls == ["begin", "insertRows(1)", "rollback"]) + } +} diff --git a/apple/Plugins/FormatPluginTests/SQLImportTransactionTests.swift b/apple/Plugins/FormatPluginTests/SQLImportTransactionTests.swift index 49768762bb..38eeb3d1a5 100644 --- a/apple/Plugins/FormatPluginTests/SQLImportTransactionTests.swift +++ b/apple/Plugins/FormatPluginTests/SQLImportTransactionTests.swift @@ -19,12 +19,16 @@ private final class RecordingSink: PluginImportDataSink, @unchecked Sendable { var calls: [Call] = [] var statementsToFail: Set = [] + var cancellation: (any Error)? + var executed = 0 var rollbackFails = false var enableForeignKeysFails = false let databaseTypeId = "SQLite" func execute(statement: String) async throws { + executed += 1 + if let cancellation { throw cancellation } if statementsToFail.contains(statement) { throw FailingStatementError(message: "no such table: nope") } @@ -75,12 +79,6 @@ private final class StatementSource: PluginImportSource, @unchecked Sendable { func fileSizeBytes() -> Int64 { 0 } } -/// A transaction is committed at most once and rolled back only while it is open. Rolling back a -/// transaction that already committed fails on the server, and that failure used to be reported to -/// the user in place of the real one (#2314). -/// -/// Serialized because `SQLImportPlugin.settings` persists through plugin storage, so two -/// instances in flight at once read each other's error-handling mode. @Suite("SQL import transaction lifecycle", .serialized) struct SQLImportTransactionTests { private func runImport( @@ -89,8 +87,6 @@ struct SQLImportTransactionTests { errorHandling: ImportErrorHandling, sink: RecordingSink ) async -> (any Error)? { - /// `settings` persists through plugin storage, so a test that writes it changes the - /// developer's own preference unless it puts the value back. let plugin = SQLImportPlugin() let original = plugin.settings defer { plugin.settings = original } @@ -124,7 +120,6 @@ struct SQLImportTransactionTests { #expect(sink.calls.contains(.rollback) == false) } - /// The rollback would fail, and that failure would replace the statement error the user needs. @Test("Stop and commit still reports the statement that failed") func stopAndCommitReportsTheStatement() async throws { let sink = RecordingSink() @@ -157,8 +152,6 @@ struct SQLImportTransactionTests { #expect(sink.calls.contains(.commit) == false) } - /// The rows are in the database. Telling the user the import failed invites a re-run and a - /// second copy of every row. @Test("A committed import is not failed by the foreign key restore") func committedImportSurvivesAFailedForeignKeyRestore() async { let sink = RecordingSink() @@ -173,3 +166,17 @@ struct SQLImportTransactionTests { #expect(sink.calls.contains(.rollback) == false) } } + + +extension SQLImportTransactionTests { + @Test("Cancellation is never swallowed by Skip and Continue", arguments: [false, true]) + func skipDoesNotSwallowCancellation(taskCancellation: Bool) async { + let sink = RecordingSink() + sink.cancellation = taskCancellation ? CancellationError() : PluginImportCancellationError() + let error = await runImport(["INSERT INTO t VALUES (1)", "INSERT INTO t VALUES (2)"], + errorHandling: .skipAndContinue, sink: sink) + #expect(error is PluginImportCancellationError || error is CancellationError) + #expect(sink.executed == 1) + #expect(!sink.calls.contains(.commit)) + } +} diff --git a/apple/Plugins/SQLImportPlugin/SQLImportFailure.swift b/apple/Plugins/SQLImportPlugin/SQLImportFailure.swift index 01ecb19c95..4fdd896c43 100644 --- a/apple/Plugins/SQLImportPlugin/SQLImportFailure.swift +++ b/apple/Plugins/SQLImportPlugin/SQLImportFailure.swift @@ -6,15 +6,9 @@ import Foundation import TableProPluginKit -/// What the user is told when an import fails. -/// -/// The cause is the only thing carrying the line number and the failing statement, so a rollback -/// that also fails on the way out is appended to it rather than thrown in its place. Reporting the -/// rollback alone left the user reading "Transaction rollback failed" with nothing about what -/// actually broke, and it turned a cancellation into an error (#2314). internal enum SQLImportFailure { internal static func error(cause: any Error, rollbackError: (any Error)?) -> any Error { - if cause is PluginImportCancellationError { + if cause is PluginImportCancellationError || cause is CancellationError { return cause } guard let rollbackError else { @@ -45,8 +39,6 @@ internal struct RollbackAlsoFailedError: LocalizedError { internal let cause: any Error internal let rollbackError: any Error - /// The database's own message rarely ends in punctuation, so the two are put on separate lines - /// rather than run together into one unreadable sentence. internal var message: String { """ \(cause.localizedDescription) diff --git a/apple/Plugins/SQLImportPlugin/SQLImportPlugin.swift b/apple/Plugins/SQLImportPlugin/SQLImportPlugin.swift index ed5c6928c1..91b6a9c205 100644 --- a/apple/Plugins/SQLImportPlugin/SQLImportPlugin.swift +++ b/apple/Plugins/SQLImportPlugin/SQLImportPlugin.swift @@ -76,6 +76,10 @@ final class SQLImportPlugin: ObservableObject, ImportFormatPlugin, SettablePlugi try await sink.execute(statement: statement, line: lineNumber) executedCount += 1 progress.incrementStatement() + } catch is PluginImportCancellationError { + throw PluginImportCancellationError() + } catch is CancellationError { + throw CancellationError() } catch { switch errorMode { case .stopAndRollback: @@ -92,14 +96,14 @@ final class SQLImportPlugin: ObservableObject, ImportFormatPlugin, SettablePlugi try await sink.commitTransaction() didCommit = true } catch { - Self.logger.warning("Failed to commit partial import: \(error.localizedDescription)") + Self.logger.warning("Failed to commit partial import") } } if settings.disableForeignKeyChecks { do { try await sink.enableForeignKeyChecks() } catch { - Self.logger.warning("Failed to re-enable foreign key checks: \(error.localizedDescription)") + Self.logger.warning("Failed to re-enable foreign key checks") } } throw PluginImportError.statementFailed( @@ -134,7 +138,7 @@ final class SQLImportPlugin: ObservableObject, ImportFormatPlugin, SettablePlugi do { try await sink.enableForeignKeyChecks() } catch { - Self.logger.warning("Failed to re-enable foreign key checks: \(error.localizedDescription)") + Self.logger.warning("Failed to re-enable foreign key checks") } } } catch { @@ -146,7 +150,7 @@ final class SQLImportPlugin: ObservableObject, ImportFormatPlugin, SettablePlugi try await sink.rollbackTransaction() } catch { Self.logger.error( - "Import failed: \(importError.localizedDescription). Rollback also failed: \(error.localizedDescription)" + "The import and its rollback both failed" ) rollbackError = error } @@ -156,7 +160,7 @@ final class SQLImportPlugin: ObservableObject, ImportFormatPlugin, SettablePlugi do { try await sink.enableForeignKeyChecks() } catch { - Self.logger.warning("Failed to re-enable foreign key checks: \(error.localizedDescription)") + Self.logger.warning("Failed to re-enable foreign key checks") } } diff --git a/apple/Plugins/TableProPluginKit/RowImportRunner.swift b/apple/Plugins/TableProPluginKit/RowImportRunner.swift index 56f1475044..25b023e476 100644 --- a/apple/Plugins/TableProPluginKit/RowImportRunner.swift +++ b/apple/Plugins/TableProPluginKit/RowImportRunner.swift @@ -95,11 +95,9 @@ public enum RowImportRunner { inserted += batch.count progress.incrementStatement(by: batch.count) } catch is PluginImportCancellationError { - /// A Stop keeps its own type. The sink splits one hand-off into several statements and - /// checks cancellation before each, so this catch sees cancellation where it used to see - /// only driver errors; wrapped as a statement failure, Stop-and-Commit committed the - /// prefix already written and recorded the stop as a failed import. throw PluginImportCancellationError() + } catch is CancellationError { + throw CancellationError() } catch { let firstLine = batch.first?.line ?? 0 throw PluginImportError.statementFailed( @@ -124,6 +122,10 @@ public enum RowImportRunner { do { try await sink.insertRow(entry.row) inserted += 1 + } catch is PluginImportCancellationError { + throw PluginImportCancellationError() + } catch is CancellationError { + throw CancellationError() } catch { skipped += 1 if errors.count < configuration.maxRecordedErrors { @@ -145,21 +147,22 @@ public enum RowImportRunner { configuration: Configuration ) async throws -> Never { if useTransaction { - if configuration.errorHandling == .stopAndCommit, !(error is PluginImportCancellationError) { + if configuration.errorHandling == .stopAndCommit, + !(error is PluginImportCancellationError), !(error is CancellationError) { do { try await sink.commitTransaction() } catch { - logger.warning("Commit of partial import failed: \(error.localizedDescription)") + logger.warning("Commit of partial import failed") } } else { do { try await sink.rollbackTransaction() } catch { - logger.warning("Rollback after failed import also failed: \(error.localizedDescription)") + logger.warning("Rollback after failed import also failed") } } } - if error is PluginImportCancellationError { throw error } + if error is PluginImportCancellationError || error is CancellationError { throw error } if error is PluginImportError { throw error } throw PluginImportError.importFailed(error.localizedDescription) } diff --git a/apple/TablePro/Core/Plugins/ImportDataSinkAdapter.swift b/apple/TablePro/Core/Plugins/ImportDataSinkAdapter.swift index 9e2c2e9d28..e06d24a770 100644 --- a/apple/TablePro/Core/Plugins/ImportDataSinkAdapter.swift +++ b/apple/TablePro/Core/Plugins/ImportDataSinkAdapter.swift @@ -8,19 +8,12 @@ import os import TableProPluginKit import TableProSQLGrammar -/// What became of the transaction an import opened, as far as the import saw. internal enum ImportTransactionOutcome: Equatable, Sendable { - /// No transaction of the import's own, or one it did not see end: what ran stays as it ran. case untracked case committed case rolledBack - /// A statement that succeeded left no transaction open, as a `CREATE` or the file's own `COMMIT` does on MySQL, so - /// what ran up to then was not rolled back. case committedBeforeRollback - /// The transaction had ended by the rollback with nothing before that naming how: a deadlock rolls it back, a - /// failing `CREATE` commits it, and the session reads the same after either. case endedByServer - /// The file ran its own `ROLLBACK`, so what stays depends on what the file ran after it. case endedByFile } @@ -29,6 +22,7 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { let targetTable: String? private let targetSchema: String? + private let importControl: ImportSinkRun? private let driver: DatabaseDriver private let databaseType: DatabaseType private let exactMapping: [String: String] @@ -40,21 +34,11 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { private let tally: OSAllocatedUnfairLock private let immutableColumns = OSAllocatedUnfairLock?>(initialState: nil) - /// Asked before every statement this sink sends, because one `insertRows` call is no longer one - /// statement: a byte-heavy batch, or an Oracle target that takes one row per statement, splits - /// it into many awaited writes. The callers check their own cancellation only before entering - /// the sink, so without this a Stop was followed by every remaining INSERT of the batch. private let isCancelled: @Sendable () -> Bool private let tableEditDialect: TableEditDialect? private let nameHazards = OSAllocatedUnfairLock<[String]>(initialState: []) - /// Whether this import opened the transaction it is in. A session connection can already hold - /// the user's own: a `BEGIN` there would join it, and the import's `COMMIT` would commit work the - /// user never asked to commit, so the import then runs inside it and leaves ending it to them. private let ownsTransaction = OSAllocatedUnfairLock(initialState: false) - /// One file of several imported in one run. The run opened the transaction and ends it, so this sink sends no - /// `BEGIN`, `COMMIT` or `ROLLBACK`: reading the session would not stop it, since `.unknown` permits a `BEGIN` - /// and MySQL commits the open transaction on one. private let joinsTransaction: Bool private let transactionRecord = OSAllocatedUnfairLock(initialState: TransactionRecord()) @@ -72,7 +56,6 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { private static let logger = Logger(subsystem: "com.TablePro", category: "ImportDataSinkAdapter") - /// Rows go in as SQL `INSERT`s, so an engine with no SQL dialect can take none of them. static func canWriteRows(into databaseType: DatabaseType) -> Bool { (try? SQLStatementGenerator( tableName: "t", columns: [], primaryKeyColumns: [], databaseType: databaseType @@ -88,8 +71,14 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { sourceFields: Set = [], rowWriting: ImportRowWriting = .insert, joinsTransaction: Bool = false, + importRun: ImportRun? = nil, + fileIndex: Int? = 0, isCancelled: @escaping @Sendable () -> Bool = { false } ) { + self.importControl = importRun.map { + ImportSinkRun(run: $0, driver: driver, databaseType: databaseType, fileIndex: fileIndex, + targetTable: targetTable, targetSchema: targetSchema, joinsTransaction: joinsTransaction) + } self.isCancelled = isCancelled self.rowWriting = rowWriting self.tally = OSAllocatedUnfairLock(initialState: ImportRowTally(mode: rowWriting.mode)) @@ -131,7 +120,8 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { } var transactionOutcome: ImportTransactionOutcome { - transactionRecord.withLock { record in + if let importControl { return importControl.transactionOutcome } + return transactionRecord.withLock { record in switch record.ending { case .committed: return .committed @@ -149,8 +139,6 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { } } - /// What the file ran that changes where a name resolves for the rest of the session, such as a temporary table, so - /// the catalog can stop following a later drop of that name onto the real table. var nameHazardStatements: [String] { nameHazards.withLock { $0 } } @@ -171,9 +159,11 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { func execute(statement: String, line: Int) async throws { // The file's own statements change how the session reads the next one. let grammar = SQLLexicalResolver.readings(for: databaseType, session: driver.sessionLexicalState).execution - noteNameHazards(in: statement, grammar: grammar) - try await send(statement, line: line, grammar: grammar) - await noteTransactionEnd(after: statement, grammar: grammar) + try await importedData(writes: true, statement: statement) { + self.noteNameHazards(in: statement, grammar: grammar) + try await self.send(statement, line: line, grammar: grammar) + await self.noteTransactionEnd(after: statement, grammar: grammar) + } } // T-SQL scopes a variable and a TRY...CATCH to its batch, so a batch goes whole where the driver can send it so. @@ -193,8 +183,6 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { } } - /// An import shows no rows, so a batch keeps as few of each result set as a driver takes, and the rest are read - /// past rather than held. private static let batchRowCap = 1 func insertRow(_ values: [String: PluginCellValue]) async throws { @@ -222,8 +210,10 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { ) } - _ = try await driver.executeParameterized(query: statement.sql, parameters: statement.parameters) - tally.withLock { $0.inserted += 1 } + try await importedData(rows: 1, writes: true) { + _ = try await self.driver.executeParameterized(query: statement.sql, parameters: statement.parameters) + self.tally.withLock { $0.inserted += 1 } + } } func insertRows(_ rows: [[String: PluginCellValue]]) async throws { @@ -286,12 +276,6 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { try await insertGroup(rows, columns: columns, generator: rowGenerator, onWrite: onWrite) } - /// One budget per group, because a group is exactly the run of rows sharing one column set and - /// the column set is what fixes a row's width. Bounded in bytes as well as parameters: 500 rows - /// of three 1 MB values fit a three-column table's 21,845-row parameter ceiling and went out as - /// a single 500 MB statement the server refused, failing the first batch and leaving nothing - /// imported. The row cap also carries the engine's multi-row `VALUES` ceiling, which this path - /// never consulted, so an Oracle target no longer gets a statement it cannot parse. private func insertGroup( _ groupValues: [[PluginCellValue]], columns: [String], @@ -323,8 +307,10 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { String(localized: "Could not build an INSERT for the mapped columns") ) } - _ = try await driver.executeParameterized(query: statement.sql, parameters: statement.parameters) - tally.withLock { $0.inserted += batch.count } + try await importedData(rows: batch.count, writes: true) { + _ = try await self.driver.executeParameterized(query: statement.sql, parameters: statement.parameters) + self.tally.withLock { $0.inserted += batch.count } + } } // MARK: - Matching existing rows @@ -434,14 +420,18 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { ) else { throw PluginImportError.importFailed(String(localized: "Could not build an UPDATE for the mapped columns")) } - _ = try await driver.executeParameterized(query: statement.sql, parameters: statement.parameters) - tally.withLock { $0.updated += 1 } + try await importedData(rows: 1, writes: true) { + _ = try await self.driver.executeParameterized(query: statement.sql, parameters: statement.parameters) + self.tally.withLock { $0.updated += 1 } + } } private func columnsUpdateCannotSet() async throws -> Set { if let known = immutableColumns.withLock({ $0 }) { return known } guard let targetTable else { return [] } - let fetched = try await driver.fetchImmutableColumns(table: targetTable, schema: targetSchema) + let fetched = try await importedData { + try await self.driver.fetchImmutableColumns(table: targetTable, schema: self.targetSchema) + } immutableColumns.withLock { $0 = fetched } return fetched } @@ -460,7 +450,9 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { guard let statement = generator.keyMatchCountStatement(keyColumns: rowWriting.keyColumns, keys: chunk) else { throw PluginImportError.importFailed(String(localized: "Could not build the query that matches rows")) } - let result = try await driver.executeParameterized(query: statement.sql, parameters: statement.parameters) + let result = try await importedData { + try await self.driver.executeParameterized(query: statement.sql, parameters: statement.parameters) + } counts += try Self.counts(in: result, expected: chunk.count) start += chunk.count } @@ -499,18 +491,10 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { """) } - /// A row carrying values none of which reach a mapped column writes nothing. Reporting it as - /// inserted is how "Import completed" came to overstate what reached the database, so it is - /// refused instead: skip-and-continue records it against its line, and the stop modes halt, - /// because a mapping that matches no field of a row holding data is one the user needs to look - /// at. A row with no values at all carries nothing to lose and passes through untouched. private static var unmappedRowMessage: String { String(localized: "No values in this row matched the column mapping") } - /// Exact names first. A case-insensitive match is only a fallback for a field the sheet never - /// listed, such as a JSON key respelled past the sample, and only when that spelling points at - /// one listed field, one mapping key and one field of the row, none of them already matched. func mappedColumnsAndValues(_ values: [String: PluginCellValue]) -> ([String], [PluginCellValue]) { var pairs: [(column: String, value: PluginCellValue)] = [] var matchedNames = Set() @@ -537,10 +521,13 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { guard !rowWriting.mode.matchesExistingRows else { throw PluginImportError.importFailed(Self.matchingRefusesClearingMessage) } - _ = try await driver.execute(query: rowGenerator.deleteAllRowsStatement()) + try await importedData(writes: true, deletes: true) { + _ = try await self.driver.execute(query: rowGenerator.deleteAllRowsStatement()) + } } func beginTransaction() async throws { + if let importControl { return try await importControl.begin() } guard !joinsTransaction else { return } let owner = WriteTransactionOwner.resolve( supportsTransactions: driver.supportsTransactions, @@ -553,6 +540,7 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { } func commitTransaction() async throws { + if let importControl { return try await importControl.commit() } guard ownsTransaction.withLock({ $0 }) else { return } try await driver.commitTransaction() ownsTransaction.withLock { $0 = false } @@ -560,6 +548,7 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { } func rollbackTransaction() async throws { + if let importControl { return try await importControl.rollback() } guard ownsTransaction.withLock({ $0 }) else { return } guard await !sessionHoldsNoTransaction() else { ownsTransaction.withLock { $0 = false } @@ -614,14 +603,51 @@ final class ImportDataSinkAdapter: PluginImportDataSink, @unchecked Sendable { func disableForeignKeyChecks() async throws { guard let statements = driver.foreignKeyDisableStatements() else { return } for stmt in statements { - _ = try await driver.execute(query: stmt) + try await withImportSession { + _ = try await self.driver.execute(query: stmt) + } } } func enableForeignKeyChecks() async throws { guard let statements = driver.foreignKeyEnableStatements() else { return } for stmt in statements { - _ = try await driver.execute(query: stmt) + try await withImportSession { + _ = try await self.driver.execute(query: stmt) + } + } + } + + func recordImportError(_ error: any Error) async { + guard let importControl else { return } + try? await importControl.session { importControl.run.record { $0.recordError(error) } } + } + + func prepareImportRun() async throws { + try await importControl?.prepare() + } + + func importCheckpoint() async throws { + try await importControl?.checkpoint() + } + + func withImportSession(_ operation: () async throws -> T) async throws -> T { + if let importControl { return try await importControl.session(operation) } + return try await operation() + } + + private func importedData( + rows: Int = 0, + writes: Bool = false, + deletes: Bool = false, + statement: String? = nil, + operation: () async throws -> T + ) async throws -> T { + if let importControl { + return try await importControl.data( + rows: rows, writes: writes, deletes: deletes, statement: statement, operation: operation + ) } + return try await operation() } } diff --git a/apple/TablePro/Core/Plugins/ImportSinkRun.swift b/apple/TablePro/Core/Plugins/ImportSinkRun.swift new file mode 100644 index 0000000000..bbad43e321 --- /dev/null +++ b/apple/TablePro/Core/Plugins/ImportSinkRun.swift @@ -0,0 +1,294 @@ +// +// ImportSinkRun.swift +// TablePro +// + +import Foundation +import TableProPluginKit +import TableProSQLGrammar + +internal final class ImportSinkRun: @unchecked Sendable { + internal let run: ImportRun + private let driver: DatabaseDriver + private let databaseType: DatabaseType + private let fileIndex: Int? + private let targetTable: String? + private let targetSchema: String? + private let joinsTransaction: Bool + private let lock = NSLock() + private var ownsTransaction = false + private var finalized = false + private var acknowledgedEnding: ImportTransactionOutcome = .untracked + private var fileCommittedEarly = false + private var fileRolledBack = false + + internal var transactionOutcome: ImportTransactionOutcome { + lock.withLock { + if acknowledgedEnding == .committed { return .committed } + if fileCommittedEarly { return .committedBeforeRollback } + if fileRolledBack { return .endedByFile } + return acknowledgedEnding + } + } + + internal init( + run: ImportRun, + driver: DatabaseDriver, + databaseType: DatabaseType, + fileIndex: Int?, + targetTable: String?, + targetSchema: String?, + joinsTransaction: Bool + ) { + self.run = run + self.driver = driver + self.databaseType = databaseType + self.fileIndex = fileIndex + self.targetTable = targetTable + self.targetSchema = targetSchema + self.joinsTransaction = joinsTransaction + } + + internal func prepare() async throws { + try await admitted(.begin) { + if !self.run.snapshot.prepared { + let state = await self.sessionState() + self.run.record { + $0.prepared = true + $0.initialState = state + $0.currentState = state + $0.holder = state.holdsTransaction ? .joinedUser : .none + $0.uncertainHistory = state == .unknown + let grammar = SQLLexicalResolver.readings( + for: self.databaseType, session: self.driver.sessionLexicalState + ).execution + $0.unseenStatements = grammar.contains(.batchSeparatorLines) + } + } + guard let fileIndex = self.fileIndex else { return } + let rollback = await self.tableRollback() + self.run.record { + $0.effects[fileIndex] = ImportFileEffect(index: fileIndex, table: self.targetTable, rollback: rollback) + } + await self.observe() + } + } + + private func tableRollback() async -> ImportTableRollback { + guard driver.supportsTransactions else { return .nontransactional } + guard let targetTable, DataSyncTransactionality.needsStorageEngines(databaseType) else { + return .transactional + } + guard let metadata = try? await driver.fetchTableMetadata(tableName: targetTable, schema: targetSchema), + let engine = metadata.engine, !engine.isEmpty else { return .unknown } + return DataSyncTransactionality.cannotRollBack(storageEngine: engine, databaseType: databaseType) + ? .nontransactional : .transactional + } + + internal func data( + rows: Int = 0, + writes: Bool = false, + deletes: Bool = false, + statement: String? = nil, + operation: () async throws -> T + ) async throws -> T { + try await admitted(.data) { + if let statement { self.noteStatement(statement) } + if writes, let index = self.fileIndex { + self.run.record { $0.effects[index]?.admittedWrite = true } + } + let before = self.run.snapshot.currentState + let hadAuthority = self.lock.withLock { self.ownsTransaction } + do { + let value = try await operation() + if let index = self.fileIndex { + self.run.record { + $0.effects[index]?.submittedRows += rows + if statement != nil { $0.effects[index]?.statements += 1 } + if deletes { $0.effects[index]?.deletedRows = true } + } + } + await self.observe() + if let statement { self.noteCompletedStatement(statement, before: before, wasOwned: hadAuthority) } + return value + } catch { + self.run.record { + $0.recordError(error) + if writes, let index = self.fileIndex { $0.effects[index]?.failedWrite = true } + } + await self.observe() + throw error + } + } + } + + private func noteStatement(_ statement: String) { + let grammar = SQLLexicalResolver.readings(for: databaseType, session: driver.sessionLexicalState).execution + if grammar.contains(.batchSeparatorLines) { + lock.withLock { ownsTransaction = false } + run.record { $0.holder = .none; $0.uncertainHistory = true } + } + let effect = CatalogChangeClassifier.effect(of: statement, databaseType: databaseType, grammar: grammar) + let keyword = QueryClassifier.leadingKeyword(of: statement, grammar: grammar) + if effect.endsTransaction || ["BEGIN", "START", "SAVEPOINT", "ROLLBACK", "ABORT"].contains(keyword) { + run.record { $0.uncertainHistory = true } + } + } + + private func noteCompletedStatement(_ statement: String, before: ImportSessionState, wasOwned: Bool) { + guard wasOwned else { return } + let grammar = SQLLexicalResolver.readings(for: databaseType, session: driver.sessionLexicalState).execution + guard !grammar.contains(.batchSeparatorLines) else { return } + let effect = CatalogChangeClassifier.effect(of: statement, databaseType: databaseType, grammar: grammar) + let keyword = QueryClassifier.leadingKeyword(of: statement, grammar: grammar) + let ended = before.holdsTransaction && run.snapshot.currentState == .idle + let starts = ["BEGIN", "START"].contains(keyword) + guard effect.endsTransaction || ended || starts else { return } + lock.withLock { + ownsTransaction = false + if effect.endsTransaction || ended { + if ["ROLLBACK", "ABORT"].contains(keyword) { fileRolledBack = true } else { fileCommittedEarly = true } + } + } + run.record { $0.holder = .none; $0.uncertainHistory = true } + } + + internal func session(_ operation: () async throws -> T) async throws -> T { + try await admitted(.session) { + do { + let value = try await operation() + await self.observe(finalizing: true) + return value + } catch { + self.run.record { $0.recordCleanupError(error); $0.uncertainHistory = true } + await self.observe(finalizing: true) + throw error + } + } + } + + internal func checkpoint() async throws { + try await admitted(.data) {} + } + + internal func begin() async throws { + guard !joinsTransaction else { return } + try await admitted(.begin) { + let state = await self.driver.heldSessionTransactionState() + let owner = WriteTransactionOwner.resolve(supportsTransactions: self.driver.supportsTransactions, sessionState: state) + guard owner.opensTransaction else { + self.run.record { $0.currentState = ImportSessionState(state) } + return + } + try await self.driver.beginTransaction(mode: .readWrite) + self.lock.withLock { self.ownsTransaction = true } + self.run.record { $0.holder = .owned; $0.ending = nil } + await self.observe() + } + } + + internal func commit() async throws { + guard mayFinalize else { return } + guard try await run.admit(.commit) == .proceed else { return } + defer { run.release() } + guard mayFinalize else { return } + let state = ImportSessionState(await driver.heldSessionTransactionState()) + if state == .aborted { + try await finishRollback() + return + } + if state == .idle { + run.record { $0.uncertainHistory = true; $0.currentState = .idle; $0.ending = .unknown } + lock.withLock { acknowledgedEnding = .endedByServer } + markFinalized() + return + } + do { + try await driver.commitTransaction() + await recordEnding(.committed) + lock.withLock { acknowledgedEnding = .committed } + markFinalized() + } catch { + run.record { $0.recordCleanupError(error); $0.ending = .unknown } + let after = ImportSessionState(await driver.heldSessionTransactionState()) + if after != .idle { + do { try await finishRollback() } catch { run.record { $0.recordCleanupError(error) } } + } else { + run.record { $0.currentState = after; $0.uncertainHistory = true } + } + markFinalized() + throw error + } + } + + internal func rollback() async throws { + guard mayFinalize else { return } + guard try await run.admit(.rollback) == .proceed else { return } + defer { run.release() } + guard mayFinalize else { return } + try await finishRollback() + } + + private var mayFinalize: Bool { + !joinsTransaction && lock.withLock { ownsTransaction && !finalized } + } + + private func finishRollback() async throws { + let state = ImportSessionState(await driver.heldSessionTransactionState()) + guard state != .idle else { + run.record { $0.uncertainHistory = true; $0.currentState = .idle; $0.ending = .unknown } + lock.withLock { acknowledgedEnding = .endedByServer } + markFinalized() + return + } + do { + try await driver.rollbackTransaction() + await recordEnding(.rolledBack) + lock.withLock { acknowledgedEnding = .rolledBack } + markFinalized() + } catch { + run.record { $0.recordCleanupError(error); $0.ending = .unknown } + await observe(finalizing: true) + markFinalized() + throw error + } + } + + private func recordEnding(_ ending: ImportLedger.Ending) async { + let state = ImportSessionState(await driver.heldSessionTransactionState()) + run.record { + $0.currentState = state + $0.ending = state == .idle ? ending : .unknown + } + } + + private func markFinalized() { + lock.withLock { finalized = true } + } + + private func sessionState() async -> ImportSessionState { + guard driver.supportsTransactions else { return .idle } + return ImportSessionState(await driver.heldSessionTransactionState()) + } + + private func observe(finalizing: Bool = false) async { + let state = await sessionState() + let previous = run.snapshot.currentState + if !finalizing, previous.holdsTransaction, state == .idle { + let revoked = lock.withLock { () -> Bool in + guard ownsTransaction else { return false } + ownsTransaction = false + acknowledgedEnding = .endedByServer + return true + } + if revoked { run.record { $0.holder = .none } } + } + run.record { $0.observe(state, finalizing: finalizing) } + } + + private func admitted(_ kind: ImportCallKind, _ operation: () async throws -> T) async throws -> T { + guard try await run.admit(kind) == .proceed else { throw PluginImportCancellationError() } + defer { run.release() } + return try await operation() + } +} diff --git a/apple/TablePro/Core/Services/Export/ImportLedger.swift b/apple/TablePro/Core/Services/Export/ImportLedger.swift new file mode 100644 index 0000000000..860da09c51 --- /dev/null +++ b/apple/TablePro/Core/Services/Export/ImportLedger.swift @@ -0,0 +1,315 @@ +// +// ImportLedger.swift +// TablePro +// + +import Foundation +import TableProPluginKit + +internal enum ImportSessionState: Equatable, Sendable { + case idle + case open + case aborted + case unknown + + internal init(_ state: PluginSessionTransactionState) { + switch state { + case .idle, .holdsSessionLocks: self = .idle + case .inTransaction: self = .open + case .abortedTransaction: self = .aborted + case .unknown: self = .unknown + @unknown default: self = .unknown + } + } + + internal var holdsTransaction: Bool { self == .open || self == .aborted } +} + +internal enum ImportTableRollback: Equatable, Sendable { + case transactional + case nontransactional + case unknown +} + +internal struct ImportFileEffect: Equatable, Sendable { + internal let index: Int + internal let table: String? + internal var rollback: ImportTableRollback = .unknown + internal var submittedRows = 0 + internal var statements = 0 + internal var admittedWrite = false + internal var failedWrite = false + internal var deletedRows = false +} + +internal struct ImportPreparationEffect: Equatable, Sendable { + internal let table: TableScope + internal let clearing: Bool + internal var completed = false + internal var fate = ImportFileFate.mayStay +} + +internal struct ImportLedger: Equatable, Sendable { + internal enum Holder: Equatable, Sendable { + case none + case joinedUser + case owned + } + + internal enum Ending: Equatable, Sendable { + case committed + case rolledBack + case unknown + } + + internal var holder = Holder.none + internal var initialState = ImportSessionState.unknown + internal var currentState = ImportSessionState.unknown + internal var prepared = false + internal var uncertainHistory = false + internal var unseenStatements = false + internal var ending: Ending? + internal var effects: [Int: ImportFileEffect] = [:] + internal var preparations: [UUID: ImportPreparationEffect] = [:] + internal var errors: [String] = [] + internal var cleanupErrors: [String] = [] + + internal var hasWrites: Bool { effects.values.contains { $0.admittedWrite } } + + internal mutating func observe(_ state: ImportSessionState, finalizing: Bool = false) { + if finalizing, state != currentState { + uncertainHistory = true + if ending != nil { ending = .unknown } + } + if !finalizing { + if state == .unknown || (currentState.holdsTransaction && state == .idle) { + uncertainHistory = true + } + if holder == .none, state.holdsTransaction { uncertainHistory = true } + } + currentState = state + } + + internal mutating func recordError(_ error: any Error) { + let description = error.localizedDescription + if errors.count < 1_000, !errors.contains(description) { errors.append(description) } + } + + internal mutating func recordCleanupError(_ error: any Error) { + let description = error.localizedDescription + if !cleanupErrors.contains(description) { cleanupErrors.append(description) } + } + + internal var canChooseCommit: Bool { + hasWrites && holder == .owned && currentState == .open && ending == nil + && !uncertainHistory && !unseenStatements + && !hasUncertainEffects + } + + private var hasUncertainEffects: Bool { + effects.values.contains { + $0.admittedWrite && (($0.rollback == .unknown && $0.table != nil) + || ($0.failedWrite && $0.rollback == .nontransactional)) + } + } + + internal var question: String { rowQuestion + preparationNote } + + private var rowQuestion: String { + guard hasWrites else { + return preparations.isEmpty ? String(localized: "Nothing has been imported yet.") + : String(localized: "No rows from this run have been imported. Earlier table preparation is listed below.") + } + if uncertainHistory || unseenStatements || currentState == .unknown + || hasUncertainEffects { + return String(localized: "Anything already imported may stay.") + openTransactionNote + } + if holder != .owned, currentState.holdsTransaction { + return (currentState == .aborted + ? String(localized: "The open transaction failed. Roll it back yourself.") + : String(localized: "The changes are in an open transaction on this connection. Commit or roll it back yourself.")) + + nontransactionalNote + } + if currentState == .aborted { + return String(localized: "The import's transaction failed and can only be rolled back.") + nontransactionalNote + } + if canChooseCommit { + return String(localized: "Choose whether to roll back or commit the import's open transaction.") + + transactionalNote + nontransactionalNote + secondaryEffectsNote + } + switch ending { + case .committed: + return String(localized: "The import's transaction was already committed.") + nontransactionalNote + case .rolledBack: + return String(localized: "The import's transaction was already rolled back.") + nontransactionalNote + case .unknown: + return String(localized: "Anything already imported may stay.") + openTransactionNote + case nil: + return String(localized: "The statements already ran outside an import transaction. Their changes stay.") + + secondaryEffectsNote + } + } + + internal var preparationNote: String { + let lines = preparations.values.sorted { $0.table.table < $1.table.table }.map { effect in + let action: String + if effect.clearing { + action = effect.completed ? String(localized: "table cleared before row import") + : String(localized: "table clearing failed before row import") + } else { + action = effect.completed ? String(localized: "table creation completed before row import") + : String(localized: "table creation failed before row import") + } + var fate = effect.fate + if fate == .openTransaction || fate == .abortedTransaction { + if currentState == .aborted { fate = .abortedTransaction } else if currentState != .open || uncertainHistory { fate = .mayStay } + } + return String(format: String(localized: "%1$@: %2$@, %3$@."), effect.table.table, action, fate.text) + } + return lines.isEmpty ? "" : "\n\n" + lines.joined(separator: "\n") + } + + internal var openTransactionNote: String { + guard currentState.holdsTransaction else { return "" } + return "\n\n" + (currentState == .aborted + ? String(localized: "The open transaction failed. Roll it back on this connection.") + : String(localized: "A transaction is still open on this connection. Commit or roll it back.")) + } + + private var transactionalNote: String { + let tables = tableNames(for: .transactional) + guard !tables.isEmpty else { return "" } + return "\n\n" + String(format: String(localized: "Direct changes to %@ are in that transaction."), tables) + } + + private var nontransactionalNote: String { + let tables = tableNames(for: .nontransactional) + guard !tables.isEmpty else { return "" } + return "\n\n" + String(format: String(localized: "Changes to %@ cannot roll back and stay either way."), tables) + } + + private var secondaryEffectsNote: String { + "\n\n" + String(localized: "Changes a rollback cannot undo stay either way.") + } + + private func tableNames(for rollback: ImportTableRollback) -> String { + let names = Set(effects.values.filter { $0.admittedWrite && $0.rollback == rollback }.compactMap(\.table)) + return ListFormatter.localizedString(byJoining: names.sorted()) + } +} + +internal struct ImportStopQuestion: Equatable, Identifiable, Sendable { + internal let generation: Int + internal let ledger: ImportLedger + internal var id: Int { generation } +} + +internal enum ImportFileFate: Equatable, Sendable { + case notImported + case mayStay + case kept + case committed + case rolledBack + case openTransaction + case abortedTransaction + + internal var text: String { + switch self { + case .notImported: return String(localized: "not imported") + case .mayStay: return String(localized: "changes may stay") + case .kept: return String(localized: "changes kept") + case .committed: return String(localized: "changes committed") + case .rolledBack: return String(localized: "direct changes rolled back") + case .openTransaction: return String(localized: "changes are in the open transaction") + case .abortedTransaction: return String(localized: "the transaction failed and needs rollback") + } + } +} + +internal struct ImportStopResult: Equatable, Sendable { + internal let ledger: ImportLedger + + internal func fate(for index: Int) -> ImportFileFate { + guard let effect = ledger.effects[index], effect.admittedWrite else { return .notImported } + if ledger.uncertainHistory || ledger.unseenStatements || effect.rollback == .unknown + || (effect.failedWrite && effect.rollback == .nontransactional) { + return .mayStay + } + if effect.rollback == .nontransactional { return .kept } + if ledger.holder == .owned { + switch ledger.ending { + case .rolledBack: return .rolledBack + case .committed: return .committed + case .unknown: return .mayStay + case nil: break + } + } + if ledger.holder != .owned, ledger.currentState.holdsTransaction { + return ledger.currentState == .aborted ? .abortedTransaction : .openTransaction + } + if ledger.holder == .none, ledger.currentState == .idle { return .kept } + return .mayStay + } + + internal func fileText(for index: Int) -> String { + guard let effect = ledger.effects[index] else { return ImportFileFate.notImported.text } + let count: String + if effect.table == nil { + count = effect.statements == 1 ? String(localized: "1 statement completed.") + : String(format: String(localized: "%lld statements completed."), Int64(effect.statements)) + } else { + count = effect.submittedRows == 1 ? String(localized: "1 row submitted in a completed write.") + : String(format: String(localized: "%lld rows submitted in completed writes."), Int64(effect.submittedRows)) + } + let fate = fate(for: index) + let outcome: String + if effect.table == nil, fate == .rolledBack { + outcome = String(localized: "the import's transaction was rolled back") + } else if effect.table == nil, fate == .committed { + outcome = String(localized: "the import's transaction was committed") + } else { + outcome = fate.text + } + var parts = [count, outcome] + if effect.deletedRows { parts.append(String(localized: "The table was emptied before importing rows.")) } + if effect.failedWrite { + parts.append(String(localized: "A failed write may have made additional changes.")) + } + return parts.joined(separator: " ") + } + + internal var completionNotes: String { + ([ledger.openTransactionNote.trimmingCharacters(in: .whitespacesAndNewlines)] + ledger.cleanupErrors) + .filter { !$0.isEmpty }.joined(separator: "\n") + } + + internal var failureText: String { + let shown = ledger.cleanupErrors + ledger.errors.prefix(3).filter { !ledger.cleanupErrors.contains($0) } + var text = shown.joined(separator: "\n") + if ledger.errors.count > 3 { + text += "\n" + String( + format: String(localized: "%lld additional errors are in Copy Details."), + Int64(ledger.errors.count - 3) + ) + } + return text + } + + internal var details: String { + text + "\n\n" + (ledger.cleanupErrors + ledger.errors).joined(separator: "\n") + } + + internal var text: String { + var parts = ledger.effects.keys.sorted().map { index in + let detail = fileText(for: index) + guard let table = ledger.effects[index]?.table else { return detail } + return String(format: String(localized: "%1$@: %2$@"), table, detail) + } + if parts.isEmpty, ledger.preparations.isEmpty { parts.append(String(localized: "Nothing was imported.")) } + if !ledger.preparationNote.isEmpty { parts.append(ledger.preparationNote.trimmingCharacters(in: .whitespacesAndNewlines)) } + parts.append(String(localized: "Changes a rollback cannot undo stay either way.")) + if !ledger.openTransactionNote.isEmpty { parts.append(ledger.openTransactionNote.trimmingCharacters(in: .whitespacesAndNewlines)) } + if !failureText.isEmpty { parts.append(failureText) } + return parts.joined(separator: "\n\n") + } +} diff --git a/apple/TablePro/Core/Services/Export/ImportRun.swift b/apple/TablePro/Core/Services/Export/ImportRun.swift new file mode 100644 index 0000000000..70fa682d39 --- /dev/null +++ b/apple/TablePro/Core/Services/Export/ImportRun.swift @@ -0,0 +1,305 @@ +// +// ImportRun.swift +// TablePro +// + +import Foundation +import TableProPluginKit + +internal enum ImportStopChoice: Equatable, Sendable { + case rollBack + case commit +} + +internal enum ImportCallKind: Equatable, Sendable { + case data + case begin + case commit + case rollback + case session +} + +internal enum ImportAdmission: Equatable, Sendable { + case proceed + case skip +} + +internal enum ImportRunPhase: Equatable, Sendable { + case running + case pausing + case paused(generation: Int) + case stopping(ImportStopChoice) + case finished +} + +// One import run's gate and ledger. Every database call the run's sinks make passes `admit` and `release`, so +// once the gate is paused nothing is in flight and the ledger cannot change until the pause ends. +internal final class ImportRun: @unchecked Sendable { + private let lock = NSLock() + private var phase = ImportRunPhase.running + private var inFlight = 0 + private var generation = 0 + private var chosenStop: ImportStopChoice? + private var waiters: [UUID: CheckedContinuation] = [:] + private var ledger = ImportLedger() + private var onChange: (@Sendable () -> Void)? + + internal var currentPhase: ImportRunPhase { + lock.withLock { phase } + } + + internal var stopChoice: ImportStopChoice? { lock.withLock { chosenStop } } + + internal var waitingCallCount: Int { lock.withLock { waiters.count } } + + internal func observe(_ handler: @escaping @Sendable () -> Void) { + lock.withLock { onChange = handler } + } + + internal var snapshot: ImportLedger { + lock.withLock { ledger } + } + + @discardableResult + internal func record(_ body: (inout ImportLedger) -> T) -> T { + lock.withLock { body(&ledger) } + } + + // Read under the same lock as the phase, so a question is never built from a pause that already ended. + internal func questionIfPaused() -> (generation: Int, ledger: ImportLedger)? { + lock.withLock { + guard case .paused(let current) = phase else { return nil } + return (current, ledger) + } + } + + internal func admit(_ kind: ImportCallKind) async throws -> ImportAdmission { + while true { + let cancelled = Task.isCancelled + let (decision, wakes) = lock.withLock { decide(kind, taskCancelled: cancelled) } + finishTransition(wakes) + switch decision { + case .proceed: + return .proceed + case .skip: + return .skip + case .refuse: + throw PluginImportCancellationError() + case .wait: + await park() + } + } + } + + internal func release() { + let (changed, wakes) = lock.withLock { () -> (Bool, [CheckedContinuation]) in + inFlight = max(0, inFlight - 1) + guard inFlight == 0 else { return (false, []) } + if phase == .pausing { + generation += 1 + phase = .paused(generation: generation) + return (true, []) + } + if case .paused = phase { return (false, []) } + return (false, takeWaiters()) + } + wakes.forEach { $0.resume() } + if changed { notify() } + } + + @discardableResult + internal func requestPause() -> Bool { + let changed = lock.withLock { () -> Bool in + guard phase == .running else { return false } + if inFlight == 0 { + generation += 1 + phase = .paused(generation: generation) + } else { + phase = .pausing + } + return true + } + if changed { notify() } + return changed + } + + // An answer to a question asked at `generation`: ignored once that pause has ended. + @discardableResult + internal func resume(generation asked: Int) -> Bool { + transition { phase in + guard phase == .paused(generation: asked) else { return false } + phase = .running + return true + } + } + + // A disappearing question must not admit a parked COMMIT before teardown cancels the task. + @discardableResult + internal func stopUnansweredPause() -> Bool { + transition { phase in + switch phase { + case .pausing, .paused: + phase = .stopping(.rollBack) + return true + case .running, .stopping, .finished: + return false + } + } + } + + @discardableResult + internal func stop(_ choice: ImportStopChoice, generation asked: Int) -> Bool { + transition { phase in + guard phase == .paused(generation: asked) else { return false } + phase = .stopping(choice) + return true + } + } + + // Teardown and cancellation: no question, and it wins over any pause. + @discardableResult + internal func stop(_ choice: ImportStopChoice) -> Bool { + transition { phase in + switch phase { + case .stopping, .finished: + return false + case .running, .pausing, .paused: + phase = .stopping(choice) + return true + } + } + } + + @discardableResult + internal func finish() -> Bool { + transition { phase in + guard inFlight == 0 else { return false } + switch phase { + case .running, .stopping: + phase = .finished + return true + case .pausing, .paused, .finished: + return false + } + } + } + + internal func complete() async { + if Task.isCancelled { stop(.rollBack) } + while currentPhase != .finished { + if finish() { return } + await park(untilIdle: true) + } + } + + // MARK: - Private + + private enum Decision { + case proceed + case skip + case refuse + case wait + } + + private func decide(_ kind: ImportCallKind, taskCancelled: Bool) -> (Decision, [CheckedContinuation]?) { + var wakes: [CheckedContinuation]? + if taskCancelled, latchesStop { + phase = .stopping(.rollBack) + chosenStop = .rollBack + wakes = takeWaiters() + } + let decision: Decision + switch phase { + case .running: + decision = .proceed + case .finished: + decision = .refuse + case .pausing, .paused: + decision = .wait + case .stopping(let choice): + decision = Self.admission(of: kind, under: choice) + } + if decision == .proceed { inFlight += 1 } + return (decision, wakes) + } + + private var latchesStop: Bool { + switch phase { + case .running, .pausing, .paused: return true + case .stopping, .finished: return false + } + } + + private static func admission(of kind: ImportCallKind, under choice: ImportStopChoice) -> Decision { + switch (kind, choice) { + case (.data, _), (.begin, _), (.commit, .rollBack): + return .refuse + case (.commit, .commit), (.rollback, .rollBack), (.session, _): + return .proceed + case (.rollback, .commit): + return .skip + } + } + + // Registers under the lock after reading the phase again, so a stop or resume that ran first is never missed. + private func park(untilIdle: Bool = false) async { + let id = UUID() + await withTaskCancellationHandler { + await withCheckedContinuation { (continuation: CheckedContinuation) in + let parked = lock.withLock { () -> Bool in + switch phase { + case .pausing, .paused: + waiters[id] = continuation + return true + case .running, .stopping: + if untilIdle, inFlight > 0 { + waiters[id] = continuation + return true + } + return false + case .finished: + return false + } + } + if !parked { continuation.resume() } + } + } onCancel: { + stop(.rollBack) + } + } + + private func takeWaiters() -> [CheckedContinuation] { + let pending = Array(waiters.values) + waiters.removeAll() + return pending + } + + @discardableResult + private func transition(_ change: (inout ImportRunPhase) -> Bool) -> Bool { + let wakes = lock.withLock { () -> [CheckedContinuation]? in + guard change(&phase) else { return nil } + if case .stopping(let choice) = phase { chosenStop = choice } + switch phase { + case .pausing, .paused: + return [] + case .running, .stopping, .finished: + return takeWaiters() + } + } + guard let wakes else { return false } + wakes.forEach { $0.resume() } + notify() + return true + } + + private func finishTransition(_ wakes: [CheckedContinuation]?) { + guard let wakes else { return } + wakes.forEach { $0.resume() } + notify() + } + + private func notify() { + let handler = lock.withLock { onChange } + handler?() + } +} diff --git a/apple/TablePro/Core/Services/Export/ImportService.swift b/apple/TablePro/Core/Services/Export/ImportService.swift index 424e152755..3f0e86c491 100644 --- a/apple/TablePro/Core/Services/Export/ImportService.swift +++ b/apple/TablePro/Core/Services/Export/ImportService.swift @@ -43,7 +43,11 @@ final class ImportService: ObservableObject { private let runningWork: RunningWorkRegistry @Published private var currentProgress: PluginImportProgress? // A Stop before a file's progress exists, while a table is created or between two files. Never reset. - private var isStopRequested = false + internal let run = ImportRun() + @Published private(set) var stopQuestion: ImportStopQuestion? + @Published private(set) var stopPhase = ImportRunPhase.running + private var isStopRequested: Bool { run.stopChoice != nil } + var stopResult: ImportStopResult { ImportStopResult(ledger: run.snapshot) } init( connection: DatabaseConnection, @@ -55,6 +59,9 @@ final class ImportService: ObservableObject { self.historyRecorder = historyRecorder self.gate = gate self.runningWork = runningWork + run.observe { [weak self] in + Task { @MainActor [weak self] in self?.syncStopState() } + } } private func beginRunningWork(named subject: String) -> UUID { @@ -71,8 +78,33 @@ final class ImportService: ObservableObject { // MARK: - Cancellation func cancelImport() { - isStopRequested = true + run.stop(.rollBack) currentProgress?.cancel() + syncStopState() + } + + func requestStop() { + run.requestPause() + syncStopState() + } + + func answerStop(_ choice: ImportStopChoice?, generation: Int) { + if let choice { + if run.stop(choice, generation: generation) { currentProgress?.cancel() } + } else { + run.resume(generation: generation) + } + syncStopState() + } + + func stopUnansweredQuestion() { + if run.stopUnansweredPause() { currentProgress?.cancel() } + syncStopState() + } + + private func syncStopState() { + stopPhase = run.currentPhase + stopQuestion = run.questionIfPaused().map { ImportStopQuestion(generation: $0.generation, ledger: $0.ledger) } } // MARK: - Public API @@ -92,6 +124,39 @@ final class ImportService: ObservableObject { sourceFields: Set = [], rowWriting: ImportRowWriting = .insert, authorization: OperationReceipt? = nil + ) async throws -> PluginImportResult { + do { + let result = try await performImportFile( + from: url, sourceName: sourceName, formatId: formatId, encoding: encoding, scope: scope, + decompressedURL: decompressedURL, ownsDecompressedFile: ownsDecompressedFile, + knownStatementCount: knownStatementCount, targetTable: targetTable, columnMapping: columnMapping, + sourceFields: sourceFields, rowWriting: rowWriting, authorization: authorization + ) + await run.complete() + syncStopState() + if run.stopChoice != nil { throw PluginImportCancellationError() } + return result + } catch { + await run.complete() + syncStopState() + throw error + } + } + + private func performImportFile( + from url: URL, + sourceName: String? = nil, + formatId: String, + encoding: String.Encoding, + scope: DatabaseScope, + decompressedURL: URL? = nil, + ownsDecompressedFile: Bool = false, + knownStatementCount: Int? = nil, + targetTable: String? = nil, + columnMapping: [String: String] = [:], + sourceFields: Set = [], + rowWriting: ImportRowWriting = .insert, + authorization: OperationReceipt? = nil ) async throws -> PluginImportResult { guard let plugin = PluginManager.shared.importPlugin(forFormat: formatId) else { throw PluginImportError.importFailed("Import format '\(formatId)' not found") @@ -143,12 +208,6 @@ final class ImportService: ObservableObject { let startedAt = Date() let operationStart = ContinuousClock.Instant.now do { - /// The whole import runs inside one lease, because the plugin's own `BEGIN` spans the - /// run: a per-statement lease would let another tab's statement execute inside the - /// import's transaction and be committed or rolled back with it. Taking the lease is - /// also what registers the import, so the health check no longer enters the same - /// non-thread-safe driver partway through a batch of inserts. `.protectedWrite` - /// because an import writes, so Stop in another tab must not reach it. result = try await DatabaseManager.shared.withScopedDriver( scope: scope, route: route, @@ -173,9 +232,6 @@ final class ImportService: ObservableObject { // An import the user cancelled is not a failed import, and the query paths already // keep cancellations out of history for the same reason. guard !(error is PluginImportCancellationError) else { throw error } - /// Cancelling while the import is still queued behind another tab's operation throws - /// before a single statement runs. That is the same "they stopped it" case, arriving - /// from the gate rather than from the plugin. guard !(error is CancellationError) else { throw error } await historyRecorder.record( @@ -250,14 +306,29 @@ final class ImportService: ObservableObject { // MARK: - Several files - /// Imports each file into its own table in one lease, one session scope and, unless the policy turns it off, - /// one transaction, so a failure in any file takes back every file under Stop and Rollback. A failure or a Stop - /// is in the report rather than thrown, because the report says what became of the files before it. func importFiles( _ files: [RowImportRunItem], scope: DatabaseScope, policy: RowImportBatchPolicy, authorization: OperationReceipt? = nil + ) async throws -> RowImportBatchReport { + do { + let result = try await performImportFiles(files, scope: scope, policy: policy, authorization: authorization) + await run.complete() + syncStopState() + return result + } catch { + await run.complete() + syncStopState() + throw error + } + } + + private func performImportFiles( + _ files: [RowImportRunItem], + scope: DatabaseScope, + policy: RowImportBatchPolicy, + authorization: OperationReceipt? = nil ) async throws -> RowImportBatchReport { let plugins: [any ImportFormatPlugin] = try files.map { file in guard let plugin = PluginManager.shared.importPlugin(forFormat: file.formatId) else { @@ -282,7 +353,8 @@ final class ImportService: ObservableObject { outcomes: files.map { _ in .notRun }, ending: .stopped(file: 0), transaction: .untracked, - executionTime: 0 + executionTime: 0, + stopResult: stopResult ) } @@ -328,59 +400,77 @@ final class ImportService: ObservableObject { driver: DatabaseDriver, policy: RowImportBatchPolicy ) async throws -> RowImportBatchReport { - let started = Date() - let owner = ImportDataSinkAdapter(driver: driver, databaseType: connection.type) - let sessionScope = await openPreservingScope(on: driver) - if policy.usesTransaction { - do { - try await owner.beginTransaction() - } catch { - await closePreservingScope(sessionScope, on: driver) - throw error - } - } + let control = run + return try await withTaskCancellationHandler { + try await self.runFilesControlled(files, plugins: plugins, driver: driver, policy: policy) + } onCancel: { control.stop(.rollBack) } + } + private func runFilesControlled( + _ files: [RowImportRunItem], + plugins: [any ImportFormatPlugin], + driver: DatabaseDriver, + policy: RowImportBatchPolicy + ) async throws -> RowImportBatchReport { + let started = Date() + let owner = ImportDataSinkAdapter(driver: driver, databaseType: connection.type, importRun: run, fileIndex: nil) + var sessionScope: PluginSessionScope? var outcomes = [RowImportBatchReport.FileOutcome](repeating: .notRun, count: files.count) var ending = RowImportBatchReport.Ending.completed - for (index, file) in files.enumerated() { - guard !isStopRequested, !Task.isCancelled else { - outcomes[index] = .stopped(rowsBefore: 0) - ending = .stopped(file: index) - break - } - switch await runFile(file, plugin: plugins[index], index: index, driver: driver, policy: policy) { - case .imported(let result): - outcomes[index] = .imported(result) - case .stopped(let rows): - outcomes[index] = .stopped(rowsBefore: rows) - ending = .stopped(file: index) - case .failed(let error, let rows): - outcomes[index] = .failed(error, rowsBefore: rows) - if policy.errorHandling != .skipAndContinue { + var currentIndex = 0 + do { + try await owner.prepareImportRun() + sessionScope = try await owner.withImportSession { await self.openPreservingScope(on: driver) } + if policy.usesTransaction { try await owner.beginTransaction() } + for (index, file) in files.enumerated() { + currentIndex = index + try await owner.importCheckpoint() + let outcome = await runFile(file, plugin: plugins[index], index: index, driver: driver) + outcomes[index] = outcome + switch outcome { + case .stopped: + ending = .stopped(file: index) + case .failed where policy.errorHandling != .skipAndContinue: ending = .failed(file: index) + case .imported, .failed, .notRun: + break } - case .notRun: - break + guard ending == .completed else { break } } - guard ending == .completed else { break } - } - - if policy.usesTransaction { - do { - try await endTransaction(owner, after: ending, policy: policy) - } catch { - await closePreservingScope(sessionScope, on: driver) - throw error + try await owner.importCheckpoint() + } catch { + if error is PluginImportCancellationError || error is CancellationError { + run.stop(.rollBack) + ending = .stopped(file: currentIndex) + } else { + await owner.recordImportError(error) + if !files.isEmpty { outcomes[currentIndex] = .failed(error, rowsBefore: submittedRows(for: currentIndex)) } + ending = .failed(file: currentIndex) } } - await closePreservingScope(sessionScope, on: driver) - return RowImportBatchReport( - files: files, - outcomes: outcomes, - ending: ending, - transaction: owner.transactionOutcome, - executionTime: Date().timeIntervalSince(started) + let commits: Bool + switch ending { + case .completed: commits = true + case .failed: commits = policy.errorHandling == .stopAndCommit + case .stopped: commits = false + } + if let error = await finishOwner(owner, commit: commits), ending == .completed, !files.isEmpty { + outcomes[currentIndex] = .failed(error, rowsBefore: submittedRows(for: currentIndex)) + ending = .failed(file: currentIndex) + } + try? await owner.withImportSession { await self.closePreservingScope(sessionScope, on: driver) } + await run.complete() + syncStopState() + if run.stopChoice != nil { ending = .stopped(file: currentIndex) } + let report = RowImportBatchReport( + files: files, outcomes: outcomes, ending: ending, transaction: owner.transactionOutcome, + executionTime: Date().timeIntervalSince(started), stopResult: stopResult ) + return report + } + + private func submittedRows(for index: Int) -> Int { + run.snapshot.effects[index]?.submittedRows ?? 0 } @MainActor @@ -388,8 +478,7 @@ final class ImportService: ObservableObject { _ file: RowImportRunItem, plugin: any ImportFormatPlugin, index: Int, - driver: DatabaseDriver, - policy: RowImportBatchPolicy + driver: DatabaseDriver ) async -> RowImportBatchReport.FileOutcome { state.fileIndex = index state.fileName = file.sourceName @@ -412,56 +501,56 @@ final class ImportService: ObservableObject { targetTable: file.targetTable, columnMapping: file.columnMapping, sourceFields: file.sourceFields, - joinsTransaction: policy.usesTransaction, + joinsTransaction: true, + importRun: run, + fileIndex: index, isCancelled: { progress.isCancelled } ) do { + try await sink.prepareImportRun() return .imported(try await plugin.performImport(source: source, sink: sink, progress: progress)) } catch is PluginImportCancellationError { - return .stopped(rowsBefore: progress.processedStatements) + run.stop(.rollBack) + return .stopped(rowsBefore: submittedRows(for: index)) } catch is CancellationError { - return .stopped(rowsBefore: progress.processedStatements) + run.stop(.rollBack) + return .stopped(rowsBefore: submittedRows(for: index)) } catch { - return .failed(error, rowsBefore: progress.processedStatements) + await sink.recordImportError(error) + return .failed(error, rowsBefore: submittedRows(for: index)) } } - /// Stop and Commit keeps what ran before the failure, as one file's import does. A failure under Stop and - /// Rollback, and a Stop under any policy, take back every file. - @MainActor - private func endTransaction( - _ owner: ImportDataSinkAdapter, - after ending: RowImportBatchReport.Ending, - policy: RowImportBatchPolicy - ) async throws { - switch ending { - case .completed: + internal func finishOwner(_ owner: ImportDataSinkAdapter, commit: Bool) async -> (any Error)? { + var selected = run.stopChoice ?? (commit ? .commit : .rollBack) + while true { do { - try await owner.commitTransaction() + if selected == .commit { try await owner.commitTransaction() } else { try await owner.rollbackTransaction() } + } catch is PluginImportCancellationError { + if let winner = run.stopChoice, winner != selected { + selected = winner + continue + } + return PluginImportCancellationError() } catch { - try? await owner.rollbackTransaction() - throw error + await owner.recordImportError(error) + return error } - case .failed where policy.errorHandling == .stopAndCommit: - do { - try await owner.commitTransaction() - } catch { - Self.logger.warning("Commit of a partial import failed: \(error.publicLogShape, privacy: .public)") - } - case .failed, .stopped: - do { - try await owner.rollbackTransaction() - } catch { - Self.logger.warning("Rollback of a failed import failed: \(error.publicLogShape, privacy: .public)") + if let winner = run.stopChoice, winner != selected { + selected = winner + continue } + return nil } } - /// A file's row is recorded only while its rows are still there, so a rolled-back run records the failure alone. private func recordHistory(of report: RowImportBatchReport, in scope: DatabaseScope) async { - for (file, outcome) in zip(report.files, report.outcomes) { + for (index, pair) in zip(report.files, report.outcomes).enumerated() { + let (file, outcome) = pair + let fate = report.stopResult?.fate(for: index) + let keepsFile = report.rowsKept || fate == .kept || fate == .committed switch outcome { - case .imported(let result) where report.rowsKept: + case .imported(let result) where keepsFile: await recordImport(of: file.sourceName, statements: result.executedStatements, executionTime: result.executionTime, error: nil, in: scope) case .failed(let error, let rows): @@ -498,8 +587,6 @@ final class ImportService: ObservableObject { ) } - /// Before the lease, because a confirmation or a Touch ID prompt answered while it is held keeps - /// every other tab on this connection waiting on the person. private func authorizeRun(named fileName: String, given authorization: OperationReceipt?) async throws { if let authorization, authorization.connectionId == connection.id, authorization.effectiveWrite { return @@ -524,8 +611,6 @@ final class ImportService: ObservableObject { return { SQLLexicalResolver.readings(for: databaseType, session: driver.sessionLexicalState).execution } } - /// The sink is built here rather than before the lease so it cannot outlive the driver it - /// wraps, and so every statement it issues lands on the leased, pinned connection. @MainActor private func runImport( plugin: any ImportFormatPlugin, @@ -536,36 +621,65 @@ final class ImportService: ObservableObject { columnMapping: [String: String], sourceFields: Set, rowWriting: ImportRowWriting + ) async throws -> PluginImportResult { + let control = run + return try await withTaskCancellationHandler { + try await self.runImportControlled( + plugin: plugin, driver: driver, source: source, progress: progress, targetTable: targetTable, + columnMapping: columnMapping, sourceFields: sourceFields, rowWriting: rowWriting + ) + } onCancel: { control.stop(.rollBack) } + } + + private func runImportControlled( + plugin: any ImportFormatPlugin, + driver: DatabaseDriver, + source: any PluginImportSource, + progress: PluginImportProgress, + targetTable: String?, + columnMapping: [String: String], + sourceFields: Set, + rowWriting: ImportRowWriting ) async throws -> PluginImportResult { let sink = ImportDataSinkAdapter( - driver: driver, - databaseType: connection.type, - targetTable: targetTable, - columnMapping: columnMapping, - sourceFields: sourceFields, - rowWriting: rowWriting, - isCancelled: { progress.isCancelled } + driver: driver, databaseType: connection.type, targetTable: targetTable, + columnMapping: columnMapping, sourceFields: sourceFields, rowWriting: rowWriting, + importRun: run, isCancelled: { progress.isCancelled } ) defer { reportNameHazards(sink.nameHazardStatements) state.transactionOutcome = sink.transactionOutcome state.rowTally = targetTable == nil ? nil : sink.rowTally } - let sessionScope = await openPreservingScope(on: driver) - let result: PluginImportResult + var sessionScope: PluginSessionScope? + var result: PluginImportResult? + var failure: (any Error)? do { + try await sink.prepareImportRun() + sessionScope = try await sink.withImportSession { await self.openPreservingScope(on: driver) } result = try await plugin.performImport(source: source, sink: sink, progress: progress) + try await sink.importCheckpoint() } catch { - await closePreservingScope(sessionScope, on: driver) - guard sink.transactionOutcome == .committedBeforeRollback else { throw error } - throw ImportCommittedTransactionError.noting(error) + failure = error + if error is PluginImportCancellationError || error is CancellationError { + run.stop(.rollBack) + } else { + await sink.recordImportError(error) + } + } + if let error = await finishOwner(sink, commit: failure == nil), failure == nil { failure = error } + try? await sink.withImportSession { await self.closePreservingScope(sessionScope, on: driver) } + await run.complete() + syncStopState() + if run.stopChoice != nil { throw PluginImportCancellationError() } + if let failure { + if sink.transactionOutcome == .committedBeforeRollback { throw ImportCommittedTransactionError.noting(failure) } + throw failure } - await closePreservingScope(sessionScope, on: driver) + guard let result else { throw PluginImportError.importFailed(String(localized: "Import did not finish.")) } return result } - /// The file runs on the editor's own session, so whatever it sets is put back once the import ends, however it - /// ends. private func openPreservingScope(on driver: DatabaseDriver) async -> PluginSessionScope? { do { return try await driver.openSessionScope(for: .preserve) @@ -580,14 +694,13 @@ final class ImportService: ObservableObject { do { try await driver.closeSessionScope(scope) } catch { + run.record { $0.recordCleanupError(error); $0.uncertainHistory = true } Self.logger.warning( "Could not restore the session settings after the import: \(error.publicLogShape, privacy: .public)" ) } } - /// The file ran on the same session as the editor, so a temporary table it made shadows a real one there - /// for as long as the connection stays open, and has to reach the catalog like any statement the editor ran. private func reportNameHazards(_ statements: [String]) { guard !statements.isEmpty else { return } CatalogChangeService.post( @@ -595,11 +708,6 @@ final class ImportService: ObservableObject { ) } - /// An import the user cancelled reports nothing, matching what history already does with one - /// and for the same reason: they stopped it, so they know. - /// - /// Named after the scope the import ran in, as its history row is. The browse database read when - /// the import ends is wherever another window moved the connection in the meantime. private func reportImportFinished( _ outcome: OperationOutcome, in scope: DatabaseScope, @@ -633,8 +741,6 @@ internal struct ImportCommittedTransactionError: LocalizedError { "\(cause.localizedDescription)\n\(Self.note)" } - /// A statement failure keeps its shape, because the alert reads its line and statement from it. A cancel stays a - /// cancel: its own alert already says what ran stays committed. internal static func noting(_ error: any Error) -> any Error { guard !(error is PluginImportCancellationError), !(error is CancellationError) else { return error } if let pluginError = error as? PluginImportError, diff --git a/apple/TablePro/Core/Services/Export/RowImportBatchRun.swift b/apple/TablePro/Core/Services/Export/RowImportBatchRun.swift index 2df72439da..a4e1fd87fe 100644 --- a/apple/TablePro/Core/Services/Export/RowImportBatchRun.swift +++ b/apple/TablePro/Core/Services/Export/RowImportBatchRun.swift @@ -6,7 +6,6 @@ import Foundation import TableProPluginKit -/// One file of a run that imports several, with the table and mapping it was given. internal struct RowImportRunItem: Sendable, Equatable { internal let url: URL internal let sourceName: String @@ -16,8 +15,6 @@ internal struct RowImportRunItem: Sendable, Equatable { internal let sourceFields: Set } -/// What a run of several files does when one of them fails. Every file shares it, because they share one -/// transaction. internal struct RowImportBatchPolicy: Sendable, Equatable { internal let errorHandling: ImportErrorHandling internal let wrapInTransaction: Bool @@ -31,21 +28,20 @@ internal struct RowImportBatchPolicy: Sendable, Equatable { self.init(errorHandling: configuration.errorHandling, wrapInTransaction: configuration.wrapInTransaction) } - /// Skip and Continue keeps every row that went in, so it opens no transaction, as one file's import does not. internal var usesTransaction: Bool { wrapInTransaction && errorHandling != .skipAndContinue } } -internal struct RowImportBatchReport { - internal enum FileOutcome { +internal struct RowImportBatchReport: Sendable { + internal enum FileOutcome: Sendable { case imported(PluginImportResult) case failed(any Error, rowsBefore: Int) case stopped(rowsBefore: Int) case notRun } - internal enum Ending: Equatable { + internal enum Ending: Equatable, Sendable { case completed case failed(file: Int) case stopped(file: Int) @@ -56,9 +52,8 @@ internal struct RowImportBatchReport { internal let ending: Ending internal let transaction: ImportTransactionOutcome internal let executionTime: TimeInterval + internal var stopResult: ImportStopResult? - /// Whether the rows the run wrote are still there. A rollback takes back every file, the ones that finished - /// included. internal var rowsKept: Bool { switch transaction { case .rolledBack, .endedByServer: @@ -80,7 +75,6 @@ internal struct RowImportBatchReport { return error } - /// A row or a whole file that Skip and Continue left out. internal var hasFailures: Bool { outcomes.contains { outcome in if case .imported(let result) = outcome { return result.skippedStatements > 0 } diff --git a/apple/TablePro/Core/Services/Export/RowImportTableMaker.swift b/apple/TablePro/Core/Services/Export/RowImportTableMaker.swift index 4215ed31d6..3e8c0b0efb 100644 --- a/apple/TablePro/Core/Services/Export/RowImportTableMaker.swift +++ b/apple/TablePro/Core/Services/Export/RowImportTableMaker.swift @@ -7,13 +7,11 @@ import Foundation import TableProPluginKit import TableProSQLGrammar -/// Creates the tables a row import fills, or clears one it created on an earlier attempt, behind one gate grant -/// that the rows reuse. One prompt covers every table of a run, so importing twenty files asks once. @MainActor internal struct RowImportTableMaker { private enum Step { case create(TableScope, statements: [String], sql: String) - case clear(String) + case clear(TableScope, sql: String) } internal let connection: DatabaseConnection @@ -31,13 +29,36 @@ internal struct RowImportTableMaker { ) } - /// `onCreated` hears of each table as soon as it exists, so a failure on a later one still leaves the planner - /// knowing the earlier ones are this sheet's to reuse. internal func prepare( _ definitions: [PluginCreateTableDefinition], scope: DatabaseScope, planner: NewTableImportPlanner, description: String? = nil, + importRun: ImportRun? = nil, + onCreated: (TableScope, String) -> Void + ) async throws -> OperationReceipt { + do { + return try await prepareControlled(definitions, scope: scope, planner: planner, description: description, + importRun: importRun, onCreated: onCreated) + } catch { + if error is PluginImportCancellationError || error is CancellationError { + importRun?.stop(.rollBack) + } else if let importRun, (try? await importRun.admit(.session)) == .proceed { + importRun.record { $0.recordError(error) } + importRun.release() + } + await importRun?.complete() + if importRun?.stopChoice != nil { throw PluginImportCancellationError() } + throw error + } + } + + private func prepareControlled( + _ definitions: [PluginCreateTableDefinition], + scope: DatabaseScope, + planner: NewTableImportPlanner, + description: String? = nil, + importRun: ImportRun? = nil, onCreated: (TableScope, String) -> Void ) async throws -> OperationReceipt { guard ImportDataSinkAdapter.canWriteRows(into: connection.type) else { @@ -53,7 +74,6 @@ internal struct RowImportTableMaker { let receipt = try await authorize(steps, description: description) var createdAny = false - /// Also when a later table fails, since the ones before it are in the database. defer { if createdAny { CatalogChangeService.post( @@ -65,12 +85,12 @@ internal struct RowImportTableMaker { switch step { case .create(let table, let statements, let sql): for statement in statements { - try await runOnLeasedDriver(statement, scope: scope) + try await runOnLeasedDriver(statement, scope: scope, table: table, clearing: false, run: importRun) createdAny = true } onCreated(table, sql) - case .clear(let sql): - try await runOnLeasedDriver(sql, scope: scope) + case .clear(let table, let sql): + try await runOnLeasedDriver(sql, scope: scope, table: table, clearing: true, run: importRun) } } return receipt @@ -103,7 +123,7 @@ internal struct RowImportTableMaker { quoteIdentifier: driver?.quoteIdentifier, rowDeleteStatement: driver?.rowDeleteStatement ) - return .clear(generator.deleteAllRowsStatement()) + return .clear(table, sql: generator.deleteAllRowsStatement()) case .nameTakenWithDifferentColumns: throw PluginImportError.importFailed( String( @@ -114,9 +134,6 @@ internal struct RowImportTableMaker { } } - /// Every statement the sheet issues on its own account goes through the gate. The retry path's `DELETE FROM` - /// used to skip it while the `CREATE TABLE` beside it did not, so a connection set to confirm destructive - /// statements emptied a table without asking. private func authorize(_ steps: [Step], description: String?) async throws -> OperationReceipt { var statements: [String] = [] var clears = false @@ -124,7 +141,7 @@ internal struct RowImportTableMaker { switch step { case .create(_, let created, _): statements += created - case .clear(let sql): + case .clear(_, let sql): statements.append(sql) clears = true } @@ -150,17 +167,64 @@ internal struct RowImportTableMaker { return receipt } - /// One statement at a time on the import's own kind of lease, always after `authorize` has returned. Taking it - /// earlier would hold the connection's gate open across a confirmation the user has not answered yet, and the - /// gate is not reentrant, so the import that follows would then wait on a sheet waiting on the user. - private func runOnLeasedDriver(_ sql: String, scope: DatabaseScope) async throws { + private func runOnLeasedDriver( + _ sql: String, scope: DatabaseScope, table: TableScope, clearing: Bool, run: ImportRun? + ) async throws { + if let run { _ = try await run.admit(.data) } + defer { run?.release() } let route = DatabaseManager.shared.executionRoute(for: scope) - _ = try await DatabaseManager.shared.withScopedDriver( - scope: scope, - route: route, - cancellation: .protectedWrite + try await DatabaseManager.shared.withScopedDriver( + scope: scope, route: route, cancellation: .protectedWrite ) { driver in - try await driver.execute(query: sql) + if let run { + try await Self.executePreparation(sql, table: table, clearing: clearing, driver: driver, run: run) + } else { + _ = try await driver.execute(query: sql) + } + } + } + + nonisolated private static func executePreparation( + _ sql: String, table: TableScope, clearing: Bool, driver: DatabaseDriver, run: ImportRun + ) async throws { + let id = UUID() + let rollback: ImportTableRollback + if !clearing { + rollback = .unknown + } else if !driver.supportsTransactions { + rollback = .nontransactional + } else if DataSyncTransactionality.needsStorageEngines(driver.connection.type) { + if let engine = try? await driver.fetchTableMetadata(tableName: table.table, schema: table.schema).engine, + !engine.isEmpty { + rollback = DataSyncTransactionality.cannotRollBack(storageEngine: engine, databaseType: driver.connection.type) + ? .nontransactional : .transactional + } else { + rollback = .unknown + } + } else { + rollback = .transactional + } + run.record { + $0.preparations[id] = ImportPreparationEffect(table: table, clearing: clearing) + } + do { + _ = try await driver.execute(query: sql) + let state = driver.supportsTransactions + ? ImportSessionState(await driver.heldSessionTransactionState()) : .idle + run.record { + $0.currentState = state + $0.preparations[id]?.completed = true + if clearing, rollback == .nontransactional || (rollback == .transactional && state == .idle) { + $0.preparations[id]?.fate = .kept + } else if clearing, rollback == .transactional, state.holdsTransaction { + $0.preparations[id]?.fate = state == .aborted ? .abortedTransaction : .openTransaction + } + } + } catch { + let state = driver.supportsTransactions + ? ImportSessionState(await driver.heldSessionTransactionState()) : .idle + run.record { $0.currentState = state; $0.recordError(error) } + throw error } } } diff --git a/apple/TablePro/Resources/Localizable.xcstrings b/apple/TablePro/Resources/Localizable.xcstrings index 466414e96e..32e8e1f460 100644 --- a/apple/TablePro/Resources/Localizable.xcstrings +++ b/apple/TablePro/Resources/Localizable.xcstrings @@ -7727,6 +7727,9 @@ } } } + }, + "%1$@: %2$@, %3$@." : { + }, "%1$@: %2$lld rows" : { "extractionState" : "manual", @@ -10553,6 +10556,9 @@ } } } + }, + "%lld additional errors are in Copy Details." : { + }, "%lld bytes" : { "localizations" : { @@ -11635,6 +11641,9 @@ } } } + }, + "%lld rows submitted in completed writes." : { + }, "%lld seconds" : { "localizations" : { @@ -11852,6 +11861,9 @@ } } } + }, + "%lld statements completed." : { + }, "%lld statements executed" : { "localizations" : { @@ -13630,6 +13642,9 @@ }, "1 row in %@ blocks the delete" : { + }, + "1 row submitted in a completed write." : { + }, "1 saved query found" : { @@ -13655,6 +13670,9 @@ } } } + }, + "1 statement completed." : { + }, "1 statement stays out of this run and %@ keeps what it has for it." : { "extractionState" : "stale", @@ -15773,6 +15791,9 @@ } } } + }, + "A failed write may have made additional changes." : { + }, "A field named “%@” can only be changed on MongoDB 5.0 or later, which can address a dot or a leading $ in a name." : { "extractionState" : "manual", @@ -17423,6 +17444,9 @@ }, "A transaction is open on this connection. Commit or roll it back in TablePro, then run the shortcut again." : { + }, + "A transaction is still open on this connection. Commit or roll it back." : { + }, "A tunnel forwards one address, and the node Sentinel names is on the server's own network, so Sentinel mode cannot run through one. Connect to a data node directly, or turn the tunnel off." : { "extractionState" : "manual", @@ -36199,6 +36223,9 @@ } } } + }, + "Changes a rollback cannot undo stay either way." : { + }, "Changes applied" : { "localizations" : { @@ -36239,9 +36266,21 @@ } } } + }, + "changes are in the open transaction" : { + + }, + "changes committed" : { + }, "Changes from iCloud could not be saved on this Mac. They download again on the next sync." : { + }, + "changes kept" : { + + }, + "changes may stay" : { + }, "Changes Not Saved" : { "extractionState" : "manual", @@ -36294,6 +36333,9 @@ "Changes the routine makes are committed when it finishes. Stop rolls them back." : { "extractionState" : "manual", "generatesSymbol" : false + }, + "Changes to %@ cannot roll back and stay either way." : { + }, "Changes Unavailable" : { @@ -39026,6 +39068,9 @@ } } } + }, + "Choose whether to roll back or commit the import's open transaction." : { + }, "Choose which columns the grid shows" : { "localizations" : { @@ -64870,6 +64915,12 @@ } } } + }, + "direct changes rolled back" : { + + }, + "Direct changes to %@ are in that transaction." : { + }, "Direct partitions" : { "localizations" : { @@ -69353,6 +69404,9 @@ } } } + }, + "Each file below reports what happened to its changes." : { + }, "Each row takes a value that exists in the referenced table." : { @@ -91825,6 +91879,9 @@ } } } + }, + "Import did not finish." : { + }, "Import failed" : { "comment" : "Title of the alert when an import fails.", @@ -119794,6 +119851,9 @@ } } } + }, + "No rows from this run have been imported. Earlier table preparation is listed below." : { + }, "No Rows Match" : { "localizations" : { @@ -123551,6 +123611,9 @@ } } } + }, + "Nothing has been imported yet." : { + }, "Nothing here matches the search." : { "localizations" : { @@ -123922,6 +123985,9 @@ } } } + }, + "Nothing was imported." : { + }, "Nothing was saved, and every change is still pending. Undo what cannot be written, or make it with a query, then save again." : { "localizations" : { @@ -133114,6 +133180,9 @@ } } } + }, + "Pausing…" : { + }, "Paw Print" : { "extractionState" : "manual", @@ -174678,6 +174747,9 @@ } } } + }, + "Stop and Commit" : { + }, "Stop and keep what ran" : { "localizations" : { @@ -174798,6 +174870,9 @@ } } } + }, + "Stop and Rollback" : { + }, "Stop Backup" : { @@ -178695,6 +178770,12 @@ } } } + }, + "table cleared before row import" : { + + }, + "table clearing failed before row import" : { + }, "Table collation differs: %@ on source, %@ on target." : { "localizations" : { @@ -178786,6 +178867,12 @@ }, "Table Created with Errors" : { + }, + "table creation completed before row import" : { + + }, + "table creation failed before row import" : { + }, "Table Data" : { @@ -185127,6 +185214,9 @@ }, "The change to this table's structure will be lost." : { + }, + "The changes are in an open transaction on this connection. Commit or roll it back yourself." : { + }, "The changes were edited while the save was checking other tables. Save again to include them." : { @@ -187093,6 +187183,9 @@ } } } + }, + "The details below show what happened to the import." : { + }, "The diagram could not be converted to a PNG image." : { "comment" : "Error message when the ER diagram could not be exported as PNG.", @@ -188173,6 +188266,21 @@ } } } + }, + "The import's transaction failed and can only be rolled back." : { + + }, + "The import's transaction was already committed." : { + + }, + "The import's transaction was already rolled back." : { + + }, + "the import's transaction was committed" : { + + }, + "the import's transaction was rolled back" : { + }, "The increment cannot be zero." : { @@ -189239,6 +189347,12 @@ } } } + }, + "The open transaction failed. Roll it back on this connection." : { + + }, + "The open transaction failed. Roll it back yourself." : { + }, "The operation failed." : { "localizations" : { @@ -193265,6 +193379,9 @@ "The statement's placeholders do not match the %d values given." : { "extractionState" : "manual", "generatesSymbol" : false + }, + "The statements already ran outside an import transaction. Their changes stay." : { + }, "The statements may or may not be saved. Check the table before running them again." : { "localizations" : { @@ -193521,6 +193638,9 @@ } } } + }, + "The table was emptied before importing rows." : { + }, "The table's primary key is not loaded, so the row cannot be written." : { "extractionState" : "manual", @@ -194273,6 +194393,9 @@ } } } + }, + "the transaction failed and needs rollback" : { + }, "The transaction on this connection can no longer be committed. Roll it back." : { "localizations" : { diff --git a/apple/TablePro/Views/Components/TransferResultAlert.swift b/apple/TablePro/Views/Components/TransferResultAlert.swift index ab66ad3699..41747e67df 100644 --- a/apple/TablePro/Views/Components/TransferResultAlert.swift +++ b/apple/TablePro/Views/Components/TransferResultAlert.swift @@ -7,9 +7,6 @@ import AppKit import TableProPluginKit import UniformTypeIdentifiers -/// Import and export results were three bespoke views with fixed widths and hand-picked green, -/// yellow and red badges. `NSAlert` supplies the icon from its style, sizes itself to its content, -/// and is the only one of the two that can carry a real suppression checkbox. @MainActor internal enum TransferResultAlert { internal static let exportSuppressionKey = "hideExportSuccessDialog" @@ -19,14 +16,6 @@ internal enum TransferResultAlert { case close } - /// An export that finished with something to report says so here, the way an import already - /// does. The suppression checkbox is offered only on a clean run: the alert the user turned - /// off is the routine one, and hiding a warning behind that switch loses it for good. - /// - /// `notes` are what the export wrote and `warnings` are what went wrong with it, so only the - /// second decides the title, the icon and whether the checkbox appears. Both go in the body, - /// notes first, which is the order `presentTransferSuccess` and `presentImportSuccess` already - /// put their own summaries in. internal static func presentExportSuccess( warnings: [String], notes: [String] = [], @@ -40,9 +29,6 @@ internal enum TransferResultAlert { alert.alertStyle = warnings.isEmpty ? .informational : .warning alert.informativeText = (notes + warnings).joined(separator: "\n\n") alert.addButton(withTitle: String(localized: "Open in Finder")) - /// `NSAlert` binds Escape by matching a button's title against "Cancel", which stops - /// matching in every localized build and never matched "Done" at all. Without this the - /// alert answers no key but Return, which opens Finder. AlertHelper.addCancelButton(to: alert, title: String(localized: "Done")) alert.showsSuppressionButton = warnings.isEmpty alert.suppressionButton?.title = String(localized: "Do not show this again") @@ -57,9 +43,6 @@ internal enum TransferResultAlert { AlertHelper.present(alert, in: window, completion: deliver) } - /// A transfer writes into another connection and leaves nothing on disk, so there is no folder - /// to open and no file to name. It still has to say how much moved and where, which it did not: - /// the sheet used to close on success and report nothing at all. internal static func presentTransferSuccess( tableCount: Int, rowCount: Int, @@ -91,19 +74,19 @@ internal enum TransferResultAlert { return String(format: template, Int64(rowCount), destinationName, Int64(tableCount)) } - /// A stopped import used to close its progress sheet without saying what it left behind, so stopping looked the - /// same as importing nothing. internal static func presentImportCancelled( - executedStatements: Int, - transaction: ImportTransactionOutcome, + result: ImportStopResult, window: NSWindow?, completion: @escaping @MainActor () -> Void ) { let alert = NSAlert() alert.messageText = String(localized: "Import stopped") alert.alertStyle = .warning - alert.informativeText = importCancelledText(executedStatements: executedStatements, transaction: transaction) + alert.informativeText = String(localized: "The details below show what happened to the import.") + if !result.completionNotes.isEmpty { alert.informativeText += "\n" + shownErrors(result.completionNotes) } + alert.accessoryView = TransferReportView(shown: result.text, copied: result.details) AlertHelper.addCancelButton(to: alert, title: String(localized: "Done")) + alert.layout() AlertHelper.present(alert, in: window) { _ in completion() } } @@ -125,7 +108,9 @@ internal enum TransferResultAlert { %lld statements had already run. Some were kept, because a statement in the file, such as \ COMMIT, CREATE, ALTER or DROP, committed the import's transaction as it ran. """) - case .untracked, .committed, .endedByServer, .endedByFile: + case .untracked, .endedByServer, .endedByFile: + return String(localized: "Anything already imported may stay.") + case .committed: template = isOne ? String(localized: "%lld statement had already run and stays committed.") : String(localized: "%lld statements had already run and stay committed.") @@ -136,6 +121,7 @@ internal enum TransferResultAlert { internal static func presentImportSuccess( result: PluginImportResult?, rowTally: ImportRowTally? = nil, + runResult: ImportStopResult? = nil, window: NSWindow?, sourceFileName: String = "", targetTable: String? = nil, @@ -149,13 +135,17 @@ internal enum TransferResultAlert { alert.alertStyle = skipped > 0 ? .warning : .informational alert.informativeText = importSummary(result, rowTally: rowTally) AlertHelper.addCancelButton(to: alert, title: String(localized: "Done")) + if let runResult, !runResult.completionNotes.isEmpty { + alert.informativeText += "\n" + shownErrors(runResult.completionNotes) + alert.accessoryView = TransferReportView(shown: runResult.text, copied: runResult.details) + alert.layout() + } let errors = result?.errors ?? [] if !errors.isEmpty { - /// The alert shows the first few, which is enough to recognise the shape of the - /// problem. Anything past that belongs in a file the user can sort and search. alert.addButton(withTitle: String(localized: "Save Report…")) - alert.accessoryView = TransferReportView(report: failureReport(for: errors)) + let details = failureReport(for: errors) + (runResult.map { "\n\n" + $0.details } ?? "") + alert.accessoryView = TransferReportView(report: details) alert.layout() } @@ -212,8 +202,6 @@ internal enum TransferResultAlert { } } - /// A run of several files says what became of each, with its row count, and whether a failure took back the files - /// before it. Copy Details carries every skipped row's error as well. internal static func presentBatchImportResult( _ report: RowImportBatchReport, window: NSWindow?, @@ -233,12 +221,14 @@ internal enum TransferResultAlert { alert.messageText = String(localized: "Import stopped") alert.alertStyle = .warning } - alert.informativeText = batchImportSummary(report) + alert.informativeText = shownErrors(batchImportSummary(report)) AlertHelper.addCancelButton(to: alert, title: String(localized: "Done")) let lines = batchImportLines(report) + let preparation = report.stopResult?.ledger.preparationNote.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" alert.accessoryView = TransferReportView( - shown: lines.joined(separator: "\n"), - copied: ([lines.joined(separator: "\n")] + batchImportErrors(report)).joined(separator: "\n\n") + shown: ([preparation] + lines).filter { !$0.isEmpty }.joined(separator: "\n"), + copied: ([lines.joined(separator: "\n")] + batchImportErrors(report) + + [report.stopResult?.details ?? ""]).joined(separator: "\n\n") ) alert.layout() AlertHelper.present(alert, in: window) { _ in completion() } @@ -253,14 +243,26 @@ internal enum TransferResultAlert { Int64(report.importedRowCount), Int64(report.files.count), seconds - ) + ) + (report.stopResult.map { $0.completionNotes.isEmpty ? "" : "\n" + $0.completionNotes } ?? "") case .failed(let index): + if let result = report.stopResult { + return [result.completionNotes, + String(localized: "Each file below reports what happened to its changes."), + String(localized: "Changes a rollback cannot undo stay either way."), result.failureText] + .filter { !$0.isEmpty }.joined(separator: "\n") + } let failure = importFailureText(for: report.failure) let template = report.rowsKept ? String(localized: "%1$@ failed, and the files imported before it were kept. %2$@") : String(localized: "%1$@ failed, so every file was rolled back. %2$@") return String(format: template, report.files[index].sourceName, failure) case .stopped(let index): + if let result = report.stopResult { + return [result.completionNotes, + String(localized: "Each file below reports what happened to its changes."), + String(localized: "Changes a rollback cannot undo stay either way."), result.failureText] + .filter { !$0.isEmpty }.joined(separator: "\n") + } let template = report.rowsKept ? String(localized: "Stopped at %@. The files imported before it were kept.") : String(localized: "Stopped at %@. Every file was rolled back.") @@ -268,9 +270,14 @@ internal enum TransferResultAlert { } } - /// One line per file: where its rows went and how many, or why none did. internal static func batchImportLines(_ report: RowImportBatchReport) -> [String] { - zip(report.files, report.outcomes).map { file, outcome in + if let result = report.stopResult, report.ending != .completed { + return report.files.enumerated().map { index, file in + String(format: String(localized: "%1$@ to %2$@: %3$@"), file.sourceName, file.targetTable, + result.fileText(for: index)) + } + } + return zip(report.files, report.outcomes).map { file, outcome in let status: String switch outcome { case .imported(let result): @@ -290,7 +297,6 @@ internal enum TransferResultAlert { } } - /// A file that stopped part way can already have written batches, which stay or go with the run. private static func rowsBefore(_ rows: Int, kept: Bool) -> String? { guard rows > 0 else { return nil } if rows == 1 { @@ -318,7 +324,6 @@ internal enum TransferResultAlert { return text } - /// Everything the lines cut short: each skipped row's error, and a failed file's whole error and statement. private static func batchImportErrors(_ report: RowImportBatchReport) -> [String] { zip(report.files, report.outcomes).compactMap { file, outcome in switch outcome { @@ -334,6 +339,7 @@ internal enum TransferResultAlert { internal static func presentImportFailure( error: (any Error)?, + result: ImportStopResult? = nil, window: NSWindow?, completion: @escaping @MainActor () -> Void ) { @@ -341,13 +347,18 @@ internal enum TransferResultAlert { alert.messageText = String(localized: "Import failed") alert.alertStyle = .critical alert.addButton(withTitle: String(localized: "Done")) - alert.informativeText = importFailureText(for: error) + alert.informativeText = importFailureDisplay(error: error, result: result) - if let pluginError = error as? PluginImportError, + if let result { + let details = importFailureSummary(error: error, result: result) + + "\n\n" + (failureReport(for: error) ?? "") + alert.accessoryView = TransferReportView(shown: details, copied: details + "\n\n" + result.details) + alert.layout() + } else if let pluginError = error as? PluginImportError, case .statementFailed(let statement, _, _) = pluginError { alert.accessoryView = TransferReportView( shown: statement, - copied: failureReport(for: error) ?? statement + copied: (failureReport(for: error) ?? statement) + "\n\n" + (result?.details ?? "") ) alert.layout() } @@ -355,7 +366,15 @@ internal enum TransferResultAlert { AlertHelper.present(alert, in: window) { _ in completion() } } - /// The text of a failed import's alert: the line it stopped at and the first of the database's errors. + internal static func importFailureDisplay(error: (any Error)?, result: ImportStopResult?) -> String { + let parts = [result?.completionNotes ?? "", importFailureText(for: error)] + return shownErrors(parts.filter { !$0.isEmpty }.joined(separator: "\n")) + } + + internal static func importFailureSummary(error: (any Error)?, result: ImportStopResult?) -> String { + importFailureText(for: error) + (result.map { "\n\n" + $0.text } ?? "") + } + internal static func importFailureText(for error: (any Error)?) -> String { guard let pluginError = error as? PluginImportError, case .statementFailed(_, let line, let underlyingError) = pluginError else { @@ -368,14 +387,9 @@ internal enum TransferResultAlert { )).plainText } - /// The most of a failure's errors an alert's text holds, in lines and in UTF-16 units. A SQL Server batch raises - /// one error per statement that failed and the driver keeps up to 1,000, one line each, while an alert grows to fit - /// its text: measured, 1,000 lines made one 54,135 points tall, its Done button far below the screen, and 1,000 - /// units on one line keep it at 423 points. Copy Details carries all of it. internal static let shownErrorLineLimit = 5 internal static let shownErrorLengthLimit = 1_000 - /// The start of `description` the alert has room for, and where the rest is. internal static func shownErrors(_ description: String) -> String { let lines = description.components(separatedBy: "\n") let firstLines = lines.prefix(shownErrorLineLimit).joined(separator: "\n") @@ -384,8 +398,6 @@ internal enum TransferResultAlert { return shown + "\n" + String(localized: "Copy Details copies the rest.") } - /// `text` cut to `limit` UTF-16 units on a character boundary, with an ellipsis saying there is more, or nil when - /// it already fits. internal static func cut(_ text: String, toUnits limit: Int) -> String? { let source = text as NSString guard source.length > limit else { return nil } @@ -393,9 +405,6 @@ internal enum TransferResultAlert { return source.substring(to: end) + "\u{2026}" } - /// The report a failed import puts in its accessory, so the line, the reason and the failing - /// statement can be selected and copied together. Returns nil for an error that carries no - /// statement, where the alert's own text already says everything there is to say. internal static func failureReport(for error: (any Error)?) -> String? { guard let pluginError = error as? PluginImportError, case .statementFailed(let statement, let line, let underlyingError) = pluginError else { @@ -410,8 +419,6 @@ internal enum TransferResultAlert { ]) } - /// A row import names its failing entry `row 12`, which the line number already says, so only - /// a statement that carries something the line number does not is worth repeating. internal static func failureReport(for failures: [PluginImportResult.ImportStatementError]) -> String { failures.map { failure in let heading = String( @@ -449,25 +456,13 @@ internal enum TransferResultAlert { } } -/// The failure report is what a user has to act on, so it is selectable text with a copy button -/// of its own. An `NSAlert` button cannot serve here because any of them dismisses the alert, so -/// the control belongs to the accessory view instead. -/// -/// What is copied can say more than what is shown. A single failure shows the statement, because -/// the alert's own text already names the line and the reason, but copying carries all three so -/// the pasted text stands on its own. internal final class TransferReportView: NSView { private static let width: CGFloat = 380 private static let textHeight: CGFloat = 140 private static let spacing: CGFloat = 8 - /// The most of a report the box shows. A failed statement can be a whole SQL Server batch, and the text view lays - /// out a paragraph whole on the main thread: measured, a 15 million unit batch holding one 5 million unit line - /// blocked it for 67 seconds and took 2.7 GB, where the first 10,000 units took 0.1 seconds. Copy Details still - /// carries every unit. internal static let shownLengthLimit = 10_000 - /// `text` cut to `shownLengthLimit` on a character boundary, with an ellipsis saying there is more. internal static func shownText(_ text: String) -> String { TransferResultAlert.cut(text, toUnits: shownLengthLimit) ?? text } diff --git a/apple/TablePro/Views/Import/ImportDialog.swift b/apple/TablePro/Views/Import/ImportDialog.swift index 215c2de16b..f9627d2e59 100644 --- a/apple/TablePro/Views/Import/ImportDialog.swift +++ b/apple/TablePro/Views/Import/ImportDialog.swift @@ -46,9 +46,6 @@ struct ImportDialog: View { @State private var importResult: PluginImportResult? @State private var importError: (any Error)? - /// The window this dialog is hosted in, used for presenting its alerts and panels. - /// Avoids `NSApp.keyWindow`, which when a result is presented is the progress sheet being - /// torn down in the same transaction, and AppKit ends a sheet's children with it (#2314). @State private var hostWindow: NSWindow? @State private var hasPreviewError = false @@ -121,15 +118,14 @@ struct ImportDialog: View { } .sheet(isPresented: $showProgressDialog) { if let service = importService { - ImportProgressView(service: service) { - service.cancelImport() - } + ImportProgressView(service: service) } } .onChange(of: showSuccessDialog) { isShowing in guard isShowing else { return } TransferResultAlert.presentImportSuccess( result: importResult, + runResult: importService?.stopResult, window: hostWindow, sourceFileName: fileURL?.lastPathComponent ?? "", targetTable: nil @@ -141,7 +137,7 @@ struct ImportDialog: View { } .onChange(of: showErrorDialog) { isShowing in guard isShowing else { return } - TransferResultAlert.presentImportFailure(error: importError, window: hostWindow) { + TransferResultAlert.presentImportFailure(error: importError, result: importService?.stopResult, window: hostWindow) { showErrorDialog = false } } @@ -149,9 +145,6 @@ struct ImportDialog: View { // MARK: - Plugin Helpers - /// This dialog runs a file of statements. A format that needs a target table is routed to - /// `RowImportSheet` instead, so offering one here only ever produced "No target table - /// configured for row import" once the user pressed Import. private var availableFormats: [any ImportFormatPlugin] { let dbTypeId = connection.type.rawValue return pluginManager.allImportPlugins() @@ -352,9 +345,6 @@ struct ImportDialog: View { settingsSnapshot = nil } - /// This dialog runs a file of statements, so Change File… offers the statement formats and no - /// others. It goes through `ImportFilePanel` for the validation `allowedContentTypes` cannot - /// give: a file it dims can still be double-clicked through (#3047). @MainActor private func selectFile() async { guard let window = hostWindow else { @@ -465,7 +455,6 @@ struct ImportDialog: View { } } - /// What the import itself splits with, so the count and the detected encoding match the run. @MainActor private var sessionGrammar: SQLLexicalGrammar { SQLLexicalResolver.executionGrammar(for: connection.type, connectionId: connection.id) @@ -506,9 +495,6 @@ struct ImportDialog: View { importTask = Task { do { - /// The scope the driver is already on, not the connection's saved default: a tab may - /// have moved it, and on an engine that reconnects to change database, pinning - /// somewhere else would refuse the import outright. guard let scope = DatabaseManager.shared.browseScope(for: connection.id) else { throw DatabaseError.notConnected } @@ -532,13 +518,11 @@ struct ImportDialog: View { await MainActor.run { showProgressDialog = false TransferResultAlert.presentImportCancelled( - executedStatements: service.state.processedStatements, - transaction: service.state.transactionOutcome, + result: service.stopResult, window: hostWindow ) {} } } catch let refusal as ExecutionGateError { - /// Nothing ran, so the catalog has nothing to catch up on, and a Cancel needs no reply. await MainActor.run { showProgressDialog = false guard case .denied = refusal else { return } @@ -553,8 +537,6 @@ struct ImportDialog: View { showErrorDialog = true } } - /// A SQL file can create, alter or drop anything, and one that failed or was cancelled - /// part way has already run every statement before that point. CatalogChangeService.post(.changed(CatalogChange(connectionId: connection.id, kinds: .everything))) } } diff --git a/apple/TablePro/Views/Import/ImportProgressView.swift b/apple/TablePro/Views/Import/ImportProgressView.swift index ff05793f7a..02aa447db0 100644 --- a/apple/TablePro/Views/Import/ImportProgressView.swift +++ b/apple/TablePro/Views/Import/ImportProgressView.swift @@ -7,10 +7,53 @@ import SwiftUI struct ImportProgressView: View { @ObservedObject var service: ImportService - let onStop: () -> Void var body: some View { - TransferProgressSheet(progress: service.state.transferProgress, onStop: onStop) + VStack(alignment: .leading, spacing: 16) { + TransferProgressView(progress: service.state.transferProgress) + HStack { + Spacer() + Button(buttonTitle) { service.requestStop() } + .disabled(service.stopPhase != .running) + .keyboardShortcut(.cancelAction) + } + } + .controlSize(.regular) + .padding(20) + .frame(width: TransferProgressSheet.width) + .interactiveDismissDisabled() + .alert(String(localized: "Stop Import?"), isPresented: questionPresented, presenting: service.stopQuestion) { question in + if question.ledger.canChooseCommit { + Button(String(localized: "Stop and Rollback"), role: .destructive) { + service.answerStop(.rollBack, generation: question.generation) + } + Button(String(localized: "Stop and Commit")) { + service.answerStop(.commit, generation: question.generation) + } + } else { + Button(String(localized: "Stop Import"), role: .destructive) { + service.answerStop(.rollBack, generation: question.generation) + } + } + Button(String(localized: "Continue"), role: .cancel) { + service.answerStop(nil, generation: question.generation) + } + } message: { question in + Text(question.ledger.question) + } + .onDisappear { service.stopUnansweredQuestion() } + } + + private var questionPresented: Binding { + Binding(get: { service.stopQuestion != nil }, set: { _ in }) + } + + private var buttonTitle: String { + switch service.stopPhase { + case .running: return String(localized: "Stop") + case .pausing, .paused: return String(localized: "Pausing…") + case .stopping, .finished: return String(localized: "Stopping…") + } } } diff --git a/apple/TablePro/Views/Import/RowImportBatchSheet.swift b/apple/TablePro/Views/Import/RowImportBatchSheet.swift index 01adedbace..1abd1cb007 100644 --- a/apple/TablePro/Views/Import/RowImportBatchSheet.swift +++ b/apple/TablePro/Views/Import/RowImportBatchSheet.swift @@ -9,9 +9,6 @@ import SwiftUI import TableProPluginKit import UniformTypeIdentifiers -/// Several files, or a folder of them, imported as rows into a table each in one run. Every file starts on the table -/// its name matches or a new table named after it, Edit… opens the one-file sheet on it, and the options of each -/// format are set once for every file in that format. struct RowImportBatchSheet: View { private struct EditRequest: Identifiable { let id: RowImportBatch.File.ID @@ -26,14 +23,12 @@ struct RowImportBatchSheet: View { @State private var editing: EditRequest? @State private var isDropTargeted = false - /// The plugins' options are shared and persistent, and this sheet edits them in place, so Cancel puts them back. @State private var settingsSnapshot: PluginSettingsSnapshot? @State private var importSucceeded = false @State private var importService: ImportService? @State private var showProgressDialog = false @State private var importTask: Task? - /// The sheet's own window for its alerts, never `NSApp.keyWindow`, which is the progress sheet while it closes. @State private var hostWindow: NSWindow? init( @@ -115,7 +110,7 @@ struct RowImportBatchSheet: View { } .sheet(isPresented: $showProgressDialog) { if let service = importService { - ImportProgressView(service: service) { service.cancelImport() } + ImportProgressView(service: service) } } } @@ -197,8 +192,6 @@ struct RowImportBatchSheet: View { return String(format: String(localized: "New table %@"), name) } - /// The table a file already points at stays listed when the table list could not be read, or the menu would - /// draw blank. private func tableChoices(for file: RowImportBatch.File) -> [String] { var names = batch.availableTables.map(\.name) if let current = file.target.existingTable, !names.contains(current) { @@ -264,7 +257,6 @@ struct RowImportBatchSheet: View { .frame(maxWidth: .infinity, alignment: .leading) } - /// A dropped file or folder joins the run, read the same way as a folder chosen in the panel. private func accept(_ providers: [NSItemProvider]) -> Bool { let lookup = ImportFormatLookup.live let options = lookup.offeredFormats(connection.type) @@ -429,7 +421,8 @@ struct RowImportBatchSheet: View { description: String( format: String(localized: "Import %@"), ImportService.batchName(fileCount: plan.items.count) - ) + ), + importRun: service.run ) { table, sql in batch.planner.recordCreated(table, createTableSQL: sql) } @@ -452,28 +445,29 @@ struct RowImportBatchSheet: View { present(report, scope: scope) } catch ExecutionGateError.cancelledByUser { showProgressDialog = false + } catch is PluginImportCancellationError { + showProgressDialog = false + TransferResultAlert.presentImportCancelled(result: service.stopResult, window: hostWindow) {} } catch is CancellationError { showProgressDialog = false + TransferResultAlert.presentImportCancelled(result: service.stopResult, window: hostWindow) {} } catch { showProgressDialog = false - TransferResultAlert.presentImportFailure(error: error, window: hostWindow) {} + TransferResultAlert.presentImportFailure(error: error, result: service.stopResult, window: hostWindow) {} } } } - /// A run that finished closes the sheet. One that stopped stays open, so a changed setting can run it again. private func present(_ report: RowImportBatchReport, scope: DatabaseScope) { let completed = report.ending == .completed if completed { importSucceeded = true } TransferResultAlert.presentBatchImportResult(report, window: hostWindow) { - if report.rowsKept { - for table in Set(report.files.map(\.targetTable)) { - AppCommands.shared.objectChanged.send( - DatabaseObjectChange(connectionId: connection.id, scope: scope, name: table, kind: .rows) - ) - } + for table in Set(report.files.map(\.targetTable)) { + AppCommands.shared.objectChanged.send( + DatabaseObjectChange(connectionId: connection.id, scope: scope, name: table, kind: .rows) + ) } if completed { isPresented = false @@ -482,8 +476,6 @@ struct RowImportBatchSheet: View { } } -/// The options a format's own view edits, followed for every format of a run: the signature decides the reads, and -/// the error handling and transaction options decide whether the files can run together. private struct RowImportOptionsObserver: View { @ObservedObject var plugin: Plugin let onChange: (String, RowImportRunner.Configuration?) -> Void diff --git a/apple/TablePro/Views/Import/RowImportSheet.swift b/apple/TablePro/Views/Import/RowImportSheet.swift index 468b8a79c3..e22f0c1b29 100644 --- a/apple/TablePro/Views/Import/RowImportSheet.swift +++ b/apple/TablePro/Views/Import/RowImportSheet.swift @@ -22,17 +22,13 @@ struct RowImportSheet: View { @Binding var isPresented: Bool let connection: DatabaseConnection let fileURL: URL - /// What the sheet, the proposed table name, the error report and history call the source. - /// It differs from `fileURL` when the rows arrive through a snapshot the app wrote. let sourceName: String let formatId: String - /// Set when the sheet edits one file of several: it hands its choices back on Done and imports nothing. let edit: RowImportSheetEdit? private typealias Destination = RowImportDestination - /// The identity of a read. `.task(id:)` cancels the read in flight when it changes. private struct SourceRead: Hashable { let destination: Destination let scope: DatabaseScope? @@ -54,55 +50,29 @@ struct RowImportSheet: View { @State private var writeMode: ImportWriteMode = .insert @State private var chosenMatchKey: ImportMatchKey? - /// The database and schema every read and write of this sheet goes to: where the connection was - /// browsing when the sheet opened. The sheet is modal to its window, but another window on the - /// same connection, or an MCP client, can move the browse database while it is open. Resolving - /// it again at each step listed one database's tables and mapped one table's columns, then - /// created, cleared and filled tables in another. Nil only while the connection has no session, - /// and taken once one exists. @State private var scope: DatabaseScope? - /// Every object the connection holds, not just the tables the destination picker offers. A - /// `CREATE TABLE` collides with a view, a materialized view or a foreign table under the same - /// name as surely as with a table, so the name check has to see all of them. @State private var databaseObjects: [TableInfo] = [] - /// Those names folded for comparison, and `nil` while the catalog is unknown. A read that - /// failed leaves an empty list, and taking that for "nothing is in the way" would propose a - /// name that already exists and then report it as free. Stored rather than computed because the - /// name check runs on every keystroke, and `body` would otherwise rebuild the set each time. @State private var catalogNameKeys: Set? @State private var tableListError: String? - /// `loadTables()` is `@MainActor` but reentrant across its `await`, so two Try Again presses - /// would interleave and a late failure could clear the keys while the picker kept its rows. - /// Concurrent callers wait for the one in flight, per the schema-loading invariant. @State private var isLoadingTables = false @State private var selectedTargetTable: String? @StateObject private var mapping = RowImportMapping() @State private var newTableName: String = "" - /// The last name this sheet proposed, so a second pass can tell its own guess from what - /// the user typed over it. @State private var proposedTableName: String = "" @State private var newTable = NewTableDraft() @State private var isLoadingContext = false - /// Copied from the plugin, whose options are edited in its own view that this sheet does not observe. @State private var detectionSignature = "" @State private var readAttempts: [Destination: Int] = [:] @State private var lastNewColumnsRead: SourceRead? @State private var loadError: String? - /// Moving focus here also selects the whole proposed name, measured rather than assumed: - /// SwiftUI hands the field editor a full selection when `@FocusState` lands on text already in - /// place, both on appear and when the field is revealed by the destination picker. So the first - /// keystroke replaces the proposal instead of appending to it, and no AppKit detour is needed. @FocusState private var newTableNameFocused: Bool - /// The plugin's own options are persistent and shared, and this sheet edits them in place. - /// Without a snapshot, Cancel kept every change, so `Delete existing rows` stayed armed for - /// the next import from anywhere in the app. @State private var settingsSnapshot: PluginSettingsSnapshot? @State private var importSucceeded = false @@ -116,14 +86,8 @@ struct RowImportSheet: View { @State private var showErrorDialog = false @State private var importTask: Task? - /// Knows the tables this sheet created. A failed import leaves its table behind, so a retry has to - /// know it already owns that table rather than trying to create it a second time and failing on - /// the name. @State private var newTablePlanner = NewTableImportPlanner() - /// The window this sheet is hosted in, used for presenting its alerts. - /// Avoids `NSApp.keyWindow`, which when a result is presented is the progress sheet being - /// torn down in the same transaction, and AppKit ends a sheet's children with it (#2314). @State private var hostWindow: NSWindow? init( @@ -159,16 +123,10 @@ struct RowImportSheet: View { // MARK: - Derived catalog state - /// Tables alone, because they are the only objects the existing-table branch can insert into. - /// A partitioned table is one of them: the server routes an INSERT to the right partition. private var availableTables: [TableInfo] { databaseObjects.filter(\.type.acceptsImportedRows) } - /// A table this sheet created is not in the way of this sheet: a failed import leaves its table - /// behind, and `NewTableImportPlanner` exists to reuse that one on the retry, so reporting the - /// name as taken would block the very attempt that mechanism exists to allow. Asked of the - /// planner rather than folded into `catalogNameKeys`, which is rebuilt only when the catalog is. private var newTableNameProblem: NewTableNameProblem? { let trimmed = newTableName.trimmingCharacters(in: .whitespaces) if let edit, !trimmed.isEmpty, edit.takenTableNames.contains(trimmed.lowercased()) { @@ -246,7 +204,7 @@ struct RowImportSheet: View { } .sheet(isPresented: $showProgressDialog) { if let service = importService { - ImportProgressView(service: service) { service.cancelImport() } + ImportProgressView(service: service) } } .onChange(of: showSuccessDialog) { isShowing in @@ -254,6 +212,7 @@ struct RowImportSheet: View { TransferResultAlert.presentImportSuccess( result: importResult, rowTally: importTally, + runResult: importService?.stopResult, window: hostWindow, sourceFileName: sourceName, targetTable: selectedTargetTable @@ -267,7 +226,7 @@ struct RowImportSheet: View { } .onChange(of: showErrorDialog) { isShowing in guard isShowing else { return } - TransferResultAlert.presentImportFailure(error: importError, window: hostWindow) { + TransferResultAlert.presentImportFailure(error: importError, result: importService?.stopResult, window: hostWindow) { showErrorDialog = false } } @@ -373,8 +332,6 @@ struct RowImportSheet: View { .frame(maxWidth: .infinity, alignment: .leading) } - /// The list is what both destinations are chosen against, so a failure to read it is worth - /// saying and worth being able to retry without losing the options already set in this sheet. private func tableListErrorRow(_ message: String) -> some View { HStack(spacing: 6) { Label( @@ -450,14 +407,10 @@ struct RowImportSheet: View { } } - /// A read takes seconds on a large file, and until it lands the list is empty, which the other - /// placeholders would report as a file with nothing in it. private var readingPlaceholder: String { String(localized: "Reading the file…") } - /// A file the plugin could not read is a failure, not an empty result. Showing the parser's - /// message as grey placeholder text left the sheet with nothing to press but Cancel. private func unreadableFile(reason: String) -> some View { UnavailableStateView { Label(String(localized: "Cannot read this file"), systemImage: "exclamationmark.triangle") @@ -470,7 +423,6 @@ struct RowImportSheet: View { } } - /// Per destination, so Try Again for one never reads the other again over its edits. @MainActor private func retryLoad() async { loadError = nil @@ -730,9 +682,6 @@ struct RowImportSheet: View { .sorted() } - /// The selection has to be one of the options by exact spelling or the menu draws blank, and - /// `typeOptions` suppresses its insert on a case-insensitive match. The getter resolves through - /// the same comparison so a differently-cased stored type still selects its own row. private func typeBinding(_ row: NewTableColumn) -> Binding { let type = row.settings.type let options = typeOptions(including: type) @@ -768,14 +717,11 @@ struct RowImportSheet: View { } } - /// Import runs only what the rows show: a read of the file with the current options, into the current - /// table and database, that finished without an error. private var currentReadIsReady: Bool { guard !isLoadingContext, loadError == nil else { return false } return currentReadHasLanded } - /// The rows on screen answer the read for the current destination, table, database and options. private var currentReadHasLanded: Bool { switch destination { case .existingTable: @@ -787,14 +733,6 @@ struct RowImportSheet: View { // MARK: - Loading - /// Both failures used to leave an empty list and say nothing, so the destination picker offered - /// "Select a table…" and nothing else with no way to tell an empty database from an unreachable - /// one, and no way to ask again. - /// - /// Read through the sheet's own scope, the browse scope it took on opening, which is what the - /// import writes to. The shared session driver is wherever a tab's execution last pinned it and - /// nothing puts it back, so reading from it listed one database's tables and mapped their columns - /// while the rows went to another's. @MainActor private func loadTables() async { guard !isLoadingTables else { return } @@ -822,14 +760,6 @@ struct RowImportSheet: View { } } - /// Proposes a name straight away and again once the catalog arrives, because only the part that - /// avoids a name already in use needs the catalog. Waiting for it would leave the field empty - /// under a "name this table" warning while the list loaded, and empty for good if it failed. - /// - /// The second pass replaces this sheet's own earlier guess and nothing else: anything the user - /// typed differs from `proposedTableName` and is left alone. A table this sheet already created - /// is left out of the avoid-set for the same reason it is left out of the name check: a retry - /// is meant to land back on it, and stepping the name to `_2` would strand the first one. @MainActor private func suggestNewTableName() { guard newTableName.isEmpty || newTableName == proposedTableName else { return } @@ -854,8 +784,6 @@ struct RowImportSheet: View { ) } - /// A read that succeeded is not repeated when switching back to its destination, and it clears an - /// error the other destination left on screen. @MainActor private func read(_ request: SourceRead) async { switch request.destination { @@ -995,8 +923,6 @@ struct RowImportSheet: View { } } - /// Hands the choices back only when they answer a read of the current table and options, the same readiness - /// Import asks for. private func finishEdit(_ edit: RowImportSheetEdit) { guard currentReadIsReady else { return } var target = RowImportTarget( @@ -1032,7 +958,8 @@ struct RowImportSheet: View { authorization = try await RowImportTableMaker(connection: connection).prepare( [newTable], scope: scope, - planner: newTablePlanner + planner: newTablePlanner, + importRun: service.run ) { table, sql in newTablePlanner.recordCreated(table, createTableSQL: sql) } @@ -1072,8 +999,7 @@ struct RowImportSheet: View { await MainActor.run { showProgressDialog = false TransferResultAlert.presentImportCancelled( - executedStatements: service.state.processedStatements, - transaction: service.state.transactionOutcome, + result: service.stopResult, window: hostWindow ) {} } diff --git a/apple/TableProTests/Core/Services/ImportRunTests.swift b/apple/TableProTests/Core/Services/ImportRunTests.swift new file mode 100644 index 0000000000..d43c3a4a25 --- /dev/null +++ b/apple/TableProTests/Core/Services/ImportRunTests.swift @@ -0,0 +1,211 @@ +import Foundation +@testable import TablePro +import TableProPluginKit +import TableProTestSupport +import Testing + +@Suite(.timeLimit(.minutes(1))) +struct ImportRunTests { + @Test("Pause waits for the last admitted write") + func drainsLastWrite() async throws { + let run = ImportRun() + #expect(try await run.admit(.data) == .proceed) + run.requestPause() + #expect(run.currentPhase == .pausing) + #expect(run.questionIfPaused() == nil) + run.release() + let question = try #require(run.questionIfPaused()) + #expect(run.currentPhase == .paused(generation: question.generation)) + run.stop(.rollBack, generation: question.generation) + await #expect(throws: PluginImportCancellationError.self) { try await run.admit(.commit) } + #expect(try await run.admit(.rollback) == .proceed) + run.release() + } + + @Test("Continue admits a parked statement exactly once") + func continueDoesNotSkip() async throws { + let run = ImportRun() + run.requestPause() + let generation = try #require(run.questionIfPaused()).generation + let arrived = AsyncGate() + let task = Task { + await arrived.open() + let admission = try await run.admit(.data) + run.record { $0.effects[0] = ImportFileEffect(index: 0, table: "t", submittedRows: 1) } + run.release() + return admission + } + await arrived.wait() + #expect(run.snapshot.effects.isEmpty) + run.resume(generation: generation) + #expect(try await task.value == .proceed) + #expect(run.snapshot.effects[0]?.submittedRows == 1) + } + + @Test("An unanswered question stops rather than reopening the commit path") + func disappearanceStops() async throws { + let run = ImportRun() + run.requestPause() + let generation = try #require(run.questionIfPaused()).generation + run.stopUnansweredPause() + #expect(!run.resume(generation: generation)) + await #expect(throws: PluginImportCancellationError.self) { try await run.admit(.commit) } + #expect(run.stopChoice == .rollBack) + } + + @Test("Answers from a previous question cannot stop a later pause") + func staleAnswer() throws { + let run = ImportRun() + run.requestPause() + let first = try #require(run.questionIfPaused()).generation + run.resume(generation: first) + run.requestPause() + let second = try #require(run.questionIfPaused()).generation + #expect(!run.stop(.commit, generation: first)) + #expect(!run.resume(generation: first)) + #expect(run.currentPhase == .paused(generation: second)) + } + + @Test("Cancellation before admission cannot commit or strand a waiter") + func cancelledBeforeAdmission() async throws { + let run = ImportRun() + run.requestPause() + let start = AsyncGate() + let task = Task { + await start.wait() + return try await run.admit(.commit) + } + await start.arrived(1) + task.cancel() + await start.open() + await #expect(throws: PluginImportCancellationError.self) { try await task.value } + #expect(run.stopChoice == .rollBack) + } + + @Test("Continue before waiter registration does not lose the wakeup") + func resumeBeforeAdmission() async throws { + let run = ImportRun() + run.requestPause() + let generation = try #require(run.questionIfPaused()).generation + let start = AsyncGate() + let task = Task { + await start.wait() + let value = try await run.admit(.data) + run.release() + return value + } + await start.arrived(1) + run.resume(generation: generation) + await start.open() + #expect(try await task.value == .proceed) + } + + @Test("Commit choice skips plugin rollback and still refuses the next write") + func commitChoice() async throws { + let run = ImportRun() + run.requestPause() + run.stop(.commit, generation: try #require(run.questionIfPaused()).generation) + #expect(try await run.admit(.rollback) == .skip) + #expect(try await run.admit(.commit) == .proceed) + run.release() + await #expect(throws: PluginImportCancellationError.self) { try await run.admit(.data) } + } + + @Test("Finished runs refuse late writes") + func finishRefusesWrites() async { + let run = ImportRun() + run.finish() + await #expect(throws: PluginImportCancellationError.self) { try await run.admit(.data) } + #expect(!run.requestPause()) + } +} + +struct ImportStopResultTests { + private func ledger() -> ImportLedger { + var value = ImportLedger() + value.prepared = true + value.initialState = .idle + value.currentState = .open + value.holder = .owned + value.effects[0] = ImportFileEffect(index: 0, table: "t", rollback: .transactional, + submittedRows: 1, admittedWrite: true) + return value + } + + @Test("Known idle after owner rollback is not an unexplained transaction end") + func knownRollback() { + var value = ledger() + value.ending = .rolledBack + value.currentState = .idle + #expect(ImportStopResult(ledger: value).fate(for: 0) == .rolledBack) + } + + @Test("A mixed batch reports each table's rollback behavior") + func mixedTables() { + var value = ledger() + value.effects[1] = ImportFileEffect(index: 1, table: "other", rollback: .nontransactional, + submittedRows: 2, admittedWrite: true) + #expect(value.question.contains("other")) + value.ending = .rolledBack + value.currentState = .idle + let result = ImportStopResult(ledger: value) + #expect(result.fate(for: 0) == .rolledBack) + #expect(result.fate(for: 1) == .kept) + } + + @Test("Duplicate target tables keep separate file counts") + func duplicateTargets() { + var value = ledger() + value.effects[1] = ImportFileEffect(index: 1, table: "t", rollback: .transactional, + submittedRows: 2, admittedWrite: true) + let result = ImportStopResult(ledger: value) + #expect(result.fileText(for: 0).contains("1 row submitted")) + #expect(result.fileText(for: 1).contains("2 rows submitted")) + } + + @Test("Unseen batches and transaction history override a final COMMIT") + func unseenHistory() { + var value = ledger() + value.unseenStatements = true + value.ending = .committed + value.currentState = .idle + #expect(!value.canChooseCommit) + #expect(value.question.contains("may stay")) + #expect(ImportStopResult(ledger: value).fate(for: 0) == .mayStay) + } + + @Test("Failed writes into a nontransactional table keep an uncertain result") + func partialFailure() { + var value = ledger() + value.effects[0]?.rollback = .nontransactional + value.effects[0]?.failedWrite = true + #expect(ImportStopResult(ledger: value).fate(for: 0) == .mayStay) + } + + @Test("A joined user transaction is never described as committed") + func joinedTransaction() { + var value = ledger() + value.holder = .joinedUser + #expect(!value.canChooseCommit) + #expect(ImportStopResult(ledger: value).fate(for: 0) == .openTransaction) + value.currentState = .aborted + #expect(ImportStopResult(ledger: value).fate(for: 0) == .abortedTransaction) + } + + @Test("A failed COMMIT that leaves a transaction open says so") + func incompleteCommit() { + var value = ledger() + value.ending = .unknown + #expect(ImportStopResult(ledger: value).fate(for: 0) == .mayStay) + #expect(ImportStopResult(ledger: value).text.contains("still open")) + } + + @Test("DELETE is visible even with no submitted rows") + func emptyingIsAnEffect() { + var value = ledger() + value.effects[0]?.submittedRows = 0 + value.effects[0]?.deletedRows = true + #expect(value.hasWrites) + #expect(ImportStopResult(ledger: value).fileText(for: 0).contains("emptied")) + } +} diff --git a/apple/TableProTests/Core/Services/ImportServiceStopTests.swift b/apple/TableProTests/Core/Services/ImportServiceStopTests.swift new file mode 100644 index 0000000000..724074b102 --- /dev/null +++ b/apple/TableProTests/Core/Services/ImportServiceStopTests.swift @@ -0,0 +1,737 @@ +import Foundation +@testable import TablePro +import TableProPluginKit +import TableProTestSupport +import Testing + +private final class StopDriver: PluginDriverStub, @unchecked Sendable { + let hold = AsyncGate() + let closing = AsyncGate() + var holdClosingScope = false + var rollbackFails = false + var closeFails = false + var heldText: String? + var failingText: String? + var implicitTransactions = false + var commitLeavesOpen = false + var commitFails = false + var supportsTransactions = true + var engines: [String: String] = [:] + private let lock = NSLock() + private var state = PluginSessionTransactionState.idle + private var queries: [String] = [] + + var events: [String] { lock.withLock { queries } } + + func setState(_ value: PluginSessionTransactionState) { lock.withLock { state = value } } + + func execute(query: String) async throws -> PluginQueryResult { + lock.withLock { queries.append(query) } + if let heldText, query.contains(heldText) { await hold.wait() } + if let failingText, query.contains(failingText) { + throw DatabaseError.queryFailed("injected write failure") + } + if query.hasPrefix("BEGIN") { setState(.inTransaction) } + if query.hasPrefix("COMMIT") { + if commitFails { throw DatabaseError.queryFailed("injected commit failure") } + if !commitLeavesOpen { setState(.idle) } + } + if query.hasPrefix("ROLLBACK") { + if rollbackFails { throw DatabaseError.queryFailed("injected rollback failure") } + setState(.idle) + } + if implicitTransactions, query.hasPrefix("INSERT") { setState(.inTransaction) } + return .empty + } + + func openSessionScope(for purpose: PluginSessionPurpose) async throws -> PluginSessionScope? { + PluginSessionScope(purpose: purpose, saved: [], applied: []) + } + + func closeSessionScope(_ scope: PluginSessionScope) async throws { + if holdClosingScope { await closing.wait() } + if closeFails { throw DatabaseError.queryFailed("injected cleanup failure") } + } + + func generateCreateTableSQL(definition: PluginCreateTableDefinition) -> String? { + "CREATE TABLE \(definition.tableName) (id TEXT)" + } + + func sessionTransactionState() async -> PluginSessionTransactionState { lock.withLock { state } } + func switchDatabase(to database: String) async throws {} + + func fetchTableMetadata(table: String, schema: String?) async throws -> PluginTableMetadata { + PluginTableMetadata(tableName: table, engine: engines[table] ?? "InnoDB") + } +} + +private final class StopRowsPlugin: ImportFormatPlugin { + static let pluginName = "Stop Rows" + static let pluginVersion = "1" + static let pluginDescription = "Rows for stop tests" + static let formatId = "stop-rows-test" + static let formatDisplayName = "Rows" + static let acceptedFileExtensions = ["rows"] + static let iconName = "doc" + static let requiresTargetTable = true + private let configuration: RowImportRunner.Configuration + + required init() { + configuration = RowImportRunner.Configuration( + errorHandling: .stopAndRollback, wrapInTransaction: true, deleteExistingRows: false + ) + } + + init(_ configuration: RowImportRunner.Configuration) { + self.configuration = configuration + } + + func performImport(source: any PluginImportSource, sink: any PluginImportDataSink, + progress: PluginImportProgress) async throws -> PluginImportResult { + let lines = try String(contentsOf: source.fileURL(), encoding: .utf8).split(separator: "\n").map(String.init) + var delivered = false + let result = try await RowImportRunner.run(configuration: configuration, sink: sink, progress: progress) { + guard !delivered else { return nil } + delivered = true + return lines.enumerated().map { (line: $0.offset + 1, row: ["id": .text($0.element)]) } + } + return PluginImportResult(executedStatements: result.inserted, executionTime: 0, + skippedStatements: result.skipped, errors: result.errors) + } +} + +@MainActor +private final class StopFixture { + let driver: StopDriver + let adapter: PluginDriverAdapter + let connection: DatabaseConnection + let service: ImportService + let plugin: StopRowsPlugin + let format = UUID().uuidString + let folder = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + + init(driver: StopDriver = StopDriver(), type: DatabaseType = .mysql, + errorHandling: ImportErrorHandling = .stopAndRollback, wrapInTransaction: Bool = true) throws { + plugin = StopRowsPlugin(.init( + errorHandling: errorHandling, wrapInTransaction: wrapInTransaction, deleteExistingRows: false + )) + self.driver = driver + connection = TestFixtures.makeConnection(database: "shop", type: type) + adapter = PluginDriverAdapter(connection: connection, pluginDriver: driver) + var session = ConnectionSession(connection: connection, driver: adapter) + session.browseDatabase = "shop" + session.status = .connected + DatabaseManager.shared.injectSession(session, for: connection.id) + service = ImportService(connection: connection, gate: AlwaysAllowGate()) + PluginManager.shared.importPlugins[format] = plugin + try FileManager.default.createDirectory(at: folder, withIntermediateDirectories: true) + } + + var scope: DatabaseScope { DatabaseScope(connectionId: connection.id, database: "shop", schema: nil) } + + func files(_ contents: [String], tables: [String]? = nil) throws -> [RowImportRunItem] { + try contents.enumerated().map { index, text in + let url = folder.appendingPathComponent("file\(index).rows") + try Data(text.utf8).write(to: url) + return RowImportRunItem(url: url, sourceName: url.lastPathComponent, formatId: format, + targetTable: tables?[index] ?? "table\(index)", + columnMapping: ["id": "id"], sourceFields: ["id"]) + } + } + + func cleanup() { + DatabaseManager.shared.removeSession(for: connection.id) + PluginManager.shared.importPlugins[format] = nil + try? FileManager.default.removeItem(at: folder) + } + + func pauseHeldWrite() async throws -> Int { + await driver.hold.arrived(1) + service.requestStop() + #expect(service.run.currentPhase == .pausing) + await driver.hold.open() + try await waitUntil { self.service.run.questionIfPaused() != nil } + return try #require(service.run.questionIfPaused()).generation + } +} + +@MainActor +@Suite(.timeLimit(.minutes(1))) +struct ImportServiceStopTests { + private let rollback = RowImportBatchPolicy(errorHandling: .stopAndRollback, wrapInTransaction: true) + + @Test("Stop during the last write rolls back instead of committing") + func lastWrite() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + fixture.driver.heldText = "last" + let file = try #require(fixture.files(["last"]).first) + let task = Task { + try await fixture.service.importFile(from: file.url, formatId: fixture.format, encoding: .utf8, + scope: fixture.scope, targetTable: file.targetTable, + columnMapping: file.columnMapping, sourceFields: file.sourceFields) + } + let generation = try await fixture.pauseHeldWrite() + #expect(!fixture.driver.events.contains("COMMIT")) + fixture.service.answerStop(.rollBack, generation: generation) + await #expect(throws: PluginImportCancellationError.self) { try await task.value } + #expect(!fixture.driver.events.contains("COMMIT")) + #expect(fixture.driver.events.contains("ROLLBACK")) + #expect(fixture.service.stopResult.fate(for: 0) == .rolledBack) + } + + @Test("A confirmation keeps the final commit parked until Continue") + func continueFinalWrite() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + fixture.driver.heldText = "last" + let files = try fixture.files(["first", "last"]) + let task = Task { try await fixture.service.importFiles(files, scope: fixture.scope, policy: rollback) } + let generation = try await fixture.pauseHeldWrite() + #expect(fixture.driver.events.filter { $0 == "COMMIT" }.isEmpty) + #expect(fixture.service.run.snapshot.effects[1]?.submittedRows == 1) + fixture.service.answerStop(nil, generation: generation) + let report = try await task.value + #expect(report.ending == .completed) + #expect(fixture.driver.events.filter { $0 == "BEGIN" }.count == 1) + #expect(fixture.driver.events.filter { $0 == "COMMIT" }.count == 1) + #expect(fixture.driver.events.filter { $0.hasPrefix("INSERT") }.count == 2) + } + + @Test("A failed write under Stop and Commit does not commit earlier files after rollback Stop") + func failedWriteBeforeStop() async throws { + let fixture = try StopFixture(errorHandling: .stopAndCommit) + defer { fixture.cleanup() } + fixture.driver.heldText = "bad" + fixture.driver.failingText = "bad" + let files = try fixture.files(["first", "bad"]) + let policy = RowImportBatchPolicy(errorHandling: .stopAndCommit, wrapInTransaction: true) + let task = Task { try await fixture.service.importFiles(files, scope: fixture.scope, policy: policy) } + let generation = try await fixture.pauseHeldWrite() + fixture.service.answerStop(.rollBack, generation: generation) + let report = try await task.value + #expect(report.ending == .stopped(file: 1)) + #expect(!fixture.driver.events.contains("COMMIT")) + #expect(fixture.driver.events.contains("ROLLBACK")) + #expect(report.stopResult?.ledger.errors.contains("injected write failure") == true) + #expect(report.stopResult?.fate(for: 0) == .rolledBack) + } + + @Test("Mixed InnoDB and MyISAM keep distinct fates") + func mixedBatch() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + fixture.driver.engines = ["table0": "InnoDB", "table1": "MyISAM"] + fixture.driver.heldText = "last" + let files = try fixture.files(["first", "last"]) + let task = Task { try await fixture.service.importFiles(files, scope: fixture.scope, policy: rollback) } + let generation = try await fixture.pauseHeldWrite() + #expect(fixture.service.run.snapshot.question.contains("table1")) + fixture.service.answerStop(.rollBack, generation: generation) + let report = try await task.value + #expect(report.stopResult?.fate(for: 0) == .rolledBack) + #expect(report.stopResult?.fate(for: 1) == .kept) + } + + @Test("Stop and Commit commits the owner once") + func stopAndCommit() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + fixture.driver.heldText = "last" + let files = try fixture.files(["first", "last"], tables: ["t", "t"]) + let task = Task { try await fixture.service.importFiles(files, scope: fixture.scope, policy: rollback) } + let generation = try await fixture.pauseHeldWrite() + #expect(fixture.service.run.snapshot.canChooseCommit) + fixture.service.answerStop(.commit, generation: generation) + let report = try await task.value + #expect(fixture.driver.events.filter { $0 == "COMMIT" }.count == 1) + #expect(!fixture.driver.events.contains("ROLLBACK")) + #expect(report.stopResult?.fate(for: 0) == .committed) + #expect(report.stopResult?.fate(for: 1) == .committed) + #expect(report.stopResult?.ledger.effects[0]?.submittedRows == 1) + #expect(report.stopResult?.ledger.effects[1]?.submittedRows == 1) + } + + @Test("Existing user transactions are left open with wrapping on or off", arguments: [true, false]) + func existingTransaction(wrapped: Bool) async throws { + let fixture = try StopFixture(wrapInTransaction: wrapped) + defer { fixture.cleanup() } + fixture.driver.setState(.inTransaction) + fixture.driver.heldText = "last" + let files = try fixture.files(["last"]) + let policy = RowImportBatchPolicy(errorHandling: .stopAndRollback, wrapInTransaction: wrapped) + let task = Task { try await fixture.service.importFiles(files, scope: fixture.scope, policy: policy) } + let generation = try await fixture.pauseHeldWrite() + fixture.service.answerStop(.rollBack, generation: generation) + let report = try await task.value + #expect(!fixture.driver.events.contains("BEGIN")) + #expect(!fixture.driver.events.contains("COMMIT")) + #expect(!fixture.driver.events.contains("ROLLBACK")) + #expect(report.stopResult?.fate(for: 0) == .openTransaction) + } + + @Test("Unwrapped implicit transactions do not become kept rows") + func implicitTransaction() async throws { + let fixture = try StopFixture(wrapInTransaction: false) + defer { fixture.cleanup() } + fixture.driver.implicitTransactions = true + fixture.driver.heldText = "last" + let files = try fixture.files(["last"]) + let policy = RowImportBatchPolicy(errorHandling: .stopAndRollback, wrapInTransaction: false) + let task = Task { try await fixture.service.importFiles(files, scope: fixture.scope, policy: policy) } + let generation = try await fixture.pauseHeldWrite() + #expect(fixture.service.run.snapshot.question.contains("may stay")) + fixture.service.answerStop(.rollBack, generation: generation) + let report = try await task.value + #expect(report.stopResult?.fate(for: 0) == .mayStay) + #expect(report.stopResult?.text.contains("still open") == true) + #expect(!fixture.driver.events.contains("ROLLBACK")) + } + + @Test("An engine without transactions reports kept changes") + func noTransactions() async throws { + let driver = StopDriver() + driver.supportsTransactions = false + let fixture = try StopFixture(driver: driver) + defer { fixture.cleanup() } + driver.heldText = "last" + let files = try fixture.files(["last"]) + let task = Task { try await fixture.service.importFiles(files, scope: fixture.scope, policy: rollback) } + let generation = try await fixture.pauseHeldWrite() + fixture.service.answerStop(.rollBack, generation: generation) + let report = try await task.value + #expect(report.stopResult?.fate(for: 0) == .kept) + #expect(!driver.events.contains("BEGIN")) + } + + @Test("A commit that leaves the transaction open reports uncertainty") + func nestedCommit() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + fixture.driver.heldText = "last" + fixture.driver.commitLeavesOpen = true + let files = try fixture.files(["last"]) + let task = Task { try await fixture.service.importFiles(files, scope: fixture.scope, policy: rollback) } + let generation = try await fixture.pauseHeldWrite() + fixture.service.answerStop(.commit, generation: generation) + let report = try await task.value + #expect(report.stopResult?.fate(for: 0) == .mayStay) + #expect(report.stopResult?.text.contains("still open") == true) + } + + @Test("Failed commit retains its error and rolls back the owned transaction") + func failedCommit() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + fixture.driver.heldText = "last" + fixture.driver.commitFails = true + let files = try fixture.files(["last"]) + let task = Task { try await fixture.service.importFiles(files, scope: fixture.scope, policy: rollback) } + let generation = try await fixture.pauseHeldWrite() + fixture.service.answerStop(.commit, generation: generation) + let report = try await task.value + #expect(report.stopResult?.fate(for: 0) == .rolledBack) + #expect(report.stopResult?.text.contains("injected commit failure") == true) + } +} + +extension ImportServiceStopTests { + @Test("A file COMMIT queued after pause never runs under a rollback answer") + func fileCommitParked() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + let run = ImportRun() + let sink = ImportDataSinkAdapter(driver: fixture.adapter, databaseType: .mysql, importRun: run) + try await sink.prepareImportRun() + try await sink.beginTransaction() + try await sink.execute(statement: "INSERT INTO t VALUES (1)") + run.requestPause() + let question = try #require(run.questionIfPaused()) + let pending = Task { try await sink.execute(statement: "COMMIT") } + run.stop(.rollBack, generation: question.generation) + await #expect(throws: PluginImportCancellationError.self) { try await pending.value } + try await sink.rollbackTransaction() + #expect(!fixture.driver.events.contains("COMMIT")) + #expect(ImportStopResult(ledger: run.snapshot).fate(for: 0) == .rolledBack) + } + + @Test("A file COMMIT already in flight makes the paused question conservative") + func fileCommitInFlight() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + fixture.driver.heldText = "COMMIT" + let run = ImportRun() + let sink = ImportDataSinkAdapter(driver: fixture.adapter, databaseType: .mysql, importRun: run) + try await sink.prepareImportRun() + try await sink.beginTransaction() + try await sink.execute(statement: "INSERT INTO t VALUES (1)") + let pending = Task { try await sink.execute(statement: "COMMIT") } + await fixture.driver.hold.arrived(1) + run.requestPause() + await fixture.driver.hold.open() + try await pending.value + let question = try #require(run.questionIfPaused()) + #expect(question.ledger.question.contains("may stay")) + #expect(!question.ledger.canChooseCommit) + run.stop(.rollBack, generation: question.generation) + try await sink.rollbackTransaction() + #expect(ImportStopResult(ledger: run.snapshot).fate(for: 0) == .mayStay) + } + + @Test("A statement paused before admission runs once after Continue") + func pausedStatementRunsOnce() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + let run = ImportRun() + let sink = ImportDataSinkAdapter(driver: fixture.adapter, databaseType: .mysql, importRun: run) + try await sink.prepareImportRun() + run.requestPause() + let question = try #require(run.questionIfPaused()) + let pending = Task { try await sink.execute(statement: "INSERT INTO t VALUES (1)") } + #expect(fixture.driver.events.isEmpty) + run.resume(generation: question.generation) + try await pending.value + #expect(fixture.driver.events == ["INSERT INTO t VALUES (1)"]) + #expect(run.snapshot.effects[0]?.statements == 1) + } + + @Test("SQL Server GO execution never promises a complete commit") + func sqlServerIsConservative() async throws { + let fixture = try StopFixture(type: .mssql) + defer { fixture.cleanup() } + let run = ImportRun() + let sink = ImportDataSinkAdapter(driver: fixture.adapter, databaseType: .mssql, importRun: run) + try await sink.prepareImportRun() + try await sink.beginTransaction() + try await sink.execute(statement: "INSERT INTO t VALUES (1)") + run.requestPause() + let question = try #require(run.questionIfPaused()) + #expect(question.ledger.unseenStatements) + #expect(!question.ledger.canChooseCommit) + #expect(question.ledger.question.contains("may stay")) + run.stop(.rollBack, generation: question.generation) + try await sink.rollbackTransaction() + #expect(ImportStopResult(ledger: run.snapshot).fate(for: 0) == .mayStay) + } + + @Test("An aborted owned transaction rolls back even under commit intent") + func abortedCommit() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + let run = ImportRun() + let sink = ImportDataSinkAdapter(driver: fixture.adapter, databaseType: .mysql, importRun: run) + try await sink.prepareImportRun() + try await sink.beginTransaction() + try await sink.execute(statement: "INSERT INTO t VALUES (1)") + fixture.driver.setState(.abortedTransaction) + run.stop(.commit) + try await sink.commitTransaction() + #expect(fixture.driver.events.last == "ROLLBACK") + #expect(!fixture.driver.events.contains("COMMIT")) + #expect(ImportStopResult(ledger: run.snapshot).fate(for: 0) == .rolledBack) + } + + @Test("Task cancellation outside a parked call refuses normal COMMIT") + func taskCancellationDuringWrite() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + fixture.driver.heldText = "last" + let files = try fixture.files(["last"]) + let task = Task { try await fixture.service.importFiles(files, scope: fixture.scope, policy: rollback) } + await fixture.driver.hold.arrived(1) + task.cancel() + await fixture.driver.hold.open() + let report = try await task.value + #expect(report.ending == .stopped(file: 0)) + #expect(!fixture.driver.events.contains("COMMIT")) + #expect(fixture.driver.events.contains("ROLLBACK")) + } +} + +extension ImportServiceStopTests { + @Test("A stopped partial handoff retains the completed sub-batch count") + func partialHandoff() async throws { + let fixture = try StopFixture(type: .oracle) + defer { fixture.cleanup() } + let run = ImportRun() + let sink = ImportDataSinkAdapter(driver: fixture.adapter, databaseType: .oracle, + targetTable: "t", importRun: run) + try await sink.prepareImportRun() + try await sink.beginTransaction() + let task = Task { + try await sink.insertRows(columns: ["id"], values: [[.text("first")], [.text("second")]]) { _ in + run.requestPause() + } + } + try await waitUntil { run.questionIfPaused() != nil } + let question = try #require(run.questionIfPaused()) + #expect(question.ledger.effects[0]?.submittedRows == 1) + run.stop(.rollBack, generation: question.generation) + await #expect(throws: PluginImportCancellationError.self) { try await task.value } + try await sink.rollbackTransaction() + #expect(fixture.driver.events.filter { $0.hasPrefix("INSERT") }.count == 1) + #expect(ImportStopResult(ledger: run.snapshot).fate(for: 0) == .rolledBack) + } + + @Test("A successful ignored insert reports submitted rows, never affected rows") + func ignoredInsertCount() async throws { + let fixture = try StopFixture(type: .sqlite) + defer { fixture.cleanup() } + let run = ImportRun() + let sink = ImportDataSinkAdapter(driver: fixture.adapter, databaseType: .sqlite, + targetTable: "t", columnMapping: ["id": "id"], importRun: run) + try await sink.prepareImportRun() + try await sink.insertRow(["id": .text("already-present")]) + let result = ImportStopResult(ledger: run.snapshot) + #expect(result.ledger.effects[0]?.submittedRows == 1) + #expect(result.fileText(for: 0).contains("row submitted in a completed write")) + } +} + + +extension ImportServiceStopTests { + @Test("A retry DELETE cannot run while paused or after Stop") + func preparationDeleteIsGated() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + let run = fixture.service.run + var planner = NewTableImportPlanner() + planner.recordCreated(TableScope(table: "t", in: fixture.scope), createTableSQL: "CREATE TABLE t (id TEXT)") + let definition = PluginCreateTableDefinition(tableName: "t", columns: [], primaryKeyColumns: []) + let maker = RowImportTableMaker(connection: fixture.connection, gate: AlwaysAllowGate()) + run.requestPause() + let task = Task { + try await maker.prepare([definition], scope: fixture.scope, planner: planner, importRun: run) { _, _ in } + } + try await waitUntil { run.waitingCallCount > 0 } + #expect(!fixture.driver.events.contains { $0.hasPrefix("DELETE") }) + fixture.service.answerStop(.rollBack, generation: try #require(run.questionIfPaused()).generation) + await #expect(throws: PluginImportCancellationError.self) { try await task.value } + #expect(!fixture.driver.events.contains { $0.hasPrefix("DELETE") }) + #expect(run.snapshot.preparations.isEmpty) + } + + @Test("A completed retry DELETE is recorded outside the later row transaction") + func preparationDeleteIsReported() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + fixture.driver.heldText = "DELETE" + let run = fixture.service.run + var planner = NewTableImportPlanner() + planner.recordCreated(TableScope(table: "t", in: fixture.scope), createTableSQL: "CREATE TABLE t (id TEXT)") + let definition = PluginCreateTableDefinition(tableName: "t", columns: [], primaryKeyColumns: []) + let maker = RowImportTableMaker(connection: fixture.connection, gate: AlwaysAllowGate()) + let task = Task { + try await maker.prepare([definition], scope: fixture.scope, planner: planner, importRun: run) { _, _ in } + } + await fixture.driver.hold.arrived(1) + fixture.service.requestStop() + #expect(run.questionIfPaused() == nil) + await fixture.driver.hold.open() + _ = try await task.value + let question = try #require(run.questionIfPaused()) + #expect(question.ledger.question.contains("table cleared")) + #expect(question.ledger.question.contains("changes kept")) + fixture.service.answerStop(.rollBack, generation: question.generation) + let files = try fixture.files(["new"], tables: ["t"]) + let report = try await fixture.service.importFiles(files, scope: fixture.scope, policy: rollback) + #expect(report.stopResult?.text.contains("table cleared") == true) + #expect(report.stopResult?.text.contains("changes kept") == true) + #expect(!fixture.driver.events.contains { $0.hasPrefix("INSERT") }) + } + + @Test("Stop during final cleanup waits for an answer before finishing") + func finalCleanupPause() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + fixture.driver.holdClosingScope = true + let files = try fixture.files(["last"]) + let task = Task { try await fixture.service.importFiles(files, scope: fixture.scope, policy: rollback) } + await fixture.driver.closing.arrived(1) + fixture.service.requestStop() + #expect(fixture.service.run.currentPhase == .pausing) + await fixture.driver.closing.open() + try await waitUntil { fixture.service.run.waitingCallCount > 0 } + let question = try #require(fixture.service.run.questionIfPaused()) + #expect(question.ledger.question.contains("already committed")) + fixture.service.answerStop(.rollBack, generation: question.generation) + let report = try await task.value + #expect(report.ending == .stopped(file: 0)) + #expect(report.stopResult?.fate(for: 0) == .committed) + } + + @Test("Commit intent replaces a policy rollback that was parked") + func commitReplacesParkedRollback() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + let run = fixture.service.run + let owner = ImportDataSinkAdapter(driver: fixture.adapter, databaseType: .mysql, + targetTable: "t", columnMapping: ["id": "id"], importRun: run) + try await owner.prepareImportRun() + try await owner.beginTransaction() + try await owner.insertRow(["id": .text("first")]) + run.requestPause() + let task = Task { await fixture.service.finishOwner(owner, commit: false) } + try await waitUntil { run.waitingCallCount > 0 } + fixture.service.answerStop(.commit, generation: try #require(run.questionIfPaused()).generation) + #expect(await task.value == nil) + #expect(fixture.driver.events.contains("COMMIT")) + #expect(!fixture.driver.events.contains("ROLLBACK")) + #expect(ImportStopResult(ledger: run.snapshot).fate(for: 0) == .committed) + } + + @Test("A transaction the file opens after COMMIT is never finalized by the import", arguments: [false, true]) + func replacementTransactionIsUnowned(stopping: Bool) async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + let run = ImportRun() + let sink = ImportDataSinkAdapter(driver: fixture.adapter, databaseType: .mysql, importRun: run) + try await sink.prepareImportRun() + try await sink.beginTransaction() + try await sink.execute(statement: "COMMIT") + try await sink.execute(statement: "BEGIN") + try await sink.execute(statement: "INSERT INTO t VALUES (1)") + if stopping { + run.stop(.rollBack) + try await sink.rollbackTransaction() + } else { + try await sink.commitTransaction() + } + #expect(fixture.driver.events.filter { $0 == "COMMIT" }.count == 1) + #expect(!fixture.driver.events.contains("ROLLBACK")) + #expect(run.snapshot.currentState == .open) + #expect(run.snapshot.holder == .none) + } + + @Test("A single-file failure includes rollback and cleanup failures") + func failureCarriesCleanupErrors() async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + fixture.driver.failingText = "bad" + fixture.driver.rollbackFails = true + fixture.driver.closeFails = true + let file = try #require(fixture.files(["bad"]).first) + var failure: (any Error)? + do { + _ = try await fixture.service.importFile(from: file.url, formatId: fixture.format, encoding: .utf8, + scope: fixture.scope, targetTable: file.targetTable, + columnMapping: file.columnMapping, sourceFields: file.sourceFields) + Issue.record("The failed write must be reported") + } catch { failure = error } + let text = TransferResultAlert.importFailureSummary(error: failure, result: fixture.service.stopResult) + #expect(text.contains("injected write failure")) + #expect(text.contains("injected rollback failure")) + #expect(text.contains("injected cleanup failure")) + #expect(text.contains("still open")) + } +} + + +extension ImportServiceStopTests { + @Test("An opaque SQL Server batch cannot leave finalization authority on its replacement transaction", + arguments: [false, true]) + func opaqueBatchRevokesAuthority(stopping: Bool) async throws { + let fixture = try StopFixture(type: .mssql) + defer { fixture.cleanup() } + let run = ImportRun() + let sink = ImportDataSinkAdapter(driver: fixture.adapter, databaseType: .mssql, importRun: run) + try await sink.prepareImportRun() + try await sink.beginTransaction() + try await sink.execute(statement: "COMMIT; BEGIN TRANSACTION; INSERT INTO t VALUES (1)") + if stopping { run.stop(.rollBack); try await sink.rollbackTransaction() } + else { try await sink.commitTransaction() } + #expect(!fixture.driver.events.contains { $0.hasPrefix("ROLLBACK") }) + #expect(fixture.driver.events.filter { $0.hasPrefix("COMMIT") }.count == 1) + #expect(run.snapshot.holder == .none) + #expect(run.snapshot.currentState == .open) + #expect(ImportStopResult(ledger: run.snapshot).completionNotes.contains("still open")) + } + + @Test("Normal completion surfaces a transaction left open after opaque SQL") + func completionShowsPendingTransaction() async throws { + let fixture = try StopFixture(type: .mssql) + defer { fixture.cleanup() } + let run = ImportRun() + let sink = ImportDataSinkAdapter(driver: fixture.adapter, databaseType: .mssql, importRun: run) + try await sink.prepareImportRun() + try await sink.beginTransaction() + try await sink.execute(statement: "INSERT INTO t VALUES (1)") + try await sink.commitTransaction() + let result = ImportStopResult(ledger: run.snapshot) + #expect(result.completionNotes.contains("still open")) + let report = RowImportBatchReport(files: [], outcomes: [], ending: .completed, transaction: .untracked, + executionTime: 0, stopResult: result) + #expect(TransferResultAlert.batchImportSummary(report).contains("still open")) + } + + @Test("Stop after a preparation failure preserves the error but reports cancellation", arguments: [false, true]) + func stopAfterPreparationFailure(clearing: Bool) async throws { + let fixture = try StopFixture() + defer { fixture.cleanup() } + let run = fixture.service.run + let keyword = clearing ? "DELETE" : "CREATE" + fixture.driver.heldText = keyword + fixture.driver.failingText = keyword + var planner = NewTableImportPlanner() + if clearing { + planner.recordCreated(TableScope(table: "t", in: fixture.scope), createTableSQL: "CREATE TABLE t (id TEXT)") + } + let definition = PluginCreateTableDefinition(tableName: "t", columns: [], primaryKeyColumns: []) + let maker = RowImportTableMaker(connection: fixture.connection, gate: AlwaysAllowGate()) + let task = Task { + try await maker.prepare([definition], scope: fixture.scope, planner: planner, importRun: run) { _, _ in } + } + await fixture.driver.hold.arrived(1) + fixture.service.requestStop() + await fixture.driver.hold.open() + try await waitUntil { run.waitingCallCount > 0 } + fixture.service.answerStop(.rollBack, generation: try #require(run.questionIfPaused()).generation) + await #expect(throws: PluginImportCancellationError.self) { try await task.value } + #expect(run.snapshot.errors.contains("injected write failure")) + #expect(fixture.service.stopResult.text.contains("failed before row import")) + } + + @Test("Long failure descriptions stay bounded on screen and remain whole in Copy Details") + func boundedFailureDetails() { + let marker = "last-error-detail" + let long = String(repeating: String(repeating: "x", count: 300) + "\n", count: 100) + marker + let error = DatabaseError.queryFailed(long) + var ledger = ImportLedger() + ledger.errors = [long] + ledger.cleanupErrors = ["rollback failed"] + ledger.currentState = .open + let result = ImportStopResult(ledger: ledger) + let display = TransferResultAlert.importFailureDisplay(error: error, result: result) + #expect(display.utf16.count < TransferResultAlert.shownErrorLengthLimit + 100) + #expect(display.components(separatedBy: "\n").count <= TransferResultAlert.shownErrorLineLimit + 1) + #expect(display.contains("still open")) + #expect(display.contains("rollback failed")) + #expect(result.details.contains(marker)) + } +} + + +extension ImportServiceStopTests { + @Test("A batch's open-transaction warning precedes preparation details", arguments: [false, true]) + func batchWarningStaysVisible(stopped: Bool) { + let connection = TestFixtures.makeConnection(database: "shop", type: .mysql) + let scope = DatabaseScope(connectionId: connection.id, database: "shop", schema: nil) + let file = RowImportRunItem(url: URL(fileURLWithPath: "/tmp/input.csv"), sourceName: "input.csv", + formatId: "csv", targetTable: "t", columnMapping: [:], sourceFields: []) + let error = DatabaseError.queryFailed("write failed") + var ledger = ImportLedger() + ledger.currentState = .open + ledger.cleanupErrors = ["rollback failed"] + for index in 0..<10 { + ledger.preparations[UUID()] = ImportPreparationEffect(table: TableScope(table: "t\(index)", in: scope), + clearing: false, completed: true) + } + let report = RowImportBatchReport(files: [file], outcomes: [.failed(error, rowsBefore: 0)], + ending: stopped ? .stopped(file: 0) : .failed(file: 0), + transaction: .untracked, executionTime: 0, + stopResult: ImportStopResult(ledger: ledger)) + let shown = TransferResultAlert.shownErrors(TransferResultAlert.batchImportSummary(report)) + #expect(shown.hasPrefix("A transaction is still open")) + #expect(shown.contains("rollback failed")) + } +} diff --git a/apple/TableProTests/Views/ImportCancelledTextTests.swift b/apple/TableProTests/Views/ImportCancelledTextTests.swift index d3a5ef1935..a203833180 100644 --- a/apple/TableProTests/Views/ImportCancelledTextTests.swift +++ b/apple/TableProTests/Views/ImportCancelledTextTests.swift @@ -7,7 +7,6 @@ import Foundation @testable import TablePro import Testing -/// A stopped import said what ran "stays committed" even when Wrap in transaction had just rolled all of it back. @MainActor struct ImportCancelledTextTests { @Test("A rolled back import says so") @@ -22,7 +21,6 @@ struct ImportCancelledTextTests { ) } - /// Under `autocommit = 0` the rollback still undoes what ran after the DDL, so only some of the work was kept. @Test("An import the database committed part way says some of it was kept, and why") func committedBeforeRollback() { #expect( @@ -37,11 +35,11 @@ struct ImportCancelledTextTests { ) } - @Test("An import with no transaction of its own keeps saying what ran stays committed") + @Test("An import with no known transaction outcome makes no durability claim") func untracked() { #expect( TransferResultAlert.importCancelledText(executedStatements: 3, transaction: .untracked) - == "3 statements had already run and stay committed." + == "Anything already imported may stay." ) #expect( TransferResultAlert.importCancelledText(executedStatements: 1, transaction: .committed) @@ -53,11 +51,11 @@ struct ImportCancelledTextTests { func endedOutsideTheImport() { #expect( TransferResultAlert.importCancelledText(executedStatements: 5, transaction: .endedByServer) - == "5 statements had already run and stay committed." + == "Anything already imported may stay." ) #expect( TransferResultAlert.importCancelledText(executedStatements: 5, transaction: .endedByFile) - == "5 statements had already run and stay committed." + == "Anything already imported may stay." ) } } From ea418bf0899ba0a2838ba964a10bbe48d7f0f5b5 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 4 Nov 2026 02:55:04 +0700 Subject: [PATCH 3/4] docs(changelog): note import stop and active dialog fixes --- apple/changelog.d/4031.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 apple/changelog.d/4031.fixed.md diff --git a/apple/changelog.d/4031.fixed.md b/apple/changelog.d/4031.fixed.md new file mode 100644 index 0000000000..9758524412 --- /dev/null +++ b/apple/changelog.d/4031.fixed.md @@ -0,0 +1 @@ +Stopping an import could misreport rollback, and opening another dialog could hide a running transfer. From bdacfa46691eeefd14ff6a857ec63e72375404b5 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 4 Nov 2026 03:12:47 +0700 Subject: [PATCH 4/4] fix(verify): build the missing Dameng bridge before plugins --- .../skills/fix-issue/scripts/lib/platform-apple.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.claude/skills/fix-issue/scripts/lib/platform-apple.sh b/.claude/skills/fix-issue/scripts/lib/platform-apple.sh index 6d62096f28..a1fb1c2acd 100644 --- a/.claude/skills/fix-issue/scripts/lib/platform-apple.sh +++ b/.claude/skills/fix-issue/scripts/lib/platform-apple.sh @@ -425,6 +425,18 @@ platform_step() { "$APPLE_ROOT/scripts/build-hana.sh" "$(uname -m)" >> "$log" 2>&1 \ && note "built the HANA helper first: $helper" fi + helper="$APPLE_ROOT/Native/DamengBridge/lib/libdameng_bridge.a" + if [ "$scheme" = "AllPlugins" ] && [ ! -e "$helper" ]; then + require_tool rustup + "$APPLE_ROOT/scripts/build-dameng.sh" "$(uname -m)" >> "$log" 2>&1 + code=$? + if [ $code -ne 0 ]; then + STATUS=FAIL + note "Dameng bridge build failed; see the log" + emit "$log" "$code" + fi + note "built the Dameng bridge first: $helper" + fi # shellcheck disable=SC2046 xcodebuild -project "$APPLE_ROOT/TablePro.xcodeproj" -scheme "$scheme" \ -configuration Debug build $(xcodebuild_flags) >> "$log" 2>&1