merge dev to main (v3.0.0-alpha.33) #214
Conversation
Co-authored-by: ymc9 <[email protected]>
* refactor: avoid creating nested query when mapping names * address pr comments
…ueries (#213) * refactor: optimize relation selections and avoid unnecessary nested queries * update
|
Warning Rate limit exceeded@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 24 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 (1)
WalkthroughThis PR bumps versions to 3.0.0-alpha.33 across the repo and updates runtime internals: CRUD dialect base adds helpers for model select and join decisions; PostgreSQL/SQLite dialects refactor relation selection/aliasing; name-mapper switches to scope-based processing; schema DB pusher applies @@map/@Map names; adds ExpressionUtils.getLiteralValue; expands name-mapping tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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 Complete ✅This is a comprehensive review of the dev → main merge PR. Here's my analysis: 📋 SummaryThis PR implements relation selection optimization and name mapping improvements as the primary functional changes. The version bumps across all packages suggest this is preparing for a release (alpha.33 → main). ✅ Strengths & QualityCode Architecture & Design
Performance Optimizations
Test Coverage
🔍 Technical Deep DiveCore Changes Analysis1. Query Name Mapper (
2. Relation Selection Optimization (
3. Schema DB Pusher Improvements (
🎯 Code Quality AssessmentBest Practices ✅
Security ✅
Maintainability ✅
📈 Performance ImpactPositive Changes:
Potential Concerns:
🐛 Potential Issues & RecommendationsLow Priority Issues:
Minor Suggestions:
🚀 Overall AssessmentThis is a high-quality, well-architected PR that delivers significant improvements to query optimization and name mapping capabilities. The changes are:
The code demonstrates strong TypeScript usage, proper error handling, and follows established patterns. The optimization improvements are substantial and the name mapping enhancements add important functionality. Recommendation: ✅ APPROVE - This PR is ready to merge with confidence. |
There was a problem hiding this comment.
Pull Request Overview
This PR merges changes from the dev branch to main, incrementing the version number across all packages from 3.0.0-alpha.32 to 3.0.0-alpha.33. The changes include significant improvements to database name mapping functionality and comprehensive test coverage for count, aggregate, and groupBy operations.
- Version bump across all 19 packages and samples in the monorepo
- Enhanced database schema generation with support for
@@mapand@mapattributes for table and column name mapping - Improved query name mapping logic for better handling of table joins and field references
- New comprehensive test suite covering count, aggregate, and groupBy operations with name mapping
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| package.json (root + 18 packages) | Version increment from alpha.32 to alpha.33 |
| packages/runtime/src/schema/expression.ts | Added utility function to extract literal values from expressions |
| packages/runtime/src/client/helpers/schema-db-pusher.ts | Enhanced schema generation with table/column name mapping support |
| packages/runtime/src/client/executor/name-mapper.ts | Refactored query name mapping with improved scope handling and join processing |
| packages/runtime/src/client/crud/dialects/*.ts | Optimized relation query building with conditional nested selects |
| packages/runtime/test/client-api/name-mapping.test.ts | Added comprehensive tests for count, aggregate, and groupBy operations |
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime/src/client/helpers/schema-db-pusher.ts (1)
93-103: Bug: base-model (delegate) FK doesn’t honor @@map/@Map for table and column names.
This path still uses logical names for table and columns; with mappings applied, the FK can point to non-existent identifiers.Apply:
if (modelDef.baseModel) { // create fk constraint - const baseModelDef = requireModel(this.schema, modelDef.baseModel); - table = table.addForeignKeyConstraint( - `fk_${modelDef.baseModel}_delegate`, - baseModelDef.idFields, - modelDef.baseModel, - baseModelDef.idFields, - (cb) => cb.onDelete('cascade').onUpdate('cascade'), - ); + const baseModelDef = requireModel(this.schema, modelDef.baseModel); + table = table.addForeignKeyConstraint( + `fk_${this.getTableName(baseModelDef)}_delegate`, + modelDef.idFields.map((f) => this.getColumnName(modelDef.fields[f]!)), + this.getTableName(baseModelDef), + baseModelDef.idFields.map((f) => this.getColumnName(baseModelDef.fields[f]!)), + (cb) => cb.onDelete('cascade').onUpdate('cascade'), + ); }
🧹 Nitpick comments (12)
packages/language/package.json (1)
4-4: Language package version updated: OK.Downstream VSCode extension depends via workspace:*, so this aligns. Consider generating/committing a consolidated changelog entry if you maintain per-release notes.
packages/ide/vscode/package.json (1)
4-4: VSCode extension manifest version bump: good.Publish script already uses --pre-release; nothing else needed. Ensure marketplace release notes reference alpha.33.
packages/runtime/src/schema/expression.ts (1)
112-115: Nice utility addition; simple and type-safe.
Returns undefined for non-literals as intended. Consider a short JSDoc noting it only handles literal expressions.packages/runtime/src/client/crud/dialects/base.ts (1)
1143-1170: Join decision helper is sensible; consider tightening for edge cases.
Optionally return false when payload.select includes computed fields (even if the model has other non-computed fields) to avoid accidental direct joins. Current blanket check (any computedFields) is safe but may force unnecessary nested selects.packages/runtime/src/client/helpers/schema-db-pusher.ts (4)
111-121: Robustness: support named args and validate literal type for @@Map.
If attributes can have named args, consider falling back to a named lookup; also assert the value is a string for clearer errors.Apply:
- private getTableName(modelDef: ModelDef) { - const mapAttr = modelDef.attributes?.find((a) => a.name === '@@map'); - if (mapAttr && mapAttr.args?.[0]) { - const mappedName = ExpressionUtils.getLiteralValue(mapAttr.args[0].value); - if (mappedName) { - return mappedName as string; - } - } - return modelDef.name; - } + private getTableName(modelDef: ModelDef) { + const mapAttr = modelDef.attributes?.find((a) => a.name === '@@map'); + const arg = (mapAttr as any)?.args?.find?.((x: any) => x?.name === 'name') ?? mapAttr?.args?.[0]; + const mappedName = arg ? ExpressionUtils.getLiteralValue(arg.value) : undefined; + return typeof mappedName === 'string' ? mappedName : modelDef.name; + }
122-131: Same suggestion for @Map on fields.- private getColumnName(fieldDef: FieldDef) { - const mapAttr = fieldDef.attributes?.find((a) => a.name === '@map'); - if (mapAttr && mapAttr.args?.[0]) { - const mappedName = ExpressionUtils.getLiteralValue(mapAttr.args[0].value); - if (mappedName) { - return mappedName as string; - } - } - return fieldDef.name; - } + private getColumnName(fieldDef: FieldDef) { + const mapAttr = fieldDef.attributes?.find((a) => a.name === '@map'); + const arg = (mapAttr as any)?.args?.find?.((x: any) => x?.name === 'name') ?? mapAttr?.args?.[0]; + const mappedName = arg ? ExpressionUtils.getLiteralValue(arg.value) : undefined; + return typeof mappedName === 'string' ? mappedName : fieldDef.name; + }
146-149: PK columns honor @Map — consider aligning constraint name with @@Map.
Using logical model name in constraint identifiers can be confusing after remapping. Optional: use getTableName(modelDef) in the name.- `pk_${modelDef.name}`, + `pk_${this.getTableName(modelDef)}`,
165-170: Unique constraints correctly map columns; optional: map table name in identifier.- `unique_${modelDef.name}_${key}`, + `unique_${this.getTableName(modelDef)}_${key}`,- `unique_${modelDef.name}_${key}`, + `unique_${this.getTableName(modelDef)}_${key}`,packages/runtime/src/client/crud/dialects/sqlite.ts (1)
203-251: Join-filter helper is solid; consider centralizingLogic covers M2M and non-M2M well. Consider moving this helper into BaseCrudDialect to avoid duplication across dialects. Also, schemas with composite IDs aren’t supported here—worth documenting.
Do you want a follow-up PR to lift this into BaseCrudDialect and add notes about composite-ID limitations?
packages/runtime/src/client/crud/dialects/postgresql.ts (1)
149-181: Join-filter helper mirrors SQLite; good parityM2M and non-M2M cases are handled; buildJoinPairs use keeps code terse. Consider hoisting to BaseCrudDialect to DRY.
packages/runtime/src/client/executor/name-mapper.ts (2)
202-235: SelectAll expansion targets only the innermost scopeEdge case: with multiple froms/joins, expanding SelectAll using the last scope may miss fields from other tables. Consider qualifying or expanding per-scope when multiple tables are present.
237-274: Ambiguous unqualified column resolutionresolveFieldFromScopes returns the nearest scope that contains the column. This may inadvertently resolve ambiguous columns (e.g., common 'id') in multi-table queries. Consider warning or requiring qualification when multiple scopes match.
Would you like a follow-up that detects multiple matches and throws or logs in dev?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (24)
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(1 hunks)packages/runtime/src/client/crud/dialects/base.ts(3 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(6 hunks)packages/runtime/src/client/crud/dialects/sqlite.ts(4 hunks)packages/runtime/src/client/executor/name-mapper.ts(9 hunks)packages/runtime/src/client/helpers/schema-db-pusher.ts(5 hunks)packages/runtime/src/schema/expression.ts(1 hunks)packages/runtime/test/client-api/name-mapping.test.ts(1 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/runtime/src/schema/expression.tspackages/language/package.jsonpackages/testtools/package.jsonsamples/blog/package.jsonpackages/ide/vscode/package.jsonpackages/zod/package.jsonpackages/runtime/package.jsonpackages/cli/package.jsonpackages/vitest-config/package.jsonpackages/typescript-config/package.jsonpackages/create-zenstack/package.jsonpackages/eslint-config/package.jsonpackages/dialects/sql.js/package.jsontests/e2e/package.jsonpackages/runtime/src/client/helpers/schema-db-pusher.tspackages/sdk/package.jsonpackages/tanstack-query/package.jsonpackages/runtime/test/client-api/name-mapping.test.tspackages/common-helpers/package.jsonpackages/runtime/src/client/crud/dialects/sqlite.tspackages/runtime/src/client/crud/dialects/postgresql.tspackages/runtime/src/client/crud/dialects/base.tspackages/runtime/src/client/executor/name-mapper.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 (6)
packages/runtime/src/schema/expression.ts (1)
packages/sdk/src/schema/expression.ts (1)
Expression(1-10)
packages/runtime/src/client/helpers/schema-db-pusher.ts (3)
packages/sdk/src/schema/schema.ts (2)
ModelDef(20-36)FieldDef(59-74)packages/runtime/src/schema/expression.ts (1)
ExpressionUtils(16-116)packages/runtime/src/client/query-utils.ts (1)
requireModel(20-26)
packages/runtime/src/client/crud/dialects/sqlite.ts (2)
packages/runtime/src/client/query-utils.ts (4)
requireField(33-39)getManyToManyRelation(222-258)getIdFields(41-44)getRelationForeignKeyFieldPairs(55-100)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/runtime/src/client/crud/dialects/postgresql.ts (4)
packages/runtime/src/client/crud-types.ts (1)
FindArgs(646-662)packages/sdk/src/schema/schema.ts (2)
GetModels(108-108)FieldDef(59-74)packages/runtime/src/client/query-utils.ts (5)
requireField(33-39)requireModel(20-26)getManyToManyRelation(222-258)getIdFields(41-44)buildJoinPairs(197-215)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/runtime/src/client/crud/dialects/base.ts (2)
packages/sdk/src/schema/schema.ts (2)
GetModels(108-108)ModelDef(20-36)packages/runtime/src/client/crud-types.ts (1)
FindArgs(646-662)
packages/runtime/src/client/executor/name-mapper.ts (3)
packages/runtime/src/client/executor/kysely-utils.ts (1)
stripAlias(7-14)packages/runtime/src/client/query-utils.ts (2)
getModel(16-18)requireModel(20-26)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
⏰ 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). (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (37)
packages/zod/package.json (1)
3-3: Approve version bump & confirm no leftover references
- Verified no remaining
3.0.0-alpha.32versions in package.json files- Confirmed all
@zenstackhq/*dependencies are usingworkspace:*- No stale
alpha.32references found anywhere elseVersion bump to
3.0.0-alpha.33LGTM 🚀packages/typescript-config/package.json (1)
3-3: TS config package version bump looks good.No API surface here; safe to ship as part of the coordinated release.
packages/create-zenstack/package.json (1)
3-3: CLI scaffolder version bump is consistent.Pre-release tag matches repo-wide bump; no script/deps changes.
packages/runtime/package.json (2)
3-3: Version bump to 3.0.0-alpha.33 looks good.
No other manifest deltas; exports and deps unchanged.
3-3: All packages are on 3.0.0-alpha.33 and no αlpha.32 references remainAll
package.jsonversion fields have been verified to be3.0.0-alpha.33, and a repo-wide scan found no occurrences of"alpha.32"in any JSON files (including dependencies/devDependencies/peerDependencies). You’re clear to merge.packages/cli/package.json (1)
6-6: CLI version bump is consistent with the release train.
No other manifest changes; safe.tests/e2e/package.json (1)
3-3: E2E package version bump acknowledged.
Keep pnpm lock refreshed post-merge.samples/blog/package.json (1)
3-3: Sample app version bump OK.
No runtime/CLI constraints changed; sample remains compatible.packages/eslint-config/package.json (1)
3-3: ESLint config version bump OK.
Private package; no publish impact expected.packages/sdk/package.json (1)
3-3: LGTM: version bumped to 3.0.0-alpha.33No issues spotted for the SDK package metadata.
packages/testtools/package.json (1)
3-3: LGTM: version bumped to 3.0.0-alpha.33Change is metadata-only; peer deps unchanged and acceptable.
packages/tanstack-query/package.json (1)
3-3: LGTM: version bumped to 3.0.0-alpha.33Package is private; no publish impact.
package.json (1)
3-3: All versions updated and workspace consistency verifiedNo stale “3.0.0-alpha.32” references detected.
Every package.json reports version 3.0.0-alpha.33, and all internal “@zenstackhq/*” dependencies use theworkspace:protocol as expected.packages/common-helpers/package.json (1)
3-3: LGTM: version bumped to 3.0.0-alpha.33No further changes required.
packages/vitest-config/package.json (1)
4-4: Version alignment verified
I ran the provided script and confirmed that every internalpackage.jsonunderpackages/,samples/, andtests/is set to version3.0.0-alpha.33.packages/dialects/sql.js/package.json (1)
3-3: LGTM: package version aligned with workspace.packages/runtime/src/client/crud/dialects/base.ts (2)
5-5: Importing ModelDef is appropriate for the new helper signature.
966-989: Approval:buildModelSelectusage verified
- Confirmed in
packages/runtime/src/client/crud/dialects/sqlite.tsat lines 81 and 92- Confirmed in
packages/runtime/src/client/crud/dialects/postgresql.tsat lines 93 and 107Good factoring: centralized sub-query builder with optional field selection and filtering/sorting/take. No functional concerns spotted.
packages/runtime/src/client/helpers/schema-db-pusher.ts (3)
76-79: Table creation now respects @@Map — good.
177-177: Column creation respects @Map — good coverage.
273-281: FK constraints now fully honor @map/@@Map on both sides — nice.packages/runtime/src/client/crud/dialects/sqlite.ts (4)
79-86: Good conditional pathing for relation selectionClean split between direct-join and nested-select keeps SQLite-compatible and reduces unnecessary subqueries.
87-104: Nested subquery aliasing is consistentselectModelAlias → subQueryName flow is correct; downstream refs resolve to subQueryName as intended.
129-132: Consistent field qualification via subQueryNameUsing subQueryName for scalar fieldRefs in both “select all” and selective branches avoids ambiguity.
Also applies to: 163-164
191-197: Unified JSON alias '$data'Standardizing on '$data' for both array and object relations aligns with Postgres path and simplifies consumers.
packages/runtime/src/client/crud/dialects/postgresql.ts (3)
61-71: Selecting relation JSON via lateral join is correctJoining once then selecting
${relationResultName}.$data as ${relationField}is efficient and clear.
91-127: Direct vs nested relation select is well-factoredThe canJoinWithoutNestedSelect branch prevents over-nesting; aliases remain stable (
relationSelectName,resultName).
200-206: Unified '$data' JSON aliasMatches SQLite path and simplifies downstream JSON extraction.
packages/runtime/src/client/executor/name-mapper.ts (6)
58-66: Scope collection from froms/joins is straightforwardMerging scopes from both sources makes downstream resolution predictable.
67-81: Selections handled with scope-aware mapping; alias preservation is niceprocessSelectQuerySelections expands/mapps thoughtfully and preserves original field names when needed.
345-375: processSelectTable handles mapped names and preserves model aliases wellThe namesMapped flag is set correctly based on per-model column mappings; subqueries default to namesMapped=true, which is sensible.
377-391: createSelectAllFields correctly re-aliases mapped columnsRe-aliasing to original field names avoids breaking callers relying on model field names.
427-443: processSelectAll mirrors selection expansion; consistent with custom SelectAll handlingGood consistency with processSelectQuerySelections; no surprises here.
168-177: DeleteQuery scopes initializationPushing from-scopes (namesMapped=false) ensures returning/where mappings apply. Guard for node.from being present if Kysely AST ever omits it for certain dialects.
packages/runtime/test/client-api/name-mapping.test.ts (3)
239-276: Count tests cover ORM and Kysely pathsGood coverage on select-projected count, relation-filtered count, and raw Kysely count.
278-341: Aggregate tests validate _count/_min/_max and filtered aggregatesSolid parallel checks via ORM and Kysely ensure mapper parity.
343-433: GroupBy tests validate by/having across providersGood dataset and assertions; this will catch scope/alias regressions.
|
Claude finished @ymc9's task —— View job PR Review Complete ✅This is a comprehensive review of the dev → main merge PR for v3.0.0-alpha.33. Here's my analysis: 📋 SummaryThis PR implements relation selection optimization and name mapping improvements as the primary functional changes, along with coordinated version bumps across all packages preparing for the alpha.33 release. ✅ Strengths & QualityCode Architecture & Design
Performance Optimizations
Test Coverage
🔍 Technical Deep DiveCore Changes Analysis1. Query Name Mapper (
2. Relation Selection Optimization (
3. Schema DB Pusher Improvements (
4. Expression Utils Enhancement
🎯 Code Quality AssessmentBest Practices ✅
Security ✅
Maintainability ✅
📈 Performance ImpactPositive Changes:
Potential Concerns:
🐛 Issues IdentifiedCritical Issue in PostgreSQL Dialect (postgresql.ts:269)// CURRENT - Missing relationModelAlias qualification:
this.fieldRef(relationModel, field, eb, undefined, false);
// SHOULD BE - Properly qualified:
this.fieldRef(relationModel, field, eb, relationModelAlias, false);Impact: Can cause ambiguous column references in JOIN queries. This should be fixed before merge. Minor Issues:
🚀 Overall AssessmentThis is a high-quality, well-architected PR that delivers significant improvements to query optimization and name mapping capabilities. The changes are:
The code demonstrates strong TypeScript usage, proper error handling, and follows established patterns. The optimization improvements are substantial and the name mapping enhancements add important functionality. 📝 Recommendation✅ APPROVE with one fix required: Fix the PostgreSQL dialect fieldRef qualification issue at line 269 before merging. The rest of the changes are excellent and ready for production. Review Tasks:
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores