fix: disable SQLite FK checks in synchronize / migrations

[imnotjames]

Description of change

this moves the PRAGMA calls to where we need them in the sqlite migration generation / synchronize

fixes #2576

Pull-Request Checklist

• Code is up-to-date with the master branch
• npm run lint passes with this change
• npm run test passes with this change
• This pull request links relevant issues as Fixes #0000
• There are new or updated unit tests validating the change
• Documentation has been updated to reflect this change
• The new commits follow conventions explained in CONTRIBUTING.md

[imnotjames]

It is not possible to enable or disable foreign key constraints in the middle of a multi-statement transaction (when SQLite is not in autocommit mode). Attempting to do so does not return an error; it simply has no effect.

[imnotjames]

So I think this approach won't work but I have a test that should confirm that..

[pleerock]

can't these methods be defined in AbstractSqliteQueryRunner?

[pleerock]

🤔

[phil294]

If I understand this correctly, migrations can now go haywire with FKs, but at the same time, they are guaranteed to not have you lose data acidentally because of ON DELETE CASCADE constraints. This was pretty daunting until now. However, is simply disabling FKs a good idea? It will now be possible to write migrations that break the internal consistency of the database. Until now, setting PRAGMA foreign_keys=OFF was tedious, but at least you had to know what you were doing.

[ed-curran]

FYI for anybody that comes accross this trying to figure out how to use PRAGMA foreign_keys=OFF in migrations, this change doesn't seem to apply to migrations:run (or when using migrationsRun: true), or migrations:revert. I don't know if that was intentional or not.

Explanation in case it wasn't intentional:
MigrationExecuter.executeMigration() was updated for this change (to use PRAGMA foreign_keys=OFF if using an sqlite driver). However Connection.runMigrations() uses MigrationExecuter.executePendingMigrations() which doesn't use MigrationExecuter.executeMigration() so hence this change didn't do anything for migrations:run.
For migrations:revert queryRunner.beforeMigration() and queryRunner.afterMigration() do get called, but inside the transaction that wraps the down() migration and so the PRAGMA foreign_keys=OFF in beforeMigration() doesn't do anything

As a result the workaround to do this for 0.2.x is still to run the migrations yourself like this (and not use migrationsRun: true), however this means you can't use the cli for migrations:run or revert.

connection.query(`PRAGMA foreign_keys=OFF`);
connection.runMigrations();
connection.query(`PRAGMA foreign_keys=ON`);

A longer term solution could be to have a beforeTransaction and afterTransaction hook inside MigrationInterface which would allow the migration author to set PRAGMA foreign_keys=ON if they want it.

[sinopsysHK]

Hello, this might be working well when we execute one specific migration.
But it does not apply for pending migrations being processed automatically when DataSource is initialize (if any):

typeorm/src/migration/MigrationExecutor.ts

Lines 334 to 361 in c669f50

	await migration
	.instance!.up(queryRunner)
	.catch((error) => {
	// informative log about migration failure
	this.connection.logger.logMigration(
	`Migration "${migration.name}" failed, error: ${error?.message}`,
	)
	throw error
	})
	.then(async () => {
	// now when migration is executed we need to insert record about it into the database
	await this.insertExecutedMigration(
	queryRunner,
	migration,
	)
	// commit transaction if we started it
	if (migration.transaction && transactionStartedByUs)
	await queryRunner.commitTransaction()
	})
	.then(() => {
	// informative log about migration success
	successMigrations.push(migration)
	this.connection.logger.logSchemaBuild(
	`Migration ${migration.name} has been ${
	this.fake ? "(fake)" : ""
	} executed successfully.`,
	)
	})

Adding here as well beforeMigration and afterMigration would be consistent

[netroy]

@imnotjames Hi 👋🏽 ,
was it intentional that the beforeMigration / afterMigration calls were not added in executePendingMigrations ?