diff --git a/.changeset/filter-entries-translate-at-every-level.md b/.changeset/filter-entries-translate-at-every-level.md new file mode 100644 index 000000000..4ad3a41fa --- /dev/null +++ b/.changeset/filter-entries-translate-at-every-level.md @@ -0,0 +1,58 @@ +--- +"@object-ui/data-objectstack": patch +--- + +fix(data-objectstack): a view's own filter no longer disappears when the user adds one + +`ObjectStackAdapter` translated object-form filter entries +(`[{ field, operator, value }, ...]`) only at the **top level** of a `$filter`. +The moment a list has both a stored view filter and a user filter, it builds + +```js +['and', [{ field: 'stage', operator: 'eq', value: 'won' }], [['amount', '>', 1]]] +``` + +whose head is the string `and`, so the old check called the whole thing +"already AST" and shipped the rules untranslated. Both server answers to that +are wrong: + +```js +isFilterAST(above) // false — a bare rule object is not an AST child +parseFilterAST(above) // { amount: { $gt: 1 } } ← `stage = won` is GONE +``` + +Since objectstack#4121 the `isFilterAST` gate turns it into a **400 and the +list fails to load**. Before it — or anywhere `parseFilterAST` is reached +without that gate — **the view's own condition is dropped without a word** and +the list returns records the view exists to exclude. + +Translation is now recursive through `and`/`or` nodes and legacy flat child +arrays, so the shape reaches the server as a valid AST +(`{$and: [{stage: 'won'}, {amount: {$gt: 1}}]}`). + +Three related fixes in the same code: + +- **An untranslatable entry is now an error, not an omission.** Entries that + failed to translate were dropped, and dropping one conjunct of an `and` + returns a *superset* of the rows asked for — dropping the last one sent no + `filter=` at all, so the whole table came back. `find()` now throws + `MalformedFilterError`, carrying `code: 'INVALID_FILTER'` / `httpStatus: 400` + so a failed list renders "the filter is malformed" rather than "check your + connection". A rule with a blank `field` passes `ViewFilterRuleSchema` + (`z.string()` admits `''`), so this is reachable from real stored metadata. + A *mixed* array (`[{ field, operator, value }, ['amount', '>', 1]]`) now + keeps both halves instead of dropping the tuple — that case was a lost + condition, not a malformed one. +- **The two `find()` routes can no longer disagree.** The "is this object + form?" test existed twice — once in `translateFilterToAST`, once inline in + `convertQueryParams` — and the copies had already drifted: the inline one + omitted a `!== null` guard, so a `$filter` of `[null]` threw a `TypeError` on + the plain route while the same value was handled on the `$expand` route. One + definition now serves both. +- **Dropped an unreachable `entry.name` fallback.** `objectFilterEntryToAST` + read `entry.field ?? entry.name` while the shape check keyed on `field` + alone, so the `name` half was dead from the commit that introduced it. The + spec agrees it is not a real shape — `ViewFilterRuleSchema.field` is + required, so such a rule cannot be saved as view metadata. + +Refs objectstack#3948, objectstack#4121, #2945 diff --git a/packages/data-objectstack/src/filter-entry-translation.test.ts b/packages/data-objectstack/src/filter-entry-translation.test.ts new file mode 100644 index 000000000..c38e457e6 --- /dev/null +++ b/packages/data-objectstack/src/filter-entry-translation.test.ts @@ -0,0 +1,280 @@ +/** + * 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. + */ + +/** + * Object-form filter entries (`[{ field, operator, value }, ...]`) → filter AST. + * + * `find()` has TWO routes to the server and both translate the filter: a plain + * read goes through `convertQueryParams`, while a read with `$expand`/`$search` + * goes through `rawFindWithPopulate`. They used to carry independent copies of + * the "is this object form?" test, and the copies had already drifted — so the + * tests below run every case down BOTH routes and assert they agree. + * + * The stakes are the same as objectstack#3948: a filter this layer fails to + * translate is not a filter it may skip. Skipping one entry of an `and` returns + * a superset of the rows asked for, and skipping the last one sends no filter + * at all — every row in the table, reported as success. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { isFilterAST } from '@objectstack/spec/data'; +import { ObjectStackAdapter, clearSharedDiscoveryCache, isMalformedFilterError } from './index'; + +function makeAdapter() { + const calls: string[] = []; + const fetchImpl = vi.fn(async (url: any) => { + const u = String(url); + calls.push(u); + if (u.includes('/api/v1/discovery')) { + return { + ok: true, status: 200, statusText: 'OK', + json: async () => ({ success: true, data: { version: 'v1', routes: {} } }), + } as any; + } + return { + ok: true, status: 200, statusText: 'OK', + json: async () => ({ success: true, data: { object: 'account', records: [], total: 0 } }), + } as any; + }); + const adapter = new ObjectStackAdapter({ + baseUrl: 'http://localhost:3000', token: 't', autoReconnect: false, fetch: fetchImpl as any, + }); + return { adapter, calls }; +} + +/** The `filter=` this filter produced on the wire, or `undefined` if none was sent. */ +async function filterOnWire( + $filter: unknown, + route: 'plain' | 'expand', +): Promise { + const { adapter, calls } = makeAdapter(); + await adapter.find('account', { + $filter, + ...(route === 'expand' ? { $expand: ['owner'] } : {}), + } as any); + const dataCall = calls.filter((u) => u.includes('/data/account')).pop(); + const raw = dataCall ? new URL(dataCall).searchParams.get('filter') : null; + return raw === null ? undefined : JSON.parse(raw); +} + +/** Run one input down both `find()` routes. */ +function bothRoutes(name: string, $filter: unknown, assert: (wire: unknown | undefined) => void) { + for (const route of ['plain', 'expand'] as const) { + it(`${name} (${route} route)`, async () => assert(await filterOnWire($filter, route))); + } +} + +async function findRejects($filter: unknown, route: 'plain' | 'expand') { + const { adapter } = makeAdapter(); + return adapter + .find('account', { $filter, ...(route === 'expand' ? { $expand: ['owner'] } : {}) } as any) + .then(() => null, (e) => e); +} + +describe('object-form filter entries reach the wire as an AST', () => { + beforeEach(() => clearSharedDiscoveryCache()); + + bothRoutes( + 'translates a single rule, mapping the operator alias', + [{ field: 'stage', operator: 'greater_than_or_equal', value: 3 }], + (wire) => expect(wire).toEqual(['stage', '>=', 3]), + ); + + bothRoutes( + 'joins several rules with `and`', + [ + { field: 'stage', operator: 'eq', value: 'won' }, + { field: 'amount', operator: 'gt', value: 100 }, + ], + (wire) => expect(wire).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]), + ); + + bothRoutes( + 'accepts the `op` shorthand key', + [{ field: 'owner', op: 'eq', value: 'me' }], + (wire) => expect(wire).toEqual(['owner', '=', 'me']), + ); + + bothRoutes( + 'passes an AST tuple through untouched', + ['stage', '=', 'won'], + (wire) => expect(wire).toEqual(['stage', '=', 'won']), + ); + + bothRoutes( + 'passes a logical AST node through untouched', + ['or', ['stage', '=', 'won'], ['stage', '=', 'lost']], + (wire) => expect(wire).toEqual(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]), + ); + + bothRoutes('sends no filter at all for an empty array', [], (wire) => expect(wire).toBeUndefined()); +}); + +describe('a `name`-keyed entry is not an accepted shape', () => { + beforeEach(() => clearSharedDiscoveryCache()); + + /** + * `objectFilterEntryToAST` read `entry.field ?? entry.name` while the shape + * sniff keyed on `field` alone, so the `name` half could never be reached — + * dead from the commit that introduced it. The spec agrees it is not a real + * shape: `ViewFilterRuleSchema.field` is required, so such a rule cannot be + * saved as view metadata. It reaches the server unchanged and is refused + * there (objectstack#4121) rather than being quietly half-understood here. + */ + bothRoutes( + 'is passed through rather than translated', + [{ name: 'stage', operator: 'eq', value: 'won' }], + (wire) => expect(wire).toEqual([{ name: 'stage', operator: 'eq', value: 'won' }]), + ); +}); + +describe('an untranslatable entry fails instead of narrowing the filter', () => { + beforeEach(() => clearSharedDiscoveryCache()); + + for (const route of ['plain', 'expand'] as const) { + it(`rejects a rule with a blank field rather than dropping it (${route})`, async () => { + // Survives `ViewFilterRuleSchema` (`field: z.string()` admits ''), so it + // can be genuine stored metadata. It used to be dropped, and dropping the + // only rule sent no filter — the whole table came back. + const err = await findRejects([{ field: '', operator: 'eq', value: 'x' }], route); + expect(err).toBeInstanceOf(Error); + expect(isMalformedFilterError(err)).toBe(true); + }); + + it(`rejects a bad entry rather than silently widening the AND (${route})`, async () => { + const err = await findRejects( + [ + { field: 'stage', operator: 'eq', value: 'won' }, + { field: 'amount', operator: 42 as any, value: 1 }, + ], + route, + ); + expect(isMalformedFilterError(err)).toBe(true); + // Names which entry, so the author can find it in the view config. + expect(String((err as Error).message)).toContain('entry 1'); + }); + } + + it('carries the status and code that classify it as a rejected request', async () => { + const err: any = await findRejects([{ field: '', operator: 'eq' }], 'plain'); + // plugin-list's `classifyLoadError` reads these to pick the error panel; + // without them a malformed filter reads as "check your connection" (#3066). + expect(err.code).toBe('INVALID_FILTER'); + expect(err.httpStatus).toBe(400); + }); +}); + +describe('object-form rules nested inside a logical node are translated too', () => { + beforeEach(() => clearSharedDiscoveryCache()); + + /** + * This is what a list builds the moment a view with a stored filter meets a + * user filter from the panel — `['and', , ]`. + * Translating only the top level shipped the rules raw, and the server has no + * good answer for that: `isFilterAST` rejects the shape (so a server with + * objectstack#4121 returns 400 and the list fails to load), while + * `parseFilterAST` on the same array returns `{ amount: { $gt: 1 } }` — the + * view's own `stage = won` dropped without a word. + */ + bothRoutes( + 'translates a rule group nested under `and`', + ['and', [{ field: 'stage', operator: 'eq', value: 'won' }], [['amount', '>', 1]]], + (wire) => expect(wire).toEqual(['and', ['stage', '=', 'won'], [['amount', '>', 1]]]), + ); + + bothRoutes( + 'translates under `or`, and through more than one level', + ['or', ['and', [{ field: 'stage', operator: 'not_in', value: ['lost'] }]], ['x', '=', 1]], + (wire) => expect(wire).toEqual(['or', ['and', ['stage', 'nin', ['lost']]], ['x', '=', 1]]), + ); + + bothRoutes( + 'translates a group inside a legacy flat array of children', + [[{ field: 'stage', operator: 'eq', value: 'won' }], ['amount', '>', 1]], + (wire) => expect(wire).toEqual([['stage', '=', 'won'], ['amount', '>', 1]]), + ); + + bothRoutes( + 'leaves a field literally named `and` alone', + ['and', '=', true], + (wire) => expect(wire).toEqual(['and', '=', true]), + ); + + /** + * A mixed array used to lose the tuple: it did not translate as a rule, so it + * was filtered out and the query ran on the remaining condition alone — + * broader than asked for. Keep both rather than dropping one or failing the + * whole query. + */ + bothRoutes( + 'keeps both halves of a rule/tuple mixed array', + [{ field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 1]], + (wire) => expect(wire).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 1]]), + ); + + it('rejects a malformed rule nested under `and` as well', async () => { + const err = await findRejects(['and', [{ field: '', operator: 'eq', value: 1 }]], 'plain'); + expect(isMalformedFilterError(err)).toBe(true); + }); +}); + +describe('what this adapter emits passes the server’s own gate', () => { + beforeEach(() => clearSharedDiscoveryCache()); + + /** + * The real contract is not a literal shape, it is "the server accepts this". + * `isFilterAST` is the function the protocol gates on, so assert against it + * directly rather than restating what it happens to accept today — a filter + * it rejects is a 400 (objectstack#4121), and was an unfiltered query before + * that (objectstack#3948). + */ + const TRANSLATABLE: Array<[string, unknown]> = [ + ['a single rule', [{ field: 'stage', operator: 'eq', value: 'won' }]], + ['several rules', [ + { field: 'stage', operator: 'contains', value: 'w' }, + { field: 'amount', operator: 'less_than_or_equal', value: 9 }, + ]], + ['rules under `and` beside a tuple', [ + 'and', [{ field: 'stage', operator: 'before', value: '2024-01-01' }], ['amount', '>', 1], + ]], + ['rules inside a legacy flat array', [ + [{ field: 'stage', operator: 'is_null', value: true }], ['amount', '>', 1], + ]], + ['a mixed rule/tuple array', [{ field: 'stage', operator: 'eq', value: 'won' }, ['a', '>', 1]]], + ]; + + for (const [name, input] of TRANSLATABLE) { + for (const route of ['plain', 'expand'] as const) { + it(`${name} → accepted by isFilterAST (${route})`, async () => { + expect(isFilterAST(await filterOnWire(input, route))).toBe(true); + }); + } + } +}); + +describe('the two routes agree on shapes that are neither form', () => { + beforeEach(() => clearSharedDiscoveryCache()); + + /** + * The inline copy of the sniff omitted the `!== null` guard that this one + * had, so `[null]` threw a TypeError on the plain route and was handled on + * the `$expand` route — the same stored filter, decided by whether the view + * happened to expand a lookup. + */ + bothRoutes( + 'does not crash on a null entry', + [null], + (wire) => expect(wire).toEqual([null]), + ); + + bothRoutes( + 'leaves a nested legacy AST alone', + [['stage', '=', 'won'], ['amount', '>', 1]], + (wire) => expect(wire).toEqual([['stage', '=', 'won'], ['amount', '>', 1]]), + ); +}); diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index 3ad7ea059..4a60cce93 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -110,15 +110,139 @@ function normalizeFilterOperator(op: unknown): string | null { return FILTER_OPERATOR_ALIASES[lower] ?? FILTER_OPERATOR_ALIASES[op] ?? op; } +/** + * A filter entry this adapter cannot translate into an AST tuple. + * + * Thrown rather than skipped. Dropping one entry out of an `and` WIDENS the + * result set, and dropping the last one emits no `filter=` at all — every row, + * no error, from a query that asked for a subset. That is the same silent + * over-fetch the server-side drivers stopped doing in objectstack#3948, and + * skipping it here just moves it one layer up. + * + * Carries the code and status the data API uses for its own version of this + * refusal (objectstack#4121) so a failed list renders "this view's filter is + * malformed" rather than "check your connection" (#3066). + */ +export class MalformedFilterError extends Error { + readonly code = 'INVALID_FILTER'; + readonly httpStatus = 400; + readonly entry: unknown; + readonly index: number; + constructor(entry: unknown, index: number) { + const shown = JSON.stringify(entry) ?? String(entry); + super( + `Filter entry ${index} is not a usable filter rule (${shown}). ` + + 'Expected { field, operator, value } with a non-empty field.', + ); + this.name = 'MalformedFilterError'; + this.entry = entry; + this.index = index; + } +} + +/** Detect the malformed-filter refusal, whether raised here or by the server. */ +export function isMalformedFilterError(error: unknown): boolean { + if (!error || typeof error !== 'object') return false; + const e = error as Record; + return e.code === 'INVALID_FILTER' || e.name === 'MalformedFilterError'; +} + function objectFilterEntryToAST(entry: any): [string, string, any] | null { if (!entry || typeof entry !== 'object') return null; - const field = entry.field ?? entry.name; + // `field` only. A `?? entry.name` fallback lived here from the day the + // function was written (4b93db4e6) and was unreachable for exactly as long: + // the shape sniff below has always keyed on `field`, so a `name`-keyed entry + // fell through to the "already AST" branch and shipped raw. The spec agrees + // it is not a real shape — `ViewFilterRuleSchema.field` is required, so a + // `name`-keyed rule cannot be saved as view metadata in the first place. + const field = entry.field; const rawOp = entry.operator ?? entry.op ?? '='; const op = normalizeFilterOperator(rawOp); if (!field || !op) return null; return [String(field), op, entry.value]; } +/** + * Which of the two array filter shapes is this — object entries or an AST node? + * + * One definition on purpose. This test used to be written out twice (here and + * inline in `convertQueryParams`) and the copies had already drifted: the + * inline one omitted the `!== null` guard, so a `$filter` of `[null]` threw a + * TypeError on the plain `find()` path while the same value was handled on the + * `$expand` path. Same stored view, different answer, decided by whether it + * happened to expand a lookup. + */ +function isObjectFilterEntryForm(filter: readonly unknown[]): boolean { + const first = filter[0]; + return filter.length > 0 + && typeof first === 'object' + && first !== null + && !Array.isArray(first) + && (first as any).field !== undefined; +} + +/** + * Translate `[{ field, operator, value }, ...]` into a filter AST node. Every + * entry must translate; see `MalformedFilterError` for why one that doesn't is + * an error rather than an omission. + */ +function objectFilterEntriesToAST(entries: readonly unknown[]): unknown[] { + const nodes = entries.map((entry, i) => { + // An entry that is itself an array is already a node — a mixed array keeps + // both conditions instead of losing one to a drop or the whole query to an + // error. + if (Array.isArray(entry)) return translateFilterChild(entry); + const tuple = objectFilterEntryToAST(entry); + if (!tuple) throw new MalformedFilterError(entry, i); + return tuple; + }); + return nodes.length === 1 ? (nodes[0] as unknown[]) : ['and', ...nodes]; +} + +/** Logical heads `parseFilterAST` recognizes (`data/filter.zod.ts`). No `not`. */ +const LOGICAL_AST_HEADS = new Set(['and', 'or']); + +/** + * Translate a filter array at EVERY level, not just the top one. + * + * Translating only the top level left the commonest composite filter there is + * shipping raw. A list whose view carries a stored filter and whose user adds + * one in the panel produces `['and', , ]`; the + * head is the string `and`, so the old top-level-only check called the whole + * thing "already AST" and sent the rules on untouched. + * + * What the server does with that depends on its version, and both answers are + * wrong: + * + * ``` + * const n = ['and', [{ field: 'stage', operator: 'eq', value: 'won' }], [['amount', '>', 1]]]; + * isFilterAST(n) // false — a bare rule object is not an AST child + * parseFilterAST(n) // { amount: { $gt: 1 } } ← `stage = won` is simply GONE + * ``` + * + * Since objectstack#4121 the `isFilterAST` gate turns that into a 400 and the + * list fails to load. Before it — or anywhere `parseFilterAST` is reached + * without the gate — the view's own condition is dropped without a word and the + * list shows records the view exists to exclude. + */ +function translateFilterArray(filter: unknown[]): unknown[] { + if (isObjectFilterEntryForm(filter)) return objectFilterEntriesToAST(filter); + const head = filter[0]; + if (typeof head === 'string' && LOGICAL_AST_HEADS.has(head.toLowerCase())) { + return [head, ...filter.slice(1).map(translateFilterChild)]; + } + // Legacy flat array of child nodes: [[...], [...]] — implicit AND. + if (filter.every((child) => Array.isArray(child))) return filter.map(translateFilterChild); + // A comparison tuple, or a shape we do not recognize. Leave it alone; the + // server decides, and since objectstack#4121 it says so with a 400. + return filter; +} + +/** A child of a logical node: another array node, or a value we leave alone. */ +function translateFilterChild(child: unknown): unknown { + return Array.isArray(child) && child.length > 0 ? translateFilterArray(child) : child; +} + /** * Translate any of the filter shapes accepted by ObjectUI into the AST format * understood by the ObjectStack server's `parseFilterAST()`. @@ -138,25 +262,7 @@ function translateFilterToAST(filter: unknown): unknown | undefined { if (Array.isArray(filter)) { if (filter.length === 0) return undefined; - - // Object form: [{ field, operator, value }, ...] - const first = filter[0]; - const isObjectForm = filter.length > 0 - && typeof first === 'object' - && first !== null - && !Array.isArray(first) - && (first as any).field !== undefined; - if (isObjectForm) { - const tuples = (filter as any[]) - .map(entry => objectFilterEntryToAST(entry)) - .filter((t): t is [string, string, any] => t !== null); - if (tuples.length === 0) return undefined; - if (tuples.length === 1) return tuples[0]; - return ['and', ...tuples]; - } - - // Already AST — pass through. - return filter; + return translateFilterArray(filter); } if (typeof filter === 'object') { @@ -2076,26 +2182,10 @@ export class ObjectStackAdapter implements DataSource { // entry into the AST tuple shape and map human-readable // operator names (`greater_than_or_equal`, `in`, `contains`, // …) to the canonical symbols the server understands. - const isObjectForm = params.$filter.length > 0 - && typeof params.$filter[0] === 'object' - && !Array.isArray(params.$filter[0]) - && (params.$filter[0] as any).field !== undefined; - if (isObjectForm) { - const tuples = (params.$filter as any[]) - .map(entry => objectFilterEntryToAST(entry)) - .filter((t): t is [string, string, any] => t !== null); - if (tuples.length === 0) { - // All entries were unrecognized — drop the filter rather than - // sending a malformed array. - } else if (tuples.length === 1) { - options.filters = tuples[0]; - } else { - options.filters = ['and', ...tuples]; - } - } else { - // Already in AST format - options.filters = params.$filter; - } + // Shared with `translateFilterToAST` so the two `find()` routes — this + // one and the `$expand`/`$search` raw GET — cannot disagree about the + // same stored filter. + options.filters = translateFilterArray(params.$filter); } else { options.filters = convertFiltersToAST(params.$filter); }