Merge pull request #7371 from kenjis/fix-modifyColumn-SQLite3

kenjis · web-flow · commit 90762f31f7bc · 2023-04-27T18:18:28.000+09:00
fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly
diff --git a/system/Database/Forge.php b/system/Database/Forge.php
@@ -153,7 +153,7 @@ class Forge
      *
      * @internal Used for marking nullable fields. Not covered by BC promise.
      */
-    protected $null = '';
+    protected $null = 'NULL';
 
     /**
      * DEFAULT value representation in CREATE/ALTER TABLE statements
@@ -562,7 +562,7 @@ public function createTable(string $table, bool $ifNotExists = false, array $att
     }
 
     /**
-     * @return string
+     * @return string SQL string
      *
      * @deprecated $ifNotExists is no longer used, and will be removed.
      */
@@ -897,13 +897,19 @@ protected function _processFields(bool $createTable = false): array
             $this->_attributeDefault($attributes, $field);
 
             if (isset($attributes['NULL'])) {
+                $nullString = ' ' . $this->null;
+
                 if ($attributes['NULL'] === true) {
-                    $field['null'] = empty($this->null) ? '' : ' ' . $this->null;
+                    $field['null'] = empty($this->null) ? '' : $nullString;
+                } elseif ($attributes['NULL'] === $nullString) {
+                    $field['null'] = $nullString;
+                } elseif ($attributes['NULL'] === '') {
+                    $field['null'] = '';
                 } else {
-                    $field['null'] = ' NOT NULL';
+                    $field['null'] = ' NOT ' . $this->null;
                 }
             } elseif ($createTable === true) {
-                $field['null'] = ' NOT NULL';
+                $field['null'] = ' NOT ' . $this->null;
             }
 
             $this->_attributeAutoIncrement($attributes, $field);
diff --git a/system/Database/OCI8/Forge.php b/system/Database/OCI8/Forge.php
@@ -120,10 +120,17 @@ protected function _alterTable(string $alterType, string $table, $field)
                 // If a null constraint is added to a column with a null constraint,
                 // ORA-01451 will occur,
                 // so add null constraint is used only when it is different from the current null constraint.
-                $isWantToAddNull    = strpos($field[$i]['null'], ' NOT') === false;
-                $currentNullAddable = $nullableMap[$field[$i]['name']];
+                // If a not null constraint is added to a column with a not null constraint,
+                // ORA-01442 will occur.
+                $wantToAddNull   = strpos($field[$i]['null'], ' NOT') === false;
+                $currentNullable = $nullableMap[$field[$i]['name']];
 
-                if ($isWantToAddNull === $currentNullAddable) {
+                if ($wantToAddNull === true && $currentNullable === true) {
+                    $field[$i]['null'] = '';
+                } elseif ($field[$i]['null'] === '' && $currentNullable === false) {
+                    // Nullable by default
+                    $field[$i]['null'] = ' NULL';
+                } elseif ($wantToAddNull === false && $currentNullable === false) {
                     $field[$i]['null'] = '';
                 }
             }
diff --git a/system/Database/Postgre/Forge.php b/system/Database/Postgre/Forge.php
@@ -109,10 +109,12 @@ protected function _alterTable(string $alterType, string $table, $field)
                     . " SET DEFAULT {$data['default']}";
             }
 
-            if (isset($data['null'])) {
-                $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name'])
-                    . ($data['null'] === true ? ' DROP' : ' SET') . ' NOT NULL';
+            $nullable = true; // Nullable by default.
+            if (isset($data['null']) && ($data['null'] === false || $data['null'] === ' NOT ' . $this->null)) {
+                $nullable = false;
             }
+            $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name'])
+                . ($nullable === true ? ' DROP' : ' SET') . ' NOT NULL';
 
             if (! empty($data['new_name'])) {
                 $sqls[] = $sql . ' RENAME COLUMN ' . $this->db->escapeIdentifiers($data['name'])
diff --git a/system/Database/SQLSRV/Forge.php b/system/Database/SQLSRV/Forge.php
@@ -203,10 +203,12 @@ protected function _alterTable(string $alterType, string $table, $field)
                     . " DEFAULT {$data['default']} FOR " . $this->db->escapeIdentifiers($data['name']);
             }
 
-            if (isset($data['null'])) {
-                $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name'])
-                    . ($data['null'] === true ? ' DROP' : '') . " {$data['type']}{$data['length']} NOT NULL";
+            $nullable = true; // Nullable by default.
+            if (isset($data['null']) && ($data['null'] === false || $data['null'] === ' NOT ' . $this->null)) {
+                $nullable = false;
             }
+            $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name'])
+                . " {$data['type']}{$data['length']} " . ($nullable === true ? '' : 'NOT') . ' NULL';
 
             if (! empty($data['comment'])) {
                 $sqls[] = 'EXEC sys.sp_addextendedproperty '
diff --git a/system/Database/SQLite3/Table.php b/system/Database/SQLite3/Table.php
@@ -183,14 +183,14 @@ public function dropColumn($columns)
      *
      * @return Table
      */
-    public function modifyColumn(array $field)
+    public function modifyColumn(array $fields)
     {
-        $field = $field[0];
-
-        $oldName = $field['name'];
-        unset($field['name']);
+        foreach ($fields as $field) {
+            $oldName = $field['name'];
+            unset($field['name']);
 
-        $this->fields[$oldName] = $field;
+            $this->fields[$oldName] = $field;
+        }
 
         return $this;
     }
diff --git a/tests/_support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php b/tests/_support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php
@@ -21,7 +21,7 @@ public function up()
                 'constraint' => 255,
             ],
         ]);
