feat: between operator
#569
Conversation
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/orm/src/client/crud/validator/index.ts (1)
730-763: Tighten Zod schema forbetweento enforce a[start, end]pair
makeCommonPrimitiveFilterComponentscurrently defines:between: baseSchema.array().optional(),while the
CommonPrimitiveFiltertype and dialect both assumebetweenis exactly[start, end]. The dialect destructures withconst [start, end] = rhsbut only checks thatrhsis an array—it does not validate length. This allows malformed payloads likebetween: []orbetween: [1]to pass validation and silently produce incorrect SQL.Align the validator with the type and dialect by constraining the array to exactly 2 elements:
Suggested change in
makeCommonPrimitiveFilterComponents- between: baseSchema.array().optional(), + between: baseSchema.array().length(2).optional(),This ensures both typed and untyped callers must supply exactly two endpoints for
between.packages/server/src/api/rest/index.ts (1)
94-109: RESTbetweenoperator is not properly parsed for array format
FilterOperationsnow includes'between', butmakeFilterValuedoes not special-case it to construct the[start, end]tuple that the validator and ORM dialect require. Instead, it falls through to the default branch and returns{ between: coerced }with a single scalar value.For a REST query like
filter[age$between]=1,10, this produces{ between: 1 }instead of the required{ between: [1, 10] }, causing validation failures when the request reaches the ORM layer.The validator expects
baseSchema.array()(see packages/orm/src/client/crud/validator/index.ts:748), and the dialect explicitly validates that the value is an array (packages/orm/src/client/crud/dialects/base-dialect.ts:768-771).Add special-case handling for
betweeninmakeFilterValueto split the comma-separated value and construct the proper two-element array:Suggested fix
} else { + if (op === 'between') { + const parts = value + .split(',') + .filter((i) => i) + .map((v) => this.coerce(fieldDef, v)); + if (parts.length !== 2) { + throw new InvalidValueError(`"between" expects exactly 2 comma-separated values`); + } + return { between: [parts[0]!, parts[1]!] }; + } + const coerced = this.coerce(fieldDef, value); switch (op) {packages/orm/src/client/crud/dialects/base-dialect.ts (1)
718-772: Add length invariant forbetweento prevent undefined comparisonsThe
betweencase relies on TypeScript's type definition (between?: [start: DataType, end: DataType]) to enforce a 2-element array, but the runtime validator inindex.tsonly checksbaseSchema.array().optional()without enforcing length. This leaves the code path reachable from JavaScript callers or scenarios that bypass TypeScript. If an array with fewer than 2 elements reaches this code, destructuring[start, end]results inendbeingundefined, producinglhs <= undefined.Add a length invariant to match the pattern used in the
inandnotIncases:Suggested fix for the
betweenbranch.with('between', () => { invariant(Array.isArray(rhs), 'right hand side must be an array'); + invariant(rhs.length === 2, '"between" expects exactly 2 values: [start, end]'); const [start, end] = rhs; return this.eb.and([this.eb(lhs, '>=', start), this.eb(lhs, '<=', end)]); })
🧹 Nitpick comments (3)
packages/orm/src/client/crud-types.ts (1)
444-448: Type-levelbetweenlooks good; consider clarifying inclusiveness in docsThe
between?: [start: DataType, end: DataType];addition toCommonPrimitiveFiltermatches the intended tuple shape and works well with the dialect implementation (>= start && <= end). You may want to note “inclusive” in the JSDoc to make the semantics explicit for users.tests/e2e/orm/client-api/filter.test.ts (2)
318-364: Stringbetweentests give good coverage of inclusive behaviorThe new
betweencases nicely exercise empty ranges, single-point ranges, and multi-row ranges in lexicographic order. This is a solid end-to-end guard for the new operator on string fields.If you want to harden further, you could add a case with reversed bounds (e.g.
between: ['z@test.com', 'a@test.com']) to assert it yields zero results, documenting that callers must passstart <= end.
634-665: Datebetweentests are strong; consider also testing ISO string endpointsThese
createdAtbetweentests check equal endpoints, full span between two timestamps, and disjoint ranges, which is great for Date objects.Given the validator also accepts ISO strings for DateTime, you might add one or two cases using
between: [user1.createdAt.toISOString(), user2.createdAt.toISOString()]to confirm strings behave identically end-to-end.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/orm/src/client/crud-types.tspackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/validator/index.tspackages/server/src/api/rest/index.tstests/e2e/orm/client-api/filter.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/client-api/filter.test.ts
🧠 Learnings (7)
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/server/src/api/rest/index.tspackages/orm/src/client/crud/validator/index.tspackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud-types.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/orm/src/client/crud/validator/index.tspackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud-types.ts
📚 Learning: 2025-10-21T16:04:56.292Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/crud/dialects/base-dialect.ts:745-747
Timestamp: 2025-10-21T16:04:56.292Z
Learning: In packages/runtime/src/client/crud/dialects/base-dialect.ts, it's intentional that buildCursorFilter applies default ordering (via makeDefaultOrderBy fallback) while buildOrderBy does not. This ensures cursor-based pagination always has stable ordering for correctness, while regular queries remain unordered unless explicitly specified. This design is to be consistent with Prisma's pagination requirements.
Applied to files:
packages/orm/src/client/crud/validator/index.tspackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud-types.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud-types.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/client-api/filter.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/client-api/filter.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
tests/e2e/orm/client-api/filter.test.ts
🧬 Code graph analysis (2)
packages/orm/src/client/crud/validator/index.ts (1)
tests/e2e/orm/prisma-consistency/test-utils.ts (1)
baseSchema(89-94)
packages/orm/src/client/crud/dialects/base-dialect.ts (1)
packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
⏰ 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: build-test (22.x, sqlite)
- GitHub Check: build-test (22.x, postgresql)
🔇 Additional comments (1)
tests/e2e/orm/client-api/filter.test.ts (1)
459-465: Numericbetweentests validate the key edge casesThe
agebetweentests cover exact, lower-inclusive, upper-inclusive, and out-of-range scenarios, which is exactly what we need to verify the operator on numeric fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/test/api/rest.test.ts (1)
571-578: Optionally verify which specific record is returned.The test validates that one record is returned, but doesn't assert that it's the correct post (id: 2, viewCount: 1). Consider adding an assertion to confirm the correct record matches:
expect(r.body.data).toHaveLength(1); expect(r.body.data[0]).toMatchObject({ id: 2 });This would provide stronger validation of the
betweenfilter behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/orm/src/client/crud-types.tspackages/server/test/api/rest.test.tstests/e2e/orm/client-api/filter.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/orm/src/client/crud-types.ts
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/client-api/filter.test.ts
🧠 Learnings (4)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/client-api/filter.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/client-api/filter.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
tests/e2e/orm/client-api/filter.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
tests/e2e/orm/client-api/filter.test.ts
🔇 Additional comments (3)
tests/e2e/orm/client-api/filter.test.ts (3)
318-369: Excellent test coverage for stringbetweenfilters.The tests comprehensively validate:
- Empty ranges and non-overlapping ranges
- Reversed order (line 331-333)
- Exact boundary matches (inclusive behavior)
- Various inclusive ranges
This thoroughly exercises the
betweenoperator for string fields.
464-470: Good test coverage for numericbetweenfilters.The tests validate:
- Exact boundary matches (line 465)
- Inclusive behavior on both lower and upper bounds (lines 466-467)
- Non-matching ranges (lines 468-469)
This provides solid coverage for the
betweenoperator on numeric fields.
639-671: Comprehensive test coverage for datebetweenfilters.The tests validate:
- Exact timestamp matches (lines 640-649)
- Inclusive range behavior (line 650-654)
- Explicit date ranges (line 655-659)
- Mixed boundary conditions with historical dates (lines 660-670)
This thoroughly exercises the
betweenoperator for date/timestamp fields.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/filter.test.ts (1)
523-525: Minor style: missing semicolons.The date variable declarations are missing semicolons, which is inconsistent with the rest of the codebase (e.g., lines 526, 529).
♻️ Suggested fix
- const now = new Date() - const past = new Date(now.getTime() - 1) - const future = new Date(now.getTime() + 2) + const now = new Date(); + const past = new Date(now.getTime() - 1); + const future = new Date(now.getTime() + 2);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/validator/index.tspackages/server/src/api/rest/index.tspackages/server/test/api/rest.test.tstests/e2e/orm/client-api/filter.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/orm/src/client/crud/dialects/base-dialect.ts
- packages/server/test/api/rest.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/client-api/filter.test.ts
🧠 Learnings (5)
📚 Learning: 2025-10-21T16:04:56.292Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/crud/dialects/base-dialect.ts:745-747
Timestamp: 2025-10-21T16:04:56.292Z
Learning: In packages/runtime/src/client/crud/dialects/base-dialect.ts, it's intentional that buildCursorFilter applies default ordering (via makeDefaultOrderBy fallback) while buildOrderBy does not. This ensures cursor-based pagination always has stable ordering for correctness, while regular queries remain unordered unless explicitly specified. This design is to be consistent with Prisma's pagination requirements.
Applied to files:
packages/orm/src/client/crud/validator/index.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/orm/src/client/crud/validator/index.tspackages/server/src/api/rest/index.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/client-api/filter.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/client-api/filter.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
tests/e2e/orm/client-api/filter.test.ts
🧬 Code graph analysis (1)
tests/e2e/orm/client-api/filter.test.ts (2)
packages/orm/src/client/functions.ts (1)
now(109-109)tests/e2e/orm/client-api/utils.ts (1)
createUser(6-21)
⏰ 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: build-test (22.x, postgresql)
- GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (4)
packages/orm/src/client/crud/validator/index.ts (1)
748-748: LGTM! Correct Zod v4 syntax for the between operator.The schema correctly validates a 2-element array using
.array().length(2).optional(), which aligns with Zod v4 array constraints and is consistent with the existing filter operator patterns.packages/server/src/api/rest/index.ts (2)
99-99: LGTM! Between operator correctly added to filter operations.
943-952: Correct line reference and document the comma-handling limitation.The code at lines 1943–1952 correctly implements the
betweenoperator with appropriate validation. However, a genuine edge case exists: if field values contain commas (e.g., JSON fields with comma-separated data), the comma-splitting approach produces incorrect results. This limitation applies tobetween,hasSome, andhasEveryoperators (lines 1945, 1960) and other multi-value filters that use.split(',').This pattern is consistent across the REST API's filter parsing, but there is no escape mechanism to handle comma-containing values and no test coverage for this scenario. Consider documenting this limitation or implementing value escaping if field values with commas need to be supported.
tests/e2e/orm/client-api/filter.test.ts (1)
318-369: Excellent test coverage for the between operator!The test suite comprehensively validates the
betweenoperator across string, numeric, and date fields, covering:
- Boundary conditions (inclusive start and end)
- Empty ranges and exact matches
- Reversed ranges (correctly expect 0 results)
- Multiple data types (Date objects and ISO strings for dates)
- Edge cases with past/future fixtures
The tests align well with the PR's stated behavior that
betweenis "equivalent to applyinggteandlte(inclusive)".Based on coding guidelines, these e2e tests validate real-world filtering scenarios that would be common in production applications.
Also applies to: 464-471, 643-709
1 participant
Adds a
betweenoperator for use with numbers, strings, and dates. This is equivalent to agteandlte(inclusive).This implementation does not use the
BETWEENoperator because SQLite is pretty funky with dates, and I wanted to keep this simple, but if that is something which is desired I can change it.Sample Usage
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.