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

[BUG]: Transactions rollback doesn't work #1723

Open
WebTarantul opened this issue Dec 28, 2023 · 26 comments
Open

[BUG]: Transactions rollback doesn't work #1723

WebTarantul opened this issue Dec 28, 2023 · 26 comments
Labels
bug Something isn't working priority Will be worked on next qb/transactions

Comments

@WebTarantul
Copy link

What version of drizzle-orm are you using?

0.29.2

What version of drizzle-kit are you using?

0.20.9

Describe the Bug

I wrote a pretty simple transaction but when the error appears rollback doesn't work.
I have 3 steps and when the error happens on the second step, the first change is still in DB.

Expected behavior

I expect that if the error happens at some point in the transaction the entire transaction rolls back. But it doesn't.

Even when I try to run rolling back manually I get an error: [DrizzleError: Rollback] { name: 'DrizzleError', cause: undefined }

Environment & setup

It is running on Vercel and the project is on Next.js 14.

import {sql} from '@vercel/postgres'
import {drizzle} from 'drizzle-orm/vercel-postgres'

export const db = drizzle(sql, {schema})
const updatedUser = await db.transaction(async (trx) => {
  const [updatedUser] = await trx
    .update(UsersTable)
    .set({
      name: input.name,
      email: input.email,
    })
    .where(eq(UsersTable.id, user.id))
    .returning();

  

    // ERROR happens here
  await trx.insert(KeyTable).values({
    id: 'some-id',
    userId: user.id,
    hashedPassword: 'hashed-password',
  });
  await trx.delete(KeyTable).where(eq(KeyTable.id, 'oldKeyId'));

  return updatedUser
});

@WebTarantul WebTarantul added the bug Something isn't working label Dec 28, 2023
@Angelelz
Copy link
Collaborator

Is there any chance you can put together a reproduction repo? I would like to look into this.

@pantoninho
Copy link

pantoninho commented Jan 17, 2024

I am also experiencing this issue with aws-data-api.

[email protected]
[email protected]

In the following example, step 1. create application commits to the database even if step 2. associate marketing entities fails.

import { RDSDataClient } from '@aws-sdk/client-rds-data';
import { drizzle } from 'drizzle-orm/aws-data-api/pg';

const rdsClient = new RDSDataClient({});

export const db = drizzle(rdsClient, {
    database: process.env.database,
    resourceArn: process.env.resourceArn,
    secretArn: process.env.secretArn
});

async function getOrCreateApplication({ userId, dealId }) {
    return await db.transaction(async tx => {
        let [application] = await tx
            .select()
            .from(applications)
            .where(eq(applications.userId, userId));

        // if already exists, return it
        if (application) {
            return application;
        }

        // 1. create application
        application = await tx
            .insert(applications)
            .values({ userId })
            .returning();

        // 2. associate marketing entities
        await tx
            .insert(hs_deals)
            .values({ dealId, applicationId: application.id });

        return application;
    });
}

@breesetposh
Copy link

breesetposh commented Feb 15, 2024

bumping this, this is a pretty important functionality to not be working or have a workaround. It's super simple to reproduce with the happy path of a rollback:

await db
    .transaction(async (transaction) => {
      try {
        const user = await transaction.insert(users).values({ key }).returning();
        throw new Error("uh oh");
      } catch (error) {
        try {
          transaction.rollback();
        } catch (error) {
          console.log({ error });
        }
      }
    });

Will output:

{
  error: TransactionRollbackError [DrizzleError]: Rollback
      at NodePgTransaction.rollback (/path/to/base/dir/node_modules/src/pg-core/session.ts:99:9)
      at /path/to/base/dir/src/scripts/transactions.ts:32:23
      at Generator.next (<anonymous>)
      at fulfilled (/path/to/base/dir/src/scripts/transactions.ts:5:58)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
}

@breesetposh
Copy link

turns out I was doing this wrong, works fine for postgres, you just have to be sure to throw within transaction and can't suppress it because it depends on catching the error in the underlying code in order to perform the rollback.

So this works and rolls back:

db.transaction(async (tx) => {
  const user = await tx.insert(users).values({...}).returning();
  tx.rollback(); // this throws an error, which causes drizzle (really the postgres client) to auto-rollback)
});

but this won't:

db.transaction(async (tx) => {
  try {
    const user = await tx.insert(users).values({...}).returning();
    tx.rollback();
  } catch (error) {
     //.. suppressing it, but needs to throw to rollback automatically
   }
});

Probably obvious, but just in case that trips anyone else up.

