-
Notifications
You must be signed in to change notification settings - Fork 10.2k
fix: disallow undefined where clause #21062
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (05/01/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add platform team as reviewer" took an action on this PR • (08/11/25)1 reviewer was added to this PR based on Keith Williams's automation. |
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.
mrge found 1 issue across 3 files. View it in mrge.io
$allModels: { | ||
async deleteMany({ args, query }) { | ||
checkUndefinedInValue(args.where); | ||
validateWhereClause(args.where); | ||
return query(args); | ||
}, | ||
async updateMany({ args, query }) { | ||
checkUndefinedInValue(args.where); | ||
validateWhereClause(args.where); | ||
return query(args); | ||
}, | ||
async findMany({ args, query }) { | ||
validateWhereClause(args.where); | ||
return query(args); |
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.
Currently, we have this guard for these operations. Do we need to add more operations here?
it("validateWhereClause should throw exception when the where object is undefined", async () => { | ||
const where = undefined; | ||
|
||
expect(() => validateWhereClause(where)).toThrowError('The "where" clause cannot be undefined.'); | ||
}); | ||
|
||
it("validateWhereClause should throw exception when the where object is {}", async () => { | ||
const where = {}; | ||
|
||
expect(() => validateWhereClause(where)).toThrowError('The "where" clause cannot be an empty object {}.'); | ||
}); | ||
|
||
it("validateWhereClause should throw exception when the where object is []", async () => { | ||
const where = []; | ||
|
||
expect(() => validateWhereClause(where)).toThrowError('The "where" clause cannot be an empty array [].'); | ||
}); | ||
|
||
it("validateWhereClause should throw exception when the 'in' field of where object is []", async () => { | ||
const where = { | ||
id: { | ||
in: [], | ||
}, | ||
}; | ||
|
||
expect(() => validateWhereClause(where)).toThrowError( | ||
'The "in" value for the field "id" cannot be an empty array [].' | ||
); | ||
}); | ||
}); |
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.
Added new test
E2E results are ready! |
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.
All E2E tests are failing
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.
mrge found 2 issues across 4 files. Review them in mrge.io
This PR is being marked as stale due to inactivity. |
This PR is being marked as stale due to inactivity. |
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis change replaces the “disallow undefined delete/update many” logic with a generalized “disallow undefined where” validation. It introduces validateWhereClause and a renamed extension disallowUndefinedWhereExtension that validates where for deleteMany, updateMany, and findMany, including checks for undefined, empty object, and empty array. Index wiring is updated to use the new extension. The old test file is removed, and a new test suite is added covering undefined and null scenarios, empty where cases, and error messages. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 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: 2
🔭 Outside diff range comments (2)
packages/prisma/extensions/disallow-undefined-where-clause.ts (2)
19-38
: Deep validation misses nested undefineds (OR/AND/NOT), only checks top-level fields and top-level 'in'Cases like { OR: [{ id: undefined }] } or nested relation filters can slip through. If the intent is to “disallow undefined fields anywhere in where,” you need recursive validation over objects and arrays.
Sketch:
function hasUndefinedDeep(value: unknown): boolean { if (value === undefined) return true; if (value === null) return false; if (Array.isArray(value)) return value.some(hasUndefinedDeep); if (typeof value === "object") { return Object.values(value as Record<string, unknown>).some(hasUndefinedDeep); } return false; } // Then inside validateWhereClause after top-level checks: if (typeof where === "object" || Array.isArray(where)) { if (hasUndefinedDeep(where)) { throw new Error('The "where" clause contains undefined values.'); } }Optionally, keep the specific "in" error message if you want a more descriptive error for that path, but a deep scan will broadly prevent undefineds anywhere.
34-37
: Use safe hasOwnProperty check and guard non-objectswhereInput can be primitives (e.g., id: "x", teamId: null), so directly accessing hasOwnProperty may be surprising. Also, use Object.prototype.hasOwnProperty.call to avoid prototype overrides.
- if (whereInput.hasOwnProperty("in") && typeof whereInput.in === "undefined") { + if ( + whereInput && + typeof whereInput === "object" && + Object.prototype.hasOwnProperty.call(whereInput, "in") && + typeof (whereInput as any).in === "undefined" + ) { message = `The "in" value for the field "${key}" cannot be undefined.`; throw new Error(message); }
♻️ Duplicate comments (1)
packages/prisma/extensions/disallow-undefined-where-clause.ts (1)
3-16
: Strengthen typing and null handling in validateWhereClause; avoid any and clarify null-case
- Replace any with unknown and add precise type guards.
- Avoid Object.keys(where || {}) and explicitly check for null to produce clearer error messages.
-export const validateWhereClause = (where: any) => { +export const validateWhereClause = (where: unknown) => { // Check if where is undefined if (where === undefined) { throw new Error('The "where" clause cannot be undefined.'); } - // Check if where is an empty object - if (typeof where === "object" && !Array.isArray(where) && Object.keys(where || {}).length === 0) { + // Disallow null explicitly to avoid misreporting it as {} + if (where === null) { + throw new Error('The "where" clause cannot be null.'); + } + + // Check if where is an empty object + if (typeof where === "object" && !Array.isArray(where) && Object.keys(where as Record<string, unknown>).length === 0) { throw new Error('The "where" clause cannot be an empty object {}.'); } // Check if where is an empty array if (Array.isArray(where) && where.length === 0) { throw new Error('The "where" clause cannot be an empty array [].'); }Pros:
- Better type-safety and clearer error for null.
- Avoids reliance on falsy coalescing which can hide intent.
Cons:
- Introduces a new error message for null; add a matching test if you adopt this.
🧹 Nitpick comments (3)
packages/prisma/extensions/disallow-undefined-where-clause.test.ts (3)
39-39
: Typos: “contain” → “contains”Minor grammar cleanup in test names.
-it("validateWhereClause should not throw exception when the where object contain null values", async () => { +it("validateWhereClause should not throw exception when the where object contains null values", () => { -it("validateWhereClause should throw exception when the where object contain null values and 'in' field is undefined", async () => { +it("validateWhereClause should throw exception when the where object contains null values and 'in' field is undefined", () => {Also applies to: 48-48
36-37
: Make “does not throw” assertions explicitPrefer an explicit not.toThrow() to make intent clear and to ensure any unexpected throw is caught by the assertion rather than the test runner.
- validateWhereClause(where); + expect(() => validateWhereClause(where)).not.toThrow();Also applies to: 45-46
62-67
: Consider adding a test for where = null (if you adopt null-specific error)Your current code treats null like empty {} due to coalescing. If you adopt explicit null handling, add a test to assert the null message.
Example:
it('validateWhereClause should throw exception when the where object is null', () => { const where = null as unknown as object; expect(() => validateWhereClause(where)).toThrowError('The "where" clause cannot be null.'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/prisma/extensions/disallow-undefined-delete-update-many.test.ts
(0 hunks)packages/prisma/extensions/disallow-undefined-where-clause.test.ts
(1 hunks)packages/prisma/extensions/disallow-undefined-where-clause.ts
(2 hunks)packages/prisma/index.ts
(3 hunks)
💤 Files with no reviewable changes (1)
- packages/prisma/extensions/disallow-undefined-delete-update-many.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts
: For Prisma queries, only select data you need; never useinclude
, always useselect
Ensure thecredential.key
field is never returned from tRPC endpoints or APIs
Files:
packages/prisma/extensions/disallow-undefined-where-clause.test.ts
packages/prisma/extensions/disallow-undefined-where-clause.ts
packages/prisma/index.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()
in hot paths like loops
Files:
packages/prisma/extensions/disallow-undefined-where-clause.test.ts
packages/prisma/extensions/disallow-undefined-where-clause.ts
packages/prisma/index.ts
🧬 Code Graph Analysis (2)
packages/prisma/extensions/disallow-undefined-where-clause.test.ts (1)
packages/prisma/extensions/disallow-undefined-where-clause.ts (1)
validateWhereClause
(3-40)
packages/prisma/index.ts (1)
packages/prisma/extensions/disallow-undefined-where-clause.ts (1)
disallowUndefinedWhereExtension
(42-61)
🪛 GitHub Check: Tests / Integration
packages/prisma/extensions/disallow-undefined-where-clause.ts
[failure] 6-6: apps/api/v1/test/lib/bookings/_get.integration-test.ts > GET /api/bookings > Returns bookings for all users when accessed by system-wide admin
Error: The "where" clause cannot be undefined.
❯ validateWhereClause packages/prisma/extensions/disallow-undefined-where-clause.ts:6:11
❯ Array.findMany packages/prisma/extensions/disallow-undefined-where-clause.ts:55:11
❯ node_modules/@prisma/client/runtime/library.js:34:4829
❯ i node_modules/@prisma/client/runtime/library.js:123:987
❯ PrismaPromise.then node_modules/@prisma/client/runtime/library.js:123:1063
[failure] 6-6: packages/features/flags/features.repository.integration-test.ts > FeaturesRepository Integration Tests > checkIfFeatureIsEnabledGlobally > should return true when feature is enabled globally
Error: The "where" clause cannot be undefined.
❯ validateWhereClause packages/prisma/extensions/disallow-undefined-where-clause.ts:6:11
❯ Array.findMany packages/prisma/extensions/disallow-undefined-where-clause.ts:55:11
❯ node_modules/@prisma/client/runtime/library.js:34:4829
❯ i node_modules/@prisma/client/runtime/library.js:123:987
❯ PrismaPromise.then node_modules/@prisma/client/runtime/library.js:123:1063
[failure] 6-6: packages/features/flags/features.repository.integration-test.ts > FeaturesRepository Integration Tests > checkIfFeatureIsEnabledGlobally > should return false when feature is not enabled globally
Error: The "where" clause cannot be undefined.
❯ validateWhereClause packages/prisma/extensions/disallow-undefined-where-clause.ts:6:11
❯ Array.findMany packages/prisma/extensions/disallow-undefined-where-clause.ts:55:11
❯ node_modules/@prisma/client/runtime/library.js:34:4829
❯ i node_modules/@prisma/client/runtime/library.js:123:987
❯ PrismaPromise.then node_modules/@prisma/client/runtime/library.js:123:1063
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Merge reports / merge-reports
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
packages/prisma/extensions/disallow-undefined-where-clause.ts (1)
46-49
: Good: Strict guards on deleteMany/updateManyBlocking deleteMany/updateMany with undefined or empty where mitigates accidental mass writes/deletes. This aligns with the PR objective.
Also applies to: 51-53
packages/prisma/extensions/disallow-undefined-where-clause.test.ts (1)
6-6
: Remove unnecessary async from tests that don't await (repeat of past comment)These tests are synchronous; async is not needed and can mask unhandled sync exceptions.
[ suggest_nitpick ][ duplicate_comment ]
- it("...", async () => { + it("...", () => {Also applies to: 18-18, 28-28, 39-39, 48-48, 62-62, 68-68, 74-74
packages/prisma/index.ts (1)
50-50
: Heads-up: Applying disallowUndefinedWhere to findMany is currently breaking integrationsWiring looks fine, but the current extension rejects findMany when where is undefined, which is a legitimate and widely used pattern (fetch all). Please update the extension’s findMany handler to only validate when where is provided (not undefined), as suggested in the extension file comment.
No changes needed here once the extension is fixed.
Also applies to: 64-64
describe("Disallow undefined where", () => { | ||
it("validateWhereClause should throw exception when the 'in' field of where object is undefined", async () => { | ||
const where = { | ||
id: { | ||
in: undefined, | ||
}, | ||
}; | ||
|
||
expect(() => validateWhereClause(where)).toThrowError( | ||
'The "in" value for the field "id" cannot be undefined.' | ||
); | ||
}); | ||
|
||
it("validateWhereClause should throw exception when a field of where object is undefined", async () => { | ||
const where = { | ||
from: undefined, | ||
}; | ||
|
||
expect(() => validateWhereClause(where)).toThrowError( | ||
'The value for the field "from" cannot be undefined.' | ||
); | ||
}); | ||
|
||
it("validateWhereClause should not throw exception when the where object does not contain undefined values", async () => { | ||
const where = { | ||
id: { | ||
in: [1, 2, 3], | ||
}, | ||
name: "test name", | ||
}; | ||
|
||
validateWhereClause(where); | ||
}); | ||
|
||
it("validateWhereClause should not throw exception when the where object contain null values", async () => { | ||
const where = { | ||
teamId: null, | ||
parentId: null, | ||
}; | ||
|
||
validateWhereClause(where); | ||
}); | ||
|
||
it("validateWhereClause should throw exception when the where object contain null values and 'in' field is undefined", async () => { | ||
const where = { | ||
teamId: null, | ||
parentId: null, | ||
id: { | ||
in: undefined, | ||
}, | ||
}; | ||
|
||
expect(() => validateWhereClause(where)).toThrowError( | ||
'The "in" value for the field "id" cannot be undefined.' | ||
); | ||
}); | ||
|
||
it("validateWhereClause should throw exception when the where object is undefined", async () => { | ||
const where = undefined; | ||
|
||
expect(() => validateWhereClause(where)).toThrowError('The "where" clause cannot be undefined.'); | ||
}); | ||
|
||
it("validateWhereClause should throw exception when the where object is {}", async () => { | ||
const where = {}; | ||
|
||
expect(() => validateWhereClause(where)).toThrowError('The "where" clause cannot be an empty object {}.'); | ||
}); | ||
|
||
it("validateWhereClause should throw exception when the where object is []", async () => { | ||
const where = []; | ||
|
||
expect(() => validateWhereClause(where)).toThrowError('The "where" clause cannot be an empty array [].'); | ||
}); | ||
}); |
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.
🛠️ Refactor suggestion
Missing cases: nested OR/AND/NOT with undefineds
If you extend validation to recurse (suggested), add tests like:
- { OR: [{ id: undefined }] } → throws
- { AND: [{ teamId: null }, { id: { in: [1, 2] } }] } → ok
- { NOT: [{ name: { equals: undefined } }] } → throws
🤖 Prompt for AI Agents
In packages/prisma/extensions/disallow-undefined-where-clause.test.ts around
lines 5 to 79, add unit tests covering nested logical operators so recursion is
validated: add tests asserting that { OR: [{ id: undefined }] } throws the
expected undefined error, that { AND: [{ teamId: null }, { id: { in: [1,2] } }]
} does not throw, and that { NOT: [{ name: { equals: undefined } }] } throws;
ensure the tests use the same validateWhereClause invocation and error message
assertions as the existing cases for consistency.
async findMany({ args, query }) { | ||
validateWhereClause(args.where); | ||
return query(args); |
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.
💡 Verification agent
❓ Verification inconclusive
Blocking: findMany guard breaks legitimate queries (see failing integrations). Allow undefined where for findMany.
Integration tests are failing because findMany commonly relies on omitting where (or passing it as undefined) to fetch all rows. Enforcing non-undefined here is a breaking change.
Harden deleteMany/updateMany (dangerous by default), but for findMany, only validate when where is provided (not undefined). This preserves existing behavior while still catching empty object/array and undefined fields when explicitly passed.
Apply this diff:
- async findMany({ args, query }) {
- validateWhereClause(args.where);
- return query(args);
- },
+ async findMany({ args, query }) {
+ // Allow undefined where for findMany to preserve "fetch all" semantics.
+ if (typeof args.where !== "undefined") {
+ validateWhereClause(args.where);
+ }
+ return query(args);
+ },
Evidence: Pipeline failures indicate findMany calls without where:
- apps/api/v1/test/lib/bookings/_get.integration-test.ts
- packages/features/flags/features.repository.integration-test.ts
#!/bin/bash
# Search for validateWhereClause definition and usages
echo "=== validateWhereClause occurrences ==="
rg -n "validateWhereClause" -C3
# Search for async findMany in extensions
echo
echo "=== findMany in disallow-undefined-where-clause.ts ==="
rg -n "async findMany" -C3
Allow undefined where in findMany to preserve “fetch all” behavior
Only run validateWhereClause
when args.where
is explicitly provided, so existing queries without a where
clause continue to work.
• packages/prisma/extensions/disallow-undefined-where-clause.ts – wrap the guard in an undefined
check within async findMany
.
• Integration tests failing in:
– apps/api/v1/test/lib/bookings/_get.integration-test.ts
– packages/features/flags/features.repository.integration-test.ts
Suggested change:
- async findMany({ args, query }) {
- validateWhereClause(args.where);
- return query(args);
- },
+ async findMany({ args, query }) {
+ // Preserve "fetch all" semantics by skipping validation when where is undefined
+ if (typeof args.where !== "undefined") {
+ validateWhereClause(args.where);
+ }
+ return query(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.
async findMany({ args, query }) { | |
validateWhereClause(args.where); | |
return query(args); | |
async findMany({ args, query }) { | |
// Preserve "fetch all" semantics by skipping validation when where is undefined | |
if (typeof args.where !== "undefined") { | |
validateWhereClause(args.where); | |
} | |
return query(args); | |
}, |
🤖 Prompt for AI Agents
In packages/prisma/extensions/disallow-undefined-where-clause.ts around lines 54
to 56, the current findMany always calls validateWhereClause(args.where) which
breaks existing "fetch all" queries; change it to only call validateWhereClause
when args.where is explicitly provided (i.e., if (args.where !== undefined)
validateWhereClause(args.where)) so queries without a where continue to work and
integration tests pass.
What does this PR do?
Summary by mrge
Disallowed undefined, empty object, and empty array values in Prisma where clauses to prevent invalid queries.