Fixes #15353 fix(framework, medusa, settings, types): prevent sorting orders by computed fields that have no DB column#15354
Conversation
… sorting orders by computed fields that have no DB column
🦋 Changeset detectedLatest commit: ed62371 The changes in this PR will be included in the next version bump. This PR includes changesets to release 78 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@Ultron03 is attempting to deploy a commit to the medusajs Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for your contribution, @Ultron03! 🎉 After an initial review, this PR looks good to us. Here's a summary: ✅ PR template is complete The approach mirrors the Notes (non-blocking, no action required):
A team member will do a final review before this is merged. We appreciate your patience! Triggered by: PR description updated |
|
Thank you for your contribution, @Ultron03! 🎉 After an initial review, this PR looks good to us. Here's a summary: ✅ PR template is complete The implementation cleanly mirrors the A team member will do a final review before this is merged. We appreciate your patience! Triggered by: PR description updated |
|
Thank you for your contribution, @Ultron03! 🎉 After an initial review, this PR looks good to us. Here's a summary: ✅ PR template is complete The implementation is solid: the API-layer guard in Notes (non-blocking):
A team member will do a final review before this is merged. We appreciate your patience! Triggered by: new PR opened |
|
Thank you for your contribution, @Ultron03! 🎉 After an initial review, this PR looks good to us. Here's a summary: ✅ PR template is complete The implementation cleanly mirrors the Notes (non-blocking): The error message in A team member will do a final review before this is merged. We appreciate your patience! Triggered by: PR description updated |
|
Thank you for your contribution, @Ultron03! After reviewing this PR, we need one thing addressed before we can move forward: Required changes:
Explanation: The most recent commit on After rebasing, the function signature should look like: function processEntityType(
schemaTypeMap: SchemaTypeMap,
typeName: string,
entity: DiscoveredEntity,
columns: ViewConfigurationColumn[],
processedFields: Set<string>,
filterRules: FieldFilterRules,
defaultVisibleFields: string[],
fieldOrdering: Record<string, number>,
nonFilterableFields: string[], // from #15262 — must be kept
propertyLabels?: Map<string, PropertyLabel>,
parentPath: string = "",
nonSortableFields: string[] = [] // your addition
): void {and both call sites in Everything else in the PR looks solid: the approach is correct, the changeset is properly formatted, the PR template is complete, and the unit + integration tests are thorough. Once the rebase is done this should be in good shape. Triggered by: PR description updated |
…s#15262 and used generic sort error message
|
Thank you for your contribution, @Ultron03! After reviewing the updated commit, the rebase issue flagged in the previous review is still present. Required changes:
Explanation: The diff still includes re-adding code that #15262 already merged into
Merging as-is will either produce conflicts or, if auto-merged, duplicate these symbols (duplicate interface properties are a TypeScript compile error; duplicate registry merges and imports are dead code). After a proper One positive update: the error message was correctly changed from Everything else — the approach, the Triggered by: new commit pushed |
|
Thank you for your contribution, @Ultron03! Good progress since the last review — the There is one remaining issue to fix before this is ready: Required changes:
Explanation: The current
- propertyLabels
+ nonFilterableFields,
+ propertyLabels,
+ "",
+ nonSortableFieldsBut - propertyLabels
+ propertyLabels,
+ "",
+ nonSortableFieldsAfter rebasing on the current Minor note (non-blocking): The changeset description still says Once the rebase is done, this should be ready. Triggered by: new commit pushed |
…le-computed-fields
Fixes #15353 fix(framework, medusa, settings, types): prevent sorting orders by computed fields that have no DB column
Summary
What — What changes are introduced in this PR?
Adds a nonSortableFields mechanism that prevents sorting list endpoints by fields that have no backing database column. Applied immediately to GET /admin/orders, blocking total, fulfillment_status, and payment_status as sort keys. Requests using these fields now receive a clear 400 INVALID_DATA response. The admin column API (generateEntityColumns) is also updated to return sortable: false for those columns so the configurable orders table never renders sort affordances for them.
Why — Why are these changes relevant or necessary?
Sorting by these fields reaches MikroORM with a key that does not correspond to any column in the Order table, causing an uncaught server crash:
Error: Trying to order by not existing property Order.total
total, fulfillment_status, and payment_status are computed values derived from related modules at query time — they are not physical columns and cannot be used as sort targets at the database level. This fix mirrors the approach taken in #15262 for filtering, which applied the same pattern to mark these fields as non-filterable.
How — How have these changes been implemented?
processEntityType, which sets sortable: false for those columns.
Testing — How have these changes been tested, or how can the reviewer test the feature?
Unit tests:
correctly.
Integration tests:
currency_code) remain sortable: true.
DESC) return 400 with the exact error message Order field is not sortable.
Manual repro:
Before fix — crashes with 500
curl "http://localhost:9000/admin/orders?order=total" -H "Authorization: Bearer "
After fix — returns 400
{ "message": "Order field total is not sortable", "type": "invalid_data" }
Checklist
Please ensure the following before requesting a review:
yarn changesetand follow the prompts