@gabrielgrover
Copy link

gabrielgrover commented Feb 16, 2024

This is not working on sqlite.

Here is the test that fails

import { test, expect } from "vitest";
import { db } from "./database";
import * as Schema from "./schema";
import {eq} from "drizzle-orm";
import { faker } from "@faker-js/faker";

test("Should rollback", async () => {
  const name = faker.person.firstName();
  await expect(() =>
    addUser(name)
  ).rejects.toThrow();
	
  const users = await db
    .select()
    .from(Schema.users)
    .where(eq(Schema.users.name, name));

  expect(users).toHaveLength(0);
});

async function addUser(name: string) {
  await db.transaction(async tx => {
      await tx.insert(Schema.users).values({
        name
      });

      throw new Error("BOOM");
    });
  }
}

@gabrielgrover
Copy link

I believe it is this line https://github.com/drizzle-team/drizzle-orm/blob/main/drizzle-orm/src/better-sqlite3/session.ts#L74

If T is a promise, and there is a throw / rejection in the call to transaction it wouldn't be caught because of the lack of an await

@gabrielgrover
Copy link

Seems like it a deeper issue. At the moment BetterSQLiteSession only implements the sync version of the SQLiteSession abstract class. I am down to take this on, but would like input from maintainers. @Angelelz do you know or know someone who knows why async SQLiteSession has not been implemented for BetterSQLiteSession? Is there a known issue with it?

@Angelelz
Copy link
Collaborator

Bettersqlite3 is only synchronous

@gabrielgrover
Copy link

gabrielgrover commented Feb 17, 2024

@Angelelz If that is the case then shouldn't the types for something like queries be synchronous as well? Since they are async it naturally follows that a developer would treat them as such and use an async call back when calling transaction.

The docs for better-sqlite on the drizzle site show awaits being used. https://orm.drizzle.team/docs/get-started-sqlite#better-sqlite3. This is confusing no?

@gabrielgrover
Copy link

gabrielgrover commented Feb 18, 2024

If anyone comes across this my work around was to just handle transaction logic. This is just repurposing the bulk of the logic in BetterSQLIteTransaction.

type Transaction = {
  readonly db: Database; // This type is the return type of `drizzle`
  readonly nestedIndex: number;
  readonly savepointName: string;
  transaction: <T>(tx: (t: Transaction) => Promise<T>) => Promise<T>;
  rollback: () => void;
};

function createTransaction(
  db: Database,
  nestedIndex?: number,
  savepointName?: string,
): Transaction {
  const idx = nestedIndex ?? 0;
  const name = savepointName ?? "sp0";

  return {
    db,
    nestedIndex: idx,
    savepointName: name,
    transaction: async (tx) => {
      db.run(sql.raw(`savepoint ${name}`));
      const t = createTransaction(db, idx + 1, `sp${idx + 1}`);

      try {
        const txResult = await tx(t);
        db.run(sql.raw(`release savepoint ${name}`));
        return txResult;
      } catch (e) {
        db.run(sql.raw(`rollback to savepoint ${name}`));
        throw e;
      }
    },
    rollback: () => {
      throw new Error("Rollback called.  Reverting transaction");
    },
  };
}

const databaseTransaction = createTransaction(db);  // pass your initialized drizzle instance

await databaseTransaction.transaction(async ({ db, transaction, rollback }) => {
  await db.insert(users).values({...});
  await transactions(async ({ db, transaction, rollback }) => { 
    // nested transaction
  });

  rollback(); // call roll back to throw and revert transaction
});

@marcotas
Copy link

I'm getting this error with the latest version of drizzle (0.30.4). This is my setup with bun (v1.0.35) and vitest (all the latest and recent updated):

This is my simple test case with vitest

describe('Contacts API', () => {
    it('should work', async () => {
        await db.transaction(async (tx) => {
            const contact = await new ContactFactory(tx).create();
            const results = await tx.select().from(contacts);
            expect(contact.id).toBeDefined();
            expect(results).toHaveLength(1);

            tx.rollback(); // this doesn't return a promise so await here is irrelevant and also doesn't make different

            console.log('Never reach this line!');
        });
    });
});

And this is the result I'm getting:

bun test v1.0.35 (940448d6)

