feat(orm): typed-json filters #473
Conversation
|
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. WalkthroughImplements typedef-aware typed-JSON filtering across the ORM: introduces generic TypedJson filter types and helpers, replaces JSON path arrays with string paths, updates validator schemas for typed JSON, adds requireTypeDef helper, and extends BaseCrudDialect plus Postgres/SQLite implementations for new string/array JSON predicates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
Claude finished @ymc9's task —— View job Pull Request ReviewI've completed a comprehensive review of this typed JSON filters implementation. Here's my analysis: Review Summary
Overall Assessment:
|
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive typed JSON filter support, enabling direct field-based filtering on JSON fields with type definitions. The implementation shifts from array-based path syntax to standard JSON path strings (e.g., '$.a.b' instead of ['a', 'b']) and introduces typed filters that support direct field access, nested filtering via is/isNot operators, and array operations (some/every/none).
Key Changes
- JSON Path Syntax: Migrated from array-based paths
['a', 'b']to JSON path strings'$.a.b','$.x[1]' - Typed JSON Filters: Added direct field filtering for typed JSON fields with support for nested structures and array operations
- Dialect-Specific Implementations: Enhanced SQLite and PostgreSQL dialects with typed JSON support including path selection, string filtering, and array predicate handling
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/orm/schemas/typed-json/schema.zmodel | Added tags array and zip optional field to test schema for comprehensive typed JSON testing |
| tests/e2e/orm/schemas/typed-json/schema.ts | Generated schema updates reflecting new test fields |
| tests/e2e/orm/client-api/json-filter.test.ts | Converted path syntax from arrays to strings; added 216 lines of comprehensive typed JSON filter tests including null handling, nested filters, and array operations |
| pnpm-workspace.yaml | Bumped better-sqlite3 to ^12.5.0 for Node 25.x compatibility |
| pnpm-lock.yaml | Lockfile updates for better-sqlite3 dependency version bump |
| packages/schema/src/schema.ts | Added GetTypeDefFieldType utility type for extracting field types from type definitions |
| packages/orm/src/client/query-utils.ts | Added requireTypeDef utility function for safe type definition retrieval |
| packages/orm/src/client/crud/validator/index.ts | Implemented makeTypedJsonFilterSchema for validating typed JSON filters; renamed makePrimitiveSchema to makeScalarSchema; changed optional typedef fields from .optional() to .nullish() |
| packages/orm/src/client/crud/dialects/base-dialect.ts | Added buildTypedJsonFilter, buildTypedJsonArrayFilter, and buildTypeJsonNonArrayFilter methods; refactored buildJsonFilter to distinguish plain vs typed JSON; made buildJsonPathSelection, buildJsonStringFilter, and buildJsonArrayExistsPredicate abstract |
| packages/orm/src/client/crud/dialects/sqlite.ts | Implemented SQLite-specific JSON path selection using -> operator; added buildJsonStringFilter with quote-aware patterns; implemented buildJsonArrayExistsPredicate using json_each |
| packages/orm/src/client/crud/dialects/postgresql.ts | Implemented PostgreSQL JSON path selection using jsonb_path_query_first; added case-sensitive/insensitive string filtering; implemented array predicate using jsonb_array_elements |
| packages/orm/src/client/crud-types.ts | Added comprehensive TypedJsonFilter type system with ArrayTypedJsonFilter, NonArrayTypedJsonFilter, and TypedJsonFieldsFilter recursive types; changed path type from string[] to string |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/orm/src/client/crud/dialects/sqlite.ts (1)
391-413: Inconsistent and overly-restrictive use ofjsonb_each— requires SQLite 3.51.0 (November 2025).Line 393 uses
jsonb_eachforarray_contains, while line 409 usesjson_eachforbuildJsonArrayExistsPredicate. Thejsonb_eachfunction was only added in SQLite 3.51.0 (released November 2025), making it unavailable on virtually all production SQLite installations. Thejson_eachvariant has been available since SQLite 3.9.0 (2015).Replace with
json_eachfor broad compatibility:- .with('array_contains', () => sql<any>`EXISTS (SELECT 1 FROM jsonb_each(${lhs}) WHERE value = ${value})`) + .with('array_contains', () => sql<any>`EXISTS (SELECT 1 FROM json_each(${lhs}) WHERE value = ${value})`)This aligns with line 409's usage and supports SQLite versions from 2015 onward.
🧹 Nitpick comments (1)
packages/orm/src/client/crud/dialects/base-dialect.ts (1)
524-546: Consider throwing on unrecognized filter patterns instead of returningtrue().When the filter doesn't match known plain JSON filter keys and isn't a typedef, the code returns
this.true()(line 544), which silently accepts any input. This could mask invalid filter payloads.Consider being more explicit about unrecognized patterns:
} else if (isTypeDef(this.schema, fieldDef.type)) { return this.buildTypedJsonFilter(receiver, filter, fieldDef.type, !!fieldDef.array); } else { - return this.true(); + // Plain Json field without recognized filter keys - treat as no filter + // This handles cases like empty objects or unknown keys + return this.true(); }Alternatively, if unknown keys should be rejected:
} else { - return this.true(); + const unknownKeys = Object.keys(filter).filter(k => !['path', 'equals', 'not', 'string_contains', 'string_starts_with', 'string_ends_with', 'array_contains', 'array_starts_with', 'array_ends_with', 'mode'].includes(k)); + if (unknownKeys.length > 0) { + invariant(false, `Invalid JSON filter keys: ${unknownKeys.join(', ')}`); + } + return this.true(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
packages/orm/src/client/crud-types.ts(6 hunks)packages/orm/src/client/crud/dialects/base-dialect.ts(6 hunks)packages/orm/src/client/crud/dialects/postgresql.ts(2 hunks)packages/orm/src/client/crud/dialects/sqlite.ts(4 hunks)packages/orm/src/client/crud/validator/index.ts(11 hunks)packages/orm/src/client/query-utils.ts(1 hunks)packages/schema/src/schema.ts(1 hunks)pnpm-workspace.yaml(1 hunks)tests/e2e/orm/client-api/json-filter.test.ts(28 hunks)tests/e2e/orm/schemas/typed-json/schema.ts(2 hunks)tests/e2e/orm/schemas/typed-json/schema.zmodel(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zmodel: Always runzenstack generateafter modifying ZModel schemas
ZModel schema files should define database structure and policies that compile to TypeScript viazenstack generate
Files:
tests/e2e/orm/schemas/typed-json/schema.zmodel
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/json-filter.test.tstests/e2e/orm/schemas/typed-json/schema.ts
🧠 Learnings (8)
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
Applied to files:
packages/orm/src/client/query-utils.tstests/e2e/orm/schemas/typed-json/schema.zmodelpackages/schema/src/schema.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/crud/validator/index.tspackages/orm/src/client/crud/dialects/base-dialect.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/query-utils.tspackages/orm/src/client/crud/dialects/sqlite.tspackages/orm/src/client/crud/validator/index.tspackages/orm/src/client/crud/dialects/base-dialect.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 **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/orm/src/client/query-utils.tstests/e2e/orm/schemas/typed-json/schema.zmodelpackages/orm/src/client/crud/validator/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 tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/schemas/typed-json/schema.zmodeltests/e2e/orm/client-api/json-filter.test.tspackages/orm/src/client/crud/validator/index.tstests/e2e/orm/schemas/typed-json/schema.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/dialects/sqlite.tspackages/orm/src/client/crud/validator/index.tspackages/orm/src/client/crud/dialects/base-dialect.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/json-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:
packages/orm/src/client/crud-types.tspackages/orm/src/client/crud/validator/index.tspackages/orm/src/client/crud/dialects/base-dialect.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: Use `pnpm` with workspaces for package management, pinned to version `pnpm10.12.1`
Applied to files:
pnpm-workspace.yaml
🧬 Code graph analysis (6)
packages/orm/src/client/query-utils.ts (2)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/errors.ts (1)
createInternalError(147-151)
packages/orm/src/client/crud/dialects/sqlite.ts (1)
packages/orm/src/client/crud/dialects/base-dialect.ts (1)
buildFilter(111-158)
tests/e2e/orm/client-api/json-filter.test.ts (2)
packages/orm/src/client/null-values.ts (6)
JsonNull(10-10)JsonNull(11-11)DbNull(4-4)DbNull(5-5)AnyNull(16-16)AnyNull(17-17)packages/testtools/src/client.ts (1)
createTestClient(101-248)
packages/orm/src/client/crud-types.ts (1)
packages/schema/src/schema.ts (11)
GetModelFieldType(166-170)FieldIsArray(262-266)ModelFieldIsOptional(238-242)SchemaDef(11-19)GetEnums(136-136)GetEnum(138-140)GetTypeDefs(142-142)GetTypeDefFields(172-175)GetTypeDefFieldType(183-187)TypeDefFieldIsArray(250-254)TypeDefFieldIsOptional(244-248)
packages/orm/src/client/crud/dialects/postgresql.ts (1)
packages/orm/src/client/crud/dialects/base-dialect.ts (1)
buildFilter(111-158)
packages/orm/src/client/crud/validator/index.ts (4)
packages/schema/src/schema.ts (2)
AttributeApplication(40-43)BuiltinType(88-98)packages/orm/src/client/query-utils.ts (2)
getTypeDef(28-30)getEnum(197-199)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/orm/src/client/null-values.ts (1)
DbNullClass(1-3)
⏰ 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). (4)
- GitHub Check: Agent
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (23)
packages/schema/src/schema.ts (1)
183-188: LGTM - well-designed type utility.The new
GetTypeDefFieldTypetype follows the established pattern ofGetModelFieldType(line 166-170) and properly extracts thetypeproperty from a TypeDef field. This enables typed JSON filtering to resolve nested field types at compile time.tests/e2e/orm/schemas/typed-json/schema.ts (2)
62-68: LGTM - auto-generated schema correctly reflects zmodel changes.The
tagsfield is correctly defined as an optional array of String (optional: true, array: true), matching thetags: String[]?definition in the corresponding zmodel file.
77-82: LGTM - Address.zip field correctly defined.The
zipfield is properly defined as an optional String, consistent withzip: String?in the zmodel.tests/e2e/orm/schemas/typed-json/schema.zmodel (2)
22-22: LGTM - new test field for optional array filtering.The
tags: String[]?field adds test coverage for optional arrays of primitives within typed JSON structures. As per coding guidelines, ensurezenstack generateis run after modifying ZModel schemas.
27-27: LGTM - new test field for nullable nested primitives.The
zip: String?field enables testing null filtering on optional nested fields within typed JSON.packages/orm/src/client/query-utils.ts (1)
40-46: LGTM - consistent helper function following established patterns.The
requireTypeDeffunction mirrors therequireModelpattern (lines 32-38), providing a throwing variant ofgetTypeDeffor cases where the type definition must exist. This is used by the typed JSON filtering logic in the CRUD dialects.tests/e2e/orm/client-api/json-filter.test.ts (3)
252-323: LGTM - path syntax migrated to JSONPath strings.The path filter tests are correctly updated from array syntax to JSONPath string notation (e.g.,
'$.a','$.x[1]'). The test coverage includes root path ('$'), nested objects ('$.a.b'), and array indices ('$.x[0]').
324-480: Excellent null filter test coverage.Comprehensive testing of
JsonNull,DbNull, andAnyNullacross multiple scenarios:
- Non-null values (lines 327-358)
- Null values in objects (lines 360-401)
- Top-level null handling (lines 403-413)
- Nulls within arrays (lines 415-480)
This ensures the semantic differences between JSON null, database NULL, and path-not-found are properly distinguished.
823-1038: Comprehensive typed JSON filtering tests.The new
typed json direct filteringtest block provides excellent coverage of the new feature:
- Direct field filtering (
name: 'Alice')- Numeric comparisons (
age: { lt: 20 })- Deep field access (
address: { country: 'US' })is/isNotoperators- Mixed shallow and deep filtering
- Nullable field handling with
nullliteral- Array quantifiers (
some,every,none)- String operators in nested arrays (
startsWith)- Rejection of mixed plain/typed JSON filters
- Null filter for optional typedef fields
This validates the typed JSON filtering feature works correctly at runtime.
packages/orm/src/client/crud/validator/index.ts (6)
300-337: LGTM - well-structured scalar schema builder.Renaming from
makePrimitiveSchematomakeScalarSchemabetter reflects its purpose, as it now handles:
- TypeDefs (recursive typed JSON)
- Enums
- Built-in primitives (String, Int, Float, etc.)
The implementation correctly delegates to specialized handlers for each type category.
369-390: Improved typedef schema with nullish handling.The changes correctly:
- Use
makeScalarSchemafor nested field types (line 370)- Apply
nullish()for optional fields (line 375), allowing bothnullandundefined- Use a custom schema wrapper (lines 385-387) to preserve original data order since Zod doesn't maintain object field order
540-613: Well-designed typed JSON filter schema builder.The
makeTypedJsonFilterSchemacomprehensively handles:
- Non-array fields: Field-level filters with proper type handling (lines 546-581)
- Array quantifiers:
some/every/nonefor array typedef fields (lines 584-592)- Object predicates:
is/isNotfor non-array fields (lines 593-601)- Plain JSON fallback: Allows raw JSON filter syntax (line 604)
- Null handling: Allows
nullliteral for optional fields (lines 606-609)The union at line 612 correctly combines all valid filter variants.
631-638: Consistent use of makeScalarSchema in array filters.The array filter schema now uses
makeScalarSchemafor element types (equals,has,hasEvery,hasSome), ensuring proper handling of typed elements including enums and typedefs.
659-669: Correct DbNull handling based on context.The logic now properly restricts
DbNull:
- In filters (
forFilter: true): Always allowed (line 663)- In mutations: Only allowed for nullable fields (lines 665-668)
This prevents invalid
DbNullusage in non-nullable JSON field mutations.
686-700: Path property now optional string.The
pathproperty is now an optional string (line 689) instead of a required array, aligning with the JSONPath string syntax ('$.a.b') used throughout the codebase. This simplifies the API while maintaining full path expression capability.pnpm-workspace.yaml (1)
13-13: LGTM - minor version bump for better-sqlite3.The catalog update from
^12.2.0to^12.5.0is a minor version increment. The releases between v12.3.0 and v12.5.0 include CI/build infrastructure updates, GitHub Action bumps, and updates to the bundled SQLite version (from 3.50.4 to 3.51.1). No breaking changes or API modifications are introduced.packages/orm/src/client/crud/dialects/postgresql.ts (1)
505-515: LGTM!The
buildJsonArrayExistsPredicateimplementation correctly uses PostgreSQL'sjsonb_array_elementsfunction with an EXISTS subquery to check for element matches within JSON arrays.packages/orm/src/client/crud/dialects/sqlite.ts (1)
371-385: LGTM!The
buildJsonStringFilterimplementation correctly handles the JSON string pattern matching. The unused_modeparameter is intentional since SQLite's LIKE is case-insensitive by default (as documented at line 470-471).packages/orm/src/client/crud-types.ts (2)
467-478: Breaking change:pathtype changed fromstring[]tostring.The
pathproperty type was changed fromstring[]tostring. This aligns with the new string-based JSON path notation (e.g.,'$.a','$.tags[0]') used in the dialect implementations, but may break existing code that passes array paths.Ensure this breaking change is documented in migration notes if applicable.
480-539: LGTM!The new
TypedJsonFiltertype system is well-designed:
XORensures mutual exclusivity between plain JSON filter and typed filter- Array filters (
some/every/none) correctly mirror relation filter semantics- Non-array filters support both
is/isNotand direct field access- Recursive
TypedJsonFieldsFilterproperly handles nested typedefs with array/optional propagationpackages/orm/src/client/crud/dialects/base-dialect.ts (3)
653-699: LGTM!The
buildTypeJsonNonArrayFilterimplementation correctly handles:
- Null comparison using JSON null literal (
'null')is/isNotfilter delegation with proper negation- Direct field filters with recursive typedef handling
- String field quote trimming via
trim()functionThe use of
requireTypeDefensures type safety when accessing typedef fields.
614-651: LGTM!The
buildTypedJsonArrayFiltercorrectly implements array quantifier semantics:
some: EXISTS predicatenone: NOT EXISTS predicateevery: NOT EXISTS with negated inner filter (equivalent to "no element fails the filter")The helper lambdas
makeExistsPredandmakeExistsNegatedPredcleanly encapsulate the EXISTS subquery construction.
1410-1437: LGTM!The abstract method signatures are well-defined:
buildJsonPathSelectionnow takesstring | undefinedpathbuildJsonStringFilterprovides pattern-based string matchingbuildJsonArrayExistsPredicateenables typed JSON array filteringThese abstractions allow PostgreSQL and SQLite dialects to provide appropriate implementations.
|
Claude finished @ymc9's task —— View job Pull Request ReviewI've completed a comprehensive review of this typed JSON filters implementation. Here's my analysis:
Overall Assessment:
|
|
Claude finished @ymc9's task —— View job I've completed a comprehensive review of this typed JSON filters implementation. Here's my analysis:
Overall Assessment: ✅ Excellent ImplementationThis is a high-quality implementation of typed JSON filtering with excellent type safety, comprehensive dialect support, and thorough test coverage. The code demonstrates solid architectural decisions and attention to detail. ✅ Major StrengthsType Safety & API Design (
Database Dialect Implementation
Validator Logic (
Test Coverage (
📊 Performance ConsiderationsPositive:
Watch Points:
🔒 Security AssessmentGood practices observed:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/orm/src/client/crud/validator/index.ts (1)
385-389: Consider improving error messages for the custom validator.The
z.custom()wrapper preserves object field order (per the comment), but sacrifices detailed Zod validation errors. When validation fails, users only see a generic failure message rather than field-specific errors. If field order preservation is critical, consider whether Zod 4's object handling has improved, or explore using.transform(v => v)as an alternative that might preserve both order and error detail.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/orm/src/client/crud/validator/index.ts(10 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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.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:
packages/orm/src/client/crud/validator/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/**/*.{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/validator/index.ts
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
Applied to files:
packages/orm/src/client/crud/validator/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 **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/orm/src/client/crud/validator/index.ts
🧬 Code graph analysis (1)
packages/orm/src/client/crud/validator/index.ts (4)
packages/schema/src/schema.ts (2)
AttributeApplication(40-43)BuiltinType(88-98)packages/orm/src/client/query-utils.ts (2)
getTypeDef(28-30)getEnum(197-199)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/orm/src/client/null-values.ts (1)
DbNullClass(1-3)
🔇 Additional comments (7)
packages/orm/src/client/crud/validator/index.ts (7)
300-337: LGTM! Good refactoring to rename the method.The rename from
makePrimitiveSchematomakeScalarSchemabetter reflects the method's scope—it handles primitives, enums, and TypeDefs. The logic remains consistent.
370-376: Good use ofmakeScalarSchemaandnullish()for Zod 4.The changes correctly use
makeScalarSchemafor consistency andnullish()to allow both null and undefined for optional TypeDef fields, which aligns with Zod 4 semantics.
540-613: Excellent TypeDef-aware filter implementation—verify edge cases through testing.The rewritten
makeTypedJsonFilterSchemaadds sophisticated filtering capabilities:
- Field-based filters for TypeDef properties
- Recursive handling for nested TypeDefs, arrays, enums, and primitives
- is/isNot filters for non-array TypeDefs
- some/every/none filters for array TypeDefs
- Plain JSON filter fallback for flexibility
- Proper null handling for optional fields
The recursive structure with
z.lazy()correctly avoids infinite loops, and the element schema derivation (line 583,array: false) is appropriate.Given the complexity and multiple edge cases (nested TypeDefs, arrays within TypeDefs, optional typed fields), please ensure comprehensive test coverage for:
- Nested TypeDef filtering (e.g.,
AddresscontainingLocation)- Array of TypeDef filtering (e.g.,
addresses: Address[])- Mixed primitive and TypeDef fields
- Optional TypeDef fields with null checks
Can you confirm that E2E or integration tests cover these typed JSON filtering scenarios?
631-639: LGTM! Consistent use ofmakeScalarSchemafor array elements.The array filter schema correctly derives element validators using
makeScalarSchema, aligning with the refactoring throughout this file.
655-680: Excellent correctness improvement for DbNull handling.The refined logic properly distinguishes between filter and mutation contexts:
- Filters: Always allow
DbNullClassandAnyNullClass(lines 659, 668) so queries can match DbNull values regardless of field nullability- Mutations: Only allow
DbNullClassif the field is nullable (lines 661-664), preventing invalid DbNull assignmentsThis prevents invalid mutations while maintaining flexible querying capabilities.
682-696: LGTM! Correct to makepathoptional in JSON filters.Making the path optional allows filtering the entire JSON value directly (e.g.,
{ equals: {...} }) without requiring a path specification. Users can still provide a path (now a string per the PR objectives) for nested field filtering.
1100-1127: LGTM! Consistent scalar schema usage in create and update paths.Both create (line 1101) and update (line 1371) data schemas correctly use
makeScalarSchemawith field attributes, maintaining consistency across mutation operations.Also applies to: 1370-1421
|
Claude finished @ymc9's task —— View job Pull Request ReviewI've completed a comprehensive review of this typed JSON filters implementation. Here's my analysis:
Overall Assessment: ✅ Excellent ImplementationThis is a high-quality implementation of typed JSON filtering with excellent type safety, comprehensive dialect support, and thorough test coverage. The code demonstrates solid architectural decisions and attention to detail. ✅ Major StrengthsType Safety & API Design (
Database Dialect Implementation
Validator Logic (
Test Coverage (
📊 Performance ConsiderationsPositive:
Watch Points:
🔒 Security AssessmentGood practices observed:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/orm/src/client/crud-types.ts (1)
714-719: Minor: Potential field name collision withis/isNot.The union type allows either
{ is: ..., isNot: ... }wrapper or directTypedJsonFieldsFilter. If a typedef has fields literally namedisorisNot, there could be ambiguous interpretation. This is an edge case and follows Prisma's established pattern for relation filters.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/orm/src/client/crud-types.ts(30 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
Applied to files:
packages/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-types.ts
🧬 Code graph analysis (1)
packages/orm/src/client/crud-types.ts (2)
packages/schema/src/schema.ts (14)
GetModelFieldType(166-170)FieldIsArray(262-266)GetEnum(138-140)SchemaDef(11-19)GetEnums(136-136)GetTypeDefs(142-142)GetTypeDefFields(172-175)GetTypeDefFieldType(183-187)TypeDefFieldIsArray(250-254)TypeDefFieldIsOptional(244-248)RelationFields(217-221)RelationFieldType(229-236)GetModels(121-121)NonRelationFields(211-215)packages/orm/src/utils/type-utils.ts (8)
NullableIf(8-8)MapBaseType(46-46)JsonValue(48-48)JsonNullValues(53-53)Optional(4-4)XOR(77-77)OrArray(59-59)NonEmptyArray(61-61)
⏰ 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). (3)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: claude-review
🔇 Additional comments (4)
packages/orm/src/client/crud-types.ts (4)
311-316: LGTM! TypedJsonFilter integration in WhereInput is well-structured.The typedef field detection and parameter passing to
TypedJsonFiltercorrectly propagates the schema, typedef name, array status, and optionality from the model field level.
692-751: Well-designed recursive TypedJsonFilter implementation.The type structure correctly handles:
- XOR between raw
JsonFilterand typed filter for flexibility- Array fields with
some/every/nonequantifiers- Non-array fields with
is/isNotpredicates or direct field matching- Recursive nesting for typedef fields containing other typedefs
- Proper nullable handling when
Optionalis trueOne observation:
WithAggregationsis hardcoded tofalsefor nestedEnumFilterandPrimitiveFilter(lines 743, 749), which is appropriate since aggregation filters only apply at the top query level.
377-406: LGTM! ArrayFilter and MapScalarType improvements.The generic parameter rename from
TtoTypeimproves readability. The added JSDoc comments andisEmptypredicate enhance the filter's usability and discoverability.
339-374: Excellent documentation additions throughout.The comprehensive JSDoc comments added to filter types, CRUD operations, and aggregation fields significantly improve developer experience and discoverability. The documentation accurately describes each field's purpose and includes helpful notes about dialect-specific behaviors (e.g.,
modenot effective for SQLite).
Summary by CodeRabbit
New Features
Tests
Schema
Chores
✏️ Tip: You can customize this high-level summary in your review settings.