fix: make computed fields on base models work #321
Conversation
|
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. WalkthroughThis PR replaces the exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1){packages,samples,tests}/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)packages/runtime/src/client/crud/dialects/base-dialect.ts (3)
⏰ 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)
🔇 Additional comments (2)
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 ProgressReviewing PR #321: Fix computed fields on base models Review Tasks:
Review SummaryThis PR successfully fixes computed fields functionality for delegate base models. The solution is clean, well-tested, and addresses the core issue effectively. 📋 Code Quality & Best Practices✅ Excellent:
✅ Follows TypeScript best practices:
🐛 Bug Analysis✅ No bugs identified:
The core fix (line 1086 in base-dialect.ts): computer = computedFields?.[fieldDef.originModel ?? model]?.[field];This correctly looks up computed fields in the base model when dealing with inherited fields. ⚡ Performance Considerations✅ Performance neutral or positive:
🔒 Security Assessment✅ No security concerns:
🧪 Test Coverage✅ Comprehensive test coverage:
Test highlights:
📚 Code Structure Improvements✅ Better code organization:
Minor Observations
Conclusion✅ Recommended for approval This is a well-executed fix that:
The changes are minimal, focused, and address the root cause effectively. No security, performance, or code quality concerns identified. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where computed fields defined on base models were not working correctly in delegate model scenarios. The fix ensures that when a computed field is inherited from a delegate base model, the system correctly looks up the field implementation using the origin model name rather than the inheriting model name.
Key changes:
- Refactored
buildSelectFieldto handle computed fields from delegate bases - Inlined
buildFieldReflogic intofieldRefmethod with support fororiginModellookup - Updated tests to verify computed fields work with delegate inheritance
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/runtime/src/client/query-utils.ts |
Removed the buildFieldRef utility function (logic moved elsewhere) |
packages/runtime/src/client/crud/dialects/base-dialect.ts |
Modified buildSelectField to handle computed fields from delegate bases and inlined buildFieldRef logic into fieldRef with origin model support |
tests/e2e/orm/client-api/computed-fields.test.ts |
Added test case for computed fields on delegate base models and cleaned up test structure |
samples/blog/zenstack/schema.zmodel |
Enhanced comment to clarify pnpm-specific workaround |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Claude finished @ymc9's task —— View job Code Review ProgressReviewing PR #321: Fix computed fields on base models Review Tasks:
Review SummaryThis PR successfully fixes computed fields functionality for delegate base models. The solution is clean, well-tested, and addresses the core issue effectively. 📋 Code Quality & Best Practices✅ Excellent:
✅ Follows TypeScript best practices:
🐛 Bug Analysis✅ No bugs identified:
The core fix (line 1084 in base-dialect.ts): computer = computedFields?.[fieldDef.originModel ?? model]?.[field];This correctly looks up computed fields in the base model when dealing with inherited fields. ⚡ Performance Considerations✅ Performance neutral or positive:
🔒 Security Assessment✅ No security concerns:
🧪 Test Coverage✅ Comprehensive test coverage:
Test highlights:
📚 Code Structure Improvements✅ Better code organization:
Response to Copilot Review CommentRegarding the concern about logic flow inversion: After reviewing the code, the new implementation is actually more correct. The key insight is that
This pattern is consistent with how Minor Observations
Conclusion✅ Recommended for approval This is a well-executed fix that:
The changes are minimal, focused, and address the root cause effectively. No security, performance, or code quality concerns identified. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/dialects/base-dialect.ts (2)
751-756: OrderBy helper picks origin alias but relation bases aren’t joinedWhen ordering by a field from a related model that originates from its delegate base, the alias for the base (e.g., Content) hasn’t been joined in the relation leftJoin branch below. This can produce invalid SQL (unknown table/alias) for inherited/computed fields on relations.
Add delegate-base joins for the relation model before recursing:
@@ } else { // order by relation const relationModel = fieldDef.type; if (fieldDef.array) { @@ } else { // order by to-one relation result = result.leftJoin(relationModel, (join) => { const joinPairs = buildJoinPairs(this.schema, model, modelAlias, field, relationModel); return join.on((eb) => this.and( ...joinPairs.map(([left, right]) => eb(this.eb.ref(left), '=', this.eb.ref(right))), ), ); }); + // ensure delegate bases of relation are joined for inherited/computed fields + { + let base = requireModel(this.schema, relationModel).baseModel; + while (base) { + result = this.buildDelegateJoin(relationModel, relationModel, base, result); + base = requireModel(this.schema, base).baseModel; + } + } result = this.buildOrderBy(result, fieldDef.type, relationModel, value, negated); } }
103-107: Add validation guard to prevent computed fields in cursor paginationThe
buildCursorFiltermethod references raw columns (${modelAlias}.${field}at line 185), which fails for computed fields since they don't exist as database columns. Add an early validation guard:if (args.cursor) { + // guard: cursor pagination cannot use computed fields in orderBy + const ob = args.orderBy ?? makeDefaultOrderBy(this.schema, model); + for (const [f] of ensureArray(ob).flatMap((o) => Object.entries<SortOrder>(o as any))) { + const fd = requireField(this.schema, model, f); + if (fd.computed) { + throw new QueryError('Cursor pagination by computed fields is not supported'); + } + } result = this.buildCursorFilter(model, result, args.cursor, args.orderBy, negateOrderBy, modelAlias); }All required utilities (
makeDefaultOrderBy,ensureArray,requireField,QueryError) are already imported in this file.
🧹 Nitpick comments (2)
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
1073-1092: fieldRef computed handler: solid, with two small improvements
- Type the handler for stronger guarantees and DX.
- Enhance the error to mention the resolved origin model.
- let computer: Function | undefined; + type ComputedHandler = (eb: ExpressionBuilder<any, any>, ctx: { modelAlias?: string }) => Expression<any>; + let computer: ComputedHandler | undefined; @@ - if (!computer) { - throw new QueryError(`Computed field "${field}" implementation not provided for model "${model}"`); - } + if (!computer) { + const resolvedModel = fieldDef.originModel ?? model; + throw new QueryError( + `Computed field "${field}" implementation not provided for model "${resolvedModel}"`, + ); + }tests/e2e/orm/client-api/computed-fields.test.ts (1)
238-274: Add test assertions for filtering and orderBy on inherited computed fields from delegate baseThe review comment's suggestions are valid and identify real coverage gaps. Current tests confirm:
orderByon computed fields exists but only for base models (lines 56, 64 showupperNameon User), not for inherited computed fields in delegate scenarios- No filtering (
whereclauses) on computed fields is tested in the computed-fields.test.ts suite- The new test (lines 238–274) validates the computed field's presence and correctness but lacks assertions for query operations (filtering, orderBy) on the inherited field
The suggested additions would harden coverage by testing:
- Filtering the sub model by inherited computed field (e.g.,
post.findMany({ where: { isNews: true } }))- OrderBy on inherited computed field from delegate base
- OrderBy involving joins with related models' inherited computed fields
These scenarios exercise distinct code paths and are not currently covered.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/runtime/src/client/crud/dialects/base-dialect.ts(2 hunks)packages/runtime/src/client/query-utils.ts(0 hunks)samples/blog/zenstack/schema.zmodel(1 hunks)tests/e2e/orm/client-api/computed-fields.test.ts(6 hunks)
💤 Files with no reviewable changes (1)
- packages/runtime/src/client/query-utils.ts
🧰 Additional context used
📓 Path-based instructions (3)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/client/crud/dialects/base-dialect.tssamples/blog/zenstack/schema.zmodeltests/e2e/orm/client-api/computed-fields.test.ts
**/schema.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
Name ZModel schema files
schema.zmodel
Files:
samples/blog/zenstack/schema.zmodel
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/orm/client-api/computed-fields.test.ts
🧬 Code graph analysis (2)
packages/runtime/src/client/crud/dialects/base-dialect.ts (2)
packages/runtime/src/client/query-utils.ts (1)
requireField(45-63)packages/runtime/src/client/errors.ts (1)
QueryError(18-22)
tests/e2e/orm/client-api/computed-fields.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(52-169)
⏰ 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: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: claude-review
🔇 Additional comments (10)
samples/blog/zenstack/schema.zmodel (1)
13-16: Comment-only tweak is fineThe note clarifies local sample constraints without affecting behavior. LGTM.
tests/e2e/orm/client-api/computed-fields.test.ts (6)
2-2: Vitest import: OKUsing vitest’s describe/it/expect directly is correct.
6-21: Non-optional computed field flow: OKGood end-to-end coverage for create/find/orderBy/aggregate/groupBy with a computed scalar.
94-130: Type-level check (non-optional): OKThe ts-expect-error guards assignment; compile-time intent is clear.
134-149: Optional computed field flow: OKHappy path for null-returning computed field is covered.
161-197: Type-level check (optional): OKCovers nullable assignment; consistent with runtime config.
200-236: Read-from-relation returns computed field: OKAsserts computed on the related author; aligns with new resolution.
packages/runtime/src/client/crud/dialects/base-dialect.ts (3)
141-147: Correct: filters now use fieldRef honoring originModelThis fixes filtering by inherited/computed fields across delegate bases. LGTM.
934-941: Select field recursion: OKSelecting via originModel for inherited fields is correct and avoids duplicating logic.
93-101: The proposed fix using fieldRef won't work for DISTINCT ON operations.The review correctly identifies that computed fields will break (they're not database columns), but the suggested solution is architecturally incorrect:
fieldRef incompatibility:
fieldRef()inlines computed fields using callback functions (line 1085 showscomputedFields?.[...][field]invocation). DISTINCT ON requires actual column references, not expressions. Inlining won't produce valid SQL.Inherited field logic flaw: The suggested code calls
fieldRef(fd.originModel, f, fd.originModel)which attempts to resolve the field in the wrong model context. This doesn't match the current query's model and table alias.Correct approach: Reject computed fields explicitly with a clear error message, allowing only regular/database columns for DISTINCT ON. The validator (
makeDistinctSchema) already allows computed fields to pass through—a runtime check rejecting them is needed.if ('distinct' in args && (args as any).distinct) { const distinct = ensureArray((args as any).distinct) as string[]; for (const f of distinct) { const fieldDef = requireField(this.schema, model, f); if (fieldDef.computed) { throw new QueryError(`Computed field "${f}" cannot be used in distinct`); } } if (this.supportsDistinctOn) { result = result.distinctOn(distinct.map((f) => this.eb.ref(`${modelAlias}.${f}`))); } else { throw new QueryError(`"distinct" is not supported by "${this.schema.provider.type}" provider`); } }Likely an incorrect or invalid review comment.
fixes #284
Summary by CodeRabbit
Improvements
Tests
Documentation