From bb75ca165b402c99881f6a14f47fa86969087389 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 22:52:05 +0000 Subject: [PATCH] fix(app-shell): diagnose non-literal elements in predicate `in` sets An `in` predicate's array branch JSON-parses the bracketed text and returns an empty array on any parse failure, with no diagnostic. A non-literal element (a path, a bare identifier, a trailing comma) collapses the WHOLE set to [], so membership silently reads false for every row -- even discarding good literal elements sitting next to the bad one. Diagnose only, mirroring objectui#4049's ruling: zero semantic change. The catch still returns [], the verdict is untouched; a dev-mode console.warn now names the predicate and, best-effort, the element that broke the parse. Fixes #4266 --- ...te-in-array-parse-catch-diagnostic-4266.md | 26 +++++ .../views/metadata-admin/predicate.test.ts | 103 ++++++++++++++++++ .../src/views/metadata-admin/predicate.ts | 102 +++++++++++++++++ 3 files changed, 231 insertions(+) create mode 100644 .changeset/predicate-in-array-parse-catch-diagnostic-4266.md diff --git a/.changeset/predicate-in-array-parse-catch-diagnostic-4266.md b/.changeset/predicate-in-array-parse-catch-diagnostic-4266.md new file mode 100644 index 0000000000..90069f5097 --- /dev/null +++ b/.changeset/predicate-in-array-parse-catch-diagnostic-4266.md @@ -0,0 +1,26 @@ +--- +'@object-ui/app-shell': patch +--- + +`predicate.ts`'s `in [...]` membership check now names an element it cannot +parse instead of silently discarding the whole set. + +`path in [...]` hands the bracketed text to `parseLiteral`'s array branch, +which JSON-parses it after normalising quotes. An element that is not a JSON +literal — a path, a bare identifier, a trailing comma — made that +`JSON.parse` throw, and the `catch` returned `[]` with nothing in the +console. `[].includes(anything)` is `false`, so the predicate silently read +FALSE FOR EVERY ROW, and — because the parse is whole-set, not per-element — +one bad element discarded every good literal sitting next to it too: +`data.type in ['text', data.a]` collapsed exactly as hard as `data.type in +[data.a]` alone. + +Same family as objectui#4049 (a silently wrong verdict, zero warning) and the +same ruling: **diagnose only, zero semantic change.** The `catch` still +returns `[]` — an `in` set that fails to parse is still, and remains, the +empty set; this evaluator does not gain the ability to resolve a path inside +`in [...]` (that stays outside the declared subset). All that changes is that +a dev-mode `console.warn` now names the predicate and, best-effort, the +element that broke the parse. + +Fixes objectui#4266. diff --git a/packages/app-shell/src/views/metadata-admin/predicate.test.ts b/packages/app-shell/src/views/metadata-admin/predicate.test.ts index 93f3b19c20..d886518c91 100644 --- a/packages/app-shell/src/views/metadata-admin/predicate.test.ts +++ b/packages/app-shell/src/views/metadata-admin/predicate.test.ts @@ -454,3 +454,106 @@ describe('a path-shaped right-hand side is diagnosed, not resolved (objectui#404 expect(evaluatePredicate(expr as string, scope(row as Record))).toBe(expected); }); }); + +/* ── 8. a non-literal element inside `in [...]` is diagnosed (objectui#4266) ── */ + +/** + * objectui#4266. `parseLiteral`'s array branch JSON-parses the bracketed text + * after normalising quotes; an element that is not a JSON literal (a path, a + * bare identifier, a trailing comma) makes that `JSON.parse` throw, and the + * `catch` returned `[]` with nothing in the console — `in` reads FALSE FOR + * EVERY ROW, and because the parse is whole-set, one bad element discards the + * good literals sitting next to it too. + * + * Ruling on this card: **diagnose only, zero semantic change** — the same + * posture #4049 took for the right-hand-literal tail. The `catch` still + * returns `[]`; the verdicts are pinned IDENTICAL before and after (§8.3); + * all that changes is that the console stops being silent. + */ +describe('a non-literal element inside `in [...]` is diagnosed, not resolved (objectui#4266)', () => { + /* 8.1 — it fires, and it names the predicate and the culprit element */ + + it('`data.type in [data.a]` warns, naming the predicate and the unparseable element', () => { + expect(evaluatePredicate('data.type in [data.a]', scope({ type: 'text', a: 'text' }))).toBe(false); + expect(warn).toHaveBeenCalledTimes(1); + expect(warnings()).toContain('data.type in [data.a]'); + expect(warnings()).toContain('`data.a`'); + }); + + it('a bare (unresolvable-shaped) identifier element is named too', () => { + expect(evaluatePredicate("data.type in ['text', foo]", scope({ type: 'text' }))).toBe(false); + expect(warnings()).toContain('`foo`'); + }); + + it('a trailing comma with otherwise-literal elements still warns, falling back to the whole set', () => { + // No single element is at fault here — every element parses fine on its + // own, so `findUnparseableSetElement` finds none, and the warning names + // the whole broken set instead of guessing at (and misnaming) a culprit. + expect(evaluatePredicate("data.type in ['a','b',]", scope({ type: 'a' }))).toBe(false); + expect(warn).toHaveBeenCalledTimes(1); + expect(warnings()).toContain("data.type in ['a','b',]"); + expect(warnings()).not.toContain('containing `'); + }); + + /* 8.2 — one bad element still discards the good literals beside it, and the + warning names the ONE that actually broke the parse, not the whole set + blindly */ + + it('one non-literal element collapses the WHOLE set, including the good literals next to it', () => { + // Both sides hold 'text' — a working `in` would be true. It is false, + // matching the pre-fix whole-set collapse; only the console changes. + expect(evaluatePredicate("data.type in ['text', data.a]", scope({ type: 'text', a: 'text' }))).toBe( + false, + ); + expect(warnings()).toContain('`data.a`'); + // The warning names the culprit, not the innocent literal beside it. + expect(warnings()).not.toContain("containing `'text'`"); + }); + + /* 8.3 — the zero-semantics proof: verdicts identical to pre-change */ + + it.each([ + ['data.type in [data.a]', { type: 'text', a: 'text' }, false], // would be true if paths resolved + ["data.type in ['text', data.a]", { type: 'text', a: 'text' }, false], // good literal discarded too + ["data.type in [data.a,]", { type: 'text', a: 'text' }, false], + ])('%s over %j is still %s — the diagnostic changes no verdict', (expr, row, expected) => { + expect(evaluatePredicate(expr, scope(row as Record))).toBe(expected); + }); + + /* 8.4 — controls: literal-only `in` sets are completely unaffected */ + + it.each([ + ["data.type in ['text','textarea']", { type: 'text' }, true], + ["data.type in ['number','currency']", { type: 'text' }, false], + ["data.type in ['a', 'b', 'c']", {}, false], + ])('%s over %j → %s, silently (literal path unperturbed)', (expr, row, expected) => { + expect(evaluatePredicate(expr, scope(row as Record))).toBe(expected); + expect(warn).not.toHaveBeenCalled(); + }); + + /* 8.5 — warn-once discipline, same bar as #6936 and #4049 */ + + it('warns ONCE per (predicate, set) pair, not once per evaluation', () => { + for (let i = 0; i < 5; i++) evaluatePredicate('data.type in [data.a]', scope({ type: 'text', a: 'x' })); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('but a different predicate carrying the same broken set gets its own warning', () => { + evaluatePredicate('data.type in [data.a]', scope({ type: 'text', a: 'x' })); + evaluatePredicate('data.kind in [data.a]', scope({ kind: 'text', a: 'x' })); + expect(warn).toHaveBeenCalledTimes(2); + }); + + /* 8.6 — dev-mode only */ + + it('the diagnostic is dev-mode only', () => { + const prev = process.env.NODE_ENV; + process.env.NODE_ENV = 'production'; + try { + expect(evaluatePredicate('data.type in [data.a]', scope({ type: 'text', a: 'text' }))).toBe(false); + expect(warn).not.toHaveBeenCalled(); + } finally { + process.env.NODE_ENV = prev; + } + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/predicate.ts b/packages/app-shell/src/views/metadata-admin/predicate.ts index 1315f5d0b0..a18d64ad2a 100644 --- a/packages/app-shell/src/views/metadata-admin/predicate.ts +++ b/packages/app-shell/src/views/metadata-admin/predicate.ts @@ -104,6 +104,43 @@ * header says, this file is an interim stand-in for `@objectstack/formula`, so * **this diagnostic retires with the file** when ROADMAP M9 lands CEL. Do not * grow it into a second evaluator. + * + * ## An unparseable element inside `in [...]` also fails silently (objectui#4266) + * + * `path in [...]` hands the bracketed text to `parseLiteral`'s array branch, + * which JSON-parses it after normalising quotes. An element that is not a JSON + * literal — a path, a bare identifier, a trailing comma — makes that + * `JSON.parse` throw, and the `catch` returned `[]` with nothing in the + * console. `[].includes(anything)` is `false`, so the predicate silently reads + * FALSE FOR EVERY ROW, and — because the parse is whole-set, not per-element — + * one bad element discards every good literal sitting next to it too: + * `data.type in ['text', data.a]` collapsed exactly as hard as `data.type in + * [data.a]` alone. + * + * Same family as objectui#4049 (silently wrong verdict, zero warning) and the + * same ruling: **diagnose only, zero semantic change.** The `catch` still + * returns `[]` — an `in` set that fails to parse is still, and remains, the + * empty set — this file does not gain the ability to resolve a path inside + * `in [...]` (that stays outside the declared subset; #4049 already draws + * that boundary for the right side of `==`/`!=` and it is not reopened here). + * All that changes is that the console names the predicate and, best-effort, + * the element that broke the parse, instead of staying silent. Verdicts are + * pinned identical before and after in `predicate.test.ts` §8.3, the same + * shape as #4049's §7.3. + * + * A louder option — making the whole predicate throw so the top-level + * fail-open in {@link evaluatePredicate} turns it `true`, mirroring + * objectstack#6936's unresolved-path ruling — was considered and rejected: + * #6936's `true` verdict corrects a fail-CLOSED bug (a hidden field is worse + * than a shown one), but here the existing verdict (`false`, i.e. hidden) is + * not a bug — it is the documented behaviour for a set this evaluator cannot + * parse, same as `#4049`'s tail returning the right-hand text verbatim + * instead of resolving it. Flipping it to fail-open `true` would be a + * semantic change with no ruling behind it, and would make an authoring + * mistake (a stray non-literal element) MORE visible than a correctly + * authored predicate that legitimately evaluates false — exactly backwards. + * + * This diagnostic retires with the file at ROADMAP M9, same as #4049's. */ export function evaluatePredicate( @@ -151,10 +188,20 @@ const warnedUnresolvedPaths = new Set(); */ const warnedPathShapedLiterals = new Set(); +/** + * The same warn-once discipline for the `in`-array parse-failure diagnostic + * (objectui#4266), keyed on (raw set text, predicate) for the same reason as + * the two Sets above: keying on the set text alone would report the first + * predicate carrying it and stay silent about a sibling predicate that + * happens to spell the same broken set. + */ +const warnedUnparseableInSets = new Set(); + /** Reset the warn-once memos. Exported for tests. */ export function resetPredicateWarnings(): void { warnedUnresolvedPaths.clear(); warnedPathShapedLiterals.clear(); + warnedUnparseableInSets.clear(); } const isDev = (): boolean => @@ -210,6 +257,56 @@ function warnPathShapedLiteral(text: string, source: string): void { ); } +/** + * Best-effort identification of WHICH element inside an unparseable `in [...]` + * set actually broke the parse — for the warning text only, never to change + * the return value. Splits on top-level commas (reusing `splitTopLevel`, + * which already tracks bracket depth and quotes for the `&&`/`||` splitters + * above) and re-runs the exact same quote-normalising `JSON.parse` the array + * branch itself uses, one element at a time, so the reported culprit is + * judged by literally the same rule that judged the whole set. Returns `null` + * when every individual element parses fine on its own (e.g. a stray trailing + * comma broke the whole-string parse but no single element is at fault) — + * the warning then falls back to naming the whole set. + */ +function findUnparseableSetElement(raw: string): string | null { + const inner = raw.slice(1, -1); + if (!inner.trim()) return null; + for (const part of splitTopLevel(inner, ',')) { + const el = part.trim(); + if (!el) continue; + try { + const json = el.replace(/'([^']*)'/g, (_, innerStr) => JSON.stringify(innerStr)); + JSON.parse(json); + } catch { + return el; + } + } + return null; +} + +function warnUnparseableInSet(raw: string, source: string): void { + if (!isDev()) return; + const memo = `${raw}::${source}`; + if (warnedUnparseableInSets.has(memo)) return; + warnedUnparseableInSets.add(memo); + const element = findUnparseableSetElement(raw); + console.warn( + `[metadata-admin] visibility predicate \`${source}\` has an \`in\` set \`${raw}\`` + + (element != null + ? ` containing \`${element}\`, which is not a literal this evaluator can parse — ` + : ', which this evaluator could not parse — ') + + 'the WHOLE set was treated as EMPTY, so the predicate is FALSE for every row (not just the ' + + 'element that failed — one bad element discards the good literals next to it too). This ' + + "evaluator only supports literal elements inside `in [...]` (supported subset: `path in " + + "['a','b']`); it does not resolve paths there — paths resolve only on the LEFT of an operator " + + '(objectui#4049). If you meant a literal, quote it. If you meant to test membership against ' + + "another field, that is outside this evaluator's subset: it is an interim stand-in for " + + '`@objectstack/formula` until CEL lands (ROADMAP M9), and predicate expressions are validated ' + + 'at publish time (objectstack#7010). objectui#4266.', + ); +} + function evalExpr( expr: string, ctx: { data: Record }, @@ -334,6 +431,11 @@ function parseLiteral(raw: string, source: string): unknown { const json = s.replace(/'([^']*)'/g, (_, inner) => JSON.stringify(inner)); return JSON.parse(json); } catch { + // Whole-set parse failure (a non-literal element, a trailing comma, …). + // Diagnose only (objectui#4266) — the verdict is UNTOUCHED: the set is + // still `[]`, `in` is still false for every row. See the header section + // "An unparseable element inside `in [...]` also fails silently". + warnUnparseableInSet(s, source); return []; } }