migrate() blocks forever if passed db.$qb.adapter

[IlyaSemenov]

In my schema/database management helper functions, I prefer to pass around instances of Db not OrchidORM<any> to reduce coupling.

However, when I call migrate() passing db.adapter, it blocks forever and never resolves. When I re-construct the ORM interface as { $qb: db, $adapter: db.adapter }, it works fine. Please check the reproduction below. Note that the bug only repeats inside a true (non-test) transaction.

At the very least, this needs to be fixed (either runtime or type-wise). Currently, db.adapter is an AdapterBase, which is what migrate() is supposed to accept.

Broader question is, why do we even need this type?

orchid-orm/packages/rake-db/src/utils.ts

Lines 3 to 8 in e7b1232

	export interface OrmParam {
	$qb: Query;
	$adapter: AdapterBase;
	}
	export type DbParam = OrmParam | AdapterBase;

I propose to get rid of this 'pseudo ORM' interface object unless there is a clear reason to have it. I believe it's not used anywhere outside rake-db?

---

Reproduction (orchid-orm@1.62.3):

import type { Db } from "orchid-orm"
import { createBaseTable } from "orchid-orm"
import { makeRakeDbConfig, migrate } from "orchid-orm/migrations"
import { orchidORM } from "orchid-orm/postgres-js"

export const BaseTable = createBaseTable()

const db = orchidORM({ log: true }, {})

const rakeDbConfig = makeRakeDbConfig({
  baseTable: BaseTable,
  basePath: import.meta.url,
  migrations: {
    "0001_initial": async () => [],
  },
  migrationsTable: "rake_migration",
})


// I don't need full ORM in these functions so prefer to pass Db, not OrchidORM<any>
async function myMigrate(db: Db) {
  await db.query`CREATE TABLE IF NOT EXISTS ${db.ref(rakeDbConfig.migrationsTable)} (version text NOT NULL, name text NOT NULL)`
  console.log("calling migrate()")
  // Below, migrate() will block forever.
  // Replacing db.adapter with { $qb: db, $adapter: db.adapter } fixes blocking.
  await migrate(db.adapter, { ...rakeDbConfig }).then(
    () => console.log("migrate() completed"),
    () => console.log("migrate() failed"),
  )
}

async function main() {
  // The issue only surfaces when using $transaction (and not testTransaction!).
  await db.$transaction(async () => {
    await myMigrate(db.$qb).then(
      () => console.log("myMigrate() completed"),
      () => console.log("myMigrate() failed"),
    )
  })
}

main()

Result:

% dropdb is;createdb is; bun 1.ts
(34.1ms) BEGIN
(5.8ms) CREATE TABLE IF NOT EXISTS "rake_migration" (version text NOT NULL, name text NOT NULL)
calling migrate()
--- blocks forever until ^C

with db.adapter replaced with { $qb: db, $adapter: db.adapter }, it works correctly:

% dropdb is;createdb is; bun 1.ts
(31.5ms) BEGIN
(6.0ms) CREATE TABLE IF NOT EXISTS "rake_migration" (version text NOT NULL, name text NOT NULL)
calling migrate()
Migrating database undefined

Migrated file:///Users/is/test/0001_initial

migrate() completed
myMigrate() completed
(10.1ms) COMMIT

[romeerez]

I'll try to fix it next week.

Broader question is, why do we even need this type?

The reason is quite simple, so that migrate can accept orchidORM's instance, that's it.

I believe it's not used anywhere outside rake-db?

Yes.

which is what migrate() is supposed to accept.

But it already does accept it:

export type DbParam = OrmParam | AdapterBase;

You can either pass orchidORM's instance, or db.$adapter.

[IlyaSemenov]

Then why the interface is { $qb: Query; $adapter: AdapterBase } and not simply { $adapter: AdapterBase }?

[romeerez]

Here is how it's used: https://github.com/romeerez/orchid-orm/blob/main/packages/rake-db/src/utils.ts

getNonTransactionAdapter is only used when creating and dropping a database, not during migrations, and it only needs an adapter.

getMaybeTransactionAdapter and ensureTransaction are used when running migrations, and here it tries to get an active transaction from $qb.internal.transactionStorage.

The idea is that if you pass orchidORM instance, it will pick up the active transaction if present, and the $adapter if no active transaction.

[romeerez]

It's getting out of hands with the internals being exposed.

The reason is:
db.query CREATE TABLE does pick the current transaction, but
migrate(db.adapter does not pick it and tries to open a new transaction. Not sure why exactly it hangs, but trying to open a transaction here is not right.

The idea behind

export type DbParam = OrmParam | AdapterBase; 

is that user has a choice:

• either use the db instance from orchidORM, and you have it here await db.$transaction, you could pass it to migrate.
• or you're manually controlling transactions, but then you should also take care of passing the proper adapter.

First option: using a db from `orchidORM`

async function myMigrate() {
  await db.$query`CREATE TABLE IF NOT EXISTS ${db.$qb.ref(
    rakeDbConfig.migrationsTable,
  )} (version text NOT NULL, name text NOT NULL)`;
  console.log('calling migrate()');
  await migrate(db, { ...rakeDbConfig }).then(
    () => console.log('migrate() completed'),
    () => console.log('migrate() failed'),
  );
}

async function main() {
  await db.$transaction(async () => {
    await myMigrate().then(
      () => console.log('myMigrate() completed'),
      () => console.log('myMigrate() failed'),
    );
  });
  await db.$close();
}

Second option: managing a transaction adapter

This is by design that users shouldn't manage it manually, it should "just work" by itself.

So it's not as trivial to manage it.

There is no nice way for the 2nd option.

With your intention in mind:

I prefer to pass around instances of Db not OrchidORM to reduce coupling.

the straightforward solution would be to additionally support the Db (Query) type in migrate.

But I'm considering a more classy solution:

• here the bug happened because we rightfully expect db.adapter to refer to the current transaction, but it's counter-intuitively not the case, so ideally it should be eliminated to prevent confusion in the future.
• instead, let there be a $getAdapter() method that will try to get a transaction's object first, and a regular db client object otherwise when not in transaction. Let it be prefixed with $ for both ORM and the db.$qb to stay polymorphic.

[romeerez]

I published a change:

now db.adapter is renamed to db.adapterNotInTransaction so it's obvious that it's something you shouldn't use in this case,
and added db.$getAdapter to get a proper adapter that will utilize the current transaction - use it instead.

Now you can pass ORM instance to the migrate function, as well as db.$qb instance, and migrate will call the $getAdapter on its own.

@IlyaSemenov I noticed there is already a logic in migrate that creates the migrations table automatically if it doesn't exist, why do you need to create it manually? Isn't it created automatically?