kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
10.43k stars 266 forks source link

Multiple migrations are run in a single transaction #1154

Open shoooe opened 4 days ago

shoooe commented 4 days ago

If you have a migration 0001 with this:

export async function up(db: Kysely<unknown>): Promise<void> {
  await db.schema.createType("some_enum").asEnum(["value_1", "value_2"]).execute();
}

and a migration 0002 with this:

export async function up(db: Kysely<unknown>): Promise<void> {
  await sql`ALTER TYPE some_enum ADD VALUE 'value_3';`.execute(
    db
  );
}

and then some migration 0003 that uses some_enum with value value_3 then:

error: unsafe use of new value "value_3" of enum type some_enum

From what I understand from the code this is caused by migrateToLatest running all pending migrations in a single transaction.

Shouldn't each migration be independent and run in its own transaction? IME this is the behaviour that most other migration systems adopt (e.g. Django migrations).

Despite this ^ I'm loving this library more and more every day, so thank you again for this magical tool. ❤️

koskimas commented 4 days ago

Having individual transaction for each migration would certainly make other things easier too. For example we could easily implement disabling transactions for individual migrations. There has been a feature request open for that forever.

However, there was a reason why it was implemented the way it is. I just can't remember it right now.

Changing the behaviour could also be a major breaking change and I don't know if we can do it without a massive deprecation period.

You could already migrate up one migration at a time to get individual transactions.

shoooe commented 4 days ago

Oh true. For reference to others this is what I went with:

  const migrations = await migrator.getMigrations();
  const pendingMigrations = migrations.filter((m) => isUndefined(m.executedAt));

  if (isEmpty(pendingMigrations)) {
    console.info(`✅ Migrations are up to date!`);
  }

  for (const migration of pendingMigrations) {
    const { error, results } = await migrator.migrateTo(migration.name);

    results?.map((it) => {
      if (it.status === "Success") {
        console.info(
          `✅ Migration "${it.migrationName}" was executed successfully`
        );
      } else if (it.status === "Error") {
        console.error(`❌ Failed to execute migration "${it.migrationName}"`);
      }
    });

    if (error) {
      console.error("❌ Failed to migrate");
      console.error(error);
      process.exit(1);
    }
  }
koskimas commented 4 days ago

Let's leave this open. It might make sense to have individual transactions.

igalklebanov commented 4 days ago

Maybe we could ship both and allow enabling this? no breaking change..

koskimas commented 3 days ago

Maybe. But enabling/disabling transaction for a single migration when we run a batch of migrations in one transaction would be horrible to implement. Basically we'd only allow that when the "transaction per migration" setting is enabled.