From b4baf8a26b17559a682c154112295958a87cd6b9 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 29 Sep 2026 19:43:08 +0700 Subject: [PATCH 1/2] fix(plugins): pin the row import sheet to one database and keep new table column edits across a re-read --- CHANGELOG.md | 2 + .../Core/Services/Export/ImportService.swift | 15 +- .../Export/NewTableImportPlanner.swift | 30 ++- .../Core/Storage/Preferences/TableScope.swift | 4 + TablePro/Views/Import/NewTableDraft.swift | 159 +++++++++++++ TablePro/Views/Import/RowImportSheet.swift | 218 +++++++----------- .../Services/ImportServiceHistoryTests.swift | 74 ++++++ .../Services/NewTableImportPlannerTests.swift | 95 ++++++-- .../Views/Import/NewTableDraftTests.swift | 212 +++++++++++++++++ .../RowImportNewTableEditsUITests.swift | 102 ++++++++ docs/features/import-export.mdx | 4 +- 11 files changed, 745 insertions(+), 170 deletions(-) create mode 100644 TablePro/Views/Import/NewTableDraft.swift create mode 100644 TableProTests/Core/Services/ImportServiceHistoryTests.swift create mode 100644 TableProTests/Views/Import/NewTableDraftTests.swift create mode 100644 TableProUITests/RowImportNewTableEditsUITests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index ce7bb90056..49764fb3c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - CSV import failing on every row when two headers differ only by case. - CSV and Excel imports reading a column under the wrong header when headers repeat or a blank one comes first. - Import sheet's Try Again for an existing table discarding the column edits made for a new table. +- Import sheet creating, emptying or filling tables in another database after a database switch in another window. +- Import sheet discarding a new table's column edits when a CSV or Excel option changes. ## [0.76.1] - 2026-09-29 diff --git a/TablePro/Core/Services/Export/ImportService.swift b/TablePro/Core/Services/Export/ImportService.swift index 9283e2fa05..9479f00d51 100644 --- a/TablePro/Core/Services/Export/ImportService.swift +++ b/TablePro/Core/Services/Export/ImportService.swift @@ -158,7 +158,7 @@ final class ImportService: ObservableObject { QueryHistoryRecordRequest( query: "-- Import from \(url.lastPathComponent) (\(progress.processedStatements) statements before failure)", connectionId: connection.id, - databaseName: DatabaseManager.shared.browseDatabaseName(for: connection), + databaseName: scope.database, databaseType: connection.type, source: .dataImport, executionTime: Date().timeIntervalSince(startedAt), @@ -169,7 +169,7 @@ final class ImportService: ObservableObject { ) reportImportFinished( - .failed(reason: error.localizedDescription), connection: connection, startedAt: operationStart + .failed(reason: error.localizedDescription), in: scope, startedAt: operationStart ) throw error } @@ -183,7 +183,7 @@ final class ImportService: ObservableObject { QueryHistoryRecordRequest( query: "-- Import from \(url.lastPathComponent) (\(result.executedStatements) statements)", connectionId: connection.id, - databaseName: DatabaseManager.shared.browseDatabaseName(for: connection), + databaseName: scope.database, databaseType: connection.type, source: .dataImport, executionTime: result.executionTime, @@ -194,7 +194,7 @@ final class ImportService: ObservableObject { reportImportFinished( .succeeded(OperationSummary(statementCount: result.executedStatements)), - connection: connection, + in: scope, startedAt: operationStart ) @@ -226,9 +226,12 @@ final class ImportService: ObservableObject { /// An import the user cancelled reports nothing, matching what history already does with one /// and for the same reason: they stopped it, so they know. + /// + /// Named after the scope the import ran in, as its history row is. The browse database read when + /// the import ends is wherever another window moved the connection in the meantime. private func reportImportFinished( _ outcome: OperationOutcome, - connection: DatabaseConnection, + in scope: DatabaseScope, startedAt: ContinuousClock.Instant ) { OperationCompletionReporter.shared.report( @@ -237,7 +240,7 @@ final class ImportService: ObservableObject { owner: .connection(connection.id), connectionId: connection.id, connectionName: connection.name, - databaseName: DatabaseManager.shared.browseDatabaseName(for: connection), + databaseName: scope.database, elapsed: startedAt.duration(to: .now), outcome: outcome ) diff --git a/TablePro/Core/Services/Export/NewTableImportPlanner.swift b/TablePro/Core/Services/Export/NewTableImportPlanner.swift index 4d188fe6b7..639de788c2 100644 --- a/TablePro/Core/Services/Export/NewTableImportPlanner.swift +++ b/TablePro/Core/Services/Export/NewTableImportPlanner.swift @@ -19,15 +19,31 @@ enum NewTableImportPlan: Equatable { /// the table as it stands would write those rows a second time. The table is only ever reused when /// this sheet created it in this session with exactly the columns being asked for now, which is the /// one case where clearing it can lose nothing but the sheet's own failed attempt. -enum NewTableImportPlanner { - static func plan( - forTable tableName: String, - createTableSQL: String, - alreadyCreated: [String: String] - ) -> NewTableImportPlan { - guard let previousSQL = alreadyCreated[tableName] else { +/// +/// A table is known by its database and schema as well as its name. Keyed by name alone, a table the +/// sheet made in one database vouched for a same-named table in another, and the retry cleared that +/// one with `DELETE FROM`. +struct NewTableImportPlanner { + private var createStatements: [TableScope: String] = [:] + + func plan(forTable table: TableScope, createTableSQL: String) -> NewTableImportPlan { + guard let previousSQL = createStatements[table] else { return .create } return previousSQL == createTableSQL ? .reuseAfterClearing : .nameTakenWithDifferentColumns } + + func created(_ table: TableScope) -> Bool { + createStatements[table] != nil + } + + func createdTableNames(in scope: DatabaseScope) -> [String] { + createStatements.keys + .filter { $0 == TableScope(table: $0.table, in: scope) } + .map(\.table) + } + + mutating func recordCreated(_ table: TableScope, createTableSQL: String) { + createStatements[table] = createTableSQL + } } diff --git a/TablePro/Core/Storage/Preferences/TableScope.swift b/TablePro/Core/Storage/Preferences/TableScope.swift index 7b26992ad6..51c02b2a21 100644 --- a/TablePro/Core/Storage/Preferences/TableScope.swift +++ b/TablePro/Core/Storage/Preferences/TableScope.swift @@ -18,6 +18,10 @@ struct TableScope: Hashable, Codable, Sendable { self.table = table } + init(table: String, in scope: DatabaseScope) { + self.init(connectionId: scope.connectionId, database: scope.database, schema: scope.schema, table: table) + } + var storageComponent: String { Self.encode([connectionId.uuidString, database ?? "", schema ?? "", table]) } diff --git a/TablePro/Views/Import/NewTableDraft.swift b/TablePro/Views/Import/NewTableDraft.swift new file mode 100644 index 0000000000..ddc347f62e --- /dev/null +++ b/TablePro/Views/Import/NewTableDraft.swift @@ -0,0 +1,159 @@ +// +// NewTableDraft.swift +// TablePro +// + +import Foundation +import TableProPluginKit + +/// What a row import makes of one field of the file in the table it creates. +internal struct NewTableColumnSettings: Equatable { + internal var include: Bool + internal var name: String + internal var type: String + internal var isPrimaryKey: Bool + internal var isNullable: Bool + internal var defaultValue: String + + /// Every field, under its own name and with the type its values suggest, nullable, with no key + /// and no default. + internal static func proposed(name: String, type: String) -> NewTableColumnSettings { + NewTableColumnSettings( + include: true, + name: name, + type: type, + isPrimaryKey: false, + isNullable: true, + defaultValue: "" + ) + } + + /// A setting still equal to what the sheet proposed was never changed, so it takes the new + /// proposal. One the user moved away from the proposal is theirs and stays. A type that differs + /// from the proposal only in case is the proposal, because the type menu offers the dialect's + /// own spelling of it. + internal func keepingEdits( + madeTo earlier: NewTableColumnSettings, + over proposal: NewTableColumnSettings + ) -> NewTableColumnSettings { + NewTableColumnSettings( + include: include == earlier.include ? proposal.include : include, + name: name == earlier.name ? proposal.name : name, + type: type.caseInsensitiveCompare(earlier.type) == .orderedSame ? proposal.type : type, + isPrimaryKey: isPrimaryKey == earlier.isPrimaryKey ? proposal.isPrimaryKey : isPrimaryKey, + isNullable: isNullable == earlier.isNullable ? proposal.isNullable : isNullable, + defaultValue: defaultValue == earlier.defaultValue ? proposal.defaultValue : defaultValue + ) + } +} + +/// One field of the file and the column the user is making of it. +internal struct NewTableColumn: Identifiable { + internal let field: PluginImportField + + /// What the sheet offered for this field on the read that produced it, so the next read can tell + /// the user's changes from the sheet's own guesses. + internal let proposal: NewTableColumnSettings + internal var settings: NewTableColumnSettings + + internal var id: String { field.name } +} + +internal enum NewTableColumnProblem: Equatable { + case unnamedColumn + case duplicateName +} + +/// The table a row import creates: the fields of the file, what the sheet proposed for each, and +/// what the user changed. +/// +/// Changing a parsing option reads the file again, and rebuilding the columns from that read threw +/// away every rename, type, key, nullability, default and exclusion the user had set. A field that +/// survives the read keeps each setting the user changed, and each setting left as proposed follows +/// the read, the way the sheet's table name keeps what the user typed over its own suggestion. +internal struct NewTableDraft { + internal var columns: [NewTableColumn] = [] + + internal mutating func load( + fields: [PluginImportField], + proposingType proposedType: (PluginImportFieldType) -> String + ) { + let earlier = Dictionary(columns.map { ($0.id, $0) }, uniquingKeysWith: { first, _ in first }) + columns = fields.map { field in + let proposal = NewTableColumnSettings.proposed(name: field.name, type: proposedType(field.inferredType)) + let settings = earlier[field.name].map { $0.settings.keepingEdits(madeTo: $0.proposal, over: proposal) } + return NewTableColumn(field: field, proposal: proposal, settings: settings ?? proposal) + } + } + + internal var includesEveryColumn: Bool { + !columns.isEmpty && columns.allSatisfy(\.settings.include) + } + + internal mutating func setAllIncluded(_ include: Bool) { + for index in columns.indices { + columns[index].settings.include = include + } + } + + /// Every field of the file in file order, the ones left out of the table included. + internal var fields: [String] { + columns.map(\.field.name) + } + + /// The column each field is written to, for every included field whose column has a name. + internal var columnMapping: [String: String] { + var mapping: [String: String] = [:] + for column in namedColumns { + mapping[column.field.name] = column.settings.name + } + return mapping + } + + internal var hasNamedColumn: Bool { + !namedColumns.isEmpty + } + + internal var problem: NewTableColumnProblem? { + let names = columns + .filter(\.settings.include) + .map { $0.settings.name.trimmingCharacters(in: .whitespaces).lowercased() } + if names.contains(where: \.isEmpty) { + return .unnamedColumn + } + return Set(names).count == names.count ? nil : .duplicateName + } + + /// Nil when no included column has both a name and a type, which leaves nothing to create. + internal func definition(tableName: String) -> PluginCreateTableDefinition? { + let included = namedColumns + .map(\.settings) + .filter { !$0.type.trimmingCharacters(in: .whitespaces).isEmpty } + guard !included.isEmpty else { return nil } + return PluginCreateTableDefinition( + tableName: tableName, + columns: included.map { column in + PluginColumnDefinition( + name: column.name, + dataType: column.type, + isNullable: column.isNullable, + defaultValue: column.defaultValue.isEmpty ? nil : column.defaultValue, + isPrimaryKey: column.isPrimaryKey, + autoIncrement: false, + comment: nil, + unsigned: false, + onUpdate: nil, + charset: nil, + collation: nil + ) + }, + primaryKeyColumns: included.filter(\.isPrimaryKey).map(\.name) + ) + } + + private var namedColumns: [NewTableColumn] { + columns.filter { + $0.settings.include && !$0.settings.name.trimmingCharacters(in: .whitespaces).isEmpty + } + } +} diff --git a/TablePro/Views/Import/RowImportSheet.swift b/TablePro/Views/Import/RowImportSheet.swift index 626a8fe13d..8c5722e5f8 100644 --- a/TablePro/Views/Import/RowImportSheet.swift +++ b/TablePro/Views/Import/RowImportSheet.swift @@ -45,19 +45,16 @@ struct RowImportSheet: View { let newTable: PluginCreateTableDefinition? } - private struct NewColumn: Identifiable { - let field: PluginImportField - var include: Bool - var name: String - var type: String - var isPrimaryKey: Bool - var isNullable: Bool - var defaultValue: String - var id: String { field.name } - } - @State private var destination: Destination = .existingTable + /// The database and schema every read and write of this sheet goes to: where the connection was + /// browsing when the sheet opened. The sheet is modal to its window, but another window on the + /// same connection, or an MCP client, can move the browse database while it is open. Resolving + /// it again at each step listed one database's tables and mapped one table's columns, then + /// created, cleared and filled tables in another. Nil only while the connection has no session, + /// and taken once one exists. + @State private var scope: DatabaseScope? + /// Every object the connection holds, not just the tables the destination picker offers. A /// `CREATE TABLE` collides with a view, a materialized view or a foreign table under the same /// name as surely as with a table, so the name check has to see all of them. @@ -81,7 +78,7 @@ struct RowImportSheet: View { /// The last name this sheet proposed, so a second pass can tell its own guess from what /// the user typed over it. @State private var proposedTableName: String = "" - @State private var newColumns: [NewColumn] = [] + @State private var newTable = NewTableDraft() @State private var isLoadingContext = false /// Copied from the plugin, whose options are edited in its own view that this sheet does not observe. @@ -111,16 +108,24 @@ struct RowImportSheet: View { @State private var showErrorDialog = false @State private var importTask: Task? - /// Tables this sheet created, against the CREATE that made each one. A failed import leaves its - /// table behind, so a retry has to know it already owns that table rather than trying to create - /// it a second time and failing on the name. - @State private var createdTables: [String: String] = [:] + /// Knows the tables this sheet created. A failed import leaves its table behind, so a retry has to + /// know it already owns that table rather than trying to create it a second time and failing on + /// the name. + @State private var newTablePlanner = NewTableImportPlanner() /// The window this sheet is hosted in, used for presenting its alerts. /// Avoids `NSApp.keyWindow`, which when a result is presented is the progress sheet being /// torn down in the same transaction, and AppKit ends a sheet's children with it (#2314). @State private var hostWindow: NSWindow? + init(isPresented: Binding, connection: DatabaseConnection, fileURL: URL, formatId: String) { + _isPresented = isPresented + self.connection = connection + self.fileURL = fileURL + self.formatId = formatId + _scope = State(initialValue: DatabaseManager.shared.browseScope(for: connection.id)) + } + // MARK: - Derived catalog state /// Tables alone, because they are the only objects the existing-table branch can insert into. @@ -131,9 +136,8 @@ struct RowImportSheet: View { /// A table this sheet created is not in the way of this sheet: a failed import leaves its table /// behind, and `NewTableImportPlanner` exists to reuse that one on the retry, so reporting the - /// name as taken would block the very attempt that mechanism exists to allow. Checked against - /// `createdTables` by key rather than folded into `catalogNameKeys`, which is rebuilt only when - /// the catalog is. + /// name as taken would block the very attempt that mechanism exists to allow. Asked of the + /// planner rather than folded into `catalogNameKeys`, which is rebuilt only when the catalog is. private var newTableNameProblem: NewTableNameProblem? { let trimmed = newTableName.trimmingCharacters(in: .whitespaces) let problem = NewTableNaming.problem( @@ -141,7 +145,9 @@ struct RowImportSheet: View { style: NewTableNameStyle.forDatabaseType(connection.type), existingNames: catalogNameKeys ) - guard problem == .nameTaken, createdTables[trimmed] != nil else { return problem } + guard problem == .nameTaken, let scope, newTablePlanner.created(TableScope(table: trimmed, in: scope)) else { + return problem + } return nil } @@ -364,7 +370,7 @@ struct RowImportSheet: View { mappingTable } case .newTable: - if newColumns.isEmpty { + if newTable.columns.isEmpty { placeholder("No columns found in the file.") } else { newColumnsTable @@ -521,7 +527,7 @@ struct RowImportSheet: View { ScrollView { VStack(spacing: 6) { - ForEach(newColumns) { row in + ForEach(newTable.columns) { row in newColumnRow(row) } } @@ -531,43 +537,45 @@ struct RowImportSheet: View { } } - private func newColumnRow(_ row: NewColumn) -> some View { - HStack(spacing: 10) { - Toggle(row.name, isOn: columnBinding(row).include) + private func newColumnRow(_ row: NewTableColumn) -> some View { + let column = row.settings + let settings = columnBinding(row).settings + return HStack(spacing: 10) { + Toggle(column.name, isOn: settings.include) .labelsHidden() - .accessibilityLabel(Text(String(format: String(localized: "Create %@"), row.name))) + .accessibilityLabel(Text(String(format: String(localized: "Create %@"), column.name))) .frame(width: 16) - TextField("name", text: columnBinding(row).name) + TextField("name", text: settings.name) .textFieldStyle(.roundedBorder) .frame(width: 150) - .disabled(!row.include) + .disabled(!column.include) Picker(String(localized: "Type"), selection: typeBinding(row)) { - ForEach(typeOptions(including: row.type), id: \.self) { type in + ForEach(typeOptions(including: column.type), id: \.self) { type in Text(type).tag(type) } } .pickerStyle(.menu) .labelsHidden() - .accessibilityLabel(Text(String(format: String(localized: "Type of %@"), row.name))) + .accessibilityLabel(Text(String(format: String(localized: "Type of %@"), column.name))) .frame(width: 150) - .disabled(!row.include) - Toggle(String(localized: "Primary key"), isOn: columnBinding(row).isPrimaryKey) + .disabled(!column.include) + Toggle(String(localized: "Primary key"), isOn: settings.isPrimaryKey) .labelsHidden() - .accessibilityLabel(Text(String(format: String(localized: "%@ is a primary key"), row.name))) + .accessibilityLabel(Text(String(format: String(localized: "%@ is a primary key"), column.name))) .frame(minWidth: 30) - .disabled(!row.include) - Toggle(String(localized: "Nullable"), isOn: columnBinding(row).isNullable) + .disabled(!column.include) + Toggle(String(localized: "Nullable"), isOn: settings.isNullable) .labelsHidden() - .accessibilityLabel(Text(String(format: String(localized: "%@ accepts null"), row.name))) + .accessibilityLabel(Text(String(format: String(localized: "%@ accepts null"), column.name))) .frame(minWidth: 30) - .disabled(!row.include) - TextField(String(localized: "Default, as SQL"), text: columnBinding(row).defaultValue) + .disabled(!column.include) + TextField(String(localized: "Default, as SQL"), text: settings.defaultValue) .labelsHidden() - .accessibilityLabel(Text(String(format: String(localized: "Default SQL for %@"), row.name))) + .accessibilityLabel(Text(String(format: String(localized: "Default SQL for %@"), column.name))) .help(String(localized: "The SQL after DEFAULT. A text value needs its own quotes.")) .textFieldStyle(.roundedBorder) .frame(maxWidth: .infinity) - .disabled(!row.include) + .disabled(!column.include) } } @@ -580,11 +588,11 @@ struct RowImportSheet: View { return $mapping.rows[index] } - private func columnBinding(_ row: NewColumn) -> Binding { - guard let index = newColumns.firstIndex(where: { $0.id == row.id }) else { + private func columnBinding(_ row: NewTableColumn) -> Binding { + guard let index = newTable.columns.firstIndex(where: { $0.id == row.id }) else { return .constant(row) } - return $newColumns[index] + return $newTable.columns[index] } private var allMappingsIncluded: Binding { @@ -596,8 +604,8 @@ struct RowImportSheet: View { private var allColumnsIncluded: Binding { Binding( - get: { !newColumns.isEmpty && newColumns.allSatisfy(\.include) }, - set: { value in for index in newColumns.indices { newColumns[index].include = value } } + get: { newTable.includesEveryColumn }, + set: { newTable.setAllIncluded($0) } ) } @@ -630,16 +638,14 @@ struct RowImportSheet: View { ) } } - let names = newColumns - .filter { $0.include } - .map { $0.name.trimmingCharacters(in: .whitespaces).lowercased() } - if names.contains(where: \.isEmpty) { + switch newTable.problem { + case .unnamedColumn: return String(localized: "Every included column needs a name.") - } - if Set(names).count != names.count { + case .duplicateName: return String(localized: "Column names must be unique.") + case nil: + return nil } - return nil } } @@ -653,11 +659,12 @@ struct RowImportSheet: View { /// The selection has to be one of the options by exact spelling or the menu draws blank, and /// `typeOptions` suppresses its insert on a case-insensitive match. The getter resolves through /// the same comparison so a differently-cased stored type still selects its own row. - private func typeBinding(_ row: NewColumn) -> Binding { - let options = typeOptions(including: row.type) + private func typeBinding(_ row: NewTableColumn) -> Binding { + let type = row.settings.type + let options = typeOptions(including: type) return Binding( - get: { options.first { $0.caseInsensitiveCompare(row.type) == .orderedSame } ?? row.type }, - set: { columnBinding(row).type.wrappedValue = $0 } + get: { options.first { $0.caseInsensitiveCompare(type) == .orderedSame } ?? type }, + set: { columnBinding(row).settings.type.wrappedValue = $0 } ) } @@ -683,8 +690,7 @@ struct RowImportSheet: View { case .existingTable: return selectedTargetTable != nil && mapping.hasMappedField case .newTable: - return !newTableName.trimmingCharacters(in: .whitespaces).isEmpty - && newColumns.contains { $0.include && !$0.name.trimmingCharacters(in: .whitespaces).isEmpty } + return !newTableName.trimmingCharacters(in: .whitespaces).isEmpty && newTable.hasNamedColumn } } @@ -706,23 +712,25 @@ struct RowImportSheet: View { /// "Select a table…" and nothing else with no way to tell an empty database from an unreachable /// one, and no way to ask again. /// - /// Read through the browse scope, which is what the import itself writes to. The shared session - /// driver is wherever a tab's execution last pinned it and nothing puts it back, so reading from - /// it listed one database's tables and mapped their columns while the rows went to another's. + /// Read through the sheet's own scope, the browse scope it took on opening, which is what the + /// import writes to. The shared session driver is wherever a tab's execution last pinned it and + /// nothing puts it back, so reading from it listed one database's tables and mapped their columns + /// while the rows went to another's. @MainActor private func loadTables() async { guard !isLoadingTables else { return } isLoadingTables = true defer { isLoadingTables = false } - guard DatabaseManager.shared.browseScope(for: connection.id) != nil else { + if scope == nil { + scope = DatabaseManager.shared.browseScope(for: connection.id) + } + guard let scope else { catalogNameKeys = nil tableListError = String(localized: "This connection is not open.") return } do { - databaseObjects = try await DatabaseManager.shared.withBrowseMetadataDriver( - connectionId: connection.id - ) { driver in + databaseObjects = try await DatabaseManager.shared.withMetadataDriver(scope: scope) { driver in try await driver.fetchTables() } catalogNameKeys = NewTableNaming.comparisonKeys(for: databaseObjects.map(\.name)) @@ -746,7 +754,7 @@ struct RowImportSheet: View { @MainActor private func suggestNewTableName() { guard newTableName.isEmpty || newTableName == proposedTableName else { return } - let ours = NewTableNaming.comparisonKeys(for: createdTables.keys) + let ours = NewTableNaming.comparisonKeys(for: scope.map(newTablePlanner.createdTableNames(in:)) ?? []) let suggestion = NewTableNaming.suggestion( forFileNamed: fileURL.lastPathComponent, style: NewTableNameStyle.forDatabaseType(connection.type), @@ -773,7 +781,7 @@ struct RowImportSheet: View { let isExisting = destination == .existingTable return SourceRead( destination: destination, - scope: isExisting ? DatabaseManager.shared.browseScope(for: connection.id) : nil, + scope: isExisting ? scope : nil, table: isExisting ? selectedTargetTable : nil, signature: detectionSignature, attempt: readAttempts[destination, default: 0] @@ -821,20 +829,8 @@ struct RowImportSheet: View { let fields = try await Self.detectFields(plugin: plugin, at: fileURL, targetTable: nil) guard !Task.isCancelled else { return } let serverVersion = DatabaseManager.shared.driver(for: connection.id)?.serverVersion - newColumns = fields.map { field in - NewColumn( - field: field, - include: true, - name: field.name, - type: ImportTypeMapper.sqlType( - for: field.inferredType, - databaseType: connection.type, - serverVersion: serverVersion - ), - isPrimaryKey: false, - isNullable: true, - defaultValue: "" - ) + newTable.load(fields: fields) { inferredType in + ImportTypeMapper.sqlType(for: inferredType, databaseType: connection.type, serverVersion: serverVersion) } lastNewColumnsRead = request } catch { @@ -860,7 +856,7 @@ struct RowImportSheet: View { guard !Task.isCancelled else { return } let fields = try await Self.detectFields(plugin: plugin, at: fileURL, targetTable: table) guard !Task.isCancelled else { return } - mapping.load(fields: fields, columns: columns, for: Self.tableScope(table, in: scope), read: request) + mapping.load(fields: fields, columns: columns, for: TableScope(table: table, in: scope), read: request) } catch { guard !Task.isCancelled else { return } loadError = error.localizedDescription @@ -869,14 +865,10 @@ struct RowImportSheet: View { isLoadingContext = false } - private static func tableScope(_ table: String, in scope: DatabaseScope) -> TableScope { - TableScope(connectionId: scope.connectionId, database: scope.database, schema: scope.schema, table: table) - } - // MARK: - Import private func performImport() { - guard let scope = DatabaseManager.shared.browseScope(for: connection.id) else { + guard let scope else { importError = DatabaseError.notConnected showErrorDialog = true return @@ -902,13 +894,13 @@ struct RowImportSheet: View { ) case .newTable: let name = newTableName.trimmingCharacters(in: .whitespaces) - guard !name.isEmpty, let definition = newTableDefinition(tableName: name) else { + guard !name.isEmpty, let definition = newTable.definition(tableName: name) else { importError = Self.createTableStatementError showErrorDialog = true return } - let columnMapping = newTableMapping() - let fields = newColumns.map(\.field.name) + let columnMapping = newTable.columnMapping + let fields = newTable.fields runImport( ImportPlan( targetTable: name, @@ -929,43 +921,6 @@ struct RowImportSheet: View { ) } - private func newTableMapping() -> [String: String] { - var mapping: [String: String] = [:] - for column in newColumns where column.include && !column.name.trimmingCharacters(in: .whitespaces).isEmpty { - mapping[column.field.name] = column.name - } - return mapping - } - - private func newTableDefinition(tableName: String) -> PluginCreateTableDefinition? { - let included = newColumns.filter { - $0.include - && !$0.name.trimmingCharacters(in: .whitespaces).isEmpty - && !$0.type.trimmingCharacters(in: .whitespaces).isEmpty - } - guard !included.isEmpty else { return nil } - - return PluginCreateTableDefinition( - tableName: tableName, - columns: included.map { column in - PluginColumnDefinition( - name: column.name, - dataType: column.type, - isNullable: column.isNullable, - defaultValue: column.defaultValue.isEmpty ? nil : column.defaultValue, - isPrimaryKey: column.isPrimaryKey, - autoIncrement: false, - comment: nil, - unsigned: false, - onUpdate: nil, - charset: nil, - collation: nil - ) - }, - primaryKeyColumns: included.filter(\.isPrimaryKey).map(\.name) - ) - } - private func runImport(_ plan: ImportPlan, scope: DatabaseScope) { let service = ImportService(connection: connection) importService = service @@ -981,7 +936,7 @@ struct RowImportSheet: View { fields: plan.fields, columns: plan.columns, columnMapping: plan.columnMapping, - in: Self.tableScope(targetTable, in: scope) + in: TableScope(table: targetTable, in: scope) ) let result = try await service.importFile( from: fileURL, @@ -1031,6 +986,7 @@ struct RowImportSheet: View { )) } let tableName = definition.tableName + let table = TableScope(table: tableName, in: scope) let statements = try await DatabaseManager.shared.createTableStatements( definition: definition, scope: scope, @@ -1038,12 +994,10 @@ struct RowImportSheet: View { ) guard !statements.isEmpty else { throw Self.createTableStatementError } let sql = statements.joined(separator: "\n") - switch NewTableImportPlanner.plan( - forTable: tableName, createTableSQL: sql, alreadyCreated: createdTables - ) { + switch newTablePlanner.plan(forTable: table, createTableSQL: sql) { case .create: try await createTable(statements: statements, scope: scope) - createdTables[tableName] = sql + newTablePlanner.recordCreated(table, createTableSQL: sql) case .reuseAfterClearing: try await clearRows(of: tableName, scope: scope) case .nameTakenWithDifferentColumns: diff --git a/TableProTests/Core/Services/ImportServiceHistoryTests.swift b/TableProTests/Core/Services/ImportServiceHistoryTests.swift new file mode 100644 index 0000000000..55910620e1 --- /dev/null +++ b/TableProTests/Core/Services/ImportServiceHistoryTests.swift @@ -0,0 +1,74 @@ +// +// ImportServiceHistoryTests.swift +// TableProTests +// + +import Foundation +@testable import TablePro +import TableProPluginKit +import Testing + +private final class UnusedImportPlugin: ImportFormatPlugin { + static let pluginName = "Unused Import" + static let pluginVersion = "1.0" + static let pluginDescription = "An import format whose import never starts" + static let formatId = "tablepro-tests-unused-import" + static let formatDisplayName = "Unused" + static let acceptedFileExtensions = ["csv"] + static let iconName = "tablecells" + static let requiresTargetTable = true + + required init() {} + + func performImport( + source: any PluginImportSource, + sink: any PluginImportDataSink, + progress: PluginImportProgress + ) async throws -> PluginImportResult { + PluginImportResult(executedStatements: 0, executionTime: 0) + } +} + +private actor RecordingHistory: QueryHistoryRecording { + private(set) var requests: [QueryHistoryRecordRequest] = [] + + func record(_ request: QueryHistoryRecordRequest) async -> Bool { + requests.append(request) + return true + } +} + +/// An import runs in the scope it was handed, and its history row has to name that database. Reading +/// the browse database when the import ended named wherever another window had moved the connection +/// in the meantime. +@MainActor +struct ImportServiceHistoryTests { + @Test("An import is recorded under the database it ran in, not the one the connection browses") + func historyNamesTheScopeDatabase() async throws { + let formatId = UnusedImportPlugin.formatId + PluginManager.shared.importPlugins[formatId] = UnusedImportPlugin() + defer { PluginManager.shared.importPlugins[formatId] = nil } + + let connection = TestFixtures.makeConnection(database: "shop") + let history = RecordingHistory() + let service = ImportService(connection: connection, historyRecorder: history) + let scope = DatabaseScope(connectionId: connection.id, database: "archive", schema: nil) + let file = FileManager.default.temporaryDirectory + .appendingPathComponent("import-history-\(UUID().uuidString)") + .appendingPathExtension("csv") + try Data("id\n1\n".utf8).write(to: file) + defer { try? FileManager.default.removeItem(at: file) } + + await #expect(throws: (any Error).self) { + try await service.importFile( + from: file, + formatId: formatId, + encoding: .utf8, + scope: scope, + targetTable: "orders" + ) + } + + #expect(await history.requests.map(\.databaseName) == ["archive"]) + } +} diff --git a/TableProTests/Core/Services/NewTableImportPlannerTests.swift b/TableProTests/Core/Services/NewTableImportPlannerTests.swift index 24c9796cad..7bb842fd03 100644 --- a/TableProTests/Core/Services/NewTableImportPlannerTests.swift +++ b/TableProTests/Core/Services/NewTableImportPlannerTests.swift @@ -3,6 +3,7 @@ // TableProTests // +import Foundation @testable import TablePro import Testing @@ -11,13 +12,29 @@ import Testing /// first attempt kept a second time. struct NewTableImportPlannerTests { private let createSQL = "CREATE TABLE people (name TEXT)" + private let connectionId = UUID() + + private var shop: DatabaseScope { + DatabaseScope(connectionId: connectionId, database: "shop", schema: nil) + } + + private var archive: DatabaseScope { + DatabaseScope(connectionId: connectionId, database: "archive", schema: nil) + } + + private func planner(creating tables: [TableScope: String]) -> NewTableImportPlanner { + var planner = NewTableImportPlanner() + for (table, sql) in tables { + planner.recordCreated(table, createTableSQL: sql) + } + return planner + } @Test("A table this sheet has not created is created") func firstAttemptCreates() { #expect( - NewTableImportPlanner.plan( - forTable: "people", createTableSQL: createSQL, alreadyCreated: [:] - ) == .create + NewTableImportPlanner().plan(forTable: TableScope(table: "people", in: shop), createTableSQL: createSQL) + == .create ) } @@ -25,10 +42,10 @@ struct NewTableImportPlannerTests { /// can lose nothing but the failed attempt's own rows. @Test("A retry with the same columns reuses the table after clearing it") func retryReusesAfterClearing() { + let people = TableScope(table: "people", in: shop) #expect( - NewTableImportPlanner.plan( - forTable: "people", createTableSQL: createSQL, alreadyCreated: ["people": createSQL] - ) == .reuseAfterClearing + planner(creating: [people: createSQL]).plan(forTable: people, createTableSQL: createSQL) + == .reuseAfterClearing ) } @@ -37,11 +54,11 @@ struct NewTableImportPlannerTests { /// columns, so the name is reported instead. @Test("A retry after editing the columns refuses the name") func retryAfterEditingColumnsRefusesTheName() { + let people = TableScope(table: "people", in: shop) #expect( - NewTableImportPlanner.plan( - forTable: "people", - createTableSQL: "CREATE TABLE people (name TEXT, email TEXT)", - alreadyCreated: ["people": createSQL] + planner(creating: [people: createSQL]).plan( + forTable: people, + createTableSQL: "CREATE TABLE people (name TEXT, email TEXT)" ) == .nameTakenWithDifferentColumns ) } @@ -49,25 +66,55 @@ struct NewTableImportPlannerTests { /// Renaming away and back is the case a single remembered name gets wrong: `t1` is still ours. @Test("A name created earlier is still recognised after other names were used") func earlierNameIsStillRecognised() { - let created = [ - "t1": "CREATE TABLE t1 (a TEXT)", - "t2": "CREATE TABLE t2 (a TEXT)", - ] - #expect( - NewTableImportPlanner.plan( - forTable: "t1", createTableSQL: "CREATE TABLE t1 (a TEXT)", alreadyCreated: created - ) == .reuseAfterClearing - ) + let first = TableScope(table: "t1", in: shop) + let created = planner(creating: [ + first: "CREATE TABLE t1 (a TEXT)", + TableScope(table: "t2", in: shop): "CREATE TABLE t2 (a TEXT)" + ]) + #expect(created.plan(forTable: first, createTableSQL: "CREATE TABLE t1 (a TEXT)") == .reuseAfterClearing) } @Test("A name this sheet never created is created even when others were") func unseenNameIsCreated() { + let created = planner(creating: [TableScope(table: "t1", in: shop): "CREATE TABLE t1 (a TEXT)"]) #expect( - NewTableImportPlanner.plan( - forTable: "t3", - createTableSQL: "CREATE TABLE t3 (a TEXT)", - alreadyCreated: ["t1": "CREATE TABLE t1 (a TEXT)"] - ) == .create + created.plan(forTable: TableScope(table: "t3", in: shop), createTableSQL: "CREATE TABLE t3 (a TEXT)") + == .create ) } + + /// The table a failed import made in `shop` does not make `archive.people` the sheet's own. Keyed + /// by name alone, the retry planned `reuseAfterClearing` for it and ran `DELETE FROM` on a table + /// the user owns. + @Test("A table created in one database does not make a same-named table in another ours") + func sameNameInAnotherDatabaseIsNotOurs() { + let created = planner(creating: [TableScope(table: "people", in: shop): createSQL]) + let archived = TableScope(table: "people", in: archive) + + #expect(created.plan(forTable: archived, createTableSQL: createSQL) == .create) + #expect(!created.created(archived)) + #expect(created.created(TableScope(table: "people", in: shop))) + } + + @Test("A table created in one schema does not make a same-named table in another ours") + func sameNameInAnotherSchemaIsNotOurs() { + let publicSchema = DatabaseScope(connectionId: connectionId, database: "shop", schema: "public") + let staging = DatabaseScope(connectionId: connectionId, database: "shop", schema: "staging") + let created = planner(creating: [TableScope(table: "people", in: publicSchema): createSQL]) + + #expect(created.plan(forTable: TableScope(table: "people", in: staging), createTableSQL: createSQL) == .create) + } + + @Test("Only the names created in the asked scope are listed") + func createdNamesAreListedPerScope() { + let created = planner(creating: [ + TableScope(table: "people", in: shop): createSQL, + TableScope(table: "orders", in: shop): "CREATE TABLE orders (id INTEGER)", + TableScope(table: "invoices", in: archive): "CREATE TABLE invoices (id INTEGER)" + ]) + + #expect(Set(created.createdTableNames(in: shop)) == ["people", "orders"]) + #expect(created.createdTableNames(in: archive) == ["invoices"]) + #expect(created.createdTableNames(in: DatabaseScope(connectionId: UUID(), database: "shop", schema: nil)).isEmpty) + } } diff --git a/TableProTests/Views/Import/NewTableDraftTests.swift b/TableProTests/Views/Import/NewTableDraftTests.swift new file mode 100644 index 0000000000..9f2a1045f4 --- /dev/null +++ b/TableProTests/Views/Import/NewTableDraftTests.swift @@ -0,0 +1,212 @@ +// +// NewTableDraftTests.swift +// TableProTests +// + +import Foundation +import TableProPluginKit +import Testing + +@testable import TablePro + +/// Changing a parsing option reads the file again. Rebuilding the new table's columns from that read +/// threw away every rename, type, key, nullability, default and exclusion the user had set. +struct NewTableDraftTests { + private static func sqlType(_ type: PluginImportFieldType) -> String { + switch type { + case .integer: return "INTEGER" + case .real: return "REAL" + default: return "TEXT" + } + } + + private func field(_ name: String, _ type: PluginImportFieldType = .text) -> PluginImportField { + PluginImportField(name: name, sampleValue: nil, inferredType: type) + } + + private func makeDraft(_ fields: [PluginImportField]) -> NewTableDraft { + var draft = NewTableDraft() + draft.load(fields: fields, proposingType: Self.sqlType) + return draft + } + + private func edit( + _ name: String, + in draft: inout NewTableDraft, + _ change: (inout NewTableColumnSettings) -> Void + ) throws { + let index = try #require(draft.columns.firstIndex { $0.field.name == name }) + change(&draft.columns[index].settings) + } + + private func settings(of name: String, in draft: NewTableDraft) throws -> NewTableColumnSettings { + try #require(draft.columns.first { $0.field.name == name }).settings + } + + @Test("The first read proposes every field under its own name, nullable, with no key and no default") + func firstReadProposesEachField() throws { + let draft = makeDraft([field("id", .integer), field("name")]) + + #expect(try settings(of: "id", in: draft) == .proposed(name: "id", type: "INTEGER")) + #expect(try settings(of: "name", in: draft) == NewTableColumnSettings( + include: true, name: "name", type: "TEXT", isPrimaryKey: false, isNullable: true, defaultValue: "" + )) + } + + @Test("A re-read keeps every setting the user changed on a field that survives it") + func reReadKeepsEdits() throws { + var draft = makeDraft([field("id", .integer), field("name"), field("notes")]) + try edit("id", in: &draft) { + $0.name = "person_id" + $0.type = "BIGINT" + $0.isPrimaryKey = true + $0.isNullable = false + } + try edit("name", in: &draft) { $0.defaultValue = "'unknown'" } + try edit("notes", in: &draft) { $0.include = false } + + draft.load(fields: [field("id", .integer), field("name"), field("notes")], proposingType: Self.sqlType) + + #expect(try settings(of: "id", in: draft) == NewTableColumnSettings( + include: true, name: "person_id", type: "BIGINT", isPrimaryKey: true, isNullable: false, defaultValue: "" + )) + #expect(try settings(of: "name", in: draft).defaultValue == "'unknown'") + #expect(try settings(of: "notes", in: draft).include == false) + } + + /// Trimming spaces turns `" 42 "` into a number, so a type the user never touched has to follow + /// the read rather than stay at what the untrimmed values suggested. + @Test("A type the user left alone follows the type the new read infers") + func untouchedTypeFollowsTheRead() throws { + var draft = makeDraft([field("amount"), field("code")]) + try edit("code", in: &draft) { $0.isPrimaryKey = true } + + draft.load(fields: [field("amount", .integer), field("code", .integer)], proposingType: Self.sqlType) + + #expect(try settings(of: "amount", in: draft).type == "INTEGER") + #expect(try settings(of: "code", in: draft).type == "INTEGER") + #expect(try settings(of: "code", in: draft).isPrimaryKey) + } + + @Test("A type the user chose stays when the new read infers another") + func chosenTypeStays() throws { + var draft = makeDraft([field("amount")]) + try edit("amount", in: &draft) { $0.type = "NUMERIC(10,2)" } + + draft.load(fields: [field("amount", .integer)], proposingType: Self.sqlType) + + #expect(try settings(of: "amount", in: draft).type == "NUMERIC(10,2)") + } + + /// The type menu offers the dialect's own spelling of a proposal, so choosing it again can write + /// back `integer` for a proposed `INTEGER` without the user changing anything. + @Test("A type differing from the proposal only in case counts as left alone") + func caseOnlyTypeChangeIsNotAnEdit() throws { + var draft = makeDraft([field("amount", .integer)]) + try edit("amount", in: &draft) { $0.type = "integer" } + + draft.load(fields: [field("amount", .real)], proposingType: Self.sqlType) + + #expect(try settings(of: "amount", in: draft).type == "REAL") + } + + @Test("A setting changed and then put back follows the read like one never changed") + func revertedSettingFollowsTheRead() throws { + var draft = makeDraft([field("amount")]) + try edit("amount", in: &draft) { $0.type = "BIGINT" } + try edit("amount", in: &draft) { $0.type = "TEXT" } + + draft.load(fields: [field("amount", .integer)], proposingType: Self.sqlType) + + #expect(try settings(of: "amount", in: draft).type == "INTEGER") + } + + /// The proposal a later read compares against is the one that read made, not the first one, so + /// the user's edit is judged against what the sheet was showing when it was made. + @Test("An edit survives several re-reads, and a later read's own proposals stay unedited") + func editsSurviveSeveralReads() throws { + var draft = makeDraft([field("amount"), field("code")]) + try edit("code", in: &draft) { $0.name = "sku" } + + draft.load(fields: [field("amount", .integer), field("code")], proposingType: Self.sqlType) + draft.load(fields: [field("amount", .real), field("code")], proposingType: Self.sqlType) + + #expect(try settings(of: "amount", in: draft).type == "REAL") + #expect(try settings(of: "code", in: draft).name == "sku") + } + + @Test("Fields follow the new read: a vanished one is dropped, a new one is proposed, order is the file's") + func fieldsFollowTheNewRead() throws { + var draft = makeDraft([field("a"), field("b"), field("c")]) + try edit("b", in: &draft) { $0.name = "renamed" } + try edit("c", in: &draft) { $0.isPrimaryKey = true } + + draft.load(fields: [field("d"), field("b"), field("a")], proposingType: Self.sqlType) + + #expect(draft.fields == ["d", "b", "a"]) + #expect(try settings(of: "d", in: draft) == .proposed(name: "d", type: "TEXT")) + #expect(try settings(of: "b", in: draft).name == "renamed") + } + + @Test("The CREATE covers the included, named columns with their keys and defaults") + func definitionCoversIncludedColumns() throws { + var draft = makeDraft([field("id", .integer), field("name"), field("skip"), field("blank")]) + try edit("id", in: &draft) { + $0.isPrimaryKey = true + $0.isNullable = false + } + try edit("name", in: &draft) { $0.defaultValue = "'x'" } + try edit("skip", in: &draft) { $0.include = false } + try edit("blank", in: &draft) { $0.name = " " } + + let definition = try #require(draft.definition(tableName: "people")) + + #expect(definition.tableName == "people") + #expect(definition.columns.map(\.name) == ["id", "name"]) + #expect(definition.columns.map(\.dataType) == ["INTEGER", "TEXT"]) + #expect(definition.columns.map(\.isNullable) == [false, true]) + #expect(definition.columns.map(\.defaultValue) == [nil, "'x'"]) + #expect(definition.primaryKeyColumns == ["id"]) + #expect(draft.columnMapping == ["id": "id", "name": "name"]) + #expect(draft.fields == ["id", "name", "skip", "blank"]) + } + + @Test("Nothing to create when no included column has a name") + func noDefinitionWithoutANamedColumn() throws { + var draft = makeDraft([field("a")]) + try edit("a", in: &draft) { $0.include = false } + + #expect(draft.definition(tableName: "t") == nil) + #expect(!draft.hasNamedColumn) + #expect(draft.columnMapping.isEmpty) + } + + @Test("An included column without a name, or two sharing one in any case, is a problem") + func columnNameProblems() throws { + var draft = makeDraft([field("a"), field("b")]) + #expect(draft.problem == nil) + + try edit("b", in: &draft) { $0.name = " " } + #expect(draft.problem == .unnamedColumn) + + try edit("b", in: &draft) { $0.name = "A" } + #expect(draft.problem == .duplicateName) + + try edit("b", in: &draft) { $0.include = false } + #expect(draft.problem == nil) + } + + @Test("Including every column is one switch, and it reads as on only when every column is in") + func includeEveryColumn() throws { + var draft = makeDraft([field("a"), field("b")]) + #expect(draft.includesEveryColumn) + + draft.setAllIncluded(false) + #expect(!draft.includesEveryColumn) + #expect(draft.columns.allSatisfy { !$0.settings.include }) + + draft.setAllIncluded(true) + #expect(draft.includesEveryColumn) + #expect(!NewTableDraft().includesEveryColumn) + } +} diff --git a/TableProUITests/RowImportNewTableEditsUITests.swift b/TableProUITests/RowImportNewTableEditsUITests.swift new file mode 100644 index 0000000000..350648eb6f --- /dev/null +++ b/TableProUITests/RowImportNewTableEditsUITests.swift @@ -0,0 +1,102 @@ +import XCTest + +/// A new table's column edits outlive a parsing option change, while what the user left alone follows +/// the new read. +/// +/// `GenreId` holds ` 9001 `, which reads as text until **Trim leading and trailing spaces** is on and +/// as a number after it, so the type menu of the untouched column is what shows the file was read +/// again. The key set on `Title` is the edit the read used to throw away. The file reaches the sheet +/// through the data file window's Import into Table, the one route that needs no open panel, and every +/// query is rooted at a window or its sheet because the sample database's grid publishes thousands of +/// elements. +final class RowImportNewTableEditsUITests: UITestCase { + private let genres = Data("GenreId,Title\n 9001 ,Imported genre\n".utf8) + + func testAColumnEditSurvivesAParsingOptionChange() throws { + let app = try launchWithSampleAndFile() + let dataWindow = app.windows.matching(identifier: "main-data-file").firstMatch + XCTAssertTrue(dataWindow.waitToExist(timeout: 30), "The CSV produced no data file window") + let connectionWindow = app.windows + .matching(NSPredicate(format: "identifier != %@", "main-data-file")) + .firstMatch + XCTAssertTrue( + waitForPredicate(timeout: 30) { + objectBrowser(in: connectionWindow).descendants(matching: .staticText).firstMatch.exists + }, + "The sample database never finished opening" + ) + + let sheet = try openImportSheet(from: dataWindow, into: connectionWindow, in: app) + let newTable = sheet.radioButtons["New table"].firstMatch + XCTAssertTrue(waitUntilHittable(newTable, timeout: 10), "The sheet must offer a new table") + newTable.click() + + let titleKey = sheet.checkBoxes["Title is a primary key"].firstMatch + XCTAssertTrue(waitUntilHittable(titleKey, timeout: 20), "The new table must list the Title column") + titleKey.click() + XCTAssertTrue( + waitForPredicate(timeout: 5) { (titleKey.value as? Int) == 1 }, + "Title must become the primary key" + ) + + let idType = sheet.popUpButtons["Type of GenreId"].firstMatch + XCTAssertTrue(idType.waitToExist(timeout: 10), "The new table must list the GenreId column") + XCTAssertEqual(typeName(of: idType), "TEXT", "Untrimmed, ' 9001 ' reads as text") + + let trim = sheet.checkBoxes["Trim leading and trailing spaces"].firstMatch + XCTAssertTrue(waitUntilHittable(trim, timeout: 10), "The CSV options must be in the sheet") + trim.click() + + XCTAssertTrue( + waitForPredicate(timeout: 20) { typeName(of: idType) == "INTEGER" }, + "Trimming must read the file again and propose a number for the untouched GenreId column" + ) + XCTAssertEqual(titleKey.value as? Int, 1, "The key set on Title must survive the new read") + + sheet.buttons["Cancel"].firstMatch.click() + } + + private func typeName(of popUp: XCUIElement) -> String? { + (popUp.value as? String)?.uppercased() + } + + private func launchWithSampleAndFile() throws -> XCUIApplication { + let root = try XCTUnwrap(sandboxRoot, "setUpWithError did not prepare a sandbox") + let fileURL = root.appendingPathComponent("genres.csv") + try genres.write(to: fileURL) + return try launchApp(environment: [ + "TABLEPRO_UI_TEST_OPEN_SAMPLE": "1", + "TABLEPRO_UI_TEST_OPEN_FILE": fileURL.path + ]) + } + + private func openImportSheet( + from dataWindow: XCUIElement, + into connectionWindow: XCUIElement, + in app: XCUIApplication + ) throws -> XCUIElement { + let grid = dataWindow.tables.matching(identifier: "data-grid").firstMatch + XCTAssertTrue(grid.waitToExist(timeout: 30), "The data file window has no grid") + XCTAssertTrue(waitForClickableRows(in: grid), "The data file must load rows") + grid.coordinate(withNormalizedOffset: .zero) + .withOffset(CGVector(dx: 80, dy: grid.tableRows.firstMatch.frame.midY - grid.frame.minY)) + .click() + + let edit = app.menuBars.menuBarItems["Edit"] + edit.click() + let data = edit.menus.menuItems["Data"].firstMatch + XCTAssertTrue(data.waitToExist(timeout: 5), "Edit must carry the Data submenu") + data.hover() + let importItem = data.menus.menuItems["Import into Table…"].firstMatch + XCTAssertTrue(importItem.waitToExist(timeout: 5), "Data must offer Import into Table") + importItem.click() + + let proceed = dataWindow.sheets.buttons["Continue"].firstMatch + XCTAssertTrue(proceed.waitToExist(timeout: 15), "Import into Table must ask for a connection") + proceed.click() + + let sheet = connectionWindow.sheets.firstMatch + XCTAssertTrue(sheet.waitToExist(timeout: 30), "The import sheet must open in the connection window") + return sheet + } +} diff --git a/docs/features/import-export.mdx b/docs/features/import-export.mdx index c527da14ca..f23c443b1f 100644 --- a/docs/features/import-export.mdx +++ b/docs/features/import-export.mdx @@ -325,6 +325,8 @@ The sheet accepts an array of objects `[{…}, {…}]`, newline-delimited JSON s The proposed name drops the extension, turns spaces and punctuation into underscores, and lowercases the result. Letters from any script are kept as they are. On Oracle the name comes through in upper case instead, and is cut to 30 bytes rather than 63. A name an existing table or view already holds gains a numeric suffix, so re-importing `users.csv` next to a `users` table proposes `users_2`. Whatever you type over it is held to the same rules: a name already taken, one longer than the engine allows, or one starting with a prefix the engine keeps for itself is reported in the sheet, with **Import** off until it changes. +The table list, the new table and the rows all belong to the database and schema the connection was on when the sheet opened. Switching database in another window while the sheet is open leaves the import where it was. + Rows insert through parameterized statements, so a JSON value is never concatenated into SQL. Nested objects and arrays are stored as JSON text. In a new PostgreSQL table their column is `jsonb`, or `json` on 9.2 and 9.3, and `text` on 9.1, which has no JSON type. ### Import XLSX @@ -344,7 +346,7 @@ The workbook is read whole rather than streamed, because a sheet's rows refer ba ### Import CSV -CSV and TSV open the same sheet as JSON, with parsing options in front of the mapping. The delimiter and encoding are detected from the file. Changing any option reads the file again: a field that is still there keeps the column it was mapped to, and a new table's column list is rebuilt. +CSV and TSV open the same sheet as JSON, with parsing options in front of the mapping. The delimiter and encoding are detected from the file. Changing any option reads the file again. A field that is still there keeps the column it was mapped to. In a new table it keeps each column setting you changed, and the ones you left alone follow the new read, such as a type inferred again from trimmed values. | Option | What it does | Default | |--------|-------------|---------| From 69c4880956bfa20d538502d48a4334dd59c6d759 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 29 Sep 2026 21:18:56 +0700 Subject: [PATCH 2/2] fix(plugins): keep a new table column setting the user picked when a read proposes the same value or misses the field --- TablePro/Views/Import/NewTableDraft.swift | 95 ++++++++++++++----- .../Views/Import/NewTableDraftTests.swift | 65 +++++++++++-- docs/features/import-export.mdx | 2 +- 3 files changed, 131 insertions(+), 31 deletions(-) diff --git a/TablePro/Views/Import/NewTableDraft.swift b/TablePro/Views/Import/NewTableDraft.swift index ddc347f62e..b62208d4be 100644 --- a/TablePro/Views/Import/NewTableDraft.swift +++ b/TablePro/Views/Import/NewTableDraft.swift @@ -27,22 +27,46 @@ internal struct NewTableColumnSettings: Equatable { defaultValue: "" ) } +} + +/// What the user set on one column, each setting nil until a write changes it. A read never touches +/// it, so a setting stays the user's even when a later read proposes the same value. +internal struct NewTableColumnEdits: Equatable { + internal var include: Bool? + internal var name: String? + internal var type: String? + internal var isPrimaryKey: Bool? + internal var isNullable: Bool? + internal var defaultValue: String? + + internal var isEmpty: Bool { + self == NewTableColumnEdits() + } - /// A setting still equal to what the sheet proposed was never changed, so it takes the new - /// proposal. One the user moved away from the proposal is theirs and stays. A type that differs - /// from the proposal only in case is the proposal, because the type menu offers the dialect's - /// own spelling of it. - internal func keepingEdits( - madeTo earlier: NewTableColumnSettings, - over proposal: NewTableColumnSettings - ) -> NewTableColumnSettings { + internal func applied(to proposal: NewTableColumnSettings) -> NewTableColumnSettings { NewTableColumnSettings( - include: include == earlier.include ? proposal.include : include, - name: name == earlier.name ? proposal.name : name, - type: type.caseInsensitiveCompare(earlier.type) == .orderedSame ? proposal.type : type, - isPrimaryKey: isPrimaryKey == earlier.isPrimaryKey ? proposal.isPrimaryKey : isPrimaryKey, - isNullable: isNullable == earlier.isNullable ? proposal.isNullable : isNullable, - defaultValue: defaultValue == earlier.defaultValue ? proposal.defaultValue : defaultValue + include: include ?? proposal.include, + name: name ?? proposal.name, + type: type ?? proposal.type, + isPrimaryKey: isPrimaryKey ?? proposal.isPrimaryKey, + isNullable: isNullable ?? proposal.isNullable, + defaultValue: defaultValue ?? proposal.defaultValue + ) + } + + /// A type differing only in case is not a change: the type menu offers the dialect's own + /// spelling of the type on show, and choosing it again writes that spelling back. + internal func recording( + _ settings: NewTableColumnSettings, + over shown: NewTableColumnSettings + ) -> NewTableColumnEdits { + NewTableColumnEdits( + include: settings.include == shown.include ? include : settings.include, + name: settings.name == shown.name ? name : settings.name, + type: settings.type.caseInsensitiveCompare(shown.type) == .orderedSame ? type : settings.type, + isPrimaryKey: settings.isPrimaryKey == shown.isPrimaryKey ? isPrimaryKey : settings.isPrimaryKey, + isNullable: settings.isNullable == shown.isNullable ? isNullable : settings.isNullable, + defaultValue: settings.defaultValue == shown.defaultValue ? defaultValue : settings.defaultValue ) } } @@ -51,12 +75,26 @@ internal struct NewTableColumnSettings: Equatable { internal struct NewTableColumn: Identifiable { internal let field: PluginImportField - /// What the sheet offered for this field on the read that produced it, so the next read can tell - /// the user's changes from the sheet's own guesses. + /// What the latest read proposes for this field. internal let proposal: NewTableColumnSettings - internal var settings: NewTableColumnSettings + internal private(set) var edits: NewTableColumnEdits + + internal init( + field: PluginImportField, + proposal: NewTableColumnSettings, + edits: NewTableColumnEdits = NewTableColumnEdits() + ) { + self.field = field + self.proposal = proposal + self.edits = edits + } internal var id: String { field.name } + + internal var settings: NewTableColumnSettings { + get { edits.applied(to: proposal) } + set { edits = edits.recording(newValue, over: settings) } + } } internal enum NewTableColumnProblem: Equatable { @@ -68,22 +106,31 @@ internal enum NewTableColumnProblem: Equatable { /// what the user changed. /// /// Changing a parsing option reads the file again, and rebuilding the columns from that read threw -/// away every rename, type, key, nullability, default and exclusion the user had set. A field that -/// survives the read keeps each setting the user changed, and each setting left as proposed follows -/// the read, the way the sheet's table name keeps what the user typed over its own suggestion. +/// away every rename, type, key, nullability, default and exclusion the user had set. Each field +/// keeps the settings the user set, and each setting left alone follows the read, the way the +/// sheet's table name keeps what the user typed over its own suggestion. internal struct NewTableDraft { internal var columns: [NewTableColumn] = [] + /// A wrong delimiter reads other fields for one read, and correcting it brings these back. + private var setAsideEdits: [String: NewTableColumnEdits] = [:] + internal mutating func load( fields: [PluginImportField], proposingType proposedType: (PluginImportFieldType) -> String ) { - let earlier = Dictionary(columns.map { ($0.id, $0) }, uniquingKeysWith: { first, _ in first }) + var edits = setAsideEdits + for column in columns { + edits[column.id] = column.edits + } columns = fields.map { field in - let proposal = NewTableColumnSettings.proposed(name: field.name, type: proposedType(field.inferredType)) - let settings = earlier[field.name].map { $0.settings.keepingEdits(madeTo: $0.proposal, over: proposal) } - return NewTableColumn(field: field, proposal: proposal, settings: settings ?? proposal) + NewTableColumn( + field: field, + proposal: .proposed(name: field.name, type: proposedType(field.inferredType)), + edits: edits.removeValue(forKey: field.name) ?? NewTableColumnEdits() + ) } + setAsideEdits = edits.filter { !$0.value.isEmpty } } internal var includesEveryColumn: Bool { diff --git a/TableProTests/Views/Import/NewTableDraftTests.swift b/TableProTests/Views/Import/NewTableDraftTests.swift index 9f2a1045f4..1a23d6ac4b 100644 --- a/TableProTests/Views/Import/NewTableDraftTests.swift +++ b/TableProTests/Views/Import/NewTableDraftTests.swift @@ -110,19 +110,72 @@ struct NewTableDraftTests { #expect(try settings(of: "amount", in: draft).type == "REAL") } - @Test("A setting changed and then put back follows the read like one never changed") - func revertedSettingFollowsTheRead() throws { + /// Nothing on screen tells a type the user picked from one the sheet proposed, so a type picked + /// last is the one kept, whether or not it is also what the sheet had proposed. + @Test("A setting changed and then put back to the proposal stays what the user picked") + func settingPutBackStaysPicked() throws { var draft = makeDraft([field("amount")]) try edit("amount", in: &draft) { $0.type = "BIGINT" } try edit("amount", in: &draft) { $0.type = "TEXT" } draft.load(fields: [field("amount", .integer)], proposingType: Self.sqlType) - #expect(try settings(of: "amount", in: draft).type == "INTEGER") + #expect(try settings(of: "amount", in: draft).type == "TEXT") + } + + @Test("Writing back what the sheet shows records no edit") + func writingBackTheShownSettingsIsNotAnEdit() throws { + var draft = makeDraft([field("amount", .integer)]) + try edit("amount", in: &draft) { + $0.type = "INTEGER" + $0.include = true + $0.isNullable = true + } + draft.setAllIncluded(true) + + #expect(draft.columns.allSatisfy { $0.edits.isEmpty }) + } + + /// ` 42 ` reads as text untrimmed and as a number trimmed, so Trim on and then off again. + @Test("A type the user chose stays after a read proposes that same type and a later read another") + func chosenTypeOutlivesAMatchingProposal() throws { + var draft = makeDraft([field("code")]) + try edit("code", in: &draft) { $0.type = "INTEGER" } + + draft.load(fields: [field("code", .integer)], proposingType: Self.sqlType) + #expect(try settings(of: "code", in: draft).type == "INTEGER") + + draft.load(fields: [field("code")], proposingType: Self.sqlType) + #expect(try settings(of: "code", in: draft).type == "INTEGER") + } + + /// A `;` picked by mistake on a comma-separated file reads one field, `id,name,notes`, and picking + /// `,` again brings the three back. + @Test("A field one read misses gets its edits back when a later read has it again") + func editsReturnWithAFieldOneReadMissed() throws { + var draft = makeDraft([field("id", .integer), field("name"), field("notes")]) + try edit("id", in: &draft) { + $0.name = "person_id" + $0.isPrimaryKey = true + } + try edit("notes", in: &draft) { $0.include = false } + + draft.load(fields: [field("id,name,notes")], proposingType: Self.sqlType) + #expect(draft.fields == ["id,name,notes"]) + #expect(try settings(of: "id,name,notes", in: draft) == .proposed(name: "id,name,notes", type: "TEXT")) + try edit("id,name,notes", in: &draft) { $0.name = "everything" } + + draft.load(fields: [field("id", .integer), field("name"), field("notes")], proposingType: Self.sqlType) + #expect(try settings(of: "id", in: draft) == NewTableColumnSettings( + include: true, name: "person_id", type: "INTEGER", isPrimaryKey: true, isNullable: true, defaultValue: "" + )) + #expect(try settings(of: "name", in: draft) == .proposed(name: "name", type: "TEXT")) + #expect(try settings(of: "notes", in: draft).include == false) + + draft.load(fields: [field("id,name,notes")], proposingType: Self.sqlType) + #expect(try settings(of: "id,name,notes", in: draft).name == "everything") } - /// The proposal a later read compares against is the one that read made, not the first one, so - /// the user's edit is judged against what the sheet was showing when it was made. @Test("An edit survives several re-reads, and a later read's own proposals stay unedited") func editsSurviveSeveralReads() throws { var draft = makeDraft([field("amount"), field("code")]) @@ -135,7 +188,7 @@ struct NewTableDraftTests { #expect(try settings(of: "code", in: draft).name == "sku") } - @Test("Fields follow the new read: a vanished one is dropped, a new one is proposed, order is the file's") + @Test("Fields follow the new read: a vanished one leaves the list, a new one is proposed, order is the file's") func fieldsFollowTheNewRead() throws { var draft = makeDraft([field("a"), field("b"), field("c")]) try edit("b", in: &draft) { $0.name = "renamed" } diff --git a/docs/features/import-export.mdx b/docs/features/import-export.mdx index f23c443b1f..ffbfe76076 100644 --- a/docs/features/import-export.mdx +++ b/docs/features/import-export.mdx @@ -346,7 +346,7 @@ The workbook is read whole rather than streamed, because a sheet's rows refer ba ### Import CSV -CSV and TSV open the same sheet as JSON, with parsing options in front of the mapping. The delimiter and encoding are detected from the file. Changing any option reads the file again. A field that is still there keeps the column it was mapped to. In a new table it keeps each column setting you changed, and the ones you left alone follow the new read, such as a type inferred again from trimmed values. +CSV and TSV open the same sheet as JSON, with parsing options in front of the mapping. The delimiter and encoding are detected from the file. Changing any option reads the file again. A field that is still there keeps the column it was mapped to. In a new table each column setting you changed stays with its field, even through a read that misses the field, such as one with the wrong delimiter. Settings you left alone follow the new read, such as a type inferred again from trimmed values. | Option | What it does | Default | |--------|-------------|---------|