Skip to content

Commit a6a4361

Browse files
huangyiireneclaude
andauthored
fix(service-analytics): the draft preview refuses a filter operator it cannot evaluate instead of answering every row (#19833)
Fixes #19810 Clause-②: no The analytics draft-data preview (`packages/services/service-analytics/src/preview-evaluator.ts`, the ADR-0037 P3 Live Canvas path) answered **true for every row** for any `where` operator its switch had no case for. A drafted chart therefore silently IGNORED those filters and CHANGED at publish, where the real filter doors apply them. ## The enumeration, read at source before any edit At `origin/main` `c1dfa5241b`, `matchOp`'s switch carried exactly TEN cases and one default arm: | | | |---|---| | Evaluated (10) | `$eq`, `$ne`, `$gt`, `$gte`, `$lt`, `$lte`, `$between`, `$in`, `$nin`, `$contains` | | Default arm (`:109`) | `default: return true; // unknown operator — permissive (preview, reads only)` | | Answered for EVERY row | `$notContains`, `$startsWith`, `$endsWith`, `$icontains`, `$null`, `$exists` (the rest of `FILTER_OPERATORS`), the staged `$like` / `$ilike`, and any typo | The card's premise holds exactly as filed. The combinators `$and` / `$or` / `$not` were and remain handled by `matchesWhere` itself. ## The repair, and which contract it matches **Fail-closed by REFUSING** — `INVALID_FILTER` / `400`, through `filter-normalizer.ts`'s already-exported `invalidFilterError`. **No new error code and no new exported symbol** (the module's five exports are byte-identical before and after). Three candidate behaviours, and why refusal: - **answer true** — the defect; - **exclude the row** — makes the preview merely DIFFERENT from publish (zero rows where publish draws numbers). That is precisely the silent shape `lowerPreviewDateRange` abolished on this same evaluator (#16322), so it trades one invisible divergence for another; - **refuse** — the only one that makes the disagreement VISIBLE to the author who can fix it. The yardstick is what the real filter doors do, read rather than invented: - `driver-memory`'s `uncompilableFieldOperatorError` (#5345): "It is refused rather than dropped: a predicate that compiles to nothing does not narrow the query, it WIDENS it — the aggregate is then computed over rows the filter excluded, and a chart drawn over them looks like a working chart (#3948, #4286/ADR-0078)." - `service-analytics` **already** refuses `$like` / `$ilike` this way. From the `FILTER_OPERATORS` docblock's own face table: "`driver-mongodb`, `objectql` `having`, `service-analytics` — REFUSE, loudly, in the ADR-0112 `INVALID_FILTER` envelope". This change puts the preview face on the posture its own package already holds. - The triage note asked the preview to keep the refusal PR #19750 / #19514 gave the two filter doors for an empty or non-string `$icontains` comparand. It is kept the strong way round: the preview does not evaluate `$icontains` at all, so there is no comparand for it to disagree about — every spelling of it refuses. Two structural points, both copied from how this defect class was closed elsewhere: 1. **The vocabulary and the evaluator are ONE table.** The switch becomes a `Map` whose keys ARE what this face accepts — the shape `memory-analytics`' `MONGO_TO_CUBE_OPERATOR` took for the identical defect: "adding a row here is the only way to widen what this face accepts, and forgetting to add one is a loud refusal rather than a wrong number." A `Map` and not an object literal, so a constraint key naming an `Object.prototype` member cannot resolve to an inherited function and be called as a predicate. 2. **The gate is row-independent.** A per-row refusal only fires if some row reaches it, so a pending seed draft holding ZERO rows — the state a draft is authored in — would have answered an empty chart for a filter it cannot evaluate. The `where` tree is walked once before any row is read, the way `driver-memory`'s `assertFilterConditionShape` runs ahead of that driver's lowering. **⛔ No case was added, deliberately.** The default arm is the defect; adding `$icontains`, `$startsWith` and `$endsWith` would have left the next unhandled operator in exactly the same state, which is why the table and not a case list is the repair. Growing the arms is separate work with its own ordering already ruled: the `FILTER_OPERATORS` docblock's #6520 constraint — a name must not land ahead of its evaluators — reads the same in this direction, so an arm joins the table in the PR that measures it against the shared text/temporal conformance kits. This face is not enrolled in `FILTER_TEXT_CASES` today, and its one shipped text arm (`$contains`) is itself off that contract (see Acceptance notes). ## Evidence — both directions Command, identical in both states: ``` pnpm --filter @objectstack/service-analytics exec vitest run --maxWorkers=2 \ src/__tests__/preview-unevaluable-operator.test.ts ``` **BEFORE** — `preview-evaluator.ts` restored to `c1dfa5241b` on disk (blob `227496af` confirmed on disk against `git rev-parse BASE:path`; marker counts `default: return true` = 1, `PREVIEW_FIELD_OPERATORS` = 0), the test file and everything else at HEAD: ``` Test Files 1 failed (1) Tests 14 failed | 17 passed (31) FAIL ... > does NOT answer the row that `name $icontains "acme"` excludes AssertionError: expected [ 'Acme Corp', 'Globex' ] to not include 'Globex' FAIL ... > refuses it in the ADR-0112 `INVALID_FILTER` / 400 envelope AssertionError: expected undefined to be an instance of Error FAIL ... > refuses over an EMPTY seed draft too — the walk is not a function of the data FAIL ... > refuses inside `$or`, `$and` and `$not` arms FAIL ... > refuses a constraint key that names an Object.prototype member FAIL ... > $notContains / $startsWith / $endsWith / $icontains / $null / $exists / $like / $ilike is refused, never answered for every row (8 rows) FAIL ... > refuses the drafted selection instead of charting every seed row AssertionError: promise resolved "{ rows: [ { …(2) }, { …(2) } ], …(1) }" instead of rejecting ``` `expected [ 'Acme Corp', 'Globex' ] to not include 'Globex'` is the card's claim measured: `Globex` does not match `name $icontains 'acme'`, and the preview charted it anyway. The restore leg was verified by hash, not by an exit code: on-disk blob back to `0e1bf302` = `HEAD:path`, `git diff HEAD` empty. **AFTER** — same command, tree at HEAD: ``` Test Files 1 passed (1) Tests 31 passed (31) ``` **The unchanged direction.** The 17 tests that pass in BOTH states are the regression guard, and they are meant to: a fail-closed default that starts rejecting rows which used to match correctly is the mirror-image defect. They assert both directions (a matching row still matches, a non-matching row still does not) for every one of the ten evaluated arms, plus the `$lte` bare-day rule (#3777), implicit equality, `$and`, `$or`, `$not`, and an absent `where`. Every predicate body is byte-for-byte the `case` it replaces. ## Checks run locally at `2deda8dab5` - `pnpm --filter @objectstack/service-analytics test` — **114 files / 2442 tests passed** - `pnpm --filter @objectstack/service-analytics run typecheck` — exit 0 - `pnpm --filter '@objectstack/service-analytics^...' build` — exit 0 (dependency closure) - `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --ran ...` — **60 derived families accounted for: 57 run green, 3 NOT MEASURED** (`check:dual-build-cjs-loads`, `check:lean-entry-closure`, `check:type-check-debt` each exit 3, PREREQUISITE NOT MET — they read a whole-workspace `pnpm build`, which is CI's Build Core job). Among the 57: `check:where-matcher` (417 matchers, 0 silently-wrong), `check:nul-bytes`, `check:issue-citations`, `check:doc-authoring`, `check:empty-changeset`, `check:test-source-alias`, `check:undeclared-dep-imports`. - The four roster gates whose ledger sits under a directory this diff touches, read and run rather than assumed silent: `check-changeset-fixed`, `check:authz-resolver`, `check:error-code-casing`, `check:filter-alias-parity` — all exit 0. - `eslint . --no-inline-config` — the whole repo, not a narrowing: **7022 files, 0 errors, 0 warnings**, run at this PR's final commit. - Control-character self-scan over all three changed files: no hits. A changeset is included: `@objectstack/service-analytics` is published and this changes its runtime behaviour. ## Acceptance notes — found in passing, NOT fixed here 1. **`$contains` in this same file folds case, and the contract says it must not.** `matchOp`'s `$contains` arm is `String(value ?? '').toLowerCase().includes(String(expected ?? '').toLowerCase())`. `filter-text-conformance.ts` records that `$contains` / `$notContains` / `$startsWith` / `$endsWith` "compare CASE-SENSITIVELY" and that `driver-memory` moved its two folding faces onto the case-exact answer in #6682. The same line also coerces a non-string stored value, which the #14079 ruling type-gates. So the preview answers `$contains` differently from every published face — the same preview-vs-publish divergence this card is about, one arm over. ⛔ Deliberately untouched: this PR's second evidence direction is that the ten evaluated arms do not change behaviour. 2. **An undeclared `$`-key in a NODE position is silently read as a field name.** `matchesWhere` handles `$and` / `$or` / `$not` and falls through everything else to implicit equality, so `{ $nor: [...] }` compares `row['$nor']` and excludes every row without a word. The published path refuses it (`unknownLogicalOperatorError`). Fails closed rather than open, so it is not this card's harm — but it is silent. 3. **An empty field constraint `{ name: {} }` matches every row.** No operator keys, so the inner loop never runs. `driver-memory` refuses this shape (`emptyFieldConstraintError`, #5240): "`{ status: {} }` did not mean 'no rows', it meant 'rows whose status is anything'". This one IS answer-true-shaped, on the same evaluator, and is outside the operator vocabulary this card closes. --- _Generated by [Claude Code](https://claude.ai/code/session_01AhQASwqJr2Z7XfGWUdvnbF)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 4112752 commit a6a4361

3 files changed

Lines changed: 384 additions & 30 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
---
4+
5+
The draft-data preview **refuses** a `where` operator it cannot evaluate instead of answering it for every row, so a drafted chart no longer silently ignores a filter and then changes at publish (#19810).
6+
7+
`preview-evaluator.ts` evaluates a pending seed draft's rows in memory — the ADR-0037 P3 Live Canvas path — and its operator switch carried ten cases (`$eq`, `$ne`, `$gt`, `$gte`, `$lt`, `$lte`, `$between`, `$in`, `$nin`, `$contains`) and then `default: return true; // unknown operator — permissive (preview, reads only)`. Every other declared operator therefore matched EVERY row: `$icontains`, `$notContains`, `$startsWith`, `$endsWith`, `$null`, `$exists`, the staged `$like` / `$ilike`, and any typo. A drafted chart with `name $icontains 'acme'` charted the whole dataset and looked exactly like a working chart; the published chart, which runs the real filter doors, applied the filter.
8+
9+
- **Fail-closed, and VISIBLE.** The operator is refused in the ADR-0112 `INVALID_FILTER` / 400 envelope this package's `where` door already speaks, through `filter-normalizer`'s exported `invalidFilterError`. No new error code and no new exported symbol. Refused rather than excluded from the result: an excluded row makes the preview merely *different* from publish — zero rows where publish draws numbers — which is the silent shape `lowerPreviewDateRange` abolished on this same evaluator; only a refusal reaches the author who can fix it. It is the call `uncompilableFieldOperatorError` states for the analytics cube face, and the posture `service-analytics` already takes for `$like` / `$ilike`.
10+
- **The vocabulary and the evaluator are now ONE table**, the shape `memory-analytics`' `MONGO_TO_CUBE_OPERATOR` took for this same defect class: adding a row is the only way to widen what this face accepts, and forgetting to add one is a loud refusal rather than a wrong number.
11+
- **The gate does not depend on the data.** It walks `where` before any row is read, so a seed draft holding zero rows — the state a draft is authored in — refuses too instead of answering an empty chart.
12+
- ⚠️ **What it costs**: a drafted chart whose filter uses one of those operators now returns `400 INVALID_FILTER` in preview where it previously rendered a number. That number was computed over rows the filter excludes, and it changed at publish. Growing the preview's arms is deliberately separate work — the `FILTER_OPERATORS` docblock's ruling that a name must not land ahead of its evaluators reads the same in this direction, so an arm joins the table in the PR that measures it against the shared conformance kits.
13+
- **The ten evaluated arms are byte-for-byte unchanged**, pinned in both directions (a matching row still matches, a non-matching row still does not).
Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#19810] The draft-preview matcher answers operators it cannot evaluate.
5+
*
6+
* ## The enumeration, read at `origin/main` (`c1dfa5241b`) before any edit
7+
*
8+
* `preview-evaluator.ts`'s `matchOp` switch carried exactly TEN cases —
9+
* `$eq`, `$ne`, `$gt`, `$gte`, `$lt`, `$lte`, `$between`, `$in`, `$nin`,
10+
* `$contains` — and then, at `:109`:
11+
*
12+
* ```
13+
* default: return true; // unknown operator — permissive (preview, reads only)
14+
* ```
15+
*
16+
* So every OTHER operator matched EVERY row. Against the declared vocabulary
17+
* (`FILTER_OPERATORS`, plus the staged `$like` / `$ilike`) the silent set was
18+
* `$notContains`, `$startsWith`, `$endsWith`, `$icontains`, `$null`,
19+
* `$exists`, `$like`, `$ilike` — and any typo besides. A drafted chart with
20+
* `name $icontains 'acme'` charted the whole dataset; the published chart,
21+
* which runs the real filter doors, applied the filter. Same shape the file's
22+
* own `$between` case records for itself (#4081) and `lowerPreviewDateRange`
23+
* closed for the date-range vocabulary (#16322).
24+
*
25+
* ## What this file pins, in the two directions the repair has
26+
*
27+
* 1. **Fail-closed.** An operator with no arm is REFUSED — `INVALID_FILTER` /
28+
* 400, the envelope this package's `where` door already speaks — and the
29+
* row it does not match is NOT answered. Refused rather than merely
30+
* excluded: an excluded row makes the preview silently DIFFERENT from
31+
* publish, which is the failure #16322 abolished on this same evaluator;
32+
* a refusal makes the divergence visible to the author who can fix it.
33+
* 2. **Unchanged.** The ten operators the face DOES evaluate answer exactly
34+
* what they answered before, in both directions (a matching row matches, a
35+
* non-matching row does not). A fail-closed default that starts rejecting
36+
* rows which used to match correctly is the mirror-image defect.
37+
*
38+
* The refusal is pinned by its `code` + `status` (ADR-0112), not by prose: a
39+
* bare `toThrow()` would be satisfied by any uncoded error, including the
40+
* `TypeError` a malformed filter raises on its own.
41+
*/
42+
43+
import { describe, it, expect, vi } from 'vitest';
44+
import { DatasetSchema } from '@objectstack/spec/ui';
45+
import { FILTER_OPERATORS } from '@objectstack/spec/data';
46+
import type { Cube } from '@objectstack/spec/data';
47+
import { AnalyticsService } from '../analytics-service.js';
48+
import { evaluateAnalyticsQueryOverRows, matchesWhere } from '../preview-evaluator.js';
49+
50+
// ── fixture ─────────────────────────────────────────────────────────────────
51+
52+
const ROWS: Record<string, unknown>[] = [
53+
{ id: '1', name: 'Acme Corp', amount: 1200, spent_on: '2026-05-03' },
54+
// ⭐ the row `name $icontains 'acme'` does NOT match. Before the repair the
55+
// preview answered it anyway; the published chart never did.
56+
{ id: '2', name: 'Globex', amount: 800, spent_on: '2026-05-12' },
57+
];
58+
59+
const DATASET = DatasetSchema.parse({
60+
name: 'expense_ds',
61+
label: 'Expense',
62+
object: 'expense',
63+
dimensions: [{ name: 'name', field: 'name', type: 'string', label: 'Name' }],
64+
measures: [{ name: 'count', aggregate: 'count' }],
65+
});
66+
67+
const CUBE: Cube = new AnalyticsService().registerDataset(DATASET).cube;
68+
69+
/** The dimension values the preview ANSWERS — empty when it refuses. */
70+
function namesAnswered(where: Record<string, unknown>, rows = ROWS): string[] {
71+
try {
72+
const result = evaluateAnalyticsQueryOverRows(
73+
{ cube: 'expense_ds', measures: ['count'], dimensions: ['name'], where },
74+
CUBE,
75+
rows,
76+
);
77+
return result.rows.map((r) => String(r.name));
78+
} catch {
79+
return [];
80+
}
81+
}
82+
83+
/** The error a preview refuses with, or `undefined` if it answered. */
84+
function refusalFor(where: Record<string, unknown>, rows = ROWS): (Error & { code?: string; status?: number }) | undefined {
85+
try {
86+
evaluateAnalyticsQueryOverRows(
87+
{ cube: 'expense_ds', measures: ['count'], dimensions: ['name'], where },
88+
CUBE,
89+
rows,
90+
);
91+
return undefined;
92+
} catch (e) {
93+
return e as Error & { code?: string; status?: number };
94+
}
95+
}
96+
97+
// ── direction 1 — an operator with no arm must not answer every row ─────────
98+
99+
describe('[#19810] an operator the draft preview cannot evaluate', () => {
100+
it('does NOT answer the row that `name $icontains "acme"` excludes', () => {
101+
// ⛔ Before the repair this read `['Acme Corp', 'Globex']`: the `default`
102+
// arm answered true for both rows, so the drafted chart counted Globex
103+
// into a filter that excludes it.
104+
expect(namesAnswered({ name: { $icontains: 'acme' } })).not.toContain('Globex');
105+
});
106+
107+
it('refuses it in the ADR-0112 `INVALID_FILTER` / 400 envelope', () => {
108+
const err = refusalFor({ name: { $icontains: 'acme' } });
109+
expect(err).toBeInstanceOf(Error);
110+
expect(err).toMatchObject({ code: 'INVALID_FILTER', status: 400 });
111+
// The operator and the face's own vocabulary are both in the message, so
112+
// the author can see which spelling to reach for.
113+
expect(err?.message).toContain('$icontains');
114+
expect(err?.message).toContain('$contains');
115+
});
116+
117+
it('refuses over an EMPTY seed draft too — the walk is not a function of the data', () => {
118+
// The state a draft is authored in. A per-row refusal never fires here, so
119+
// the preview would answer `count: 0` for a filter it cannot evaluate.
120+
const err = refusalFor({ name: { $icontains: 'acme' } }, []);
121+
expect(err).toMatchObject({ code: 'INVALID_FILTER', status: 400 });
122+
});
123+
124+
it('refuses inside `$or`, `$and` and `$not` arms', () => {
125+
for (const where of [
126+
{ $or: [{ name: { $startsWith: 'Ac' } }, { amount: { $eq: -1 } }] },
127+
{ $and: [{ name: { $endsWith: 'Corp' } }] },
128+
{ $not: { name: { $notContains: 'zzz' } } },
129+
]) {
130+
expect(refusalFor(where)).toMatchObject({ code: 'INVALID_FILTER', status: 400 });
131+
}
132+
});
133+
134+
it('refuses a constraint key that names an Object.prototype member', () => {
135+
// A table keyed by a plain object literal would resolve `toString` to the
136+
// inherited function and CALL it as a predicate; a `Map` cannot.
137+
expect(refusalFor({ name: { toString: 'Acme' } })).toMatchObject({
138+
code: 'INVALID_FILTER',
139+
status: 400,
140+
});
141+
});
142+
143+
/**
144+
* The declared vocabulary, minus the ten arms this face carries. Derived
145+
* from `FILTER_OPERATORS` rather than hand-listed, so an operator added to
146+
* the protocol without an arm here lands in this table by itself instead of
147+
* going silent.
148+
*/
149+
const EVALUATED = ['$eq', '$ne', '$gt', '$gte', '$lt', '$lte', '$between', '$in', '$nin', '$contains'];
150+
const UNEVALUATED = [...FILTER_OPERATORS, '$like', '$ilike'].filter((op) => !EVALUATED.includes(op));
151+
152+
it('has a non-empty unevaluated set — otherwise the table below asserts nothing', () => {
153+
expect(UNEVALUATED).toEqual(
154+
expect.arrayContaining(['$notContains', '$startsWith', '$endsWith', '$icontains', '$null', '$exists', '$like', '$ilike']),
155+
);
156+
});
157+
158+
it.each(UNEVALUATED)('%s is refused, never answered for every row', (op) => {
159+
const where = { name: { [op]: 'acme' } } as Record<string, unknown>;
160+
expect(refusalFor(where)).toMatchObject({ code: 'INVALID_FILTER', status: 400 });
161+
expect(namesAnswered(where)).toEqual([]);
162+
});
163+
});
164+
165+
// ── direction 2 — the ten arms are untouched ────────────────────────────────
166+
167+
describe('[#19810] the operators the draft preview DOES evaluate are unchanged', () => {
168+
const ROW = { name: 'Acme Corp', amount: 800, spent_on: '2026-05-12' };
169+
170+
// [matches, does not match] for each arm — both directions, because a
171+
// fail-closed default that starts REJECTING correct matches is the
172+
// mirror-image defect of the one this card fixes.
173+
const MATRIX: Array<[string, Record<string, unknown>, Record<string, unknown>]> = [
174+
['$eq', { amount: { $eq: 800 } }, { amount: { $eq: 1200 } }],
175+
['$ne', { amount: { $ne: 1200 } }, { amount: { $ne: 800 } }],
176+
['$gt', { amount: { $gt: 700 } }, { amount: { $gt: 800 } }],
177+
['$gte', { amount: { $gte: 800 } }, { amount: { $gte: 801 } }],
178+
['$lt', { amount: { $lt: 900 } }, { amount: { $lt: 800 } }],
179+
['$lte', { amount: { $lte: 800 } }, { amount: { $lte: 799 } }],
180+
['$lte (bare-day, #3777)', { spent_on: { $lte: '2026-05-12' } }, { spent_on: { $lte: '2026-05-11' } }],
181+
['$between', { amount: { $between: [700, 900] } }, { amount: { $between: [900, 1000] } }],
182+
['$in', { name: { $in: ['Acme Corp', 'Globex'] } }, { name: { $in: ['Globex'] } }],
183+
['$nin', { name: { $nin: ['Globex'] } }, { name: { $nin: ['Acme Corp'] } }],
184+
['$contains', { name: { $contains: 'cme' } }, { name: { $contains: 'zzz' } }],
185+
['implicit equality', { name: 'Acme Corp' }, { name: 'Globex' }],
186+
['$and', { $and: [{ amount: { $gt: 700 } }] }, { $and: [{ amount: { $gt: 900 } }] }],
187+
['$or', { $or: [{ amount: { $gt: 900 } }, { name: { $contains: 'cme' } }] }, { $or: [{ amount: { $gt: 900 } }] }],
188+
['$not', { $not: { amount: { $eq: 1200 } } }, { $not: { amount: { $eq: 800 } } }],
189+
];
190+
191+
it.each(MATRIX)('%s still answers both directions', (_label, hit, miss) => {
192+
expect(matchesWhere(ROW, hit)).toBe(true);
193+
expect(matchesWhere(ROW, miss)).toBe(false);
194+
});
195+
196+
it('an absent `where` still matches every row', () => {
197+
expect(matchesWhere(ROW, undefined)).toBe(true);
198+
expect(namesAnswered({})).toEqual(expect.arrayContaining(['Acme Corp', 'Globex']));
199+
});
200+
});
201+
202+
// ── the refusal reaches the request path ────────────────────────────────────
203+
204+
describe('[#19810] the refusal propagates through queryDataset({ previewDrafts })', () => {
205+
it('refuses the drafted selection instead of charting every seed row', async () => {
206+
const service = new AnalyticsService({ draftRowsResolver: vi.fn(async () => ROWS) });
207+
await expect(
208+
service.queryDataset(
209+
DATASET,
210+
{ dimensions: ['name'], measures: ['count'], runtimeFilter: { name: { $icontains: 'acme' } } },
211+
undefined,
212+
{ previewDrafts: true },
213+
),
214+
).rejects.toMatchObject({ code: 'INVALID_FILTER', status: 400 });
215+
});
216+
});

0 commit comments

Comments
 (0)