fix(datagrid): withhold the column drag on a rearranged list, and refuse to rebuild a virtual table

Author: datlechin

Plugins/TableProPluginKit/SQLiteTableDDL.swift

@@ -49,10 +49,16 @@ public enum SQLiteTableDDL {
     ]
 
     /// Splits `CREATE TABLE x (…) WITHOUT ROWID` into its prefix, its top-level entries and its
-    /// trailing options. Nil when the statement has no balanced parenthesised body, which is what a
-    /// `CREATE TABLE … AS SELECT` looks like and what a rebuild must not touch.
+    /// trailing options.
+    ///
+    /// Nil for anything that is not an ordinary table, because a rebuild of one of those destroys
+    /// it. `sqlite_master` stores an FTS5 table as `CREATE VIRTUAL TABLE docs USING fts5(title,
+    /// body)`, whose parentheses parse exactly like a column list, so accepting it would recreate
+    /// the table as a plain one and take the index and its shadow tables down with the `DROP`.
+    /// `CREATE TABLE … AS SELECT` has no column list to reorder either.
     public static func parse(createTableSQL sql: String) -> Parsed? {
         guard let open = topLevelBodyStart(in: sql) else { return nil }
+        guard isOrdinaryTable(prefix: sql[sql.startIndex..<open]) else { return nil }
         guard let close = matchingCloseParen(in: sql, from: open) else { return nil }
 
         let body = String(sql[sql.index(after: open)..<close])
@@ -106,6 +112,23 @@ public enum SQLiteTableDDL {
         return trailing.isEmpty ? "" : " \(trailing)"
     }
 
+    /// Whether what stands before the column list is a plain `CREATE TABLE`. Only the keywords
+    /// SQLite allows there are accepted, so an unrecognised form is refused rather than rebuilt.
+    private static func isOrdinaryTable(prefix: Substring) -> Bool {
+        let words = prefix
+            .split(whereSeparator: { $0.isWhitespace })
+            .map { $0.uppercased() }
+        guard let tableIndex = words.firstIndex(of: "TABLE") else { return false }
+        /// Only these may stand before TABLE. VIRTUAL does not, which is what rules out an FTS or
+        /// R-tree table whose module arguments would otherwise read as a column list.
+        guard words[..<tableIndex].allSatisfy({ ["CREATE", "TEMP", "TEMPORARY"].contains($0) }),
+              words.first == "CREATE" else { return false }
+        /// What follows is `IF NOT EXISTS` and a name, which may itself be several whitespace
+        /// separated tokens when it is quoted. A bare AS among them is `CREATE TABLE … AS SELECT`,
+        /// which has no column list to reorder.
+        return !words[tableIndex...].contains("AS")
+    }
+
     /// The opening parenthesis of the column list, skipping any that a quoted table name contains.
     private static func topLevelBodyStart(in sql: String) -> String.Index? {
         var scanner = Scanner(sql)

TablePro/Models/Schema/ColumnReorderSupport.swift

@@ -59,7 +59,8 @@ enum ColumnReorderPolicy {
         engineName: String,
         isColumnsTab: Bool,
         canEditSchema: Bool,
-        hasStagedChanges: Bool
+        hasStagedChanges: Bool,
+        isRearranged: Bool
     ) -> ColumnReorderAvailability {
         guard isColumnsTab else { return .notApplicable }
         guard canEditSchema else {
@@ -81,6 +82,15 @@ enum ColumnReorderPolicy {
                     reason: String(localized: "Save or discard the pending structure changes before reordering columns.")
                 )
             }
+            /// A drop reports the row's position in what is on screen, and a filtered or sorted
+            /// list is not the table's order, so "third from the top" names a different column in
+            /// each. There is nothing to map it back to either: the wanted order is a statement
+            /// about every column, and a filtered list is not showing every column.
+            guard !isRearranged else {
+                return .unavailable(
+                    reason: String(localized: "Clear the filter and the sort to reorder columns.")
+                )
+            }
             return .available(support)
         }
     }

TablePro/Views/Structure/TableStructureView+ColumnReorder.swift

@@ -21,7 +21,8 @@ extension TableStructureView {
             engineName: connection.type.displayName,
             isColumnsTab: selectedTab == .columns,
             canEditSchema: connection.type.supportsSchemaEditing,
-            hasStagedChanges: structureChangeManager.hasChanges
+            hasStagedChanges: structureChangeManager.hasChanges,
+            isRearranged: !searchText.isEmpty || structureSortDescriptor != nil
         )
     }
 

TableProTests/Models/Schema/ColumnReorderPlannerTests.swift

