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
14 changes: 14 additions & 0 deletions .changeset/public-picker-filter-lowering.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
"@objectstack/rest": patch
---

`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`.

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.

- **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".
- **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.
- **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.
- **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.

No authoring surface moves: `FormFieldPublicPickerSchema` already declared this dialect as accepted, and this makes the runtime honour it.
476 changes: 476 additions & 0 deletions packages/rest/src/public-form-lookup-filter-lowering.test.ts

Large diffs are not rendered by default.

26 changes: 16 additions & 10 deletions packages/rest/src/public-form-lookup-picker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,14 +206,19 @@ describe('#7467 a spec-valid stored form carrying a publicPicker reaches the loo
//
// [#16337] The KEYS are the canonical QueryAST ones (`where` / `fields`
// / `orderBy`); until then the route spelled them `filters` / `select` /
// `sort`, wire aliases the normalizer folds onto exactly these. The
// VALUES are byte-identical across that rewrite, which is the point —
// and note what `where` carries: `ViewFilterRule` rows, the dialect
// `FormFieldPublicPickerSchema.filter` declares, NOT a
// `FilterCondition`. `findData` is stubbed in this suite, so it never
// meets the ingress's verdict on that value; the real normalizer
// refuses it (#16581) — ⛔ do not "repair" it by editing this
// expectation.
// `sort`, wire aliases the normalizer folds onto exactly these.
//
// [#16581] The VALUE on `where` is the part that moved. It used to be
// the `ViewFilterRule` rows verbatim — the dialect
// `FormFieldPublicPickerSchema.filter` declares — which the ingress
// refuses with `400 INVALID_FILTER`, so this endpoint answered 400 for
// every non-empty search. The route now LOWERS them to the
// `FilterArray` grammar the parser reads, and the declared conjunction
// is written down rather than left to the list form's implicit AND.
// ⚠️ `findData` is stubbed in this suite, so this remains a COMPOSITION
// pin and cannot say the value is served: that is measured against the
// real normalizer in `public-form-lookup-filter-lowering.test.ts`,
// whose §3 keeps the control that the parser itself was NOT loosened.
expect(findData).toHaveBeenCalledTimes(1);
const call = findData.mock.calls[0][0];
expect(call.object).toBe('sys_user');
Expand All @@ -224,8 +229,9 @@ describe('#7467 a spec-valid stored form carrying a publicPicker reaches the loo
// ascending. The route's `picker.sort ??` read is retired.
expect(call.query.orderBy).toEqual([{ field: 'name', order: 'asc' }]);
expect(call.query.where).toEqual([
{ field: 'is_active', operator: 'equals', value: true },
{ field: 'name', operator: 'contains', value: 'ad' },
'and',
['is_active', 'equals', true],
['name', 'contains', 'ad'],
]);
expect(call.context.anonymous).toBe(true);
});
Expand Down
17 changes: 16 additions & 1 deletion packages/rest/src/rest-server-canonical-query-ast.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,15 @@
* pair is asserted equal by both REFUSING — its `where` carries
* `ViewFilterRule` rows, which the ingress declines with `400 INVALID_FILTER`
* before and after this card alike. Equality is the assertion; the verdict on
* either side is the ingress's, and repairing it is #16581.
* either side is the ingress's.
*
* [#16581] The ROUTE no longer builds that literal — it lowers the rule rows to
* the `FilterArray` grammar before dispatch — but the pair stays exactly as
* frozen here, and its CONTROL becomes load-bearing in a second way: it is one
* of the two independent pins that the object dialect is still REFUSED, i.e.
* that #16581 lowered the route rather than loosening the parser. ⛔ Never
* "update" the picker pair to the lowered shape: a frozen BEFORE that is
* rewritten to match the after measures nothing.
*/

import { describe, it, expect, vi } from 'vitest';
Expand Down Expand Up @@ -330,6 +338,13 @@ describe('[#16337] §3 the rewrite moves nothing — driven through the real nor
// Stated rather than left implicit: this pair's equality is not evidence
// that the picker query is served. Both sides carry `ViewFilterRule`
// rows on the filter slot, which is not a `FilterCondition`.
//
// [#16581] ⭐ And this is now the discriminating control for that card:
// the route lowers those rows before dispatch, so it no longer sends
// this literal — while the literal itself must still be REFUSED. A
// green picker search plus a green line here means "the route lowers";
// a green picker search with this line flipped would have meant "the
// parser was loosened", the repair the ruling excludes.
const outcome = await normalized(PAIRS[3].canonical) as { refused?: { code?: string; status?: number } };
expect(outcome.refused).toEqual({ code: 'INVALID_FILTER', status: 400 });
});
Expand Down
62 changes: 48 additions & 14 deletions packages/rest/src/rest-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,8 @@ import {
type ExportFieldMeta,
} from './export-format.js';
import { runImport } from './import-runner.js';
// [#16581] The public picker's authoring-dialect → parser-grammar lowering.
import { lowerViewFilterRules } from './view-filter-rule-lowering.js';
import { prepareImportRequest } from './import-prepare.js';
import { loadExcelJs, type Worksheet } from './xlsx-module.js';
import { enrichOpenApiWithEndpoints } from './openapi-endpoints.js';
Expand Down Expand Up @@ -10530,9 +10532,25 @@ export class RestServer {
// then the search predicate over displayFields. The
// search predicate uses `contains` on the first
// display field so non-indexed columns still work.
const filters: any[] = [];
if (Array.isArray(picker.filter)) filters.push(...picker.filter);
if (q) filters.push({ field: displayFields[0], operator: 'contains', value: q });
//
// [#16581] …and then LOWER the composed rows to the filter
// grammar the ingress parses. BOTH halves are the authoring
// dialect `FormFieldPublicPickerSchema.filter` declares
// (`{field, operator, value}`) — the declared rows because
// an author wrote them, the search row because this route
// built it in the same shape — and the normalizer refuses
// that shape with `400 INVALID_FILTER`. So the endpoint
// answered 400 for EVERY non-empty search, with or without a
// declared `publicPicker.filter`; only the degenerate
// no-filter call could succeed. `lowerViewFilterRules` is
// the one-way translation (authoring dialect →
// `FilterArray`) and lives at this door because this is the
// door that speaks both; ⛔ the repair the ruling excludes
// is teaching `findData` a second dialect.
const rules: any[] = [];
if (Array.isArray(picker.filter)) rules.push(...picker.filter);
if (q) rules.push({ field: displayFields[0], operator: 'contains', value: q });
const filters = lowerViewFilterRules(rules);

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

// Project the response server-side too — never trust
// that the driver respected `select`.
const rows: any[] = Array.isArray(result?.data) ? result.data : Array.isArray(result?.items) ? result.items : [];
//
// [#16581] `records` FIRST, which is the key `findData`
// actually returns (`{ object, records, total, hasMore }`)
// and the order the other three read sites in this file
// already use. This one read `data` / `items` and NOT
// `records`, so against the real protocol it matched
// nothing and the picker answered `200 {"data":[]}` — an
// empty list for every search. Invisible until the filter
// above stopped 400ing, and invisible to the sibling suite
// because its `findData` double answers `{ data }`, a shape
// the protocol does not produce. The legacy aliases stay so
// those doubles and alternate protocols keep working.
const rows: any[] = Array.isArray(result?.records) ? result.records
: Array.isArray(result?.data) ? result.data
: Array.isArray(result?.items) ? result.items
: Array.isArray(result?.rows) ? result.rows
: Array.isArray(result) ? result : [];
const projected = rows.slice(0, maxResults).map((row: any) => {
const out: any = { id: row?.id };
for (const f of displayFields) {
Expand Down
90 changes: 90 additions & 0 deletions packages/rest/src/view-filter-rule-lowering.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.

/**
* [#16581] Lower `ViewFilterRule` rows to the filter grammar the data ingress
* actually parses.
*
* ## The gap this closes
*
* `FormFieldPublicPickerSchema.filter` declares the object dialect in so many
* words — *"Same `{ field, operator, value }` dialect as list-view filters"* —
* and `GET /forms/:slug/lookup/:field` put those rows straight onto the
* `findData` filter slot. That slot is read by
* `@objectstack/metadata-protocol`'s normalizer, which accepts a
* `FilterCondition` object or a `FilterArray` (`[field, operator, value]`, a
* logical node, or a list of those) and refuses anything else with
* `400 INVALID_FILTER`. An array of `{field, operator, value}` OBJECTS is none
* of those, so the route answered 400 for **every** non-empty search — the
* declared pre-filter and the route's own `q` predicate alike, since the `q`
* branch builds the same object shape. Only the degenerate empty-filter call
* could succeed.
*
* ⛔ The repair is NOT a second dialect on `findData`. Two filter grammars in
* the data layer would be maintained forever and would spread the object shape
* to every `findData` caller; the declaring side already promises the object
* dialect on the AUTHORING surface, so what has to change is the side that
* failed to honour it. This module is that side: authoring dialect in,
* parser grammar out, at the one door that speaks both.
*
* ## The operator fold is the spec's own, not a second table
*
* {@link normalizeFilterOperator} (`@objectstack/spec/ui`) is the fold
* `ViewFilterRuleSchema.operator` itself runs as its `z.preprocess`, exported
* precisely so "producers and renderers can normalize stored metadata against
* the SAME canonical map the schema uses, instead of inventing a second
* dialect". ⛔ Never hand-write an alias table here: a stored row predating a
* spelling's canonicalisation (`notEquals`, `isNotEmpty`, `gt`) must fold the
* way the schema folds it, and `AST_OPERATOR_MAP`'s coverage of that vocabulary
* is what `filter-view-operator-parity.test.ts` holds.
*
* ## An unlowerable row is FORWARDED, never dropped
*
* A row this function cannot read as a rule passes through verbatim, so the
* ingress refuses the whole request exactly as it did before. That direction is
* deliberate and it is the fail-CLOSED one: a picker's static filter is often
* the only thing keeping an anonymous visitor's search inside the rows a form
* is allowed to expose (`filter: [{ field: 'status', … 'published' }]`).
* Skipping a row we did not understand would turn a loud 400 into a 200 over an
* UNFILTERED table on an unauthenticated surface — a widening, delivered
* silently, by the code that was supposed to be repairing a refusal.
*/

import { normalizeFilterOperator } from '@objectstack/spec/ui';

/**
* One rule → one `FilterArray` comparison node, or the input verbatim when it
* is not a readable `{ field, operator, value }` row (see the module header:
* that is the fail-closed path, not a fallback).
*
* `value: undefined` emits the two-element form the grammar declares
* (`[field, operator]`) rather than a triple with an `undefined` in comparand
* position. That is the shape a unary rule authors as — `ViewFilterRuleSchema`
* documents `is_empty` / `is_not_empty` / `is_null` / `is_not_null` as taking
* their direction from the operator NAME and ignoring `value` — and it needs no
* local list of which operators are unary, which would be a third copy of a
* vocabulary the spec already owns.
*/
function lowerViewFilterRule(rule: unknown): unknown {
if (!rule || typeof rule !== 'object' || Array.isArray(rule)) return rule;
const { field, operator, value } = rule as { field?: unknown; operator?: unknown; value?: unknown };
if (typeof field !== 'string' || field.length === 0) return rule;
if (typeof operator !== 'string') return rule;
const op = normalizeFilterOperator(operator);
return value === undefined ? [field, op] : [field, op, value];
}

/**
* Lower a list of `ViewFilterRule` rows to the value the filter slot takes.
*
* - no rows → `[]`, which every path already reads as "no filter". ⛔ Not
* `['and']`: a logical node with nothing to join is itself refused (the one
* shape that used to return every row silently), and "the author declared no
* pre-filter" must not become a rejected request.
* - one or more rows → an explicit `['and', …]` node. The route ANDs its
* static rows with the visitor's search predicate, so the conjunction is
* written down rather than left to the list form's implicit AND.
*/
export function lowerViewFilterRules(rules: readonly unknown[]): unknown[] {
if (rules.length === 0) return [];
return ['and', ...rules.map(lowerViewFilterRule)];
}
15 changes: 15 additions & 0 deletions scripts/engine-double-contract.pinned.json
Original file line number Diff line number Diff line change
Expand Up @@ -3146,6 +3146,21 @@
"verb": "update",
"pinned": 1
},
{
"file": "packages/rest/src/public-form-lookup-filter-lowering.test.ts",
"verb": "delete",
"pinned": 1
},
{
"file": "packages/rest/src/public-form-lookup-filter-lowering.test.ts",
"verb": "findOne",
"pinned": 1
},
{
"file": "packages/rest/src/public-form-lookup-filter-lowering.test.ts",
"verb": "update",
"pinned": 1
},
{
"file": "packages/rest/src/public-form-lookup-picker.test.ts",
"verb": "delete",
Expand Down
Loading