romeerez / orchid-orm

Orchid ORM
https://orchid-orm.netlify.app/
MIT License
488 stars 14 forks source link

afterCreateCommit crash and unexpected rollback when running inside transaction #355

Closed IlyaSemenov closed 1 month ago

IlyaSemenov commented 1 month ago

Investigating existing options for #354, I tried to use afterCreateCommit query hook inside a transaction, and it crashed:

await db.$transaction(async () => {
  await db.user
    .afterCreateCommit(["id", "name"], async ([user]) => {
      console.log(`Actually created ${user.name}`)
    })
    .create({ name: "Alice" })
})
(1.1ms) BEGIN
(1.8ms) INSERT INTO "user"("name") VALUES ($1) RETURNING * ['Alice']
(3.8ms) COMMIT
(3.9ms) ROLLBACK
/Users/is/work/ss/node_modules/.pnpm/pqb@0.38.4/node_modules/pqb/src/queryMethods/transaction.ts:121
            for (const fn of afterCommit[i + 2] as AfterCommitHook[]) {
                             ^

TypeError: afterCommit[(i + 2)] is not iterable
    at Db.transaction (/Users/is/work/ss/node_modules/.pnpm/pqb@0.38.4/node_modules/pqb/src/queryMethods/transaction.ts:121:30)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at <anonymous> (/Users/is/work/ss/2.ts:30:1)

Also note that it runs extra unexpected ROLLBACK.

Changing the callback to be non-async gives a different error:

(0.7ms) BEGIN
(1.0ms) INSERT INTO "user"("name") VALUES ($1) RETURNING * ['Alice']
(2.5ms) COMMIT
(2.6ms) ROLLBACK
/Users/is/work/ss/2.ts:32
    .afterCreateCommit(["id", "name"], ([user]) => {
                                       ^

TypeError: object is not iterable (cannot read property Symbol(Symbol.iterator))
    at <anonymous> (/Users/is/work/ss/2.ts:32:40)
    at Db.transaction (/Users/is/work/ss/node_modules/.pnpm/pqb@0.38.4/node_modules/pqb/src/queryMethods/transaction.ts:122:29)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at <anonymous> (/Users/is/work/ss/2.ts:30:1)
Full reproduction ```ts import process from "node:process" import { createBaseTable, orchidORM, testTransaction } from "orchid-orm" const BaseTable = createBaseTable() class UserTable extends BaseTable { readonly table = "user" columns = this.setColumns(t => ({ id: t.serial().primaryKey(), name: t.varchar(), })) } const db = orchidORM( { databaseURL: process.env.DATABASE_URL, log: true }, { user: UserTable, }, ) await db.$query` drop table if exists "user"; create table "user" ( id serial not null primary key, name varchar not null );` await db.$transaction(async () => { await db.user .afterCreateCommit(["id", "name"], async ([user]) => { console.log(`Actually created ${user.name}`) }) .create({ name: "Alice" }) }) await db.$query`drop table "user"` await db.$close() ```
romeerez commented 1 month ago

Going to fix it in the first order.

romeerez commented 1 month ago

I published a fix, it happened because I cut a corner when was writing a test for it, and now improved that. Now the transaction logic keeps track of surrounding test transactions, and afterCommit hooks will trigger after the highest non-test transaction commits. So now it should be possible to test email sending flows based on afterCommit hooks, while still relying on test transactions that never actually commit.