From 7e93c130ef1e4dcecd8d6bc4d0af986d941c05f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 11:15:19 +0000 Subject: [PATCH 1/2] fix(types): declare QueryParams.$filter as the union it already accepts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `$filter` was declared `Record` with a record-only `@example`, while two standing producers — plugin-list's `buildEffectiveFilter` and plugin-view's `ObjectView` — have fed ObjectQL AST arrays through the slot all along, and the data sources accept them. Nothing is narrowed: `Record` already accepts arrays structurally, so the union documents what was always legal. The array half is bound to `@objectstack/spec/data`'s `FilterArray` rather than restated, and the comment names `translateFilterToAST` as the authoritative accepted set rather than carrying a second list to drift from. Drops the cast objectui#3908 took at one assignment in `useRecordQuery` as declared debt; `hasFilter` narrows to the slot's own type instead. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L --- .changeset/3909-query-params-filter-union.md | 38 ++++++ packages/fields/src/widgets/useRecordQuery.ts | 20 ++-- .../query-params-filter-union.test.ts | 112 ++++++++++++++++++ packages/types/src/data.ts | 34 +++++- 4 files changed, 195 insertions(+), 9 deletions(-) create mode 100644 .changeset/3909-query-params-filter-union.md create mode 100644 packages/types/src/__tests__/query-params-filter-union.test.ts diff --git a/.changeset/3909-query-params-filter-union.md b/.changeset/3909-query-params-filter-union.md new file mode 100644 index 0000000000..e17e77ec51 --- /dev/null +++ b/.changeset/3909-query-params-filter-union.md @@ -0,0 +1,38 @@ +--- +'@object-ui/types': patch +'@object-ui/fields': patch +--- + +`QueryParams.$filter` now declares both shapes the data sources actually accept — the +MongoDB-style field-keyed record, or a `FilterArray`, the ObjectQL AST sugar bound from +`@objectstack/spec/data` (objectui#3909). + +**Nothing is narrowed and no accepted value changes.** `Record` already +accepted arrays structurally — they satisfy its string index — so the union documents +shapes that were always legal rather than admitting new ones. Measured both ways under +`tsc --strict`: all five inputs `translateFilterToAST` enumerates assign to the old and +new declarations alike, and both reject a bare number and a bare string identically. A +downstream `turbo run build` over all 43 dependent packages is green, which is the +evidence a published type change breaks no consumer. + +The harm was entirely on the type face, and it was two-sided. The declaration blocked +nothing while describing one legal shape as though it were the only one — objectui#3831 +is what that cost, a rule array accepted by a `Record` slot, object-spread +flattened to `{"0": {...}}`, types green, and the query filtering on a column literally +named `0`. And someone writing a new consumer would read the type and its record-only +`@example`, conclude the array path was illegal, and add a tolerant conversion for it — +the "widen the consumer to tolerate the producer" shape AGENTS.md #0.1 forbids. Two +producers have fed arrays through this slot all along: `plugin-list`'s +`buildEffectiveFilter` (grid and export) and `plugin-view`'s `ObjectView` (calendar / +kanban / gallery / timeline). The runtime was right; the declaration was narrow. + +The array half is **bound** to the spec's `FilterArray` rather than restated locally, so +it cannot fork from the vocabulary the servers parse — the same failure two hand-written +operator lists had in objectui#3948. The doc comment names `translateFilterToAST` as the +authoritative accepted set instead of carrying a second list to drift from. + +`@object-ui/fields` drops the local cast this defect forced. PR objectui#3908 wrote +`filter as Record` at one assignment in `useRecordQuery`, deliberately, as +debt rather than widening the shared type. `hasFilter` is now a type predicate narrowing +to the `$filter` slot's own type, so the assignment needs no cast and the guard cannot +drift from the declaration it guards. Type-only throughout; no runtime behaviour changes. diff --git a/packages/fields/src/widgets/useRecordQuery.ts b/packages/fields/src/widgets/useRecordQuery.ts index 73ac4ba675..ddb37ad426 100644 --- a/packages/fields/src/widgets/useRecordQuery.ts +++ b/packages/fields/src/widgets/useRecordQuery.ts @@ -111,8 +111,12 @@ export interface UseRecordQueryResult { * `Object.keys` on an array returns its INDICES — so the record-only test read * `['0','1','2']` for an AST node and was right only by accident. An empty array * is "no filter" for the same reason an empty object is. + * + * Narrows to the `$filter` slot's own type rather than a restatement of it + * (#3909), so the caller assigns with no cast and this predicate cannot drift + * from the declaration it is guarding. */ -function hasFilter(filter: unknown): boolean { +function hasFilter(filter: unknown): filter is NonNullable { if (filter === null || filter === undefined) return false; if (Array.isArray(filter)) return filter.length > 0; if (typeof filter !== 'object') return false; @@ -172,12 +176,14 @@ export function useRecordQuery(options: UseRecordQueryOptions): UseRecordQueryRe if (searchTerm && searchTerm.trim()) params.$search = searchTerm.trim(); if (searchFields && searchFields.length > 0) params.$searchFields = searchFields; if (sortArg) params.$orderby = { [sortArg.field]: sortArg.direction }; - // `QueryParams.$filter` is declared `Record< string, any >`, which the - // AST-array form does not describe — the cast is at this ONE assignment - // rather than widening a shared type that several other producers - // (plugin-list's `buildEffectiveFilter`, plugin-view's ObjectView) - // already feed arrays through. - if (hasFilter(filter)) params.$filter = filter as Record; + // No cast: `QueryParams.$filter` now declares both shapes it accepts + // (#3909), so the AST-array form the picker's merge yields is describable + // here. The local cast this replaces was deliberate debt — taken at this + // ONE assignment rather than widening the shared type that several other + // producers (plugin-list's `buildEffectiveFilter`, plugin-view's + // ObjectView) already feed arrays through. The shared type is honest now, + // so the debt is paid rather than moved. + if (hasFilter(filter)) params.$filter = filter; if (expand && expand.length > 0) params.$expand = expand; const result = await dataSource.find(objectName, params); diff --git a/packages/types/src/__tests__/query-params-filter-union.test.ts b/packages/types/src/__tests__/query-params-filter-union.test.ts new file mode 100644 index 0000000000..8ca89720ce --- /dev/null +++ b/packages/types/src/__tests__/query-params-filter-union.test.ts @@ -0,0 +1,112 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#3909 — `QueryParams.$filter` declares BOTH shapes the data sources + * accept, and binds the array half to `@objectstack/spec`'s `FilterArray` + * rather than restating it. + * + * ## Why this file exists at all + * + * The defect it pins was invisible to every runtime suite, and had to be: the + * declaration was `Record< string, any >`, which **structurally accepts arrays** + * (they satisfy its string index). So the two producers that have fed ObjectQL + * AST arrays through this slot all along — `plugin-list`'s + * `buildEffectiveFilter` (grid and export) and `plugin-view`'s `ObjectView` + * (calendar / kanban / gallery / timeline) — type-checked, ran, and shipped + * correct results. Nothing was broken at runtime and nothing could go red. + * + * The cost was paid on the type face instead, in both directions: + * + * 1. The type **blocked nothing while describing one legal shape as if it were + * the only one**. objectui#3831 is what that buys: a `Record< string, any >` + * slot accepted a rule array, an object spread flattened it to + * `{"0": {...}}`, types stayed green, and the query filtered on a column + * literally named `0`. + * 2. Someone writing a new consumer reads the type and its `@example`, + * concludes only the record form is legal, and adds a tolerant conversion + * for the array path — the "widen the consumer to tolerate the producer" + * shape AGENTS.md #0.1 forbids. + * + * Both failure modes are compile-time by nature, so the pins are too. Reverting + * `$filter` to `Record< string, any >` leaves every runtime suite green and + * turns THIS FILE red under `tsc -p tsconfig.test.json` (the `type-check` + * script) — the drift's own signature, reproduced deliberately. + * + * ## What is NOT pinned here + * + * That the union is the *authoritative* accepted set. It is not: the authority + * is `translateFilterToAST` (`@object-ui/data-objectstack`), which enumerates + * five input shapes. A second list here would be a third place to drift from — + * which is exactly how two operator vocabularies came apart in #3948. The + * binding below is to the spec's `FilterArray`, so the array half cannot fork + * locally. + */ + +import { describe, it, expect } from 'vitest'; +import type { FilterArray } from '@objectstack/spec/data'; +import type { QueryParams } from '../data'; + +type Assert< T extends true > = T; +/** True when `V` is accepted by the `$filter` slot. */ +type AcceptsFilter< V > = V extends QueryParams['$filter'] ? true : false; + +describe('QueryParams.$filter — declares the union it actually accepts (#3909)', () => { + it('accepts the MongoDB-style field-keyed record', () => { + type _Record = Assert< AcceptsFilter< { age: { $gt: number } } > >; + const params: QueryParams = { $filter: { age: { $gt: 18 }, status: 'active' } }; + expect(params.$filter).toEqual({ age: { $gt: 18 }, status: 'active' }); + }); + + it('accepts a bare AST comparison tuple with no cast', () => { + // The shape `buildEffectiveFilter` returns for a single condition. Before + // #3909 this compiled only because arrays satisfy `Record`'s string index — + // accepted by accident rather than by declaration. + const params: QueryParams = { $filter: ['status', '=', 'active'] }; + expect(params.$filter).toEqual(['status', '=', 'active']); + }); + + it('accepts a logical AST group with no cast', () => { + // What `mergeFilterNodes` returns once more than one source is active. + const params: QueryParams = { + $filter: ['and', ['age', '>=', 18], ['status', '=', 'active']], + }; + expect(Array.isArray(params.$filter)).toBe(true); + }); + + it('accepts the legacy bare list, combined with implicit AND', () => { + const params: QueryParams = { + $filter: [['stage', '=', 'won'], ['amount', '>', 1000]], + }; + expect(Array.isArray(params.$filter)).toBe(true); + }); + + it('binds the array half to the spec rather than restating it', () => { + // A locally re-declared AST type would satisfy the assignments above just + // as well — and would then be free to drift from the spec's vocabulary the + // way two hand-written operator lists did (#3948). This pin fails if the + // union stops admitting the spec's own `FilterArray`. + type _Bound = Assert< AcceptsFilter< FilterArray > >; + const fromSpec: FilterArray = ['status', '=', 'active']; + const params: QueryParams = { $filter: fromSpec }; + expect(params.$filter).toBe(fromSpec); + }); + + it('still refuses a value that is neither shape', () => { + // The union documents; it must not have become `any` on the way. Note the + // slot sits on an interface that also carries `[key: string]: any` — these + // pins prove the declared property still wins over that index signature, + // which is the whole reason the declaration is worth anything. + // @ts-expect-error a number is not a filter + const bad: QueryParams = { $filter: 42 }; + // @ts-expect-error a string is not a filter + const alsoBad: QueryParams = { $filter: 'status eq active' }; + expect(bad.$filter).toBe(42); + expect(alsoBad.$filter).toBe('status eq active'); + }); +}); diff --git a/packages/types/src/data.ts b/packages/types/src/data.ts index 0227f95b46..218995618d 100644 --- a/packages/types/src/data.ts +++ b/packages/types/src/data.ts @@ -31,6 +31,7 @@ import type { CreateExportJobInput as SpecCreateExportJobInput, CreateExportJobResult, } from '@objectstack/spec/contracts'; +import type { FilterArray } from '@objectstack/spec/data'; import type { ValidationError } from '@objectstack/spec/kernel'; export type { ExportJobStatus, ImportJobStatus, ImportWriteMode, ValidationError }; @@ -47,10 +48,39 @@ export interface QueryParams { $select?: string[]; /** - * Filter expression + * Filter expression, in either of the two forms the data sources accept: + * the MongoDB-style field-keyed record, or a `FilterArray` — the spec-owned + * ObjectQL AST sugar (`@objectstack/spec/data`). + * + * Both forms are normal here, and the array form is not an edge case: the + * repo's own canonical sink `mergeFilterNodes` / `toFilterNode` + * (`@object-ui/core`'s `filter-converter.ts`) returns AST nodes, and its two + * standing producers — `plugin-list`'s `buildEffectiveFilter` (grid and + * export) and `plugin-view`'s `ObjectView` (calendar / kanban / gallery / + * timeline) — have fed arrays through this slot all along. + * + * ⛔ Do not add a "tolerant conversion" in a consumer to cope with the array + * path. The array IS legal input; a consumer that needs one shape lowers + * through the shared sink rather than widening itself to tolerate the + * producer. + * + * The authoritative acceptable set is the one `translateFilterToAST` + * (`@object-ui/data-objectstack`'s `index.ts`) enumerates — five input + * shapes, of which the array forms below are three. Read it there rather than + * trusting a second list here; a partial restatement is exactly how two + * operator vocabularies drifted apart before. + * + * Note the declaration does not *narrow* anything: `Record` + * already structurally accepts arrays (they satisfy its string index), so the + * union documents the shapes that were always accepted rather than admitting + * new ones. It is the description that was wrong, not the runtime. + * * @example { age: { $gt: 18 }, status: 'active' } + * @example ['status', '=', 'active'] + * @example ['and', ['age', '>=', 18], ['status', '=', 'active']] + * @example [['stage', '=', 'won'], ['amount', '>', 1000]] */ - $filter?: Record; + $filter?: Record | FilterArray; /** * Sort order From ab70c6508751fc0017239d54e4e11cb4c7d8a9c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 11:18:57 +0000 Subject: [PATCH 2/2] test(types): make the $filter pin an identity check, not a phantom MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The assignment-shaped pins were vacuous as regression guards, measured rather than assumed: reverting the declaration to `Record` left `type-check` green (exit 0). Assignability cannot separate the two — arrays satisfy the record's string index, so `FilterArray extends QueryParams['$filter']` holds under both declarations. Identity is the property that differs. The pin now fails on a revert AND on a locally forked AST type replacing the spec binding. Reverse-verified: mutated tree -> type-check exit 2 (TS2344), vitest 6/6 green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L --- .../query-params-filter-union.test.ts | 31 +++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/packages/types/src/__tests__/query-params-filter-union.test.ts b/packages/types/src/__tests__/query-params-filter-union.test.ts index 8ca89720ce..01f21b41fd 100644 --- a/packages/types/src/__tests__/query-params-filter-union.test.ts +++ b/packages/types/src/__tests__/query-params-filter-union.test.ts @@ -55,6 +55,9 @@ import type { QueryParams } from '../data'; type Assert< T extends true > = T; /** True when `V` is accepted by the `$filter` slot. */ type AcceptsFilter< V > = V extends QueryParams['$filter'] ? true : false; +/** Exact type identity — NOT mutual assignability. See the note below. */ +type Equal< A, B > = + (< T >() => T extends A ? 1 : 2) extends (< T >() => T extends B ? 1 : 2) ? true : false; describe('QueryParams.$filter — declares the union it actually accepts (#3909)', () => { it('accepts the MongoDB-style field-keyed record', () => { @@ -86,12 +89,28 @@ describe('QueryParams.$filter — declares the union it actually accepts (#3909) expect(Array.isArray(params.$filter)).toBe(true); }); - it('binds the array half to the spec rather than restating it', () => { - // A locally re-declared AST type would satisfy the assignments above just - // as well — and would then be free to drift from the spec's vocabulary the - // way two hand-written operator lists did (#3948). This pin fails if the - // union stops admitting the spec's own `FilterArray`. - type _Bound = Assert< AcceptsFilter< FilterArray > >; + it('binds the array half to the spec, by IDENTITY not assignability', () => { + // ## Why this pin is an identity check, and why nothing weaker works + // + // Every assignment-shaped pin in this file is, on its own, VACUOUS as a + // regression guard — measured, not assumed. Reverting the declaration to + // `Record< string, any >` and re-running `type-check` leaves it GREEN + // (exit 0), because assignability cannot separate the two: arrays satisfy + // `Record< string, any >`'s string index, so `FilterArray extends + // QueryParams['$filter']` holds under BOTH declarations, and the old + // declaration is itself assignable to the new union. A guard that passes + // equally before and after the fix is a phantom check — it reads like + // enforcement and enforces nothing. + // + // Identity is the property that actually differs. This assertion goes red + // on a revert to the bare record, AND on the subtler regression: someone + // re-declaring a local `FilterNode` fork instead of binding the spec's + // type. That fork would satisfy every assignment above while being free to + // drift from the vocabulary the servers parse — the exact failure two + // hand-written operator lists had in #3948. + type _Bound = Assert< + Equal< NonNullable< QueryParams['$filter'] >, Record< string, any > | FilterArray > + >; const fromSpec: FilterArray = ['status', '=', 'active']; const params: QueryParams = { $filter: fromSpec }; expect(params.$filter).toBe(fromSpec);