diff --git a/CHANGELOG.md b/CHANGELOG.md index e6975574..7547e591 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- DELETE and UPDATE queries using all columns in WHERE clause instead of just the primary key for PostgreSQL, Redshift, MSSQL, and ClickHouse - SSL/TLS always being enabled for MongoDB, Redis, and ClickHouse connections due to case mismatch in SSL mode string comparison (#249) - Redis sidebar click showing data briefly then going empty due to double-navigation race condition (#251) - MongoDB showing "Invalid database name: ''" when connecting without a database name diff --git a/Plugins/ClickHouseDriverPlugin/ClickHousePlugin.swift b/Plugins/ClickHouseDriverPlugin/ClickHousePlugin.swift index 99d61c9c..5b4f9232 100644 --- a/Plugins/ClickHouseDriverPlugin/ClickHousePlugin.swift +++ b/Plugins/ClickHouseDriverPlugin/ClickHousePlugin.swift @@ -233,6 +233,25 @@ final class ClickHousePluginDriver: PluginDatabaseDriver, @unchecked Sendable { } func fetchAllColumns(schema: String?) async throws -> [String: [PluginColumnInfo]] { + // Pre-fetch PK columns for all tables. Falls back to sorting_key when + // primary_key is empty (MergeTree without explicit PRIMARY KEY clause). + // Note: expression-based keys like toDate(col) won't match bare column names. + let pkSql = """ + SELECT name, primary_key, sorting_key FROM system.tables + WHERE database = currentDatabase() + """ + let pkResult = try await execute(query: pkSql) + var pkLookup: [String: Set] = [:] + for row in pkResult.rows { + guard let tableName = row[safe: 0] ?? nil else { continue } + let primaryKey = (row[safe: 1] ?? nil) ?? "" + let sortingKey = (row[safe: 2] ?? nil) ?? "" + let keyString = primaryKey.isEmpty ? sortingKey : primaryKey + guard !keyString.isEmpty else { continue } + let cols = Set(keyString.split(separator: ",").map { String($0).trimmingCharacters(in: .whitespaces) }) + pkLookup[tableName] = cols + } + let sql = """ SELECT table, name, type, default_kind, default_expression, comment FROM system.columns @@ -265,7 +284,7 @@ final class ClickHousePluginDriver: PluginDatabaseDriver, @unchecked Sendable { name: colName, dataType: dataType, isNullable: isNullable, - isPrimaryKey: false, + isPrimaryKey: pkLookup[tableName]?.contains(colName) == true, defaultValue: defaultValue, extra: extra, comment: (comment?.isEmpty == false) ? comment : nil diff --git a/Plugins/MSSQLDriverPlugin/MSSQLPlugin.swift b/Plugins/MSSQLDriverPlugin/MSSQLPlugin.swift index 1a4ea406..fca8c35c 100644 --- a/Plugins/MSSQLDriverPlugin/MSSQLPlugin.swift +++ b/Plugins/MSSQLDriverPlugin/MSSQLPlugin.swift @@ -461,18 +461,29 @@ final class MSSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { let esc = effectiveSchemaEscaped(schema) let sql = """ SELECT - COLUMN_NAME, - DATA_TYPE, - CHARACTER_MAXIMUM_LENGTH, - NUMERIC_PRECISION, - NUMERIC_SCALE, - IS_NULLABLE, - COLUMN_DEFAULT, - COLUMNPROPERTY(OBJECT_ID(TABLE_SCHEMA + '.' + TABLE_NAME), COLUMN_NAME, 'IsIdentity') AS IS_IDENTITY - FROM INFORMATION_SCHEMA.COLUMNS - WHERE TABLE_NAME = '\(escapedTable)' - AND TABLE_SCHEMA = '\(esc)' - ORDER BY ORDINAL_POSITION + c.COLUMN_NAME, + c.DATA_TYPE, + c.CHARACTER_MAXIMUM_LENGTH, + c.NUMERIC_PRECISION, + c.NUMERIC_SCALE, + c.IS_NULLABLE, + c.COLUMN_DEFAULT, + COLUMNPROPERTY(OBJECT_ID(c.TABLE_SCHEMA + '.' + c.TABLE_NAME), c.COLUMN_NAME, 'IsIdentity') AS IS_IDENTITY, + CASE WHEN pk.COLUMN_NAME IS NOT NULL THEN 1 ELSE 0 END AS IS_PK + FROM INFORMATION_SCHEMA.COLUMNS c + LEFT JOIN ( + SELECT kcu.COLUMN_NAME + FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS tc + JOIN INFORMATION_SCHEMA.KEY_COLUMN_USAGE kcu + ON tc.CONSTRAINT_NAME = kcu.CONSTRAINT_NAME + AND tc.TABLE_SCHEMA = kcu.TABLE_SCHEMA + WHERE tc.CONSTRAINT_TYPE = 'PRIMARY KEY' + AND tc.TABLE_SCHEMA = '\(esc)' + AND tc.TABLE_NAME = '\(escapedTable)' + ) pk ON c.COLUMN_NAME = pk.COLUMN_NAME + WHERE c.TABLE_NAME = '\(escapedTable)' + AND c.TABLE_SCHEMA = '\(esc)' + ORDER BY c.ORDINAL_POSITION """ let result = try await execute(query: sql) return result.rows.compactMap { row -> PluginColumnInfo? in @@ -484,6 +495,7 @@ final class MSSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { let isNullable = (row[safe: 5] ?? nil) == "YES" let defaultValue = row[safe: 6] ?? nil let isIdentity = (row[safe: 7] ?? nil) == "1" + let isPk = (row[safe: 8] ?? nil) == "1" let baseType = (dataType ?? "nvarchar").lowercased() let fixedSizeTypes: Set = [ @@ -509,7 +521,7 @@ final class MSSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { name: name, dataType: fullType, isNullable: isNullable, - isPrimaryKey: false, + isPrimaryKey: isPk, defaultValue: defaultValue, extra: isIdentity ? "IDENTITY" : nil ) diff --git a/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift b/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift index 9972265f..02c448ea 100644 --- a/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift +++ b/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift @@ -184,7 +184,8 @@ final class PostgreSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { c.column_default, c.collation_name, pgd.description, - c.udt_name + c.udt_name, + CASE WHEN pk.column_name IS NOT NULL THEN 'YES' ELSE 'NO' END AS is_pk FROM information_schema.columns c LEFT JOIN pg_catalog.pg_statio_all_tables st ON st.schemaname = c.table_schema @@ -192,6 +193,16 @@ final class PostgreSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { LEFT JOIN pg_catalog.pg_description pgd ON pgd.objoid = st.relid AND pgd.objsubid = c.ordinal_position + LEFT JOIN ( + SELECT DISTINCT kcu.column_name + FROM information_schema.table_constraints tc + JOIN information_schema.key_column_usage kcu + ON tc.constraint_name = kcu.constraint_name + AND tc.table_schema = kcu.table_schema + WHERE tc.constraint_type = 'PRIMARY KEY' + AND tc.table_schema = '\(escapedSchema)' + AND tc.table_name = '\(escapeLiteral(table))' + ) pk ON c.column_name = pk.column_name WHERE c.table_schema = '\(escapedSchema)' AND c.table_name = '\(escapeLiteral(table))' ORDER BY c.ordinal_position """ @@ -214,6 +225,7 @@ final class PostgreSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { let defaultValue = row[3] let collation = row.count > 4 ? row[4] : nil let comment = row.count > 5 ? row[5] : nil + let isPk = row.count > 7 && row[7] == "YES" let charset: String? = { guard let coll = collation else { return nil } @@ -227,7 +239,7 @@ final class PostgreSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { name: name, dataType: dataType, isNullable: isNullable, - isPrimaryKey: false, + isPrimaryKey: isPk, defaultValue: defaultValue, charset: charset, collation: collation, @@ -246,7 +258,8 @@ final class PostgreSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { c.column_default, c.collation_name, pgd.description, - c.udt_name + c.udt_name, + CASE WHEN pk.column_name IS NOT NULL THEN 'YES' ELSE 'NO' END AS is_pk FROM information_schema.columns c LEFT JOIN pg_catalog.pg_statio_all_tables st ON st.schemaname = c.table_schema @@ -254,6 +267,15 @@ final class PostgreSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { LEFT JOIN pg_catalog.pg_description pgd ON pgd.objoid = st.relid AND pgd.objsubid = c.ordinal_position + LEFT JOIN ( + SELECT DISTINCT kcu.table_name, kcu.column_name + FROM information_schema.table_constraints tc + JOIN information_schema.key_column_usage kcu + ON tc.constraint_name = kcu.constraint_name + AND tc.table_schema = kcu.table_schema + WHERE tc.constraint_type = 'PRIMARY KEY' + AND tc.table_schema = '\(escapedSchema)' + ) pk ON c.table_name = pk.table_name AND c.column_name = pk.column_name WHERE c.table_schema = '\(escapedSchema)' ORDER BY c.table_name, c.ordinal_position """ @@ -278,6 +300,7 @@ final class PostgreSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { let defaultValue = row[4] let collation = row.count > 5 ? row[5] : nil let comment = row.count > 6 ? row[6] : nil + let isPk = row.count > 8 && row[8] == "YES" let charset: String? = { guard let coll = collation else { return nil } @@ -291,7 +314,7 @@ final class PostgreSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { name: name, dataType: dataType, isNullable: isNullable, - isPrimaryKey: false, + isPrimaryKey: isPk, defaultValue: defaultValue, charset: charset, collation: collation, diff --git a/Plugins/PostgreSQLDriverPlugin/RedshiftPluginDriver.swift b/Plugins/PostgreSQLDriverPlugin/RedshiftPluginDriver.swift index dfbbdafa..c85e4c70 100644 --- a/Plugins/PostgreSQLDriverPlugin/RedshiftPluginDriver.swift +++ b/Plugins/PostgreSQLDriverPlugin/RedshiftPluginDriver.swift @@ -184,7 +184,8 @@ final class RedshiftPluginDriver: PluginDatabaseDriver, @unchecked Sendable { c.column_default, c.collation_name, pgd.description, - c.udt_name + c.udt_name, + CASE WHEN pk.column_name IS NOT NULL THEN 'YES' ELSE 'NO' END AS is_pk FROM information_schema.columns c LEFT JOIN pg_catalog.pg_class cls ON cls.relname = c.table_name @@ -192,6 +193,16 @@ final class RedshiftPluginDriver: PluginDatabaseDriver, @unchecked Sendable { LEFT JOIN pg_catalog.pg_description pgd ON pgd.objoid = cls.oid AND pgd.objsubid = c.ordinal_position + LEFT JOIN ( + SELECT DISTINCT kcu.column_name + FROM information_schema.table_constraints tc + JOIN information_schema.key_column_usage kcu + ON tc.constraint_name = kcu.constraint_name + AND tc.table_schema = kcu.table_schema + WHERE tc.constraint_type = 'PRIMARY KEY' + AND tc.table_schema = '\(escapedSchema)' + AND tc.table_name = '\(safeTable)' + ) pk ON c.column_name = pk.column_name WHERE c.table_schema = '\(escapedSchema)' AND c.table_name = '\(safeTable)' ORDER BY c.ordinal_position """ @@ -214,6 +225,7 @@ final class RedshiftPluginDriver: PluginDatabaseDriver, @unchecked Sendable { let defaultValue = row[3] let collation = row.count > 4 ? row[4] : nil let comment = row.count > 5 ? row[5] : nil + let isPk = row.count > 7 && row[7] == "YES" let charset: String? = { guard let coll = collation else { return nil } @@ -227,7 +239,7 @@ final class RedshiftPluginDriver: PluginDatabaseDriver, @unchecked Sendable { name: name, dataType: dataType, isNullable: isNullable, - isPrimaryKey: false, + isPrimaryKey: isPk, defaultValue: defaultValue, charset: charset, collation: collation, @@ -246,7 +258,8 @@ final class RedshiftPluginDriver: PluginDatabaseDriver, @unchecked Sendable { c.column_default, c.collation_name, pgd.description, - c.udt_name + c.udt_name, + CASE WHEN pk.column_name IS NOT NULL THEN 'YES' ELSE 'NO' END AS is_pk FROM information_schema.columns c LEFT JOIN pg_catalog.pg_class cls ON cls.relname = c.table_name @@ -254,6 +267,15 @@ final class RedshiftPluginDriver: PluginDatabaseDriver, @unchecked Sendable { LEFT JOIN pg_catalog.pg_description pgd ON pgd.objoid = cls.oid AND pgd.objsubid = c.ordinal_position + LEFT JOIN ( + SELECT DISTINCT kcu.table_name, kcu.column_name + FROM information_schema.table_constraints tc + JOIN information_schema.key_column_usage kcu + ON tc.constraint_name = kcu.constraint_name + AND tc.table_schema = kcu.table_schema + WHERE tc.constraint_type = 'PRIMARY KEY' + AND tc.table_schema = '\(escapedSchema)' + ) pk ON c.table_name = pk.table_name AND c.column_name = pk.column_name WHERE c.table_schema = '\(escapedSchema)' ORDER BY c.table_name, c.ordinal_position """ @@ -278,6 +300,7 @@ final class RedshiftPluginDriver: PluginDatabaseDriver, @unchecked Sendable { let defaultValue = row[4] let collation = row.count > 5 ? row[5] : nil let comment = row.count > 6 ? row[6] : nil + let isPk = row.count > 8 && row[8] == "YES" let charset: String? = { guard let coll = collation else { return nil } @@ -291,7 +314,7 @@ final class RedshiftPluginDriver: PluginDatabaseDriver, @unchecked Sendable { name: name, dataType: dataType, isNullable: isNullable, - isPrimaryKey: false, + isPrimaryKey: isPk, defaultValue: defaultValue, charset: charset, collation: collation, diff --git a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift new file mode 100644 index 00000000..1ecee048 --- /dev/null +++ b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift @@ -0,0 +1,219 @@ +// +// SQLStatementGeneratorPKRegressionTests.swift +// TableProTests +// +// Regression tests verifying DELETE/UPDATE uses PK-only WHERE clause +// for each database type that previously had broken PK detection. +// + +@testable import TablePro +import Testing + +@Suite("SQL Statement Generator PK Regression") +struct SQLStatementGeneratorPKRegressionTests { + private func makeGenerator( + tableName: String = "users", + columns: [String] = ["id", "name", "email"], + primaryKeyColumn: String? = "id", + databaseType: DatabaseType = .postgresql + ) -> SQLStatementGenerator { + SQLStatementGenerator( + tableName: tableName, + columns: columns, + primaryKeyColumn: primaryKeyColumn, + databaseType: databaseType + ) + } + + private func makeDeleteChange(rowIndex: Int, originalRow: [String?]) -> RowChange { + RowChange( + rowIndex: rowIndex, + type: .delete, + cellChanges: [], + originalRow: originalRow + ) + } + + private func makeUpdateChange( + rowIndex: Int, + columnIndex: Int, + columnName: String, + oldValue: String?, + newValue: String?, + originalRow: [String?] + ) -> RowChange { + RowChange( + rowIndex: rowIndex, + type: .update, + cellChanges: [CellChange(rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, oldValue: oldValue, newValue: newValue)], + originalRow: originalRow + ) + } + + // MARK: - PostgreSQL DELETE with PK + + @Test("PostgreSQL delete with PK uses $N placeholder and PK-only WHERE") + func testPostgreSQLDeleteWithPK() { + let generator = makeGenerator(databaseType: .postgresql) + let changes = [makeDeleteChange(rowIndex: 0, originalRow: ["1", "John", "john@test.com"])] + + let statements = generator.generateStatements( + from: changes, + insertedRowData: [:], + deletedRowIndices: [0], + insertedRowIndices: [] + ) + + #expect(statements.count == 1) + let stmt = statements[0] + #expect(stmt.sql.contains("DELETE FROM")) + #expect(stmt.sql.contains("\"users\"")) + #expect(stmt.sql.contains("\"id\" = $1")) + #expect(!stmt.sql.contains("\"name\"")) + #expect(!stmt.sql.contains("\"email\"")) + #expect(stmt.parameters.count == 1) + #expect(stmt.parameters[0] as? String == "1") + } + + @Test("PostgreSQL batch delete with PK uses OR") + func testPostgreSQLBatchDeleteWithPK() { + let generator = makeGenerator(databaseType: .postgresql) + let changes = [ + makeDeleteChange(rowIndex: 0, originalRow: ["1", "John", "john@test.com"]), + makeDeleteChange(rowIndex: 1, originalRow: ["2", "Jane", "jane@test.com"]) + ] + + let statements = generator.generateStatements( + from: changes, + insertedRowData: [:], + deletedRowIndices: [0, 1], + insertedRowIndices: [] + ) + + #expect(statements.count == 1) + let stmt = statements[0] + #expect(stmt.sql.contains("\"id\" = $1")) + #expect(stmt.sql.contains("\"id\" = $2")) + #expect(stmt.sql.contains(" OR ")) + #expect(!stmt.sql.contains("\"name\"")) + #expect(stmt.parameters.count == 2) + } + + // MARK: - MSSQL DELETE with PK + + @Test("MSSQL delete with PK uses ? placeholder and PK-only WHERE") + func testMSSQLDeleteWithPK() { + let generator = makeGenerator(databaseType: .mssql) + let changes = [makeDeleteChange(rowIndex: 0, originalRow: ["1", "John", "john@test.com"])] + + let statements = generator.generateStatements( + from: changes, + insertedRowData: [:], + deletedRowIndices: [0], + insertedRowIndices: [] + ) + + #expect(statements.count == 1) + let stmt = statements[0] + #expect(stmt.sql.contains("DELETE FROM")) + #expect(stmt.sql.contains("[users]")) + #expect(stmt.sql.contains("[id] = ?")) + #expect(!stmt.sql.contains("[name]")) + #expect(!stmt.sql.contains("[email]")) + #expect(stmt.parameters.count == 1) + } + + // MARK: - ClickHouse DELETE with PK + + @Test("ClickHouse delete with PK uses ALTER TABLE DELETE") + func testClickHouseDeleteWithPK() { + let generator = makeGenerator(databaseType: .clickhouse) + let changes = [makeDeleteChange(rowIndex: 0, originalRow: ["1", "John", "john@test.com"])] + + let statements = generator.generateStatements( + from: changes, + insertedRowData: [:], + deletedRowIndices: [0], + insertedRowIndices: [] + ) + + #expect(statements.count == 1) + let stmt = statements[0] + #expect(stmt.sql.contains("ALTER TABLE")) + #expect(stmt.sql.contains("DELETE WHERE")) + #expect(stmt.sql.contains("`id`")) + #expect(!stmt.sql.contains("`name`")) + #expect(!stmt.sql.contains("`email`")) + } + + // MARK: - UPDATE with PK + + @Test("PostgreSQL update with PK uses PK-only WHERE") + func testPostgreSQLUpdateWithPK() { + let generator = makeGenerator(databaseType: .postgresql) + let changes = [makeUpdateChange( + rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Jane", + originalRow: ["1", "John", "john@test.com"] + )] + + let statements = generator.generateStatements( + from: changes, + insertedRowData: [:], + deletedRowIndices: [], + insertedRowIndices: [] + ) + + #expect(statements.count == 1) + let stmt = statements[0] + #expect(stmt.sql.contains("UPDATE")) + #expect(stmt.sql.contains("\"name\" = $1")) + #expect(stmt.sql.contains("\"id\" = $2")) + #expect(!stmt.sql.contains("\"email\"")) + } + + @Test("MSSQL update with PK uses PK-only WHERE") + func testMSSQLUpdateWithPK() { + let generator = makeGenerator(databaseType: .mssql) + let changes = [makeUpdateChange( + rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "John", newValue: "Jane", + originalRow: ["1", "John", "john@test.com"] + )] + + let statements = generator.generateStatements( + from: changes, + insertedRowData: [:], + deletedRowIndices: [], + insertedRowIndices: [] + ) + + #expect(statements.count == 1) + let stmt = statements[0] + #expect(stmt.sql.contains("UPDATE")) + #expect(stmt.sql.contains("[name] = ?")) + #expect(stmt.sql.contains("[id] = ?")) + #expect(!stmt.sql.contains("[email]")) + } + + // MARK: - Redshift DELETE with PK + + @Test("Redshift delete with PK uses $N placeholder and PK-only WHERE") + func testRedshiftDeleteWithPK() { + let generator = makeGenerator(databaseType: .redshift) + let changes = [makeDeleteChange(rowIndex: 0, originalRow: ["1", "John", "john@test.com"])] + + let statements = generator.generateStatements( + from: changes, + insertedRowData: [:], + deletedRowIndices: [0], + insertedRowIndices: [] + ) + + #expect(statements.count == 1) + let stmt = statements[0] + #expect(stmt.sql.contains("DELETE FROM")) + #expect(stmt.sql.contains("\"id\" = $1")) + #expect(!stmt.sql.contains("\"name\"")) + #expect(!stmt.sql.contains("\"email\"")) + #expect(stmt.parameters.count == 1) + } +}