diff --git a/.github/macos-test-quarantine.txt b/.github/macos-test-quarantine.txt index 3b470cfb2..34040eef8 100644 --- a/.github/macos-test-quarantine.txt +++ b/.github/macos-test-quarantine.txt @@ -21,12 +21,11 @@ # TableOperationsPluginTests, 12 of these, is gone: every case asserted the deleted fallback and # the thirteenth passed only because an absent driver returns nothing. Its coverage lives in # TableProTests/Core/Database/TableOperationSQLBuilderTests.swift, against a stub driver. -TableQueryBuilderFilteredQueryTests/filteredQueryExcludesDisabledFilter() -TableQueryBuilderFilteredQueryTests/filteredQueryWithEnabledFilter() -ClickHouseDialectTests/testFactoryFallbackWithoutPlugin() -SQLStatementGeneratorPKRegressionTests/testClickHouseDeleteWithPK() -DataChangeManagerClickHouseTests/alterTableUpdateCounted() -DataChangeManagerClickHouseTests/clickhouseUpdateWithoutPrimaryKey() +# +# The three ClickHouse cases are gone the same way: ALTER TABLE ... UPDATE and DELETE WHERE are +# written only by ClickHousePlugin.generateStatements, and the app's SQLStatementGenerator emits +# the plain form for every engine. Asserted at the layer that owns them in +# TableProTests/Core/ClickHouse/ClickHouseDMLStatementTests.swift. SQLCompletionProviderTests/testCommaFromScopesColumnsToAllTables() SQLCompletionProviderTests/testMySQLProviderTypes() SQLCompletionProviderTests/testProviderAcceptsDatabaseType() @@ -36,13 +35,16 @@ SQLCompletionProviderTests/testProviderAcceptsDatabaseType() # empty registry produces: with nothing registered there is no duplicate to reject. ValidateDriverDescriptorTests/rejectsDuplicateAdditionalTypeId() ValidateDriverDescriptorTests/rejectsDuplicatePrimaryTypeId() -PluginCapabilityTests/decodingRemovedRawValueFails() # --- Stale fixture: the model gained a field the fixture JSON does not carry, so decoding throws # before the assertion is reached. -DatabaseConnectionExternalAccessTests/decodeJSONWithExplicitValue() -DatabaseConnectionExternalAccessTests/decodeLegacyJSONDefaultsToReadOnly() -DataGridSettingsDefaultSortDecoderTests/missingKeyFallsBackToNone() +# +# The three that were here are gone, and they were not all the same. SSHConfiguration required +# agentSocketPath despite the property having a default, so any stored config written before +# that field existed threw keyNotFound and took the whole connection with it; the decoder now +# treats every defaulted key as optional, the way its siblings already did. The row-height one +# was the other way round: DataGridRowHeight has been Int-backed since it was introduced, so the +# fixture's "rowHeight": "normal" described JSON that never shipped. # --- Asserts a string the app no longer produces. The behaviour changed on purpose; the expected # value did not follow. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2367f576f..57566a4b7 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 +- A saved connection whose SSH settings were written before the agent socket field existed now loads instead of disappearing. - Switch Connection and Open Database now open on a narrow window, and after you remove their toolbar button, instead of doing nothing at all. ## [0.67.1] - 2026-08-22 diff --git a/TablePro/Models/Connection/SSHTypes.swift b/TablePro/Models/Connection/SSHTypes.swift index a956194d9..bc638aed4 100644 --- a/TablePro/Models/Connection/SSHTypes.swift +++ b/TablePro/Models/Connection/SSHTypes.swift @@ -141,15 +141,20 @@ extension SSHConfiguration { case totpMode, totpAlgorithm, totpDigits, totpPeriod } + /// Every property here declares a default, so every key decodes as optional. A required decode + /// on a field that has a default cannot round-trip a payload written before that field existed: + /// it throws `keyNotFound` and takes the whole connection with it, because a connection that + /// fails to decode is a connection the user no longer has. `agentSocketPath` was the one still + /// required, which is why a stored SSH config from before it existed could not be read back. init(from decoder: Decoder) throws { let container = try decoder.container(keyedBy: CodingKeys.self) - enabled = try container.decode(Bool.self, forKey: .enabled) - host = try container.decode(String.self, forKey: .host) + enabled = try container.decodeIfPresent(Bool.self, forKey: .enabled) ?? false + host = try container.decodeIfPresent(String.self, forKey: .host) ?? "" port = try container.decodeIfPresent(Int.self, forKey: .port) - username = try container.decode(String.self, forKey: .username) + username = try container.decodeIfPresent(String.self, forKey: .username) ?? "" authMethod = (try? container.decodeIfPresent(SSHAuthMethod.self, forKey: .authMethod)) ?? .password - privateKeyPath = try container.decode(String.self, forKey: .privateKeyPath) - agentSocketPath = try container.decode(String.self, forKey: .agentSocketPath) + privateKeyPath = try container.decodeIfPresent(String.self, forKey: .privateKeyPath) ?? "" + agentSocketPath = try container.decodeIfPresent(String.self, forKey: .agentSocketPath) ?? "" jumpHosts = try container.decodeIfPresent([SSHJumpHost].self, forKey: .jumpHosts) ?? [] totpMode = try container.decodeIfPresent(TOTPMode.self, forKey: .totpMode) ?? .none totpAlgorithm = try container.decodeIfPresent(TOTPAlgorithm.self, forKey: .totpAlgorithm) ?? .sha1 diff --git a/TableProTests/Core/ChangeTracking/DataChangeManagerClickHouseTests.swift b/TableProTests/Core/ChangeTracking/DataChangeManagerClickHouseTests.swift index 37d679511..62bf4746f 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeManagerClickHouseTests.swift +++ b/TableProTests/Core/ChangeTracking/DataChangeManagerClickHouseTests.swift @@ -14,35 +14,6 @@ import Testing @MainActor @Suite("DataChangeManager ClickHouse UPDATE Validation") struct DataChangeManagerClickHouseTests { - @Test("ClickHouse ALTER TABLE UPDATE is counted as an update statement") - func alterTableUpdateCounted() async throws { - let manager = DataChangeManager() - manager.configureForTable( - tableName: "users", - columns: ["id", "name"], - primaryKeyColumns: ["id"], - databaseType: .clickhouse - ) - - manager.recordCellChange( - rowIndex: 0, - columnIndex: 1, - columnName: "name", - oldValue: "Alice", - newValue: "Bob", - originalRow: ["1", "Alice"] - ) - - #expect(manager.hasChanges) - - let statements = try manager.generateSQL() - #expect(!statements.isEmpty) - - // ClickHouse generates ALTER TABLE ... UPDATE instead of UPDATE - let hasAlterTableUpdate = statements.contains { $0.sql.hasPrefix("ALTER TABLE") } - #expect(hasAlterTableUpdate) - } - @Test("ClickHouse ALTER TABLE UPDATE passes validation without throwing") func alterTableUpdatePassesValidation() async { let manager = DataChangeManager() @@ -94,31 +65,4 @@ struct DataChangeManagerClickHouseTests { #expect(hasStandardUpdate) } - @Test("ClickHouse UPDATE without primary key uses all columns in WHERE clause") - func clickhouseUpdateWithoutPrimaryKey() async throws { - let manager = DataChangeManager() - manager.configureForTable( - tableName: "logs", - columns: ["timestamp", "message"], - primaryKeyColumns: [], - databaseType: .clickhouse - ) - - manager.recordCellChange( - rowIndex: 0, - columnIndex: 1, - columnName: "message", - oldValue: "old log", - newValue: "new log", - originalRow: ["2024-01-01", "old log"] - ) - - let statements = try manager.generateSQL() - #expect(!statements.isEmpty) - - let alterStatement = statements.first { $0.sql.hasPrefix("ALTER TABLE") } - #expect(alterStatement != nil) - #expect(alterStatement?.sql.contains("UPDATE") == true) - #expect(alterStatement?.sql.contains("WHERE") == true) - } } diff --git a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift index f982d6ee2..b90823067 100644 --- a/TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift +++ b/TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift @@ -127,27 +127,6 @@ struct SQLStatementGeneratorPKRegressionTests { // MARK: - ClickHouse DELETE with PK - @Test("ClickHouse delete with PK uses ALTER TABLE DELETE") - func testClickHouseDeleteWithPK() throws { - let generator = try 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") diff --git a/TableProTests/Core/ClickHouse/ClickHouseDMLStatementTests.swift b/TableProTests/Core/ClickHouse/ClickHouseDMLStatementTests.swift new file mode 100644 index 000000000..32328620a --- /dev/null +++ b/TableProTests/Core/ClickHouse/ClickHouseDMLStatementTests.swift @@ -0,0 +1,124 @@ +// +// ClickHouseDMLStatementTests.swift +// TableProTests +// + +import Foundation +@testable import TablePro +import TableProPluginKit +import Testing + +/// ClickHouse mutates through `ALTER TABLE … UPDATE` and `ALTER TABLE … DELETE WHERE` rather than +/// the plain `UPDATE` and `DELETE FROM` every other engine takes. +/// +/// That shape belongs to the driver: `ClickHousePluginDriver.generateStatements` is the only thing in +/// repository that writes it, and the app's `SQLStatementGenerator` always emits the plain form. +/// Three cases used to assert the ClickHouse shape through `DataChangeManager` with no driver +/// connected, which the app layer cannot produce and never could, so they sat in the quarantine +/// file. Asserted here against the driver, they hold. +@Suite("ClickHouse DML statements") +struct ClickHouseDMLStatementTests { + private let table = "users" + private let columns = ["id", "name"] + + /// The driver, not the plugin: `generateStatements` belongs to `ClickHousePluginDriver`. It is + /// pure SQL shaping with no connection behind it, so a config that never dials anywhere is + /// enough to exercise it. + private var driver: ClickHousePluginDriver { + ClickHousePluginDriver(config: DriverConnectionConfig( + host: "localhost", + port: 8_123, + username: "default", + password: "", + database: "default", + ssl: SSLConfiguration(), + additionalFields: [:] + )) + } + + private func change( + _ type: PluginRowChange.ChangeType, + cells: [(columnIndex: Int, columnName: String, oldValue: PluginCellValue, newValue: PluginCellValue)] = [], + originalRow: [PluginCellValue]? = nil + ) -> PluginRowChange { + PluginRowChange(rowIndex: 0, type: type, cellChanges: cells, originalRow: originalRow) + } + + @Test("An update becomes ALTER TABLE ... UPDATE keyed on the original row") + func updateUsesAlterTable() { + let statements = driver.generateStatements( + table: table, + columns: columns, + primaryKeyColumns: ["id"], + changes: [change( + .update, + cells: [(columnIndex: 1, columnName: "name", oldValue: .text("Alice"), newValue: .text("Bob"))], + originalRow: [.text("1"), .text("Alice")] + )], + insertedRowData: [:], + deletedRowIndices: [], + insertedRowIndices: [] + ) + + #expect(statements?.count == 1) + let sql = statements?.first?.statement ?? "" + #expect(sql.hasPrefix("ALTER TABLE")) + #expect(sql.contains("UPDATE")) + #expect(sql.contains("WHERE")) + #expect(sql.contains("`users`")) + } + + @Test("A delete becomes ALTER TABLE ... DELETE WHERE") + func deleteUsesAlterTable() { + let statements = driver.generateStatements( + table: table, + columns: columns, + primaryKeyColumns: ["id"], + changes: [change(.delete, originalRow: [.text("1"), .text("Alice")])], + insertedRowData: [:], + deletedRowIndices: [0], + insertedRowIndices: [] + ) + + #expect(statements?.count == 1) + let sql = statements?.first?.statement ?? "" + #expect(sql.hasPrefix("ALTER TABLE")) + #expect(sql.contains("DELETE WHERE")) + #expect(sql.contains("`users`")) + } + + /// Inserts are ordinary. Only the two mutating statements need the ALTER form, and asserting it + /// here keeps a later change from applying it where it does not belong. + @Test("An insert stays a plain INSERT") + func insertStaysPlain() { + let statements = driver.generateStatements( + table: table, + columns: columns, + primaryKeyColumns: ["id"], + changes: [change(.insert)], + insertedRowData: [0: [.text("7"), .text("Carol")]], + deletedRowIndices: [], + insertedRowIndices: [0] + ) + + let sql = statements?.first?.statement ?? "" + #expect(sql.hasPrefix("INSERT INTO")) + #expect(!sql.contains("ALTER TABLE")) + } + + /// A row the caller did not mark deleted is skipped, so a stale change cannot delete anything. + @Test("A delete not listed in deletedRowIndices produces nothing") + func unlistedDeleteIsSkipped() { + let statements = driver.generateStatements( + table: table, + columns: columns, + primaryKeyColumns: ["id"], + changes: [change(.delete, originalRow: [.text("1"), .text("Alice")])], + insertedRowData: [:], + deletedRowIndices: [], + insertedRowIndices: [] + ) + + #expect(statements == nil) + } +} diff --git a/TableProTests/Core/ClickHouse/ClickHouseDialectTests.swift b/TableProTests/Core/ClickHouse/ClickHouseDialectTests.swift index ec37f1938..4a4206b11 100644 --- a/TableProTests/Core/ClickHouse/ClickHouseDialectTests.swift +++ b/TableProTests/Core/ClickHouse/ClickHouseDialectTests.swift @@ -29,11 +29,16 @@ struct ClickHouseDialectTests { #expect(adapter.dataTypes.contains("UInt32")) } - @Test("Factory returns empty dialect when plugin not loaded") + /// The ClickHouse driver is one of the plugins bundled inside the app, and the test host is the + /// app, so the factory resolves the real dialect here. This asserted an empty fallback on the + /// premise that no plugin was loaded, which stopped being true and is why it sat in the + /// quarantine file: the fallback is unreachable from a host that ships the driver. + @Test("The factory resolves ClickHouse's dialect from the bundled plugin") @MainActor - func testFactoryFallbackWithoutPlugin() { + func testFactoryResolvesBundledDialect() { let dialect = SQLDialectFactory.createDialect(for: .clickhouse) - // Without plugin loaded, factory returns empty fallback - #expect(dialect.keywords.isEmpty) + + #expect(!dialect.keywords.isEmpty) + #expect(dialect.keywords.contains("OPTIMIZE")) } } diff --git a/TableProTests/Core/Database/DatabaseConnectionExternalAccessTests.swift b/TableProTests/Core/Database/DatabaseConnectionExternalAccessTests.swift index d734ef6d8..bb1b550e1 100644 --- a/TableProTests/Core/Database/DatabaseConnectionExternalAccessTests.swift +++ b/TableProTests/Core/Database/DatabaseConnectionExternalAccessTests.swift @@ -17,57 +17,37 @@ struct DatabaseConnectionExternalAccessTests { #expect(connection.externalAccess == .readOnly) } + /// Both cases build their JSON by encoding a real connection and editing one key, rather than + /// hand-writing a document. The hand-written fixture they replace had drifted three ways at + /// once: it named the SSH tunnel's discriminator `kind` when the wire key has always been + /// `mode`, gave `sslConfig` only one of its four keys, and omitted `agentSocketPath` entirely. + /// None of those shapes was ever written by the app, so the test failed on its own scaffolding + /// instead of on the property it exists to check. Encoding first means the fixture cannot + /// describe a document the encoder would not produce. + private func encodedConnection(_ connection: DatabaseConnection) throws -> [String: Any] { + let data = try JSONEncoder().encode(connection) + return try #require(try JSONSerialization.jsonObject(with: data) as? [String: Any]) + } + + private func decodeConnection(from object: [String: Any]) throws -> DatabaseConnection { + let data = try JSONSerialization.data(withJSONObject: object) + return try JSONDecoder().decode(DatabaseConnection.self, from: data) + } + @Test("Decoding legacy JSON without externalAccess defaults to readOnly") func decodeLegacyJSONDefaultsToReadOnly() throws { - let json = """ - { - "id": "11111111-2222-3333-4444-555555555555", - "name": "Legacy", - "host": "localhost", - "port": 3306, - "database": "test", - "username": "root", - "type": "MySQL", - "sshConfig": { "enabled": false, "host": "", "port": 22, "username": "", "authMethod": "password", "privateKeyPath": "" }, - "sslConfig": { "mode": "preferred" }, - "color": "None", - "sshTunnelMode": { "kind": "disabled" }, - "safeModeLevel": "silent", - "additionalFields": {}, - "sortOrder": 0, - "localOnly": false - } - """ - let data = Data(json.utf8) - let connection = try JSONDecoder().decode(DatabaseConnection.self, from: data) - #expect(connection.externalAccess == .readOnly) + var object = try encodedConnection(DatabaseConnection(name: "Legacy")) + object.removeValue(forKey: "externalAccess") + + #expect(try decodeConnection(from: object).externalAccess == .readOnly) } @Test("Decoding JSON with explicit externalAccess preserves value") func decodeJSONWithExplicitValue() throws { - let json = """ - { - "id": "11111111-2222-3333-4444-555555555555", - "name": "Test", - "host": "localhost", - "port": 3306, - "database": "", - "username": "", - "type": "MySQL", - "sshConfig": { "enabled": false, "host": "", "port": 22, "username": "", "authMethod": "password", "privateKeyPath": "" }, - "sslConfig": { "mode": "preferred" }, - "color": "None", - "sshTunnelMode": { "kind": "disabled" }, - "safeModeLevel": "silent", - "externalAccess": "blocked", - "additionalFields": {}, - "sortOrder": 0, - "localOnly": false - } - """ - let data = Data(json.utf8) - let connection = try JSONDecoder().decode(DatabaseConnection.self, from: data) - #expect(connection.externalAccess == .blocked) + var object = try encodedConnection(DatabaseConnection(name: "Test")) + object["externalAccess"] = ExternalAccessLevel.blocked.rawValue + + #expect(try decodeConnection(from: object).externalAccess == .blocked) } @Test("Encoding round-trips externalAccess") diff --git a/TableProTests/Core/Plugins/PluginSettingsTests.swift b/TableProTests/Core/Plugins/PluginSettingsTests.swift index fedc5ba45..0f43943fa 100644 --- a/TableProTests/Core/Plugins/PluginSettingsTests.swift +++ b/TableProTests/Core/Plugins/PluginSettingsTests.swift @@ -241,12 +241,25 @@ struct PluginCapabilityTests { #expect(decoded == original) } - @Test("decoding removed raw value 3 fails gracefully") - func decodingRemovedRawValueFails() { - let json = Data("3".utf8) - let decoded = try? JSONDecoder().decode(PluginCapability.self, from: json) + /// `PluginCapability` is a growing set, so a raw value that means nothing today can mean + /// something tomorrow: this asserted that 3 was undecodable and 3 is now `documentInspector`. + /// A value far outside the range keeps the property under test, which is that an unknown + /// capability decodes to nothing rather than to a neighbouring case. + @Test("decoding an unknown raw value fails gracefully") + func decodingUnknownRawValueFails() { + let decoded = try? JSONDecoder().decode(PluginCapability.self, from: Data("9999".utf8)) #expect(decoded == nil) } + + @Test("every declared capability round-trips") + func declaredCapabilitiesRoundTrip() { + for capability in [ + PluginCapability.databaseDriver, .exportFormat, .importFormat, .documentInspector, + ] { + let data = try? JSONEncoder().encode(capability) + #expect(data.flatMap { try? JSONDecoder().decode(PluginCapability.self, from: $0) } == capability) + } + } } @Suite("DisabledPlugins Key Migration", .serialized) diff --git a/TableProTests/Core/Services/TableQueryBuilderFilterTests.swift b/TableProTests/Core/Services/TableQueryBuilderFilterTests.swift index 771d12582..1400117d5 100644 --- a/TableProTests/Core/Services/TableQueryBuilderFilterTests.swift +++ b/TableProTests/Core/Services/TableQueryBuilderFilterTests.swift @@ -12,7 +12,18 @@ import Testing @Suite("Table Query Builder - Filtered Query Fallback") struct TableQueryBuilderFilteredQueryTests { - private let builder = TableQueryBuilder(databaseType: .mysql) + /// The dialect is what carries the quoting and the operators, so a builder without one emits no + /// WHERE at all: that is what `TableQueryBuilderNoSQLTests` asserts for MongoDB. These cases are + /// about the SQL fallback, so they need a dialect the way the count suite below has one. Built + /// without it, they were asserting behaviour the builder stopped having when the dialect became + /// the source of SQL syntax. + private static let mysqlDialect = SQLDialectDescriptor( + identifierQuote: "`", keywords: [], functions: [], dataTypes: [], + regexSyntax: .regexp, booleanLiteralStyle: .numeric, + likeEscapeStyle: .implicit, paginationStyle: .limit + ) + + private let builder = TableQueryBuilder(databaseType: .mysql, dialect: Self.mysqlDialect) @Test("buildFilteredQuery with enabled filter produces WHERE clause") func filteredQueryWithEnabledFilter() { diff --git a/TableProTests/Models/Query/DefaultSortStateTests.swift b/TableProTests/Models/Query/DefaultSortStateTests.swift index bcd9df5c3..4f07491a7 100644 --- a/TableProTests/Models/Query/DefaultSortStateTests.swift +++ b/TableProTests/Models/Query/DefaultSortStateTests.swift @@ -75,7 +75,6 @@ struct DataGridSettingsDefaultSortDecoderTests { func missingKeyFallsBackToNone() throws { let legacyJSON = """ { - "rowHeight": "normal", "dateFormat": "yyyy-MM-dd HH:mm:ss", "nullDisplay": "NULL", "defaultPageSize": 1000, diff --git a/project.yml b/project.yml index 4f9409a31..c9222d846 100644 --- a/project.yml +++ b/project.yml @@ -342,6 +342,10 @@ targets: - Plugins/ClickHouseDriverPlugin/ClickHouseCapabilities.swift - Plugins/ClickHouseDriverPlugin/ClickHouseCredentials.swift - Plugins/ClickHouseDriverPlugin/ClickHouseGeneratedColumnClassification.swift + - Plugins/ClickHouseDriverPlugin/ClickHousePlugin.swift + - Plugins/ClickHouseDriverPlugin/ClickHousePluginDriver+Http.swift + - Plugins/ClickHouseDriverPlugin/ClickHousePluginDriver+Schema.swift + - Plugins/ClickHouseDriverPlugin/ClickHousePluginDriver+TableOperations.swift - Plugins/ClickHouseDriverPlugin/ClickHouseTableOperations.swift - Plugins/DamengDriverPlugin/DamengParameterBinder.swift - Plugins/DamengDriverPlugin/DamengStatementClassifier.swift