Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add new mode to wrap each migration in transaction #3208

Closed
wants to merge 2 commits into from

Conversation

garbles
Copy link

@garbles garbles commented Dec 6, 2018

Addresses #2693

Creates a new, backwards compatible mode for running migrations each in their own transaction. This behaviour 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 all of from scratch.

import { MigrationInterface, QueryRunner } from "typeorm";

export class CreateUuidExtension1544044606093 implements MigrationInterface {
  public up(queryRunner: QueryRunner): Promise<any> {
    return queryRunner.query(`CREATE EXTENSION IF NOT EXISTS "uuid-ossp";`);
  }

  public down(queryRunner: QueryRunner): Promise<any> {
    return queryRunner.query('DROP EXTENSION "uuid-ossp"');
  }
}
import { MigrationInterface, QueryRunner, Table } from "typeorm";

export class CreateUsers1543965157399 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");
  }
}

The error is function uuid_generate_v4() does not exist because extensions cannot be referenced from the same transaction which they are created.

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).

TODO:

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

@garbles garbles force-pushed the master branch 2 times, most recently from fa16207 to 72c549d Compare December 7, 2018 06:09
@@ -272,14 +272,13 @@ export class Connection {
* Runs all pending migrations.
* Can be used only after connection to the database is established.
*/
async runMigrations(options?: { transaction?: boolean }): Promise<Migration[]> {
async runMigrations(options?: { transactionMode?: "all" | "none" | "each" }): Promise<Migration[]> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify it and just call it transaction

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer transactionMode or a variant thereof in this case. When a parameter is called transaction I would expect to pass an object of type Transaction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay...

try {
await PromiseUtils.runInSequence(pendingMigrations, async migration => {
await queryRunner.startTransaction();
return migration.instance!.up(queryRunner)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this block is repeated 3 times, probably we should extract it into separate method

@pleerock
Copy link
Member

Thanks for the contribution!

Seems tests are failing, can you please fix them?
Also can you please address comments I have left?

@mordof
Copy link

mordof commented Jan 24, 2019

@garbles is this something you're going to finish up? this is a feature I'd like to see added; if you're not interested in finishing this i'd be glad to take care of the feedback given to get this ready.

@garbles
Copy link
Author

garbles commented Jan 28, 2019

@mordof hey! Sorry I haven't had time to finish it. Please feel free to take on this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants