Bug: [SQLite3] Forge class modifyColumn() method seriously messes up sqlite3 table

[dgvirtual]

PHP Version

8.1

CodeIgniter4 Version

4.4.4

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Linux

Which server did you use?

cli-server (PHP built-in webserver)

Database

SQLite3

What happened?

Changing a single table column name results in modifications of multiple columns, both column constraints (for example, turning NOT NULL fields to NULL) and defaults (0 to '0', NULL to 'NULL', etc) get changed.

Steps to Reproduce

Start with empty project.
Rename env to .env, change these lines in it to the values:

app.baseURL = 'http://localhost:8080/'
database.default.database = ci4
database.default.DBDriver = SQLite3

Add a cli route in app/Config/Routes.php:

$routes->cli('home', 'Home::index');

Add migration for a table:

php spark make:migration CreateTestTable

Replace up() and down() methods with:

    public function up()
    {
        $this->forge->addField([
            'id' => [
                'type'           => 'INT',
                'constraint'     => 10,
                'auto_increment' => true,
                'null'           => false,
            ],
            'user_id' => [
                'type'       => 'INT',
                'constraint' => 10,
                'null'       => false,
            ],
            'month' => [
                'type'       => 'VARCHAR',
                'constraint' => 7,
                'null'       => false,
            ],
            'invoice_series' => [
                'type'       => 'VARCHAR',
                'constraint' => 10,
                'null'       => false,
            ],
            'total' => [
                'type' => 'DECIMAL',
                'null' => false,
            ],
            'paid' => [
                'type'    => 'TINYINT',
                'default' => 0,
            ],
            'paid_date' => [
                'type'       => 'VARCHAR',
                'constraint' => 10,
                'default'    => null,
            ],
            'created_at' => [
                'type' => 'DATETIME',
                'null' => false,
            ],
            'updated_at' => [
                'type' => 'DATETIME',
                'null' => false,
            ],
        ]);

        $this->forge->addKey('id', true);
        $this->forge->createTable('invoices');
    }

    public function down()
    {
        $this->forge->dropTable('invoices');
    }

Add test code to the Home controller, replace the index() method with:

public function index()
{

    $forge = \Config\Database::forge();

    $fields = [
        'invoice_series' => [
            'name'       => 'series',
            'type'       => 'VARCHAR',
            'constraint' => 10,
            'null'       => false,
        ],
    ];

    $forge->modifyColumn('invoices', $fields);

}

Now you can run the tests; here is what I do and what I get:

dg@tvenkinys:~/Programavimas/MOK/ci4bugDemo$ php spark migrate

CodeIgniter v4.4.4 Command Line Tool - Server Time: 2024-01-10 22:14:45 UTC+00:00

Running all new migrations...
        Running: (App) 2024-01-10-214032_App\Database\Migrations\CreateTestTable
Migrations complete.
dg@tvenkinys:~/Programavimas/MOK/ci4bugDemo$ php spark db:table invoices --metadata

CodeIgniter v4.4.4 Command Line Tool - Server Time: 2024-01-10 22:14:56 UTC+00:00

List of Metadata Information in Table "invoices":

+----------------+----------+------------+----------+---------+-------------+
| Field Name     | Type     | Max Length | Nullable | Default | Primary Key |
+----------------+----------+------------+----------+---------+-------------+
| id             | INTEGER  |            | Yes      |         | Yes         |
| user_id        | INT      |            | No       |         | No          |
| month          | VARCHAR  |            | No       |         | No          |
| invoice_series | VARCHAR  |            | No       |         | No          |
| total          | DECIMAL  |            | No       |         | No          |
| paid           | TINYINT  |            | No       | 0       | No          |
| paid_date      | VARCHAR  |            | Yes      | NULL    | No          |
| created_at     | DATETIME |            | No       |         | No          |
| updated_at     | DATETIME |            | No       |         | No          |
+----------------+----------+------------+----------+---------+-------------+

dg@tvenkinys:~/Programavimas/MOK/ci4bugDemo$ php public/index.php home
dg@tvenkinys:~/Programavimas/MOK/ci4bugDemo$ php spark db:table invoices --metadata

CodeIgniter v4.4.4 Command Line Tool - Server Time: 2024-01-10 22:15:25 UTC+00:00

List of Metadata Information in Table "invoices":

