Bug: createTable() method in Forge class checks for table existence using cache and causes problems

[AnakinA1]

PHP Version

8.1

CodeIgniter4 Version

4.2.2

CodeIgniter4 Installation Method

Composer (as dependency to an existing project)

Which operating systems have you tested for this bug?

macOS, Linux

Which server did you use?

fpm-fcgi

Database

MariaDB 10.8

What happened?

In the new minor version of CI Database/Forge class has now:

// If table exists lets stop here
if ($ifNotExists === true && $this->db->tableExists($table)) {
    $this->reset();

    return true;
}

Let's focus on $this->db->tableExists($table) if flag $ifNotExists is true

Will give a real example of issue:

For tests I have a migration which fully drop current database and restore it from the provided SQL file. I'm doing it with direct db connection ($this->db->mysqli) using multi_query(). Then I need to create a migration table in order for the migration system to work. I'm doing it with Forge class, because it's a framework technical table and should not be included into project SQL scheme.

Steps to Reproduce

In the migration file:

• drop db
• add db
• then try to create migration table if it doesn't exist (to prevent possible error on failing)
• table is not created, because the check $this->db->tableExists($table) uses cache

This example only applies to my case, but let's assume the following:

Just drop any table with query method and then try to restore it via forge class

Expected Output

Forge class shouldn't rely on cache, or should be highlighted in the documentation that Forge class relies on the cache in its work and should not be used (or it is necessary to clear the cache) when working with direct querying, not through the builder

Anything else?

Ref #6249

[sclubricants]

Perhaps the method should have a parameter that allows for specifying whether to get cached results or not.

$this->db->tableExists($table, $cached = true);

Then could could call it with false before running table create.

Cache works fine though if you stay within the framework.

Same goes cache for indexes.

[kenjis]

Yes, I agree with the following.

$this->db->tableExists($table, $cached = true);

[sclubricants]

Problem is that tableExists() gets its data from listTables().. so its not as easy as just looking up one table.

This could load a list of hundreds of tables. Not somethings I'd want to do before every createTable()

We could add a fair bit of code to handle just one table.. OR

My suggestion is that if you are manipulating the database outside of the framework then you should call:

$this->db->resetDataCache();

If you call this first then it will rebuild the cache and things should work. This seems like the ideal solution being that you have manipulated things outside of Forge. It would be wise to call this regardless.

We might need to add something about resetDataCache() in docs.. https://codeigniter4.github.io/CodeIgniter4/dbmgmt/forge.html?highlight=forge

[kenjis]

You cannot add a public method in patch release.
It must go to 4.3.

[sclubricants]

There is issues when dropping and recreating database. This is a messy operation and requires two connections to forge.

        $forgeDefault = \Config\Database::forge('default');
        
        // create new database
        $forgeDefault->createDatabase('newdb', true);
        
        // create new database forge
        $forgeNewdb = \Config\Database::forge('newdb');

        // create user table in new database
        $forgeNewdb->addField([
        'id'         => ['type' => 'INTEGER', 'constraint' => 3, 'auto_increment' => true],
        'name'       => ['type' => 'VARCHAR', 'constraint' => 80],
        'email'      => ['type' => 'VARCHAR', 'constraint' => 100],
        'country'    => ['type' => 'VARCHAR', 'constraint' => 40],
        'created_at' => ['type' => 'DATETIME', 'null' => true],
        'updated_at' => ['type' => 'DATETIME', 'null' => true],
        'deleted_at' => ['type' => 'DATETIME', 'null' => true],
        ])->addKey('id', true)->addUniqueKey('email')->addKey('country')->createTable('user', true);

        // drop new database
        $forgeDefault->dropDatabase('newdb');
        
        // create new database again
        $forgeDefault->createDatabase('newdb', true);
        
        // Only with this call does the following table get created
        $forgeNewdb->getConnection()->resetDataCache();        

        // create user table if it doesn't exist - it doesn't because database was dropped
        $forgeNewdb->addField([
        'id'         => ['type' => 'INTEGER', 'constraint' => 3, 'auto_increment' => true],
        'name'       => ['type' => 'VARCHAR', 'constraint' => 80],
        'email'      => ['type' => 'VARCHAR', 'constraint' => 100],
        'country'    => ['type' => 'VARCHAR', 'constraint' => 40],
        'created_at' => ['type' => 'DATETIME', 'null' => true],
        'updated_at' => ['type' => 'DATETIME', 'null' => true],
        'deleted_at' => ['type' => 'DATETIME', 'null' => true],
        ])->addKey('id', true)->addUniqueKey('email')->addKey('country')->createTable('user', true);        
        
        // No table is created unless called $forgeNewdb->getConnection()->resetDataCache();

You have to call $forge->getConnection()->resetDataCache(); on the connection that creates the tables in the new database.

I looked to add $this->db->resetDataCache(); to Forge::dropDatabase() but even if you do it doesn't help because you can't drop the database from a connection using the database.

It might not be a bad idea to do a tableExists() without cache and actually verify the table's existence.

[AnakinA1]

About discarding the cache, I may have generalized too much for the Forge class, it can really be useful in some cases. But I would suggest doing:

For such operations, where there is IF NOT EXISTS, remove checks for the existence of tables / databases in the Forge class itself. Because, logically, such operations are low-cost, and from the fact that you use a cache check instead of a direct query to the database, I'm afraid there may not be even a millisecond gain. Moreover, such operations are most often done in migrations, in which the execution time actually may not play a role in order to save it so much, and at the same time create problems when working with the framework.

As a result, we will get rid of the current problems associated with the cache, without losing the performance of the Forge class itself.

[kenjis]

@AnakinA1 Now we don't use IF NOT EXISTS clause.
We removed it in #6249

[AnakinA1]

ok, got it, thanks