fix: using computed fields for orderBy and aggregations #155
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. WalkthroughCentralizes field reference construction by introducing BaseCrudDialect.fieldRef and replacing buildFieldRef usages across dialects and operations. Refactors group-by to use expressionBuilder and a unified aggregate helper. Expands aggregate operators in query-utils. Adds tests validating computed fields in filtering, ordering, and aggregations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 Pull Request Review Complete ✅Reviewing PR #155: "fix: using computed fields for orderBy and aggregations" Todo List
SummaryThis PR fixes the usage of computed fields in 🟢 Positive AspectsCode Quality & Architecture
Bug Fix Quality
Test Coverage
🟡 Areas for ImprovementMinor Code Issues
Documentation & Comments
🟢 Performance Considerations
🟢 Security Assessment
🔍 Specific Code Review
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes support for using computed fields in orderBy clauses and aggregation operations by replacing direct SQL field references with a new fieldRef method that properly handles both regular and computed fields.
- Introduces a
fieldRefmethod in the base dialect class that delegates to the existingbuildFieldRefutility - Updates orderBy operations to use computed field expressions instead of raw SQL references
- Modifies aggregation operations in group-by queries to properly handle computed fields
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/test/client-api/computed-fields.test.ts | Adds comprehensive test cases for orderBy and aggregation operations with computed fields |
| packages/runtime/src/client/query-utils.ts | Minor formatting change to the aggregate function signature |
| packages/runtime/src/client/crud/operations/group-by.ts | Updates group-by operations to use fieldRef method for proper computed field handling |
| packages/runtime/src/client/crud/operations/base.ts | Replaces buildFieldRef calls with dialect.fieldRef method calls |
| packages/runtime/src/client/crud/dialects/sqlite.ts | Updates SQLite dialect to use fieldRef method instead of buildFieldRef |
| packages/runtime/src/client/crud/dialects/postgresql.ts | Updates PostgreSQL dialect to use fieldRef method instead of buildFieldRef |
| packages/runtime/src/client/crud/dialects/base.ts | Introduces the fieldRef method and updates orderBy logic to handle computed fields |
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 review |
✅ Actions performedReview triggered.
|
![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
🔭 Outside diff range comments (1)
packages/runtime/src/client/crud/operations/base.ts (1)
1268-1278: Ensure computed fields cannot be updated via any path
The validator and CRUD layer currently do not reject updates to computed fields. This allows invalid SQL to be generated (e.g.dialect.fieldReffor a computed field). We need to block all update mutations against computed columns as early as possible.Please add a guard in
processScalarFieldUpdateData(in packages/runtime/src/client/crud/operations/base.ts) before any transform:private processScalarFieldUpdateData(model: GetModels<Schema>, field: string, data: any): any { const fieldDef = this.requireField(model, field); + // ✋ Disallow updating computed columns + if (fieldDef.computed) { + throw new QueryError(`Field "${field}" is computed and cannot be updated`); + } if (this.isNumericIncrementalUpdate(fieldDef, data[field])) { return this.transformIncrementalUpdate(model, field, fieldDef, data[field]);Additionally, exclude computed fields from the Zod update schema in
makeUpdateDataSchema(packages/runtime/src/client/crud/validator.ts) by adding at the top of the loop:Object.keys(modelDef.fields).forEach((field) => { - if (withoutFields.includes(field)) return; + const fieldDef = requireField(this.schema, model, field); + // ✋ Disallow computed fields entirely + if (fieldDef.computed) return; + if (withoutFields.includes(field)) return; if (fieldDef.relation) { // …These two changes ensure neither the validator nor the CRUD SQL generator allows writing to computed columns.
♻️ Duplicate comments (1)
packages/runtime/src/client/crud/operations/group-by.ts (1)
47-47: Reuse a single ExpressionBuilder for fieldRef to avoid unnecessary instantiationsCreating a new ExpressionBuilder for each fieldRef call adds overhead without benefit. Reuse one per query scope.
Apply this diff:
- const fieldRef = (field: string) => this.dialect.fieldRef(this.model, field, expressionBuilder(), '$sub'); + const sharedEb = expressionBuilder<any, any>(); + const fieldRef = (field: string) => this.dialect.fieldRef(this.model, field, sharedEb, '$sub');
🧹 Nitpick comments (2)
packages/runtime/src/client/query-utils.ts (1)
321-329: Add explicit return type to aggregate for API stability and chaining clarityType inference likely returns a Kysely ExpressionWrapper, but making it explicit avoids accidental type widening/regression and clarifies that the result supports .as().
Apply this diff:
-export function aggregate(eb: ExpressionBuilder<any, any>, expr: Expression<any>, op: AGGREGATE_OPERATORS) { +export function aggregate( + eb: ExpressionBuilder<any, any>, + expr: Expression<any>, + op: AGGREGATE_OPERATORS, +): ExpressionWrapper<any, any, unknown> { return match(op) .with('_count', () => eb.fn.count(expr)) .with('_sum', () => eb.fn.sum(expr)) .with('_avg', () => eb.fn.avg(expr)) .with('_min', () => eb.fn.min(expr)) .with('_max', () => eb.fn.max(expr)) .exhaustive(); }packages/runtime/src/client/crud/dialects/base.ts (1)
98-103: Consider adding null checks for computed field safety.While the logic correctly handles field origin, computed fields might have undefined
originModel. Consider adding explicit validation for computed fields.- const fieldRef = this.fieldRef( - fieldDef.originModel ?? model, - key, - eb, - fieldDef.originModel ?? modelAlias, - ); + const fieldRef = fieldDef.computed + ? this.fieldRef(model, key, eb, modelAlias) + : this.fieldRef( + fieldDef.originModel ?? model, + key, + eb, + fieldDef.originModel ?? modelAlias, + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/runtime/src/client/crud/dialects/base.ts(8 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(2 hunks)packages/runtime/src/client/crud/dialects/sqlite.ts(2 hunks)packages/runtime/src/client/crud/operations/base.ts(3 hunks)packages/runtime/src/client/crud/operations/group-by.ts(5 hunks)packages/runtime/src/client/query-utils.ts(1 hunks)packages/runtime/test/client-api/computed-fields.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/runtime/src/client/query-utils.ts (2)
packages/sdk/src/schema/expression.ts (1)
Expression(1-10)packages/runtime/src/client/constants.ts (2)
AGGREGATE_OPERATORS(29-29)AGGREGATE_OPERATORS(30-30)
packages/runtime/src/client/crud/operations/base.ts (1)
packages/runtime/src/client/crud/dialects/base.ts (1)
fieldRef(996-998)
packages/runtime/src/client/crud/dialects/base.ts (2)
packages/runtime/src/client/query-utils.ts (2)
aggregate(321-329)buildFieldRef(157-179)packages/runtime/src/client/constants.ts (2)
AGGREGATE_OPERATORS(29-29)AGGREGATE_OPERATORS(30-30)
packages/runtime/src/client/crud/operations/group-by.ts (2)
packages/runtime/src/client/crud/dialects/base.ts (1)
fieldRef(996-998)packages/runtime/src/client/query-utils.ts (1)
aggregate(321-329)
⏰ 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 (11)
packages/runtime/src/client/crud/dialects/postgresql.ts (1)
229-229: LGTM: Centralizing field references through dialect.fieldRefGood move replacing buildFieldRef with this.fieldRef. This keeps computed-field logic consistent at the dialect layer and aligns with the PR’s objective.
Also applies to: 252-252
packages/runtime/test/client-api/computed-fields.test.ts (1)
40-91: Great coverage for computed fields across where/orderBy/aggregate/groupByThese assertions exercise the critical paths introduced by fieldRef and aggregate. They should prevent regressions for computed-field support.
packages/runtime/src/client/crud/operations/base.ts (1)
189-191: LGTM: Distinct fallback now correctly uses dialect.fieldRefUsing the dialect-aware fieldRef here ensures distinct-on emulation works even for computed fields.
packages/runtime/src/client/crud/operations/group-by.ts (1)
91-101: LGTM: Aggregations now use the centralized aggregate helperThis simplifies branching and keeps aggregate operator handling consistent across call sites, aligning with the new AGGREGATE_OPERATORS set.
packages/runtime/src/client/crud/dialects/base.ts (5)
3-3: LGTM! Clean import consolidation.The addition of
expressionBuilderto the imports is well-placed and follows the existing import organization pattern.
755-757: Good use of expressionBuilder for computed field resolution.The usage of
expressionBuilder()here correctly enables computed field resolution in orderBy clauses, which is a key improvement for supporting computed fields in sorting operations.
868-868: LGTM! Consistent field reference handling.The use of
fieldReffor computed fields in SELECT operations ensures proper resolution through the centralized mechanism.
996-998: Excellent centralization of field reference logic.The new
fieldRefmethod effectively centralizes field reference construction, delegating tobuildFieldRefwith appropriate schema and options. This abstraction improves maintainability and consistency across the codebase.
728-730: Aggregate operator handling is fully exhaustive and type-safeThe
aggregatefunction inpackages/runtime/src/client/crud/dialects/base.tscovers every member ofAGGREGATE_OPERATORSvia a ts-patternmatch(...).exhaustive(). The union is declared as:// packages/runtime/src/client/constants.ts export const AGGREGATE_OPERATORS = [ '_count', '_sum', '_avg', '_min', '_max' ] as const; export type AGGREGATE_OPERATORS = (typeof AGGREGATE_OPERATORS)[number];Since the pattern match covers all five operators and ends with
.exhaustive(), any invalid string cast toAGGREGATE_OPERATORSwould be caught at runtime with an exception. Castingfield as AGGREGATE_OPERATORSis safe here provided thatfieldcan only be one of the five literals.No changes required.
packages/runtime/src/client/crud/dialects/sqlite.ts (2)
173-173: LGTM! Consistent migration to centralized fieldRef.The replacement of
buildFieldRefwiththis.fieldRefcorrectly maintains the same functionality while using the new centralized field reference method.
202-202: Type casting maintained correctly.The
as ArgsTypecast is properly preserved after migrating tothis.fieldRef, ensuring type compatibility within the context.
Summary by CodeRabbit
New Features
Refactor
Tests