+------------+----------+------------+----------+---------+-------------+
| Field Name | Type     | Max Length | Nullable | Default | Primary Key |
+------------+----------+------------+----------+---------+-------------+
| id         | INTEGER  |            | Yes      | NULL    | Yes         |
| user_id    | INT      |            | Yes      | NULL    | No          |
| month      | VARCHAR  |            | Yes      | NULL    | No          |
| series     | VARCHAR  |            | No       | ''      | No          |
| total      | DECIMAL  |            | Yes      | NULL    | No          |
| paid       | TINYINT  |            | No       | '0'     | No          |
| paid_date  | VARCHAR  |            | Yes      | 'NULL'  | No          |
| created_at | DATETIME |            | Yes      | NULL    | No          |
| updated_at | DATETIME |            | Yes      | NULL    | No          |
+------------+----------+------------+----------+---------+-------------+

Note that the only table field that had to change (and only in it's name) was invoice_series -> series, and not the massive changes in Nullable and Default columns.

Another strange thing: if I shorten the test code to this, omitting the array items for everything but the table column name:

$fields = [
        'invoice_series' => [
            'name'       => 'series',
        ],
    ];

The table name does not get changed (and nothing is corrupted). But then I add

'type' => 'VARCHAR',

and boom, all the damage is done after running the code.

Expected Output

I expect only the table column name to change.

Anything else?

I have not tested the other DB's, but I expect this to be an issue only in SQLite3 case (though someone might run my code on mysql to be sure).

[kenjis]

I have confirmed. But it seems very difficult to fix this issue.
SQLite cannot modify column, so Forge recreates the table, but it cannot get full information to create the exact same table.

I recommend you don't use Forge to modify column for now. Use SQL statements for SQLite directly.
SQLite added support for renaming column since version 3.25.0 using the ALTER TABLE statement.
See https://www.sqlitetutorial.net/sqlite-rename-column/

    public function testModifyColumnRename(): void
    {
        $this->forge->dropTable('forge_test_three', true);

        $this->forge->addField([
            'id' => [
                'type'           => 'INT',
                'constraint'     => 10,
                'auto_increment' => true,
                'null'           => false,
            ],
            'user_id' => [
                'type'       => 'INT',
                'constraint' => 10,
                'null'       => false,
            ],
            'month' => [
                'type'       => 'VARCHAR',
                'constraint' => 7,
                'null'       => false,
            ],
            'invoice_series' => [
                'type'       => 'VARCHAR',
                'constraint' => 10,
                'null'       => false,
            ],
        ]);

        $this->forge->addKey('id', true);
        $this->forge->createTable('forge_test_three');

        var_dump($this->db->getFieldData('forge_test_three'));

        $this->forge->modifyColumn('forge_test_three', [
            'invoice_series' => [
                'name'       => 'series',
                'type'       => 'VARCHAR',
                'constraint' => 10,
                'null'       => false,
            ],
        ]);

        $this->db->resetDataCache();

        var_dump($this->db->getFieldData('forge_test_three'));
        exit;
    }

Runtime:       PHP 8.3.0
Configuration: /.../CodeIgniter4/phpunit.xml

.../CodeIgniter4/tests/system/Database/Live/ForgeTest.php:1312:
array(4) {
  [0] =>
  class stdClass#250 (6) {
    public $name =>
    string(2) "id"
    public $type =>
    string(7) "INTEGER"
    public $max_length =>
    NULL
    public $default =>
    NULL
    public $primary_key =>
    bool(true)
    public $nullable =>
    bool(true)
  }
  [1] =>
  class stdClass#80 (6) {
    public $name =>
    string(7) "user_id"
    public $type =>
    string(3) "INT"
    public $max_length =>
    NULL
    public $default =>
    NULL
    public $primary_key =>
    bool(false)
    public $nullable =>
    bool(false)
  }
  [2] =>
  class stdClass#81 (6) {
    public $name =>
    string(5) "month"
    public $type =>
    string(7) "VARCHAR"
    public $max_length =>
    NULL
    public $default =>
    NULL
    public $primary_key =>
    bool(false)
    public $nullable =>
    bool(false)
  }
  [3] =>
  class stdClass#82 (6) {
    public $name =>
    string(14) "invoice_series"
    public $type =>
    string(7) "VARCHAR"
    public $max_length =>
    NULL
    public $default =>
    NULL
    public $primary_key =>
    bool(false)
    public $nullable =>
    bool(false)
  }
}
.../CodeIgniter4/tests/system/Database/Live/ForgeTest.php:1325:
array(4) {
  [0] =>
  class stdClass#77 (6) {
    public $name =>
    string(2) "id"
    public $type =>
    string(7) "INTEGER"
    public $max_length =>
    NULL
    public $default =>
    string(4) "NULL"
    public $primary_key =>
    bool(true)
    public $nullable =>
    bool(true)
  }
  [1] =>
  class stdClass#631 (6) {
    public $name =>
    string(7) "user_id"
    public $type =>
    string(3) "INT"
    public $max_length =>
    NULL
    public $default =>
    string(4) "NULL"
    public $primary_key =>
    bool(false)
    public $nullable =>
    bool(true)
  }
  [2] =>
  class stdClass#79 (6) {
    public $name =>
    string(5) "month"
    public $type =>
    string(7) "VARCHAR"
    public $max_length =>
    NULL
    public $default =>
    string(4) "NULL"
    public $primary_key =>
    bool(false)
    public $nullable =>
    bool(true)
  }
  [3] =>
  class stdClass#250 (6) {
    public $name =>
    string(6) "series"
    public $type =>
    string(7) "VARCHAR"
    public $max_length =>
    NULL
    public $default =>
    string(2) "''"
    public $primary_key =>
    bool(false)
    public $nullable =>
    bool(false)
  }
}

Process finished with exit code 0

[dgvirtual]

I recommend you don't use Forge to modify column for now. Use SQL statements for SQLite directly. SQLite added support for renaming column since version 3.25.0 using the ALTER TABLE statement. See https://www.sqlitetutorial.net/sqlite-rename-column/

That is an acceptable workaround in my case, thanks! I am running a few projects where sqlite3 is used in production, and I run into this problem after doing some optimizations on database.

I guess in most cases sqlite3 is used in testing (while mysql – in production), and so it will mess up the tests only (provided Forge renaming is used). Still, this issue should perhaps be mentioned in docs.

[kenjis]

@dgvirtual By the way, why do you set null to default?
What do you want?

            'paid_date' => [
                'type'       => 'VARCHAR',
                'constraint' => 10,
                'default'    => null,
            ],

[dgvirtual]

While the invoice is not paid, the value is not set... It is usually set later than the invoice entry is created. I guess empty string would work as well. What would the best practice be? 2024-01-19, pn 04:49, kenjis ***@***.***> rašė:

…

@dgvirtual By the way, why do you set null to default? What do you want? 'paid_date' => [ 'type' => 'VARCHAR', 'constraint' => 10, 'default' => null, ],

[kenjis]

If you permitNULL, you can make the column nullable with ['null' => true].
https://codeigniter4.github.io/CodeIgniter4/dbmgmt/forge.html#adding-fields

I don't see the need to dare to add the DEFALUT NULL constraint.
Is there any reason to do so?

E.g.,

            'number' => [
                'type'       => 'INT',
                'null'       => true,
            ],

sqlite> .schema test1
CREATE TABLE IF NOT EXISTS "test1" (
  "id" INTEGER PRIMARY KEY AUTOINCREMENT,
  "name" VARCHAR NOT NULL,
  "number" INT NULL
);

sqlite> INSERT INTO test1 (name) VALUES ('Smith');

sqlite> .mode box
sqlite> .nullvalue (NULL)
sqlite> SELECT * FROM test1;
┌────┬───────┬────────┐
│ id │ name  │ number │
├────┼───────┼────────┤
│ 1  │ Smith │ (NULL) │
└────┴───────┴────────┘

[dgvirtual]

I don't see the need to dare to add the DEFALUT NULL constraint.
Is there any reason to do so?

No need whatsoever, as I now realize. I didn't realize there was a difference :)

[kenjis]

I found one bug that makes SQLite table mess up.
https://github.com/codeigniter4/CodeIgniter4/pull/8434/files#diff-f4ae4edd52ba64de6cabc5b7aa455081daa4b870f9e22ff9f39a2ccb6fd2c690R136
But we still need a method to know existing column is autoincrement or not.

[dgvirtual]

$this->forge->addForeignKey('user_id', 'users', 'id', 'NO ACTION', 'CASCADE');
$this->forge->processIndexes('invoices');

These commands make the same mess in an SQLite3 a table.

[sclubricants]

Don't call processIndexes() after addForeignKey().

[dgvirtual]

Don't call processIndexes() after addForeignKey().

But I see in docs it is how adding foreign keys to existing tables is to be done: https://codeigniter4.github.io/userguide/dbmgmt/forge.html#adding-keys-to-a-table

[kenjis]

@dgvirtual The fromTable() method does not work, because there is no API to get auto_increment columns. If fromTable() processes a table with columns with auto_increment, the auto_increment will be lost.

So all methods that calls fromTable() does not work now if you use auto_increment.

CodeIgniter4/system/Database/SQLite3/Table.php

Lines 86 to 93 in 2190110

	/**
	* Reads an existing database table and
	* collects all of the information needed to
	* recreate this table.
	*
	* @return Table
	*/
	public function fromTable(string $table)

And there is another bug that messes up field definitions in here:

CodeIgniter4/system/Database/SQLite3/Forge.php

Line 136 in 2190110

->modifyColumn($processedFields) // @TODO Bug: should be NOT processed fields