diff --git a/CHANGELOG.md b/CHANGELOG.md index e8a44ad72..6e2b640dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The same statements committed twice when Cmd+S is pressed again during a slow save. - Switching tabs during a save clearing the edits of the tab switched to, and re-running its query. - Placeholder SQL with no values in the Safe Mode confirmation and the authorization prompt. +- Deleting one row of a pasted batch dropping the other rows, or saving them with another row's values. +- Undoing the deletion of a new row putting its values in the wrong columns. +- Undoing a cell edit reverting the wrong row while a column filter is active. +- A save that deletes a row and reuses its unique value failing on the constraint. +- "Ignore foreign key checks" doing nothing on SQLite, libSQL and Cloudflare D1. +- A save reported as failed on an engine without transactions after some statements had been written. ## [0.68.1] - 2026-08-26 diff --git a/TablePro/Core/ChangeTracking/DataChangeManager.swift b/TablePro/Core/ChangeTracking/DataChangeManager.swift index 082c41f0f..0f2cbad23 100644 --- a/TablePro/Core/ChangeTracking/DataChangeManager.swift +++ b/TablePro/Core/ChangeTracking/DataChangeManager.swift @@ -270,7 +270,8 @@ final class DataChangeManager: ChangeManaging { ) } else { pending.reapplyCellChange( - rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, + rowIndex: rowIndex, + columnIndex: columnIndex, columnName: columnName, originalDBValue: newValue, newValue: previousValue, originalRow: originalRow ) } diff --git a/TablePro/Core/ChangeTracking/DataChangeModels.swift b/TablePro/Core/ChangeTracking/DataChangeModels.swift index 655bdcb05..ad37d3b00 100644 --- a/TablePro/Core/ChangeTracking/DataChangeModels.swift +++ b/TablePro/Core/ChangeTracking/DataChangeModels.swift @@ -14,21 +14,18 @@ enum ChangeType: Hashable { struct CellChange: Identifiable, Equatable { let id: UUID - let rowIndex: Int let columnIndex: Int let columnName: String let oldValue: PluginCellValue let newValue: PluginCellValue init( - rowIndex: Int, columnIndex: Int, columnName: String, oldValue: PluginCellValue, newValue: PluginCellValue ) { self.id = UUID() - self.rowIndex = rowIndex self.columnIndex = columnIndex self.columnName = columnName self.oldValue = oldValue @@ -43,17 +40,27 @@ struct RowChange: Identifiable, Equatable { var cellChanges: [CellChange] let originalRow: [PluginCellValue]? + /// The order the user made this change in. + /// + /// Not the array position. `PendingChanges` removes a cancelled change by swapping the last + /// element into its slot, so array order stops matching edit order the first time anything is + /// undone. Statement generation has to know which change came first, because deleting a row + /// and reusing its unique value in a new one only works in that order. + var sequence: Int + init( rowIndex: Int, type: ChangeType, cellChanges: [CellChange] = [], - originalRow: [PluginCellValue]? = nil + originalRow: [PluginCellValue]? = nil, + sequence: Int = 0 ) { self.id = UUID() self.rowIndex = rowIndex self.type = type self.cellChanges = cellChanges self.originalRow = originalRow + self.sequence = sequence } } diff --git a/TablePro/Core/ChangeTracking/PendingChanges.swift b/TablePro/Core/ChangeTracking/PendingChanges.swift index dd6174e98..6bc949235 100644 --- a/TablePro/Core/ChangeTracking/PendingChanges.swift +++ b/TablePro/Core/ChangeTracking/PendingChanges.swift @@ -15,6 +15,11 @@ import TableProPluginKit struct PendingChanges: Equatable { private(set) var changes: [RowChange] = [] private(set) var deletedRowIndices: Set = [] + + /// Stamped onto every change so statement generation can recover the order the user worked in. + /// `changes` cannot carry that order itself: a cancelled change is removed by swapping the last + /// element into its slot. + private var nextSequence = 0 private(set) var insertedRowIndices: Set = [] private(set) var modifiedCells: [Int: Set] = [:] private(set) var insertedRowData: [Int: [PluginCellValue]] = [:] @@ -67,7 +72,6 @@ struct PendingChanges: Equatable { } let cellChange = CellChange( - rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, oldValue: oldValue, @@ -137,16 +141,11 @@ struct PendingChanges: Equatable { mutating func undoBatchRowInsertion(rowIndices: [Int], columnCount: Int) -> [[PluginCellValue]] { let validRows = rowIndices.filter { insertedRowIndices.contains($0) } - var rowValues: [[PluginCellValue]] = [] - for rowIndex in validRows { - if let idx = changeIndex[RowChangeKey(rowIndex: rowIndex, type: .insert)] { - let values = changes[idx].cellChanges - .sorted { $0.columnIndex < $1.columnIndex } - .map { $0.newValue } - rowValues.append(values) - } else { - rowValues.append(Array(repeating: .null, count: columnCount)) - } + /// `insertedRowData` holds the whole row. `cellChanges` holds only the columns the user + /// typed, so rebuilding from it drops the untouched ones and slides the rest left: a name + /// typed into the third column comes back in the first. + let rowValues = validRows.map { rowIndex in + insertedRowData[rowIndex] ?? Array(repeating: PluginCellValue.null, count: columnCount) } for rowIndex in validRows { @@ -155,20 +154,7 @@ struct PendingChanges: Equatable { insertedRowData.removeValue(forKey: rowIndex) } - let sortedRemoved = validRows.sorted() - - var newInserted = Set() - for idx in insertedRowIndices { - newInserted.insert(idx - Self.countLessThan(idx, in: sortedRemoved)) - } - insertedRowIndices = newInserted - - for i in 0..= insertionPoint { - changes[i].rowIndex += 1 - } - insertedRowIndices = Set(insertedRowIndices.map { $0 >= insertionPoint ? $0 + 1 : $0 }) - deletedRowIndices = Set(deletedRowIndices.map { $0 >= insertionPoint ? $0 + 1 : $0 }) - - var newInsertedRowData: [Int: [PluginCellValue]] = [:] - for (key, value) in insertedRowData { - newInsertedRowData[key >= insertionPoint ? key + 1 : key] = value + /// Renumbers every collection this type keys by row index, in one place. + /// + /// The state is six things that have to agree: `changes[].rowIndex`, `changeIndex`, + /// `insertedRowIndices`, `deletedRowIndices`, `insertedRowData` and `modifiedCells`. There used + /// to be three renumbering paths handling three different subsets of them, and the gaps were + /// invisible: undoing one row of a pasted batch left the surviving rows' values filed under + /// their old indices, so Save wrote one row with another row's values and dropped the rest + /// without reporting anything. A single primitive is what makes that class of omission + /// impossible rather than merely absent. + /// + /// A new row always lands at the end of the grid, so nothing pending is ever below an inserted + /// one and the delete and modified-cell arms do not fire today. They are here because the + /// alternative is three renumbering paths covering three different subsets again, which is + /// what this replaced. + private mutating func reindex(_ transform: (Int) -> Int) { + for i in 0 ..< changes.count { + changes[i].rowIndex = transform(changes[i].rowIndex) } - insertedRowData = newInsertedRowData - - var newModifiedCells: [Int: Set] = [:] - for (key, value) in modifiedCells { - newModifiedCells[key >= insertionPoint ? key + 1 : key] = value - } - modifiedCells = newModifiedCells - + insertedRowIndices = Set(insertedRowIndices.map(transform)) + deletedRowIndices = Set(deletedRowIndices.map(transform)) + insertedRowData = Dictionary( + uniqueKeysWithValues: insertedRowData.map { (transform($0.key), $0.value) } + ) + modifiedCells = Dictionary( + uniqueKeysWithValues: modifiedCells.map { (transform($0.key), $0.value) } + ) rebuildChangeIndex() } - private mutating func shiftRowIndicesDown(at removedRow: Int) { - for i in 0.. removedRow { - changes[i].rowIndex -= 1 - } - insertedRowIndices = Set(insertedRowIndices.map { $0 > removedRow ? $0 - 1 : $0 }) + private mutating func shiftRowIndicesUp(from insertionPoint: Int) { + reindex { $0 >= insertionPoint ? $0 + 1 : $0 } + } - var newInsertedRowData: [Int: [PluginCellValue]] = [:] - for (key, value) in insertedRowData { - newInsertedRowData[key > removedRow ? key - 1 : key] = value - } - insertedRowData = newInsertedRowData + private mutating func shiftRowIndicesDown(at removedRow: Int) { + modifiedCells.removeValue(forKey: removedRow) + reindex { $0 > removedRow ? $0 - 1 : $0 } + } - var newModifiedCells: [Int: Set] = [:] - for (key, value) in modifiedCells where key != removedRow { - newModifiedCells[key > removedRow ? key - 1 : key] = value + /// The same renumbering for a whole batch removed at once. + private mutating func shiftRowIndicesDown(atSortedRows removedRows: [Int]) { + for removedRow in removedRows { + modifiedCells.removeValue(forKey: removedRow) } - modifiedCells = newModifiedCells - rebuildChangeIndex() + reindex { $0 - Self.countLessThan($0, in: removedRows) } } /// Binary search: count of elements strictly less than `target` in a sorted array. diff --git a/TablePro/Core/ChangeTracking/SQLStatementGenerator.swift b/TablePro/Core/ChangeTracking/SQLStatementGenerator.swift index dd3d61bcb..ca2c3c5eb 100644 --- a/TablePro/Core/ChangeTracking/SQLStatementGenerator.swift +++ b/TablePro/Core/ChangeTracking/SQLStatementGenerator.swift @@ -90,9 +90,14 @@ struct SQLStatementGenerator { /// The same statements, each carrying how many rows it is meant to touch. /// - /// A delete is batched, so the count is not always one and the batching rule lives here. A - /// caller that verified the server's affected-row count against a rule of its own would be a - /// second copy of that rule with nothing keeping the two in step. + /// Emitted in the order the user made the changes, because that order can be load-bearing: a + /// row deleted to free a unique value, and a new row taking that value, only work if the + /// DELETE runs first. Grouping every INSERT ahead of every DELETE, which is what this used to + /// do, turned that save into a constraint violation and rolled the whole thing back. + /// + /// Deletes still batch, but only across a consecutive run of them. That keeps the case that + /// matters, selecting many rows and pressing Delete, on one statement, while a delete + /// separated from another delete by an insert stays on its own side of it. func generateAttributedStatements( from changes: [RowChange], insertedRowData: [Int: [PluginCellValue]], @@ -100,44 +105,35 @@ struct SQLStatementGenerator { insertedRowIndices: Set ) -> [AttributedStatement] { var statements: [AttributedStatement] = [] + var deleteRun: [RowChange] = [] - // Collect UPDATE and DELETE changes to batch them - var updateChanges: [RowChange] = [] - var deleteChanges: [RowChange] = [] + func flushDeleteRun() { + guard !deleteRun.isEmpty else { return } + statements.append(contentsOf: generateDeleteStatements(for: deleteRun)) + deleteRun.removeAll(keepingCapacity: true) + } - for change in changes { + for change in changes.sorted(by: { $0.sequence < $1.sequence }) { switch change.type { case .update: - updateChanges.append(change) + flushDeleteRun() + if let stmt = generateUpdateSQL(for: change) { + statements.append(AttributedStatement(statement: stmt, kind: .update, rowCount: 1)) + } case .insert: // SAFETY: Verify the row is still marked as inserted - guard insertedRowIndices.contains(change.rowIndex) else { - continue - } + guard insertedRowIndices.contains(change.rowIndex) else { continue } + flushDeleteRun() if let stmt = generateInsertSQL(for: change, insertedRowData: insertedRowData) { statements.append(AttributedStatement(statement: stmt, kind: .insert, rowCount: 1)) } case .delete: // SAFETY: Verify the row is still marked as deleted - guard deletedRowIndices.contains(change.rowIndex) else { - continue - } - deleteChanges.append(change) + guard deletedRowIndices.contains(change.rowIndex) else { continue } + deleteRun.append(change) } } - - // Generate individual UPDATE statements (safer than batched CASE/WHEN) - if !updateChanges.isEmpty { - for change in updateChanges { - if let stmt = generateUpdateSQL(for: change) { - statements.append(AttributedStatement(statement: stmt, kind: .update, rowCount: 1)) - } - } - } - - if !deleteChanges.isEmpty { - statements.append(contentsOf: generateDeleteStatements(for: deleteChanges)) - } + flushDeleteRun() return statements } diff --git a/TablePro/Core/Coordinators/RowEditingCoordinator+Discard.swift b/TablePro/Core/Coordinators/RowEditingCoordinator+Discard.swift index b90134efa..416a80c82 100644 --- a/TablePro/Core/Coordinators/RowEditingCoordinator+Discard.swift +++ b/TablePro/Core/Coordinators/RowEditingCoordinator+Discard.swift @@ -41,7 +41,7 @@ extension RowEditingCoordinator { route: DatabaseManager.shared.executionRoute(for: scope), cancellation: .protectedWrite ) { driver in - try await DataWriteExecutor.run(statements: statements, mode: mode, on: driver) + _ = try await DataWriteExecutor.run(statements: statements, mode: mode, on: driver) } } diff --git a/TablePro/Core/Coordinators/RowEditingCoordinator+SaveChanges.swift b/TablePro/Core/Coordinators/RowEditingCoordinator+SaveChanges.swift index 39c889bb2..84c80ca9c 100644 --- a/TablePro/Core/Coordinators/RowEditingCoordinator+SaveChanges.swift +++ b/TablePro/Core/Coordinators/RowEditingCoordinator+SaveChanges.swift @@ -191,7 +191,7 @@ extension RowEditingCoordinator { let operationStart = ContinuousClock.Instant.now do { - let results = try await DatabaseManager.shared.withScopedDriver( + let run = try await DatabaseManager.shared.withScopedDriver( scope: scope, route: route, cancellation: .protectedWrite @@ -200,8 +200,9 @@ extension RowEditingCoordinator { } let history = recordSuccessHistory( - steps: validSteps, results: results, connection: conn, scope: scope + steps: validSteps, results: run.results, connection: conn, scope: scope ) + recordSideStatementHistory(run.sideStatements, connection: conn, scope: scope) captureRewindRecord(plan: plan, history: history, connection: conn) finishSuccessfulSave( @@ -215,7 +216,7 @@ extension RowEditingCoordinator { parent.saveCompletionContinuation?.resume(returning: true) parent.saveCompletionContinuation = nil reportSaveFinished( - .succeeded(OperationSummary(rowsAffected: results.reduce(0) { $0 + $1.rowsAffected })), + .succeeded(OperationSummary(rowsAffected: run.results.reduce(0) { $0 + $1.rowsAffected })), connection: conn, database: scope.database, tabId: savingTabId, @@ -231,33 +232,36 @@ extension RowEditingCoordinator { startedAt: operationStart ) - for step in validSteps { - let historySQL = step.statement.sql.trimmingCharacters(in: .whitespacesAndNewlines) - parent.recordHistory( - QueryHistoryRecordRequest( - query: historySQL.hasSuffix(";") ? historySQL : historySQL + ";", - connectionId: conn.id, - databaseName: scope.database, - databaseType: conn.type, - schemaName: scope.schema, - source: .rowEdit, - executionTime: executionTime, - rowCount: -1, - wasSuccessful: false, - errorMessage: error.localizedDescription - ) - ) - } + let partial = error as? DataWritePartialCommitError + recordFailureHistory( + steps: validSteps, + committed: partial?.committed ?? [], + connection: conn, + scope: scope, + executionTime: executionTime, + error: error + ) let diagnosis = DatabaseWriteRejectionDiagnosis.classify(error) let writeError = error as? DataWriteError - AlertHelper.showErrorSheet( - title: String(localized: "Save Failed"), - message: writeError?.errorDescription ?? diagnosis?.errorDescription ?? error.localizedDescription, - recoverySuggestion: writeError?.recoverySuggestion ?? diagnosis?.recoverySuggestion, - window: parent.contentWindow - ) + if let partial { + AlertHelper.showErrorSheet( + title: String(localized: "Save Incomplete"), + message: [partial.errorDescription, partial.partialCommitMessage] + .compactMap { $0 }.joined(separator: "\n\n"), + recoverySuggestion: partial.recoverySuggestion, + window: parent.contentWindow + ) + } else { + AlertHelper.showErrorSheet( + title: String(localized: "Save Failed"), + message: writeError?.errorDescription ?? diagnosis?.errorDescription + ?? error.localizedDescription, + recoverySuggestion: writeError?.recoverySuggestion ?? diagnosis?.recoverySuggestion, + window: parent.contentWindow + ) + } if clearTableOps { restorePendingTableOperations( @@ -372,6 +376,68 @@ extension RowEditingCoordinator { return firstRowEdit } + /// The foreign-key toggles run outside the transaction, so they are not steps and would + /// otherwise vanish from history even though they ran against the user's database. + private func recordSideStatementHistory( + _ statements: [String], + connection: DatabaseConnection, + scope: DatabaseScope + ) { + for statement in statements { + let sql = statement.trimmingCharacters(in: .whitespacesAndNewlines) + guard !sql.isEmpty else { continue } + parent.recordHistory( + QueryHistoryRecordRequest( + query: sql.hasSuffix(";") ? sql : sql + ";", + connectionId: connection.id, + databaseName: scope.database, + databaseType: connection.type, + schemaName: scope.schema, + source: .rowEdit, + executionTime: 0, + rowCount: -1, + wasSuccessful: true + ) + ) + } + } + + /// A statement that committed before the failure is recorded as the success it was. + /// + /// Marking the whole batch failed is a lie on any engine without transactions, and it is the + /// lie that makes the user press Save again and write those rows twice. + private func recordFailureHistory( + steps: [DataWriteStep], + committed: [DataWriteStepResult], + connection: DatabaseConnection, + scope: DatabaseScope, + executionTime: TimeInterval, + error: any Error + ) { + for (offset, step) in steps.enumerated() { + let sql = step.statement.sql.trimmingCharacters(in: .whitespacesAndNewlines) + guard !sql.isEmpty else { continue } + /// Only the statement right after the committed ones is the one that failed. Anything + /// past it never ran, so it is not history at all. + if offset > committed.count { break } + let didCommit = offset < committed.count + parent.recordHistory( + QueryHistoryRecordRequest( + query: sql.hasSuffix(";") ? sql : sql + ";", + connectionId: connection.id, + databaseName: scope.database, + databaseType: connection.type, + schemaName: scope.schema, + source: .rowEdit, + executionTime: didCommit ? committed[offset].executionTime : executionTime, + rowCount: didCommit ? committed[offset].rowsAffected : -1, + wasSuccessful: didCommit, + errorMessage: didCommit ? nil : error.localizedDescription + ) + ) + } + } + /// Keeps what the rows looked like before this save, so it can be offered back. /// /// Written after the commit and before the change set is cleared, which is the only window diff --git a/TablePro/Core/Coordinators/RowEditingCoordinator.swift b/TablePro/Core/Coordinators/RowEditingCoordinator.swift index 768e6377e..7fa252084 100644 --- a/TablePro/Core/Coordinators/RowEditingCoordinator.swift +++ b/TablePro/Core/Coordinators/RowEditingCoordinator.swift @@ -271,8 +271,11 @@ final class RowEditingCoordinator { let tabId = tab.id var application = RowOperationsManager.UndoApplicationResult(adjustedSelection: nil, delta: .none) + let displayIDs = parent.activeGridDisplayIDs parent.mutateActiveTableRows(for: tabId) { rows in - let applied = parent.rowOperationsManager.applyUndoResult(result, tableRows: &rows) + let applied = parent.rowOperationsManager.applyUndoResult( + result, displayIDs: displayIDs, tableRows: &rows + ) application = applied return applied.delta } diff --git a/TablePro/Core/DataWrite/DataWriteExecutor.swift b/TablePro/Core/DataWrite/DataWriteExecutor.swift index cfcaf4a4f..89393f507 100644 --- a/TablePro/Core/DataWrite/DataWriteExecutor.swift +++ b/TablePro/Core/DataWrite/DataWriteExecutor.swift @@ -25,29 +25,92 @@ struct DataWriteStepResult: Sendable { let wasVerified: Bool } +/// What a run of the plan did: one result per step that ran, plus the prologue and epilogue +/// statements that were executed around them, which still belong in query history. +struct DataWriteRun: Sendable { + let results: [DataWriteStepResult] + let sideStatements: [String] +} + +/// Some of the batch is on the server and cannot be taken back. +/// +/// Distinct from `DataWriteError` on purpose: that one is an `Equatable` enum whose cases are +/// compared in tests, and results do not belong in it. Modelled on `PrincipalApplyError`, which +/// reports the same shape for principals. +struct DataWritePartialCommitError: LocalizedError { + let committed: [DataWriteStepResult] + let totalStatements: Int + let engine: String + let underlying: any Error + + var errorDescription: String? { + underlying.localizedDescription + } + + var partialCommitMessage: String { + String( + format: String( + localized: "%1$lld of %2$lld statements were already written, and %3$@ cannot roll them back." + ), + committed.count, totalStatements, engine + ) + } + + var recoverySuggestion: String? { + String(localized: "Refresh the table to see what was written. Saving again would write those rows a second time.") + } +} + enum DataWriteExecutor { nonisolated private static let logger = Logger(subsystem: "com.TablePro", category: "DataWriteExecutor") /// Runs every step of the plan against one driver, and returns what each one actually did. /// - /// The rollback and the foreign-key re-enable are part of the same lease as the statements: - /// resolving a driver again afterwards can reach a handle that has already been released, or - /// one sitting on another database. + /// The prologue runs before the transaction opens and the epilogue after it closes, on both + /// the commit and the rollback path. The rollback and the epilogue are part of the same lease + /// as the statements: resolving a driver again afterwards can reach a handle that has already + /// been released, or one sitting on another database. nonisolated static func run( _ plan: DataWritePlan, mode: PluginTransactionAccessMode = .readWrite, on driver: DatabaseDriver - ) async throws -> [DataWriteStepResult] { + ) async throws -> DataWriteRun { let steps = plan.steps.filter { !$0.statement.sql.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty } - guard !steps.isEmpty else { return [] } + guard !steps.isEmpty || !plan.prologue.isEmpty else { return DataWriteRun(results: [], sideStatements: []) } - let useTransaction = driver.supportsTransactions - if useTransaction { - try await driver.beginTransaction(mode: mode) + var sideStatements: [String] = [] + for statement in plan.prologue { + do { + _ = try await driver.execute(query: statement) + sideStatements.append(statement) + } catch { + logger.warning( + "Prologue statement failed '\(statement, privacy: .public)': \(error.localizedDescription, privacy: .public)" + ) + } } + let useTransaction = driver.supportsTransactions var results: [DataWriteStepResult] = [] + + func drainEpilogue() async { + for statement in plan.epilogue { + do { + _ = try await driver.execute(query: statement) + sideStatements.append(statement) + } catch { + logger.warning( + "Failed to re-enable foreign key checks with statement '\(statement, privacy: .public)': \(error.localizedDescription, privacy: .public)" + ) + } + } + } + do { + if useTransaction { + try await driver.beginTransaction(mode: mode) + } + for step in steps { let start = Date() let result: QueryResult @@ -74,25 +137,33 @@ enum DataWriteExecutor { try await driver.commitTransaction() } } catch { + var rollbackSucceeded = useTransaction if useTransaction { do { try await driver.rollbackTransaction() } catch { + rollbackSucceeded = false logger.error("Rollback failed: \(error.localizedDescription, privacy: .public)") } } - for statement in plan.epilogue { - do { - _ = try await driver.execute(query: statement) - } catch { - logger.warning( - "Failed to re-enable foreign key checks with statement '\(statement, privacy: .public)': \(error.localizedDescription, privacy: .public)" - ) - } + await drainEpilogue() + + /// Without a transaction the statements that already ran are on the server for good, + /// and so they are when the rollback itself failed. Reporting that as a plain failure + /// tells the user to try again, and trying again writes them a second time. + if !rollbackSucceeded, !results.isEmpty { + throw DataWritePartialCommitError( + committed: results, + totalStatements: steps.count, + engine: plan.databaseType.rawValue, + underlying: error + ) } throw error } - return results + + await drainEpilogue() + return DataWriteRun(results: results, sideStatements: sideStatements) } /// For a caller that has statements rather than a plan: the discard path, which throws work @@ -102,7 +173,7 @@ enum DataWriteExecutor { epilogue: [String] = [], mode: PluginTransactionAccessMode = .readWrite, on driver: DatabaseDriver - ) async throws -> [DataWriteStepResult] { + ) async throws -> DataWriteRun { try await run( DataWritePlan( scope: DatabaseScope(connectionId: UUID(), database: "", schema: nil), diff --git a/TablePro/Core/DataWrite/DataWritePlan.swift b/TablePro/Core/DataWrite/DataWritePlan.swift index 174e2ddff..fe6750190 100644 --- a/TablePro/Core/DataWrite/DataWritePlan.swift +++ b/TablePro/Core/DataWrite/DataWritePlan.swift @@ -104,7 +104,17 @@ struct DataWritePlan: Sendable { /// The rows this plan writes, in the order the user changed them. Built from the change set /// rather than from the statements, so it is complete even for a driver that writes its own. let rowOperations: [RowWriteOperation] - /// Statements that must run after the transaction, never inside it. + /// Statements that must run before the transaction opens, never inside it. + /// + /// SQLite, libSQL and Cloudflare D1 disable foreign keys with `PRAGMA foreign_keys = OFF`, + /// which SQLite documents as a no-op inside a transaction. Run there, "Ignore foreign key + /// checks" silently does nothing. MySQL's session variable would work either side, so outside + /// is the placement that is correct for every engine rather than most of them. + let prologue: [String] + + /// Statements that must run after the transaction, never inside it. They run on the way out of + /// both a commit and a rollback, because leaving foreign keys disabled is worse than the + /// failure that got there. let epilogue: [String] init( @@ -112,17 +122,23 @@ struct DataWritePlan: Sendable { databaseType: DatabaseType, steps: [DataWriteStep], rowOperations: [RowWriteOperation] = [], + prologue: [String] = [], epilogue: [String] = [] ) { self.scope = scope self.databaseType = databaseType self.steps = steps self.rowOperations = rowOperations + self.prologue = prologue self.epilogue = epilogue } var statements: [ParameterizedStatement] { - steps.map(\.statement) + (prologue + epilogue).isEmpty + ? steps.map(\.statement) + : prologue.map { ParameterizedStatement(sql: $0, parameters: []) } + + steps.map(\.statement) + + epilogue.map { ParameterizedStatement(sql: $0, parameters: []) } } var containsTableOperation: Bool { @@ -131,6 +147,7 @@ struct DataWritePlan: Sendable { var isEmpty: Bool { steps.allSatisfy { $0.statement.sql.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty } + && prologue.isEmpty } /// The plan as a person would read it, with the bound values written in. @@ -139,7 +156,9 @@ struct DataWritePlan: Sendable { /// Safe Mode confirmation and the authorization gate. Showing them the parameterized form /// asks someone to approve `WHERE "id" = ?`, which names no row at all. var displayStatements: [String] { - steps.map { SQLParameterInliner.inline($0.statement, databaseType: databaseType) } + prologue + + steps.map { SQLParameterInliner.inline($0.statement, databaseType: databaseType) } + + epilogue } var displaySQL: String { diff --git a/TablePro/Core/DataWrite/Rewind/RewindExecutor.swift b/TablePro/Core/DataWrite/Rewind/RewindExecutor.swift index 487420920..d29fad2ea 100644 --- a/TablePro/Core/DataWrite/Rewind/RewindExecutor.swift +++ b/TablePro/Core/DataWrite/Rewind/RewindExecutor.swift @@ -125,7 +125,7 @@ struct RewindExecutor { ) let route = DatabaseManager.shared.executionRoute(for: scope) - let results = try await ExecutionGateProvider.shared.authorizing( + let run = try await ExecutionGateProvider.shared.authorizing( OperationRequest( connectionId: connection.id, databaseType: connection.type, @@ -145,16 +145,16 @@ struct RewindExecutor { } } - recordHistory(for: plan, results: results) + recordHistory(for: plan, results: run.results) await captureInverseRecord(for: plan) Self.logger.info( - "Restored \(plan.restorableCount, privacy: .public) rows in \(results.count, privacy: .public) statements" + "Restored \(plan.restorableCount, privacy: .public) rows in \(run.results.count, privacy: .public) statements" ) return RewindApplyResult( restoredRows: plan.restorableCount, skippedRows: plan.skippedCount, - statementCount: results.count + statementCount: run.results.count ) } } diff --git a/TablePro/Core/DataWrite/Rewind/RewindPlanner.swift b/TablePro/Core/DataWrite/Rewind/RewindPlanner.swift index 2c55fd47d..9d97db452 100644 --- a/TablePro/Core/DataWrite/Rewind/RewindPlanner.swift +++ b/TablePro/Core/DataWrite/Rewind/RewindPlanner.swift @@ -199,7 +199,7 @@ struct RewindPlanner { index < preImage.count, index < postImage.count else { return nil } return CellChange( - rowIndex: 0, columnIndex: index, columnName: column, + columnIndex: index, columnName: column, oldValue: postImage[index], newValue: preImage[index] ) } diff --git a/TablePro/Core/Services/Query/RowOperationsManager.swift b/TablePro/Core/Services/Query/RowOperationsManager.swift index f00c87b6f..50f661cb3 100644 --- a/TablePro/Core/Services/Query/RowOperationsManager.swift +++ b/TablePro/Core/Services/Query/RowOperationsManager.swift @@ -180,10 +180,26 @@ final class RowOperationsManager { ) } - func applyUndoResult(_ result: UndoResult, tableRows: inout TableRows) -> UndoApplicationResult { + /// `displayIDs` is the grid's display order when a per-column value filter is narrowing it. + /// + /// A cell edit is tracked by its display row, the same as the modified and deleted marks the + /// grid draws, but `TableRows` is indexed by storage position. The forward write converts; this + /// one has to as well, or undoing an edit under a filter puts the old value into whichever row + /// happens to sit at that storage offset. The row arms need no conversion: an inserted row is + /// tracked by its storage index from the start, which is what lets it be physically removed. + func applyUndoResult( + _ result: UndoResult, + displayIDs: [RowID]?, + tableRows: inout TableRows + ) -> UndoApplicationResult { switch result.action { case .cellEdit(let rowIndex, let columnIndex, _, let previousValue, _, _): - let delta = tableRows.edit(row: rowIndex, column: columnIndex, value: previousValue) + guard let storageRow = DisplayRowMapping.rowIndex( + forDisplay: rowIndex, displayIDs: displayIDs, in: tableRows + ) else { + return UndoApplicationResult(adjustedSelection: nil, delta: .none) + } + let delta = tableRows.edit(row: storageRow, column: columnIndex, value: previousValue) return UndoApplicationResult(adjustedSelection: nil, delta: delta) case .rowInsertion(let rowIndex): diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+SQLPreview.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+SQLPreview.swift index f20ccfabf..17bc1c4cf 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+SQLPreview.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+SQLPreview.swift @@ -61,16 +61,15 @@ extension MainContentCoordinator { let hasPendingTableOps = !pendingTruncates.isEmpty || !pendingDeletes.isEmpty var steps: [DataWriteStep] = [] - // FK disable must be FIRST, before any transaction begins + /// The foreign-key toggles are not steps. A step runs inside the transaction, and + /// `PRAGMA foreign_keys` is a no-op there on every SQLite-derived engine, so the option + /// would silently do nothing. They travel as the plan's prologue and epilogue instead. let needsDisableFK = PluginManager.shared.supportsForeignKeyDisable(for: dbType) && pendingTruncates.union(pendingDeletes).contains { tableName in tableOperationOptions[tableName]?.ignoreForeignKeys == true } - if needsDisableFK { - steps.append(contentsOf: fkDisableStatements(for: dbType).map { - DataWriteStep(kind: .foreignKeyToggle, statement: ParameterizedStatement(sql: $0, parameters: [])) - }) - } + let prologue = needsDisableFK ? fkDisableStatements(for: dbType) : [] + let epilogue = needsDisableFK ? fkEnableStatements(for: dbType) : [] let scope = selectedTabScope ?? DatabaseScope(connectionId: connection.id, database: "", schema: nil) @@ -99,18 +98,13 @@ extension MainContentCoordinator { }) } - // FK re-enable must be LAST, after the row work - let foreignKeyEnableStatements = needsDisableFK ? fkEnableStatements(for: dbType) : [] - steps.append(contentsOf: foreignKeyEnableStatements.map { - DataWriteStep(kind: .foreignKeyToggle, statement: ParameterizedStatement(sql: $0, parameters: [])) - }) - return DataWritePlan( scope: scope, databaseType: dbType, steps: steps, rowOperations: rowOperations, - epilogue: foreignKeyEnableStatements + prologue: prologue, + epilogue: epilogue ) } diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarSave.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarSave.swift index b261f1212..c2d431a4d 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarSave.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+SidebarSave.swift @@ -45,7 +45,6 @@ extension MainContentCoordinator { ? originalRow[field.columnIndex] : .null return CellChange( - rowIndex: rowIndex, columnIndex: field.columnIndex, columnName: field.columnName, oldValue: oldValue, diff --git a/TableProTests/Core/ChangeTracking/DataChangeModelsTests.swift b/TableProTests/Core/ChangeTracking/DataChangeModelsTests.swift index 20a6b67f1..cac0a254d 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeModelsTests.swift +++ b/TableProTests/Core/ChangeTracking/DataChangeModelsTests.swift @@ -30,14 +30,12 @@ struct DataChangeModelsTests { @Test("CellChange stores values correctly") func cellChangeStoresValues() { let cellChange = CellChange( - rowIndex: 5, columnIndex: 2, columnName: "email", oldValue: "old@example.com", newValue: "new@example.com" ) - #expect(cellChange.rowIndex == 5) #expect(cellChange.columnIndex == 2) #expect(cellChange.columnName == "email") #expect(cellChange.oldValue == "old@example.com") @@ -47,7 +45,6 @@ struct DataChangeModelsTests { @Test("CellChange with nil values") func cellChangeNilValues() { let cellChange = CellChange( - rowIndex: 0, columnIndex: 1, columnName: "description", oldValue: nil, @@ -61,14 +58,12 @@ struct DataChangeModelsTests { @Test("CellChange has unique id") func cellChangeUniqueId() { let change1 = CellChange( - rowIndex: 1, columnIndex: 2, columnName: "name", oldValue: "old", newValue: "new" ) let change2 = CellChange( - rowIndex: 1, columnIndex: 2, columnName: "name", oldValue: "old", @@ -82,7 +77,6 @@ struct DataChangeModelsTests { @Test("RowChange stores values correctly") func rowChangeStoresValues() { let cellChange = CellChange( - rowIndex: 3, columnIndex: 1, columnName: "status", oldValue: "active", diff --git a/TableProTests/Core/ChangeTracking/PendingChangesReindexTests.swift b/TableProTests/Core/ChangeTracking/PendingChangesReindexTests.swift new file mode 100644 index 000000000..c5a075f56 --- /dev/null +++ b/TableProTests/Core/ChangeTracking/PendingChangesReindexTests.swift @@ -0,0 +1,105 @@ +// +// PendingChangesReindexTests.swift +// TableProTests +// +// Undoing one row of a pasted batch used to renumber only two of the six things PendingChanges +// keys by row index, so the survivors' values stayed filed under their old numbers and Save wrote +// one row with another row's values while dropping the rest, reporting success either way. +// + +import Foundation +import TableProPluginKit +@testable import TablePro +import Testing + +@Suite("PendingChanges - reindexing") +struct PendingChangesReindexTests { + @Test("Undoing one row of a batch leaves the survivors' values under their new indices") + func partialBatchUndoKeepsSurvivorValues() { + var pending = PendingChanges() + pending.recordRowInsertion(rowIndex: 10, values: ["a1", "a2"]) + pending.recordRowInsertion(rowIndex: 11, values: ["b1", "b2"]) + pending.recordRowInsertion(rowIndex: 12, values: ["c1", "c2"]) + + let removed = pending.undoBatchRowInsertion(rowIndices: [10], columnCount: 2) + + #expect(removed == [["a1", "a2"]]) + #expect(pending.savedInsertedValues(forRow: 10) == ["b1", "b2"]) + #expect(pending.savedInsertedValues(forRow: 11) == ["c1", "c2"]) + #expect(pending.savedInsertedValues(forRow: 12) == nil) + #expect(pending.isRowInserted(10)) + #expect(pending.isRowInserted(11)) + #expect(!pending.isRowInserted(12)) + } + + @Test("The returned values are the whole row, not only the columns that were typed into") + func partialBatchUndoReturnsWholeRow() { + var pending = PendingChanges() + pending.recordRowInsertion(rowIndex: 0, values: [.null, .null, .null, .null]) + pending.recordCellChange( + rowIndex: 0, columnIndex: 2, columnName: "name", + oldValue: .null, newValue: "Bob" + ) + + let removed = pending.undoBatchRowInsertion(rowIndices: [0], columnCount: 4) + + #expect(removed.first?.count == 4) + #expect(removed.first?[2] == "Bob") + #expect(removed.first?[0] == .null) + } + + @Test("A restored batch comes back with the values it had, not a compacted version of them") + func undoThenRedoRoundTrips() { + var pending = PendingChanges() + pending.recordRowInsertion(rowIndex: 0, values: [.null, .null, .null]) + pending.recordCellChange( + rowIndex: 0, columnIndex: 1, columnName: "name", + oldValue: .null, newValue: "Bob" + ) + + let removed = pending.undoBatchRowInsertion(rowIndices: [0], columnCount: 3) + pending.reinsertBatch(rowIndices: [0], rowValues: removed, columns: ["id", "name", "note"]) + + #expect(pending.savedInsertedValues(forRow: 0)?.count == 3) + #expect(pending.savedInsertedValues(forRow: 0)?[1] == "Bob") + } +} + +@Suite("PendingChanges - change order") +struct PendingChangesSequenceTests { + @Test("Every recorded change gets a rising sequence number") + func sequenceRises() { + var pending = PendingChanges() + pending.recordRowDeletion(rowIndex: 0, originalRow: ["a"]) + pending.recordRowInsertion(rowIndex: 1, values: ["b"]) + + let sequences = pending.changes.map(\.sequence) + #expect(sequences == sequences.sorted()) + #expect(Set(sequences).count == sequences.count) + } + + /// A cancelled change is removed by swapping the last element into its slot, so array order + /// stops matching edit order. The sequence number is what survives that. + @Test("Cancelling a change does not disturb the order of the ones that remain") + func cancellingKeepsOrder() { + var pending = PendingChanges() + pending.recordCellChange( + rowIndex: 0, columnIndex: 0, columnName: "a", + oldValue: "before", newValue: "after", originalRow: ["before"] + ) + pending.recordRowDeletion(rowIndex: 1, originalRow: ["b"]) + pending.recordRowInsertion(rowIndex: 2, values: ["c"]) + + let deleteSequence = pending.changes.first { $0.type == .delete }?.sequence + let insertSequence = pending.changes.first { $0.type == .insert }?.sequence + + pending.recordCellChange( + rowIndex: 0, columnIndex: 0, columnName: "a", + oldValue: "after", newValue: "before", originalRow: ["before"] + ) + + #expect(pending.changes.contains { $0.type == .delete && $0.sequence == deleteSequence }) + #expect(pending.changes.contains { $0.type == .insert && $0.sequence == insertSequence }) + #expect(deleteSequence.map { seq in insertSequence.map { $0 > seq } ?? false } == true) + } +} diff --git a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorBinaryTests.swift b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorBinaryTests.swift index 5cdbb96bc..1e557bcd8 100644 --- a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorBinaryTests.swift +++ b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorBinaryTests.swift @@ -31,7 +31,6 @@ struct SQLStatementGeneratorBinaryTests { type: .update, cellChanges: [ CellChange( - rowIndex: 0, columnIndex: 1, columnName: "payload", oldValue: .null, @@ -65,7 +64,6 @@ struct SQLStatementGeneratorBinaryTests { type: .insert, cellChanges: [ CellChange( - rowIndex: 0, columnIndex: 1, columnName: "payload", oldValue: .null, @@ -106,7 +104,6 @@ struct SQLStatementGeneratorBinaryTests { type: .update, cellChanges: [ CellChange( - rowIndex: 0, columnIndex: 1, columnName: "payload", oldValue: .null, @@ -164,7 +161,6 @@ struct SQLStatementGeneratorBinaryTests { type: .update, cellChanges: [ CellChange( - rowIndex: 0, columnIndex: 1, columnName: "payload", oldValue: .text("old"), diff --git a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorCompositePKTests.swift b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorCompositePKTests.swift index 9a5765970..39b1d165e 100644 --- a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorCompositePKTests.swift +++ b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorCompositePKTests.swift @@ -40,7 +40,7 @@ struct SQLStatementGeneratorCompositePKTests { rowIndex: rowIndex, type: .update, cellChanges: [CellChange( - rowIndex: rowIndex, columnIndex: columnIndex, + columnIndex: columnIndex, columnName: columnName, oldValue: PluginCellValue.fromOptional(oldValue), newValue: PluginCellValue.fromOptional(newValue) @@ -149,8 +149,8 @@ struct SQLStatementGeneratorCompositePKTests { let stmts = generate([ makeMultiCellUpdateChange( cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 2, columnName: "quantity", oldValue: "5", newValue: "10"), - CellChange(rowIndex: 0, columnIndex: 3, columnName: "price", oldValue: "9.99", newValue: "12.99"), + CellChange(columnIndex: 2, columnName: "quantity", oldValue: "5", newValue: "10"), + CellChange(columnIndex: 3, columnName: "price", oldValue: "9.99", newValue: "12.99"), ], originalRow: ["1", "42", "5", "9.99"] ), @@ -467,9 +467,9 @@ struct SQLStatementGeneratorCompositePKTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 0, columnName: "order_id", oldValue: "1", newValue: "1"), - CellChange(rowIndex: 0, columnIndex: 1, columnName: "product_id", oldValue: "42", newValue: "42"), - CellChange(rowIndex: 0, columnIndex: 2, columnName: "quantity", oldValue: "5", newValue: "10"), + CellChange(columnIndex: 0, columnName: "order_id", oldValue: "1", newValue: "1"), + CellChange(columnIndex: 1, columnName: "product_id", oldValue: "42", newValue: "42"), + CellChange(columnIndex: 2, columnName: "quantity", oldValue: "5", newValue: "10"), ], originalRow: nil ) @@ -488,7 +488,7 @@ struct SQLStatementGeneratorCompositePKTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 2, columnName: "quantity", oldValue: "5", newValue: "10"), + CellChange(columnIndex: 2, columnName: "quantity", oldValue: "5", newValue: "10"), ], originalRow: nil // No originalRow, and only quantity in cellChanges — missing PK columns ) diff --git a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorGeneratedColumnTests.swift b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorGeneratedColumnTests.swift index cd4152e3c..79a45d483 100644 --- a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorGeneratedColumnTests.swift +++ b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorGeneratedColumnTests.swift @@ -54,9 +54,8 @@ struct SQLStatementGeneratorGeneratedColumnTests { rowIndex: 0, type: .insert, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: .null, newValue: "John"), + CellChange(columnIndex: 1, columnName: "name", oldValue: .null, newValue: "John"), CellChange( - rowIndex: 0, columnIndex: 2, columnName: "full_name", oldValue: .null, @@ -87,9 +86,8 @@ struct SQLStatementGeneratorGeneratedColumnTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny"), + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny"), CellChange( - rowIndex: 0, columnIndex: 2, columnName: "full_name", oldValue: "John Doe", @@ -121,7 +119,6 @@ struct SQLStatementGeneratorGeneratedColumnTests { type: .update, cellChanges: [ CellChange( - rowIndex: 0, columnIndex: 2, columnName: "full_name", oldValue: "John Doe", diff --git a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorMSSQLTests.swift b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorMSSQLTests.swift index 8b9af863e..fa493046a 100644 --- a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorMSSQLTests.swift +++ b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorMSSQLTests.swift @@ -44,7 +44,6 @@ struct SQLStatementGeneratorMSSQLTests { type: .update, cellChanges: [ CellChange( - rowIndex: rowIndex, columnIndex: 1, columnName: columnName, oldValue: PluginCellValue.fromOptional(oldValue), diff --git a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorNoPKTests.swift b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorNoPKTests.swift index a9afe0fb4..d397784a0 100644 --- a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorNoPKTests.swift +++ b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorNoPKTests.swift @@ -39,7 +39,7 @@ struct SQLStatementGeneratorNoPKTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") ], originalRow: ["1", "John", "john@example.com"] ) @@ -71,7 +71,7 @@ struct SQLStatementGeneratorNoPKTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: nil, newValue: "Johnny") + CellChange(columnIndex: 1, columnName: "name", oldValue: nil, newValue: "Johnny") ], originalRow: ["1", nil, "john@example.com"] ) @@ -98,7 +98,7 @@ struct SQLStatementGeneratorNoPKTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") ], originalRow: nil ) @@ -122,8 +122,8 @@ struct SQLStatementGeneratorNoPKTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny"), - CellChange(rowIndex: 0, columnIndex: 2, columnName: "email", oldValue: "john@example.com", newValue: "johnny@example.com") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny"), + CellChange(columnIndex: 2, columnName: "email", oldValue: "john@example.com", newValue: "johnny@example.com") ], originalRow: ["1", "John", "john@example.com"] ) @@ -229,7 +229,7 @@ struct SQLStatementGeneratorNoPKTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") ], originalRow: ["1", "John", "john@example.com"] ), diff --git a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorOrderingTests.swift b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorOrderingTests.swift new file mode 100644 index 000000000..1f2d425a6 --- /dev/null +++ b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorOrderingTests.swift @@ -0,0 +1,119 @@ +// +// SQLStatementGeneratorOrderingTests.swift +// TableProTests +// +// The generator used to emit every INSERT, then every UPDATE, then every DELETE, whatever order +// the user worked in. Deleting a row to free a unique value and adding a new row that takes it is +// a valid save that came out as a constraint violation. +// + +import Foundation +import TableProPluginKit +import Testing +@testable import TablePro + +@Suite("SQL statement ordering") +struct SQLStatementGeneratorOrderingTests { + private let columns = ["id", "email"] + + private func makeGenerator() throws -> SQLStatementGenerator { + try SQLStatementGenerator( + tableName: "users", + columns: columns, + primaryKeyColumns: ["id"], + databaseType: .sqlite, + dialect: nil + ) + } + + private func update(row: Int, sequence: Int) -> RowChange { + RowChange( + rowIndex: row, + type: .update, + cellChanges: [ + CellChange(columnIndex: 1, columnName: "email", oldValue: "old@b.com", newValue: "new@b.com"), + ], + originalRow: [.text("\(row)"), "old@b.com"], + sequence: sequence + ) + } + + private func delete(row: Int, sequence: Int) -> RowChange { + RowChange(rowIndex: row, type: .delete, originalRow: [.text("\(row)"), "a@b.com"], sequence: sequence) + } + + private func insert(row: Int, sequence: Int) -> RowChange { + RowChange(rowIndex: row, type: .insert, sequence: sequence) + } + + private func verbs(_ statements: [AttributedStatement]) -> [String] { + statements.map { String($0.statement.sql.prefix(while: { $0 != " " })) } + } + + @Test("A delete that frees a value runs before the insert that takes it") + func deleteBeforeInsert() throws { + let statements = try makeGenerator().generateAttributedStatements( + from: [delete(row: 0, sequence: 0), insert(row: 1, sequence: 1)], + insertedRowData: [1: ["9", "a@b.com"]], + deletedRowIndices: [0], + insertedRowIndices: [1] + ) + + #expect(verbs(statements) == ["DELETE", "INSERT"]) + } + + @Test("Every kind keeps the position the user gave it") + func mixedOrderIsPreserved() throws { + let statements = try makeGenerator().generateAttributedStatements( + from: [update(row: 0, sequence: 0), delete(row: 1, sequence: 1), insert(row: 2, sequence: 2)], + insertedRowData: [2: ["9", "c@b.com"]], + deletedRowIndices: [1], + insertedRowIndices: [2] + ) + + #expect(verbs(statements) == ["UPDATE", "DELETE", "INSERT"]) + } + + @Test("Order comes from the sequence, not the array, because a cancelled change is swap-removed") + func arrayOrderIsNotTrusted() throws { + let statements = try makeGenerator().generateAttributedStatements( + from: [insert(row: 2, sequence: 5), delete(row: 1, sequence: 1)], + insertedRowData: [2: ["9", "c@b.com"]], + deletedRowIndices: [1], + insertedRowIndices: [2] + ) + + #expect(verbs(statements) == ["DELETE", "INSERT"]) + } + + @Test("Consecutive deletes still batch into one statement") + func consecutiveDeletesBatch() throws { + let statements = try makeGenerator().generateAttributedStatements( + from: [delete(row: 0, sequence: 0), delete(row: 1, sequence: 1), delete(row: 2, sequence: 2)], + insertedRowData: [:], + deletedRowIndices: [0, 1, 2], + insertedRowIndices: [] + ) + + #expect(statements.count == 1) + #expect(statements.first?.rowCount == 3) + } + + @Test("A delete run interrupted by another kind splits at the interruption") + func interruptedDeleteRunSplits() throws { + let statements = try makeGenerator().generateAttributedStatements( + from: [ + delete(row: 0, sequence: 0), + update(row: 3, sequence: 1), + delete(row: 1, sequence: 2), + ], + insertedRowData: [:], + deletedRowIndices: [0, 1], + insertedRowIndices: [] + ) + + #expect(verbs(statements) == ["DELETE", "UPDATE", "DELETE"]) + #expect(statements.first?.rowCount == 1) + #expect(statements.last?.rowCount == 1) + } +} diff --git a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift index b90823067..945db12ca 100644 --- a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift +++ b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift @@ -47,7 +47,7 @@ struct SQLStatementGeneratorPKRegressionTests { RowChange( rowIndex: rowIndex, type: .update, - cellChanges: [CellChange(rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, oldValue: PluginCellValue.fromOptional(oldValue), newValue: PluginCellValue.fromOptional(newValue))], + cellChanges: [CellChange(columnIndex: columnIndex, columnName: columnName, oldValue: PluginCellValue.fromOptional(oldValue), newValue: PluginCellValue.fromOptional(newValue))], originalRow: originalRow.map(PluginCellValue.fromOptional) ) } diff --git a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorParameterStyleTests.swift b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorParameterStyleTests.swift index 54ac2d805..559ccab7c 100644 --- a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorParameterStyleTests.swift +++ b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorParameterStyleTests.swift @@ -191,7 +191,7 @@ struct SQLStatementGeneratorParameterStyleTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Jane") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Jane") ], originalRow: ["1", "John", "john@example.com"] ) diff --git a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorTests.swift b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorTests.swift index 523799114..b73e4ee56 100644 --- a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorTests.swift +++ b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorTests.swift @@ -143,9 +143,9 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .insert, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 0, columnName: "id", oldValue: nil, newValue: "1"), - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: nil, newValue: "John"), - CellChange(rowIndex: 0, columnIndex: 2, columnName: "email", oldValue: nil, newValue: "john@example.com") + CellChange(columnIndex: 0, columnName: "id", oldValue: nil, newValue: "1"), + CellChange(columnIndex: 1, columnName: "name", oldValue: nil, newValue: "John"), + CellChange(columnIndex: 2, columnName: "email", oldValue: nil, newValue: "john@example.com") ], originalRow: nil ) @@ -287,7 +287,7 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") ], originalRow: ["1", "John", "john@example.com"] ) @@ -320,8 +320,8 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny"), - CellChange(rowIndex: 0, columnIndex: 2, columnName: "email", oldValue: "john@example.com", newValue: "johnny@example.com") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny"), + CellChange(columnIndex: 2, columnName: "email", oldValue: "john@example.com", newValue: "johnny@example.com") ], originalRow: ["1", "John", "john@example.com"] ) @@ -349,7 +349,7 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 2, columnName: "email", oldValue: "john@example.com", newValue: nil) + CellChange(columnIndex: 2, columnName: "email", oldValue: "john@example.com", newValue: nil) ], originalRow: ["1", "John", "john@example.com"] ) @@ -374,7 +374,7 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "__DEFAULT__") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "__DEFAULT__") ], originalRow: ["1", "John", "john@example.com"] ) @@ -401,7 +401,7 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 2, columnName: "email", oldValue: "old@example.com", newValue: "CURRENT_TIMESTAMP()") + CellChange(columnIndex: 2, columnName: "email", oldValue: "old@example.com", newValue: "CURRENT_TIMESTAMP()") ], originalRow: ["1", "John", "old@example.com"] ) @@ -428,7 +428,7 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") ], originalRow: ["1", "John", "john@example.com"] ) @@ -455,7 +455,7 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") ], originalRow: ["42", "John", "john@example.com"] ) @@ -637,7 +637,7 @@ struct SQLStatementGeneratorTests { rowIndex: 1, type: .update, cellChanges: [ - CellChange(rowIndex: 1, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") ], originalRow: ["1", "John", "john@example.com"] ), @@ -844,7 +844,7 @@ struct SQLStatementGeneratorTests { rowIndex: 1, type: .update, cellChanges: [ - CellChange(rowIndex: 1, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") ], originalRow: ["1", "John", "john@example.com"] ), @@ -875,8 +875,8 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny"), - CellChange(rowIndex: 0, columnIndex: 2, columnName: "email", oldValue: "john@example.com", newValue: "johnny@example.com") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny"), + CellChange(columnIndex: 2, columnName: "email", oldValue: "john@example.com", newValue: "johnny@example.com") ], originalRow: ["1", "John", "john@example.com"] ) @@ -966,7 +966,7 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny") ], originalRow: ["1", "John", "john@example.com"] ) @@ -1041,8 +1041,8 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny"), - CellChange(rowIndex: 0, columnIndex: 2, columnName: "email", oldValue: "john@example.com", newValue: "johnny@example.com") + CellChange(columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Johnny"), + CellChange(columnIndex: 2, columnName: "email", oldValue: "john@example.com", newValue: "johnny@example.com") ], originalRow: ["1", "John", "john@example.com"] ) @@ -1081,7 +1081,7 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "database", oldValue: "old_db", newValue: "new_db") + CellChange(columnIndex: 1, columnName: "database", oldValue: "old_db", newValue: "new_db") ], originalRow: ["1", "old_db", "users", "5"] ) @@ -1172,7 +1172,7 @@ struct SQLStatementGeneratorTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: 1, columnName: "database", oldValue: "old_db", newValue: "new_db") + CellChange(columnIndex: 1, columnName: "database", oldValue: "old_db", newValue: "new_db") ], originalRow: ["1", "old_db", "5"] ) diff --git a/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift b/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift index 6f3220035..4ead5da55 100644 --- a/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift +++ b/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift @@ -8,17 +8,36 @@ import TableProPluginKit @testable import TablePro import Testing -/// Reports whatever affected-row count the test asks for, and remembers whether it was rolled back. +/// One test double for every case in this file: it records the order of everything it was asked to +/// do, reports whatever affected-row count the test wants, and can be told to fail on the Nth +/// statement. private final class CountingDriver: PluginDatabaseDriver, @unchecked Sendable { + struct Failure: Error {} + let affectedRows: Int let transactional: Bool - private(set) var executed: [String] = [] - private(set) var didCommit = false - private(set) var didRollBack = false + /// One-based index of the statement that should throw, counting only the ones the plan runs. + let failOnStatement: Int? + let rollbackFails: Bool + + /// Every call in the order it arrived: statement text, plus "BEGIN", "COMMIT" and "ROLLBACK". + private(set) var trace: [String] = [] + private var statementCount = 0 - init(affectedRows: Int, transactional: Bool = true) { + var executed: [String] { trace.filter { !["BEGIN", "COMMIT", "ROLLBACK"].contains($0) } } + var didCommit: Bool { trace.contains("COMMIT") } + var didRollBack: Bool { trace.contains("ROLLBACK") } + + init( + affectedRows: Int, + transactional: Bool = true, + failOnStatement: Int? = nil, + rollbackFails: Bool = false + ) { self.affectedRows = affectedRows self.transactional = transactional + self.failOnStatement = failOnStatement + self.rollbackFails = rollbackFails } var supportsSchemas: Bool { false } @@ -31,15 +50,20 @@ private final class CountingDriver: PluginDatabaseDriver, @unchecked Sendable { func ping() async throws {} func execute(query: String) async throws -> PluginQueryResult { - executed.append(query) + trace.append(query) + statementCount += 1 + if let failOnStatement, statementCount == failOnStatement { throw Failure() } return PluginQueryResult( columns: [], columnTypeNames: [], rows: [], rowsAffected: affectedRows, executionTime: 0 ) } - func beginTransaction(mode: PluginTransactionAccessMode) async throws {} - func commitTransaction() async throws { didCommit = true } - func rollbackTransaction() async throws { didRollBack = true } + func beginTransaction(mode: PluginTransactionAccessMode) async throws { trace.append("BEGIN") } + func commitTransaction() async throws { trace.append("COMMIT") } + func rollbackTransaction() async throws { + trace.append("ROLLBACK") + if rollbackFails { throw Failure() } + } func fetchTables(schema: String?) async throws -> [PluginTableInfo] { [] } func fetchColumns(table: String, schema: String?) async throws -> [PluginColumnInfo] { [] } @@ -59,18 +83,25 @@ private final class CountingDriver: PluginDatabaseDriver, @unchecked Sendable { @Suite("Data write execution") struct DataWriteExecutorTests { - private func plan(expectedRowCount: Int?) -> DataWritePlan { + private func plan( + expectedRowCount: Int?, + statementCount: Int = 1, + prologue: [String] = [], + epilogue: [String] = [] + ) -> DataWritePlan { DataWritePlan( scope: DatabaseScope(connectionId: UUID(), database: "shop", schema: nil), databaseType: .sqlite, - steps: [ + steps: (0 ..< statementCount).map { index in DataWriteStep( kind: .rowWrite, - statement: ParameterizedStatement(sql: "UPDATE \"t\" SET \"b\" = ? WHERE \"a\" = ?", parameters: ["x", "1"]), + statement: ParameterizedStatement(sql: "UPDATE \"t\" SET \"b\" = \(index)", parameters: []), expectedRowCount: expectedRowCount, tableName: "t" - ), - ] + ) + }, + prologue: prologue, + epilogue: epilogue ) } @@ -81,7 +112,7 @@ struct DataWriteExecutorTests { @Test("The driver's real affected-row count reaches the caller") func reportsRealRowCount() async throws { let counting = CountingDriver(affectedRows: 42) - let results = try await DataWriteExecutor.run(plan(expectedRowCount: 50), on: driver(counting)) + let results = try await DataWriteExecutor.run(plan(expectedRowCount: 50), on: driver(counting)).results #expect(results.first?.rowsAffected == 42) #expect(counting.didCommit) @@ -111,17 +142,114 @@ struct DataWriteExecutorTests { @Test("Fewer rows than expected is an ordinary save, because MySQL reports zero for an unchanged value") func fewerRowsIsNotAFailure() async throws { let counting = CountingDriver(affectedRows: 0) - let results = try await DataWriteExecutor.run(plan(expectedRowCount: 1), on: driver(counting)) + let results = try await DataWriteExecutor.run(plan(expectedRowCount: 1), on: driver(counting)).results #expect(results.first?.rowsAffected == 0) #expect(counting.didCommit) #expect(counting.didRollBack == false) } + @Test("The foreign-key disable runs before the transaction, and the re-enable after it") + func togglesRunOutsideTheTransaction() async throws { + let counting = CountingDriver(affectedRows: 1) + _ = try await DataWriteExecutor.run( + plan( + expectedRowCount: 1, + prologue: ["PRAGMA foreign_keys = OFF"], + epilogue: ["PRAGMA foreign_keys = ON"] + ), + on: driver(counting) + ) + + #expect(counting.trace == [ + "PRAGMA foreign_keys = OFF", + "BEGIN", + "UPDATE \"t\" SET \"b\" = 0", + "COMMIT", + "PRAGMA foreign_keys = ON", + ]) + } + + @Test("Foreign keys are re-enabled after a rollback too") + func togglesRunAfterRollback() async throws { + let counting = CountingDriver(affectedRows: 1, failOnStatement: 2) + await #expect(throws: (any Error).self) { + try await DataWriteExecutor.run( + plan( + expectedRowCount: 1, + prologue: ["PRAGMA foreign_keys = OFF"], + epilogue: ["PRAGMA foreign_keys = ON"] + ), + on: driver(counting) + ) + } + + #expect(counting.didRollBack) + #expect(counting.trace.last == "PRAGMA foreign_keys = ON") + } + + @Test("The statements the run executed around the transaction are reported back") + func sideStatementsAreReported() async throws { + let counting = CountingDriver(affectedRows: 1) + let run = try await DataWriteExecutor.run( + plan(expectedRowCount: 1, prologue: ["A"], epilogue: ["B"]), + on: driver(counting) + ) + + #expect(run.sideStatements == ["A", "B"]) + } + + @Test("Without transactions a failure halfway reports what already committed") + func partialCommitCarriesTheCommittedResults() async throws { + let counting = CountingDriver(affectedRows: 1, transactional: false, failOnStatement: 2) + + do { + _ = try await DataWriteExecutor.run( + plan(expectedRowCount: 1, statementCount: 3), on: driver(counting) + ) + Issue.record("expected the run to throw") + } catch let error as DataWritePartialCommitError { + #expect(error.committed.count == 1) + #expect(error.totalStatements == 3) + } + } + + @Test("With transactions a failure halfway is an ordinary rollback, not a partial commit") + func transactionalFailureIsNotPartial() async throws { + let counting = CountingDriver(affectedRows: 1, failOnStatement: 2) + + do { + _ = try await DataWriteExecutor.run( + plan(expectedRowCount: 1, statementCount: 3), on: driver(counting) + ) + Issue.record("expected the run to throw") + } catch is DataWritePartialCommitError { + Issue.record("a rolled-back transaction left nothing committed") + } catch { + #expect(counting.didRollBack) + } + } + + /// A rollback that fails leaves the committed extent unknown, which is the same problem as + /// having no transaction at all. + @Test("A failed rollback is reported as a partial commit") + func failedRollbackIsPartial() async throws { + let counting = CountingDriver(affectedRows: 1, failOnStatement: 2, rollbackFails: true) + + do { + _ = try await DataWriteExecutor.run( + plan(expectedRowCount: 1, statementCount: 3), on: driver(counting) + ) + Issue.record("expected the run to throw") + } catch let error as DataWritePartialCommitError { + #expect(error.committed.count == 1) + } + } + @Test("A statement whose row count carries no meaning is not held to one") func unverifiedStepIsNotChecked() async throws { let counting = CountingDriver(affectedRows: 9_999) - let results = try await DataWriteExecutor.run(plan(expectedRowCount: nil), on: driver(counting)) + let results = try await DataWriteExecutor.run(plan(expectedRowCount: nil), on: driver(counting)).results #expect(results.first?.wasVerified == false) #expect(counting.didCommit) diff --git a/TableProTests/Core/DataWrite/RowWriteOperationBuilderTests.swift b/TableProTests/Core/DataWrite/RowWriteOperationBuilderTests.swift index 1796a9829..34a2ae156 100644 --- a/TableProTests/Core/DataWrite/RowWriteOperationBuilderTests.swift +++ b/TableProTests/Core/DataWrite/RowWriteOperationBuilderTests.swift @@ -40,7 +40,7 @@ struct RowWriteOperationBuilderTests { rowIndex: 0, type: .update, cellChanges: [ - CellChange(rowIndex: 0, columnIndex: index, columnName: column, oldValue: old, newValue: new), + CellChange(columnIndex: index, columnName: column, oldValue: old, newValue: new), ], originalRow: ["7", "Ada", "2026-01-01"] ) diff --git a/TableProTests/Core/Services/UndoRowIndexTests.swift b/TableProTests/Core/Services/UndoRowIndexTests.swift new file mode 100644 index 000000000..2aad3ea2f --- /dev/null +++ b/TableProTests/Core/Services/UndoRowIndexTests.swift @@ -0,0 +1,106 @@ +// +// UndoRowIndexTests.swift +// TableProTests +// +// A cell edit is tracked by its display row, the same as the modified marks the grid draws, but +// TableRows is indexed by storage position. Undo used to write straight through with the display +// number, so with a value filter narrowing the grid it reverted whichever row happened to sit at +// that storage offset. +// + +import Foundation +import TableProPluginKit +@testable import TablePro +import Testing + +@MainActor +@Suite("Undo row indices") +struct UndoRowIndexTests { + private static let columns = ["id", "name"] + + private func makeTableRows() -> TableRows { + TableRows.from( + queryRows: [ + ["1", "keep"], + ["2", "drop"], + ["3", "keep"], + ].map { $0.map { PluginCellValue.text($0) } }, + columns: Self.columns, + columnTypes: Array(repeating: .text(rawType: nil), count: 2) + ) + } + + private func makeManager() -> RowOperationsManager { + let changeManager = DataChangeManager() + changeManager.configureForTable( + tableName: "users", + columns: Self.columns, + primaryKeyColumns: ["id"], + databaseType: .sqlite + ) + return RowOperationsManager(changeManager: changeManager) + } + + private func cellEditUndo(displayRow: Int, previous: PluginCellValue) -> UndoResult { + UndoResult( + action: .cellEdit( + rowIndex: displayRow, + columnIndex: 1, + columnName: "name", + previousValue: previous, + newValue: "edited", + originalRow: nil + ), + needsRowRemoval: false, + needsRowRestore: false, + restoreRow: nil + ) + } + + /// Display 1 is storage 2 once the middle row is filtered out. + @Test("Undoing a cell edit under a value filter reverts the row that was edited") + func undoResolvesThroughTheFilter() { + var tableRows = makeTableRows() + let displayIDs = [tableRows.rows[0].id, tableRows.rows[2].id] + tableRows.rows[2].values[1] = "edited" + + _ = makeManager().applyUndoResult( + cellEditUndo(displayRow: 1, previous: "keep"), + displayIDs: displayIDs, + tableRows: &tableRows + ) + + #expect(tableRows.rows[2].values[1] == "keep") + #expect(tableRows.rows[1].values[1] == "drop") + } + + @Test("With no filter the display row is the storage row, and nothing changes") + func undoWithoutFilterIsUnchanged() { + var tableRows = makeTableRows() + tableRows.rows[1].values[1] = "edited" + + _ = makeManager().applyUndoResult( + cellEditUndo(displayRow: 1, previous: "drop"), + displayIDs: nil, + tableRows: &tableRows + ) + + #expect(tableRows.rows[1].values[1] == "drop") + } + + @Test("A display row the filter no longer shows reverts nothing rather than the wrong row") + func undoForAHiddenRowIsANoOp() { + var tableRows = makeTableRows() + let displayIDs = [tableRows.rows[0].id] + let before = tableRows.rows.map { $0.values } + + let result = makeManager().applyUndoResult( + cellEditUndo(displayRow: 5, previous: "keep"), + displayIDs: displayIDs, + tableRows: &tableRows + ) + + #expect(result.delta == .none) + #expect(tableRows.rows.map { $0.values } == before) + } +} diff --git a/TableProTests/Helpers/TestFixtures.swift b/TableProTests/Helpers/TestFixtures.swift index 88b64edb1..a4c861034 100644 --- a/TableProTests/Helpers/TestFixtures.swift +++ b/TableProTests/Helpers/TestFixtures.swift @@ -58,7 +58,6 @@ enum TestFixtures { new: String? = "value" ) -> CellChange { return CellChange( - rowIndex: row, columnIndex: col, columnName: colName, oldValue: PluginCellValue.fromOptional(old),