Add new transaction mode to wrap each migration in transaction

[nlenepveu]

Hello people!

This work is based on the awesome work done by @garbles in #3208.

---

Addresses #2693

Presentation by @garbles from the original pull request:

Creates a new, backward-compatible mode for running migrations each in their own transaction. This behavior is what I think most expect as default from an ORM, so running all migrations in a single transaction seems odd. Using a single transaction also does not allow you to do certain things (in Postgres at least) if you expect to be able to run all migrations at once.

For example, if you have the following two migrations, you can never run them in a row.

export class CreateUsers0000000000002 implements MigrationInterface {
    public up(queryRunner: QueryRunner): Promise<any> {
        return queryRunner.createTable(
            new Table({
                name: "users",
                columns: [
                    {
                        name: "id",
                        type: "uuid",
                        isPrimary: true,
                        default: "uuid_generate_v4()"
                    }
                ]
            })
        );
    }

    public down(queryRunner: QueryRunner): Promise<any> {
        return queryRunner.dropTable("users");
    }
}

export class InsertUser0000000000003 implements MigrationInterface {
    public up(queryRunner: QueryRunner): Promise<any> {
        const userRepo = queryRunner.connection.getRepository<User>(User);
        return userRepo.save(new User());
    }

    public down(queryRunner: QueryRunner): Promise<any> {
        return Promise.resolve();
    }
}

The INSERT in the second migration will fail as the schema does not exist at that point. The first migration should be committed before enabling the second one to access the newly created table.

Changes to the -t flag to accept: all (run all migrations in a single transaction), each (run each migration in a separate transaction), none (run all migrations without transactions) or false (same as none).

typeorm migration:run --transaction=each
typeorm migration:run -t=each

TODO:

• Add new tests for 'each' mode
• Clean up code

@pleerock I addressed some of the comments you left some months ago. It would be great if you can have a look at this work!

[garbles]

Wow fantastic. Nice work @nlenepveu !

[pleerock]

Looks good

[antoine-pous]

@pleerock there's any due date for merging this PR? 😄

[garbles]

👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏

[antoine-pous]

🎉 WOOOP!!! 🎉

[obedm503]

@pleerock There's a breaking change here. runMigrations({ transaction: true }) is no longer valid. I understand typeorm is technically not stable, but it would be nice if the breaking change was documented in the changelog.

[brunoMaurice]

I don't understand why it's not possible to use transaction for all in your exemple.
I don't know for other db, but at least for postgres it's possible to have a create and insert for the created table in the same transaction.

The connection between your two migrations is not the same ?

[pleerock]

@obedm503 we might miss breaking changes easily. Please make sure to subscribe to TypeORM issues and pull requests and join others for PR reviews and left your feedback before they go into release version.

[nlenepveu]

@obedm503 Strictly speaking, I'm not sure we can talk about a breaking change here. { transaction: true } has never been officially supported in the codebase, only { transaction: false } had effects. It's like saying { transaction: "on" } is no longer working.

[obedm503]

@nlenepveu the function signature was async runMigrations(options?: { transaction?: boolean }) and not async runMigrations(options?: { transaction?: false }).
{ transaction: true } was a legitimately possible value.

This was not really an issue for me. Typescript type-checking helped me catch it. I was just commenting for documentation purposes.

[nlenepveu]

@obedm503 Indeed, you’re right. I missed that point.

[nlenepveu]

@brunoMaurice Indeed, this issue happens when you do not use the same connection. It appears that the QueryRunner injected in the up and down methods initiate a new connection for each migration (I did not verify this in the codebase but the negative test goes in that direction).
We could have fix the initial issue differently by ensuring that the same connection is shared between all migration.

[brunoMaurice]

@nlenepveu Thanks for the answer. Did you think, it can be an enhancement to use the same QueryRunner to keep the same connection for each migration and be able to use transaction:all even if we have insert statement ?

[nlenepveu]

@brunoMaurice definitely! It should be a better option to keep a single transaction between each migration.
When releasing the code of our apps and something bad happens, we would want to revert all the changes in the database if the code finally does not go live.