Uh oh!
There was an error while loading. Please reload this page.
fix(orm): support parameterized computed fields in aggregation orderBy - #2784
Merged
Conversation
The zod schema for groupBy orderBy accepts `{ args, sort, nulls? }` for a
parameterized computed field under `_avg`/`_sum`/`_min`/`_max`, but the
dialect's aggregation order-by path asserted a plain 'asc'/'desc' value and
dropped the computed field's args, crashing with an invariant error on
input that passed validation.
- accept `{ sort, nulls?, args? }` objects in `applyAggregationOrderBy`,
forward `args` to the field ref builder, support `nulls` via
`buildOrderByField`, and throw an invalid-input error for bad sort values
- fix `SumAvgInput`/`MinMaxInput` typing so a parameterized computed field
in orderBy position carries `{ sort, nulls? }` alongside `args` (the
aggregate-args context is unchanged)
- add e2e coverage for groupBy ordered by aggregated parameterized
computed fields
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughParameterized computed fields in aggregate and group-by ordering now accept sort direction, optional NULL ordering, and query-time arguments. Dialect handling validates and applies these values. End-to-end tests cover ChangesParameterized aggregate ordering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/orm/src/client/crud/dialects/base-dialect.ts`:
- Around line 1290-1292: Update the orderBy payload validation near the
Object.entries loop to require a non-null plain object, rejecting null and
arrays with createInvalidInputError before iteration. Preserve valid aggregate
object handling and the existing invalid-input error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c4da15a-809c-49a7-b69a-42e287da8a86
📒 Files selected for processing (3)
packages/orm/src/client/crud-types.tspackages/orm/src/client/crud/dialects/base-dialect.tstests/e2e/orm/client-api/computed-fields.test.ts
Uh oh!
There was an error while loading. Please reload this page.
… input error Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ordering
groupByresults by an aggregated parameterized computed field (orderBy: { _sum: { field: { args, sort } } }) passed input validation but crashed with an internal invariant error, and the computed field'sargswere never forwarded.The zod schema for
groupByorderBy validates each_avg/_sum/_min/_maxentry against the full order-by schema, which accepts{ args, sort, nulls? }for a parameterized computed field — butapplyAggregationOrderByasserted a plain'asc'/'desc'value and its localbuildFieldRefsignature omittedcomputedArgs.Changes
applyAggregationOrderBynow accepts either a plain sort order or an object carryingsort, optionalnulls, and the computed field'sargs; forwardsargsto the field ref builder; supportsnullsvia the existingbuildOrderByField; and throws an invalid-input error (instead of an invariant) for bad sort values.SumAvgInput/MinMaxInputin orderBy position now type a parameterized computed field as{ args, sort, nulls? }— previously{ args }alone, with no way to express direction (mismatching the zod schema, which requiressort). The aggregate-args context ({ args }alone) is unchanged.groupByordered by_sum/_minof a parameterized computed field, including that differentargsproduce different orderings and that omittingsortis rejected by validation.Testing
Invariant failed: invalid orderBy value for field "_sum"before the fix.computed-fields.test.ts(20 tests) and thegroup-by/aggregate/find/zode2e suites (203 tests) all pass;tsc --noEmiton the ORM package is clean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes