drizzle-orm icon indicating copy to clipboard operation
drizzle-orm copied to clipboard

[FEATURE]: AsyncLocalStorage for transactions

Open hyzyla opened this issue 1 year ago • 1 comments

Describe want to want

The idea is to use AsyncLocalStorage for automatically detecting transaction context, without using transaction object. Example how it should look like:

await db.transaction(async () => {
  await db.insert(users).values(newUser);
  await db.transaction(async () => {
    await db.update(users).set({ name: 'Mr. Dan' }).where(eq(users.name, 'Dan'));
    await db.delete(users).where(eq(users.name, 'Dan'));
  });
});

It can make the library easier to use, especially in case if database call is buried in call chain:

Instead of using this:

async createUser(dto, transaction?: ...) {
    await (transaction || db).insert(users).values(newUser);
}

User can write just this:

async createUser(dto) {
    await db.insert(users).values(newUser); // automatically detect that current request is in transaction
}

hyzyla avatar May 03 '23 08:05 hyzyla

This sounds like a code readability and debugging nightmare. Current version provides much clearer picture of what is actually happening.

kibertoad avatar May 21 '23 21:05 kibertoad

I did same thing in my kysely-orm library https://github.com/seeden/kysely-orm#transactions

seeden avatar Jul 15 '23 11:07 seeden

Here is working example which I started to use

import { AsyncLocalStorage } from 'node:async_hooks';
import { drizzle } from 'drizzle-orm/postgres-js';
import { AnyPgTable } from 'drizzle-orm/pg-core';

type AfterCommitCallback = () => Promise<any>;

export default function database(db: ReturnType<typeof drizzle>) {
  const asyncLocalStorage = new AsyncLocalStorage<TransactionState>();

  type DatabaseType = typeof db;
  type Transaction = Parameters<Parameters<DatabaseType['transaction']>[0]>[0];

  type TransactionState = {
    transaction: Transaction;
    committed: boolean;
    afterCommit: AfterCommitCallback[];
  };

  type TransactionResponse = { 
    transaction: Transaction;
    afterCommit: (callback: AfterCommitCallback) => void;
  };
  
  type TransactionCallback<Type> = (trx: TransactionResponse) => Promise<Type>;

  function selectAll() {
    return db.select();
  }

  function select<SelectedFields extends Parameters<DatabaseType['select']>[0]>(selectedFields: SelectedFields) {
    return db.select(selectedFields);
  }

  function dbSelect(): ReturnType<typeof selectAll>;
  function dbSelect<SelectedFields extends Parameters<DatabaseType['select']>[0]>(selectedFields: SelectedFields): ReturnType<typeof select>;
  function dbSelect<SelectedFields extends Parameters<DatabaseType['select']>[0]>(selectedFields?: SelectedFields) {
    return selectedFields ? select(selectedFields) : selectAll();
  }

  return {
    delete: <Table extends AnyPgTable>(table: Table) => db.delete(table),
    insert: <Table extends AnyPgTable>(table: Table) => db.insert(table),
    update: <Table extends AnyPgTable>(table: Table) => db.update(table),
    select: dbSelect,
    transaction: async <Type>(callback: TransactionCallback<Type>) => {
      const transactionState = asyncLocalStorage.getStore();
      if (transactionState && !transactionState.committed) {
        console.log('you are already in transaction. using current transaction instance');
        return callback({
          transaction: transactionState.transaction,
          afterCommit(callback: AfterCommitCallback) {
            transactionState.afterCommit.push(callback);
          },
        });
      }

      const afterCommit: AfterCommitCallback[]  = [];

      const result = await db.transaction((transaction) => {
        const newTransactionState: TransactionState = {
          transaction,
          committed: false,
          afterCommit,
        };

        return new Promise<Type>((resolve, reject) => {
          asyncLocalStorage.run(newTransactionState, async () => {
            try {
              const result = await callback({
                transaction,
                afterCommit(callback: AfterCommitCallback) {
                  newTransactionState.afterCommit.push(callback);
                },
              });
              resolve(result);
            } catch (error) {
              reject(error);
            } finally {
              newTransactionState.committed = true;
            }
          });
        });
      })

      for (const afterCommitCallback of afterCommit) {
        await afterCommitCallback();
      }

      return result;
    },
  };
}

seeden avatar Jul 21 '23 09:07 seeden

mikro-orm also supports this feature. it would be great to see it here.

rhyek avatar Dec 15 '23 04:12 rhyek

Here is working example which I started to use

Please help me understand the feature request here. You would like to replace the db returned by the drizzle function like this:

const drizzleDb = drizzle(...);
export const db = database(drizzleDb)

And then use the exported db everywhere in the codebase how? Is the idea to call db.transaction deep in a stack of calls and only using one transaction?

Angelelz avatar Dec 15 '23 17:12 Angelelz

Is the idea to call db.transaction deep in a stack of calls and only using one transaction

Yes, all database operations down the stack will be part of that transaction without directly using the transaction object ‘tx’. Just wrap a batch of functions in a transaction, and everything else will automatically become part of that transaction.

Pros:

  • There’s no need to write separate functions for operations within and outside of transactions, as illustrated in the createUser example.
  • It reduces the likelihood of errors where write operations are mistakenly executed directly without a transaction (tx).

Cons:

  • all pitfalls of AsyncLocalStorage

hyzyla avatar Dec 15 '23 19:12 hyzyla

You would like to replace the db returned by the drizzle function like this:

I’m not certain if this is the best solution for the problem, but I would like to have an option to configure that behavior globally

hyzyla avatar Dec 15 '23 19:12 hyzyla