api/contacts/index.test.ts:
1 | import { entityKind } from "./entity.js";
2 | class DrizzleError extends Error {
3 |   static [entityKind] = "DrizzleError";
4 |   constructor({ message, cause }) {
5 |     super(message);
            ^
DrizzleError: Rollback
      at new DrizzleError (/Users/marco/projects/testings/project-x/node_modules/drizzle-orm/errors.js:5:9)
      at new TransactionRollbackError (/Users/marco/projects/testings/project-x/node_modules/drizzle-orm/errors.js:13:5)
      at rollback (/Users/marco/projects/testings/project-x/node_modules/drizzle-orm/pg-core/session.js:55:15)
      at /Users/marco/projects/testings/project-x/api/contacts/index.test.ts:16:7
✗ Contacts API > should work [122.68ms]

 0 pass
 1 fail
 2 expect() calls
Ran 1 tests across 1 files. [288.00ms]

@97albertus
Copy link

This seems to be the normal behaviour, tx.rollback() is just a shorthand for throwing the DrizzleError: Rollback inside a transaction, which should then automagically abort. The error thrown inside the tx then propagates to db.transaction() caller, which is why your test fails.

Thats just my intuition though, as the transaction API in drizzle isn't really documented aside from basically mentioning that it exists.

@nemanjaonex2
Copy link

I get:

 tx.rollback is not a function

@acerbisgianluca
Copy link

Did you find a workaround? Throwing an error inside the transaction function doesn't seem to trigger the auto-rollback. tx.rollback() doesn't work either.

PS: using bun sqlite driver

@IDovgalyuk
Copy link

My workaround, for better-sqlite driver with better-sqlite-pool lib.

import { BetterSQLite3Database, drizzle } from "drizzle-orm/better-sqlite3";
import { Pool } from "better-sqlite-pool";
import * as schema from "./schema";

export type DB = BetterSQLite3Database<typeof schema> & {
  release: () => void;
};

export class ConnectionPool {
  constructor(
    dbPath: string,
  ) {
    this.pool = new Pool(dbPath, {
      onConnectionCreated: sqlite => {
        // enabling Write-Ahead Logging for better performance
        sqlite.pragma("journal_mode = WAL");

        // enforce foreign key constraints
        sqlite.pragma("foreign_keys = ON");
      },
    });
  }

  private readonly pool: Pool;

  public acquire = async (): Promise<DB> => {
    const connection = await this.pool.acquire();
    const db = drizzle(connection, {
      schema,
    });

    Object.assign(db, { release: () => connection.release() });
    return db as DB;
  };
}

export const runInReadMode =
  <TResult, TArgs = void>(
    connectionPool: ConnectionPool,
    fn: (db: DB, args: TArgs) => Promise<TResult>
  ) =>
  async (args: TArgs) => {
    const db = await connectionPool.acquire();
    try {
      return await fn(db, args);
    } finally {
      db.release();
    }
  };

export const runInTransaction = <TResult, TArgs = void>(
  connectionPool: ConnectionPool,
  fn: (db: DB, args: TArgs) => Promise<TResult>
) => {
  return async (args: TArgs) => {
    const db = await connectionPool.acquire();
    db.run(sql.raw(`BEGIN TRANSACTION`));
    try {
      const res = await fn(db, args);
      db.run(sql.raw(`COMMIT TRANSACTION`));
      return res;
    } catch (e) {
      db.run(sql.raw(`ROLLBACK TRANSACTION`));
      throw e;
    } finally {
      db.release();
    }
  };
};

const main = async () => {
  const connectionPool = new ConnectionPool(dbPath);
  const readResult = await runInReadMode(connectionPool, readSomeData);
  const writeResult = await runInTransaction(connectionPool, writeSomeData);
}

const readSomeData = async (db: DB) => {
  const users = await db.select().from(usersTable);
  const products = await db.select().from(productsTable);
  return { products, users };
}

const writeSomeData = async (db: DB) => {
  const user = await db.insert(usersTable).values({ name: "John" }).returning();
  const product = await db.insert(productsTable).values({ name: "Apple" }).returning();

  if (!product || !user) {
    throw new Error("Some error");
  }

  return { user, product };
}

I am using this approach for node app that has incoming requests, requests either read-only or read-write. So they are wrapped in corresponding wrappers.
For each such request I am acquiring connection from the pool. Most of the time only a single connection is used in my scenarios.

@eaoliver
Copy link

eaoliver commented Jun 5, 2024

I'm getting this error with the latest version of drizzle (0.30.4). This is my setup with bun (v1.0.35) and vitest (all the latest and recent updated):

This is my simple test case with vitest

describe('Contacts API', () => {
    it('should work', async () => {
        await db.transaction(async (tx) => {
            const contact = await new ContactFactory(tx).create();
            const results = await tx.select().from(contacts);
            expect(contact.id).toBeDefined();
            expect(results).toHaveLength(1);

            tx.rollback(); // this doesn't return a promise so await here is irrelevant and also doesn't make different

            console.log('Never reach this line!');
        });
    });
});

And this is the result I'm getting:

bun test v1.0.35 (940448d6)

api/contacts/index.test.ts:
1 | import { entityKind } from "./entity.js";
2 | class DrizzleError extends Error {
3 |   static [entityKind] = "DrizzleError";
4 |   constructor({ message, cause }) {
5 |     super(message);
            ^
DrizzleError: Rollback
      at new DrizzleError (/Users/marco/projects/testings/project-x/node_modules/drizzle-orm/errors.js:5:9)
      at new TransactionRollbackError (/Users/marco/projects/testings/project-x/node_modules/drizzle-orm/errors.js:13:5)
      at rollback (/Users/marco/projects/testings/project-x/node_modules/drizzle-orm/pg-core/session.js:55:15)
      at /Users/marco/projects/testings/project-x/api/contacts/index.test.ts:16:7
✗ Contacts API > should work [122.68ms]

 0 pass
 1 fail
 2 expect() calls
Ran 1 tests across 1 files. [288.00ms]

Same with me using 0.30.10 and postgres.

@ThienDuc3112
Copy link

Does anyone know a workaround for this in expo-sqlite? I have been trying to call tx.rollback() in a transaction but instead db.transaction(...) didn't roll back and just throw the error back at me.

@busybox11
Copy link

busybox11 commented Jul 16, 2024

Still having the issue. No idea what caused it, it happened pretty much overnight and I started getting this error as soon as I upgraded my iOS simulator from 17.4 to 17.5. No code change.

Also I don't manually call the rollback function. I am getting a "Failed to run the query" on a CREATE TABLE statement from useMigrations, but from what I see in the logger, the rollback is causing the error? I'm not quite sure what's happening, my apologies if I made a mistake.

@jackkrone
Copy link

Running into the same issue today on 0.32.0.

@agcty
Copy link

agcty commented Aug 3, 2024

Also running into this issue, thought i was going crazy while trying to debug

@michalss
Copy link

michalss commented Aug 8, 2024

Same here driving me crazy.. :(

		const affectedRows = await db.transaction(async (tx) => {
			// Perform the update within a transaction
			const [updateResult] = await tx.execute(
				sql`UPDATE ${projectsData} SET text_translated = REPLACE(text_translated, ${oldText}, ${newText})
            WHERE text_translated LIKE ${'%' + oldText + '%'}`
			);

			// Get the number of affected rows
			const affectedRows = updateResult.affectedRows - 1;

			// Rollback the transaction to simulate without actual changes
			if (dryRun) {
				tx.rollback();
			}

			return affectedRows;
		});

always drops to catch with error Rollback. Basically returning catch block. This is stupid... Bug is open for almost the year and still not fixed... :(

@KpjComp
Copy link

KpjComp commented Sep 18, 2024

Came across this when using Drizzle with sqlite. The problem is if you use an async callback the result is not awaited, so the transaction is free to commit, and the exception is raised outside the transaction.

I posted an issue on better-sqlite3 to make the callback work with both async & sync callbacks.

WiseLibs/better-sqlite3#1262

So this is not really a Drizzle bug, but a better-sqlite3 bug.

@mouhannad-sh
Copy link

Not sure if this is helpful but I've been using a raw query to avoid this weird error

await tx.execute(sql`ROLLBACK`);

@bajcmartinez
Copy link

Not sure if this is helpful but I've been using a raw query to avoid this weird error

await tx.execute(sql`ROLLBACK`);

How do you do that? I wrapped my transaction code in try...catch, then on the catch I run

trx.run(sql`ROLLBACK`);

but I get an error from that line: Failed to run the query 'ROLLBACK'.

This really sucks, as it's pretty basic for transactions to work as intended, sad there's no solution in sight

@Firephoenix25
Copy link

for me was that I left "db" instead of convert all query with "tx"..... such a stupid error...😂

Btw I'm using neon serverless driver because the http driver of don't support transactions yet

@KoenCa
Copy link

KoenCa commented Nov 28, 2024

Does anyone know a workaround for this in expo-sqlite? I have been trying to call tx.rollback() in a transaction but instead db.transaction(...) didn't roll back and just throw the error back at me.

I'm also using expo-sqlite and I tried several things to make it work, but doesn't seem to work... From what I could gather the main cause is when you're using async await.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority Will be worked on next qb/transactions
Projects
None yet
Development

No branches or pull requests