drizzle-team / drizzle-orm

Headless TypeScript ORM with a head. Runs on Node, Bun and Deno. Lives on the Edge and yes, it's a JavaScript ORM too 😅
https://orm.drizzle.team
Apache License 2.0
23.59k stars 579 forks source link

[FEATURE]: AsyncLocalStorage for transactions #543

Open hyzyla opened 1 year ago

hyzyla commented 1 year ago

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
}
kibertoad commented 1 year ago

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

seeden commented 1 year ago

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

seeden commented 1 year ago

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;
    },
  };
}
rhyek commented 9 months ago

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

Angelelz commented 9 months ago

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?

hyzyla commented 9 months ago

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:

Cons:

hyzyla commented 9 months ago

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

rhyek commented 9 months ago

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.

Angelelz commented 9 months ago

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.

rhyek commented 9 months ago

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 commented 9 months ago

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

Angelelz commented 9 months ago

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.

rhyek commented 9 months ago

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 commented 9 months ago

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.

Angelelz commented 9 months ago

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:

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.

guy-borderless commented 8 months ago

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/