merge dev to main (v3.0.0-beta.12) #327
Conversation
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
WalkthroughThis pull request bumps the package version from 3.0.0-beta.11 to 3.0.0-beta.12 across multiple packages in the monorepo. It also enhances CRUD operations with configurable return fields for mutations and adjusts the transaction isolation level for query execution from RepeatableRead to ReadCommitted. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR contains a large cohort of repetitive, trivial version bumps across 18 package.json files (requiring minimal review), but is offset by moderate-complexity CRUD operation changes affecting multiple files with new conditional logic, helper methods, and transaction handling adjustments. The heterogeneous nature of changes—combining homogeneous version updates with dense logic modifications in base, create, update, and delete handlers—warrants a moderate complexity assessment. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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 Code Review AnalysisReviewing PR #327 (dev -> main) for v3.0.0-beta.12 release. Analyzed changes across 27 files focusing on code quality, performance, security, and testing. Todo List
🔍 Code Review Summary✅ Strengths🚀 Performance Improvements
🧹 Code Quality
✅ Test Coverage
|
There was a problem hiding this comment.
Pull Request Overview
This PR merges the dev branch to main for release v3.0.0-beta.12. The changes include version bumps across all packages and significant optimizations to database operations by leveraging SQL RETURNING clauses to avoid unnecessary read-back queries when possible.
- Version bump from 3.0.0-beta.11 to 3.0.0-beta.12 across all packages
- Transaction isolation level changed from RepeatableRead to ReadCommitted
- CRUD operations optimized to use SQL RETURNING instead of separate read queries when appropriate
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Root package version bump |
| packages/cli/package.json | CLI package version bump |
| packages/common-helpers/package.json | Common helpers package version bump |
| packages/config/eslint-config/package.json | ESLint config package version bump |
| packages/config/typescript-config/package.json | TypeScript config package version bump |
| packages/config/vitest-config/package.json | Vitest config package version bump |
| packages/create-zenstack/package.json | Create-zenstack package version bump |
| packages/dialects/sql.js/package.json | SQL.js dialect package version bump |
| packages/language/package.json | Language package version bump |
| packages/plugins/policy/package.json | Policy plugin package version bump |
| packages/runtime/package.json | Runtime package version bump |
| packages/runtime/src/client/crud/operations/base.ts | Core CRUD optimization logic with mutationNeedsReadBack helper and returning clause support |
| packages/runtime/src/client/crud/operations/create.ts | Create operations updated to use RETURNING optimization |
| packages/runtime/src/client/crud/operations/delete.ts | Delete operations refactored to use RETURNING and avoid pre-delete reads |
| packages/runtime/src/client/crud/operations/update.ts | Update operations optimized with needReadBack analysis and RETURNING support |
| packages/runtime/src/client/executor/zenstack-query-executor.ts | Transaction isolation level changed to ReadCommitted |
| packages/sdk/package.json | SDK package version bump |
| packages/tanstack-query/package.json | TanStack Query package version bump |
| packages/testtools/package.json | Test tools package version bump |
| packages/zod/package.json | Zod package version bump |
| samples/blog/package.json | Blog sample version bump |
| tests/e2e/orm/client-api/create-many-and-return.test.ts | Import order reorganization |
| tests/e2e/orm/client-api/create.test.ts | Import order reorganization |
| tests/e2e/orm/client-api/update-many.test.ts | Test updated to verify select behavior and import order reorganization |
| tests/e2e/orm/client-api/upsert.test.ts | Test updated to verify select/omit behavior and import order reorganization |
| tests/e2e/package.json | E2E tests package version bump |
| tests/regression/package.json | Regression tests package version bump |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/runtime/src/client/crud/operations/update.ts (1)
133-147: Ensure IDs are returned for upsert read-backCreation path uses selectedFields; if it omits IDs, getIdValues cannot build a unique filter.
- const { needReadBack, selectedFields } = this.needReadBack(args); + const { needReadBack, selectedFields } = this.needReadBack(args); + const selectedForMutation = + needReadBack ? this.ensureIdsSelected(this.model, selectedFields) : selectedFields; @@ - let mutationResult: unknown = await this.update( + let mutationResult: unknown = await this.update( tx, this.model, args.where, args.update, undefined, true, false, - selectedFields, + selectedForMutation, ); @@ - mutationResult = await this.create(tx, this.model, args.create, undefined, undefined, selectedFields); + mutationResult = await this.create( + tx, + this.model, + args.create, + undefined, + undefined, + selectedForMutation + );Also applies to: 149-167
packages/runtime/src/client/crud/operations/base.ts (1)
2067-2067: I need to investigate further to determine if this is actually a breaking change and whether tests exist for transaction isolation behavior.Now let me search the codebase for tests related to transaction isolation and concurrency:
Based on my verification, I've found critical information that confirms the review comment's concerns. ZenStack's documentation explicitly states that "the database's transaction isolation level should be set to at least Repeatable Read" to ensure access policies are properly enforced.
The code change at line 2086 sets the default to
ReadCommittedwhen no isolation level is specified. This creates a breaking change:
MySQL InnoDB's default is REPEATABLE READ, while PostgreSQL's default is Read Committed, meaning the new default creates dialect-specific behavior differences.
More critically, to ensure access policies are properly enforced, the isolation level should be at least Repeatable Read, which can be done via the isolationLevel option when creating PrismaClient.
Cross-dialect isolation level default changed to ReadCommitted — violates access policy enforcement requirements
ZenStack's access policies require at least Repeatable Read isolation to function correctly. Setting ReadCommitted as the default bypasses this requirement and could leave policies unenforced, especially for MySQL users accustomed to REPEATABLE READ. Verify that:
- All policy-enforcement tests pass under ReadCommitted isolation
- Documentation is updated if this represents an intentional policy change
- Developers explicitly override to RepeatableRead where policies are active
🧹 Nitpick comments (5)
packages/runtime/src/client/crud/operations/create.ts (1)
84-87: Avoidas anyin OR filterDefine a WhereUnique type or reuse an existing one so
OR: createResult.map(getIdValues(...))is properly typed.tests/e2e/orm/client-api/upsert.test.ts (1)
38-47: LGTM: upsert result shaping viaselectis correctAssertions reflect narrowed payload and final full read-back. Consider a negative test to ensure
selectandincludeare not accepted together.Also applies to: 52-53, 69-69
packages/runtime/src/client/crud/operations/delete.ts (1)
25-41: Add documentation clarifying policy-blocked read-back behaviorThe transaction executes delete, then attempts to read back the result. If policy blocks the read, the record remains deleted and error is thrown post-transaction. This pattern mirrors
update.ts(line ~63), which includes an inline comment explaining this edge case. Add similar documentation todelete.ts(around lines 42–47) to clarify this is intentional: "delete succeeded but result cannot be read back due to policy".packages/runtime/src/client/crud/operations/update.ts (1)
181-206: Consider centralizing ID-selection pattern into BaseOperationHandlerBoth
create.tsandupdate.tsduplicate the pattern of mappinggetIdValues()across result arrays in their*ManyAndReturnmethods. Moving this into a reusable helper on BaseOperationHandler would reduce duplication and prevent future regressions across operations.packages/runtime/src/client/crud/operations/base.ts (1)
246-253: Unify parameter naming:returnFieldsvsfieldsToReturnUse a single name across CRUD methods (prefer
fieldsToReturn) for consistency and easier API comprehension.Also applies to: 669-670, 908-909, 1214-1215, 1945-1947
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
package.json(1 hunks)packages/cli/package.json(1 hunks)packages/common-helpers/package.json(1 hunks)packages/config/eslint-config/package.json(1 hunks)packages/config/typescript-config/package.json(1 hunks)packages/config/vitest-config/package.json(1 hunks)packages/create-zenstack/package.json(1 hunks)packages/dialects/sql.js/package.json(1 hunks)packages/language/package.json(1 hunks)packages/plugins/policy/package.json(1 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/client/crud/operations/base.ts(16 hunks)packages/runtime/src/client/crud/operations/create.ts(2 hunks)packages/runtime/src/client/crud/operations/delete.ts(1 hunks)packages/runtime/src/client/crud/operations/update.ts(6 hunks)packages/runtime/src/client/executor/zenstack-query-executor.ts(1 hunks)packages/sdk/package.json(1 hunks)packages/tanstack-query/package.json(1 hunks)packages/testtools/package.json(1 hunks)packages/zod/package.json(1 hunks)samples/blog/package.json(1 hunks)tests/e2e/orm/client-api/create-many-and-return.test.ts(1 hunks)tests/e2e/orm/client-api/create.test.ts(1 hunks)tests/e2e/orm/client-api/update-many.test.ts(2 hunks)tests/e2e/orm/client-api/upsert.test.ts(3 hunks)tests/e2e/package.json(1 hunks)tests/regression/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/dialects/sql.js/package.jsonpackages/config/vitest-config/package.jsonpackages/zod/package.jsonpackages/language/package.jsonpackages/runtime/src/client/executor/zenstack-query-executor.tstests/e2e/orm/client-api/create.test.tstests/e2e/orm/client-api/create-many-and-return.test.tssamples/blog/package.jsonpackages/common-helpers/package.jsontests/regression/package.jsonpackages/plugins/policy/package.jsonpackages/create-zenstack/package.jsonpackages/testtools/package.jsontests/e2e/orm/client-api/update-many.test.tstests/e2e/package.jsonpackages/runtime/src/client/crud/operations/update.tspackages/cli/package.jsonpackages/config/eslint-config/package.jsonpackages/runtime/src/client/crud/operations/create.tspackages/runtime/src/client/crud/operations/delete.tspackages/config/typescript-config/package.jsonpackages/runtime/package.jsonpackages/tanstack-query/package.jsonpackages/runtime/src/client/crud/operations/base.tspackages/sdk/package.jsontests/e2e/orm/client-api/upsert.test.ts
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/orm/client-api/create.test.tstests/e2e/orm/client-api/create-many-and-return.test.tstests/e2e/orm/client-api/update-many.test.tstests/e2e/package.jsontests/e2e/orm/client-api/upsert.test.ts
package.json
📄 CodeRabbit inference engine (CLAUDE.md)
Pin the repository package manager to
pnpm@10.12.1via thepackageManagerfield
Files:
package.json
🧬 Code graph analysis (4)
packages/runtime/src/client/crud/operations/update.ts (3)
packages/runtime/src/client/crud-types.ts (1)
WhereInput(207-243)packages/sdk/src/schema/schema.ts (1)
GetModels(109-109)packages/runtime/src/client/query-utils.ts (1)
getIdValues(173-179)
packages/runtime/src/client/crud/operations/create.ts (3)
packages/runtime/src/client/crud/operations/update.ts (1)
needReadBack(181-206)packages/runtime/src/client/query-utils.ts (1)
getIdValues(173-179)packages/runtime/src/client/crud-types.ts (1)
WhereInput(207-243)
packages/runtime/src/client/crud/operations/delete.ts (3)
packages/runtime/src/client/crud/operations/update.ts (1)
needReadBack(181-206)packages/runtime/src/client/errors.ts (2)
NotFoundError(32-36)RejectedByPolicyError(61-69)packages/runtime/src/client/crud-types.ts (1)
DeleteManyArgs(968-971)
packages/runtime/src/client/crud/operations/base.ts (1)
packages/runtime/src/client/query-utils.ts (1)
requireIdFields(70-77)
⏰ 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: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (27)
tests/e2e/orm/client-api/create-many-and-return.test.ts (1)
3-4: LGTM! Import reordering improves code organization.The import statements have been reordered to group vitest imports together and separate external dependencies from local imports, following common conventions. This is a minor readability improvement with no functional impact.
tests/e2e/orm/client-api/update-many.test.ts (2)
3-4: LGTM: Import additions are correct.The
afterEachimport and schema import are properly added and utilized in the test file.
90-105: LGTM: Improved async test pattern.The refactoring to use
expect(...).resolves.toMatchObjectfor the first assertion is a better practice for async testing, making the test more idiomatic and readable. The variable rename tor1maintains clarity, and the test properly validates both full and selective field returns with theupdateManyAndReturnoperation.packages/tanstack-query/package.json (1)
3-3: Version bump looks good.The version increment from 3.0.0-beta.11 to 3.0.0-beta.12 aligns with the PR objectives for this release.
tests/e2e/package.json (1)
3-3: Version bump looks good.The version increment aligns with the PR objectives for this release.
packages/plugins/policy/package.json (1)
3-3: Version bump looks good.The version increment aligns with the PR objectives for this release.
tests/regression/package.json (1)
3-3: Version bump looks good.The version increment aligns with the PR objectives for this release.
packages/runtime/package.json (1)
3-3: Version bump looks good.The version increment aligns with the PR objectives for this release.
packages/sdk/package.json (1)
3-3: Version bump looks good.The version increment aligns with the PR objectives for this release.
package.json (1)
3-3: Version bump looks good.The version increment aligns with the PR objectives for this release. The
packageManagerfield correctly pins topnpm@10.12.1as per coding guidelines.packages/runtime/src/client/executor/zenstack-query-executor.ts (1)
92-94: Verified: This is an intentional performance optimization.The change from
RepeatableReadtoReadCommittedwas made in commit 68e0072 (Oct 22, 2025) with the explicit message "fix: use a weaker tx consistency level for better perf (#326)". This aligns with the pre-existing default inbase.ts(line 2086), which already usedReadCommitted.The mutation patterns in the codebase (single-record create/update/delete operations) do not require
RepeatableReadguarantees, and hooks that load entities do so within the same transaction context. Existing tests validate that mutations and entity hooks continue to work correctly with this isolation level.packages/config/vitest-config/package.json (1)
4-4: Version bump to 3.0.0-beta.12 approved.Straightforward metadata update aligned with the PR's coordinated version bump across the monorepo.
packages/zod/package.json (1)
3-3: Version bump to 3.0.0-beta.12 approved.Straightforward metadata update aligned with the coordinated version bump across the monorepo.
packages/create-zenstack/package.json (1)
3-3: Version bump to 3.0.0-beta.12 approved.Straightforward metadata update aligned with the coordinated version bump across the monorepo.
packages/cli/package.json (1)
6-6: Version bump to 3.0.0-beta.12 approved.Straightforward metadata update aligned with the coordinated version bump across the monorepo.
packages/language/package.json (1)
4-4: Version bump to 3.0.0-beta.12 approved.Straightforward metadata update aligned with the coordinated version bump across the monorepo.
packages/testtools/package.json (1)
3-3: Version bump to 3.0.0-beta.12 approved.Straightforward metadata update aligned with the coordinated version bump across the monorepo.
samples/blog/package.json (1)
3-3: Version bump to 3.0.0-beta.12 approved.Straightforward metadata update aligned with the coordinated version bump across the monorepo.
packages/config/eslint-config/package.json (1)
3-3: Version bump to 3.0.0-beta.12 approved.Straightforward metadata update aligned with the coordinated version bump across the monorepo.
packages/config/typescript-config/package.json (1)
3-3: Version bump approved.The version update is consistent with the broader package-wide release to 3.0.0-beta.12.
packages/common-helpers/package.json (1)
3-3: Version bump approved.The version update is consistent with the broader package-wide release to 3.0.0-beta.12. Workspace dependencies using
workspace:*will automatically resolve to the updated versions.packages/dialects/sql.js/package.json (1)
3-3: Version bump approved.The version update is consistent with the broader package-wide release to 3.0.0-beta.12. External and workspace dependency references remain valid.
tests/e2e/orm/client-api/create.test.ts (1)
3-4: LGTM: import reordering is fineNo behavior change; consistent with test style elsewhere.
packages/runtime/src/client/crud/operations/delete.ts (1)
56-56: LGTM: deleteMany returns{ count }Matches the public API expectation.
packages/runtime/src/client/crud/operations/create.ts (1)
27-33: mutationNeedsReadBack does NOT guarantee ID inclusion—the concern is validThe
mutationNeedsReadBackmethod returnsselectedFieldsthat can omit ID fields when explicitargs.selectis provided. Inbase.tsline 346, therequireIdFieldsfallback only applies ifreturnFieldsis falsy, so passing a non-ID-includingselectedFieldsdirectly bypasses that safety guard.When a user explicitly selects fields without including IDs (e.g.,
select: { name: true }),allFieldsSelectedwill exclude IDs. This gets passed tocreate()and returned without IDs, potentially breaking transactions that need ID values for subsequent operations.The fix ensuring ID inclusion is necessary.
packages/runtime/src/client/crud/operations/base.ts (2)
10-10: Import LGTMAdding
QueryResultis appropriate for the new delete return shape.
344-347: Returning selected scalar fields: approach looks goodDefaults to id fields when unspecified; aligns with read-back optimization.
Please confirm callers validate
fieldsToReturnagainst actual scalar fields of the model to avoid runtime SQL errors.Also applies to: 352-352, 772-774, 1054-1060, 1303-1305
Summary by CodeRabbit
New Features
Chores