CenterForDigitalHumanities / CenterForDigitalHumanities/rerum_server_nodejs

`/query` paginates with no sort, so page boundaries rest on MongoDB natural order

Open
#300 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
3
Forks
6
Avg merge
1h 25m
Merged PRs (30d)
3

Description

## Summary

`/query` pages with `db.find(props).limit(limit).skip(skip)` (`controllers/crud.js:87`) and applies no `sort`. `HEAD /query` does the same (`controllers/history.js:91`). Without a sort, `skip` counts into MongoDB's natural order, which the server makes no guarantee about and which the client cannot reason about.

In practice it holds today, because RERUM marks deletions rather than removing documents, so the collection is close to append-only. That is a property of the data policy, not a promise the API makes, and it is not something a client can rely on across a compaction, an index change, a resharding, or a storage-engine upgrade.

This matters now rather than later because both of the paging improvements that follow are built on the assumption that consecutive pages tile a stable total order. `Link: rel="next"` is only honest if page boundaries are deterministic, and keyset cursors require an actual sort key. This issue is the prerequisite for both.

## Why this matters

**Paging correctness rests on an accident.** Two clients making the same paged walk minutes apart are not guaranteed to see the same page boundaries, and no error surfaces when they do not. A document can be skipped or returned twice with nothing distinguishing that from correct behavior.

**It blocks the fixes.** `rel="next"` promises "there is another page, and here is where it starts." Under natural order, that promise is only as stable as the storage layer happens to be. Cursor-based continuation (#303) is stronger still: a keyset cursor is literally a position in a sort order, so it cannot be built without one.

**It is a silent divergence from `/search`.** `/search` intends a relevance order (it does not currently achieve one; see #307). `/query` intends nothing. Two endpoints under the same `getPagination()` contract should both be able to state what order they page in.

## Evidence

Verified 2026-09-02 and re-run 2026-09-03, read-only.

`/query` does tile correctly today. Five sequential `limit=100` pages from `skip=50000` return the same 500 ids in the same order as one `limit=500` request, and repeating a request returns the same page. So this is a latent correctness issue and a blocker for the follow-on work, not an observed failure.

The absence is in the source: `controllers/crud.js:87` and `controllers/history.js:91` contain no `sort` call, and no other layer supplies one.

## Affected lines

| File | Line | Current |
|------|------|---------|
| `controllers/crud.js` | 87 | `db.find(props).limit(limit).skip(skip)` — no sort |
| `controllers/history.js` | 91 | Same shape for `HEAD /query` |
| `controllers/gog.js` | 36, 167 | Aggregation-based; confirm the same ordering guarantee applies |

## Proposed change

Sort ascending on `_id`:

```js
let matches = await db.find(props).sort({ _id: 1 }).limit(limit).skip(skip).toArray()
```

`_id` is always indexed, is unique, and is the natural cursor key for the follow-on work, so this is the cheapest total order available and the only one that does not require a new index.

Two things to verify before merging:

- **Query planning.** Run `explain()` on representative `/query` bodies with an unindexed `props` filter to confirm Mongo chooses an `_id` index scan rather than a collection scan followed by a blocking in-memory sort. A blocking sort would trade a latent correctness problem for a real performance one, and the dev collection is large enough to show it.
- **Mixed `_id` types.** `newID()` returns `new ObjectId().toHexString()`, so modern RERUM `_id` values are **strings**. Legacy v0 documents on the dev collection carry embedded-object `_id` values (roughly 6400 of them; production has none). BSON type ordering sorts strings and embedded objects into separate, deterministically ordered blocks. The result is still a total order and paging is still correct, but the ordering is not "oldest to newest" and should not be documented as if it were.

Apply the same sort to `HEAD /query` so the two verbs page over the same order.

Then state the ordering guarantee in `public/API.html` and in `openapi/contracts/core-provider.openapi.yaml`. An order clients cannot read about is not much better than no order.

## Notes

- **Breaking in the loosest sense.** Result order changes for every existing `/query` client. Nothing documented promises the current order, but a client relying on it will notice. Land on dev first and soak.
- Prerequisite for #302 and #303.
- Sorting on `_id` is the recommendation from the parent draft ([the detailed report on #299](https://github.com/CenterForDigitalHumanities/rerum_server_nodejs/issues/299#issuecomment-5529634101), "What is working correctly"), reached independently.
- If `explain()` shows a blocking sort on common query shapes, say so on this thread before merging. The alternative is a compound index, which is a larger decision than this issue should make on its own.

## Acceptance criteria

- [ ] `/query` and `HEAD /query` both sort ascending on `_id`
- [ ] `explain()` output for representative query bodies is posted on the issue, confirming no blocking in-memory sort
- [ ] Sequential paged results still equal a single larger request, in the same order
- [ ] Paging is verified across mixed `_id` types on the dev collection, including the legacy embedded-object documents
- [ ] The ordering guarantee is stated in `public/API.html` and in the OpenAPI contract

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.