merge dev to main #261
merge dev to main #261
Conversation
* fix: format zod errors, issue with "this" member access * update tests
* feat(policy): support "insert on conflict update" * address pr comments
* fix(policy): relation access via `this`, more test cases * minor fixes
…e with "using" (#253) * feat(policy): support read filtering for update with "from" and delete with "using" * addressing pr comments * more robust alias handling * addressing pr comments
* feat(policy): implementing `check` function * addressing PR comments
* feat(policy): many-to-many and self-relation support * address PR comments, refactor m2m check * extra fixes and tests
|
Warning Rate limit exceeded@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 4 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)
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 DB-matrix CI, version bumps, and new scripts. Introduces helpers (zip, kysely utils), extends language stdlib, and broad runtime changes: dialect output transforms, stricter ID handling, CRUD type updates, policy error enum, plugin function “check”, client/result processing refactor, input validator tweaks, and extensive test refactors/additions. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
Claude finished @ymc9's task —— View job PR Review for dev → main mergeReviewing this large PR (100 files, 13K+ additions) for code quality, bugs, performance, security, and test coverage. Review Tasks:
🔍 Review SummaryThis is a substantial PR merging the development branch to main for ZenStack v3.0.0-beta.5. The changes primarily focus on policy engine improvements, validation enhancements, and comprehensive test coverage expansion. ✅ Positive Aspects1. Code Quality & Architecture
2. Security
3. Test Coverage
|
There was a problem hiding this comment.
Pull Request Overview
This PR merges development branch changes to main, introducing significant improvements to the ZenStack framework including computed field parameter renaming, new test utilities, and enhanced policy testing capabilities.
- Renames
currentModelparameter tomodelAliasin computed field contexts for better semantic clarity - Introduces centralized test utilities and database provider configuration
- Adds comprehensive policy test coverage with migrated test suites
Reviewed Changes
Copilot reviewed 133 out of 134 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/test/utils.ts | Major refactoring of test utilities with database provider abstraction and improved schema generation |
| packages/sdk/src/ts-schema-generator.ts | Renames computed field parameter from currentModel to modelAlias |
| packages/runtime/test/policy/migrated/* | Extensive migration of policy tests with comprehensive CRUD and relation testing |
| samples/blog/main.ts | Updates computed field usage to use new modelAlias parameter |
| Multiple package.json files | Version bump to 3.0.0-beta.5 across all packages |
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
packages/runtime/src/client/helpers/schema-db-pusher.ts (1)
41-74: Ensure topological order is restricted to non-view models only.Constrain edges to in-scope nodes and filter the final order to avoid view models leaking back from dependencies.
Apply this diff:
- private sortModels(models: ModelDef[]): ModelDef[] { - const graph: [ModelDef, ModelDef | undefined][] = []; - - for (const model of models) { + private sortModels(models: ModelDef[]): ModelDef[] { + const graph: [ModelDef, ModelDef | undefined][] = []; + const inScope = new Set(models); + + for (const model of 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; + if (inScope.has(baseDef)) { + 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 (inScope.has(targetModel)) { + 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); + return toposort(graph) + .reverse() + .filter((m): m is ModelDef => !!m && inScope.has(m)); }packages/runtime/test/client-api/type-coverage.test.ts (1)
87-99: Bytes equality can differ across providers (Buffer vs Uint8Array)Postgres often returns
Bufferwhile SQLite may returnUint8Array.toMatchObject(data)can fail due to instance mismatch. Match by type or content instead.- await db.foo.create({ data }); - await expect(db.foo.findUnique({ where: { id: '1' } })).resolves.toMatchObject(data); + await db.foo.create({ data }); + await expect(db.foo.findUnique({ where: { id: '1' } })).resolves.toMatchObject({ + ...data, + // Buffer is a Uint8Array subclass; this keeps the assertion portable + Bytes: expect.any(Uint8Array), + });Alternative: compare byte content explicitly after fetching and spreading to arrays.
Also applies to: 121-122
packages/runtime/test/policy/migrated/client-extensions.test.ts (1)
6-8: Ensure test clients are torn down to avoid resource leaks
- Root cause: createPolicyTestClient delegates to createTestClient (packages/runtime/test/policy/utils.ts -> packages/runtime/test/utils.ts); createTestClient constructs and returns a ZenStackClient and opens dialect resources (better-sqlite3 / pg Pool) but does not schedule/perform client.$disconnect().
- Evidence: many policy tests create a client via createPolicyTestClient and never call db.$disconnect() or add afterEach teardown (example: packages/runtime/test/policy/migrated/client-extensions.test.ts).
- Action: add deterministic teardown — either have createTestClient/createPolicyTestClient register a Vitest afterEach that awaits client.$disconnect(), or update affected tests to call await db.$disconnect() (add afterEach).
packages/runtime/test/policy/crud/update.test.ts (1)
4-4: Close clients to avoid leaked connectionscreatePolicyTestClient uses a real Pool/dialect. None of the tests in this file call $disconnect(), which can hang CI (especially on PostgreSQL). Add finally { await db.$disconnect() } per test, or refactor to beforeEach/afterEach to manage lifecycle.
packages/language/src/validators/expression-validator.ts (1)
114-131: Relational ops with null may slip throughFor
<, <=, >, >=, comparing againstnullshould be invalid. With the new checks, when a side isnull,declis falsy and no error is raised. Add an explicit guard.Apply this diff near the start of the comparator case:
case '>', '>=' , '<', '<=': { + if (isNullExpr(expr.left) || isNullExpr(expr.right)) { + accept('error', 'operand cannot be null for relational comparison', { node: expr }); + break; + }packages/runtime/src/client/crud/operations/update.ts (1)
34-45: Don’t swallow non-policy errors during read-backCatching all errors can mask real DB/query bugs and misreport them as policy denials later. Only suppress policy errors; rethrow others.
Apply this diff:
- try { - readBackResult = await this.readUnique(tx, this.model, { + try { + readBackResult = await this.readUnique(tx, this.model, { select: args.select, include: args.include, omit: args.omit, where: readFilter as WhereInput<Schema, GetModels<Schema>, false>, - }); - } catch { - // commit the update even if read-back failed - } + }); + } catch (err) { + // commit the update even if read-back failed due to policy; bubble up other errors + if (!(err instanceof RejectedByPolicyError)) { + throw err; + } + }packages/runtime/src/client/crud/operations/base.ts (1)
2084-2099: getEntityIds should always return a PromiseCurrently returns a Promise or a plain object depending on input, which harms type clarity. Make it async to stabilize the contract.
Apply this diff:
- private getEntityIds(kysely: ToKysely<Schema>, model: GetModels<Schema>, uniqueFilter: any) { + private async getEntityIds(kysely: ToKysely<Schema>, model: GetModels<Schema>, uniqueFilter: any) { const idFields: string[] = requireIdFields(this.schema, model); if ( // all id fields are provided idFields.every((f) => f in uniqueFilter && uniqueFilter[f] !== undefined) && // no non-id filter exists Object.keys(uniqueFilter).every((k) => idFields.includes(k)) ) { - return uniqueFilter; + return uniqueFilter; } - return this.readUnique(kysely, model, { + return this.readUnique(kysely, model, { where: uniqueFilter, }); }packages/runtime/src/plugins/policy/expression-transformer.ts (1)
115-118: Number literals are forced toInt; useFloatfor non-integers.This can generate wrong predicates for floating literals.
Apply this diff:
- return this.transformValue( - expr.value, - typeof expr.value === 'string' ? 'String' : typeof expr.value === 'boolean' ? 'Boolean' : 'Int', - ); + const t = + typeof expr.value === 'string' + ? 'String' + : typeof expr.value === 'boolean' + ? 'Boolean' + : Number.isInteger(expr.value as number) + ? 'Int' + : 'Float'; + return this.transformValue(expr.value, t);
packages/runtime/test/policy/migrated/relation-one-to-many-filter.test.ts
Show resolved
Hide resolved
packages/runtime/test/policy/migrated/relation-one-to-one-filter.test.ts
Show resolved
Hide resolved
|
Claude finished @ymc9's task —— View job PR Review for dev → main mergeReviewing this large PR (100 files, 13K+ additions) for code quality, bugs, performance, security, and test coverage. Review Tasks:
🔍 Review SummaryThis is a substantial PR merging the development branch to main for ZenStack v3.0.0-beta.5. The changes primarily focus on policy engine improvements, validation enhancements, and comprehensive test coverage expansion. ✅ Positive Aspects1. Code Quality & Architecture
2. Security
3. Test Coverage
|
|
Claude finished @ymc9's task —— View job PR Review for dev → main mergeReviewing this large PR (100 files, 13K+ additions) for code quality, bugs, performance, security, and test coverage. Review Tasks:
🔍 Review SummaryThis is a substantial PR merging the development branch to main for ZenStack v3.0.0-beta.5. The changes primarily focus on policy engine improvements, validation enhancements, and comprehensive test coverage expansion. ✅ Positive Aspects1. Code Quality & Architecture
2. Security
3. Test Coverage
|
Summary by CodeRabbit