From a567d72cee856dcd57cad042302c9f40fce2ba8d Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 26 Jan 2024 13:18:03 +0900 Subject: [PATCH 1/7] test: add assertions --- tests/system/Database/Live/ForgeTest.php | 28 ++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/tests/system/Database/Live/ForgeTest.php b/tests/system/Database/Live/ForgeTest.php index f8a695396c9b..a0e3c9cc7c81 100644 --- 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,7 +1314,7 @@ public function testModifyColumnRename(): void $this->forge->modifyColumn('forge_test_three', [ 'name' => [ 'name' => 'altered', - 'type' => 'varchar', + 'type' => 'VARCHAR', 'constraint' => 255, 'null' => true, ], @@ -1312,9 +1322,23 @@ public function testModifyColumnRename(): void $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); } From b47e87848d048c0f77383e5cd47acef586fbffae Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 26 Jan 2024 13:18:48 +0900 Subject: [PATCH 2/7] fix: SQLite3 Forge::modifyColumn() messes up table --- system/Database/SQLite3/Forge.php | 15 ++++++++++++++- system/Database/SQLite3/Table.php | 10 ++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/system/Database/SQLite3/Forge.php b/system/Database/SQLite3/Forge.php index b02193f5057c..c3d8e12f1935 100644 --- a/system/Database/SQLite3/Forge.php +++ b/system/Database/SQLite3/Forge.php @@ -131,9 +131,22 @@ 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; + + $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 index ce574033afb0..725090dbcedc 100644 --- a/system/Database/SQLite3/Table.php +++ b/system/Database/SQLite3/Table.php @@ -392,6 +392,16 @@ 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 { + $return[$field->name]['default'] = trim($field->default, "'"); + } + if ($field->primary_key) { $this->keys['primary'] = [ 'fields' => [$field->name], From 8d0a5e57e6477d45c58cd76f7c1ae0c20b3243bd Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 26 Jan 2024 13:56:18 +0900 Subject: [PATCH 3/7] test: update assertions for fromTable() --- tests/system/Database/Live/SQLite3/AlterTableTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/system/Database/Live/SQLite3/AlterTableTest.php b/tests/system/Database/Live/SQLite3/AlterTableTest.php index 4a06a8ada091..a738484009c1 100644 --- 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'])); From c28c0c3d9cc1b743c35d51f7632cf4463a4aa4de Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 26 Jan 2024 14:25:53 +0900 Subject: [PATCH 4/7] fix: make default NULL when modifying column --- system/Database/SQLite3/Forge.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/system/Database/SQLite3/Forge.php b/system/Database/SQLite3/Forge.php index c3d8e12f1935..4be650e03155 100644 --- a/system/Database/SQLite3/Forge.php +++ b/system/Database/SQLite3/Forge.php @@ -141,6 +141,12 @@ protected function _alterTable(string $alterType, string $table, $processedField $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; } From eafb2d2e21329747eaab34ea62a2ec082d1fae68 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 30 Jan 2024 09:28:28 +0900 Subject: [PATCH 5/7] test: add test for SQLite3 Forge::modifyColumn() --- .../Live/SQLite3/ForgeModifyColumnTest.php | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 tests/system/Database/Live/SQLite3/ForgeModifyColumnTest.php diff --git a/tests/system/Database/Live/SQLite3/ForgeModifyColumnTest.php b/tests/system/Database/Live/SQLite3/ForgeModifyColumnTest.php new file mode 100644 index 000000000000..7905dcd7d684 --- /dev/null +++ b/tests/system/Database/Live/SQLite3/ForgeModifyColumnTest.php @@ -0,0 +1,113 @@ + + * + * 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 + { + $this->forge->dropTable('forge_test_three', 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('forge_test_three'); + + $this->assertTrue($this->db->fieldExists('name', 'forge_test_three')); + + $this->forge->modifyColumn('forge_test_three', [ + 'name' => [ + 'name' => 'altered', + '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->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('forge_test_three', true); + } +} From 26f786f723c1768f9f251459a33d9e5fa32d6f00 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 30 Jan 2024 09:29:13 +0900 Subject: [PATCH 6/7] fix: SQLite3 Forge::modifyColumn() changes numeric default value See https://github.com/codeigniter4/CodeIgniter4/pull/8457#issuecomment-1915547675 --- system/Database/SQLite3/Table.php | 34 ++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/system/Database/SQLite3/Table.php b/system/Database/SQLite3/Table.php index 725090dbcedc..47e6d1a7097e 100644 --- a/system/Database/SQLite3/Table.php +++ b/system/Database/SQLite3/Table.php @@ -399,7 +399,15 @@ protected function formatFields($fields) // 'NULL' means that the default value is NULL. $return[$field->name]['default'] = null; } else { - $return[$field->name]['default'] = trim($field->default, "'"); + $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) { @@ -413,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. From 8d30df7fedc12be0014f4a5a3f749bd8e58d4a51 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 30 Jan 2024 09:36:12 +0900 Subject: [PATCH 7/7] test: use variable $table --- .../Live/SQLite3/ForgeModifyColumnTest.php | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/tests/system/Database/Live/SQLite3/ForgeModifyColumnTest.php b/tests/system/Database/Live/SQLite3/ForgeModifyColumnTest.php index 7905dcd7d684..1237cb6da28e 100644 --- a/tests/system/Database/Live/SQLite3/ForgeModifyColumnTest.php +++ b/tests/system/Database/Live/SQLite3/ForgeModifyColumnTest.php @@ -39,7 +39,9 @@ protected function setUp(): void public function testModifyColumnRename(): void { - $this->forge->dropTable('forge_test_three', true); + $table = 'forge_test_three'; + + $this->forge->dropTable($table, true); $this->forge->addField([ 'id' => [ @@ -71,11 +73,11 @@ public function testModifyColumnRename(): void ]); $this->forge->addKey('id', true); - $this->forge->createTable('forge_test_three'); + $this->forge->createTable($table); - $this->assertTrue($this->db->fieldExists('name', 'forge_test_three')); + $this->assertTrue($this->db->fieldExists('name', $table)); - $this->forge->modifyColumn('forge_test_three', [ + $this->forge->modifyColumn($table, [ 'name' => [ 'name' => 'altered', 'type' => 'VARCHAR', @@ -86,15 +88,15 @@ public function testModifyColumnRename(): void $this->db->resetDataCache(); - $fieldData = $this->db->getFieldData('forge_test_three'); + $fieldData = $this->db->getFieldData($table); $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->assertFalse($this->db->fieldExists('name', $table)); + $this->assertTrue($this->db->fieldExists('altered', $table)); $this->assertFalse($fields['int']->nullable); $this->assertSame('0', $fields['int']->default); @@ -108,6 +110,6 @@ public function testModifyColumnRename(): void $this->assertTrue($fields['altered']->nullable); $this->assertNull($fields['altered']->default); - $this->forge->dropTable('forge_test_three', true); + $this->forge->dropTable($table, true); } }