fix(orm): unify zenstack v3 and prisma behavior on empty branches in OR filter#2528
Open
eredzik wants to merge 4 commits intozenstackhq:devfrom
Open
fix(orm): unify zenstack v3 and prisma behavior on empty branches in OR filter#2528eredzik wants to merge 4 commits intozenstackhq:devfrom
eredzik wants to merge 4 commits intozenstackhq:devfrom
Conversation
…argument normalization
Contributor
📝 WalkthroughWalkthroughThe PR modifies the base dialect's OR filter construction to exclude branches with entirely undefined payloads and to skip undefined operator values in standard filters. It introduces a private helper method to identify all-undefined branches. Two new E2E tests validate this behavior for OR clause handling of undefined values. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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
ymc9
reviewed
Mar 31, 2026
| key: (typeof LOGICAL_COMBINATORS)[number], | ||
| payload: any, | ||
| ): Expression<SqlBool> { | ||
| return match(key) |
Member
There was a problem hiding this comment.
I think we should probably do a refactor here and uniformly preprocess payload to strip:
- object entries with
undefinedvalues like:AND: { id: undefined, x: 1 }, orAND: [ {id: undefined}, { x, 1}] - array elements that are objects that are empty (may be result of step above)
Then have the processed payload enter the following match statement? What do you think?
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