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

Sequential transactions run out of order when using client extensions and middleware #18276

Closed
Tracked by #19416
LucianBuzzo opened this issue Mar 10, 2023 · 17 comments · Fixed by #19538
Closed
Tracked by #19416
Assignees
Labels
bug/2-confirmed Bug has been reproduced and confirmed. domain/client Issue in the "Client" domain: Prisma Client, Prisma Studio etc. kind/bug A reported bug. topic: clientExtensions topic: middleware topic: transaction topic: $transaction([...]) Sequential Prisma Client operations
Milestone

Comments

@LucianBuzzo
Copy link

Bug description

When using a batch transaction inside a client query extension, if you are also using a middleware that uses await then the batch transaction will run out of order.

How to reproduce

The bug can be easily demonstrated using the example given in the docs for batch transactions in queries, including SET ROLE statements.

Schema:

datasource db {
  provider = "postgresql"
  url      = env("DATABASE_URL")
}

generator client {
  provider        = "prisma-client-js"
  previewFeatures = ["clientExtensions"]
}

model User {
  id        Int      @id @default(autoincrement())
  createdAt DateTime @default(now())
  email     String   @unique
  name      String?
  role      Role     @default(USER)
}

enum Role {
  USER
  ADMIN
}

Minimal reproduction:

import { PrismaClient } from "@prisma/client";

const prisma = new PrismaClient({
	log: ["query"],
});

prisma.$use(async (params, next) => {
	await "test";

	return next(params);
});

const run = async () => {
	try {
		await prisma.$executeRawUnsafe("CREATE ROLE test_user");
	} catch (e) {}

	const xprisma = prisma.$extends({
		query: {
			user: {
				async findFirst({ args, query }) {
					const [_, result] = await prisma.$transaction([
						prisma.$queryRawUnsafe("SET ROLE test_user"),
						query(args),
						prisma.$queryRawUnsafe("SET ROLE none"),
					]);
					return result;
				},
			},
		},
	});
	const result = await xprisma.user.findFirst();

	console.log(result);
};

run();

The SQL is then executed out of order:

prisma:query CREATE ROLE test_user
prisma:query BEGIN
prisma:query SELECT "public"."User"."id", "public"."User"."createdAt", "public"."User"."email", "public"."User"."name", "public"."User"."role" FROM "public"."User" WHERE 1=1 LIMIT $1 OFFSET $2
prisma:query SET ROLE test_user
prisma:query SET ROLE none
prisma:query COMMIT

If you comment out the await 'test' line, then the transaction will run sequentially.

Expected behavior

I expect batch transactions to run sequentially when combining query extension and middleware with await statement.

Prisma information

// Add your schema.prisma
// Add your code using Prisma Client

Environment & setup

  • OS: macOS
  • Database: PostgreSQL 11
  • Node.js version: 16.8.0

Prisma Version

prisma                  : 4.9.0
@prisma/client          : 4.9.0
Current platform        : darwin-arm64
Query Engine (Node-API) : libquery-engine ceb5c99003b99c9ee2c1d2e618e359c14aef2ea5 (at node_modules/@prisma/engines/libquery_engine-darwin-arm64.dylib.node)
Migration Engine        : migration-engine-cli ceb5c99003b99c9ee2c1d2e618e359c14aef2ea5 (at node_modules/@prisma/engines/migration-engine-darwin-arm64)
Introspection Engine    : introspection-core ceb5c99003b99c9ee2c1d2e618e359c14aef2ea5 (at node_modules/@prisma/engines/introspection-engine-darwin-arm64)
Format Binary           : prisma-fmt ceb5c99003b99c9ee2c1d2e618e359c14aef2ea5 (at node_modules/@prisma/engines/prisma-fmt-darwin-arm64)
Format Wasm             : @prisma/prisma-fmt-wasm 4.9.0-42.ceb5c99003b99c9ee2c1d2e618e359c14aef2ea5
Default Engines Hash    : ceb5c99003b99c9ee2c1d2e618e359c14aef2ea5
Studio                  : 0.479.0
Preview Features        : clientExtensions
@LucianBuzzo LucianBuzzo added the kind/bug A reported bug. label Mar 10, 2023
@LucianBuzzo
Copy link
Author

