Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork126
.count use same flow as findMany#2163
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coderabbitaibot commentedJun 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
📝 WalkthroughWalkthroughThe changes extend the Changes
Sequence Diagram(s)sequenceDiagram participant Caller participant PolicyProxyHandler participant PrismaClient Caller->>PolicyProxyHandler: count(args) PolicyProxyHandler->>PolicyProxyHandler: doFind('count', args, []) PolicyProxyHandler->>PrismaClient: count(modified args) PrismaClient-->>PolicyProxyHandler: result PolicyProxyHandler-->>Caller: result✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
SupportNeed help? Create a ticket on oursupport page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime/src/enhancements/node/policy/handler.ts(3 hunks)
🔇 Additional comments (2)
packages/runtime/src/enhancements/node/policy/handler.ts (2)
38-38:LGTM: Type extension aligns with unified find operations flow.The extension of
FindOperationsto include'count'correctly supports the PR objective of making count operations follow the same policy enforcement flow as other find operations.
147-149:Verify compatibility with Prisma's count API.Setting
_args.select = truefor count operations may not be compatible with Prisma's count method, which typically doesn't use select clauses like find operations. This could potentially cause runtime errors or unexpected behavior.Please verify that this approach works correctly with Prisma's count API:
#!/bin/bash# Search for how count operations are typically structured in Prisma documentation or examplesrg -A 10 -B 5"\.count\(" --type ts
| } | ||
| returnthis.modelClient.count(args); | ||
| }); | ||
| returncreateDeferredPromise<unknown[]>(()=>this.doFind(args,'count',()=>[])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Critical: Incorrect return type and fallback for count operation.
The count method implementation has several issues:
- Return type mismatch: Count operations should return a number or count object, not
Promise<unknown[]> - Incorrect fallback: The fallback
() => []returns an empty array, but count operations should return0when no results match - API incompatibility: Using
doFindfor count operations may not work correctly with Prisma's count API
Consider this approach instead:
-count(args: any) {- return createDeferredPromise<unknown[]>(() => this.doFind(args, 'count', () => []));-}+count(args: any): Promise<number> {+ return createDeferredPromise<number>(async () => {+ const _args = this.policyUtils.safeClone(args);+ if (!this.policyUtils.injectForRead(this.prisma, this.model, _args)) {+ if (this.shouldLogQuery) {+ this.logger.info(`[policy] \`count\` ${this.model}: unconditionally denied`);+ }+ return 0;+ }++ if (this.shouldLogQuery) {+ this.logger.info(`[policy] \`count\` ${this.model}:\n${formatObject(_args)}`);+ }++ return this.modelClient.count(_args);+ });+}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| returncreateDeferredPromise<unknown[]>(()=>this.doFind(args,'count',()=>[])); | |
| count(args: any):Promise<number>{ | |
| returncreateDeferredPromise<number>(async()=>{ | |
| const_args=this.policyUtils.safeClone(args); | |
| if(!this.policyUtils.injectForRead(this.prisma,this.model,_args)){ | |
| if(this.shouldLogQuery){ | |
| this.logger.info(`[policy] \`count\`${this.model}: unconditionally denied`); | |
| } | |
| return0; | |
| } | |
| if(this.shouldLogQuery){ | |
| this.logger.info(`[policy] \`count\`${this.model}:\n${formatObject(_args)}`); | |
| } | |
| returnthis.modelClient.count(_args); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/runtime/src/enhancements/node/policy/handler.ts at line 1657, thecount method incorrectly returns a Promise of an unknown array and uses an emptyarray as fallback, which is incompatible with expected count results. To fixthis, change the return type to a number or appropriate count object, replacethe fallback with a function returning 0, and avoid using doFind for countoperations by directly invoking Prisma's count API or a method designed forcounting.PodkopovP commentedJun 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The two issues I found so far:
|
JoeRoddy commentedJul 8, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Relevant discord discussion from a similar issue. https://discord.com/channels/1035538056146595961/1338578707086049372 I was never able to create a simple proof of concept replication. It was only happening in our large real world project with highly complex rules across tables, including using |
The current .count implementation (and probably other aggregate functions) create a different .where clause to findMany - this means that doing a .count and .where with the same inputs, could (after zenstacks RLS implementation) cause be looking at different results - i.e. you might end up with 4 results from .findMany, but .count says there should be 10 results.