From 197cc05e90c0182357d85aa1ce7ae45de99d9d98 Mon Sep 17 00:00:00 2001 From: sinopsysHK Date: Thu, 6 Apr 2023 16:12:10 +0800 Subject: [PATCH] fix: prevent foreign key support during migration batch under sqlite (#9775) * fix: prevent foreign keys support during migration batch under sqlite Schema changes in migrations batch cannot be done on table which has referring foreign keys with ON DELETE CASCADE without deleting its content. Closes: #9770 * Update MigrationExecutor.ts * Update command.ts --------- Co-authored-by: Umed Khudoiberdiev --- src/migration/MigrationExecutor.ts | 10 ++- .../migrations/show-command/command.ts | 2 +- test/github-issues/9770/entity/Bar.ts | 26 ++++++ test/github-issues/9770/entity/Foo.ts | 9 ++ test/github-issues/9770/issue-9770.ts | 87 +++++++++++++++++++ .../9770/migration/1675779246631-amendFoo.ts | 19 ++++ 6 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 test/github-issues/9770/entity/Bar.ts create mode 100644 test/github-issues/9770/entity/Foo.ts create mode 100644 test/github-issues/9770/issue-9770.ts create mode 100644 test/github-issues/9770/migration/1675779246631-amendFoo.ts diff --git a/src/migration/MigrationExecutor.ts b/src/migration/MigrationExecutor.ts index 2ea4b17a5..4e15dcf7e 100644 --- a/src/migration/MigrationExecutor.ts +++ b/src/migration/MigrationExecutor.ts @@ -311,6 +311,7 @@ export class MigrationExecutor { // start transaction if its not started yet let transactionStartedByUs = false if (this.transaction === "all" && !queryRunner.isTransactionActive) { + await queryRunner.beforeMigration() await queryRunner.startTransaction() transactionStartedByUs = true } @@ -327,6 +328,7 @@ export class MigrationExecutor { } if (migration.transaction && !queryRunner.isTransactionActive) { + await queryRunner.beforeMigration() await queryRunner.startTransaction() transactionStartedByUs = true } @@ -347,8 +349,10 @@ export class MigrationExecutor { migration, ) // commit transaction if we started it - if (migration.transaction && transactionStartedByUs) + if (migration.transaction && transactionStartedByUs) { await queryRunner.commitTransaction() + await queryRunner.afterMigration() + } }) .then(() => { // informative log about migration success @@ -362,8 +366,10 @@ export class MigrationExecutor { } // commit transaction if we started it - if (this.transaction === "all" && transactionStartedByUs) + if (this.transaction === "all" && transactionStartedByUs) { await queryRunner.commitTransaction() + await queryRunner.afterMigration() + } } catch (err) { // rollback transaction if we started it if (transactionStartedByUs) { diff --git a/test/functional/migrations/show-command/command.ts b/test/functional/migrations/show-command/command.ts index c31901f76..35657342e 100644 --- a/test/functional/migrations/show-command/command.ts +++ b/test/functional/migrations/show-command/command.ts @@ -12,7 +12,7 @@ describe("migrations > show command", () => { async () => (connections = await createTestingConnections({ migrations: [__dirname + "/migration/*.js"], - enabledDrivers: ["postgres"], + enabledDrivers: ["postgres", "sqlite"], schemaCreate: true, dropSchema: true, })), diff --git a/test/github-issues/9770/entity/Bar.ts b/test/github-issues/9770/entity/Bar.ts new file mode 100644 index 000000000..7d2bbcda6 --- /dev/null +++ b/test/github-issues/9770/entity/Bar.ts @@ -0,0 +1,26 @@ +import { + Entity, + Column, + ManyToOne, + JoinColumn, + PrimaryGeneratedColumn, +} from "../../../../src" + +import { Foo } from "./Foo" + +@Entity() +export class Bar { + @PrimaryGeneratedColumn() + id: number + + @ManyToOne(() => Foo, { + cascade: true, + onDelete: "CASCADE", + nullable: false, + }) + @JoinColumn() + foo!: Foo + + @Column() + data: string +} diff --git a/test/github-issues/9770/entity/Foo.ts b/test/github-issues/9770/entity/Foo.ts new file mode 100644 index 000000000..5512d8957 --- /dev/null +++ b/test/github-issues/9770/entity/Foo.ts @@ -0,0 +1,9 @@ +import { Entity, Column, PrimaryGeneratedColumn } from "../../../../src" + +@Entity() +export class Foo { + @PrimaryGeneratedColumn() id: number + + @Column() + data: string +} diff --git a/test/github-issues/9770/issue-9770.ts b/test/github-issues/9770/issue-9770.ts new file mode 100644 index 000000000..0e671fbeb --- /dev/null +++ b/test/github-issues/9770/issue-9770.ts @@ -0,0 +1,87 @@ +import "reflect-metadata" +import { expect } from "chai" + +import { DataSource } from "../../../src" +//import { DataSource, TableColumn } from "../../../src" +import { + closeTestingConnections, + createTestingConnections, + reloadTestingDatabases, +} from "../../utils/test-utils" + +import { Foo } from "./entity/Foo" +import { Bar } from "./entity/Bar" + +describe("github issues > #9770 check for referencing foreign keys when altering a table using sqlite", () => { + let dataSources: DataSource[] + before(async () => { + dataSources = await createTestingConnections({ + entities: [__dirname + "/entity/*{.js,.ts}"], + migrations: [__dirname + "/migration/*{.js,.ts}"], + enabledDrivers: ["sqlite", "better-sqlite3"], + schemaCreate: true, + dropSchema: true, + logging: true, + }) + }) + beforeEach(() => reloadTestingDatabases(dataSources)) + after(() => closeTestingConnections(dataSources)) + + it("shouldn't loose dependant table data", () => + Promise.all( + dataSources.map(async (dataSource) => { + const manager = dataSource.manager + + // Insert records in the tables + const foo = new Foo() + foo.data = "foo" + await manager.save(foo) + const foundFoo = await manager.findOne(Foo, { + where: { + id: 1, + }, + }) + expect(foundFoo).not.to.be.null + + if (!foundFoo) return + + const bar = new Bar() + bar.foo = foundFoo + bar.data = "bar" + await manager.save(bar) + + const foundBar = await manager.findOne(Bar, { + where: { + foo: { + id: foundFoo.id, + }, + }, + }) + expect(foundBar).not.to.be.null + + // check current state (migrations pending and entries in db) + const migrations = await dataSource.showMigrations() + migrations.should.be.equal(true) + + const queryRunner = dataSource.createQueryRunner() + let barRecords = await queryRunner.query(`SELECT * FROM "bar"`) + expect(barRecords).to.have.lengthOf.above(0) + + // run migrations which contains a table drop + await dataSource.runMigrations() + + // check post migration (no more pending migration and data still in db) + const migrations2 = await dataSource.showMigrations() + migrations2.should.be.equal(false) + + // check if data still exists in dependant table + barRecords = await queryRunner.query(`SELECT * FROM "bar"`) + expect(barRecords).to.have.lengthOf.above(0) + + // revert changes + await queryRunner.executeMemoryDownSql() + + await queryRunner.release() + }), + )) +}) diff --git a/test/github-issues/9770/migration/1675779246631-amendFoo.ts b/test/github-issues/9770/migration/1675779246631-amendFoo.ts new file mode 100644 index 000000000..f0c006b30 --- /dev/null +++ b/test/github-issues/9770/migration/1675779246631-amendFoo.ts @@ -0,0 +1,19 @@ +import { MigrationInterface, QueryRunner, TableColumn } from "../../../../src" + +export class amendFoo1675779246631 implements MigrationInterface { + public async up(q: QueryRunner): Promise { + await q.addColumn( + "foo", + new TableColumn({ + name: "comment", + type: "varchar", + isNullable: true, + default: null, + }), + ) + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.dropColumn("foo", "comment") + } +}