Fix multi-column update (#97)

* Add tests that should reaffirm failure mode. * ws change to trigger tests * Take a pass at getting those commas in there between alterations. * switch to update(column:) * tuple clarity improvements and an additional warning log message * use local syntax var once more * instead of skipping modifications if they are not supported, allow SQL to fall over after giving user a warning.
vapor · Feb 28, 2020 · ab38827 · ab38827
1 parent b560d6d
commit ab38827
Show file tree

Hide file tree

Showing 5 changed files with 105 additions and 13 deletions.
diff --git a/Sources/SQLKit/Query/SQLAlterTable.swift b/Sources/SQLKit/Query/SQLAlterTable.swift
@@ -23,22 +23,40 @@ public struct SQLAlterTable: SQLExpression {
     }
 
     public func serialize(to serializer: inout SQLSerializer) {
+        let syntax = serializer.dialect.alterTableSyntax
+
+        if !syntax.allowsBatch && self.addColumns.count + self.modifyColumns.count + self.dropColumns.count > 1 {
+            serializer.database.logger.warning("Database does not support batch table alterations. You will need to rewrite as individual alter statements.")
+        }
+
+        if syntax.alterColumnDefinitionClause == nil && self.modifyColumns.count > 0 {
+            serializer.database.logger.warning("Database does not support column modifications. You will need to rewrite as drop and add clauses.")
+        }
+
+        let additions = self.addColumns.map { column in
+            (verb: SQLRaw("ADD"), definition: column)
+        }
+
+        let removals = self.dropColumns.map { column in
+            (verb: SQLRaw("DROP"), definition: column)
+        }
+
+        let alterColumnDefinitionCaluse = syntax.alterColumnDefinitionClause ?? SQLRaw("MODIFY")
+        let modifications = self.modifyColumns.map { column in
+            (verb: alterColumnDefinitionCaluse, definition: column)
+        }
+
+        let alterations = additions + removals + modifications
+
         serializer.statement {
             $0.append("ALTER TABLE")
             $0.append(self.name)
-            for column in self.addColumns {
-                $0.append("ADD")
-                $0.append(column)
-            }
-            if let clause = $0.dialect.alterTableSyntax.alterColumnDefinitionClause, !self.modifyColumns.isEmpty {
-                $0.append(clause)
-                for column in self.modifyColumns {
-                    $0.append(column)
+            for (idx, alteration) in alterations.enumerated() {
+                if idx > 0 {
+                    $0.append(",")
                 }
-            }
-            for column in self.dropColumns {
-                $0.append("DROP")
-                $0.append(column)
+                $0.append(alteration.verb)
+                $0.append(alteration.definition)
             }
         }
     }

diff --git a/Sources/SQLKit/SQLDialect.swift b/Sources/SQLKit/SQLDialect.swift
@@ -31,12 +31,18 @@ public struct SQLAlterTableSyntax {
     /// `nil` indicates that no extra keyword is required.
     public var alterColumnDefinitionTypeKeyword: SQLExpression?
 
+    /// If true, the dialect supports chaining multiple modifications together. If false,
+    /// the dialect requires separate statements for each change.
+    public var allowsBatch: Bool
+
     public init(
         alterColumnDefinitionClause: SQLExpression? = nil,
-        alterColumnDefinitionTypeKeyword: SQLExpression? = nil
+        alterColumnDefinitionTypeKeyword: SQLExpression? = nil,
+        allowsBatch: Bool = true
     ) {
         self.alterColumnDefinitionClause = alterColumnDefinitionClause
         self.alterColumnDefinitionTypeKeyword = alterColumnDefinitionTypeKeyword
+        self.allowsBatch = allowsBatch
     }
 }
 

diff --git a/Sources/SQLKitBenchmark/SQLBenchmark+Planets.swift b/Sources/SQLKitBenchmark/SQLBenchmark+Planets.swift
@@ -77,6 +77,25 @@ extension SQLBenchmarker {
             .from("planets")
             .where("galaxyID", .equal, SQLBind(5))
             .run().wait()
+
+        // add columns for the sake of testing adding columns
+        try self.db.alter(table: "planets")
+            .column("extra", type: .int)
+            .run().wait()
+
+        if self.db.dialect.alterTableSyntax.allowsBatch {
+            try self.db.alter(table: "planets")
+                .column("very_extra", type: .bigint)
+                .column("extra_extra", type: .text)
+                .run().wait()
+
+            // drop, add, and modify columns
+            try self.db.alter(table: "planets")
+                .dropColumn("extra_extra")
+                .update(column: "extra", type: .text)
+                .column("hi", type: .text)
+                .run().wait()
+        }
     }
 }
 

diff --git a/Tests/SQLKitTests/SQLKitTests.swift b/Tests/SQLKitTests/SQLKitTests.swift
@@ -112,6 +112,53 @@ final class SQLKitTests: XCTestCase {
         try db.drop(table: "planets").restrict().run().wait()
         XCTAssertEqual(db.results[9], "DROP TABLE `planets` RESTRICT")
     }
+
+    func testAltering() throws {
+        let db = TestDatabase()
+
+        // SINGLE
+        try db.alter(table: "alterable")
+            .column("hello", type: .text)
+            .run().wait()
+        XCTAssertEqual(db.results[0], "ALTER TABLE `alterable` ADD `hello` TEXT")
+
+        try db.alter(table: "alterable")
+            .dropColumn("hello")
+            .run().wait()
+        XCTAssertEqual(db.results[1], "ALTER TABLE `alterable` DROP `hello`")
+
+        try db.alter(table: "alterable")
+            .modifyColumn("hello", type: .text)
+            .run().wait()
+        XCTAssertEqual(db.results[2], "ALTER TABLE `alterable` MODIFY `hello` TEXT")
+
+        // BATCH
+        try db.alter(table: "alterable")
+            .column("hello", type: .text)
+            .column("there", type: .text)
+            .run().wait()
+        XCTAssertEqual(db.results[3], "ALTER TABLE `alterable` ADD `hello` TEXT , ADD `there` TEXT")
+
+        try db.alter(table: "alterable")
+            .dropColumn("hello")
+            .dropColumn("there")
+            .run().wait()
+        XCTAssertEqual(db.results[4], "ALTER TABLE `alterable` DROP `hello` , DROP `there`")
+
+        try db.alter(table: "alterable")
+            .update(column: "hello", type: .text)
+            .update(column: "there", type: .text)
+            .run().wait()
+        XCTAssertEqual(db.results[5], "ALTER TABLE `alterable` MODIFY `hello` TEXT , MODIFY `there` TEXT")
+
+        // MIXED
+        try db.alter(table: "alterable")
+            .column("hello", type: .text)
+            .dropColumn("there")
+            .update(column: "again", type: .text)
+            .run().wait()
+        XCTAssertEqual(db.results[6], "ALTER TABLE `alterable` ADD `hello` TEXT , DROP `there` , MODIFY `again` TEXT")
+    }
 
     func testDistinct() throws {
         let db = TestDatabase()

diff --git a/Tests/SQLKitTests/Utilities.swift b/Tests/SQLKitTests/Utilities.swift
@@ -110,6 +110,8 @@ struct GenericDialect: SQLDialect {
 
     var triggerSyntax = SQLTriggerSyntax()
 
+    var alterTableSyntax = SQLAlterTableSyntax(alterColumnDefinitionClause: SQLRaw("MODIFY"), alterColumnDefinitionTypeKeyword: nil)
+
     mutating func setTriggerSyntax(create: SQLTriggerSyntax.Create = [], drop: SQLTriggerSyntax.Drop = []) {
         self.triggerSyntax.create = create
         self.triggerSyntax.drop = drop