Transaction issue while using umzug.up()

[abhishekS-mygstcafe]

I am facing an issue while using umzug.up() in my project.

Suppose I have 10 migration files. While executing the last migration, a query fails, which correctly causes the transaction to roll back. However, despite the rollback, the name of the migration file still gets stored in the database.

As a result, when I attempt to run the migration again, Umzug considers it already applied. I have to manually remove the migration record from the database to rerun it, which is not ideal.

After debugging, I found that the following function inside the Umzug storage implementation does not use the transaction I passed:

async logMigration({ name: migrationName }) { await this.syncModel(); await this.model.create({ [this.columnName]: migrationName, }); }

Since this function doesn't respect the transaction, the migration record is committed even if the actual migration fails and rolls back.

[pocketrocket]

@abhishekS-mygstcafe you probably forgot to throw the exception after rolling back? If the migration has a normal return, the framework is considering it as executed successfully - hence writes the filename name to be successfully executed

[zinefer]

I'm having a similar issue.

I have a situation where I have multiple identical schemas in a database. We loop over these schemas and apply each set of un-run migrations with an umzug up.

We've noticed some issues around de-sync when one of these schemas has an error but the rest do not. We would much prefer for an error in any of these schemas to cause a rollback for all the attempted migrations in that run.

We end up creating a single transaction and pass this down into all of our migrations and if any of the tenants have an issue, all the contents of the migration roll-back nicely.

The problem is there seems to be no way to get our transaction into the queries that are logging the migrations as successful.

[Jaigupta-1]

You can implement a custom storage and integrate it with Sequelize storage as shown in the following example:

`interface CustomStorage {
executed(): Promise<string[]>;
logMigration(params: { name: string }): Promise;
unlogMigration(params: { name: string }): Promise;
}

class TransactionalStorage implements CustomStorage {
constructor(
private sequelize: Sequelize,
private transaction: Transaction,
private tableName: string,
private schema: string
) {}

async executed() {
const [results] = await this.sequelize.query(
SELECT name FROM "${this.schema}"."${this.tableName}" ORDER BY name,
{ transaction: this.transaction }
);
return (results as any[]).map(r => r.name);
}

async logMigration({ name }: { name: string }) {
await this.sequelize.query(
INSERT INTO "${this.schema}"."${this.tableName}" (name) VALUES (:name),
{ replacements: { name }, transaction: this.transaction }
);
}

async unlogMigration({ name }: { name: string }) {
await this.sequelize.query(
DELETE FROM "${this.schema}"."${this.tableName}" WHERE name = :name,
{ replacements: { name }, transaction: this.transaction }
);
}
}
`

[zinefer]

Thanks! That's pretty much what we went with but here's a slightly more type-safe version:

class TransactionalSequelizeStorage extends SequelizeStorage {
  private transaction?: Transaction;

  constructor(
    options: SequelizeStorageConstructorOptions,
    transaction?: Transaction,
  ) {
    super(options);
    this.transaction = transaction;
  }

  async logMigration({ name: migrationName }: { name: string }): Promise<void> {
    await this.syncModel();

    // umzug is using lazy typing which omits the second argument to create()
    // This is a gnarly looking cast but should be rather safe.
    const castModel = this.model as unknown as ModelStatic<Model>;

    await castModel.create(
      {
        [this.columnName]: migrationName,
      },
      { transaction: this.transaction },
    );

    config.logger.info('Logged migration with transaction', { migrationName });
  }

  async unlogMigration({
    name: migrationName,
  }: {
    name: string;
  }): Promise<void> {
    await this.syncModel();
    await this.model.destroy({
      where: { [this.columnName]: migrationName },
      transaction: this.transaction,
    });
  }

  async executed(): Promise<string[]> {
    await this.syncModel();

    // umzug is using lazy typing which omits the second argument to create()
    // This is a gnarly looking cast but should be rather safe.
    const castModel = this.model as unknown as ModelStatic<Model>;

    const migrations = await castModel.findAll({
      order: [[this.columnName, 'ASC']],
      transaction: this.transaction,
    });

    return migrations.map((migration) => {
      const name = migration.get(this.columnName);
      if (typeof name !== 'string') {
        throw new TypeError(
          `Unexpected migration name type: expected string, got ${typeof name}`,
        );
      }

      return name;
    });
  }
}

I kinda think this should be provided by this project though.

If anyone is dealing with multiple schemas like I am, make sure you set the modelName on the storage or you'll have issues when Umzug pulls a model out of the cache with the incorrect schema.