Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .changeset/client-find-pagination-presence.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
---
"@objectstack/client": patch
---

fix(client): `data.find()` emits `top`/`skip` on presence, so `limit: 0` reaches the server (#6485)

Both `find` implementations — `ObjectStackClient.data.find` and its
byte-identical `ScopedProjectClient.data.find` copy — emitted the two pagination
transport params on **truthiness**:

```ts
if (normalizedOptions.top) queryParams.set('top', normalizedOptions.top.toString());
if (normalizedOptions.skip) queryParams.set('skip', normalizedOptions.skip.toString());
```

while the canonical normalizer ten lines above already tested **presence**
(`if (v2.limit != null) normalizedOptions.top = v2.limit`). So `0` survived the
normalizer and was then discarded by the emitter. Both now test presence, in
both copies.

**What changes on the wire, and why that is the fix rather than a preference.**
`find('task', { limit: 0 })` — and equally `{ top: 0 }` — used to reach the
server with **no `top` param at all**. The GET list route has no default page
size, so an absent `top` returns the *entire* match set: the caller who asked
for no records received every record, under HTTP 200 with no warning.

The direction was measured before the change rather than assumed, because a
client fix is only worth having if the server honours what it sends:

| layer | `top=0` |
|:---|:---|
| REST list route → `ObjectStackProtocolImplementation.findData` | not rejected, not ignored — folds `top` into `limit`, coerces `Number('0')`, forwards `{ limit: 0 }` to the engine; envelope reports `total: 0, hasMore: false` |
| `SqlDriver.find` (the driver behind the default file-backed SQLite datasource, and every Postgres/MySQL deployment) | paginates on presence — `LIMIT 0`, **zero rows** |
| `TursoRemoteTransport` | presence — `LIMIT ?` bound to `0`, zero rows |

So `limit: 0` now means "return no records" end to end, which is what the
canonical branch already implied.

**`offset: 0` / `skip: 0` were dropped too, and that half is a consistency
change with no behavioural consequence** — `skip=0` is already the server's
default, so the request means the same thing whether the param is sent or not.
They are aligned because one emitter must not hold two rules for one pair, not
because a wrong answer was being returned.

Callers passing a non-zero `limit`/`top`/`offset`/`skip`, or omitting them
entirely, are unaffected — the emitted query string is byte-identical.
56 changes: 56 additions & 0 deletions packages/client/src/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1337,6 +1337,40 @@ describe('data.find() — canonical/legacy transport parameters (both copies)',
// change that fixes one vocabulary by breaking the other goes red.
'legacy single key: { top }': { options: { top: 20 }, wire: 'top=20' },
'legacy single key: { skip }': { options: { skip: 5 }, wire: 'skip=5' },

// ── #6485: ZERO IS A VALUE, NOT AN ABSENCE ──────────────────────────
//
// The two pagination params were emitted on TRUTHINESS
// (`if (normalizedOptions.top)`) while the canonical branch ten lines
// above already normalized them on PRESENCE (`if (v2.limit != null)`).
// So `0` survived the normalizer and was then discarded by the
// emitter, in both copies.
//
// `limit: 0` is the half that changes the answer, and the direction is
// measured, not assumed. Through the REST list route
// (`ObjectStackProtocolImplementation.findData`) `top=0` is neither
// rejected nor ignored: it folds to `limit: 0` and reaches the engine,
// and `SqlDriver.find` — the driver behind the default file-backed
// SQLite datasource — applies pagination on presence
// (`if (query.limit !== undefined) b.limit(query.limit)`), so the
// statement carries `LIMIT 0` and answers with zero rows. Dropping the
// param instead did NOT mean "the server's default page": this route
// has no default page size, so an absent `top` returns the ENTIRE
// match set. `find('task', { limit: 0 })` therefore answered with every
// record when it asked for none — HTTP 200, no warning.
//
// `offset: 0` / `skip: 0` are the consistency half: `skip=0` is already
// the server's default, so sending it or omitting it means the same
// thing. They are pinned here because the emitter must not have two
// rules for one pair, not because the wire meaning changed.
'canonical zero: { limit: 0 }': { options: { limit: 0 }, wire: 'top=0' },
'canonical zero: { offset: 0 }': { options: { offset: 0 }, wire: 'skip=0' },
'legacy zero: { top: 0 }': { options: { top: 0 }, wire: 'top=0' },
'legacy zero: { skip: 0 }': { options: { skip: 0 }, wire: 'skip=0' },
'canonical zero pair: { limit: 0, offset: 0 }': {
options: { limit: 0, offset: 0 },
wire: 'top=0&skip=0',
},
'canonical: where + limit': {
options: { where: { contact_id: 'c1' }, limit: 20 },
wire: 'top=20&contact_id=c1',
Expand Down Expand Up @@ -1376,6 +1410,28 @@ describe('data.find() — canonical/legacy transport parameters (both copies)',
expect(canonical.scoped).toBe(legacy.scoped);
});

/**
* [#6485] `{ limit: 0 }` and `{}` are DIFFERENT REQUESTS, and the wire has
* to be able to tell them apart.
*
* The table rows above pin each spelling's exact query string. This asserts
* the property those rows exist for: the two bags must not collapse onto
* one wire. Stated as an inequality rather than as two more literals
* because the defect was precisely a collapse — `top` absent in both cases,
* so the caller who asked for no records and the caller who asked for
* everything sent byte-identical requests and got byte-identical answers.
*
* Both copies, one property: a fix landing on only one of them leaves the
* other's pair equal and this goes red.
*/
it('`{ limit: 0 }` is distinguishable from `{}` on the wire, on both copies', async () => {
const zero = await driveBoth({ limit: 0 });
const absent = await driveBoth({});

expect(zero.direct).not.toBe(absent.direct);
expect(zero.scoped).not.toBe(absent.scoped);
});

/**
* A nested per-relation query inside `expand` has no spelling on a GET, so
* it is REFUSED rather than trimmed away — trimming would send a wider
Expand Down
25 changes: 21 additions & 4 deletions packages/client/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4221,8 +4221,23 @@ export class ObjectStackClient {
}

// 1. Handle Pagination
if (normalizedOptions.top) queryParams.set('top', normalizedOptions.top.toString());
if (normalizedOptions.skip) queryParams.set('skip', normalizedOptions.skip.toString());
//
// [#6485] PRESENCE, not truthiness — the same test the canonical
// normalizer directly above already applies (`if (v2.limit != null)`).
// Emitting on truthiness made `0` survive the normalizer and then be
// discarded here, so `find('task', { limit: 0 })` reached the server
// with no `top` param. The GET list route has no default page size, so
// an absent `top` returns the ENTIRE match set: the caller who asked
// for no records got every record, under a 200 with no warning.
// `top=0` is honoured end to end — the protocol normalizer folds it to
// `limit: 0` and forwards it, and `SqlDriver.find` paginates on
// presence too, so the statement carries `LIMIT 0` and answers empty.
// `skip=0` is a consistency change only: it already equals the
// server's default, so the request means the same either way — but one
// emitter must not hold two rules for one pair.
// Mirrored verbatim in `ScopedProjectClient.data.find`.
if (normalizedOptions.top != null) queryParams.set('top', normalizedOptions.top.toString());
if (normalizedOptions.skip != null) queryParams.set('skip', normalizedOptions.skip.toString());

// 2. Handle Sort
if (normalizedOptions.sort) {
Expand Down Expand Up @@ -4905,8 +4920,10 @@ export class ScopedProjectClient {
Object.assign(normalizedOptions, options);
}

if (normalizedOptions.top) queryParams.set('top', normalizedOptions.top.toString());
if (normalizedOptions.skip) queryParams.set('skip', normalizedOptions.skip.toString());
// [#6485] Presence, not truthiness — see the twin in
// `ObjectStackClient.data.find` for why `0` must reach the wire.
if (normalizedOptions.top != null) queryParams.set('top', normalizedOptions.top.toString());
if (normalizedOptions.skip != null) queryParams.set('skip', normalizedOptions.skip.toString());
if (normalizedOptions.sort) {
if (Array.isArray(normalizedOptions.sort) && typeof normalizedOptions.sort[0] === 'object') {
queryParams.set('sort', JSON.stringify(normalizedOptions.sort));
Expand Down
Loading