Skip to content

feat: apply the sort query parameter - #405

Merged
mvantellingen merged 2 commits into
mainfrom
feat/query-sort
Aug 11, 2026
Merged

feat: apply the sort query parameter#405
mvantellingen merged 2 commits into
mainfrom
feat/query-sort

Conversation

@mvantellingen

Copy link
Copy Markdown
Member

The problem

sort is accepted and typed on QueryParams (src/storage/abstract.ts:32) but never applied. query() goes from where-filtering straight to resources.slice(offset, offset + limit), so results come back in insertion order.

Where that bites is cursor paging:

sort: "id asc"
where: `id > "${lastId}"`

which is how consumers walk a collection larger than one page — iterateProductPages, getStores and similar helpers in downstream projects all use it. Against the mock the cursor is taken from the last row of an unsorted page, so it is not the maximum id seen and the next query skips resources. The same code is correct against the real API, so a test written against the mock passes or fails for the wrong reason. I hit both while writing a paging-heavy feature and had to demote the data-level assertions to request-level ones.

The fix

src/lib/sortParser.tsparseSortClauses + applySort, called from both the in-memory and sqlite backends after where and before offset/limit.

Supports field, field asc, field desc, dot-separated paths (name.en-GB), and several clauses passed as an array (which is how a repeated sort parameter arrives). Numbers compare numerically — "10" < "2" as strings is the trap. Everything else compares as a string, which orders ISO timestamps correctly. Stable, so a secondary clause only breaks ties from the first.

Two decisions worth a look

Strings compare with < / >, not localeCompare. predicateParser compares with those same operators (src/lib/predicateParser.ts:313). If sorting and the where predicate disagreed on ordering, a cursor could step over resources — the exact bug being fixed here. There is a test asserting ["A", "B", "a", "b"] and that each element is > its predecessor.

A missing value counts as larger than any present one, so it sorts last ascending and first descending, as SQL does. I could not verify commercetools' actual rule for this, so I picked the one that yields a total, deterministic order and documented it rather than guessing at fidelity. Happy to change it if someone knows the real behaviour.

Testing

  • src/lib/sortParser.test.ts — 17 tests: parsing, directions, dot paths, numeric vs string comparison, missing values, tie-breaking, stability, no input mutation, predicate agreement, ISO timestamps.
  • src/storage/storage.test.ts — 6 tests in the existing query block, including one that pages 25 categories in fours with a cursor and asserts all 25 are seen exactly once, in order. Runs against both storage backends.
  • All 784 tests pass (761 pre-existing, 23 new). biome check && tsc clean.

Changeset included as a minor.

`sort` was accepted and typed on QueryParams but never used: query() went from
where-filtering straight to slice(offset, limit), so results came back in
insertion order.

That matters most for cursor paging — `sort: "id asc"` with
`where: 'id > "<last>"'`, which is how consumers walk a collection larger than
one page. Against the mock the cursor is taken from an unsorted page, so
resources are silently skipped; the same code is correct against the real API.
Tests written against the mock therefore pass or fail for the wrong reason.

Handles `field`, `field asc`, `field desc`, dot paths and several clauses as an
array. Two choices worth noting:

- Strings compare with `<`/`>`, not localeCompare, because predicateParser
  compares with those same operators. If sort and the where predicate disagreed
  on ordering, a cursor could step over resources — the exact bug this fixes.
- A missing value counts as larger than any present one, so it sorts last
  ascending and first descending, as SQL does. Commercetools' own rule for this
  is unverified; what a cursor needs is only that the order is total.

Applied in both the in-memory and sqlite backends, before offset/limit, and
stable so a secondary clause only breaks ties.
@changeset-bot

changeset-bot Bot commented Aug 11, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2b7b1ce

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@labdigital/commercetools-mock Minor

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

The new "leaves order untouched without a sort" case asserted insertion order,
which only the in-memory backend gives: sqlite returns rows by id, so it read
cat-1, cat-10, cat-11 and the sqlite matrix leg failed.

Order without a sort is the backend's business. Assert only what both backends
guarantee — repeating a query does not reshuffle it. The "applySort returns the
input unchanged" behaviour is already covered in sortParser.test.ts.
@mvantellingen
mvantellingen merged commit c512dd2 into main Aug 11, 2026
10 checks passed
@mvantellingen
mvantellingen deleted the feat/query-sort branch August 11, 2026 20:19
@github-actions github-actions Bot mentioned this pull request Aug 11, 2026
mvantellingen added a commit that referenced this pull request Aug 11, 2026
ProductProjectionRepository.query() does not delegate to
AbstractStorage.query(): it reads all products, transforms them to projections,
then filters, applies price selection, expands and slices. So it never picked up
the sort support added in #405, even though ProductProjectionQueryParams
declares the parameter.

That left the endpoint most likely to be walked with a cursor still returning
insertion order, which is the case #405 set out to fix. It is the only repository
with its own query path; everything else goes through the storage layer.

Five tests through the HTTP layer, including a cursor-paged walk that visits
every published product exactly once. Verified they fail without the fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant