From 3adb37d4276dcc3fd3b41149df851b1b3b632fdc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 15:22:27 +0000 Subject: [PATCH] fix(analytics): refuse a zero-operator field constraint on the draft preview `matchesWhere` iterated a field constraint's entries; `{ name: {} }` has none, so the row fell through to a match and the draft preview charted every row for a filter all three shipped drivers refuse. Refuse it in the INVALID_FILTER / 400 envelope the preview already speaks, from the row-independent gate (whole tree, nested combinators included) and from matchesWhere's field arm. Claude-Session: https://claude.ai/code/session_01AhQASwqJr2Z7XfGWUdvnbF Co-authored-by: Claude --- .../19835-preview-empty-field-constraint.md | 13 ++ .../preview-empty-field-constraint.test.ts | 142 ++++++++++++++++++ .../src/preview-evaluator.ts | 89 ++++++++++- 3 files changed, 239 insertions(+), 5 deletions(-) create mode 100644 .changeset/19835-preview-empty-field-constraint.md create mode 100644 packages/services/service-analytics/src/__tests__/preview-empty-field-constraint.test.ts diff --git a/.changeset/19835-preview-empty-field-constraint.md b/.changeset/19835-preview-empty-field-constraint.md new file mode 100644 index 00000000000..4625441c8ac --- /dev/null +++ b/.changeset/19835-preview-empty-field-constraint.md @@ -0,0 +1,13 @@ +--- +"@objectstack/service-analytics": patch +--- + +The draft-data preview **refuses** a field constraint with zero operators (`{ name: {} }`) instead of answering it with every row, so a drafted chart no longer shows rows for a filter publish refuses outright (#19835). + +`preview-evaluator.ts`'s `matchesWhere` iterated a field constraint's entries; an empty object has none, so the loop never ran and the row fell through to a MATCH. `matchesWhere({ name: 'Globex' }, { name: {} })` answered `true`. Every data driver refuses this shape (`driver-memory`, `driver-mongodb`, and `driver-sql` at the top level and inside `$and`/`$or`/`$not`), and so does this package's own `where` door, so the preview and publish gave opposite answers to the same filter. + +- **Refused in the ADR-0112 `INVALID_FILTER` / 400 envelope**, through the same `invalidFilterError` the preview already uses for an operator it cannot evaluate. No new error code and no new exported symbol. The message follows the drivers' wording: it names the constraint and its position (`where.$or[1].amount`), and gives the two legal repairs (name an operator, or write a direct comparand). +- **Not answered as "matches zero rows" either.** `{ status: {} }` does not mean "no rows". Read literally it means "rows whose status is anything", and the shape is almost always an authoring accident: a filter builder that recorded a field but never its operator. Only a refusal names the constraint to repair. +- **Nesting cannot route around it.** The check walks the whole `where` before any row is read, `$and` / `$or` / `$not` arms included. So a constraint in an `$or` arm that a matching row would short-circuit past still refuses, and so does a seed draft holding zero rows. +- ⚠️ **What it costs**: a drafted chart whose filter carries `{ field: {} }` now returns `400 INVALID_FILTER` in preview, where before it rendered a number computed over every row. Fix: name the operator the constraint was meant to carry, e.g. `{ status: { $eq: 'open' } }` or `{ status: 'open' }`. +- **Unchanged**: constraints that name an operator, implicit-equality comparands, and an empty *node* (`where: {}` or `$and: [{}]`, which is the identity and not a field constraint). diff --git a/packages/services/service-analytics/src/__tests__/preview-empty-field-constraint.test.ts b/packages/services/service-analytics/src/__tests__/preview-empty-field-constraint.test.ts new file mode 100644 index 00000000000..2ef01243a72 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/preview-empty-field-constraint.test.ts @@ -0,0 +1,142 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#19835] The draft-preview matcher answered a field constraint with ZERO + * operators — `{ name: {} }` — with EVERY row. + * + * `matchesWhere`'s per-field loop iterates the constraint's entries; an empty + * object has none, so the loop never ran and the row fell through to the + * closing `return true`. Probe at `origin/main` before the repair: + * `matchesWhere({ name: 'Globex' }, { name: {} })` → `true`. The #19810 + * operator-vocabulary refusal could not reach it: no key, no lookup to fail. + * + * Every shipped driver refuses the shape (`driver-memory` / `driver-mongodb` + * `emptyFieldConstraintError`, `driver-sql` at the top level and inside + * combinators) — ruled on #5240, refused everywhere. So the preview charted + * every row for a filter publish refuses outright. + * + * ## What this file pins + * + * 1. **Refused** — `INVALID_FILTER` / 400 (ADR-0112), pinned by `code` + + * `status`, never by a bare `toThrow()`. ⛔ Not "matches zero rows": that + * is the other silent reading the ruling declined. + * 2. **Not bypassable by nesting** — under `$and`, `$or` (including the arm a + * short-circuit would never reach) and `$not`, and over an EMPTY seed. + * 3. **Unchanged** — a constraint that names an operator, an implicit + * comparand, and an empty NODE (`{}` as the whole `where` or as a + * combinator arm — the identity, not a field constraint) answer exactly + * what they answered before. + */ + +import { describe, it, expect } from 'vitest'; +import { DatasetSchema } from '@objectstack/spec/ui'; +import type { Cube } from '@objectstack/spec/data'; +import { AnalyticsService } from '../analytics-service.js'; +import { evaluateAnalyticsQueryOverRows, matchesWhere } from '../preview-evaluator.js'; + +type Refusal = Error & { code?: string; status?: number }; + +const ROWS: Record[] = [ + { id: '1', name: 'Acme Corp', amount: 1200 }, + { id: '2', name: 'Globex', amount: 800 }, +]; + +const CUBE: Cube = new AnalyticsService().registerDataset( + DatasetSchema.parse({ + name: 'expense_ds', + label: 'Expense', + object: 'expense', + dimensions: [{ name: 'name', field: 'name', type: 'string', label: 'Name' }], + measures: [{ name: 'count', aggregate: 'count' }], + }), +).cube; + +function run(where: Record, rows = ROWS) { + return evaluateAnalyticsQueryOverRows( + { cube: 'expense_ds', measures: ['count'], dimensions: ['name'], where }, + CUBE, + rows, + ); +} + +function refusalFor(thunk: () => unknown): Refusal | undefined { + try { + thunk(); + return undefined; + } catch (e) { + return e as Refusal; + } +} + +function namesAnswered(where: Record): string[] { + return run(where).rows.map((r) => String(r.name)).sort(); +} + +describe('[#19835] a field constraint with zero operators on the draft preview', () => { + it('the card probe — `matchesWhere({ name: "Globex" }, { name: {} })` — refuses instead of matching', () => { + const err = refusalFor(() => matchesWhere({ name: 'Globex' }, { name: {} })); + expect(err).toBeInstanceOf(Error); + expect(err).toMatchObject({ code: 'INVALID_FILTER', status: 400 }); + }); + + it('refuses a whole preview query at the top level, naming the field and its position', () => { + const err = refusalFor(() => run({ name: {} })); + expect(err).toMatchObject({ code: 'INVALID_FILTER', status: 400 }); + expect(err?.message).toContain('where.name'); + expect(err?.message).toContain('{ "name": {} }'); + }); + + it('refuses over an EMPTY seed draft — the walk is not a function of the data', () => { + expect(refusalFor(() => run({ name: {} }, []))).toMatchObject({ code: 'INVALID_FILTER', status: 400 }); + }); + + describe('nesting cannot route around it', () => { + it('inside `$and`', () => { + const err = refusalFor(() => run({ $and: [{ name: 'Globex' }, { amount: {} }] })); + expect(err).toMatchObject({ code: 'INVALID_FILTER', status: 400 }); + expect(err?.message).toContain('where.$and[1].amount'); + }); + + it('inside `$or`, in the arm a short-circuit would never reach for a matching row', () => { + // Row `Globex` satisfies arm 0, so a per-row walk would `some()` past arm + // 1 and answer it; the refusal must not depend on which row is tested. + const err = refusalFor(() => run({ $or: [{ name: 'Globex' }, { amount: {} }] })); + expect(err).toMatchObject({ code: 'INVALID_FILTER', status: 400 }); + expect(err?.message).toContain('where.$or[1].amount'); + }); + + it('under `$not`', () => { + const err = refusalFor(() => run({ $not: { name: {} } })); + expect(err).toMatchObject({ code: 'INVALID_FILTER', status: 400 }); + expect(err?.message).toContain('where.$not.name'); + }); + + it('several combinators deep', () => { + const err = refusalFor(() => run({ $and: [{ $or: [{ $not: { name: {} } }] }] })); + expect(err).toMatchObject({ code: 'INVALID_FILTER', status: 400 }); + }); + + it('through `matchesWhere` directly, under `$and`', () => { + const err = refusalFor(() => matchesWhere({ name: 'Globex' }, { $and: [{ name: {} }] })); + expect(err).toMatchObject({ code: 'INVALID_FILTER', status: 400 }); + }); + }); + + describe('non-empty constraints answer exactly as before', () => { + it('an operator constraint', () => { + expect(namesAnswered({ name: { $eq: 'Globex' } })).toEqual(['Globex']); + expect(namesAnswered({ amount: { $gt: 1000 } })).toEqual(['Acme Corp']); + expect(matchesWhere({ name: 'Globex' }, { name: { $ne: 'Globex' } })).toBe(false); + }); + + it('an implicit-equality comparand', () => { + expect(namesAnswered({ name: 'Globex' })).toEqual(['Globex']); + }); + + it('an empty NODE — the whole `where`, or a combinator arm — is the identity, not a field constraint', () => { + expect(namesAnswered({})).toEqual(['Acme Corp', 'Globex']); + expect(namesAnswered({ $and: [{}] })).toEqual(['Acme Corp', 'Globex']); + expect(matchesWhere({ name: 'Globex' }, {})).toBe(true); + }); + }); +}); diff --git a/packages/services/service-analytics/src/preview-evaluator.ts b/packages/services/service-analytics/src/preview-evaluator.ts index 0e1bf30218d..b4aeff76fa1 100644 --- a/packages/services/service-analytics/src/preview-evaluator.ts +++ b/packages/services/service-analytics/src/preview-evaluator.ts @@ -22,7 +22,8 @@ // live only in the pending seed draft, so there is no live path to hand the // query to. It is REFUSED — `INVALID_FILTER` / 400, the envelope this package's // `where` door already speaks — and never answered true. See -// PREVIEW_FIELD_OPERATORS. +// PREVIEW_FIELD_OPERATORS. [#19835] So is a field constraint carrying ZERO +// operators (`{ name: {} }`), at any depth — see isEmptyFieldConstraint. import { calendarPartsInTzOrUtc, @@ -193,6 +194,66 @@ function previewUnevaluableOperatorError(op: string, field: string): Error { ); } +/** + * [#19835] Is this field spec `{}` — a field constrained by ZERO operators? + * + * A plain object with no own enumerable keys, and nothing else — the predicate + * `driver-memory`, `driver-mongodb`, `driver-sql` and `@objectstack/formula` + * each apply under the same name. A `Date`, a `RegExp` or a class instance also + * enumerates to nothing, but it is a COMPARAND rather than a constraint, so the + * prototype check keeps it out of this refusal exactly as it does there. + * + * Mirrored locally, not imported: the exported copy lives in `driver-memory` + * (`filter-refusal.ts`), a package this service does not depend on, and a + * dependency on a driver for a four-line predicate is the wrong direction. + */ +function isEmptyFieldConstraint(spec: unknown): boolean { + if (spec === null || typeof spec !== 'object' || Array.isArray(spec)) return false; + const proto = Object.getPrototypeOf(spec); + if (proto !== Object.prototype && proto !== null) return false; + return Object.keys(spec as Row).length === 0; +} + +/** + * [#19835] `{ field: {} }` — a field constrained by ZERO operators — REFUSED, + * in the same `INVALID_FILTER` / 400 envelope as + * {@link previewUnevaluableOperatorError}. + * + * ⛔ It used to MATCH EVERY ROW, and not by anyone's decision: the per-field + * loop in {@link matchesWhere} iterates the constraint's entries, an empty + * object has none, so the loop body never ran and the row fell through to the + * function's closing `return true`. The operator-vocabulary refusal (#19810) + * could not reach it either — with no key there is no operator to look up and + * no lookup to fail. + * + * Every shipped backend already refuses this shape (#5240 ruled it: refused + * everywhere, one wording): `driver-memory`'s and `driver-mongodb`'s + * `emptyFieldConstraintError`, and `driver-sql`'s at the top level and inside + * `$and`/`$or`/`$not` alike. This package's own `where` door refuses it too + * (`filter-normalizer`'s wrapper arm). So the draft preview charted every row + * for a filter publish never answers at all — the preview/publish divergence + * this evaluator's other refusals exist to make visible. + * + * ⛔ Refused, NOT answered as "matches nothing". `{ status: {} }` does not mean + * "no rows" — read literally it means "rows whose status is anything", and the + * shape is almost always an authoring accident (a filter builder that recorded + * a field and never its operator). Either silent reading hands the author a row + * count they never asked for; only the refusal names the constraint to repair. + * + * The message follows the drivers' wording (constraint, position, the two legal + * repairs) and carries no tracker number, per the runtime-string rule. + */ +function previewEmptyFieldConstraintError(field: string, path: string): Error { + return invalidFilterError( + `[analytics] Field constraint at ${path} carries zero operators ({ "${field}": {} }). A field ` + + `constraint must name at least one operator (e.g. { "${field}": { "$eq": "value" } }) or be a ` + + `direct comparand (e.g. { "${field}": "value" }). It is refused rather than evaluated: the ` + + `draft-data preview used to answer it with EVERY row, while every data driver and the live ` + + `analytics filter refuse it — so the drafted chart was drawn over rows the published one never ` + + `returns. It does not mean "no rows" either; name the operator the constraint was meant to carry.`, + ); +} + /** * [#19810] Refuse a `where` this face cannot evaluate BEFORE any row is read. * @@ -206,17 +267,30 @@ function previewUnevaluableOperatorError(op: string, field: string): Error { * * It mirrors {@link matchesWhere}'s own traversal exactly, malformed shapes * included — a non-array `$and` is left for `matchesWhere` to fault on as it - * always has, so this gate widens no refusal beyond the operator vocabulary. + * always has, so this gate widens no refusal beyond the operator vocabulary + * and the zero-operator constraint. + * + * [#19835] The zero-operator constraint is judged HERE, for the whole tree, + * rather than only where {@link matchesWhere} meets it: that walk + * short-circuits (`every`/`some`, and a node returns on its first false entry), + * so a `{ $or: [{ name: 'Globex' }, { amount: {} }] }` would refuse or answer + * depending on which ROW was being tested. A malformed filter is refused for + * every row or none — the posture `@objectstack/formula`'s `assertFilterShape` + * takes for the same shape — and nesting under `$and`/`$or`/`$not` cannot + * route around it, the position `driver-sql` once dropped it in. */ -function assertPreviewCanEvaluate(where: Record | undefined): void { +function assertPreviewCanEvaluate(where: Record | undefined, path = 'where'): void { if (!where) return; for (const [key, cond] of Object.entries(where)) { + const here = `${path}.${key}`; if (key === '$and' || key === '$or') { - if (Array.isArray(cond)) for (const arm of cond) assertPreviewCanEvaluate(arm as Row); + if (Array.isArray(cond)) cond.forEach((arm, i) => assertPreviewCanEvaluate(arm as Row, `${here}[${i}]`)); } else if (key === '$not') { if (cond !== null && typeof cond === 'object' && !Array.isArray(cond)) { - assertPreviewCanEvaluate(cond as Row); + assertPreviewCanEvaluate(cond as Row, here); } + } else if (isEmptyFieldConstraint(cond)) { + throw previewEmptyFieldConstraintError(key, here); } else if (cond !== null && typeof cond === 'object' && !Array.isArray(cond)) { for (const op of Object.keys(cond as Row)) { if (!PREVIEW_FIELD_OPERATORS.has(op)) throw previewUnevaluableOperatorError(op, key); @@ -240,6 +314,11 @@ export function matchesWhere(row: Row, where: Record | undefine if (!(cond as Row[]).some((c) => matchesWhere(row, c as Row))) return false; } else if (key === '$not') { if (matchesWhere(row, cond as Row)) return false; + } else if (isEmptyFieldConstraint(cond)) { + // [#19835] Zero entries would leave the loop below unrun and fall through + // to a MATCH. Refused here too, so a direct caller of this matcher gets + // the same answer the row-independent gate gives a whole query. + throw previewEmptyFieldConstraintError(key, key); } else if (cond !== null && typeof cond === 'object' && !Array.isArray(cond)) { for (const [op, expected] of Object.entries(cond as Row)) { if (!matchOp(row[key], op, expected, key)) return false;