LucianBuzzo commented Mar 10, 2023

I think it's also worth adding that using an interactive transaction doesn't work here either, though for a different reason. With the following code, the query(args) statement runs outside of the transaction entirely:

const xprisma = prisma.$extends({
	query: {
		user: {
			async findFirst({ args, query }) {
				const result = await prisma.$transaction(async (tx) => {
					await tx.$queryRawUnsafe("SET ROLE test_user");
					const res = await query(args);
					await tx.$queryRawUnsafe("SET ROLE none");
					return res;
				});

				return result;
			},
		},
	},
});

@LucianBuzzo
Copy link
Author

You can force the query to run in a transaction by "reconstructing" the prisma query like so:

const result = await prisma.$transaction(async (tx) => {
    await tx.$queryRawUnsafe("SET ROLE test_user");
    const res = await (tx as any)[model][operation](args);
    await tx.$queryRawUnsafe("SET ROLE none");
    return res;
})

However, this will result in middleware running again for the reconstructed query, which is highly undesirable.
You can bypass this by using a secondary PrismaClient, which doesn't have any middleware attached to it, but then we run into issues with connection pool management, as we can't (AFAICT) share a connection pool between multiple clients.

LucianBuzzo added a commit to cerebruminc/yates that referenced this issue Mar 13, 2023
This fixes a bug where RLS policies would not be used correctly when
Yates is combined with async middleware.
See prisma/prisma#18276

Signed-off-by: Lucian Buzzo <lucian.buzzo@gmail.com>
LucianBuzzo added a commit to cerebruminc/yates that referenced this issue Mar 13, 2023
This fixes a bug where RLS policies would not be used correctly when
Yates is combined with async middleware.
See prisma/prisma#18276

Signed-off-by: Lucian Buzzo <lucian.buzzo@gmail.com>
@janpio janpio added bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. topic: transaction topic: middleware domain/client Issue in the "Client" domain: Prisma Client, Prisma Studio etc. topic: clientExtensions labels Mar 13, 2023
@LucianBuzzo
Copy link
Author

LucianBuzzo commented Mar 22, 2023

We eventually solved this by using private methods of Prisma to bypass the middleware, it's not ideal, but it works for now:

// The interactive transaction ensures that the queries run in the correct order
const queryResults = await prisma.$transaction(async (tx) => {
    await tx.$queryRawUnsafe(`SET ROLE test_user`);
    // We need to manually reconstruct the query, and attached the "secret" transaction ID.
    // This ensures that the query will run inside the transaction AND that middlewares will not be re-applied
    // https://github.com/prisma/prisma/blob/4.11.0/packages/client/src/runtime/getPrismaClient.ts#L1013
    const txId = (tx as any)[Symbol.for("prisma.client.transaction.id")];
    // See https://github.com/prisma/prisma/blob/4.11.0/packages/client/src/runtime/getPrismaClient.ts#L860
    const __internalParams = (params as any).__internalParams;
    const result = await prisma._executeRequest({
        ...__internalParams,
        // The transaction data isn't included in __internalParams
        transaction: {
            kind: "itx",
            id: txId,
        },
    });
    // Switch role back to admin user
    await tx.$queryRawUnsafe("SET ROLE none");

    return result;
});

@andyjy
Copy link
Contributor

andyjy commented Apr 3, 2023

Also hit this; big thanks and +100 helpfulness points to @LucianBuzzo for the clear + detailed explanation.

Successfully using your workaround in the last comment for now (tweaked to use the _executeRequest() call from your linked repo in place of the reference to __internalParams currently included in the comment above):

...
// See https://github.com/prisma/prisma/blob/4.11.0/packages/client/src/runtime/getPrismaClient.ts#L860
const result = await prisma._executeRequest({
  args,
  clientMethod: `${model.toLowerCase()}.${operation}`,
  jsModelName: model.toLowerCase(),
  action: operation,
  model,
  transaction: {
    kind: "itx",
    id: txId,
  },
});
...

