.count use same flow as findMany #2163
Conversation
📝 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 with CodeRabbit:
SupportNeed help? Create a ticket on our support 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 examples rg -A 10 -B 5 "\.count\(" --type ts
| } | ||
| return this.modelClient.count(args); | ||
| }); | ||
| return createDeferredPromise<unknown[]>(() => this.doFind(args, 'count', () => [])); |
There was a problem hiding this comment.
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.
| 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); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/runtime/src/enhancements/node/policy/handler.ts at line 1657, the
count method incorrectly returns a Promise of an unknown array and uses an empty
array as fallback, which is incompatible with expected count results. To fix
this, change the return type to a number or appropriate count object, replace
the fallback with a function returning 0, and avoid using doFind for count
operations by directly invoking Prisma's count API or a method designed for
counting.
|
The two issues I found so far:
|
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.