diff --git a/TablePro/Core/DataWrite/RowChangeStatementFactory.swift b/TablePro/Core/DataWrite/RowChangeStatementFactory.swift index 989c745ca4..6ac516dfbe 100644 --- a/TablePro/Core/DataWrite/RowChangeStatementFactory.swift +++ b/TablePro/Core/DataWrite/RowChangeStatementFactory.swift @@ -124,6 +124,7 @@ struct RowChangeStatementFactory { ) { return .driverWritten(driverStatements) } + try refuseMatchesReachingOtherRows(in: changes, deletedRowIDs: deletedRowIDs) return .counted( try hostStatements( for: changes, @@ -134,6 +135,44 @@ struct RowChangeStatementFactory { ) } + // These engines write their own rows, yet their saves reach the host's UPDATE and DELETE, keyed on a key the + // engine does not keep unique, with no row count to hold them to (#3776). Dameng enforces its primary key. + private static let enginesWithoutUniqueRowMatch: Set = [.clickhouse, .snowflake, .trino] + + private func refuseMatchesReachingOtherRows(in changes: [RowChange], deletedRowIDs: Set) throws { + let reason: String + if Self.enginesWithoutUniqueRowMatch.contains(databaseType) { + reason = String( + format: String( + localized: "This version of TablePro cannot write %@ rows from the grid. Use a SQL statement instead." + ), + databaseType.displayName + ) + } else if databaseType == .dameng, primaryKeyColumns.isEmpty { + reason = String( + format: String( + localized: """ + This version of TablePro cannot write rows of a %@ table without a primary key from the grid. \ + Use a SQL statement instead. + """ + ), + databaseType.displayName + ) + } else { + return + } + for change in changes { + switch change.type { + case .update: + throw DataWriteError.changeRefused(table: tableName, kind: .update, reason: reason) + case .delete where deletedRowIDs.contains(change.rowID): + throw DataWriteError.changeRefused(table: tableName, kind: .delete, reason: reason) + case .insert, .delete: + continue + } + } + } + /// A value staged for a column the server owns can never be written, however it got staged: the grid offers no /// editor for one, but the set of such columns is empty until the table's schema arrives, and an edit made in /// that window survives its arrival. Sending it fails on the server (SQL Server Msg 8102 for an `IDENTITY`), and a diff --git a/TableProTests/Core/ChangeTracking/DataChangeManagerClickHouseTests.swift b/TableProTests/Core/ChangeTracking/DataChangeManagerClickHouseTests.swift index 8b93720f45..da43c979a9 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeManagerClickHouseTests.swift +++ b/TableProTests/Core/ChangeTracking/DataChangeManagerClickHouseTests.swift @@ -2,9 +2,6 @@ // DataChangeManagerClickHouseTests.swift // TableProTests // -// Tests for ClickHouse-specific UPDATE statement validation in DataChangeManager. -// ClickHouse uses ALTER TABLE ... UPDATE syntax instead of standard UPDATE. -// import Foundation import TableProPluginKit @@ -13,8 +10,8 @@ import Testing @MainActor struct DataChangeManagerClickHouseTests { - @Test("ClickHouse ALTER TABLE UPDATE passes validation without throwing") - func alterTableUpdatePassesValidation() async { + @Test("A ClickHouse edit is refused and stays pending instead of keyed on the sorting key") + func clickHouseEditIsRefusedAndStaysPending() async { let manager = DataChangeManager() manager.configureForTable( tableName: "events", @@ -33,10 +30,10 @@ struct DataChangeManagerClickHouseTests { originalRow: ["42", "pending"] ) - // Should not throw — ALTER TABLE UPDATE must be recognized as valid - #expect(throws: Never.self) { + #expect(throws: DataWriteError.self) { _ = try manager.generateSQL() } + #expect(manager.hasChanges) } @Test("Standard UPDATE prefix is still detected for non-ClickHouse databases") diff --git a/TableProTests/Core/DataWrite/RowChangeStatementFactoryUniqueMatchTests.swift b/TableProTests/Core/DataWrite/RowChangeStatementFactoryUniqueMatchTests.swift new file mode 100644 index 0000000000..895581e9d4 --- /dev/null +++ b/TableProTests/Core/DataWrite/RowChangeStatementFactoryUniqueMatchTests.swift @@ -0,0 +1,140 @@ +// +// RowChangeStatementFactoryUniqueMatchTests.swift +// TableProTests +// + +import Foundation +@testable import TablePro +import TableProPluginKit +import Testing + +@MainActor +struct RowChangeStatementFactoryUniqueMatchTests { + private let columns = ["d", "id", "v"] + private let row: [PluginCellValue] = ["2026-10-22", "1", "a"] + + private func factory(_ databaseType: DatabaseType, primaryKeyColumns: [String] = ["d"]) -> RowChangeStatementFactory { + RowChangeStatementFactory( + tableName: "t", + schemaName: nil, + columns: columns, + primaryKeyColumns: primaryKeyColumns, + databaseType: databaseType, + pluginDriver: nil + ) + } + + private var edit: RowChange { + RowChange( + rowID: .existing(0), + type: .update, + cellChanges: [CellChange(columnIndex: 2, columnName: "v", oldValue: "a", newValue: "b")], + originalRow: row + ) + } + + private var delete: RowChange { + RowChange(rowID: .existing(0), type: .delete, originalRow: row) + } + + private func refusal(_ write: () throws -> [ParameterizedStatement]) -> (kind: RowWriteKind?, reason: String)? { + do { + let statements = try write() + Issue.record("Sent \(statements.map(\.sql))") + return nil + } catch let DataWriteError.changeRefused(table, kind, reason) { + #expect(table == "t") + return (kind, reason) + } catch { + Issue.record("Unexpected error: \(error)") + return nil + } + } + + @Test(arguments: [DatabaseType.clickhouse, .snowflake, .trino]) + func aDeleteIsRefusedInsteadOfKeyedOnAKeyTheEngineDoesNotKeepUnique(databaseType: DatabaseType) { + let refused = refusal { + try factory(databaseType).statements(for: [delete], deletedRowIDs: [.existing(0)]) + } + #expect(refused?.kind == .delete) + #expect(refused?.reason.contains(databaseType.displayName) == true) + #expect(refused?.reason.contains("SQL statement") == true) + } + + @Test(arguments: [DatabaseType.clickhouse, .snowflake, .trino]) + func anEditIsRefusedInsteadOfKeyedOnAKeyTheEngineDoesNotKeepUnique(databaseType: DatabaseType) { + let refused = refusal { try factory(databaseType).statements(for: [edit]) } + #expect(refused?.kind == .update) + } + + @Test + func aNewClickHouseRowIsStillInserted() throws { + let inserted = RowID.inserted(UUID()) + let statements = try factory(.clickhouse).statements( + for: [RowChange(rowID: inserted, type: .insert)], + insertedRowData: [inserted: row], + insertedRowIDs: [inserted] + ) + #expect(statements.count == 1) + #expect(statements.first?.sql.hasPrefix("INSERT") == true) + } + + @Test + func aDamengTableWithAPrimaryKeyIsStillWritten() throws { + let deleted = RowChange(rowID: .existing(1), type: .delete, originalRow: ["2026-10-22", "2", "a"]) + let statements = try factory(.dameng, primaryKeyColumns: ["id"]).statements( + for: [edit, deleted], + deletedRowIDs: [.existing(1)] + ) + #expect(Set(statements.map { String($0.sql.prefix(6)) }) == ["UPDATE", "DELETE"]) + } + + @Test + func aDamengTableWithoutAPrimaryKeyRefusesADelete() { + let refused = refusal { + try factory(.dameng, primaryKeyColumns: []).statements(for: [delete], deletedRowIDs: [.existing(0)]) + } + #expect(refused?.kind == .delete) + #expect(refused?.reason.contains("primary key") == true) + } + + @Test + func rewindingAClickHouseInsertIsRefused() { + let target = DataWriteTarget(database: "default", schema: nil, table: "t") + let record = RewindRecord( + id: UUID(), + historyId: nil, + connectionId: UUID(), + databaseType: .clickhouse, + target: target, + capturedAt: Date(timeIntervalSince1970: 0), + generatedColumns: [], + identityColumns: [], + operations: [ + RowWriteOperation( + kind: .insert, + target: target, + columns: columns, + primaryKeyColumns: ["d"], + preImage: nil, + postImage: row, + writtenColumns: columns, + refusal: nil + ), + ] + ) + let planner = RewindPlanner( + record: record, + factory: factory(.clickhouse), + queryBuilder: TableQueryBuilder( + databaseType: .clickhouse, + dialect: PluginManager.shared.sqlDialect(for: .clickhouse), + pagination: .offset + ) + ) + + #expect(throws: DataWriteError.self) { + try planner.plan(currentRows: [RewindCurrentRow(values: row)]) + } + } +} diff --git a/changelog.d/3794.changed.md b/changelog.d/3794.changed.md new file mode 100644 index 0000000000..6c25371a2b --- /dev/null +++ b/changelog.d/3794.changed.md @@ -0,0 +1 @@ +The grid no longer edits or deletes ClickHouse, Snowflake, Trino or keyless Dameng rows; use a SQL statement instead. diff --git a/changelog.d/3794.fixed.md b/changelog.d/3794.fixed.md new file mode 100644 index 0000000000..9cf261e79e --- /dev/null +++ b/changelog.d/3794.fixed.md @@ -0,0 +1 @@ +A grid edit or delete on ClickHouse, Snowflake, Trino or a keyless Dameng table could change other rows too.