Skip to content

Commit d5c4022

Browse files
os-salesclaude
andauthored
fix(rest): lower public-picker filter rules to the grammar the ingress parses (#16673)
* fix(rest): lower public-picker filter rules to the grammar the ingress 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 * fix(rest): read `records` from the picker's findData result 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 * test(rest): pin the new picker doubles and hold the caller's bound `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 * docs(rest): the picker matcher implements three comparisons, not two Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent ecf44b1 commit d5c4022

7 files changed

Lines changed: 675 additions & 25 deletions
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
"@objectstack/rest": patch
3+
---
4+
5+
`GET /forms/:slug/lookup/:field` answers a search again: the public-form lookup picker no longer refuses every non-empty query with `400 INVALID_FILTER`.
6+
7+
The route composed its filter list out of `ViewFilterRule` objects — the `{ field, operator, value }` dialect `FormFieldPublicPickerSchema.filter` declares in so many words ("Same `{ field, operator, value }` dialect as list-view filters") — and put them straight onto the `findData` filter slot. That slot accepts a `FilterCondition` object or a `FilterArray` (`[field, operator, value]`, a logical node, or a list of those) and refuses anything else. The refusal did not depend on an author declaring `publicPicker.filter`: the route's own `q` predicate is built in the same object shape, so **every** non-empty search was refused and only the degenerate empty-filter call could succeed — on an anonymous surface where a public-form applicant has no way around it.
8+
9+
- **The route lowers; the parser is untouched.** The composed rows are translated to the array grammar the ingress parses, at the one door that speaks both dialects. ⛔ The repair deliberately NOT taken is teaching `findData` a second dialect: that maintains two filter grammars in the data layer permanently and spreads the object shape to every `findData` caller. The declaration already promises the object dialect on the authoring surface, so what changes is the side that failed to honour the promise. A test keeps the control that the object shape fed to the parser directly is still refused, so "the route lowers" cannot be confused with "the parser was loosened".
10+
- **Both branches.** The declared `publicPicker.filter` rows and the route's own `contains` search row are lowered together and ANDed explicitly; no declared filter still means no filter (`[]`), never an empty logical node the ingress would refuse.
11+
- **The operator fold is the spec's own.** Lowering reuses `normalizeFilterOperator` from `@objectstack/spec/ui` — the fold `ViewFilterRuleSchema.operator` itself runs — so a stored row carrying a legacy spelling (`notEquals`, `isNotEmpty`, `gt`) folds exactly as the schema folds it. No second alias table.
12+
- **A rule that cannot be read is forwarded, not dropped.** The request is then refused exactly as before. That direction is deliberate: a picker's static filter is often the only thing keeping an anonymous visitor's search inside the rows a form may expose, and silently skipping a row nobody understood would answer 200 over an unfiltered table.
13+
14+
No authoring surface moves: `FormFieldPublicPickerSchema` already declared this dialect as accepted, and this makes the runtime honour it.

packages/rest/src/public-form-lookup-filter-lowering.test.ts

Lines changed: 476 additions & 0 deletions
Large diffs are not rendered by default.

packages/rest/src/public-form-lookup-picker.test.ts

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -206,14 +206,19 @@ describe('#7467 a spec-valid stored form carrying a publicPicker reaches the loo
206206
//
207207
// [#16337] The KEYS are the canonical QueryAST ones (`where` / `fields`
208208
// / `orderBy`); until then the route spelled them `filters` / `select` /
209-
// `sort`, wire aliases the normalizer folds onto exactly these. The
210-
// VALUES are byte-identical across that rewrite, which is the point —
211-
// and note what `where` carries: `ViewFilterRule` rows, the dialect
212-
// `FormFieldPublicPickerSchema.filter` declares, NOT a
213-
// `FilterCondition`. `findData` is stubbed in this suite, so it never
214-
// meets the ingress's verdict on that value; the real normalizer
215-
// refuses it (#16581) — ⛔ do not "repair" it by editing this
216-
// expectation.
209+
// `sort`, wire aliases the normalizer folds onto exactly these.
210+
//
211+
// [#16581] The VALUE on `where` is the part that moved. It used to be
212+
// the `ViewFilterRule` rows verbatim — the dialect
213+
// `FormFieldPublicPickerSchema.filter` declares — which the ingress
214+
// refuses with `400 INVALID_FILTER`, so this endpoint answered 400 for
215+
// every non-empty search. The route now LOWERS them to the
216+
// `FilterArray` grammar the parser reads, and the declared conjunction
217+
// is written down rather than left to the list form's implicit AND.
218+
// ⚠️ `findData` is stubbed in this suite, so this remains a COMPOSITION
219+
// pin and cannot say the value is served: that is measured against the
220+
// real normalizer in `public-form-lookup-filter-lowering.test.ts`,
221+
// whose §3 keeps the control that the parser itself was NOT loosened.
217222
expect(findData).toHaveBeenCalledTimes(1);
218223
const call = findData.mock.calls[0][0];
219224
expect(call.object).toBe('sys_user');
@@ -224,8 +229,9 @@ describe('#7467 a spec-valid stored form carrying a publicPicker reaches the loo
224229
// ascending. The route's `picker.sort ??` read is retired.
225230
expect(call.query.orderBy).toEqual([{ field: 'name', order: 'asc' }]);
226231
expect(call.query.where).toEqual([
227-
{ field: 'is_active', operator: 'equals', value: true },
228-
{ field: 'name', operator: 'contains', value: 'ad' },
232+
'and',
233+
['is_active', 'equals', true],
234+
['name', 'contains', 'ad'],
229235
]);
230236
expect(call.context.anonymous).toBe(true);
231237
});

packages/rest/src/rest-server-canonical-query-ast.test.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,15 @@
4141
* pair is asserted equal by both REFUSING — its `where` carries
4242
* `ViewFilterRule` rows, which the ingress declines with `400 INVALID_FILTER`
4343
* before and after this card alike. Equality is the assertion; the verdict on
44-
* either side is the ingress's, and repairing it is #16581.
44+
* either side is the ingress's.
45+
*
46+
* [#16581] The ROUTE no longer builds that literal — it lowers the rule rows to
47+
* the `FilterArray` grammar before dispatch — but the pair stays exactly as
48+
* frozen here, and its CONTROL becomes load-bearing in a second way: it is one
49+
* of the two independent pins that the object dialect is still REFUSED, i.e.
50+
* that #16581 lowered the route rather than loosening the parser. ⛔ Never
51+
* "update" the picker pair to the lowered shape: a frozen BEFORE that is
52+
* rewritten to match the after measures nothing.
4553
*/
4654

4755
import { describe, it, expect, vi } from 'vitest';
@@ -330,6 +338,13 @@ describe('[#16337] §3 the rewrite moves nothing — driven through the real nor
330338
// Stated rather than left implicit: this pair's equality is not evidence
331339
// that the picker query is served. Both sides carry `ViewFilterRule`
332340
// rows on the filter slot, which is not a `FilterCondition`.
341+
//
342+
// [#16581] ⭐ And this is now the discriminating control for that card:
343+
// the route lowers those rows before dispatch, so it no longer sends
344+
// this literal — while the literal itself must still be REFUSED. A
345+
// green picker search plus a green line here means "the route lowers";
346+
// a green picker search with this line flipped would have meant "the
347+
// parser was loosened", the repair the ruling excludes.
333348
const outcome = await normalized(PAIRS[3].canonical) as { refused?: { code?: string; status?: number } };
334349
expect(outcome.refused).toEqual({ code: 'INVALID_FILTER', status: 400 });
335350
});

packages/rest/src/rest-server.ts

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,8 @@ import {
300300
type ExportFieldMeta,
301301
} from './export-format.js';
302302
import { runImport } from './import-runner.js';
303+
// [#16581] The public picker's authoring-dialect → parser-grammar lowering.
304+
import { lowerViewFilterRules } from './view-filter-rule-lowering.js';
303305
import { prepareImportRequest } from './import-prepare.js';
304306
import { loadExcelJs, type Worksheet } from './xlsx-module.js';
305307
import { enrichOpenApiWithEndpoints } from './openapi-endpoints.js';
@@ -10530,9 +10532,25 @@ export class RestServer {
1053010532
// then the search predicate over displayFields. The
1053110533
// search predicate uses `contains` on the first
1053210534
// display field so non-indexed columns still work.
10533-
const filters: any[] = [];
10534-
if (Array.isArray(picker.filter)) filters.push(...picker.filter);
10535-
if (q) filters.push({ field: displayFields[0], operator: 'contains', value: q });
10535+
//
10536+
// [#16581] …and then LOWER the composed rows to the filter
10537+
// grammar the ingress parses. BOTH halves are the authoring
10538+
// dialect `FormFieldPublicPickerSchema.filter` declares
10539+
// (`{field, operator, value}`) — the declared rows because
10540+
// an author wrote them, the search row because this route
10541+
// built it in the same shape — and the normalizer refuses
10542+
// that shape with `400 INVALID_FILTER`. So the endpoint
10543+
// answered 400 for EVERY non-empty search, with or without a
10544+
// declared `publicPicker.filter`; only the degenerate
10545+
// no-filter call could succeed. `lowerViewFilterRules` is
10546+
// the one-way translation (authoring dialect →
10547+
// `FilterArray`) and lives at this door because this is the
10548+
// door that speaks both; ⛔ the repair the ruling excludes
10549+
// is teaching `findData` a second dialect.
10550+
const rules: any[] = [];
10551+
if (Array.isArray(picker.filter)) rules.push(...picker.filter);
10552+
if (q) rules.push({ field: displayFields[0], operator: 'contains', value: q });
10553+
const filters = lowerViewFilterRules(rules);
1053610554

1053710555
const context: any = {
1053810556
permissions: ['guest_portal'],
@@ -10547,16 +10565,16 @@ export class RestServer {
1054710565
// moves the value verbatim, so this is a spelling change
1054810566
// and nothing else.
1054910567
//
10550-
// ⚠️ The VALUE on `where` is unchanged and is NOT a
10551-
// `FilterCondition`: `filters` carries `ViewFilterRule`
10552-
// rows (`{field, operator, value}` objects, the dialect
10553-
// `FormFieldPublicPickerSchema.filter` declares) composed
10554-
// with the route's own search row, and the ingress refuses
10555-
// a non-empty one with `400 INVALID_FILTER` — measured, and
10556-
// filed as #16581. ⛔ Not repaired here: this card retypes
10557-
// the SPELLING of these literals and moves no behaviour.
10558-
// `FilterCondition`'s `[key: string]: any` index signature
10559-
// is why the array still compiles against the slot.
10568+
// ⚠️ The VALUE on `where` is a `FilterArray`, not a
10569+
// `FilterCondition`. #16337 left `ViewFilterRule` OBJECTS
10570+
// here — the dialect `FormFieldPublicPickerSchema.filter`
10571+
// declares — which the ingress refuses with
10572+
// `400 INVALID_FILTER`; #16581 lowers them above, so what
10573+
// arrives is the declared array grammar the normalizer
10574+
// parses. `FilterCondition`'s `[key: string]: any` index
10575+
// signature is why an array compiles against the slot at
10576+
// all; that the value is now a filter the ingress ACCEPTS
10577+
// is measured end-to-end, not asserted by the type.
1056010578
query: {
1056110579
object: referenceTo,
1056210580
limit: maxResults,
@@ -10581,7 +10599,23 @@ export class RestServer {
1058110599

1058210600
// Project the response server-side too — never trust
1058310601
// that the driver respected `select`.
10584-
const rows: any[] = Array.isArray(result?.data) ? result.data : Array.isArray(result?.items) ? result.items : [];
10602+
//
10603+
// [#16581] `records` FIRST, which is the key `findData`
10604+
// actually returns (`{ object, records, total, hasMore }`)
10605+
// and the order the other three read sites in this file
10606+
// already use. This one read `data` / `items` and NOT
10607+
// `records`, so against the real protocol it matched
10608+
// nothing and the picker answered `200 {"data":[]}` — an
10609+
// empty list for every search. Invisible until the filter
10610+
// above stopped 400ing, and invisible to the sibling suite
10611+
// because its `findData` double answers `{ data }`, a shape
10612+
// the protocol does not produce. The legacy aliases stay so
10613+
// those doubles and alternate protocols keep working.
10614+
const rows: any[] = Array.isArray(result?.records) ? result.records
10615+
: Array.isArray(result?.data) ? result.data
10616+
: Array.isArray(result?.items) ? result.items
10617+
: Array.isArray(result?.rows) ? result.rows
10618+
: Array.isArray(result) ? result : [];
1058510619
const projected = rows.slice(0, maxResults).map((row: any) => {
1058610620
const out: any = { id: row?.id };
1058710621
for (const f of displayFields) {
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#16581] Lower `ViewFilterRule` rows to the filter grammar the data ingress
5+
* actually parses.
6+
*
7+
* ## The gap this closes
8+
*
9+
* `FormFieldPublicPickerSchema.filter` declares the object dialect in so many
10+
* words — *"Same `{ field, operator, value }` dialect as list-view filters"* —
11+
* and `GET /forms/:slug/lookup/:field` put those rows straight onto the
12+
* `findData` filter slot. That slot is read by
13+
* `@objectstack/metadata-protocol`'s normalizer, which accepts a
14+
* `FilterCondition` object or a `FilterArray` (`[field, operator, value]`, a
15+
* logical node, or a list of those) and refuses anything else with
16+
* `400 INVALID_FILTER`. An array of `{field, operator, value}` OBJECTS is none
17+
* of those, so the route answered 400 for **every** non-empty search — the
18+
* declared pre-filter and the route's own `q` predicate alike, since the `q`
19+
* branch builds the same object shape. Only the degenerate empty-filter call
20+
* could succeed.
21+
*
22+
* ⛔ The repair is NOT a second dialect on `findData`. Two filter grammars in
23+
* the data layer would be maintained forever and would spread the object shape
24+
* to every `findData` caller; the declaring side already promises the object
25+
* dialect on the AUTHORING surface, so what has to change is the side that
26+
* failed to honour it. This module is that side: authoring dialect in,
27+
* parser grammar out, at the one door that speaks both.
28+
*
29+
* ## The operator fold is the spec's own, not a second table
30+
*
31+
* {@link normalizeFilterOperator} (`@objectstack/spec/ui`) is the fold
32+
* `ViewFilterRuleSchema.operator` itself runs as its `z.preprocess`, exported
33+
* precisely so "producers and renderers can normalize stored metadata against
34+
* the SAME canonical map the schema uses, instead of inventing a second
35+
* dialect". ⛔ Never hand-write an alias table here: a stored row predating a
36+
* spelling's canonicalisation (`notEquals`, `isNotEmpty`, `gt`) must fold the
37+
* way the schema folds it, and `AST_OPERATOR_MAP`'s coverage of that vocabulary
38+
* is what `filter-view-operator-parity.test.ts` holds.
39+
*
40+
* ## An unlowerable row is FORWARDED, never dropped
41+
*
42+
* A row this function cannot read as a rule passes through verbatim, so the
43+
* ingress refuses the whole request exactly as it did before. That direction is
44+
* deliberate and it is the fail-CLOSED one: a picker's static filter is often
45+
* the only thing keeping an anonymous visitor's search inside the rows a form
46+
* is allowed to expose (`filter: [{ field: 'status', … 'published' }]`).
47+
* Skipping a row we did not understand would turn a loud 400 into a 200 over an
48+
* UNFILTERED table on an unauthenticated surface — a widening, delivered
49+
* silently, by the code that was supposed to be repairing a refusal.
50+
*/
51+
52+
import { normalizeFilterOperator } from '@objectstack/spec/ui';
53+
54+
/**
55+
* One rule → one `FilterArray` comparison node, or the input verbatim when it
56+
* is not a readable `{ field, operator, value }` row (see the module header:
57+
* that is the fail-closed path, not a fallback).
58+
*
59+
* `value: undefined` emits the two-element form the grammar declares
60+
* (`[field, operator]`) rather than a triple with an `undefined` in comparand
61+
* position. That is the shape a unary rule authors as — `ViewFilterRuleSchema`
62+
* documents `is_empty` / `is_not_empty` / `is_null` / `is_not_null` as taking
63+
* their direction from the operator NAME and ignoring `value` — and it needs no
64+
* local list of which operators are unary, which would be a third copy of a
65+
* vocabulary the spec already owns.
66+
*/
67+
function lowerViewFilterRule(rule: unknown): unknown {
68+
if (!rule || typeof rule !== 'object' || Array.isArray(rule)) return rule;
69+
const { field, operator, value } = rule as { field?: unknown; operator?: unknown; value?: unknown };
70+
if (typeof field !== 'string' || field.length === 0) return rule;
71+
if (typeof operator !== 'string') return rule;
72+
const op = normalizeFilterOperator(operator);
73+
return value === undefined ? [field, op] : [field, op, value];
74+
}
75+
76+
/**
77+
* Lower a list of `ViewFilterRule` rows to the value the filter slot takes.
78+
*
79+
* - no rows → `[]`, which every path already reads as "no filter". ⛔ Not
80+
* `['and']`: a logical node with nothing to join is itself refused (the one
81+
* shape that used to return every row silently), and "the author declared no
82+
* pre-filter" must not become a rejected request.
83+
* - one or more rows → an explicit `['and', …]` node. The route ANDs its
84+
* static rows with the visitor's search predicate, so the conjunction is
85+
* written down rather than left to the list form's implicit AND.
86+
*/
87+
export function lowerViewFilterRules(rules: readonly unknown[]): unknown[] {
88+
if (rules.length === 0) return [];
89+
return ['and', ...rules.map(lowerViewFilterRule)];
90+
}

scripts/engine-double-contract.pinned.json

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3146,6 +3146,21 @@
31463146
"verb": "update",
31473147
"pinned": 1
31483148
},
3149+
{
3150+
"file": "packages/rest/src/public-form-lookup-filter-lowering.test.ts",
3151+
"verb": "delete",
3152+
"pinned": 1
3153+
},
3154+
{
3155+
"file": "packages/rest/src/public-form-lookup-filter-lowering.test.ts",
3156+
"verb": "findOne",
3157+
"pinned": 1
3158+
},
3159+
{
3160+
"file": "packages/rest/src/public-form-lookup-filter-lowering.test.ts",
3161+
"verb": "update",
3162+
"pinned": 1
3163+
},
31493164
{
31503165
"file": "packages/rest/src/public-form-lookup-picker.test.ts",
31513166
"verb": "delete",

0 commit comments

Comments
 (0)