merge dev to main (v3.0.0-beta.28) #478
Conversation
* feat(orm): implement JSON null values and equality filter * Update packages/orm/src/client/crud/validator/index.ts Co-authored-by: Copilot <[email protected]> * Update packages/orm/src/utils/type-utils.ts Co-authored-by: Copilot <[email protected]> * address PR comments * speed up test type-checking --------- Co-authored-by: Copilot <[email protected]>
* chore(deps): bump next from 16.0.1 to 16.0.7 Bumps [next](https://github.com/vercel/next.js) from 16.0.1 to 16.0.7. - [Release notes](https://github.com/vercel/next.js/releases) - [Changelog](https://github.com/vercel/next.js/blob/canary/release.js) - [Commits](vercel/next.js@v16.0.1...v16.0.7) --- updated-dependencies: - dependency-name: next dependency-version: 16.0.7 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * chore(deps): bump next from 16.0.1 to 16.0.7 (#467) * Initial plan * chore: update pnpm-lock.yaml for next 16.0.7 Co-authored-by: ymc9 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: ymc9 <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Copilot <[email protected]>
* chore(deps): bump next from 16.0.1 to 16.0.7 Bumps [next](https://github.com/vercel/next.js) from 16.0.1 to 16.0.7. - [Release notes](https://github.com/vercel/next.js/releases) - [Changelog](https://github.com/vercel/next.js/blob/canary/release.js) - [Commits](vercel/next.js@v16.0.1...v16.0.7) --- updated-dependencies: - dependency-name: next dependency-version: 16.0.7 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * chore(deps): bump next from 16.0.1 to 16.0.7 (#467) * Initial plan * chore: update pnpm-lock.yaml for next 16.0.7 Co-authored-by: ymc9 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: ymc9 <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Copilot <[email protected]>
Co-authored-by: ymc9 <[email protected]>
* feat(orm): typed-json filters * address PR comments * remove unused code * update typing documentation
* fix(type-check): try improving type checking performance * update * update * update * revert to ZenStackPromise * update promise
…ayload (#477) * fix(orm): entity is not created when upserting with an empty update payload * fix tests * update test * updates * update test
|
Claude finished @ymc9's task —— View job Pull Request ReviewI've completed a comprehensive review of this v3.0.0-beta.28 merge PR. Here's my detailed feedback: Review Checklist:
✅ Code Quality & Best PracticesStrengths:
Areas for improvement:
🐛 Bug AnalysisFixed Issues:
Potential Issues Found:
⚡ Performance ConsiderationsPositive impacts:
Potential concerns:
🔒 Security AssessmentGood practices:
Recommendations:
🧪 Test Coverage AnalysisExcellent coverage added:
Test structure improvements:
📦 Schema Generation ChangesPositive updates:
🎯 Final Recommendations✅ Ready to MergeThis PR represents high-quality work with:
💡 Suggested Follow-ups (for future PRs):
🏆 Overall Assessment: APPROVED
This is a solid release that significantly enhances the ORM's JSON capabilities while fixing critical bugs. The code demonstrates mature software engineering practices and maintains the high quality standards of the ZenStack project. |
|
Warning Rate limit exceeded@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughIntroduces comprehensive typed JSON filtering (including DbNull/JsonNull/AnyNull), broad readonly-array typing, dialect-level JSON path/array/string filter implementations, refactors schema generation to class-based SchemaType instances, centralizes test-generation tooling, many generated type artifacts, and bumps versions to 3.0.0-beta.28. 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)
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 merges the development branch to main, releasing version 3.0.0-beta.28. The main changes include:
- Schema generation consolidation: Consolidates multiple test generation scripts into a single centralized script (
scripts/test-generate.ts) - Schema format migration: Migrates generated schema files from object literal pattern to class-based pattern (implements
SchemaDefinterface) - Test improvements: Adds comprehensive JSON filter tests, improves upsert tests, and updates policy test assertions to reflect more accurate behavior
Reviewed changes
Copilot reviewed 119 out of 124 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/runtimes/*/scripts/generate.ts | Removed local generation scripts in favor of centralized approach |
| tests/runtimes/*/package.json | Updated to use centralized test-generate.ts script |
| tests//schemas//schema.ts | Migrated from object literal to class-based schema pattern |
| tests/e2e/orm/client-api/json-filter.test.ts | New comprehensive test suite for JSON filtering functionality |
| tests/e2e/orm/client-api/upsert.test.ts | Added tests for empty update/create payloads |
| tests/e2e/orm/policy/*/test.ts | Updated test expectations to match actual filtering behavior |
| tests/e2e/github-repos/*/schema.zmodel | Removed generator blocks (prisma-client-js, etc.) |
| tests/e2e/github-repos/*/{models,input}.ts | New generated type definition files |
| tests/regression/tsconfig.json | Removed src/**/*.ts from include path |
| tests//schemas//schema.zmodel | Updated plugin provider paths to use relative paths |
All changes appear to be part of a coordinated release focusing on schema generation improvements and expanded test coverage. The codebase follows the established patterns documented in memories.
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: 16
♻️ Duplicate comments (3)
tests/e2e/orm/client-api/delegate.test.ts (1)
11-14: Same type safety concern as in raw-query.test.ts.This file exhibits the same
as anycast pattern that erodes type safety. Please see the comment ontests/e2e/orm/client-api/raw-query.test.tsline 10 for a detailed analysis.tests/e2e/orm/client-api/create.test.ts (1)
10-10: Type cast pattern consistent with other test files.This matches the pattern in group-by.test.ts and other e2e tests to explicitly loosen typing for test flexibility during the typed JSON support rollout.
tests/e2e/orm/client-api/count.test.ts (1)
10-10: Type cast pattern consistent with other test files.Same intentional typing relaxation as seen in group-by.test.ts and create.test.ts.
🧹 Nitpick comments (28)
packages/common-helpers/src/zip.ts (1)
4-9: Nice readonly-parameter tightening; consider avoiding the non-null assertions ifnoUncheckedIndexedAccessisn’t required here.The
readonlyinputs align with the repo-wide immutability push and keep runtime behavior identical. The!onarr1[i]/arr2[i]is safe due to the loop bound, but it does suppress potential type-safety ifnoUncheckedIndexedAccessis enabled—worth double-checking whether you want to rely on assertions here vs. a small helper/assert to keep the check explicit.tests/e2e/orm/plugin-infra/on-kysely-query.test.ts (1)
10-12: Avoidas anyhere; keep the test’s type-safety.Right now
_clientis declared asClientContract<typeof schema>(Line 8) but you’re assigningany(Line 11), which can mask API/type regressions in this suite.Suggested change:
- _client = (await createTestClient(schema)) as any; + _client = (await createTestClient(schema)) as unknown as ClientContract<typeof schema>;Optionally, consider a tiny helper (e.g.,
createTypedTestClient<typeof schema>(schema)) to centralize this pattern across e2e tests. Based on learnings, E2E tests should validate real-world schema compatibility with established projects; if this suite is meant to remain “infra-focused” with../schemas/basic, it may be worth confirming that expectation in a short comment or folder-level README.tests/e2e/orm/schemas/delegate/typecheck.ts (1)
54-64: Make the relation case narrowing both runtime-safe and type-narrowing-friendly (avoid optional-chain narrowing).
If the previous block was removed due to optional chaining / narrowing friction, prefer a local binding + null-check; it’s clearer and reliably exercises TS narrowing.Suggested change:
@@ - // TODO: discriminated sub-model fields - // if (r2.assets[0]?.assetType === 'Video') { - // // video - // console.log(r2.assets[0]?.duration); - // // only one choice `RatedVideo` - // console.log(r2.assets[0]?.rating); - // } else { - // // image - // console.log(r2.assets[0]?.format); - // } + // discriminated sub-model fields should be available after narrowing (relation case) + const a0 = r2.assets[0]; + if (a0) { + if (a0.assetType === 'Video') { + console.log(a0.duration); + console.log(a0.rating); + } else { + console.log(a0.format); + } + }tests/e2e/apps/rally/rally.test.ts (2)
8-8: Type safety concern: usinganyfor Options parameter weakens E2E validation.The second generic parameter of
ClientContractrepresents the options type, but usinganydisables type checking for all options-related operations on thedbclient. This undermines the E2E test's purpose of validating real-world schema compatibility.Consider using the actual options type if possible, or explicitly typing the options to maintain type safety in this compatibility test.
Based on coding guidelines: E2E tests should validate real-world schema compatibility with established projects.
Is the
anytype parameter necessary here? If this pattern is required across all E2E tests due to typing complexity increateTestClient, consider improving the return type ofcreateTestClientto preserve type safety.
11-22: Casting toanydefeats type safety in E2E test initialization.The explicit cast to
anybypasses TypeScript's type checking for the client initialization. SincecreateTestClientalready returnsPromise<any>(perpackages/testtools/src/client.ts), this cast should be unnecessary unless there's a type constraint issue.This reduces the effectiveness of E2E tests at catching real-world compatibility issues with the generated schema and client types.
Based on coding guidelines: E2E tests should validate real-world schema compatibility with established projects.
Can you verify if this cast is necessary? If so, consider improving
createTestClientto return properly typed clients (e.g.,ClientContract<Schema, ClientOptions<Schema>>) to maintain type safety across E2E tests. The AI summary indicates this pattern is used across multiple tests, suggesting a systemic typing issue that would be better addressed at the source.README.md (1)
28-29: Consider using markdown link format.The bare URL works but using markdown link syntax (
[text](url)) would be more consistent with markdown best practices and improve accessibility.Apply this diff:
-> Read full documentation at 👉🏻 https://zenstack.dev/v3. +> Read full documentation at 👉🏻 [https://zenstack.dev/v3](https://zenstack.dev/v3).tests/e2e/orm/client-api/create-many.test.ts (1)
7-11: Prefer a narrow cast (or no cast) overas anyin test setup.
If you need a cast here, consider casting to the intended contract type instead ofanyto keep IntelliSense/type safety for the rest of the test.- client = (await createTestClient(schema)) as any; + client = (await createTestClient(schema)) as ClientContract<typeof schema>;tests/e2e/orm/client-api/name-mapping.test.ts (1)
8-15: Prefer narrowing the cast instead ofas anyfordb.
This keeps the test type-checked while still allowing you to override an inference issue at the boundary.- db = (await createTestClient(schema, { + db = (await createTestClient(schema, { usePrismaPush: true, schemaFile: path.join(__dirname, '../schemas/name-mapping/schema.zmodel'), - })) as any; + })) as ClientContract<SchemaType>;tests/e2e/orm/policy/migrated/nested-to-one.test.ts (1)
187-200: Good: asserts state preservation after filtered nested update.
Optional: make the follow-up read deterministic (e.g.,findUnique({ where: { id: '1' } })) instead offindFirst()with no filter. Based on learnings, policy tests should be robust and explicit.tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts (1)
8-12: Avoidas anyfor_clientunless it’s strictly necessary.
If a cast is needed, prefer the narrow type you already declared:- _client = (await createTestClient(schema, {})) as any; + _client = (await createTestClient(schema, {})) as ClientContract<typeof schema>;packages/orm/src/utils/object-utils.ts (1)
4-6: Readonly parameter change is solid; consider Set-based lookup if this is hot.
IfextractFieldsis used on large objects/field lists,fields.includesin a loop is quadratic;const s = new Set(fields)would make it linear.packages/orm/src/client/crud-types.ts (1)
60-82: Consider removing commented delegate model code or adding issue reference.The delegate model discriminated union result logic is commented out with a TODO. For maintainability, consider either:
- Removing the commented code entirely and tracking in an issue
- Adding an issue reference to the TODO comment
> = WrapType< { [Key in NonRelationFields<Schema, Model> as ShouldOmitField<Schema, Model, Options, Key, Omit> extends true ? never : Key]: MapModelFieldType<Schema, Model, Key>; }, - // TODO: revisit how to efficiently implement discriminated sub model types - // IsDelegateModel<Schema, Model> extends true - // ? // delegate model's selection result is a union of all sub-models - // DelegateUnionResult<Schema, Model, Options, GetSubModels<Schema, Model>, Omit> - // : { - // [Key in NonRelationFields<Schema, Model> as ShouldOmitField< - // Schema, - // Model, - // Options, - // Key, - // Omit - // > extends true - // ? never - // : Key]: MapModelFieldType<Schema, Model, Key>; - // }, + // TODO: revisit how to efficiently implement discriminated sub model types (tracked in issue #XXX) Optional, Array >;tests/e2e/orm/client-api/aggregate.test.ts (1)
11-11: Type assertion toanybypasses type safety.The explicit cast to
anydefeats the purpose of theClientContract<typeof schema>type declaration on line 8. This pattern repeats across many test files in this PR, suggesting a temporary accommodation during schema system refactoring.Consider addressing the type mismatch by either:
- Updating
createTestClientto returnClientContract<SchemaDef>instead ofany- Adjusting the client type declaration to match the actual return type
- Using a more specific type assertion if the types are compatible but not recognized by the compiler
Since this is test code and the pattern is consistent across the test suite, this can be deferred but should be tracked for cleanup post-refactor.
tests/e2e/orm/client-api/delete-many.test.ts (1)
10-10: Type assertion toanybypasses type safety.The explicit cast to
anydefeats the purpose of theClientContract<typeof schema>type declaration on line 7. This pattern appears across multiple test files in this PR.Consider addressing the type mismatch by updating
createTestClientto return the correct type or adjusting the client type declaration. Since this is test code and the pattern is consistent across the suite, this can be deferred but should be tracked for cleanup.tests/e2e/orm/client-api/update.test.ts (1)
11-11: Type assertion toanybypasses type safety.The explicit cast to
anydefeats the purpose of theClientContract<typeof schema>type declaration on line 8. This pattern repeats across many test files in this PR.Consider addressing the type mismatch by updating
createTestClientto return the correct type or adjusting the client type declaration. Since this is test code and the pattern is consistent across the suite, this can be deferred but should be tracked for cleanup.tests/e2e/orm/policy/basic-schema-read.test.ts (1)
11-11: Type assertion toanybypasses type safety.The explicit cast to
anydefeats the purpose of theClientContract<typeof schema>type declaration on line 8. This pattern appears across multiple test files in this PR.Consider addressing the type mismatch by updating
createTestClientto return the correct type or adjusting the client type declaration. Since this is test code and the pattern is consistent across the suite, this can be deferred but should be tracked for cleanup.tests/e2e/orm/client-api/find.test.ts (1)
11-11: Type assertion toanybypasses type safety.The explicit cast to
anydefeats the purpose of theClientContract<typeof schema>type declaration on line 8. This pattern repeats across many test files in this PR.Consider addressing the type mismatch by updating
createTestClientto return the correct type or adjusting the client type declaration. Since this is test code and the pattern is consistent across the suite, this can be deferred but should be tracked for cleanup.tests/e2e/orm/client-api/delete.test.ts (1)
10-10: Type assertion toanybypasses type safety.The explicit cast to
anydefeats the purpose of theClientContract<typeof schema>type declaration on line 7. This pattern appears across multiple test files in this PR.Consider addressing the type mismatch by updating
createTestClientto return the correct type or adjusting the client type declaration. Since this is test code and the pattern is consistent across the suite, this can be deferred but should be tracked for cleanup.tests/e2e/orm/client-api/filter.test.ts (1)
10-10: Type assertion toanybypasses type safety.The explicit cast to
anydefeats the purpose of theClientContract<typeof schema>type declaration on line 7. This pattern repeats across many test files in this PR.Consider addressing the type mismatch by updating
createTestClientto return the correct type or adjusting the client type declaration. Since this is test code and the pattern is consistent across the suite, this can be deferred but should be tracked for cleanup.tests/e2e/orm/client-api/undefined-values.test.ts (1)
11-11: Type assertion toanybypasses type safety.The explicit cast to
anydefeats the purpose of theClientContract<typeof schema>type declaration on line 8. This pattern appears across multiple test files in this PR.Consider addressing the type mismatch by updating
createTestClientto return the correct type or adjusting the client type declaration. Since this is test code and the pattern is consistent across the suite, this can be deferred but should be tracked for cleanup.TODO.md (2)
43-43: Consider fixing list indentation.The markdown linter flags inconsistent indentation (expected 6 spaces, actual 12). While this doesn't affect functionality, consistent formatting improves readability.
Apply this diff to align with markdown conventions:
- - [x] JSON filtering + - [x] JSON filtering
89-89: Consider fixing list indentation.The markdown linter flags inconsistent indentation (expected 4 spaces, actual 8).
Apply this diff to align with markdown conventions:
- - [x] DbNull vs JsonNull + - [x] DbNull vs JsonNulltests/e2e/orm/client-api/json-filter.test.ts (1)
367-368: Clarify intent: Unawaited promise for type-check only.This statement lacks
awaitand its result is unused. While the@ts-expect-errorsuggests this is intentional type checking, consider adding a comment to clarify intent, or useawait expect(...).rejectspattern if you also want to verify runtime behavior.// @ts-expect-error - db.foo.findFirst({ where: { data: { path: '$.a', equals: null } } }); + // Type-check only: verifying that `null` is not allowed as equals value + db.foo.findFirst({ where: { data: { path: '$.a', equals: null } } });packages/orm/src/common-types.ts (1)
6-25: Well-implemented branded sentinel types for JSON null handling.The branding pattern using private
__brandfields effectively creates distinct nominal types forDbNull,JsonNull, andAnyNull. This mirrors Prisma's approach and ensures type-safe null handling in JSON operations.Consider using
declare private __brandinstead of the@ts-ignoresuppression, which avoids runtime overhead and is more idiomatic for branding:export class DbNullClass { - // @ts-ignore - private __brand = 'DbNull' as const; + declare private __brand: 'DbNull'; }This is a minor stylistic preference and the current implementation is functionally correct.
samples/orm/zenstack/schema.ts (1)
8-13: Consider making generated schema fieldsreadonlyto preserve immutability expectations
The new class-based schema shape is consistent with other generated schemas, butprovider/models/typeDefs/enums/authType/pluginsremain mutable at runtime. If any consumer mutates these, it can create very hard-to-debug behavior differences vs the priorconstobject export.Proposed generator-side tweak (illustrative):
-export class SchemaType implements SchemaDef { - provider = { type: "sqlite" } as const; - models = { ... } as const; +export class SchemaType implements SchemaDef { + readonly provider = { type: "sqlite" } as const; + readonly models = { ... } as const; ... - plugins = {}; + readonly plugins = {}; }Also applies to: 207-244
tests/e2e/github-repos/formbricks/models.ts (2)
10-412: Generated model JSDoc can drift from the schema
Given this is auto-generated, consider either (a) generating JSDoc directly from schema docs, or (b) omitting verbose per-model field docs to avoid stale/misleading comments.
413-468: Remove duplicate JSDoc and fixdisplayOptionsnaming inconsistencyThe union extraction pattern
(typeof Enum)[keyof typeof Enum]is correct as-is—$schema.enums.X.valuesis an object-map with string keys and values, making the extraction pattern valid.However, two minor generator inconsistencies should be addressed:
displayOptionsis lowercase (lines 427–428) while all other enums use PascalCase; should beDisplayOptionsApiKeyPermissionhas duplicated JSDoc (lines 443–452); remove the redundant comment before the type declarationtests/e2e/github-repos/trigger.dev/input.ts (1)
8-10: Consider combining the twoimport typestatements from@zenstackhq/orm(generator polish)
Not required for correctness, but reduces generated diff churn and keeps output consistent.
* fix(orm): escape special characters in string search patterns * fix tests
|
Claude finished @ymc9's task —— View job Pull Request Review - v3.0.0-beta.28I've completed a comprehensive review of this v3.0.0-beta.28 merge PR. Here's my detailed feedback: Review Checklist:
✅ Code Quality & Best PracticesStrengths:
Areas for improvement:
🐛 Bug AnalysisFixed Issues:
Potential Issues Found:
⚡ Performance ConsiderationsPositive impacts:
Potential concerns:
🔒 Security AssessmentGood practices:
Recommendations:
🧪 Test Coverage AnalysisExcellent coverage added:
Test structure improvements:
📦 Schema Generation ChangesPositive updates:
🎯 Final Recommendations✅ Ready to MergeThis PR represents high-quality work with:
💡 Suggested Follow-ups (for future PRs):
🏆 Overall Assessment: APPROVED
This is a solid release that significantly enhances the ORM's JSON capabilities while fixing critical bugs. The code demonstrates mature software engineering practices and maintains the high quality standards of the ZenStack project. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/orm/src/client/crud/dialects/base-dialect.ts (2)
614-651: Don’t useinvariant(false, ...)for invalid typed-JSON array filter keys — throwcreateInvalidInputErrorinstead.default: - invariant(false, `Invalid typed JSON array filter key: ${key}`); + throw createInvalidInputError(`Invalid typed JSON array filter key: ${key}`);
677-697: Bug: primitive-array fields inside TypeDefs are treated as SQL arrays (@>,&&) — butfieldReceiveris JSON-path extracted data.Today this does:
- Line 681-686:
fieldReceiver = buildJsonPathSelection(receiver, '$.<key>')- Line 685-687:
buildArrayFilter(fieldReceiver, ...)→ SQL array opsThis will generate invalid/meaningless SQL for JSON-stored arrays. Please route primitive-array filters under TypeDefs through JSON-array logic (likely via
buildJsonArrayExistsPredicate/buildJsonArrayFilter) instead ofbuildArrayFilter.Prisma JSON filters: what are the exact semantics and expected input shapes for `array_contains`, `array_starts_with`, `array_ends_with` on PostgreSQL/SQLite?packages/orm/src/client/crud/dialects/sqlite.ts (1)
371-385: Usejson_each(notjsonb_each) forarray_containsto match the rest of the file / broader SQLite 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})`)Also: please confirm whether
array_*filters expect scalar or array inputs (and whether starts/ends mean “first/last element equals” vs “prefix/suffix matches”).SQLite JSON1: behavior of `->`, `json_extract`, `json_each` with arrays; and Prisma JSON `array_*` filter semantics on SQLite.
🧹 Nitpick comments (5)
tests/e2e/orm/policy/policy-functions.test.ts (1)
163-177: Good regression test for%-escaping instartsWith; consider expanding to_as well.
The expectations look correct for catching wildcard mishandling. For completeness across dialects, add a case where the input contains_(and verify it’s treated literally).packages/orm/src/client/functions.ts (2)
56-63: Good LIKE wildcard escaping, butCAST(... as text)may be dialect-specific.Escaping
\,%,_at the SQL level is solid. Please confirm this works on all supported dialects in this repo; if MySQL (or others) are supported, consider moving “cast-to-string + escape-like-pattern” behind the dialect layer to avoid... as textportability issues.
65-65:ESCAPE '\\'clause: please validate on each supported DB + add regression coverage.This is the right direction, but it’s easy to regress across engines—please run/extend tests to include searches containing
%,_, and\and ensureLIKE/ILIKE ... ESCAPEis accepted for each dialect you ship.packages/orm/src/client/crud/dialects/base-dialect.ts (1)
548-557: Prefer INVALID_INPUT errors overinvariantfor user-provided JSON filtermode(to keep error surfaces consistent).- const mode = filter.mode ?? 'default'; - invariant(mode === 'default' || mode === 'insensitive', 'Invalid JSON filter mode'); + const mode = filter.mode ?? 'default'; + if (mode !== 'default' && mode !== 'insensitive') { + throw createInvalidInputError(`Invalid JSON filter mode: ${mode}`); + }packages/orm/src/client/crud/dialects/postgresql.ts (1)
46-53:AnyNullClassshould be surfaced as INVALID_INPUT (not an invariant crash) if it can originate from user input paths.Consider swapping the invariant for
createInvalidInputError(...)(and importing it) to keep error handling consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/language/package.json(2 hunks)packages/orm/src/client/crud/dialects/base-dialect.ts(6 hunks)packages/orm/src/client/crud/dialects/postgresql.ts(3 hunks)packages/orm/src/client/crud/dialects/sqlite.ts(3 hunks)packages/orm/src/client/functions.ts(2 hunks)tests/e2e/orm/client-api/filter.test.ts(9 hunks)tests/e2e/orm/client-api/json-filter.test.ts(1 hunks)tests/e2e/orm/policy/policy-functions.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/orm/client-api/filter.test.ts
- tests/e2e/orm/client-api/json-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/policy/policy-functions.test.ts
🧠 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:
tests/e2e/orm/policy/policy-functions.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/policy/policy-functions.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:
packages/orm/src/client/functions.tspackages/orm/src/client/crud/dialects/sqlite.tspackages/orm/src/client/crud/dialects/postgresql.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} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/orm/src/client/functions.tspackages/orm/src/client/crud/dialects/postgresql.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/functions.tspackages/language/package.json
📚 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:
packages/language/package.json
📚 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-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/dialects/base-dialect.ts
🧬 Code graph analysis (2)
tests/e2e/orm/policy/policy-functions.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(258-269)
packages/orm/src/client/crud/dialects/base-dialect.ts (4)
packages/orm/src/client/query-utils.ts (2)
isTypeDef(201-203)requireTypeDef(40-46)packages/schema/src/schema.ts (2)
BuiltinType(88-98)FieldDef(62-78)packages/orm/src/client/errors.ts (1)
createInvalidInputError(116-120)packages/orm/src/common-types.ts (3)
DbNullClass(6-9)JsonNullClass(13-16)AnyNullClass(20-23)
⏰ 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, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (5)
packages/language/package.json (1)
70-70: Verify [email protected] Node version compatibility and pinning location.The glob dependency has been updated to ^11.1.0 (line 70) and the Volta block has been removed. However, [email protected] requires Node 20 or >=22. Removing the Volta block without establishing Node version constraints elsewhere could allow developers or CI/CD to use incompatible Node versions (e.g., Node 18).
Additionally, per the past review, [email protected] may not be a published version—the latest glob 11.x is typically 11.0.3 with Node requirements of "20 || >=22".
Please verify the following:
- Confirm the glob version is correct — Is [email protected] actually published? If not, revert to a published version (e.g., ^11.0.3) or a version supporting Node 18.
- Verify Node version pinning — Where is the Node version constraint now defined? (root package.json, .nvmrc, CI workflows, etc.)
- Ensure monorepo-wide consistency — Are all other packages in the monorepo also compatible with the same Node version?
#!/bin/bash # Description: Check glob version in lockfile and verify Node version pinning across monorepo echo "=== Checking glob version in pnpm-lock.yaml ===" if [ -f "pnpm-lock.yaml" ]; then rg "glob@" pnpm-lock.yaml -A 3 | head -20 else echo "pnpm-lock.yaml not found" fi echo "" echo "=== Checking for Node version constraints in root package.json ===" if [ -f "package.json" ]; then cat package.json | jq '.engines // .volta // "No engines/volta found"' fi echo "" echo "=== Checking for .nvmrc or .node-version files ===" if [ -f ".nvmrc" ]; then echo ".nvmrc: $(cat .nvmrc)" elif [ -f ".node-version" ]; then echo ".node-version: $(cat .node-version)" else echo "No .nvmrc or .node-version file found" fiAlso applies to: 4-4
packages/orm/src/client/functions.ts (1)
2-2: ConfirmSqlBoolexists in your pinned Kysely version.If the repo’s Kysely version doesn’t export
SqlBool, this will break typechecking; please verify against your lockfile/installed version.packages/orm/src/client/crud/dialects/base-dialect.ts (1)
505-522: JSON/TypeDef dispatch inbuildPrimitiveFilterlooks right — routing TypeDef fields tobuildJsonFilterkeeps scalar/enum behavior intact.packages/orm/src/client/crud/dialects/sqlite.ts (2)
38-67: JSON/TypeDef stringification intransformPrimitiveis consistent with JSON-as-text storage (and aligns with the->JSON operator usage below).
387-397:buildJsonArrayExistsPredicateusingjson_each+ EXISTS is a solid base.
* fix(orm): "array_contains" filter fixes * update
|
Claude finished @ymc9's task —— View job Pull Request Review - v3.0.0-beta.28I've completed a comprehensive review of this v3.0.0-beta.28 merge PR. Here's my detailed feedback: Review Checklist:
✅ Code Quality & Best PracticesStrengths:
Areas for improvement:
🐛 Bug AnalysisFixed Issues:
Potential Issues Found:
⚡ Performance ConsiderationsPositive impacts:
Potential concerns:
🔒 Security AssessmentGood practices:
Recommendations:
🧪 Test Coverage AnalysisExcellent coverage added:
Test structure improvements:
📦 Schema Generation ChangesPositive updates:
🎯 Final Recommendations✅ Ready to MergeThis PR represents high-quality work with:
💡 Suggested Follow-ups (for future PRs):
🏆 Overall Assessment: APPROVED
This is a solid release that significantly enhances the ORM's JSON capabilities while fixing critical bugs. The code demonstrates mature software engineering practices and maintains the high quality standards of the ZenStack project. |
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.