@andyjy
Copy link
Contributor

andyjy commented Apr 4, 2023

...neither forms of the workaround work with the prisma data proxy 😢

@janpio
Copy link
Contributor

janpio commented Apr 5, 2023

Can you open an issue for that please @andyjy? In a separate thread with all the information we might be able to debug and hopefully fix this.

@andyjy
Copy link
Contributor

andyjy commented Apr 5, 2023

@janpio sorry, to clarify I was referring to how the workaround figured out by @LucianBuzzo using the private _executeRequest method of Prisma to bypass the middleware doesn't work with the data proxy. This is unsurprising since after brief inspection of the prisma codebase I saw the data proxy requires additional internal parameters which are not set by the workaround.

I did explore briefly whether something additional could be done to make it work but with the internals of how Prisma processes requests there were too many unknowns.

Glad to file a separate issue or anything else that may be helpful - but given this is messing with the private / undocumented internals of Prisma I don't really consider it an issue requiring fixing per-se.

It would be wonderful to have the original issue posted here by @LucianBuzzo resolved so that transactions within client extensions don't run out of order when combined with middleware that uses await. Once that is resolved none of the other discussion above should be relevant. Thanks!

@andyjy
Copy link
Contributor

andyjy commented Apr 6, 2023

I realised it's possible to "convert" middleware to a client extension by wrapping it:

function middlewareAsClientExtension(middleware: Prisma.Middleware) {
  return Prisma.defineExtension(
  (prisma) =>
    prisma.$extends({
      name: "middleware-as-client-extension",
      query: {
        $allModels: {
          async $allOperations({ args, model, operation, query }) {
            // wrap original query in middleware
            return await middleware(
              {
                action: operation as Prisma.PrismaAction, // TODO: not exactly equivalent
                args,
                model,
                runInTransaction: false, // TODO: fix when prisma exposes this to client extensions
                dataPath: [], // TODO: don't know what this is
              },
              (params) => query(params.args)
            );
          },
        },
      },
    }) as GenericPrismaClientExtended
  );
}

// before:
// prisma.$use(myMiddleware);
// const prismaClient = prisma;

// after:
const prismaClient = prisma.$extends(middlewareAsClientExtension(myMiddleware));

In my testing this provides a temporary workaround for the original issue in a "cleaner" way - i.e. without accessing Prisma internals - and also works with the Prisma Data Proxy.

@andyjy
Copy link
Contributor

andyjy commented Apr 28, 2023

Update: I don't think this is middleware-specific - I hit this same issue (transactions running out of order) when extending a client that had previously been extended with a no-op $allModels.$allOperations:

  return Prisma.defineExtension({
    query: {
      $allModels: {
        $allOperations({ args, model, operation, query }) {
          console.log(`prisma $allOperations: ${model}.${operation}`);
          return query(args);
        },
      },
    },
  });

@andyjy
Copy link
Contributor

andyjy commented May 2, 2023

Partial-diagnosis of what I suspect causes the original issue:

Batch transactions are implemented via _transactionWithArray(), which in turn calls waitForBatch() with the array of requests:

return waitForBatch(requests)

waitForBatch() then iterates over each PrismaPromise value calling .then() - and in particular does so without awaiting each call to return before proceeding to call the next:

