fix(client): derive data.find's canonical-key predicate from QueryOptionsV2, and map expand to the wire - #6480
Merged
os-project-manager merged 1 commit intoAug 8, 2026
Conversation
…ionsV2 (#6322) `data.find()` picked its canonical-vs-legacy branch with a hand-written condition naming four keys. That condition restated what `QueryOptionsV2` declares, and had fallen behind it twice: `limit` was absent, so `find('task', { limit: 20 })` fell to the legacy branch and reached the server with an empty query string (default page size, HTTP 200, no warning), while its twin `{ offset: 5 }` worked; and `expand` was absent AND unmapped on both branches, so it never reached the wire at all. The predicate is now `Exclude<keyof QueryOptionsV2, keyof QueryOptions>` held as a `Record<…, true>` — a new canonical key is a compile error until listed. `expand` maps to `?expand=<comma-separated names>`, the spelling `HttpFindQueryParamsSchema` declares for the GET list route; a nested per-relation query, which has no GET spelling, is refused rather than silently trimmed. Both `find` copies (ObjectStackClient / ScopedProjectClient) read the one shared predicate and mapping, and are driven through one expectation table in the tests so a future fork goes red. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uFVNMmTxLpmfQYiuKM1Yx
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 14 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-project-manager
marked this pull request as ready for review
August 8, 2026 01:49
os-project-manager
deleted the
claude/issue-6322-client-find-canonical-keys
branch
August 8, 2026 02:06
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #6322
What was wrong
data.find()accepts two vocabularies — canonicalQueryOptionsV2and legacyQueryOptions— and chose its branch with a hand-written condition naming four keys:duplicated byte-for-byte in both implementations (
ObjectStackClient.data.find,ScopedProjectClient.data.find). That condition is a second, independent statement of whatQueryOptionsV2declares, and it had already fallen behind the interface twice.A.
{ limit: N }was silently dropped.limitwas not one of the four, soclient.data.find('task', { limit: 20 })— a canonical key as the only key, and the most natural spelling of "first 20" — fell to the legacy branch. That branch reads onlytop/skip/sort/select/filter/filters/aggregations/groupBy; nothing there readslimit. The request went out with an empty query string: server default page size, HTTP 200, no warning. Its pagination twin{ offset: 5 }worked, becauseoffsethappened to be listed — one interface, two pagination keys, opposite behaviour.B.
expandwas declared but never delivered. Declared onQueryOptionsV2and documented as replacing a legacypopulatethatQueryOptionsnever had — carried by neither branch. Not one character reached the wire, on either copy.Premise verification (against
origin/maineb7613c)The issue verified against
a682670; both halves still hold, and the PM's server-side mechanism assumption forexpandholds as well:packages/spec/src/api/protocol.zod.ts—HttpFindQueryParamsSchemadeclaresexpandon the GET list route as "Comma-separated list of lookup/master_detail field names to expand. Resolved to populate array and passed to the engine for batch $in expansion."packages/metadata-protocol/src/protocol.ts(findData) splits that string on commas and folds each name into{ [name]: { object: name } }, then gates it throughassertExpandTargetsExist.packages/rest/src/rest.test.ts(findData handler expand/populate forwarding) drives exactlyquery: { expand: 'order,product', top: '10' }.So the accepted wire spelling is a comma-joined relation-name list. Nothing was invented.
What changed
One predicate, derived from the interface. The canonical-only key set is now computed, not restated:
TypeScript rejects this object if a canonical-only key is missing and rejects it if a non-canonical-only key is present, so the next key added to
QueryOptionsV2is a compile error here until it is listed.aggregations/groupByfall out automatically — both options types declare them, so shared vocabulary cannot discriminate. Appendinglimitto the old list would have been the third round of the same mistake, which is why the ruling forbade it.expandmapped to the spelling the server accepts —expand=contact,owner. TheRecordform contributes its keys, which are the same relation names the server derives from the comma list, so that form maps losslessly too.Both copies read the one shared predicate and the one shared mapping.
Judgement call worth a maintainer's eye
A nested per-relation query (
expand: { contact: { fields: ['name'] } }) has no spelling on a GET. Trimming it away would send a wider read than the caller asked for and say nothing — the same silent-drop defect this PR exists to close, and the one the engine itself refused insideexpandin #4371. So it is now refused with an error naming the relation and the keys it could not carry, pointing atdata.query()(which carries a QueryAST body and is where nested expand detail belongs).This is the only behaviour in the PR that turns a silent no-op into a throw. Blast radius measured: the
Recordform has zero callers anywhere in the workspace, and it never worked, so nothing that functions today starts failing. Flagging it explicitly rather than burying it — the alternative reading (narrowQueryOptionsV2.expandtostring[]and drop theRecordform under ADR-0049) is a public-type call I did not make unilaterally.Reverse verification — direction predicted before running
Predicted: the rows for the two defects go red on unmodified source, the rows pinning the already-correct translation layer stay green. Ran the new table against
origin/main'sindex.tswith the new tests in place — 6 failed / 253 passed, and the six were exactly the predicted set:with
{ limit: 20 }reportingexpected '' to be 'top=20'— the empty query string from the issue's measurement table, reproduced.{ offset: 5 },{ where },{ fields },{ orderBy },{ top },{ skip }and both full five-key rows were green before and after: the translation layer itself was always correct, and this PR does not touch it.Tests — one table, both copies
Per the multi-face test clause — 测试放在未来的分叉会被抓住的地方,不是放在一个独立测试文件里 — the cases live in
packages/client/src/client.test.tsbeside the existingfindcoverage, and every row is driven through both implementations against the same expected query string, so a future edit landing on only one copy goes red.Expectations are exact full query strings, not
toContainsubstrings: the defect was a param that never appeared at all, and a substring assertion on the params that did appear stays green through exactly that. This supersedes the two previousQueryOptionsV2tests, which assertedtoContainon one copy only.The single-key sweep is typed
Record< CanonicalOnlyKey, TransportRow >withCanonicalOnlyKeyrecomputed in the test from the two exported interfaces — so a new canonical key is a compile error in the test too, until someone states what it puts on the wire.The refusal case is a client-side pre-flight guard, so there is no ADR-0112
code/statusenvelope to assert. Two independent bits are asserted instead: the message names the offending relation and the nested keys it could not carry, and no request was issued — so a refusal firing after the read went out, or one resolving instead of throwing, both go red.Verification run
pnpm --filter @objectstack/client test— 259 passed (259), 20 files.pnpm --filter @objectstack/client typecheck— clean (tsc --noEmit+check:test-typecheck, 0 debt entries).pnpm lint— clean.check:*steps enumerated from.github/workflows/lint.yml's ESLint job — every one PASS, includingcheck:query-options-erasure,check:route-envelope,check:error-code-casing,check:engine-double-contract,check:nul-bytes.check:type-check-coverage,check:type-check-debt,@objectstack/client-react,@objectstack/cli,@objectstack/example-todoand@objectstack/downstream-contracttypechecks — all clean after building the dependency closure (the first pass showedTS2307reds incliand a root-package debt drift; both were the AGENTS.md §9 stale-artefact trap, confirmed by measuring pristineorigin/maincontent in the same worktree and getting the identical count).A
.changeset/*.md(patch) is included — this is a user-visible fix.Generated by Claude Code