@@ -97,4 +97,56 @@ struct ColumnReorderPlannerTests {
     func nonPermutationProducesNoCycle() {
         #expect(PluginColumnReorderPlanner.appendCycle(from: current, to: ["a", "b"]).isEmpty)
     }
+
+    // MARK: - Exhaustive
+
+    /// Both planners are asked for every permutation of five columns. A tie in the common
+    /// subsequence can pick a different set to leave alone without changing how many columns move,
+    /// so the guarantee worth pinning is the result, not the choice.
+    @Test("Every permutation of five columns is reached, by both mechanisms, in the minimum moves")
+    func everyPermutationIsReachable() {
+        let start = ["a", "b", "c", "d", "e"]
+        for desired in permutations(of: start) {
+            let moves = PluginColumnReorderPlanner.moves(from: start, to: desired)
+            var byMove = start
+            for move in moves {
+                byMove.removeAll { $0 == move.column }
+                if let after = move.afterColumn, let index = byMove.firstIndex(of: after) {
+                    byMove.insert(move.column, at: byMove.index(after: index))
+                } else {
+                    byMove.insert(move.column, at: 0)
+                }
+            }
+            #expect(byMove == desired, "moves did not reach \(desired)")
+            #expect(moves.count == start.count - longestCommonSubsequenceLength(start, desired))
+
+            var byCycle = start
+            for column in PluginColumnReorderPlanner.appendCycle(from: start, to: desired) {
+                byCycle.removeAll { $0 == column }
+                byCycle.append(column)
+            }
+            #expect(byCycle == desired, "cycling did not reach \(desired)")
+        }
+    }
+
+    private func permutations(of values: [String]) -> [[String]] {
+        guard values.count > 1 else { return [values] }
+        return values.indices.flatMap { index -> [[String]] in
+            var rest = values
+            let picked = rest.remove(at: index)
+            return permutations(of: rest).map { [picked] + $0 }
+        }
+    }
+
+    private func longestCommonSubsequenceLength(_ lhs: [String], _ rhs: [String]) -> Int {
+        var lengths = Array(repeating: Array(repeating: 0, count: rhs.count + 1), count: lhs.count + 1)
+        for i in stride(from: lhs.count - 1, through: 0, by: -1) {
+            for j in stride(from: rhs.count - 1, through: 0, by: -1) {
+                lengths[i][j] = lhs[i] == rhs[j]
+                    ? lengths[i + 1][j + 1] + 1
+                    : max(lengths[i + 1][j], lengths[i][j + 1])
+            }
+        }
+        return lengths[0][0]
+    }
 }

TableProTests/Models/Schema/ColumnReorderPolicyTests.swift

@@ -13,14 +13,16 @@ struct ColumnReorderPolicyTests {
         support: ColumnReorderSupport = .alter,
         isColumnsTab: Bool = true,
         canEditSchema: Bool = true,
-        hasStagedChanges: Bool = false
+        hasStagedChanges: Bool = false,
+        isRearranged: Bool = false
     ) -> ColumnReorderAvailability {
         ColumnReorderPolicy.resolve(
             support: support,
             engineName: "PostgreSQL",
             isColumnsTab: isColumnsTab,
             canEditSchema: canEditSchema,
-            hasStagedChanges: hasStagedChanges
+            hasStagedChanges: hasStagedChanges,
+            isRearranged: isRearranged
         )
     }
 
@@ -66,4 +68,20 @@ struct ColumnReorderPolicyTests {
         let availability = resolve(support: .unsupported, hasStagedChanges: true)
         #expect(availability.unavailableReason?.contains("PostgreSQL") == true)
     }
+
+    /// A drop reports a position in what is on screen. Filtered or sorted, that is not the table's
+    /// order, and the delegate hands the position over without mapping it back, so the drag is
+    /// withheld rather than acted on against the wrong column.
+    @Test("A filtered or sorted column list withholds the drag")
+    func rearrangedListWithholdsTheDrag() {
+        let availability = resolve(isRearranged: true)
+        #expect(!availability.isAvailable)
+        #expect(availability.unavailableReason != nil)
+    }
+
+    @Test("Staged edits outrank a rearranged list, because saving is the first thing to do")
+    func stagedChangesOutrankRearrangement() {
+        let staged = resolve(hasStagedChanges: true, isRearranged: true)
+        #expect(staged.unavailableReason == resolve(hasStagedChanges: true).unavailableReason)
+    }
 }

TableProTests/Models/Schema/SQLiteTableDDLTests.swift

@@ -42,6 +42,30 @@ struct SQLiteTableDDLTests {
     @Test("A statement with no column list is refused rather than half parsed")
     func parseRefusesCreateTableAsSelect() {
         #expect(SQLiteTableDDL.parse(createTableSQL: "CREATE TABLE t AS SELECT 1") == nil)
+        #expect(SQLiteTableDDL.parse(createTableSQL: "CREATE TABLE t AS SELECT (1)") == nil)
+    }
+
+    /// `sqlite_master` stores an FTS5 table as a `CREATE VIRTUAL TABLE` whose parentheses parse
+    /// exactly like a column list. Rebuilding one recreates it as a plain table and drops the index
+    /// and its shadow tables with the original, so it has to be refused before that.
+    @Test("A virtual table is refused, whatever its module", arguments: [
+        "CREATE VIRTUAL TABLE docs USING fts5(title, body)",
+        "CREATE VIRTUAL TABLE t USING rtree(id, minX, maxX)",
+        "CREATE VIRTUAL TABLE IF NOT EXISTS v USING fts4(a, b)"
+    ])
+    func parseRefusesVirtualTables(sql: String) {
+        #expect(SQLiteTableDDL.parse(createTableSQL: sql) == nil)
+    }
+
+    @Test("The ordinary forms are still accepted", arguments: [
+        "CREATE TABLE t(a INT)",
+        "CREATE TEMP TABLE t(a INT)",
+        "CREATE TEMPORARY TABLE t(a INT)",
+        "CREATE TABLE IF NOT EXISTS t(a INT)",
+        "CREATE TABLE \"my (odd) name\"(a INT)"
+    ])
+    func parseAcceptsOrdinaryTables(sql: String) {
+        #expect(SQLiteTableDDL.parse(createTableSQL: sql) != nil)
     }
 
     @Test("Reordering moves the column definitions verbatim and leaves the constraints in place")