fix(rest): lower public-picker filter rules to the grammar the ingress parses - #16673
Conversation
…s parses
`GET /forms/:slug/lookup/:field` composed its filter list out of
`ViewFilterRule` objects — the `{ field, operator, value }` dialect
`FormFieldPublicPickerSchema.filter` declares — and put them on the
`findData` filter slot, which accepts a `FilterCondition` or a
`FilterArray` and refuses everything else with `400 INVALID_FILTER`. The
`q` branch builds the same object shape itself, so every non-empty
search was refused whether or not an author declared
`publicPicker.filter`; only the degenerate empty-filter call succeeded.
The route now lowers the composed rows to the array grammar, reusing
`normalizeFilterOperator` from `@objectstack/spec/ui` rather than
restating the alias table. A row that cannot be read as a rule is
forwarded verbatim so the ingress still refuses the request — the
fail-closed direction on an anonymous surface, where the picker's static
filter is what bounds what a visitor can search.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ
The public lookup handler read `result.data ?? result.items` and never
`result.records`, which is the key `findData` returns
(`{ object, records, total, hasMore }`) and the order this file's three
other read sites already use. With the filter lowered the route
therefore answered `200 {"data":[]}` — an empty picker for every search,
the same user-visible outcome as the 400 by a different route.
Bounded in-place: same handler, same defect class (the route speaking a
shape the protocol layer does not), and the acceptance for #16581 is
"200 with the RIGHT ROWS", which is unreachable without it. The legacy
`data` / `items` / `rows` / bare-array aliases stay so protocol doubles
and alternate protocols keep working.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ
`check:engine-double-contract` and `check:objectql-double-limit` both name the new suite: the metadata-write double's delete/findOne/update seams were not in the pinned ledger (regenerated with `--write`, three additive rows, no losses), and the data double returned its filtered rows without applying `options.limit`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ
…blic-picker-filter-lowering
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ
📓 Docs Drift CheckThis PR changes 1 package(s): 1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 13 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 21cc35fea1d633684324efe12ab6fd506e4399c6 && git checkout 21cc35fea1d633684324efe12ab6fd506e4399c6
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 2539f4aec60e327f5a9ff83e6d07f16054d876ee eecc4b0722a14f8a515014516b3d987df336848b && git checkout -B drift-repro 2539f4aec60e327f5a9ff83e6d07f16054d876ee && git merge --no-ff eecc4b0722a14f8a515014516b3d987df336848b
node scripts/docs-audit/affected-docs.mjs --json 2539f4aec60e327f5a9ff83e6d07f16054d876ee
|
Fixes #16581
GET /forms/:slug/lookup/:fieldanswered400 INVALID_FILTERfor every non-empty search. The route composed its filter list out ofViewFilterRuleobjects — the{ field, operator, value }dialectFormFieldPublicPickerSchema.filterdeclares in so many words ("Same{ field, operator, value }dialect as list-view filters") — and put them on thefindDatafilter slot, which accepts aFilterConditionobject or aFilterArrayand refuses everything else. The route's ownqpredicate is built in the same object shape, so the refusal never depended on an author declaringpublicPicker.filter: only the degenerate empty-filter call could succeed. On an anonymous application form the picker is the only sensible way to choose a job, and there is no application-side workaround.The direction
The ROUTE lowers
ViewFilterRuleobjects to the parser's grammar. The declaring side already promises the object dialect on the authoring surface, so what changes is the side that failed to honour the promise.findDatawas NOT taught a second dialect. That would maintain two filter grammars in the data layer permanently and spread the object shape to everyfindDatacaller.normalizeFilterOperatorfrom@objectstack/spec/ui(packages/spec/src/ui/view.zod.ts) — the foldViewFilterRuleSchema.operatoritself runs as itsz.preprocess, exported precisely so producers "normalize stored metadata against the SAME canonical map the schema uses, instead of inventing a second dialect". No second alias table was written. This route reads STORED bodies, never re-parsed ones, so a row carrying a legacy spelling (notEquals,isNotEmpty,gt) folds exactly as the schema folds it — driven by a test.packages/spec/src/is edited. The schema is imported, not changed.Both branches are lowered
picker.filter(the declared rows) andq(the row the route builds itself) are composed and lowered together, ANDed explicitly. No rules still means no filter ([]), never an empty logical node the ingress would refuse — that degenerate call is pinned so the one shape that always worked keeps working.A row the lowering cannot read as a rule is forwarded verbatim, so the ingress refuses the request exactly as before. That direction is deliberate and fail-CLOSED: a picker's static filter is often the only thing bounding what an anonymous visitor can search, and silently skipping a row nobody understood would answer 200 over an unfiltered table.
⭐ The discriminating control
"The route lowers correctly" and "the parser was loosened" produce identical greens and have opposite consequences, so an assertion that the object shape fed directly to the parser still answers 400 is kept, in two independent places:
public-form-lookup-filter-lowering.test.ts§3 drives the card's own control pair throughGET /data/:object, same server, same object, same anonymity, only the filter's shape differing — object shape →400 INVALID_FILTER; triple shape →200with the right rows. That pair is what attributes the failure to SHAPE rather than to permissions, anonymity or another part of the route.rest-server-canonical-query-ast.test.ts§3's CONTROL keeps its frozen picker literal and asserts the ingress still refuses it. The literal is a historical record, so it is deliberately left as it was rather than "updated" to the lowered shape.Both ablation legs below leave §3 green, which is what makes it a control rather than a restatement.
A second defect, fixed in place
Once the 400 was gone the route answered
200 {"data":[]}— an empty picker for every search. The handler readresult.data ?? result.itemsand neverresult.records, which is the keyfindDatareturns ({ object, records, total, hasMore }) and the order this file's three other read sites already use. Bounded in-place repair: same handler, same defect class (the route speaking a shape the protocol layer does not), and the acceptance for this card is "200 with the RIGHT ROWS", which is unreachable without it. The legacydata/items/rows/ bare-array aliases stay, and a test drives one.It was invisible twice over: unreachable while every non-empty search 400'd, and unreachable in
public-form-lookup-picker.test.ts, whosefindDatadouble answers{ data: rows }— a shape the real protocol does not produce.Verification
Implementation committed FIRST, then mutated; every mutation proven on disk by marker counts and blob hash, restored under an
EXIT/INT/TERMtrap with an absolute path, and the restore proven by a whole-treegit status --porcelainplus blob-hash equality against HEAD.ablation-dist-preflightrun on both legs of both ablations.filters = rules)INVALID_FILTER. §3 stays GREEN--absent: marker absent from all 6 built files; tree cleanrecordsread removedexpected [] to deeply equal [...], i.e. 200 with an empty list, zeroINVALID_FILTER. §3 stays GREEN--absent: marker absent from all 6 built files; tree cleanAll readings below are taken on
eecc4b0— this branch with currentorigin/mainmerged in, so nothing here is measured on a tree nobody is on (the derivation warned STALE TREE before the merge and the warning is gone after it).pnpm --filter @objectstack/rest test— 190 files / 3228 tests passed.pnpm --filter @objectstack/rest typecheck— green, includingcheck:test-typecheck(0 files / 0 errors / 0 pinned debt).node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, re-derived on the merged head and reconciled with--ran: 65 derived, 65 run, 0 NOT-MEASURED, 0 UNRUN. Two of them (check:dual-build-cjs-loads,check:type-check-debt) first reported exit 3 — PREREQUISITE NOT MET on a partially built tree; a fullturbo run buildacross the workspace was run so both became real readings rather than a non-measurement.check:engine-double-contract(three additive rows regenerated with--write, no losses) andcheck:objectql-double-limit(the data double now holds the caller's bound, after the filter and by presence).Changeset:
patchon@objectstack/rest— a user-visible runtime repair on a published route, with no package graduating or breaking.Clause ② — re-derived from the delivered diff:
noAn external caller does observe 400 → 200 on a published route, but no declared face moves.
FormFieldPublicPickerSchema.filteralready declares the object dialect as accepted and its describe text already promises it; this makes the runtime honour what the declaration says. The diff touchespackages/restand one generated ledger underscripts/, and nothing underpackages/spec/src/. Repairing a route that refuses what its own contract promises is a defect fix, not a contract change.验收备注
Observed while working here, deliberately NOT changed and NOT filed:
packages/rest's picker suites build theirfindDatadoubles from response shapes the real protocol does not produce ({ data },{ items }). That is what hid therecordsread for as long as it hid it. A repo-wide "protocol double answers the protocol's own envelope" sweep is a class-closing job, not a rider on this card.packages/rest/src/view-filter-rule-lowering.tsbecause the ruling put the translation at the door that speaks both dialects. If a second consumer ever needs it, promoting it is its own card — it is not needed by anything today.rest-server.tsemits an unused-import warning forzodIssuesToFieldsduringtsup's ESM pass onmainas well as here; pre-existing, unrelated to this diff, and not a build failure.Generated by Claude Code