"FOREIGN KEY constraint failed" after updating to 4.4.0

[kolorafa]

This is a (multiple allowed):

• bug

• enhancement

• feature-discussion (RFC)

• CakePHP Version: 5.1.1

• Migrations plugin version: 4.4.0 (works when downgraded to 4.3.2)

• Database server (MySQL, SQLite, Postgres): SQLite Library => 3.37.2

• PHP Version: 8.3

• Platform / OS: Ubuntu-20.04 LTS / Linux Mint 21.03

What you did

1. use SQLite
2. have existing table with data and constraints set
3. add new column to table

        $this->table('some_table')
            ->addColumn('description', AdapterInterface::PHINX_TYPE_JSON, [
                'null' => true,
                'default' => null,
            ])
            ->update();

Expected Behavior

Work :)

Actual Behavior

Previous error: PDOException:
SQLSTATE[23000]: Integrity constraint violation: 19 FOREIGN KEY constraint failed
#0 /home/kolorafa/src/Impress-Connect-PHP/vendor/cakephp/cakephp/src/Database/Statement/Statement.php(160): PDOStatement->execute()

The issue happen in:
vendor/cakephp/migrations/src/Db/Adapter/SqliteAdapter.php:1049

It looks like the logic for the alter is to copy the table to tmp_some_table, then drop existing and rename the new one.
I tracked the issue down to some old issue
cakephp/phinx#2130
ndm2/phinx@9191730#diff-8bd456d859714dd59755ea246b4d2e1584560144dde972213c28faf1d51eef8eR849

[markstory]

Thanks for opening this issue. How could I reproduce this problem?

have existing table with data and constraints set

What does the table schema look like? Does some_table have inbound foreign keys from another table?

[kolorafa]

Good point, forgot to investigate and mention the real cause of this issue.

The bug occur because this table is in relation ManyToMany with other table, and we have field constraints between the primary key in this model and the join table table that connects those 2 tables.

So the way to reproduce it would be:

1. create roles table
2. create users table
3. create users_roles table (and optionally set ManyToMany relation between Roles and Users, should not be needed to trigger issue)
4. add field constraint between roles.id and users_roles.roles_id
5. add some row to roles table, and add some related row to users_roles table with matching values - to trigger constraint
6. do the migration to add additional field to roles table,

The actual behavior:

Because the sqlite logic is not just modifying the original table, but rather creating totally new table, copying the values.
The copying values is fine, but the third step that drop the existing table will fail because there are still constraints connecting those 2 tables so the engine will not allow for table deletion.

My guess the issue will be the same for both ManyToMany relation and also for hasMany relation if you have a proper field constraint set on the relation.

[markstory]

Thanks for the more detailed steps. I'll have some time in a few days to look into this.

[markstory]

I was able to reproduce the error you're seeing, but I'm not sure that another foreign_keys = OFF will help here. With the following migrations

<?php
declare(strict_types=1);

use Migrations\AbstractMigration;

class CreateUsersRolesTables extends AbstractMigration
{
    /**
     * Change Method.
     *
     * More information on this method is available here:
     * https://book.cakephp.org/phinx/0/en/migrations.html#the-change-method
     * @return void
     */
    public function change(): void
    {
        $roles = $this->table('constraint_roles');
        $roles->addColumn('name', 'string')
            ->create();

        $users = $this->table('constraint_users');
        $users->addColumn('username', 'string')
            ->addColumn('role_id', 'integer', ['null' => false])
            ->addForeignKey(['role_id'], $roles->getTable(), ['id'])
            ->create();

        $adapter = $this->getAdapter();
        $adapter->insert($roles->getTable(), ['name' => 'admin']);
        $adapter->insert($users->getTable(), ['username' => 'test', 'role_id' => 1]);
    }
}

and

<?php
declare(strict_types=1);

use Migrations\AbstractMigration;

class ModifyRoles extends AbstractMigration
{
    /**
     * Change Method.
     *
     * More information on this method is available here:
     * https://book.cakephp.org/phinx/0/en/migrations.html#the-change-method
     * @return void
     */
    public function change(): void
    {
        $roles = $this->table('constraint_roles');
        $roles
            ->addColumn('description', 'string', ['default' => 'short desc'])
            ->update();
    }
}

I'm able to reproduce the issue if my connection includes init => ['PRAGMA foreign_keys = ON']. The current SQL log of migrations 4.4.0 for the second migration is

CREATE TABLE `tmp_constraint_roles` (`id` INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, `name` VARCHAR NOT NULL, `description` VARCHAR NOT NULL DEFAULT 'short desc');
PRAGMA foreign_keys = OFF;
INSERT INTO `tmp_constraint_roles`(`id`, `name`) SELECT `id`, `name` FROM `constraint_roles`;
DROP TABLE `constraint_roles`;

foreign_keys are already disabled. I think what is happening is a bug in the migrations/phinx integration where init commands defined in Cake aren't set in the phinx connection, so migrations run without foreign key checks with sqlite. I'll do some more validation on that theory later this week.

[kolorafa]

I also don't think that doing foreign_keys = OFF is the way to go.

Because the logic is creating new table and doing rename, just disabling it would still result in having bad table state.
As the newly created table would not have any (previously existing) indexes and foreign constraints.

If it can't be just altered, then the proper way would be to actually list all indexes, remove them, and them add them back later. This way foreign constraint errors should not happen.

---

Thanks for your investigation, will also try to debug it more, starting with printing all the sql commands it execute (and try to pinpoint it more specific).

[markstory]

As the newly created table would not have any (previously existing) indexes and foreign constraints.

Yes, the newly created table is lacking constraints. Constraints are currently not part of the schema that is preserved. Only triggers and indicies are preserved. This is also how phinx behaved.

We should be able to expand the functionality of sqlite schema changes to also preserve foreign keys. We'll need to collect all existing foreign keys, generate or mangle the existing constraint definitions and drop/create new constraints.

[markstory]

I think I've tracked this down to how transactions are being managed. The existing logic in migrations will use PRAGMA foreign_keys = OFF correctly as was done in phinx. What is different now is that there is a transaction, and within a transaction SQLite doesn't allow foreign_key state to be modified. I checked this with a short SQLite session:

ᐉ sqlite3 app.sqlite
SQLite version 3.37.2 2022-01-06 13:25:41
Enter ".help" for usage hints.
sqlite> pragma foreign_keys = on;
sqlite> pragma foreign_keys;
1
sqlite> pragma foreign_keys = off;
sqlite> pragma foreign_keys;
0
sqlite> pragma foreign_keys = on;
sqlite> begin;
sqlite> pragma foreign_keys = off;
sqlite> pragma foreign_keys;
1
sqlite> commit;
sqlite> pragma foreign_keys;
1

While phinx also used transactions, the migrations plugin did not preserve the init commands when connection phinx uses is created. This resulted in foreign keys not being enabled for migrations. I wasn't able to reproduce this scenario in tests because the driver test methods aren't executed within a transaction. A workaround that will work until a proper solution can be created would be to add

$this->getAdapter()->rollbackTransaction();
// Migration contents here
$this->getAdapter()->beginTransaction();

This will close the implicit transactions that migrations is creating, allow your migration to run and then open a new empty transaction. This tricks the framework into doing the right thing. I'll work on a fix now that the problem has been identified.

[markstory]

@kolorafa I opened #745 to add a hook method to migrations that allow you to escape the wrapping transaction if you need (which you now do for sqlite alters).

I considered making this behavior automatic, but felt that it could have unsafe consequences and chose an opt-in API instead. I would appreciate any feedback you have on this solution.