Merge pull request #8457 from kenjis/fix-sqlite3-modifyColumn

fix: [SQLite3] Forge::modifyColumn() messes up table
codeigniter4 · Feb 2, 2024 · 24ed52b · 24ed52b
2 parents 51965f3 + 8d30df7
commit 24ed52b
Show file tree

Hide file tree

Showing 5 changed files with 206 additions and 6 deletions.
diff --git a/system/Database/SQLite3/Forge.php b/system/Database/SQLite3/Forge.php
@@ -131,9 +131,28 @@ protected function _alterTable(string $alterType, string $table, $processedField
                 return ''; // Why empty string?
 
             case 'CHANGE':
+                $fieldsToModify = [];
+
+                foreach ($processedFields as $processedField) {
+                    $name    = $processedField['name'];
+                    $newName = $processedField['new_name'];
+
+                    $field             = $this->fields[$name];
+                    $field['name']     = $name;
+                    $field['new_name'] = $newName;
+
+                    // Unlike when creating a table, if `null` is not specified,
+                    // the column will be `NULL`, not `NOT NULL`.
+                    if ($processedField['null'] === '') {
+                        $field['null'] = true;
+                    }
+
+                    $fieldsToModify[] = $field;
+                }
+
                 (new Table($this->db, $this))
                     ->fromTable($table)
-                    ->modifyColumn($processedFields) // @TODO Bug: should be NOT processed fields
+                    ->modifyColumn($fieldsToModify)
                     ->run();
 
                 return null; // Why null?

diff --git a/system/Database/SQLite3/Table.php b/system/Database/SQLite3/Table.php
@@ -392,6 +392,24 @@ protected function formatFields($fields)
                 'null'    => $field->nullable,
             ];
 