Here is working example which I started to use

Please help me understand the feature request here. You would like to replace the db returned by the drizzle function like this:

const drizzleDb = drizzle(...);
export const db = database(drizzleDb)

And then use the exported db everywhere in the codebase how? Is the idea to call db.transaction deep in a stack of calls and only using one transaction?

i would not make it necessary to use a separate function to wrap the db object. invocations on db should just use async local storage to check wether there is an encompassing transaction (created with db.transaction(...)) and use it thus making it transparent to the user.

rhyek avatar Dec 15 '23 19:12 rhyek

What is the advantage of using a node API over using a state variable saved inside the class returned by the drizzle function? Also, other that the convenience, is there an advantage of using the same transaction for all your operations as opposed to nested transactions that would probably have savepoints? My other observation is that this feature would also introduce hidden behavior, I would honestly prefer another API, like db.uniqueTransaction. This will both prevent any breaking changes and make it have a clear intent.

Angelelz avatar Dec 15 '23 19:12 Angelelz

you're right about introducing hidden behavior. it could be opt-in.

the benefit, in my experience, is being able to:

  1. invoke re-usable methods exported from other modules that run their own queries + business logic, etc, without having to pass around a tx param everywhere.
  2. these queries might be reads or writes, but imagine reads (selects) where you would want them to transparently run inside the current tx if there is one due to transaction isolation level.
  3. when working with a team of developers on a complex app, i as a lead would feel more confident that my juniors are respecting transaction contexts by not relying on them remembering to pass that tx param around.

rhyek avatar Dec 15 '23 19:12 rhyek

by opt-in i mean a global setting, not changing the api or introducing a new method.

rhyek avatar Dec 15 '23 19:12 rhyek

The following is my personal opinion, not endorsed by anyone, especially the drizzle team:

  1. Relying on a node API would immediately complicate the possible support for deno, hermes, and any other possible environments/runtimes with no/little support for AsyncLocalStorage.
  2. I would personally prefer passing tx/db around than having a global magic state that morphs a db call to make it run either in a transaction or a regular connection depending on the state of my application. This will basically serialize all my database transactions in one single connection. I'm team dependency injection over global state when possible.
  3. The tx object is a class that extends the db returned by the drizzle function, the methods are all the same except for tx.rollback() which you don't really need because all it does is throw an error. That means that in OP's example, you can make the db object required, and it would work with a tx.
async createUser(dto, db: typeof db)

I obviously don't know the structure/requirements of your projects but I feel like making the db globally available and let it have an internal state that can be used in the same way as proposed with AsyncLocalStorage is a better pattern. Also, introducing a new method will provide the flexibility to make this pattern opt-in per request.

Angelelz avatar Dec 15 '23 20:12 Angelelz

Well, tbh, I am not sure of a use-case for making database queries outside of a transaction in the current async call stack (single http request) if one exists. If I saw that in a code base I would assume it's a bug and not intended, yet it is so easy to make the mistake. If this is true, it would make sense for the ALS proposal to be the default behavior, or at the very least it should be an option.

@Angelelz if it were opt-in, your projects would not need updating.

I am not sure of drizzle's goals in terms of supported runtimes, but I know that Bun supports ALS.

rhyek avatar Dec 15 '23 21:12 rhyek

additionally, the preference for passing the db/tx object around seems at odds even with drizzle's own documentation. for example at https://orm.drizzle.team/docs/sql-schema-declaration/#:~:text=Database%20and%20table%20explicit%20entity%20types you see the following:

const db = drizzle(...);
const result: User[] = await db.select().from(users);
export async function insertUser(user: NewUser): Promise<User[]> {
  return db.insert(users).values(user).returning();
}

here you can clearly see the intent is to create a global db object that is imported and used directly in your functions without receiving it as a parameter. so having to declare db parameters everywhere and passing the db or tx around seems to not be aligned with the spirit of the library's design.

rhyek avatar Dec 15 '23 21:12 rhyek

Well, tbh, I am not sure of a use-case for making database queries outside of a transaction in the current async call stack (single http request) if one exists.

In the context of a backend server, which seems to be the context you are referring to, I can think of many examples where it wouldn't only be undesirable but actually counterproductive to have all the db queries serialized in a transaction:

  • SSR server getting all the data for an admin dashboard (possibly from many different unrelated tables).
  • Writing to tables unrelated to the operation being performed in the request, like logs or analytics. This is one of the reasons pools exists. In other contexts, there are many many other examples.

I am not sure of drizzle's goals in terms of supported runtimes, but I know that Bun supports ALS.

Neither am I, but looking at the codebase and the history, drizzle can easily run in the browser, an electron app, a react-native app, on the edge, in a worker thread, etc. etc. And there is an actual issue opened by one of the team members to support deno.

the preference for passing the db/tx object around seems at odds even with drizzle's own documentation.

I did say that my opinion was my own. But I think it's quite the opposite, drizzle is a zero dependency library because of the fact that it uses dependency injection: You have to give it the driver.

In any case, if this feature is important, gains traction, and the team agrees, it could be easily implemented by also using dependency injection:

const db = drizzle(myDriver, { schema, logger, transactionStorage })

Where transactionStorage can be a wrapper (possibly provided by drizzle) of AsyncLocalStorage.

Angelelz avatar Dec 15 '23 23:12 Angelelz

Relying on a node API would immediately complicate the possible support for deno, hermes, and any other possible environments/runtimes with no/little support for AsyncLocalStorage.

FYI deno supports AsyncLocalStorage, I use it. also note: https://tc39.es/proposal-async-context/

guy-borderless avatar Feb 06 '24 12:02 guy-borderless