From 16e88cd06243d8e9a6088aaa2decf5dfc5d5f320 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 16:45:36 +0000 Subject: [PATCH 1/4] fix(rest): lower public-picker filter rules to the grammar the ingress parses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ --- .changeset/public-picker-filter-lowering.md | 14 + ...public-form-lookup-filter-lowering.test.ts | 412 ++++++++++++++++++ .../src/public-form-lookup-picker.test.ts | 26 +- .../rest-server-canonical-query-ast.test.ts | 17 +- packages/rest/src/rest-server.ts | 44 +- .../rest/src/view-filter-rule-lowering.ts | 90 ++++ 6 files changed, 579 insertions(+), 24 deletions(-) create mode 100644 .changeset/public-picker-filter-lowering.md create mode 100644 packages/rest/src/public-form-lookup-filter-lowering.test.ts create mode 100644 packages/rest/src/view-filter-rule-lowering.ts diff --git a/.changeset/public-picker-filter-lowering.md b/.changeset/public-picker-filter-lowering.md new file mode 100644 index 0000000000..867b2569c2 --- /dev/null +++ b/.changeset/public-picker-filter-lowering.md @@ -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. diff --git a/packages/rest/src/public-form-lookup-filter-lowering.test.ts b/packages/rest/src/public-form-lookup-filter-lowering.test.ts new file mode 100644 index 0000000000..d7a19c456c --- /dev/null +++ b/packages/rest/src/public-form-lookup-filter-lowering.test.ts @@ -0,0 +1,412 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#16581] `GET /forms/:slug/lookup/:field` answers a SEARCH, not a 400. + * + * ## The defect + * + * 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 on the `findData` filter slot, which + * accepts a `FilterCondition` object or a `FilterArray` and refuses everything + * else with `400 INVALID_FILTER`. ⭐ The `q` branch builds the SAME object shape + * itself, so the refusal did not depend on an author declaring + * `publicPicker.filter`: every non-empty search 400'd, and only the degenerate + * empty-filter call could succeed. That is the endpoint's entire purpose, on an + * anonymous surface an applicant has no way around. + * + * ## Why this file exists next to `public-form-lookup-picker.test.ts` + * + * That suite stubs `findData` and pins the route's request COMPOSITION, so it + * could never have met the ingress's verdict on the value it composed — which + * is exactly how a route shipped for this long building a filter nothing would + * parse. Here the protocol's `findData` is the REAL + * `ObjectStackProtocolImplementation`, so the request crosses the same + * normalizer a served deployment uses and the assertions are on the ANSWER + * (status and rows), not on the source this card wrote. + * + * ## ⭐ §3 is the discriminating control and is not optional + * + * "The route lowers correctly" and "the parser was loosened" produce the same + * green in §1 and §2 and have opposite consequences. §3 keeps the card's own + * control pair — the object shape and the triple shape, fed to the ingress + * DIRECTLY through `GET /data/:object`'s `$filter` — and asserts the object + * shape is still refused. ⛔ Never delete or "repair" §3 to make a change pass: + * a green §1/§2 means nothing without it. (`rest-server-canonical-query-ast.ts`'s + * §3 CONTROL pins the same fact from its own frozen literal; two independent + * pins, deliberately.) + */ + +import { describe, expect, it, vi } from 'vitest'; +// The engine-double contract (#4434 / #5619): a fake engine's update/delete +// must be exactly as strict as ObjectQL's dispatch, or a dead route ships with +// its suite green. Both predicates live in metadata-core. +import { + assertEngineDeleteDispatch, + assertEngineUpdateDispatch, + assertEngineFindOnePredicate, + type EngineFindOneQueryInput, +} from '@objectstack/metadata-core'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from './rest-server.js'; + +// ─── the fixture: the card's own shape (an anonymous job-application form) ─── + +const JOBS = [ + { id: 'job_1', title: 'Senior software engineer', city: 'Berlin', status: 'published' }, + { id: 'job_2', title: 'Lead engineer', city: 'Lisbon', status: 'draft' }, + { id: 'job_3', title: 'Product designer', city: 'Berlin', status: 'published' }, + { id: 'job_4', title: 'Staff engineer', city: 'Remote', status: 'published' }, +]; + +const jobObject = { + name: 'ats_job', + label: 'Job', + nameField: 'title', + fields: { + id: { name: 'id', type: 'text' }, + title: { name: 'title', type: 'text', label: 'Title' }, + city: { name: 'city', type: 'text', label: 'City' }, + status: { name: 'status', type: 'text', label: 'Status' }, + }, +}; + +const applicationObject = { + name: 'ats_application', + label: 'Application', + fields: { + id: { name: 'id', type: 'text' }, + job: { name: 'job', type: 'lookup', reference: 'ats_job', label: 'Job' }, + }, +}; + +/** The picker the card declares, verbatim. */ +const PICKER_WITH_FILTER = { + displayFields: ['title', 'city'], + filter: [{ field: 'status', operator: 'equals', value: 'published' }], +}; + +/** The same picker with NO declared filter — the case that 400'd anyway. */ +const PICKER_NO_FILTER = { displayFields: ['title', 'city'] }; + +const applyForm = (picker: unknown) => ({ + name: 'ats_application.apply', + object: 'ats_application', + viewKind: 'form', + label: 'Apply', + config: { + type: 'simple', + data: { provider: 'object', object: 'ats_application' }, + sharing: { allowAnonymous: true, publicLink: '/forms/apply' }, + sections: [{ label: 'Your application', fields: [{ field: 'job', publicPicker: picker }] }], + }, +}); + +// ─── the real save path, so the fixture is a form the spec ACCEPTS ────────── + +/** The slice of the engine the `sys_metadata` write path touches. */ +function metadataEngine() { + const rows: Array> = []; + let nextId = 0; + return { + rows, + engine: { + async findOne(object: string, query?: EngineFindOneQueryInput) { + assertEngineFindOnePredicate(object, query); return null; + }, + async find() { return rows.slice(); }, + async insert(table: string, data: Record) { + if (table === 'sys_metadata_audit') return { id: 'audit_skip' }; + nextId += 1; + rows.push({ id: `r_${nextId}`, ...data }); + return { id: `r_${nextId}` }; + }, + async update(_t: string, data: Record, opts: { where: Record }) { + assertEngineUpdateDispatch(data, opts); + return { id: null }; + }, + async delete(_t: string, opts: { where: Record }) { + assertEngineDeleteDispatch(opts); + return { deleted: 0 }; + }, + registry: { registerItem: () => {}, registerObject: () => {}, listItems: () => [] }, + } as any, + }; +} + +/** + * Persist a view through the REAL `saveMetaItem` and return the stored body. + * A 422 here would mean the picker fixture is not spec-valid, which would make + * every route assertion below a statement about an unauthorable form. + */ +async function persistedBody(item: unknown): Promise { + const { engine, rows } = metadataEngine(); + const protocol = new ObjectStackProtocolImplementation(engine, () => new Map()) as any; + const result = await protocol.saveMetaItem({ type: 'view', name: 'ats_application.apply', item }); + expect(result.success, JSON.stringify(result)).toBe(true); + const row = rows.find((r) => r.type === 'view'); + expect(row, 'the save persisted no view row').toBeDefined(); + return JSON.parse(row!.metadata); +} + +// ─── the data engine: rows filtered by the condition that REALLY arrives ──── + +/** + * Evaluate a lowered `FilterCondition` against a row. + * + * ⚠️ Deliberately tiny and deliberately LOUD. It implements exactly the two + * comparisons this card's filters lower to and throws on anything else, + * including an array — a filter still in the authoring dialect reaching a + * driver is the defect itself, and a matcher that shrugged at it would let a + * half-lowered filter pass as "the right rows". Case-sensitive `$contains` + * follows the spec's split (`icontains` is the insensitive twin); which of the + * two the route composes is #16581's business, not this matcher's. + */ +function matchesCondition(row: Record, cond: unknown): boolean { + if (cond === undefined || cond === null) return true; + if (Array.isArray(cond)) { + if (cond.length === 0) return true; // `[]` — "no filter", every path reads it so + throw new Error(`an UNLOWERED filter reached the driver: ${JSON.stringify(cond)}`); + } + if (typeof cond !== 'object') throw new Error(`unexpected filter: ${JSON.stringify(cond)}`); + for (const [key, expected] of Object.entries(cond as Record)) { + if (key === '$and') { + if (!(expected as unknown[]).every((c) => matchesCondition(row, c))) return false; + continue; + } + if (key === '$or') { + if (!(expected as unknown[]).some((c) => matchesCondition(row, c))) return false; + continue; + } + if (expected && typeof expected === 'object' && !Array.isArray(expected)) { + for (const [op, operand] of Object.entries(expected as Record)) { + if (op === '$eq') { + if (row[key] !== operand) return false; + } else if (op === '$contains') { + if (!String(row[key] ?? '').includes(String(operand))) return false; + } else { + throw new Error(`this suite's matcher does not implement "${op}"`); + } + } + continue; + } + if (row[key] !== expected) return false; // implicit-equality form + } + return true; +} + +/** The option bag `engine.find` last received, for the receipt assertions. */ +type DataEngine = { engine: any; seen: () => Record | undefined }; + +function dataEngine(): DataEngine { + let seen: Record | undefined; + const objects: Record = { ats_job: jobObject, ats_application: applicationObject }; + const engine = { + registry: { getObject: (n: string) => objects[n] }, + find: async (object: string, options: Record) => { + seen = options; + if (object !== 'ats_job') return []; + return JOBS.filter((r) => matchesCondition(r, options?.where)); + }, + aggregate: async () => [], + count: async () => 0, + }; + return { engine, seen: () => seen }; +} + +// ─── the real routes over a REAL `findData` ───────────────────────────────── + +function mockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), + use: vi.fn(), listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined), + }; +} + +function mockRes() { + const res: any = { statusCode: 200, body: undefined }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: any) => { res.body = b; return res; }); + res.header = vi.fn(() => res); + res.end = vi.fn(() => res); + return res; +} + +/** + * Mount the real routes. `getMetaItems` is stubbed (it serves the stored form + * and the object definitions); `findData` is the REAL protocol's, bound to the + * data engine above — so the filter this route composes crosses the real + * ingress and the real lowering before any row is matched. + */ +function routesOver(storedView: any) { + const { engine, seen } = dataEngine(); + const real = new ObjectStackProtocolImplementation(engine as never) as any; + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', routes: { data: '', metadata: '' } }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItem: vi.fn().mockResolvedValue(undefined), + getMetaItems: vi.fn(async ({ type }: { type: string }) => { + if (type === 'view') return [storedView]; + if (type === 'object') return [jobObject, applicationObject]; + return []; + }), + findData: (request: unknown) => real.findData(request), + }; + const rest = new RestServer(mockServer() as any, protocol, { api: { requireAuth: false } } as any); + (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); + rest.registerRoutes(); + const route = (method: string, suffix: string) => { + const found = rest.getRoutes().find((r: any) => r.method === method && r.path.endsWith(suffix)); + if (!found) throw new Error(`route ${method} …${suffix} is not mounted`); + return found as any; + }; + return { seen, lookup: route('GET', '/forms/:slug/lookup/:field'), list: route('GET', '/data/:object') }; +} + +/** Drive the anonymous picker exactly as a browser does: no cookie, one `q`. */ +async function lookup(storedView: any, q?: string) { + const { lookup: route, seen } = routesOver(storedView); + const res = mockRes(); + await route.handler({ params: { slug: 'apply', field: 'job' }, query: q === undefined ? {} : { q } } as any, res); + return { status: res.statusCode, body: res.body, where: seen()?.where }; +} + +// --------------------------------------------------------------------------- +// §1 the `q` branch — the half that 400'd with NO declared filter at all +// --------------------------------------------------------------------------- + +describe('[#16581] §1 the route lowers the search predicate it builds itself', () => { + it('q=engineer answers 200 with the matching rows, not 400 INVALID_FILTER', async () => { + // The card's headline call. Before the fix this was + // `400 {"code":"INVALID_FILTER"}` — the route's own `{ field, operator: + // 'contains', value: q }` row is the object dialect too, so no author + // had to declare anything for the endpoint to be unusable. + const stored = await persistedBody(applyForm(PICKER_NO_FILTER)); + const { status, body, where } = await lookup(stored, 'engineer'); + + expect(status).toBe(200); + expect(body.data).toEqual([ + { id: 'job_1', title: 'Senior software engineer', city: 'Berlin' }, + { id: 'job_2', title: 'Lead engineer', city: 'Lisbon' }, + { id: 'job_4', title: 'Staff engineer', city: 'Remote' }, + ]); + // The receipt: what the ENGINE received is a lowered `FilterCondition`, + // which is the only way the rows above could have been produced. + expect(where).toEqual({ title: { $contains: 'engineer' } }); + }); + + it('the degenerate empty search still answers 200 — the one call that always worked', async () => { + // A guard, not a new capability: `filters: []` was the single shape the + // ingress accepted before, and the lowering must not turn "no filter" + // into `['and']`, a logical node with nothing to join that the ingress + // refuses outright. + const stored = await persistedBody(applyForm(PICKER_NO_FILTER)); + const { status, body, where } = await lookup(stored); + + expect(status).toBe(200); + expect(body.data.map((r: any) => r.id)).toEqual(['job_1', 'job_2', 'job_3', 'job_4']); + expect(where).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- +// §2 the declared `publicPicker.filter` branch, and the two composed +// --------------------------------------------------------------------------- + +describe('[#16581] §2 the declared filter is lowered too, and ANDed ahead of the search', () => { + it('the declared filter alone answers 200 and really restricts the rows', async () => { + const stored = await persistedBody(applyForm(PICKER_WITH_FILTER)); + const { status, body, where } = await lookup(stored); + + expect(status).toBe(200); + // `job_2` is `draft`: the declared pre-filter is APPLIED, not merely + // accepted. On this surface that distinction is the security property — + // the filter is what keeps an anonymous visitor inside the rows the form + // is allowed to expose. + expect(body.data.map((r: any) => r.id)).toEqual(['job_1', 'job_3', 'job_4']); + expect(where).toEqual({ status: 'published' }); + }); + + it('the declared filter AND the visitor search compose — the card\'s full combination', async () => { + const stored = await persistedBody(applyForm(PICKER_WITH_FILTER)); + const { status, body, where } = await lookup(stored, 'engineer'); + + expect(status).toBe(200); + // `job_3` fails the search, `job_2` fails the declared filter: only rows + // passing BOTH survive, which is what proves both branches lowered. + expect(body.data).toEqual([ + { id: 'job_1', title: 'Senior software engineer', city: 'Berlin' }, + { id: 'job_4', title: 'Staff engineer', city: 'Remote' }, + ]); + expect(where).toEqual({ $and: [{ status: 'published' }, { title: { $contains: 'engineer' } }] }); + }); + + it('a legacy operator spelling in a STORED row folds through the spec\'s own normalizer', async () => { + // `notEquals` is a `VIEW_FILTER_OPERATOR_ALIASES` row: authored today the + // schema folds it on parse, but a row stored before that fold — and this + // route reads STORED bodies, never re-parsed ones — still carries it. + // The lowering reuses `normalizeFilterOperator`, the schema's own + // preprocess, so the canonical spelling is what reaches the parser. ⛔ A + // second alias table here is what that reuse exists to prevent. + const stored = await persistedBody(applyForm(PICKER_NO_FILTER)); + stored.config.sections[0].fields[0].publicPicker.filter = [ + { field: 'status', operator: 'notEquals', value: 'draft' }, + ]; + const { status, body, where } = await lookup(stored); + + expect(status).toBe(200); + expect(body.data.map((r: any) => r.id)).toEqual(['job_1', 'job_3', 'job_4']); + expect(where).toEqual({ status: { $ne: 'draft' } }); + }); + + it('an unreadable stored rule is FORWARDED, so the request is still refused — never served unfiltered', async () => { + // The fail-closed direction, stated as a test because the tempting + // repair is the opposite one. A row the lowering cannot read as a rule + // is passed through and the ingress refuses the whole request; dropping + // it would answer 200 over an UNFILTERED table on an anonymous surface — + // a widening delivered silently by the code repairing a refusal. + const stored = await persistedBody(applyForm(PICKER_NO_FILTER)); + stored.config.sections[0].fields[0].publicPicker.filter = [{ nonsense: true }]; + const { status, body } = await lookup(stored, 'engineer'); + + expect(status).toBe(400); + expect(body.code).toBe('INVALID_FILTER'); + }); +}); + +// --------------------------------------------------------------------------- +// ⭐ §3 THE DISCRIMINATING CONTROL — the card's own control pair +// --------------------------------------------------------------------------- + +describe('[#16581] §3 CONTROL: the parser was NOT loosened — the object shape still answers 400', () => { + /** `GET /data/:object?$filter=…` on the same server, same ingress. */ + async function dataApi($filter: string) { + const stored = await persistedBody(applyForm(PICKER_NO_FILTER)); + const { list } = routesOver(stored); + const res = mockRes(); + await list.handler({ params: { object: 'ats_job' }, query: { $filter } } as any, res); + return { status: res.statusCode, body: res.body }; + } + + it('the OBJECT shape fed straight to the parser is refused — 400 INVALID_FILTER', async () => { + // ⭐ Without this assertion a green §1/§2 cannot be told apart from "the + // parser was loosened to accept `ViewFilterRule` objects", which is the + // repair the ruling excludes: it would maintain two filter grammars in + // the data layer forever and spread the shape to every `findData` + // caller. ⛔ Do not delete, weaken or "repair" this expectation. + const { status, body } = await dataApi('[{"field":"status","operator":"equals","value":"published"}]'); + expect(status).toBe(400); + expect(body.code).toBe('INVALID_FILTER'); + expect(body.error).toContain('is not a recognised filter shape'); + }); + + it('…and the TRIPLE shape on the same call answers 200 — the pair attributes the failure to SHAPE', async () => { + // The other half of the card's control: same server, same object, same + // anonymity, only the filter's shape differs. That is what rules out + // permissions, anonymity and every other part of the route as the cause. + const { status, body } = await dataApi('[["status","=","published"]]'); + expect(status).toBe(200); + expect(body.data.map((r: any) => r.id)).toEqual(['job_1', 'job_3', 'job_4']); + }); +}); diff --git a/packages/rest/src/public-form-lookup-picker.test.ts b/packages/rest/src/public-form-lookup-picker.test.ts index 53e3929cad..3ebd42b9f6 100644 --- a/packages/rest/src/public-form-lookup-picker.test.ts +++ b/packages/rest/src/public-form-lookup-picker.test.ts @@ -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'); @@ -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); }); diff --git a/packages/rest/src/rest-server-canonical-query-ast.test.ts b/packages/rest/src/rest-server-canonical-query-ast.test.ts index 96c828b78d..2f980ac619 100644 --- a/packages/rest/src/rest-server-canonical-query-ast.test.ts +++ b/packages/rest/src/rest-server-canonical-query-ast.test.ts @@ -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'; @@ -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 }); }); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 2bc1172e13..52792af1d4 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -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'; @@ -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'], @@ -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, diff --git a/packages/rest/src/view-filter-rule-lowering.ts b/packages/rest/src/view-filter-rule-lowering.ts new file mode 100644 index 0000000000..634cd23e3f --- /dev/null +++ b/packages/rest/src/view-filter-rule-lowering.ts @@ -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)]; +} From a4b8d4fb8dc5aba5f6b9e180cd3c47cb1f57d186 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 17:03:58 +0000 Subject: [PATCH 2/4] fix(rest): read `records` from the picker's findData result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ --- ...public-form-lookup-filter-lowering.test.ts | 61 ++++++++++++++++++- packages/rest/src/rest-server.ts | 18 +++++- 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/packages/rest/src/public-form-lookup-filter-lowering.test.ts b/packages/rest/src/public-form-lookup-filter-lowering.test.ts index d7a19c456c..458c068b05 100644 --- a/packages/rest/src/public-form-lookup-filter-lowering.test.ts +++ b/packages/rest/src/public-form-lookup-filter-lowering.test.ts @@ -183,6 +183,8 @@ function matchesCondition(row: Record, cond: unknown): boolean for (const [op, operand] of Object.entries(expected as Record)) { if (op === '$eq') { if (row[key] !== operand) return false; + } else if (op === '$ne') { + if (row[key] === operand) return false; } else if (op === '$contains') { if (!String(row[key] ?? '').includes(String(operand))) return false; } else { @@ -405,8 +407,65 @@ describe('[#16581] §3 CONTROL: the parser was NOT loosened — the object shape // The other half of the card's control: same server, same object, same // anonymity, only the filter's shape differs. That is what rules out // permissions, anonymity and every other part of the route as the cause. + // + // `records` is the key `findData` returns — this route hands its result + // through untouched, which is also how the picker's own `data`/`items` + // read was measured to match nothing (repaired in the same card). const { status, body } = await dataApi('[["status","=","published"]]'); expect(status).toBe(200); - expect(body.data.map((r: any) => r.id)).toEqual(['job_1', 'job_3', 'job_4']); + expect(body.records.map((r: any) => r.id)).toEqual(['job_1', 'job_3', 'job_4']); + }); +}); + +// --------------------------------------------------------------------------- +// §4 the response the route READS back — the second half of "the right rows" +// --------------------------------------------------------------------------- + +/** + * The picker read `result.data ?? result.items` and never `result.records`, + * which is the key `findData` returns (`{ object, records, total, hasMore }`) + * and the order the file's three other read sites already use. So with the + * filter lowered the route answered `200 {"data":[]}` — an empty picker for + * every search, the same user-visible outcome as the 400 by a different route. + * + * It was invisible twice over: unreachable while every non-empty search 400'd, + * and unreachable in `public-form-lookup-picker.test.ts`, whose `findData` + * double answers `{ data: rows }` — a shape the real protocol does not produce. + * A double that invents its subject's response shape cannot report that the + * consumer reads the wrong key. + */ +describe('[#16581] §4 the projection reads `records`, the key `findData` actually returns', () => { + it('rows survive the real response envelope — not 200 with an empty list', async () => { + const stored = await persistedBody(applyForm(PICKER_WITH_FILTER)); + const { status, body } = await lookup(stored, 'engineer'); + + expect(status).toBe(200); + expect(body.total).toBe(2); + expect(body.data.length).toBe(2); + }); + + it('the legacy `data` envelope a protocol double may answer with still works', async () => { + // The aliases are kept, so the sibling suite's double and any alternate + // protocol keep being read. Driven here rather than assumed. + const stored = await persistedBody(applyForm(PICKER_NO_FILTER)); + const rest = new RestServer(mockServer() as any, { + getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', routes: { data: '', metadata: '' } }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItem: vi.fn().mockResolvedValue(undefined), + getMetaItems: vi.fn(async ({ type }: { type: string }) => { + if (type === 'view') return [stored]; + if (type === 'object') return [jobObject, applicationObject]; + return []; + }), + findData: vi.fn().mockResolvedValue({ data: [{ id: 'job_9', title: 'Legacy envelope', city: 'Oslo' }] }), + } as any, { api: { requireAuth: false } } as any); + (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); + rest.registerRoutes(); + const route = rest.getRoutes().find((r: any) => r.method === 'GET' && r.path.endsWith('/forms/:slug/lookup/:field'))!; + const res = mockRes(); + await (route as any).handler({ params: { slug: 'apply', field: 'job' }, query: {} } as any, res); + + expect(res.statusCode).toBe(200); + expect(res.body.data).toEqual([{ id: 'job_9', title: 'Legacy envelope', city: 'Oslo' }]); }); }); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 52792af1d4..e1276d369c 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -10599,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) { From 029a1727ffceda4d1944a4aa115eab0d25297e8e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 17:24:22 +0000 Subject: [PATCH 3/4] 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 Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ --- .../public-form-lookup-filter-lowering.test.ts | 7 ++++++- scripts/engine-double-contract.pinned.json | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/rest/src/public-form-lookup-filter-lowering.test.ts b/packages/rest/src/public-form-lookup-filter-lowering.test.ts index 458c068b05..2c7dea718d 100644 --- a/packages/rest/src/public-form-lookup-filter-lowering.test.ts +++ b/packages/rest/src/public-form-lookup-filter-lowering.test.ts @@ -209,7 +209,12 @@ function dataEngine(): DataEngine { find: async (object: string, options: Record) => { seen = options; if (object !== 'ats_job') return []; - return JOBS.filter((r) => matchesCondition(r, options?.where)); + const rows = JOBS.filter((r) => matchesCondition(r, options?.where)); + // Hold the caller's bound, AFTER the filter and by PRESENCE — a + // limit-blind double reports a page size the engine never granted + // (`check:objectql-double-limit`). The picker sends + // `limit: maxResults`, so this is also the shape it really meets. + return typeof options?.limit === 'number' ? rows.slice(0, options.limit) : rows; }, aggregate: async () => [], count: async () => 0, diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 13a278c518..5b6bc99505 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -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", From eecc4b0722a14f8a515014516b3d987df336848b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 17:34:21 +0000 Subject: [PATCH 4/4] docs(rest): the picker matcher implements three comparisons, not two Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ --- packages/rest/src/public-form-lookup-filter-lowering.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rest/src/public-form-lookup-filter-lowering.test.ts b/packages/rest/src/public-form-lookup-filter-lowering.test.ts index 2c7dea718d..c837287333 100644 --- a/packages/rest/src/public-form-lookup-filter-lowering.test.ts +++ b/packages/rest/src/public-form-lookup-filter-lowering.test.ts @@ -155,7 +155,7 @@ async function persistedBody(item: unknown): Promise { /** * Evaluate a lowered `FilterCondition` against a row. * - * ⚠️ Deliberately tiny and deliberately LOUD. It implements exactly the two + * ⚠️ Deliberately tiny and deliberately LOUD. It implements exactly the three * comparisons this card's filters lower to and throws on anything else, * including an array — a filter still in the authoring dialect reaching a * driver is the defect itself, and a matcher that shrugged at it would let a