-        $this->forge->createTable('foo', true);
+        $this->forge->createTable('foo');
 
         $this->db->table('foo')->insert([
             'key' => 'foobar',
diff --git a/tests/system/Commands/Database/MigrateStatusTest.php b/tests/system/Commands/Database/MigrateStatusTest.php
@@ -14,6 +14,7 @@
 use CodeIgniter\CLI\CLI;
 use CodeIgniter\Test\CIUnitTestCase;
 use CodeIgniter\Test\StreamFilterTrait;
+use Config\Database;
 
 /**
  * @group DatabaseLive
@@ -29,6 +30,9 @@ final class MigrateStatusTest extends CIUnitTestCase
 
     protected function setUp(): void
     {
+        $forge = Database::forge();
+        $forge->dropTable('foo', true);
+
         parent::setUp();
 
         if (! is_file($this->migrationFileFrom)) {
diff --git a/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php b/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php
@@ -56,6 +56,9 @@ final class DatabaseTestCaseMigrationOnce1Test extends CIUnitTestCase
 
     protected function setUp(): void
     {
+        $forge = Database::forge();
+        $forge->dropTable('foo', true);
+
         $this->setUpMethods[] = 'setUpAddNamespace';
 
         parent::setUp();
diff --git a/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php b/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php
@@ -13,6 +13,7 @@
 
 use CodeIgniter\Test\CIUnitTestCase;
 use CodeIgniter\Test\DatabaseTestTrait;
+use Config\Database;
 use Config\Services;
 
 /**
@@ -55,6 +56,9 @@ final class DatabaseTestCaseMigrationOnce2Test extends CIUnitTestCase
 
     protected function setUp(): void
     {
+        $forge = Database::forge();
+        $forge->dropTable('foo', true);
+
         $this->setUpMethods[] = 'setUpAddNamespace';
 
         parent::setUp();
diff --git a/tests/system/Database/DatabaseTestCaseTest.php b/tests/system/Database/DatabaseTestCaseTest.php
@@ -13,6 +13,7 @@
 
 use CodeIgniter\Test\CIUnitTestCase;
 use CodeIgniter\Test\DatabaseTestTrait;
+use Config\Database;
 use Config\Services;
 use Tests\Support\Database\Seeds\AnotherSeeder;
 use Tests\Support\Database\Seeds\CITestSeeder;
@@ -60,6 +61,9 @@ final class DatabaseTestCaseTest extends CIUnitTestCase
 
     protected function setUp(): void
     {
+        $forge = Database::forge();
+        $forge->dropTable('foo', true);
+
         $this->setUpMethods[] = 'setUpAddNamespace';
 
         parent::setUp();
diff --git a/tests/system/Database/Live/ForgeTest.php b/tests/system/Database/Live/ForgeTest.php
@@ -16,7 +16,9 @@
 use CodeIgniter\Test\CIUnitTestCase;
 use CodeIgniter\Test\DatabaseTestTrait;
 use Config\Database;
+use LogicException;
 use RuntimeException;
+use stdClass;
 use Tests\Support\Database\Seeds\CITestSeeder;
 
 /**
@@ -1276,6 +1278,91 @@ public function testModifyColumnRename()
         $this->forge->dropTable('forge_test_three', true);
     }
 
+    public function testModifyColumnNullTrue()
+    {
+        // @TODO remove this in `4.4` branch
+        if ($this->db->DBDriver === 'SQLSRV') {
+            $this->markTestSkipped('SQLSRV does not support getFieldData() nullable.');
+        }
+
+        $this->forge->dropTable('forge_test_modify', true);
+
+        $this->forge->addField([
+            'col1' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => true],
+            'col2' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => true],
+            'col3' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => true],
+        ]);
+        $this->forge->createTable('forge_test_modify');
+
+        $this->forge->modifyColumn('forge_test_modify', [
+            'col1' => ['type' => 'VARCHAR', 'constraint' => 1],
+            'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true],
+            'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false],
+        ]);
+
+        $this->db->resetDataCache();
+
+        $col1 = $this->getMetaData('col1', 'forge_test_modify');
+        $this->assertTrue($col1->nullable);
+        $col2 = $this->getMetaData('col2', 'forge_test_modify');
+        $this->assertTrue($col2->nullable);
+        $col3 = $this->getMetaData('col3', 'forge_test_modify');
+        $this->assertFalse($col3->nullable);
+
+        $this->forge->dropTable('forge_test_modify', true);
+    }
+
+    public function testModifyColumnNullFalse()
+    {
+        // @TODO remove this in `4.4` branch
+        if ($this->db->DBDriver === 'SQLSRV') {
+            $this->markTestSkipped('SQLSRV does not support getFieldData() nullable.');
+        }
+
+        $this->forge->dropTable('forge_test_modify', true);
+
+        $this->forge->addField([
+            'col1' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => false],
+            'col2' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => false],
+            'col3' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => false],
+        ]);
+        $this->forge->createTable('forge_test_modify');
+
+        $this->forge->modifyColumn('forge_test_modify', [
+            'col1' => ['type' => 'VARCHAR', 'constraint' => 1],
+            'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true],
+            'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false],
+        ]);
+
+        $this->db->resetDataCache();
+
+        $col1 = $this->getMetaData('col1', 'forge_test_modify');
+        $this->assertTrue($col1->nullable); // Nullable by default.
+        $col2 = $this->getMetaData('col2', 'forge_test_modify');
+        $this->assertTrue($col2->nullable);
+        $col3 = $this->getMetaData('col3', 'forge_test_modify');
+        $this->assertFalse($col3->nullable);
+
+        $this->forge->dropTable('forge_test_modify', true);
+    }
+
+    private function getMetaData(string $column, string $table): stdClass
+    {
+        $fields = $this->db->getFieldData($table);
+
+        $name = array_search(
+            $column,
+            array_column($fields, 'name'),
+            true
+        );
+
+        if ($name === false) {
+            throw new LogicException('Column not found: ' . $column);
+        }
+
+        return $fields[$name];
+    }
+
     public function testConnectWithArrayGroup()
     {
         $group = config('Database');
diff --git a/tests/system/Database/Migrations/MigrationRunnerTest.php b/tests/system/Database/Migrations/MigrationRunnerTest.php
@@ -349,6 +349,9 @@ public function testLatestSuccess()
 
     public function testRegressSuccess()
     {
+        $forge = Database::forge();
+        $forge->dropTable('foo', true);
+
         $runner = new MigrationRunner($this->config);
         $runner->setSilent(false)
             ->setNamespace('Tests\Support\MigrationTestMigrations')
@@ -368,6 +371,9 @@ public function testRegressSuccess()
 
     public function testLatestTriggersEvent()
     {
+        $forge = Database::forge();
+        $forge->dropTable('foo', true);
+
         $runner = new MigrationRunner($this->config);
         $runner->setSilent(false)
             ->setNamespace('Tests\Support\MigrationTestMigrations');
@@ -387,6 +393,9 @@ public function testLatestTriggersEvent()
 
     public function testRegressTriggersEvent()
     {
+        $forge = Database::forge();
+        $forge->dropTable('foo', true);
+
         $runner = new MigrationRunner($this->config);
         $runner->setSilent(false)
             ->setNamespace('Tests\Support\MigrationTestMigrations');
diff --git a/user_guide_src/source/changelogs/v4.3.4.rst b/user_guide_src/source/changelogs/v4.3.4.rst
@@ -35,6 +35,21 @@ Redirect Status Code
   always be used when you don't specify a status code. In previous versions,
   302 might be changed.
 
+.. _v434-forge-modifycolumn:
+
+Forge::modifyColumn()
+---------------------
+
+- The :ref:`$forge->modifyColumn() <db-forge-modifyColumn>` has been fixed. Due
+  to a bug, in previous versions, SQLite3/Postgres/SQLSRV might change
+  ``NULL``/``NOT NULL`` unpredictably.
+- In previous versions, the OCI8 driver did not change ``NULL``/``NOT NULL``
+  when you don't specify the ``null`` key.
+- Now in all database drivers ``$forge->modifyColumn()`` always sets ``NULL``
+  when you don't specify the ``null`` key.
+- The ``NULL``/``NOT NULL`` change may still be unexpectedly, it is recommended
+  to always specify the ``null`` key.
+
 Message Changes
 ***************
 
diff --git a/user_guide_src/source/dbmgmt/forge.rst b/user_guide_src/source/dbmgmt/forge.rst
@@ -285,6 +285,8 @@ Used to remove multiple columns from a table.
 Modifying a Field in a Table
 ============================
 
+.. _db-forge-modifyColumn:
+
 $forge->modifyColumn()
 ----------------------
 
@@ -294,6 +296,17 @@ change the name, you can add a "name" key into the field defining array.
 
 .. literalinclude:: forge/026.php
 
+.. note:: The ``modifyColumn()`` may unexpectedly change ``NULL``/``NOT NULL``.
+    So it is recommended to always specify the value for ``null`` key. Unlike when creating
+    a table, if ``null`` is not specified, the column will be ``NULL``, not
+    ``NOT NULL``.
+
+.. note:: Due to a bug, prior v4.3.3, SQLite3 may not set ``NOT NULL`` even if you
+    specify ``'null' => false``.
+
+.. note:: Due to a bug, prior v4.3.3, Postgres and SQLSRV set ``NOT NULL`` even
+    if you specify ``'null' => false``.
+
 .. _db-forge-adding-keys-to-a-table:
 
 Adding Keys to a Table
diff --git a/user_guide_src/source/dbmgmt/forge/026.php b/user_guide_src/source/dbmgmt/forge/026.php
@@ -4,7 +4,8 @@
     'old_name' => [
         'name' => 'new_name',
         'type' => 'TEXT',
+        'null' => false,
     ],
 ];
 $forge->modifyColumn('table_name', $fields);
-// gives ALTER TABLE `table_name` CHANGE `old_name` `new_name` TEXT
+// gives ALTER TABLE `table_name` CHANGE `old_name` `new_name` TEXT NOT NULL
diff --git a/user_guide_src/source/installation/upgrade_434.rst b/user_guide_src/source/installation/upgrade_434.rst
@@ -25,6 +25,18 @@ Redirect Status Code
   :ref:`ChangeLog v4.3.4 <v434-redirect-status-code>` and if the code is not
   what you want, :ref:`specify status codes <response-redirect-status-code>`.
 
+Forge::modifyColumn() and NULL
+==============================
+
+A bug fix may have changed the NULL constraint in the result of
+:ref:`$forge->modifyColumn() <db-forge-modifyColumn>`. See
+:ref:`Change Log <v434-forge-modifycolumn>`.
+To set the desired NULL constraint, change ``Forge::modifyColumn()`` to always
+specify the ``null`` key.
+
+Note that the bug may have changed unexpected NULL constraints in previous
+versions.
+
 Breaking Enhancements
 *********************
 

Original file line number	Diff line number	Diff line change
`@@ -109,10 +109,12 @@ protected function _alterTable(string $alterType, string $table, $field)`
`109`	`109`	`. " SET DEFAULT {$data['default']}";`
`110`	`110`	`}`
`111`	`111`
`112`		`- if (isset($data['null'])) {`
`113`		`- $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name'])`
`114`		`- . ($data['null'] === true ? ' DROP' : ' SET') . ' NOT NULL';`
	`112`	`+ $nullable = true; // Nullable by default.`
	`113`	`+ if (isset($data['null']) && ($data['null'] === false \|\| $data['null'] === ' NOT ' . $this->null)) {`
	`114`	`+ $nullable = false;`
`115`	`115`	`}`
	`116`	`+ $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name'])`
	`117`	`+ . ($nullable === true ? ' DROP' : ' SET') . ' NOT NULL';`
`116`	`118`
`117`	`119`	`if (! empty($data['new_name'])) {`
`118`	`120`	`$sqls[] = $sql . ' RENAME COLUMN ' . $this->db->escapeIdentifiers($data['name'])`
Original file line number	Diff line number	Diff line change
`@@ -203,10 +203,12 @@ protected function _alterTable(string $alterType, string $table, $field)`
`203`	`203`	`. " DEFAULT {$data['default']} FOR " . $this->db->escapeIdentifiers($data['name']);`
`204`	`204`	`}`
`205`	`205`
`206`		`- if (isset($data['null'])) {`
`207`		`- $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name'])`
`208`		`- . ($data['null'] === true ? ' DROP' : '') . " {$data['type']}{$data['length']} NOT NULL";`
	`206`	`+ $nullable = true; // Nullable by default.`
	`207`	`+ if (isset($data['null']) && ($data['null'] === false \|\| $data['null'] === ' NOT ' . $this->null)) {`
	`208`	`+ $nullable = false;`
`209`	`209`	`}`
	`210`	`+ $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name'])`
	`211`	`+ . " {$data['type']}{$data['length']} " . ($nullable === true ? '' : 'NOT') . ' NULL';`
`210`	`212`
`211`	`213`	`if (! empty($data['comment'])) {`
`212`	`214`	`$sqls[] = 'EXEC sys.sp_addextendedproperty '`
Original file line number	Diff line number	Diff line change
`@@ -183,14 +183,14 @@ public function dropColumn($columns)`
`183`	`183`	`*`
`184`	`184`	`* @return Table`
`185`	`185`	`*/`
`186`		`- public function modifyColumn(array $field)`
	`186`	`+ public function modifyColumn(array $fields)`
`187`	`187`	`{`
`188`		`- $field = $field[0];`
`189`		`-`
`190`		`- $oldName = $field['name'];`
`191`		`- unset($field['name']);`
	`188`	`+ foreach ($fields as $field) {`
	`189`	`+ $oldName = $field['name'];`
	`190`	`+ unset($field['name']);`
`192`	`191`
`193`		`- $this->fields[$oldName] = $field;`
	`192`	`+ $this->fields[$oldName] = $field;`
	`193`	`+ }`
`194`	`194`
`195`	`195`	`return $this;`
`196`	`196`	`}`
Original file line number	Diff line number	Diff line change
`@@ -56,6 +56,9 @@ final class DatabaseTestCaseMigrationOnce1Test extends CIUnitTestCase`
`56`	`56`
`57`	`57`	`protected function setUp(): void`
`58`	`58`	`{`
	`59`	`+ $forge = Database::forge();`
	`60`	`+ $forge->dropTable('foo', true);`
	`61`	`+`
`59`	`62`	`$this->setUpMethods[] = 'setUpAddNamespace';`
`60`	`63`
`61`	`64`	`parent::setUp();`