for (let i = 0; i < promises.length; i++) {
promises[i].then(
(result) => {
successfulResults[i] = result
settleOnePromise()
},

My assumption here is that under typical usage, this causes each request to be started in the correct order, as expected.

However - when execution of a particular request is drawn out (e.g. via middleware that calls await) to delay the eventual request to the database server to a subsequent tick of the event loop(?), the requests will reach the database in a different order vs. expected.

@LucianBuzzo
Copy link
Author

@andyjy Yep that definitely looks like the culprit!

@jadamduff
Copy link

@andyjy I've hit this issue and it doesn't seem to be specific to interactive transactions. I have a non-interactive transaction that is also running out of order when one of the queries is extended.

@andyjy
Copy link
Contributor

andyjy commented May 4, 2023

@jadamduff yes! The issue affects batch transactions, not interactive transactions.

(The references to interactive transactions above are attempts to find a workaround; most clearly illustrated in @LucianBuzzo's first comment under his original post. If I've written anything above that introduces confusion regarding interactive transactions please quote it and I'll try edit to clarify. Thanks!)

@janpio janpio added the topic: $transaction([...]) Sequential Prisma Client operations label May 26, 2023
@SevInf SevInf self-assigned this May 31, 2023
@SevInf SevInf added bug/2-confirmed Bug has been reproduced and confirmed. and removed bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. labels May 31, 2023
SevInf pushed a commit that referenced this issue May 31, 2023
Root cause of the problem: data loader batches requests in the order
they come in, which in case of `$transaction` call happens to be the
order in which they are specified in the array. However, depending on
middlewares/extensions (and possibly other factors), it is possible that
they will be deleviered to the data loader in a different order.
Fortunately, we already have an index of each request in a batch as a
part of request, so we can just sort the requests before sending them to
the engine.

Fix #18276
SevInf pushed a commit that referenced this issue May 31, 2023
Root cause of the problem: data loader batches requests in the order
they come in, which in case of `$transaction` call happens to be the
order in which they are specified in the array. However, depending on
middlewares/extensions (and possibly other factors), it is possible that
they will be deleviered to the data loader in a different order.
Fortunately, we already have an index of each request in a batch as a
part of request, so we can just sort the requests before sending them to
the engine.

Fix #18276
@Jolg42 Jolg42 added this to the 4.16.0 milestone May 31, 2023
SevInf pushed a commit that referenced this issue Jun 5, 2023
Root cause of the problem: data loader batches requests in the order
they come in, which in case of `$transaction` call happens to be the
order in which they are specified in the array. However, depending on
middlewares/extensions (and possibly other factors), it is possible that
they will be deleviered to the data loader in a different order.
Fortunately, we already have an index of each request in a batch as a
part of request, so we can just sort the requests before sending them to
the engine.

Fix #18276
SevInf pushed a commit that referenced this issue Jun 5, 2023
* fix(client): Fix batch order for middleware/query combo

Root cause of the problem: data loader batches requests in the order
they come in, which in case of `$transaction` call happens to be the
order in which they are specified in the array. However, depending on
middlewares/extensions (and possibly other factors), it is possible that
they will be deleviered to the data loader in a different order.
Fortunately, we already have an index of each request in a batch as a
part of request, so we can just sort the requests before sending them to
the engine.

Fix #18276

* Fix process.nextTick polyifill for edge client

Promise callback is actually called before next callback in the
microtask queue. When locks are used, this causes batch to be flushed
before all requests waiting on lock have finished.
@LucianBuzzo
Copy link
Author

Whoop! Awesome, thanks @SevInf

@SevInf
Copy link
Contributor

SevInf commented Jun 5, 2023

Fix will be published with the next Prisma release.
If you you want to check out dev snapshot, you can use version 4.16.0-dev.21, however, we do not recommend you use dev snapshots in production.

@adamnyberg
Copy link

Did this issue pop up again? 🤔

I'm using an extended prisma client like below and my list of queries does not always run in order. Or is it that I'm using a transaction inside of my extended prisma client that is the problem?

prisma.$extends({
    query: {
      $allModels: {
        async $allOperations({ model, args, query }) {
          const organizationId = getClaimedOrgId();
          if (!organizationId) {
            throw new Error('Organization ID not found in context');
          }

          const [, result] = await prisma.$transaction([
            prisma.$executeRaw`SELECT set_config('app.orgId', ${`${organizationId}`}, TRUE)`,
            query(args),
          ]);
          return result;
        },
      },
    },
  })

@janpio
Copy link
Contributor

janpio commented Oct 4, 2023

Please open a new issue @adamnyberg, then we can properly react to this. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/2-confirmed Bug has been reproduced and confirmed. domain/client Issue in the "Client" domain: Prisma Client, Prisma Studio etc. kind/bug A reported bug. topic: clientExtensions topic: middleware topic: transaction topic: $transaction([...]) Sequential Prisma Client operations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants