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

Batching of findUnique() queries fails for certain parameters #23313

Closed
eliasnorrby opened this issue Feb 28, 2024 · 6 comments
Closed

Batching of findUnique() queries fails for certain parameters #23313

eliasnorrby opened this issue Feb 28, 2024 · 6 comments
Assignees
Labels
bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. domain/client Issue in the "Client" domain: Prisma Client, Prisma Studio etc. kind/bug A reported bug. tech/typescript Issue for tech TypeScript. topic: batching topic: findUnique()

Comments

@eliasnorrby
Copy link

eliasnorrby commented Feb 28, 2024

Bug description

The docs on query optimization state that

The Prisma Client dataloader automatically batches findUnique queries that ✔ occur in the same tick and ✔ have the same where and include parameters.

I find this is not always true, especially when where includes either OR or AND.

Here's an example project leveraging batching of findUnique queries in a resolved graphql field:

  async posts(@Root() user: User, @Context() ctx): Promise<Post[]> {
    return this.prismaService.user
      .findUnique({
        where: {
          id: user.id,
        },
      })
      .posts()
  }

Changing it to

  async posts(@Root() user: User, @Context() ctx): Promise<Post[]> {
    return this.prismaService.user
      .findUnique({
        where: {
          id: user.id,
+         AND: [],
        },
      })
      .posts()
  }

results in batching not working.

Background – the real world use case

For context, we're using @casl/prisma to control user permissions. This means we transform a set of access rules into where args for the relevant model, so we can e.g. list all posts a user can see by implementing allPosts something like:

async allPosts(@Ability() ability: PrismaAbility) {
  const where = accessibleBy(ability).Posts
  return await this.prismaService.posts.findMany({
    where
  })
}

We're also leveraging Prisma's data loaders, which means we'll be running things like:

    const whereAccessible = accessibleBy(ability).Posts 
    return this.prismaService.user
      .findUnique({
        where: {
          id: user.id,
          AND: whereAccessible
        }
      })
      .posts()

and this is where batching breaks down. Despite the parameters being identical, batching fails.

I've set up a reproduction repository based on the graphql nestjs example.

Video demonstration

Screen.Recording.2024-02-29.at.00.08.46.mov

How to reproduce

  1. Go to eliasnorrby/prisma-dataloader-issue-reproduction
  2. Follow the reproduction instructions

Expected behavior

No response

Prisma information

See reproduction repository.

Environment & setup

  • OS: macOS
  • Database: PostgreSQL, SQLite
  • Node.js version: v18.16.1

Prisma Version

prisma                  : 5.10.2
@prisma/client          : 5.10.2
Computed binaryTarget   : darwin-arm64
Operating System        : darwin
Architecture            : arm64
Node.js                 : v18.16.1
Query Engine (Node-API) : libquery-engine 5a9203d0590c951969e85a7d07215503f4672eb9 (at node_modules/.pnpm/@[email protected]/node_modules/@prisma/engines/libquery_engine-darwin-arm64.dylib.node)
Schema Engine           : schema-engine-cli 5a9203d0590c951969e85a7d07215503f4672eb9 (at node_modules/.pnpm/@[email protected]/node_modules/@prisma/engines/schema-engine-darwin-arm64)
Schema Wasm             : @prisma/prisma-schema-wasm 5.10.0-34.5a9203d0590c951969e85a7d07215503f4672eb9
Default Engines Hash    : 5a9203d0590c951969e85a7d07215503f4672eb9
Studio                  : 0.499.0
@eliasnorrby eliasnorrby added the kind/bug A reported bug. label Feb 28, 2024
@eliasnorrby
Copy link
Author

I can add that I've done some troubleshooting of the prisma client already. I found the DataLoader implementation and suspected that batchBy would compute different hashes for my findUnique calls, but that was not the case. I also added some debug logging to the batchLoader to verify the queries were being submitted in a proper batch, and they were. Still, the engine did not execute a single query, but one for each request.

@millsp millsp added bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. tech/typescript Issue for tech TypeScript. topic: batching domain/client Issue in the "Client" domain: Prisma Client, Prisma Studio etc. labels Mar 1, 2024
@janpio janpio changed the title Batching of findUnique queries fails for certain parameters Batching of findUnique() queries fails for certain parameters Mar 1, 2024
@Druue
Copy link
Contributor

Druue commented May 3, 2024

Hey @eliasnorrby, boolean operators are not intended to work with batching. To provide further context, we have the following function:

fn invalid_compact_filter(op: &Operation, schema: &QuerySchema) -> bool

We use it to sort through queries and validate if they're using operations that we can't compact for batching; one of these filters is boolean operators, i.e. AND, OR, NOT.
So you seeing an increase in the number of queries being sent by just adding the following AND: [] to your where is not surprising as that caused it to be rendered invalid.

To ask for further context, are you specifically talking about the case of BOOLEAN_OPERATOR: [] and not also BOOLEAN_OPERATOR: [...]? Given that we do not render the boolean operator in the sql in such cases, that could be considered a bug

To otherwise support boolean operators, BOOLEAN OPERATOR: [...] in batching would be a feature request.
For further context as to what we consider an invalid operation for batching, you can see the following doc comment over the function I mentioned above:

/// Those filters are:
/// - non scalar filters (ie: relation filters, boolean operators...)
/// - any scalar filters that is not `EQUALS`
/// - nativetypes (citext)

@Druue
Copy link
Contributor

Druue commented May 3, 2024

Note: we don't seem to have this explicitly stated anywhere in our docs but it is here in our v4.6.0 release notes

This has now been updated

@eliasnorrby
Copy link
Author

Thanks for clarifying @Druue! Happy to see the documentation updated. I guess this should be closed as "working as intended" then?

@Druue
Copy link
Contributor

Druue commented May 6, 2024

Absolutely, glad we could help :)

I'll be closing this now ✨

Feel free to open up a new issue as a feature request if this is something you want to see us support!

@Druue Druue closed this as completed May 6, 2024
@janpio
Copy link
Contributor

janpio commented May 6, 2024

Do you want us to add some more batching, and see a way how that could work @eliasnorrby? If so, that is definitely worth a feature request. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. domain/client Issue in the "Client" domain: Prisma Client, Prisma Studio etc. kind/bug A reported bug. tech/typescript Issue for tech TypeScript. topic: batching topic: findUnique()
Projects
None yet
Development

No branches or pull requests

4 participants