merge dev to main (v3.0.0-alpha.22) #160
Conversation
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
* feat: let schema pusher support delegate models * update
WalkthroughThis PR bumps versions across multiple package.json files, updates packages/runtime/package.json to add toposort and @types/toposort, and refactors packages/runtime/src/client/helpers/schema-db-pusher.ts to topologically sort model creation and adjust internal helper signatures and constraint naming. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Claude finished @ymc9's task —— View job PR Review: Merge dev to mainI've completed a comprehensive review of this PR. Here are my findings: Review Tasks:
📋 Review Summary✅ What's GoodVersion Management:
Code Quality - Delegate Model Implementation:
|
There was a problem hiding this comment.
Pull Request Overview
This is a version bump PR that updates all packages from version 3.0.0-alpha.21 to 3.0.0-alpha.22 across the ZenStack monorepo. Additionally, it includes significant improvements to the database schema creation logic to handle model inheritance and foreign key dependencies through topological sorting.
- Version increment across all packages and samples from alpha.21 to alpha.22
- Enhanced schema database pusher with topological sorting for proper table creation order
- Added support for model inheritance in database table creation
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| package.json | Root package version bump |
| packages/*/package.json | Package version updates and dependency additions |
| samples/blog/package.json | Sample project version update |
| tests/e2e/package.json | E2E test package version update |
| packages/runtime/src/client/helpers/schema-db-pusher.ts | Major refactoring to support model inheritance and proper table creation ordering |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
package.json(1 hunks)packages/cli/package.json(1 hunks)packages/common-helpers/package.json(1 hunks)packages/create-zenstack/package.json(1 hunks)packages/dialects/sql.js/package.json(1 hunks)packages/eslint-config/package.json(1 hunks)packages/ide/vscode/package.json(1 hunks)packages/language/package.json(1 hunks)packages/runtime/package.json(3 hunks)packages/runtime/src/client/helpers/schema-db-pusher.ts(6 hunks)packages/sdk/package.json(1 hunks)packages/tanstack-query/package.json(1 hunks)packages/testtools/package.json(1 hunks)packages/typescript-config/package.json(1 hunks)packages/vitest-config/package.json(1 hunks)packages/zod/package.json(1 hunks)samples/blog/package.json(1 hunks)tests/e2e/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/testtools/package.jsonpackages/zod/package.jsonpackages/vitest-config/package.jsonpackages/cli/package.jsonpackages/eslint-config/package.jsonpackages/tanstack-query/package.jsonpackages/sdk/package.jsonsamples/blog/package.jsonpackages/language/package.jsontests/e2e/package.jsonpackages/dialects/sql.js/package.jsonpackages/common-helpers/package.jsonpackages/typescript-config/package.jsonpackages/ide/vscode/package.jsonpackages/create-zenstack/package.jsonpackages/runtime/package.jsonpackages/runtime/src/client/helpers/schema-db-pusher.ts
tests/e2e/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
E2E tests are in
tests/e2e/directory
Files:
tests/e2e/package.json
🧠 Learnings (1)
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: `zenstack generate` compiles ZModel to TypeScript schema (`schema.ts`)
Applied to files:
packages/language/package.json
🧬 Code Graph Analysis (1)
packages/runtime/src/client/helpers/schema-db-pusher.ts (4)
packages/sdk/src/schema/schema.ts (2)
ModelDef(20-36)FieldDef(59-74)packages/runtime/src/client/query-utils.ts (1)
requireModel(20-26)packages/runtime/src/client/crud/operations/base.ts (1)
requireModel(111-113)packages/runtime/src/client/query-builder.ts (1)
ToKysely(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x)
🔇 Additional comments (23)
tests/e2e/package.json (1)
3-3: Monorepo-wide version bump verifiedAll
package.jsonfiles have been updated to3.0.0-alpha.22and no references to3.0.0-alpha.21remain. Ready to merge.packages/common-helpers/package.json (1)
3-3: Monorepo version alignment confirmed.Pure metadata bump; no API surface change. Safe to proceed.
packages/zod/package.json (1)
3-3: Version bump approved.No functional or dependency changes besides metadata. Workspace dependency on runtime will resolve to the same release.
packages/language/package.json (1)
4-4: Version bump approved.No code/config changes tied to the language package; safe metadata update.
packages/create-zenstack/package.json (1)
3-3: CLI package version bump approved.No changes to bin/exports; publishing should be straightforward with this version.
packages/ide/vscode/package.json (1)
4-4: ✅ Monorepo versions are in sync at 3.0.0-alpha.22All
package.jsonfiles across the repository match the root version (3.0.0-alpha.22). No further changes needed before publishing.packages/cli/package.json (1)
6-6: CLI package version updated correctly.Metadata-only change; no API/behavioral impact.
packages/eslint-config/package.json (1)
3-3: ESLint config version bump is fine.Private package; change is scoped to metadata only.
packages/vitest-config/package.json (1)
4-4: Vitest config version bump LGTM.No functional changes; exports unchanged.
packages/testtools/package.json (1)
3-3: Testtools version bump approved.No changes to dependencies/exports; safe metadata update.
package.json (2)
3-3: LGTM: root version bumped to 3.0.0-alpha.22No functional or behavioral changes; the bump aligns with the PR intent.
3-3: All version bumps and runtime dependencies verified
- No leftover
"3.0.0-alpha.21"references found across the monorepo.packages/runtime/package.jsonincludes both"toposort": "^2.0.2"and"@types/toposort": "^2.0.7"as expected.packages/tanstack-query/package.json (1)
3-3: LGTM: version bump onlyChange is scoped to the version field; exports, deps, and scripts remain unchanged.
packages/dialects/sql.js/package.json (1)
3-3: LGTM: dialect package version bumpOnly the version changed; no impact on exports or peer requirements.
samples/blog/package.json (1)
3-3: LGTM: sample app version bumpNo behavioral changes; aligns with monorepo-wide version sync.
packages/typescript-config/package.json (1)
3-3: LGTM: config package version bumpNo config changes; version aligns with the release train.
packages/sdk/package.json (1)
3-3: Version bump looks good.The version update from 3.0.0-alpha.21 to 3.0.0-alpha.22 aligns with the repository-wide version synchronization across all packages.
packages/runtime/src/client/helpers/schema-db-pusher.ts (4)
3-3: Good addition of topological sorting for dependency management.The addition of
toposortis appropriate for handling model creation order based on foreign key and inheritance dependencies.
79-82: Correct logic for skipping inherited fields.The condition correctly skips non-ID fields that are inherited from the base model, avoiding duplicate column creation.
91-101: Well-implemented base model delegation with proper cascade behavior.The foreign key constraint for base model delegation is correctly configured with cascade on delete and update, ensuring referential integrity in inheritance relationships.
146-180: Simplified and consistent field creation logic.The refactored
createModelFieldmethod with the new signature(table, fieldDef, modelDef)is cleaner and more consistent, deriving the column name directly fromfieldDef.name.packages/runtime/package.json (2)
3-3: Version synchronized with other packages.The version bump to 3.0.0-alpha.22 is consistent with the repository-wide update.
73-73: Appropriate addition of toposort dependency.The addition of
toposortas a runtime dependency and@types/toposortas a dev dependency properly supports the topological sorting functionality in the schema database pusher.Also applies to: 95-95
| private sortModels(models: Record<string, ModelDef>): ModelDef[] { | ||
| const graph: [ModelDef, ModelDef | undefined][] = []; | ||
|
|
||
| for (const model of Object.values(models)) { | ||
| let added = false; | ||
|
|
||
| if (model.baseModel) { | ||
| // base model should be created before concrete model | ||
| const baseDef = requireModel(this.schema, model.baseModel); | ||
| // edge: concrete model -> base model | ||
| graph.push([model, baseDef]); | ||
| added = true; | ||
| } | ||
|
|
||
| for (const field of Object.values(model.fields)) { | ||
| // relation order | ||
| if (field.relation && field.relation.fields && field.relation.references) { | ||
| const targetModel = requireModel(this.schema, field.type); | ||
| // edge: fk side -> target model | ||
| graph.push([model, targetModel]); | ||
| added = true; | ||
| } | ||
| } | ||
|
|
||
| if (!added) { | ||
| // no relations, add self to graph to ensure it is included in the result | ||
| graph.push([model, undefined]); | ||
| } | ||
| } | ||
|
|
||
| return toposort(graph) | ||
| .reverse() | ||
| .filter((m) => !!m); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling circular dependencies in the model graph.
The topological sorting implementation correctly handles foreign key and base model dependencies. However, if there are circular dependencies in the model relationships, toposort will throw an error that isn't caught.
Consider adding error handling to provide a more helpful error message:
private sortModels(models: Record<string, ModelDef>): ModelDef[] {
const graph: [ModelDef, ModelDef | undefined][] = [];
for (const model of Object.values(models)) {
let added = false;
if (model.baseModel) {
// base model should be created before concrete model
const baseDef = requireModel(this.schema, model.baseModel);
// edge: concrete model -> base model
graph.push([model, baseDef]);
added = true;
}
for (const field of Object.values(model.fields)) {
// relation order
if (field.relation && field.relation.fields && field.relation.references) {
const targetModel = requireModel(this.schema, field.type);
// edge: fk side -> target model
graph.push([model, targetModel]);
added = true;
}
}
if (!added) {
// no relations, add self to graph to ensure it is included in the result
graph.push([model, undefined]);
}
}
- return toposort(graph)
- .reverse()
- .filter((m) => !!m);
+ try {
+ return toposort(graph)
+ .reverse()
+ .filter((m) => !!m);
+ } catch (error) {
+ if (error instanceof Error && error.message.includes('Cyclic dependency')) {
+ throw new Error(`Circular dependency detected in model relationships: ${error.message}`);
+ }
+ throw error;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private sortModels(models: Record<string, ModelDef>): ModelDef[] { | |
| const graph: [ModelDef, ModelDef | undefined][] = []; | |
| for (const model of Object.values(models)) { | |
| let added = false; | |
| if (model.baseModel) { | |
| // base model should be created before concrete model | |
| const baseDef = requireModel(this.schema, model.baseModel); | |
| // edge: concrete model -> base model | |
| graph.push([model, baseDef]); | |
| added = true; | |
| } | |
| for (const field of Object.values(model.fields)) { | |
| // relation order | |
| if (field.relation && field.relation.fields && field.relation.references) { | |
| const targetModel = requireModel(this.schema, field.type); | |
| // edge: fk side -> target model | |
| graph.push([model, targetModel]); | |
| added = true; | |
| } | |
| } | |
| if (!added) { | |
| // no relations, add self to graph to ensure it is included in the result | |
| graph.push([model, undefined]); | |
| } | |
| } | |
| return toposort(graph) | |
| .reverse() | |
| .filter((m) => !!m); | |
| } | |
| private sortModels(models: Record<string, ModelDef>): ModelDef[] { | |
| const graph: [ModelDef, ModelDef | undefined][] = []; | |
| for (const model of Object.values(models)) { | |
| let added = false; | |
| if (model.baseModel) { | |
| // base model should be created before concrete model | |
| const baseDef = requireModel(this.schema, model.baseModel); | |
| // edge: concrete model -> base model | |
| graph.push([model, baseDef]); | |
| added = true; | |
| } | |
| for (const field of Object.values(model.fields)) { | |
| // relation order | |
| if (field.relation && field.relation.fields && field.relation.references) { | |
| const targetModel = requireModel(this.schema, field.type); | |
| // edge: fk side -> target model | |
| graph.push([model, targetModel]); | |
| added = true; | |
| } | |
| } | |
| if (!added) { | |
| // no relations, add self to graph to ensure it is included in the result | |
| graph.push([model, undefined]); | |
| } | |
| } | |
| try { | |
| return toposort(graph) | |
| .reverse() | |
| .filter((m) => !!m); | |
| } catch (error) { | |
| if (error instanceof Error && error.message.includes('Cyclic dependency')) { | |
| throw new Error(`Circular dependency detected in model relationships: ${error.message}`); | |
| } | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/runtime/src/client/helpers/schema-db-pusher.ts around lines 40 to
73, the call to toposort can throw on circular dependencies and is not handled;
wrap the toposort + reverse + filter sequence in a try/catch, catch the error,
build and throw a new Error with a clear message that a circular dependency was
detected (include contextual info such as the schema name or the list of model
names/edges from the graph for easier debugging), and rethrow or return a
controlled failure; ensure the function still returns the expected ModelDef[]
type when no error occurs.
| // create fk constraint | ||
| const baseModelDef = requireModel(this.schema, modelDef.baseModel); | ||
| table = table.addForeignKeyConstraint( | ||
| `fk_${modelDef.baseModel}_delegate`, |
There was a problem hiding this comment.
Fix constraint naming: should use the concrete model name.
The foreign key constraint name uses modelDef.baseModel but it should use modelDef.name to maintain consistency with other constraint naming patterns and avoid potential naming conflicts.
table = table.addForeignKeyConstraint(
- `fk_${modelDef.baseModel}_delegate`,
+ `fk_${modelDef.name}_delegate`,
baseModelDef.idFields,
modelDef.baseModel,
baseModelDef.idFields,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `fk_${modelDef.baseModel}_delegate`, | |
| table = table.addForeignKeyConstraint( | |
| - `fk_${modelDef.baseModel}_delegate`, | |
| + `fk_${modelDef.name}_delegate`, | |
| baseModelDef.idFields, | |
| modelDef.baseModel, | |
| baseModelDef.idFields, | |
| ); |
🤖 Prompt for AI Agents
In packages/runtime/src/client/helpers/schema-db-pusher.ts around line 95, the
foreign-key constraint is currently named using modelDef.baseModel but should
use the concrete model name to match other constraint naming; replace use of
modelDef.baseModel with modelDef.name (e.g., build the constraint name as
fk_${modelDef.name}_delegate) so the constraint names are consistent and avoid
conflicts.
Summary by CodeRabbit
Bug Fixes
Chores