Fix Sqlite Table::createTable()

[sclubricants]

This PR fixes #6205

The indexes need to be checked for missing fields on composite keys.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[sclubricants]

@MGatner check and see if this resolves #6205 for you.

[MGatner]

I will test when I'm on desktop. I actually ended up refactoring that whole table out of the repo that had the issue but I can pull an older version of the code that should still have the bug.

[sclubricants]

Looking at your code It looks like if you want to restore it to your original table then you will have to add the composite index back.

    private function createActions(): void
    {
        $this->forge = \Config\Database::forge('tests');
        
        $this->forge->dropTable('actions', true);
        // Actions
        $fields = [
            'category'    => ['type' => 'varchar', 'constraint' => 63],
            'name'        => ['type' => 'varchar', 'constraint' => 63],
            'uid'         => ['type' => 'varchar', 'constraint' => 63],
            'class'       => ['type' => 'varchar', 'constraint' => 63, 'null' => true],
            'input'       => ['type' => 'varchar', 'constraint' => 63, 'null' => true],
            'role'        => ['type' => 'varchar', 'constraint' => 63, 'default' => ''],
            'icon'        => ['type' => 'varchar', 'constraint' => 63, 'default' => ''],
            'summary'     => ['type' => 'varchar', 'constraint' => 255, 'default' => ''],
            'description' => ['type' => 'text', 'null' => true],
            'created_at'  => ['type' => 'datetime', 'null' => true],
            'updated_at'  => ['type' => 'datetime', 'null' => true],
            'deleted_at'  => ['type' => 'datetime', 'null' => true],
        ];

        $this->forge->addField('id');
        $this->forge->addField($fields);

        $this->forge->addKey('name');
        $this->forge->addKey('uid');
        $this->forge->addKey(['category', 'name']);
        $this->forge->addKey(['deleted_at', 'id']);
        $this->forge->addKey('created_at');

        $this->forge->createTable('actions');
    }

    public function up(): void
    {
        $this->forge->dropColumn('actions', [
            'description',
            'summary',
            'category',
            'icon',
            'role',
            'class',
        ]);
    }

    public function down(): void
    {
        $this->forge->addColumn('actions', [
            'class'       => ['type' => 'varchar', 'constraint' => 63, 'null' => true],
            'role'        => ['type' => 'varchar', 'constraint' => 63, 'default' => ''],
            'icon'        => ['type' => 'varchar', 'constraint' => 63, 'default' => ''],
            'category'    => ['type' => 'varchar', 'constraint' => 63, 'default' => ''],
            'summary'     => ['type' => 'varchar', 'constraint' => 255],
            'description' => ['type' => 'text', 'constraint' => 255],
        ]);

        // NEED TO ADD BACK COMPOSITE INDEX
        $this->forge->addKey(['category', 'name'])->processIndexes('actions');
    }

This would require a new method to Forge:

    /**
     * Executes Sql to add or remove indexes
     */
    public function processIndexes(string $table): bool
    {
        if (! empty($this->keys)) {
            if (empty($this->fields)) {
                $this->fields = array_flip(array_map(
                    static fn ($columnName) => $columnName->name,
                    $this->db->getFieldData($this->db->DBPrefix . $table)
                ));
            }

            for ($i = 0, $sqls = $this->_processIndexes($this->db->DBPrefix . $table), $c = count($sqls); $i < $c; $i++) {
                if ($this->db->query($sqls[$i]) === false) {
                    return false;
                }
            }
        }

        $this->reset();

        return true;
    }

Then of course we would need to test it for all DBMS.

I'm not sure why this method doesn't exist. There is a dropKey() method that executes but the addKey() method does not. I know its because sometimes the keys are created with tables while others do not. This really ought to be fixed.

[MGatner]

I agree in the key methods. I think needing to re-add the key is fine, the important part is that the alteration doesn't cause a database error. If you've tested that with this PR then I will proceed with the review.

[sclubricants]

I've found there are a lot of issues related to this. In MySql, if you have a composite index named category_name and then you drop the column category, the index drops the field category and retains name.. and still assumes the index name category_name. So then if you try and add the index category_name back in then it throws an error. Things really get messy when dealing with auto increment primary keys.

I think we need to just leave it up to the developer to properly manage the indexes. In your example you should have dropped any related indexes before dropping the columns.. then add back those indexes after recreating the columns.

One thing I realized though is that there is no method to drop primary keys and no method to create indexes on existing tables. I'm working on a separate PR to add these methods. At least in MySql dropKey() doesn't work with primary key and requires a different SQL statement.

The code in this PR should stop the error you were reporting, however it still doesn't provide a way to add the index back.

[kenjis]

The code in this PR should stop the error you were reporting, however it still doesn't provide a way to add the index back.

Should this PR be closed because this seems jiust a workaround to prevent the error?

[sclubricants]

No this PR is still necessary. It fixes a bug. SQLite uses a Table class unlike the rest of the DBMS. It basically copies the table but on recreate doesn't create fields that were dropped. The problem was that it was trying to create indexes for fields that were dropped.

I'm working on another PR that adds the features to create indexes after table creation. I can add that in here if you would rather. I was going to submit that one to 4.3

[kenjis]

The problem was that it was trying to create indexes for fields that were dropped.

I got it. Can you add test code?

I'm working on another PR that adds the features to create indexes after table creation. I can add that in here if you would rather. I was going to submit that one to 4.3

#4814
It is an enhancement. Please send to 4.3.

[MGatner]

All this tracks with what we've experienced around Forge and the different drivers. I'm happy to have this in as a bugfix, and look forward to what other changes you have for these other issues. Hopefully we can get some DB people to review.

[sclubricants]

Added testDropColumnDropCompositeKey()

[MGatner]

That test looks strangely familiar 😂
Thanks @sclubricants this looks good.

[kenjis]

Thank you! LGTM!