feat(orm): implement JSON null values and equality filter #464
Conversation
|
Claude finished @ymc9's task —— View job Code Review for JSON Null Values and Equality FilterReviewing PR #464: feat(orm): implement JSON null values and equality filter Todo List:
SummaryThis PR implements a sophisticated solution for handling JSON null values in the ORM, distinguishing between database NULL, JSON null, and providing an "any null" matcher. The implementation is well-designed and addresses a real need in JSON field handling. ✅ StrengthsExcellent Architecture Design
Robust Implementation
Thorough Test Coverage
|
|
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. WalkthroughAdds first-class JSON support: introduces branded null sentinels (DbNull, JsonNull, AnyNull), JSON-aware CRUD types and filters, validator schemas for JSON values/filters, dialect handling for JSON nulls (Postgres/SQLite), public re-exports, e2e tests and generated test schemas for a PlainJson model. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive JSON null value handling and equality filtering for the ORM. It introduces a clear distinction between JSON null values (the literal JSON null), database NULL values, and a special AnyNull filter that matches both.
Key changes:
- Introduced
JsonNull,DbNull, andAnyNullclasses to distinguish between different types of null values - Implemented JSON equality and negation filters with support for all null value types
- Updated type definitions to allow
nullvalues within JSON objects while maintaining type safety
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/orm/src/client/null-values.ts | Defines three null value classes (JsonNull, DbNull, AnyNull) for precise null handling |
| packages/orm/src/client/index.ts | Exports the new null value classes for public API use |
| packages/orm/src/utils/type-utils.ts | Updates JsonValue, JsonObject, and JsonArray types to properly handle null values |
| packages/orm/src/client/crud-types.ts | Adds JsonFilter type and updates scalar payloads to support JSON null values |
| packages/orm/src/client/crud/validator/index.ts | Implements validation schemas for JSON values and filters with null support |
| packages/orm/src/client/crud/dialects/base-dialect.ts | Implements buildJsonFilter logic with null value handling for all dialects |
| packages/orm/src/client/crud/dialects/sqlite.ts | Transforms JsonNull/DbNull/AnyNull values for SQLite-specific JSON storage |
| packages/orm/src/client/crud/dialects/postgresql.ts | Transforms JsonNull/DbNull/AnyNull values for PostgreSQL-specific JSON storage |
| tests/e2e/orm/client-api/json-filter.test.ts | Comprehensive test suite covering JSON filters and null value distinctions |
| tests/e2e/orm/schemas/json/schema.zmodel | Test schema defining PlainJson model with required and optional JSON fields |
| tests/e2e/orm/schemas/json/schema.ts | Auto-generated TypeScript schema definition |
| tests/e2e/orm/schemas/json/models.ts | Auto-generated model type exports |
| tests/e2e/orm/schemas/json/input.ts | Auto-generated input type exports for CRUD operations |
| README.md | Adds documentation link reference |
💡 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: 0
🧹 Nitpick comments (6)
README.md (1)
28-29: Wrap the URL to satisfy markdownlint (MD034).
markdownlintflags the bare URL on Line 28. Consider wrapping it so it’s no longer “bare”:- > Read full documentation at 👉🏻 https://zenstack.dev/v3. + > Read full documentation at 👉🏻 <https://zenstack.dev/v3>.packages/orm/src/client/crud/dialects/sqlite.ts (1)
16-44: SQLite JSON null handling matches the intended sentinel semantics.
JsonNullClass→'null'(JSON text) andDbNullClass→null(DB NULL) correctly distinguish JSON null vs DB null for JSON fields.AnyNullClassguarded withinvariant(false, ...)is a sensible defensive check.If you want to be extra strict, you could optionally gate these
instanceofchecks behindtype === 'Json'to avoid affecting non‑JSON fields in case a sentinel leaks, but the type/validator layers should already prevent that.packages/orm/src/utils/type-utils.ts (1)
2-54: JSON utility types align with sentinel design; double‑check array/null semantics.
- Importing the sentinel types and defining
JsonNullValues = DbNull | JsonNull | AnyNullis consistent with the new JSON‑null handling.JsonValuenow excludesnull, whileJsonObjectallowsJsonValue | nullfor property values andJsonArrayisReadonlyArray<JsonValue>.This means JSON arrays cannot contain
nullat the type level (only objects can havenullvalues, and top‑level nulls are represented viaJsonNullValues). If you intend to support arrays like[1, null, 2], you may want to relaxJsonArray(and possiblyJsonValue) accordingly; if the goal is to disallow literalnullexcept via sentinels/top‑level filters, then this definition is consistent but fairly strict.packages/orm/src/client/crud/dialects/postgresql.ts (1)
17-52: Postgres JSON null handling is consistent with sqlite and jsonb semantics.
JsonNullClass→'null'(parsed by Postgres as JSONnullfor json/jsonb columns) andDbNullClass→null(SQL NULL) correctly encode the two null concepts.AnyNullClassbeing rejected withinvariant(false, ...)is a good defensive measure.- Placement before the JSON type branch keeps behavior centralized and mirrors the SQLite dialect.
As with SQLite, you could optionally guard these checks by
type === 'Json'if you want to avoid affecting non‑JSON fields in case of mis‑typed inputs, but functionally this looks solid.packages/orm/src/client/crud/validator/index.ts (2)
571-591: Review nullability handling inmakeJsonValueSchema.There's potential redundancy in nullability handling:
- Line 575: When
nullable=true,DbNullClassis added to the union- Line 590:
nullableIf(schema, nullable)is called, which would add.nullable()to the schemaThis means when
nullable=true, the schema accepts bothz.instanceof(DbNullClass)ANDnullvia.nullable(). While this may be intentional (allowing both the sentinel and literalnull), the tests at lines 86-98 injson-filter.test.tsshow that literalnullshould be rejected in favor of the sentinel types.Consider removing the
nullableIfwrapper or clarifying the intent:- return this.nullableIf(schema, nullable); + return schema;
593-599: Consider usingz.strictObjectfor consistency.Other filter schemas in this file use
z.strictObjectto reject unknown keys, butmakeJsonFilterSchemausesz.object. This inconsistency could allow unexpected keys to pass validation.private makeJsonFilterSchema(optional: boolean) { const valueSchema = this.makeJsonValueSchema(optional, true); - return z.object({ + return z.strictObject({ equals: valueSchema.optional(), not: valueSchema.optional(), }); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(1 hunks)packages/orm/src/client/crud-types.ts(6 hunks)packages/orm/src/client/crud/dialects/base-dialect.ts(2 hunks)packages/orm/src/client/crud/dialects/postgresql.ts(2 hunks)packages/orm/src/client/crud/dialects/sqlite.ts(2 hunks)packages/orm/src/client/crud/validator/index.ts(8 hunks)packages/orm/src/client/index.ts(1 hunks)packages/orm/src/client/null-values.ts(1 hunks)packages/orm/src/utils/type-utils.ts(2 hunks)tests/e2e/orm/client-api/json-filter.test.ts(1 hunks)tests/e2e/orm/schemas/json/input.ts(1 hunks)tests/e2e/orm/schemas/json/models.ts(1 hunks)tests/e2e/orm/schemas/json/schema.ts(1 hunks)tests/e2e/orm/schemas/json/schema.zmodel(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/json/models.tstests/e2e/orm/schemas/json/schema.tstests/e2e/orm/schemas/json/input.ts
**/*.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/json/schema.zmodel
🧠 Learnings (8)
📚 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:
packages/orm/src/client/index.tstests/e2e/orm/client-api/json-filter.test.tstests/e2e/orm/schemas/json/models.tsREADME.mdtests/e2e/orm/schemas/json/input.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/index.tstests/e2e/orm/client-api/json-filter.test.tspackages/orm/src/client/crud/validator/index.tspackages/orm/src/client/crud/dialects/base-dialect.tstests/e2e/orm/schemas/json/models.tsREADME.mdtests/e2e/orm/schemas/json/schema.tspackages/orm/src/client/crud-types.tstests/e2e/orm/schemas/json/input.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/index.tstests/e2e/orm/schemas/json/models.tsREADME.mdtests/e2e/orm/schemas/json/schema.tspackages/orm/src/client/crud-types.tstests/e2e/orm/schemas/json/input.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/json-filter.test.tstests/e2e/orm/schemas/json/models.tstests/e2e/orm/schemas/json/schema.tstests/e2e/orm/schemas/json/input.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/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:
tests/e2e/orm/schemas/json/schema.zmodeltests/e2e/orm/schemas/json/models.tsREADME.mdtests/e2e/orm/schemas/json/schema.tstests/e2e/orm/schemas/json/input.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: Database migrations should use Prisma CLI under the hood via ZenStack commands
Applied to files:
README.md
📚 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:
tests/e2e/orm/schemas/json/schema.ts
🧬 Code graph analysis (7)
packages/orm/src/client/crud/validator/index.ts (1)
packages/orm/src/client/null-values.ts (3)
JsonNullClass(7-9)DbNullClass(1-3)AnyNullClass(13-15)
packages/orm/src/client/crud/dialects/postgresql.ts (2)
packages/orm/src/client/null-values.ts (3)
JsonNullClass(7-9)DbNullClass(1-3)AnyNullClass(13-15)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/orm/src/client/crud/dialects/sqlite.ts (2)
packages/orm/src/client/null-values.ts (3)
JsonNullClass(7-9)DbNullClass(1-3)AnyNullClass(13-15)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/orm/src/client/crud/dialects/base-dialect.ts (4)
packages/schema/src/schema.ts (1)
BuiltinType(88-98)packages/orm/src/client/errors.ts (1)
createInvalidInputError(116-120)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/orm/src/client/null-values.ts (3)
DbNullClass(1-3)JsonNullClass(7-9)AnyNullClass(13-15)
packages/orm/src/client/null-values.ts (1)
packages/orm/src/client/index.ts (3)
DbNull(7-7)JsonNull(7-7)AnyNull(7-7)
packages/orm/src/client/crud-types.ts (3)
packages/orm/src/utils/type-utils.ts (2)
JsonValue(48-48)JsonNullValues(53-53)packages/orm/src/client/index.ts (2)
JsonNull(7-7)DbNull(7-7)packages/orm/src/client/null-values.ts (4)
JsonNull(10-10)JsonNull(11-11)DbNull(4-4)DbNull(5-5)
packages/orm/src/utils/type-utils.ts (2)
packages/orm/src/client/index.ts (3)
DbNull(7-7)JsonNull(7-7)AnyNull(7-7)packages/orm/src/client/null-values.ts (6)
DbNull(4-4)DbNull(5-5)JsonNull(10-10)JsonNull(11-11)AnyNull(16-16)AnyNull(17-17)
🪛 markdownlint-cli2 (0.18.1)
README.md
28-28: Bare URL used
(MD034, no-bare-urls)
⏰ 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 (19)
packages/orm/src/client/null-values.ts (1)
1-17: Branded null singletons look consistent and self-contained.The three sentinel classes plus singleton exports and type aliases are straightforward and align with their intended use in dialects and validators. No issues spotted.
packages/orm/src/client/index.ts (1)
7-7: Re-exportingAnyNull/DbNull/JsonNullfrom the client looks good.This cleanly exposes the null sentinels on the main ORM client surface without affecting existing exports.
tests/e2e/orm/schemas/json/schema.zmodel (1)
1-9: Schema is fine; ensure generated artifacts are refreshed.The
PlainJsonmodel (Json + Json?) is a good minimal schema for JSON tests. Please make surezenstack generatehas been rerun soschema.ts,models.ts,input.ts, etc., are in sync with this.zmodel.tests/e2e/orm/schemas/json/models.ts (1)
1-10: GeneratedPlainJsonmodel alias looks correct.Header, imports, and
PlainJson = $ModelResult<$Schema, "PlainJson">match the expected generated pattern; no manual changes needed.tests/e2e/orm/schemas/json/schema.ts (1)
9-46: LGTM!The auto-generated schema correctly defines a
PlainJsonmodel with required and optional JSON fields, suitable for testing JSON null value semantics. The structure aligns with ZenStack conventions.tests/e2e/orm/client-api/json-filter.test.ts (4)
6-20: LGTM!The simple equality filter test correctly validates basic JSON filtering with
equalsandnotoperators.
22-99: Comprehensive null type distinction testing.This test thoroughly validates the semantic differences between
JsonNull,DbNull, andAnyNullacross multiple scenarios including filtering, negation, and input validation. The test structure is well-organized with clear record creation and verification steps.
101-132: LGTM!The update test correctly validates state transitions between JSON null, DB null, and actual JSON objects.
134-152: Good coverage of JSON array handling.The test appropriately uses an inline schema with PostgreSQL provider to test
Json[]array fields, validating both direct array assignment and thesetsyntax, plus proper rejection ofDbNullfor array fields.Based on learnings, E2E tests should validate real-world schema compatibility - this test achieves that by testing PostgreSQL-specific JSON array support.
packages/orm/src/client/crud/dialects/base-dialect.ts (2)
518-534: LGTM!The JSON filter implementation correctly handles
equalsandnotoperators with proper clause construction usingbuildJsonValueFilterClause.
536-547: The implementation is correct as-is. ThebuildJsonValueFilterClausemethod correctly handles JSON null values consistently across PostgreSQL and SQLite using string literal comparisons, which is the standard approach for both databases. No dialect-specific overrides are required.packages/orm/src/client/crud/validator/index.ts (2)
1028-1034: LGTM!The create schema correctly allows
DbNullClassfor optional JSON fields, enabling explicit DB NULL assignment while keeping required JSON fields non-nullable.
1313-1318: LGTM!The update schema mirrors the create schema behavior for optional JSON fields, maintaining consistency.
tests/e2e/orm/schemas/json/input.ts (1)
1-30: LGTM!The auto-generated input types correctly export all CRUD operation argument types for the
PlainJsonmodel, following ZenStack conventions.packages/orm/src/client/crud-types.ts (5)
34-51: LGTM!The import organization is clean—runtime singleton types (
DbNull,JsonNull) come fromnull-values, while composite type aliases (JsonNullValues,JsonValue) come fromtype-utils. This separation maintains a clear dependency hierarchy.
354-366: LGTM!The integration of
JsonFilterintoPrimitiveFilterfollows the established pattern. The intentional omission ofNullableandWithAggregationsparameters is appropriate since JSON null semantics are handled through the dedicatedJsonNullValuesunion type.
458-461: LGTM!The
JsonFiltertype correctly implements equality-based JSON filtering with proper support for the three null semantics (DbNullfor database NULL,JsonNullfor JSON literal null,AnyNullfor matching either). The minimal filter surface (equals/notonly) aligns with standard JSON filtering capabilities.
783-810: Well-designed JSON mutation semantics.The distinction between optional and required JSON fields is correctly implemented:
- Required fields:
JsonValue | JsonNull— allows JSON literal null but prevents database NULL- Optional fields:
JsonValue | JsonNull | DbNull— additionally permits database NULLThis prevents the invalid state of setting a required field to database NULL while still allowing the JSON literal
nullvalue.
952-972: LGTM!Unifying create and update payloads through
ScalarFieldMutationPayloadensures consistent JSON null-handling semantics across all mutation operations while adhering to DRY principles.
Co-authored-by: Copilot <[email protected]>
|
Claude finished @ymc9's task —— View job Code Review for JSON Null Values and Equality FilterReviewing PR #464: feat(orm): implement JSON null values and equality filter Todo List:
SummaryThis PR implements a sophisticated solution for handling JSON null values in the ORM, distinguishing between database NULL, JSON null, and providing an "any null" matcher. The implementation is well-designed and addresses a real need in JSON field handling. ✅ StrengthsExcellent Architecture Design
Robust Implementation
Thorough Test Coverage
|
Co-authored-by: Copilot <[email protected]>
|
Claude finished @ymc9's task —— View job Claude finished @ymc9's task —— View job Code Review for JSON Null Values and Equality FilterReviewing PR #464: feat(orm): implement JSON null values and equality filter Todo List:
SummaryThis PR implements a sophisticated solution for handling JSON null values in the ORM, distinguishing between database NULL, JSON null, and providing an "any null" matcher. The implementation is well-designed and addresses a real need in JSON field handling. ✅ StrengthsExcellent Architecture Design
Robust Implementation
Thorough Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/orm/src/client/crud/validator/index.ts (3)
553-569: JSON filter currently only allows{ equals, not }objects (no bare value form)For other primitives, filters accept either a bare value or an operator object via
makeCommonPrimitiveFilterSchema, but forJsonyou only accept the operator object returned bymakeJsonFilterSchema. If that’s intentional (e.g., to future‑proof for more JSON operators), fine; otherwise, you might want to allow bare JSON values for consistency:- .with('Json', () => this.makeJsonFilterSchema(optional)) + .with('Json', () => + z.union([ + this.makeJsonValueSchema(optional, true), + this.makeJsonFilterSchema(optional), + ]), + )This would make
where: { jsonField: someJson }valid in addition towhere: { jsonField: { equals: someJson } }, matching scalar behavior.
571-591: JSON value schema null behavior: objects allownull, arrays don’t—please confirm intentThe new
makeJsonValueSchemalooks good overall, and thez.record(..., z.lazy(() => z.union([this.makeJsonValueSchema(false, false), z.null()])))part neatly addresses the earlier concern about{ key: null }being rejected forJsonObjectvalues.Two subtle points worth double‑checking:
- Elements of JSON arrays are
this.makeJsonValueSchema(false, false)only, so an array like[1, null]will be rejected unlessnullis represented viaJsonNullClass. If yourJsonArraytype allows plainnullitems, you may want to mirror the object behavior for arrays as well.- For filter usage (
nullable = trueandforFilter = true), the schema accepts plainnull(vianullableIf) as well asDbNullClassandAnyNullClass. That’s flexible but slightly redundant; ensure this matches your intended DB‑null vs JSON‑null semantics.If your TS JSON value types are meant to permit
nullinside arrays, consider adjusting the array branch accordingly.
593-599: JSON filter schema is non‑strict; consider usingz.strictObjectfor consistency
makeJsonFilterSchemacurrently usesz.object({ equals, not }), whereas other filter objects (e.g., for enums, numbers, arrays) usez.strictObject(...). That means extra keys under a JSON filter (e.g.{ equals: ..., foo: 1 }) will be silently allowed/stripped, unlike other scalar filters.For consistency and earlier feedback to users, you may want:
- return z.object({ + return z.strictObject({ equals: valueSchema.optional(), not: valueSchema.optional(), });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/orm/src/client/crud/validator/index.ts(8 hunks)packages/orm/src/utils/type-utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/orm/src/utils/type-utils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/orm/src/client/crud/validator/index.ts (1)
packages/orm/src/client/null-values.ts (3)
JsonNullClass(7-9)DbNullClass(1-3)AnyNullClass(13-15)
⏰ 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: claude-review
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (5)
packages/orm/src/client/crud/validator/index.ts (5)
35-35: Null sentinel imports are correctly wiredImporting
AnyNullClass,DbNullClass, andJsonNullClasshere matches their later usage in JSON value and filter schemas, so constructor identity will be consistent across the client.
332-335: DateTime and Json primitive schemas: behavior change is intentional but verify callers
DateTimenow accepts bothDateand ISO datetime strings viaz.union([z.date(), z.iso.datetime()]), andJsondelegates tomakeJsonValueSchema(false, false)for a unified JSON value shape. This looks consistent with the new JSON and datetime handling, but it does broaden accepted inputs, so please confirm any upstream code/tests that previously assumedDate‑only or narrower Json shapes are updated accordingly.
606-609: DateTime filter base schema correctly mirrors primitive DateTime supportUsing
z.union([z.iso.datetime(), z.date()])inmakeDateTimeFilterSchemakeeps the filter side aligned with the updated primitive DateTime schema while still flowing through the common primitive filter machinery. This looks coherent with the widened DateTime input support.
1009-1033: Create data: JsonDbNullhandling and nullability look consistentFor non‑relation fields in
makeCreateDataSchema, you now:
- Build the base schema via
makePrimitiveSchema(fieldDef.type, fieldDef.attributes)(so Json usesmakeJsonValueSchema(false, false)).- Make it optional when the field is optional or has a default.
- For optional Json fields only, allow
DbNullClassinstead of using.nullable(), while other optional fields still use.nullable().This cleanly separates DB null (
DbNullClass) from JSON null (JsonNullClasswithinmakeJsonValueSchema) for Json columns at create time, and preserves existing semantics for other scalar types.
1279-1318: Update data: JsonDbNullhandling aligns with create semanticsThe update path mirrors the create semantics:
- Base scalar schema still comes from
makePrimitiveSchema, so Json usesmakeJsonValueSchema(false, false).- Numeric fields get arithmetic operator objects as before.
- Optional Json fields accept
DbNullClassinstead of.nullable(), while other optional fields remain.nullable().- All fields are then made optional for updates.
This keeps create/update Json handling consistent and ensures DB‑null writes for Json fields always go through the explicit
DbNullClasssentinel rather than plainnull.
|
Claude finished @ymc9's task —— View job Code Review for JSON Null Values and Equality FilterReviewing PR #464: feat(orm): implement JSON null values and equality filter Todo List:
SummaryThis PR implements a sophisticated solution for handling JSON null values in the ORM, distinguishing between database NULL, JSON null, and providing an "any null" matcher. The implementation is well-designed and addresses a real need in JSON field handling. ✅ StrengthsExcellent Architecture Design
Robust Implementation
Thorough Test Coverage
|
|
Claude Code is working… I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/e2e/orm/client-api/undefined-values.test.ts (1)
10-12: Castingclienttoanyis redundant and weakens type-safetyOn Line 11,
(await createTestClient(schema)) as anybypasses compile-time checks even thoughclientis explicitly typed asClientContract<typeof schema>. IfcreateTestClient’s return type is now incompatible due to recent JSON/null typing changes, it’s better to adjustcreateTestClient’s signature (e.g., to return aClientContract-compatible type) than to cast here. This keeps the E2E test validating a realistic, strongly-typed client API while still reflecting real-world schema compatibility. Based on learnings, this aligns better with the intent of E2E tests.For example, if type compatibility allows, you could revert to:
- client = (await createTestClient(schema)) as any; + client = await createTestClient(schema);tests/e2e/apps/rally/rally.test.ts (1)
8-8: Type widening forClientContractin e2e test is reasonable, but consider a slightly tighter castSwitching
dbtoClientContract<SchemaType, any>and initializing it fromcreateTestClientkeeps the Rally e2e test focused on real schema compatibility while relaxing options typing, which is fine for this level of test. One small optional improvement: instead of casting the awaited result toany, you could cast to the concrete contract type to keep a clearer intent and avoid overusingany:db = (await createTestClient(schema, { /* ... */ })) as ClientContract<SchemaType, any>;Functionally it’s the same today (since
createTestClientreturnsany), but it documents the expected shape a bit better and may play nicer with future typing improvements. This still preserves the goal of validating Rally’s real-world schema behavior. Based on learnings, this continues to satisfy the “real-world schema compatibility” aim for e2e tests.Also applies to: 11-22
tests/e2e/orm/client-api/delegate.test.ts (1)
10-19: Align test client typing and make teardown more robustCasting the created client to
anyis okay for runtime tests, but it hides mismatches betweencreateTestClient’s return type andClientContract<SchemaType>. If possible, adjustcreateTestClient(orClientContract) soawait createTestClient(schema, ...)is assignable without anas anyhere and in similar tests.Also, consider guarding teardown against setup failures by using optional chaining, consistent with other tests:
- afterEach(async () => { - await client.$disconnect(); - }); + afterEach(async () => { + await client?.$disconnect(); + });tests/e2e/orm/client-api/group-by.test.ts (1)
10-11: Casting test client toanyhere is acceptable and keeps runtime behavior unchanged
clientstill has the declaredClientContract<typeof schema>type, and this cast just works around assignment typing while preserving the existing, realistic E2E coverage on the basic schema. Longer-term, you might consider tighteningcreateTestClient’s return type so callers don’t needas any, but this change is fine as-is. Based on learnings, this keeps the E2E contract realistic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
tests/e2e/apps/rally/rally.test.ts(2 hunks)tests/e2e/orm/client-api/aggregate.test.ts(1 hunks)tests/e2e/orm/client-api/count.test.ts(1 hunks)tests/e2e/orm/client-api/create-many-and-return.test.ts(2 hunks)tests/e2e/orm/client-api/create-many.test.ts(1 hunks)tests/e2e/orm/client-api/create.test.ts(1 hunks)tests/e2e/orm/client-api/delegate.test.ts(1 hunks)tests/e2e/orm/client-api/delete-many.test.ts(1 hunks)tests/e2e/orm/client-api/delete.test.ts(1 hunks)tests/e2e/orm/client-api/filter.test.ts(1 hunks)tests/e2e/orm/client-api/find.test.ts(1 hunks)tests/e2e/orm/client-api/group-by.test.ts(1 hunks)tests/e2e/orm/client-api/name-mapping.test.ts(1 hunks)tests/e2e/orm/client-api/raw-query.test.ts(1 hunks)tests/e2e/orm/client-api/transaction.test.ts(1 hunks)tests/e2e/orm/client-api/undefined-values.test.ts(1 hunks)tests/e2e/orm/client-api/update-many.test.ts(1 hunks)tests/e2e/orm/client-api/update.test.ts(1 hunks)tests/e2e/orm/client-api/upsert.test.ts(1 hunks)tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts(1 hunks)tests/e2e/orm/plugin-infra/on-kysely-query.test.ts(1 hunks)tests/e2e/orm/plugin-infra/on-query-hooks.test.ts(1 hunks)tests/e2e/orm/policy/basic-schema-read.test.ts(1 hunks)tests/e2e/tsconfig.json(1 hunks)tests/regression/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/regression/tsconfig.json
- tests/e2e/orm/plugin-infra/on-kysely-query.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/undefined-values.test.tstests/e2e/orm/client-api/update.test.tstests/e2e/orm/client-api/filter.test.tstests/e2e/orm/client-api/delegate.test.tstests/e2e/orm/client-api/delete-many.test.tstests/e2e/orm/client-api/find.test.tstests/e2e/orm/client-api/transaction.test.tstests/e2e/orm/client-api/aggregate.test.tstests/e2e/orm/client-api/upsert.test.tstests/e2e/orm/client-api/create.test.tstests/e2e/orm/client-api/group-by.test.tstests/e2e/orm/plugin-infra/on-query-hooks.test.tstests/e2e/orm/client-api/raw-query.test.tstests/e2e/orm/client-api/name-mapping.test.tstests/e2e/orm/policy/basic-schema-read.test.tstests/e2e/orm/plugin-infra/entity-mutation-hooks.test.tstests/e2e/orm/client-api/count.test.tstests/e2e/apps/rally/rally.test.tstests/e2e/orm/client-api/update-many.test.tstests/e2e/orm/client-api/create-many.test.tstests/e2e/orm/client-api/delete.test.tstests/e2e/orm/client-api/create-many-and-return.test.ts
🧠 Learnings (7)
📚 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/undefined-values.test.tstests/e2e/orm/client-api/update.test.tstests/e2e/orm/client-api/filter.test.tstests/e2e/tsconfig.jsontests/e2e/orm/client-api/delegate.test.tstests/e2e/orm/client-api/delete-many.test.tstests/e2e/orm/client-api/find.test.tstests/e2e/orm/client-api/transaction.test.tstests/e2e/orm/client-api/aggregate.test.tstests/e2e/orm/client-api/upsert.test.tstests/e2e/orm/client-api/create.test.tstests/e2e/orm/client-api/group-by.test.tstests/e2e/orm/plugin-infra/on-query-hooks.test.tstests/e2e/orm/client-api/raw-query.test.tstests/e2e/orm/client-api/name-mapping.test.tstests/e2e/orm/policy/basic-schema-read.test.tstests/e2e/orm/plugin-infra/entity-mutation-hooks.test.tstests/e2e/orm/client-api/count.test.tstests/e2e/apps/rally/rally.test.tstests/e2e/orm/client-api/update-many.test.tstests/e2e/orm/client-api/create-many.test.tstests/e2e/orm/client-api/delete.test.tstests/e2e/orm/client-api/create-many-and-return.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/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/client-api/undefined-values.test.tstests/e2e/orm/client-api/update.test.tstests/e2e/orm/client-api/filter.test.tstests/e2e/orm/client-api/delegate.test.tstests/e2e/orm/client-api/delete-many.test.tstests/e2e/orm/client-api/find.test.tstests/e2e/orm/client-api/transaction.test.tstests/e2e/orm/client-api/aggregate.test.tstests/e2e/orm/client-api/upsert.test.tstests/e2e/orm/client-api/create.test.tstests/e2e/orm/client-api/group-by.test.tstests/e2e/orm/plugin-infra/on-query-hooks.test.tstests/e2e/orm/client-api/raw-query.test.tstests/e2e/orm/client-api/name-mapping.test.tstests/e2e/orm/policy/basic-schema-read.test.tstests/e2e/orm/plugin-infra/entity-mutation-hooks.test.tstests/e2e/orm/client-api/count.test.tstests/e2e/apps/rally/rally.test.tstests/e2e/orm/client-api/update-many.test.tstests/e2e/orm/client-api/create-many.test.tstests/e2e/orm/client-api/delete.test.tstests/e2e/orm/client-api/create-many-and-return.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/undefined-values.test.tstests/e2e/orm/client-api/filter.test.tstests/e2e/orm/client-api/delegate.test.tstests/e2e/orm/client-api/delete-many.test.tstests/e2e/orm/client-api/find.test.tstests/e2e/orm/client-api/transaction.test.tstests/e2e/orm/client-api/upsert.test.tstests/e2e/orm/plugin-infra/on-query-hooks.test.tstests/e2e/orm/client-api/raw-query.test.tstests/e2e/orm/client-api/name-mapping.test.tstests/e2e/orm/policy/basic-schema-read.test.tstests/e2e/orm/plugin-infra/entity-mutation-hooks.test.tstests/e2e/orm/client-api/create-many.test.tstests/e2e/orm/client-api/delete.test.tstests/e2e/orm/client-api/create-many-and-return.test.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:
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/tsconfig.jsontests/e2e/orm/client-api/upsert.test.tstests/e2e/orm/client-api/group-by.test.tstests/e2e/orm/plugin-infra/on-query-hooks.test.tstests/e2e/orm/client-api/raw-query.test.tstests/e2e/orm/client-api/name-mapping.test.tstests/e2e/orm/plugin-infra/entity-mutation-hooks.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} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
tests/e2e/orm/client-api/find.test.tstests/e2e/orm/plugin-infra/on-query-hooks.test.tstests/e2e/orm/client-api/raw-query.test.tstests/e2e/apps/rally/rally.test.tstests/e2e/orm/client-api/create-many-and-return.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 **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
tests/e2e/apps/rally/rally.test.ts
🧬 Code graph analysis (15)
tests/e2e/orm/client-api/undefined-values.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/delegate.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/delete-many.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/find.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/transaction.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/aggregate.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/upsert.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/create.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/group-by.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/plugin-infra/on-query-hooks.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/raw-query.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/apps/rally/rally.test.ts (4)
packages/orm/src/client/contract.ts (1)
ClientContract(63-197)tests/e2e/orm/schemas/name-mapping/schema.ts (2)
SchemaType(127-127)schema(126-126)tests/e2e/apps/rally/zenstack/schema.ts (2)
SchemaType(2527-2527)schema(2526-2526)packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/update-many.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/delete.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
tests/e2e/orm/client-api/create-many-and-return.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-247)
⏰ 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, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (20)
tests/e2e/tsconfig.json (1)
6-6: TypeScript config aligns with Node.js test environment.Adding "node" to compiler types is appropriate for e2e tests running in a Node.js environment, ensuring proper type inference for Node APIs and globals.
tests/e2e/orm/client-api/delete-many.test.ts (1)
9-11: Client initialization cast is acceptable for this e2e testCasting the created test client to
anyhere keeps runtime behavior unchanged and matches the pattern used across the suite; no issues from a test-behavior perspective.tests/e2e/orm/client-api/create-many.test.ts (1)
9-11: Relaxed typing in setup is fine hereUsing
(await createTestClient(schema)) as anyforclientis consistent with other e2e client-api tests and doesn’t affect the createMany behavior being exercised.tests/e2e/orm/client-api/update.test.ts (1)
10-12: Client setup change preserves update test behaviorThe cast to
anyduring client creation only relaxes typing; the extensive nested update tests still exercise the same runtime paths.tests/e2e/orm/client-api/aggregate.test.ts (1)
10-12: Type assertion in aggregate test setup is non-invasiveCasting the created client to
anyinbeforeEachdoesn’t change how aggregates are executed or verified; setup remains sound for these e2e checks.tests/e2e/orm/client-api/transaction.test.ts (1)
9-11: Casting the transaction test client toanyis acceptableThe new
as anyon the created client only affects typing; the interactive and batch$transactionflows still run exactly as before.tests/e2e/orm/client-api/count.test.ts (1)
9-11: Count test setup change is safeUsing
(await createTestClient(schema)) as anykeeps the count tests’ runtime behavior identical while only loosening compile-time typing.tests/e2e/orm/plugin-infra/on-query-hooks.test.ts (1)
9-11: Looser typing for_clientis fine for plugin e2e coverageCasting the created client to
anyhere doesn’t affect the onQuery hook behavior being tested; plugin interception and argument/result propagation remain the same.tests/e2e/orm/client-api/delete.test.ts (1)
9-11: Consistentas anycast for test client initializationThis mirrors the pattern used in other client-api tests: it relaxes the assignment typing to
ClientContract<typeof schema>without changing how the client behaves at runtime or what the tests exercise.tests/e2e/orm/client-api/upsert.test.ts (1)
9-11: Typing relaxation inbeforeEachlooks fineCasting the
createTestClient(schema)result toanyonly affects TypeScript’s view of the assignment and leaves the upsert flows and expectations unchanged.tests/e2e/orm/client-api/update-many.test.ts (1)
9-11: Safe to cast test client toanyfor setupThe updateMany tests still use
clientas aClientContract<typeof schema>; theas anyhere simply sidesteps assignment typing without reducing what the tests validate.tests/e2e/orm/client-api/find.test.ts (1)
10-12: Find tests’ client initialization change is purely type-levelUsing
(await createTestClient(schema)) as anykeeps the runtime client identical while resolving assignment typing; the rich find/filter scenarios in this file remain fully exercised.tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts (1)
10-12: Casting_clienttoanyin setup does not affect hook behaviorThe entity-mutation hook tests still exercise
_client.$useand transaction behavior against the same underlying client; the cast only relaxes the assignment’s static typing.tests/e2e/orm/client-api/filter.test.ts (1)
9-11: Filter tests’beforeEachcast is acceptableRelaxing the assignment to
client: ClientContract<typeof schema>viaas anykeeps the extensive filter scenarios intact and only adjusts TypeScript’s assignment checking.tests/e2e/orm/client-api/create.test.ts (1)
9-11: Create tests use the sameas anysetup pattern and remain validThis aligns with other client-api tests: the client instance is unchanged at runtime, and the tests continue to use the strongly typed
ClientContract<typeof schema>surface.tests/e2e/orm/client-api/name-mapping.test.ts (1)
11-14: LGTM: Consistent type cast patternThe explicit cast to
anyaligns with the broader test suite pattern for test client initialization. WhilecreateTestClientalready returnsPromise<any>, the explicit cast adds clarity and consistency across test files.tests/e2e/orm/policy/basic-schema-read.test.ts (1)
11-11: LGTM: Consistent type cast patternThe explicit cast to
anyaligns with the test suite standardization for client initialization.tests/e2e/orm/client-api/raw-query.test.ts (1)
10-10: LGTM: Consistent type cast patternThe explicit cast to
anymaintains consistency with the test suite's client initialization pattern.tests/e2e/orm/client-api/create-many-and-return.test.ts (2)
10-10: LGTM: Consistent type cast patternThe explicit cast to
anyaligns with the test suite standardization for client initialization.
62-76: LGTM: Improved variable scopingThe refactoring from reusing
let rto using distinctconst randconst r1is a positive change that:
- Replaces mutable
letwith immutableconst- Eliminates variable reuse, improving code clarity
- Makes test assertions more explicit by using distinct identifiers
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.