fix(orm): unify zenstack v3 and prisma behavior on empty branches in OR filter#2528
Merged
Conversation
…argument normalization
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated filter construction to ignore payload branches and operator entries that are entirely Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/filter.test.ts (1)
804-832: Please add the single-undefined-branch OR case to this test.This test validates mixed branches well, but it misses
OR: [{ id: undefined }], which is the core parity edge case.Suggested addition
const withUndefinedBranch = await client.user.findFirst({ where: { OR: [{ id: undefined }, { id: user2.id }], } as any, orderBy: { createdAt: 'asc' }, }); + const onlyUndefinedBranch = await client.user.findFirst({ + where: { + OR: [{ id: undefined }], + } as any, + orderBy: { createdAt: 'asc' }, + }); + expect(baseline?.email).toBe(user2.email); expect(withUndefinedBranch?.email).toBe(baseline?.email); + expect(onlyUndefinedBranch).toBeNull(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/filter.test.ts` around lines 804 - 832, Add a new assertion that checks the single-undefined-branch OR case by calling client.user.findFirst with where: { OR: [{ id: undefined }] } (similar to the existing baseline and withUndefinedBranch calls) and assert its email (or result) equals the baseline; locate the test "ignores undefined branch inside OR filter" and add a call like the baseline/withUndefinedBranch patterns and an expect comparing the single-undefined result to baseline?.email so the OR: [{ id: undefined }] parity edge case is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/orm/src/client/crud/dialects/base-dialect.ts`:
- Around line 267-274: The OR branch currently returns this.true() when all
sub-branches are filtered out, causing things like OR: [{ id: undefined }] to
match-all; update the OR handler in with('OR', ...) (within buildFilter /
enumerate / isTrue logic) so that when meaningfulBranches.length === 0 it
returns this.false() (not this.true()), i.e., treat an OR of
only-empty/undefined branches as no-match; keep the existing behavior for an
empty array (allBranches.length === 0) as false as well.
In `@packages/orm/src/client/crud/operations/base.ts`:
- Around line 2539-2544: The loop in doNormalizeArgs only recurses when
isPlainObject(value), so array-valued properties (e.g., where.OR) never get
normalized; update the logic inside the for (const [key, value] of
Object.entries(args)) loop to detect arrays (Array.isArray(value)) and iterate
over their elements, calling this.doNormalizeArgs(elem) for each element that is
a plain object (and optionally removing undefined elements), while keeping the
existing deletion of undefined for non-array entries; reference doNormalizeArgs,
isPlainObject, args, and the where.OR case to locate the change.
---
Nitpick comments:
In `@tests/e2e/orm/client-api/filter.test.ts`:
- Around line 804-832: Add a new assertion that checks the
single-undefined-branch OR case by calling client.user.findFirst with where: {
OR: [{ id: undefined }] } (similar to the existing baseline and
withUndefinedBranch calls) and assert its email (or result) equals the baseline;
locate the test "ignores undefined branch inside OR filter" and add a call like
the baseline/withUndefinedBranch patterns and an expect comparing the
single-undefined result to baseline?.email so the OR: [{ id: undefined }] parity
edge case is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 779ec2fb-2da5-49f5-87b1-766b8b51806f
📒 Files selected for processing (3)
packages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/operations/base.tstests/e2e/orm/client-api/filter.test.ts
… enhance test coverage
…refining argument normalization
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/orm/src/client/crud/dialects/base-dialect.ts (1)
267-271:⚠️ Potential issue | 🟠 MajorPrune
ORbranches after normalization, not by raw payload shape.
isAllUndefinedFilter()only catches top-level{ field: undefined }. A branch likeOR: [{ name: { contains: undefined } }]survives Line 269, then normalizes totrue, so Line 271 still becomes match-all instead of no-match. The pruning needs to happen on the built expression (!this.isTrue(expr)), while keeping an explicit literaltruebranch.Suggested fix
.with('OR', () => { - const branches = enumerate(payload) - .filter((subPayload) => !this.isAllUndefinedFilter(subPayload)) - .map((subPayload) => this.buildFilter(model, modelAlias, subPayload)); - return this.or(...branches); + const branches = enumerate(payload).map((subPayload) => ({ + subPayload, + expr: this.buildFilter(model, modelAlias, subPayload), + })); + + const meaningfulBranches = branches.filter( + ({ subPayload, expr }) => subPayload === true || !this.isTrue(expr), + ); + + return this.or(...meaningfulBranches.map(({ expr }) => expr)); })- private isAllUndefinedFilter(payload: unknown): boolean { - if (!payload || typeof payload !== 'object' || Array.isArray(payload)) { - return false; - } - const entries = Object.entries(payload); - return entries.length > 0 && entries.every(([, v]) => v === undefined); - }Also applies to: 1313-1319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/dialects/base-dialect.ts` around lines 267 - 271, The OR branch pruning currently uses isAllUndefinedFilter on the raw payload which misses cases that normalize to a true expression; update the .with('OR', ...) handler so you build each branch via this.buildFilter(model, modelAlias, subPayload) first, then filter out branches where this.isTrue(expr) is true (but preserve explicit literal true branches), and finally pass the remaining normalized branches into this.or(...branches); use this.isTrue to decide pruning instead of this.isAllUndefinedFilter and keep the explicit true branch handling consistent with other dialect code (see isAllUndefinedFilter, buildFilter, isTrue, enumerate, and or).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/orm/src/client/crud/dialects/base-dialect.ts`:
- Around line 267-271: The OR branch pruning currently uses isAllUndefinedFilter
on the raw payload which misses cases that normalize to a true expression;
update the .with('OR', ...) handler so you build each branch via
this.buildFilter(model, modelAlias, subPayload) first, then filter out branches
where this.isTrue(expr) is true (but preserve explicit literal true branches),
and finally pass the remaining normalized branches into this.or(...branches);
use this.isTrue to decide pruning instead of this.isAllUndefinedFilter and keep
the explicit true branch handling consistent with other dialect code (see
isAllUndefinedFilter, buildFilter, isTrue, enumerate, and or).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47112bf7-5d02-4524-87e3-5c3ff34dd123
📒 Files selected for processing (1)
packages/orm/src/client/crud/dialects/base-dialect.ts
…T and follow Prisma behavior
…into fix/or-queries
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/orm/src/client/crud/operations/base.ts (1)
1155-1155: Type annotation mismatch with boolean default value.The variable
combinedWherehas typeRecord<string, unknown>, butwhere ?? truecan assign a boolean. WhilebuildFiltercorrectly handles boolean inputs per its signature (where: boolean | object | undefined), the type annotation here should reflect that to maintain type safety.💡 Suggested fix
-let combinedWhere: Record<string, unknown> = where ?? true; +let combinedWhere: Record<string, unknown> | boolean = where ?? true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/operations/base.ts` at line 1155, The local variable combinedWhere is incorrectly typed as Record<string, unknown> while it is initialized from where ?? true which can be a boolean; change the type annotation to boolean | Record<string, unknown> (or the exact union used by buildFilter) so combinedWhere's type matches buildFilter's signature and callers (refer to combinedWhere and buildFilter in base.ts) and update any narrowings if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/orm/src/client/crud/operations/base.ts`:
- Line 1155: The local variable combinedWhere is incorrectly typed as
Record<string, unknown> while it is initialized from where ?? true which can be
a boolean; change the type annotation to boolean | Record<string, unknown> (or
the exact union used by buildFilter) so combinedWhere's type matches
buildFilter's signature and callers (refer to combinedWhere and buildFilter in
base.ts) and update any narrowings if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5df59c9d-0fa8-4014-8ea7-27080dd99b27
📒 Files selected for processing (5)
packages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/utils/object-utils.tstests/e2e/orm/client-api/filter.test.tstests/e2e/orm/client-api/find.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/orm/client-api/filter.test.ts
ymc9
approved these changes
Apr 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I found this issue in our migration to v3 where behavior differed from previous prisma based v2 version.
For example queries such as:
Is treated by zenstack as:
but prisma interpreted it as:
Work done
Possible further discussions
The way zenstack interprets these queries seems more intuitive than prisma. Question is whether project aims for 1:1 behavior parity with how prisma queries data (I assume so)?
Summary by CodeRabbit
Bug Fixes
Tests