+            if ($field->default === null) {
+                // `null` means that the default value is not defined.
+                unset($return[$field->name]['default']);
+            } elseif ($field->default === 'NULL') {
+                // 'NULL' means that the default value is NULL.
+                $return[$field->name]['default'] = null;
+            } else {
+                $default = trim($field->default, "'");
+
+                if ($this->isIntegerType($field->type)) {
+                    $default = (int) $default;
+                } elseif ($this->isNumericType($field->type)) {
+                    $default = (float) $default;
+                }
+
+                $return[$field->name]['default'] = $default;
+            }
+
             if ($field->primary_key) {
                 $this->keys['primary'] = [
                     'fields' => [$field->name],
@@ -403,6 +421,30 @@ protected function formatFields($fields)
         return $return;
     }
 
+    /**
+     * Is INTEGER type?
+     *
+     * @param string $type SQLite data type (case-insensitive)
+     *
+     * @see https://www.sqlite.org/datatype3.html
+     */
+    private function isIntegerType(string $type): bool
+    {
+        return strpos(strtoupper($type), 'INT') !== false;
+    }
+
+    /**
+     * Is NUMERIC type?
+     *
+     * @param string $type SQLite data type (case-insensitive)
+     *
+     * @see https://www.sqlite.org/datatype3.html
+     */
+    private function isNumericType(string $type): bool
+    {
+        return in_array(strtoupper($type), ['NUMERIC', 'DECIMAL'], true);
+    }
+
     /**
      * Converts keys retrieved from the database to
      * the format needed to create later.

diff --git a/tests/system/Database/Live/ForgeTest.php b/tests/system/Database/Live/ForgeTest.php
@@ -1289,8 +1289,18 @@ public function testModifyColumnRename(): void
                 'unsigned'       => false,
                 'auto_increment' => true,
             ],
+            'int' => [
+                'type'       => 'INT',
+                'constraint' => 10,
+                'null'       => false,
+            ],
+            'varchar' => [
+                'type'       => 'VARCHAR',
+                'constraint' => 7,
+                'null'       => false,
+            ],
             'name' => [
-                'type'       => 'varchar',
+                'type'       => 'VARCHAR',
                 'constraint' => 255,
                 'null'       => true,
             ],
@@ -1304,17 +1314,31 @@ public function testModifyColumnRename(): void
         $this->forge->modifyColumn('forge_test_three', [
             'name' => [
                 'name'       => 'altered',
-                'type'       => 'varchar',
+                'type'       => 'VARCHAR',
                 'constraint' => 255,
                 'null'       => true,
             ],
         ]);
 
         $this->db->resetDataCache();
 
+        $fieldData = $this->db->getFieldData('forge_test_three');
+        $fields    = [];
+
+        foreach ($fieldData as $obj) {
+            $fields[$obj->name] = $obj;
+        }
+
         $this->assertFalse($this->db->fieldExists('name', 'forge_test_three'));
         $this->assertTrue($this->db->fieldExists('altered', 'forge_test_three'));
 
+        $this->assertTrue($fields['altered']->nullable);
+        $this->assertFalse($fields['int']->nullable);
+        $this->assertFalse($fields['varchar']->nullable);
+        $this->assertNull($fields['altered']->default);
+        $this->assertNull($fields['int']->default);
+        $this->assertNull($fields['varchar']->default);
+
         $this->forge->dropTable('forge_test_three', true);
     }
 

diff --git a/tests/system/Database/Live/SQLite3/AlterTableTest.php b/tests/system/Database/Live/SQLite3/AlterTableTest.php
@@ -89,17 +89,17 @@ public function testFromTableFillsDetails(): void
 
         $this->assertCount(5, $fields);
         $this->assertArrayHasKey('id', $fields);
-        $this->assertNull($fields['id']['default']);
+        $this->assertArrayNotHasKey('default', $fields['id']);
         $this->assertTrue($fields['id']['null']);
         $this->assertSame('integer', strtolower($fields['id']['type']));
 
         $this->assertArrayHasKey('name', $fields);
-        $this->assertNull($fields['name']['default']);
+        $this->assertArrayNotHasKey('default', $fields['name']);
         $this->assertFalse($fields['name']['null']);
         $this->assertSame('varchar', strtolower($fields['name']['type']));
 
         $this->assertArrayHasKey('email', $fields);
-        $this->assertNull($fields['email']['default']);
+        $this->assertArrayNotHasKey('default', $fields['email']);
         $this->assertTrue($fields['email']['null']);
         $this->assertSame('varchar', strtolower($fields['email']['type']));
 

diff --git a/tests/system/Database/Live/SQLite3/ForgeModifyColumnTest.php b/tests/system/Database/Live/SQLite3/ForgeModifyColumnTest.php
@@ -0,0 +1,115 @@
+<?php
+
+/**
+ * This file is part of CodeIgniter 4 framework.
+ *
+ * (c) CodeIgniter Foundation <[email protected]>
+ *
+ * For the full copyright and license information, please view
+ * the LICENSE file that was distributed with this source code.
+ */
+
+namespace CodeIgniter\Database\Live\SQLite3;
+
+use CodeIgniter\Database\Forge;
+use CodeIgniter\Test\CIUnitTestCase;
+use Config\Database;
+
+/**
+ * @group DatabaseLive
+ *
+ * @internal
+ */
+final class ForgeModifyColumnTest extends CIUnitTestCase
+{
+    private Forge $forge;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->db = Database::connect($this->DBGroup);
+
+        if ($this->db->DBDriver !== 'SQLite3') {
+            $this->markTestSkipped('This test is only for SQLite3.');
+        }
+
+        $this->forge = Database::forge($this->DBGroup);
+    }
+
+    public function testModifyColumnRename(): void
+    {
+        $table = 'forge_test_three';
+
+        $this->forge->dropTable($table, true);
+
+        $this->forge->addField([
+            'id' => [
+                'type'           => 'INTEGER',
+                'constraint'     => 11,
+                'auto_increment' => true,
+            ],
+            'int' => [
+                'type'       => 'INT',
+                'constraint' => 10,
+                'null'       => false,
+                'default'    => 0,
+            ],
+            'varchar' => [
+                'type'       => 'VARCHAR',
+                'constraint' => 7,
+                'null'       => false,
+            ],
+            'decimal' => [
+                'type'       => 'DECIMAL',
+                'constraint' => '10,5',
+                'default'    => 0.1,
+            ],
+            'name' => [
+                'type'       => 'VARCHAR',
+                'constraint' => 255,
+                'null'       => true,
+            ],
+        ]);
+
+        $this->forge->addKey('id', true);
+        $this->forge->createTable($table);
+
+        $this->assertTrue($this->db->fieldExists('name', $table));
+
+        $this->forge->modifyColumn($table, [
+            'name' => [
+                'name'       => 'altered',
+                'type'       => 'VARCHAR',
+                'constraint' => 255,
+                'null'       => true,
+            ],
+        ]);
+
+        $this->db->resetDataCache();
+
+        $fieldData = $this->db->getFieldData($table);
+        $fields    = [];
+
+        foreach ($fieldData as $obj) {
+            $fields[$obj->name] = $obj;
+        }
+
+        $this->assertFalse($this->db->fieldExists('name', $table));
+        $this->assertTrue($this->db->fieldExists('altered', $table));
+
+        $this->assertFalse($fields['int']->nullable);
+        $this->assertSame('0', $fields['int']->default);
+
+        $this->assertFalse($fields['varchar']->nullable);
+        $this->assertNull($fields['varchar']->default);
+
+        $this->assertFalse($fields['decimal']->nullable);
+        $this->assertSame('0.1', $fields['decimal']->default);
+
+        $this->assertTrue($fields['altered']->nullable);
+        $this->assertNull($fields['altered']->default);
+
+        $this->forge->dropTable($table, true);
+    }
+}