From 5c82df0685bf46d8c00eb92aa43715b0b67686a4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 07:06:46 +0000 Subject: [PATCH] fix(app-shell): bridge `Is null` to the spec's `$null` instead of erasing the filter `groupToCondition` had no row for `isNull` / `isNotNull`, so an `Is null` row fell through to the unmapped-operator drop. A dropped last row makes the function return `undefined`, and the dataset inspector commits on every change, so switching the only condition's operator to an ordinary menu entry committed `{ filter: undefined }` and destroyed the stored `dataset.filter`. Nothing errored and the panel still showed the condition. Both directions now carry the spec's `$null` predicate, kept distinct from the `$exists` pair the two `isEmpty` rows already used. Operators this bridge does not map are still dropped rather than emitted in a spelling that means something else; that behaviour and the list of offered-but-unmappable operators are pinned alongside the fix. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01UzHd6hDYatoDn17BuwKxnZ --- .../9363-dataset-filter-isnull-erases.md | 29 +++ ...FilterCondition.nullOperators-9363.test.ts | 235 ++++++++++++++++++ .../inspectors/datasetFilterCondition.ts | 51 +++- 3 files changed, 312 insertions(+), 3 deletions(-) create mode 100644 .changeset/9363-dataset-filter-isnull-erases.md create mode 100644 packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.nullOperators-9363.test.ts diff --git a/.changeset/9363-dataset-filter-isnull-erases.md b/.changeset/9363-dataset-filter-isnull-erases.md new file mode 100644 index 0000000000..ab21c8303b --- /dev/null +++ b/.changeset/9363-dataset-filter-isnull-erases.md @@ -0,0 +1,29 @@ +--- +'@object-ui/app-shell': patch +--- + +Stop the Studio dataset-filter bridge from ERASING a stored filter when the author +picks `Is null` (objectui#9363). + +`groupToCondition` had no mapping for `isNull` / `isNotNull`, so those rows fell +through to the unmapped-operator drop. A dropped last row makes the function return +`undefined`, and the dataset inspector commits on every change — so an author with a +working `dataset.filter` (or a `measure.filter`) who opened the filter popover and +switched the single condition's operator to **Is null** committed `undefined`, and the +persisted filter was destroyed. Nothing errored and the panel still showed the +condition. `Is null` is an ordinary entry in that menu, not an opt-in one. + +Both directions now bridge the spec's `$null` predicate: `isNull` serializes to +`{ field: { $null: true } }` and `isNotNull` to `{ $null: false }`, and a stored +`$null` reads back as the operator the author picked instead of degrading the whole +filter to "edit it in the Source tab". + +`$null` stays distinct from `$exists`: `isEmpty` / `isNotEmpty` are unchanged, because +the dropdown offers both pairs as their own rows and the spec's filter vocabulary +carries both predicates. + +Operators this bridge still does not map are still dropped rather than emitted in a +spelling that means something else — that behaviour is deliberate and is now pinned +alongside the fix, together with the list of operators the menu offers and this bridge +cannot store (`notContains`, `between`, `startsWith`, `endsWith`), so the next unmapped +addition fails a test instead of erasing a filter. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.nullOperators-9363.test.ts b/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.nullOperators-9363.test.ts new file mode 100644 index 0000000000..382b4a4f5a --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.nullOperators-9363.test.ts @@ -0,0 +1,235 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `Is null` must not ERASE the stored dataset filter (objectui#9363). + * + * ## The mechanism, and why it is data loss rather than a wrong result set + * + * `groupToCondition` returns `undefined` when no row survives serialization, + * and the Studio dataset inspector commits on EVERY change — the filter popover + * mounts the shared `FilterBuilder` and hands each emitted group straight to + * `onCommit(groupToCondition(g))`, which lands as `onPatch({ filter })` and is + * spread over the draft. So an author with a working `dataset.filter` who opens + * the inspector and switches the single condition's operator to **Is null** + * commits `undefined`: the persisted key is destroyed, nothing errors, and the + * panel still shows the condition. + * + * The inspector passes no `extraOperators`, so `isNull` / `isNotNull` are + * ordinary entries in the default menu — not opt-in ones. The parity block at + * the bottom of this file reads that offering from the builder's own exported + * bucket function rather than restating it. + * + * ## The distinction this file exists to keep + * + * The `continue` these two operators fell into is a DELIBERATE decision for + * operators the bridge does not map: dropping beats emitting a filter that + * means something else. That behaviour is kept, and pinned below, because a + * repair that made the fallback stop dropping everything it cannot map would + * emit wrong filters — a worse defect than this one. + * + * `isNull` / `isNotNull` are a different case and that is the whole repair: the + * dialect CAN express them (the spec's own `$null`, asserted below against + * `FILTER_OPERATORS` rather than assumed), the builder draws them as COMPLETE + * rows with no value input, and this inspector offers them. The drop was an + * unhandled operator falling into the fallback's path, not the fallback doing + * its job. Both readings are pinned so they stay distinguishable. + * + * ## Red-first + * + * Before the repair, the two `$null` expectations fail with `undefined` while + * the `equals` control in the same run passes — a table of all-`undefined` + * answers and a dead function are otherwise indistinguishable. + */ +import { describe, it, expect } from 'vitest'; +import { FILTER_OPERATORS, FieldOperatorsSchema } from '@objectstack/spec/data'; +import { + FILTER_BUILDER_OPERATORS, + VALUELESS_FILTER_BUILDER_OPERATORS, + operatorsForFieldType, +} from '@object-ui/components'; +import { groupToCondition, conditionToGroup } from './datasetFilterCondition'; +import type { BuilderGroup } from './datasetFilterCondition'; + +/** One condition row, as the builder emits it. */ +const row = (operator: string, value: unknown = ''): BuilderGroup => ({ + id: 'g', + logic: 'and', + conditions: [{ id: 'c1', field: 'closed_at', operator, value }], +}); + +describe('groupToCondition — the null predicates this inspector offers (objectui#9363)', () => { + it('CONTROL: a mapped operator still serializes, so an empty answer below is about that operator', () => { + expect(groupToCondition(row('equals', 'acme'))).toEqual({ closed_at: { $eq: 'acme' } }); + }); + + it('isNull serializes to the dialect\'s null predicate instead of vanishing', () => { + expect( + groupToCondition(row('isNull')), + 'an `Is null` row serialized to nothing; committing that ERASES dataset.filter', + ).toEqual({ closed_at: { $null: true } }); + }); + + it('isNotNull serializes to the same predicate negated', () => { + expect(groupToCondition(row('isNotNull'))).toEqual({ closed_at: { $null: false } }); + }); + + it('keeps a null row alongside a complete one instead of dropping either', () => { + expect(groupToCondition({ + id: 'g', + logic: 'and', + conditions: [ + { id: 'c1', field: 'stage', operator: 'equals', value: 'won' }, + { id: 'c2', field: 'closed_at', operator: 'isNull', value: '' }, + ], + })).toEqual({ $and: [{ stage: { $eq: 'won' } }, { closed_at: { $null: true } }] }); + }); + + it('THE DEFECT: switching the only row of a stored filter to Is null no longer commits `undefined`', () => { + // The exact author gesture: a dataset that already has a filter, opened in + // the inspector, one operator change. `undefined` here is not "unchanged" — + // it is what the host spreads over the draft as `{ filter: undefined }`, + // the same patch shape `objectChangePatch` uses to CLEAR the filter. + const stored = { stage: { $eq: 'won' } }; + const { group, representable } = conditionToGroup(stored); + expect(representable).toBe(true); + const edited: BuilderGroup = { + ...group, + conditions: [{ ...group.conditions[0], operator: 'isNull', value: '' }], + }; + expect( + groupToCondition(edited), + 'the commit for this gesture was `undefined`, which erases the stored filter', + ).toEqual({ stage: { $null: true } }); + }); + + it('leaves the $exists pair exactly as it was', () => { + expect(groupToCondition(row('isEmpty'))).toEqual({ closed_at: { $exists: false } }); + expect(groupToCondition(row('isNotEmpty'))).toEqual({ closed_at: { $exists: true } }); + }); + + it('still drops an operator it does not map, rather than emitting a wrong filter', () => { + // Deliberate, and kept: see this file's header. These four are OFFERED by + // the menu and dropped, which erases the same way — tracked as its own + // finding, not widened here on the way past. + expect(groupToCondition(row('notContains', 'a'))).toBeUndefined(); + expect(groupToCondition(row('between', [1, 5]))).toBeUndefined(); + expect(groupToCondition(row('startsWith', 'a'))).toBeUndefined(); + expect(groupToCondition(row('endsWith', 'a'))).toBeUndefined(); + }); + + it('an empty group is still `undefined` — that is the author CLEARING the filter', () => { + expect(groupToCondition({ id: 'g', logic: 'and', conditions: [] })).toBeUndefined(); + }); +}); + +describe('the emitted token is the spec\'s, not a local invention (objectui#9363)', () => { + it('`$null` is a member of the spec\'s filter operator vocabulary', () => { + expect(FILTER_OPERATORS).toContain('$null'); + // Negative control: membership is a real reading, not a list that contains + // everything. A plausible-looking spelling this bridge could have invented + // is NOT in it. + expect(FILTER_OPERATORS).not.toContain('$isNull'); + }); + + it('the spec\'s field-operator door accepts the boolean comparand and refuses a wrong one', () => { + expect(FieldOperatorsSchema.safeParse({ $null: true }).success).toBe(true); + expect(FieldOperatorsSchema.safeParse({ $null: false }).success).toBe(true); + // Negative control: this door judges the VALUE, so a string comparand is + // refused — without this leg the assertion above would pass for a schema + // that accepts anything. + expect(FieldOperatorsSchema.safeParse({ $null: 'yes' }).success).toBe(false); + }); +}); + +describe('conditionToGroup — the read half round-trips the new shape (objectui#9363)', () => { + it('reads a stored $null back as the operator the author picked', () => { + expect(conditionToGroup({ closed_at: { $null: true } })).toEqual({ + group: { id: 'g', logic: 'and', conditions: [{ id: 'c0', field: 'closed_at', operator: 'isNull', value: '' }] }, + representable: true, + }); + expect(conditionToGroup({ closed_at: { $null: false } }).group.conditions[0].operator) + .toBe('isNotNull'); + }); + + it('round-trips condition → group → condition', () => { + for (const c of [{ closed_at: { $null: true } }, { closed_at: { $null: false } }]) { + const { group, representable } = conditionToGroup(c); + expect(representable, `${JSON.stringify(c)} fell back to the source editor`).toBe(true); + expect(groupToCondition(group)).toEqual(c); + } + }); +}); + +/** + * Offered ⇄ expressible parity for THIS inspector. + * + * The direction that broke: every guard in the repo sweeps spec → objectui, + * asking whether an operator an author may DECLARE can be rendered. None asks + * whether an operator this dropdown OFFERS can be stored by the consumer that + * mounted it — and that is the direction where an unmapped operator becomes + * silent data loss rather than a rendering gap. + * + * The offering is read from the builder's own bucket function with NO + * `extraOperators`, which is exactly what `DatasetFilterField` passes, so a + * future opt-in granted at that call site has to come through here. + */ +const PROBE_FIELD_TYPES: ReadonlyArray = [ + undefined, 'text', 'a_type_this_builder_has_never_heard_of', 'number', 'currency', + 'percent', 'rating', 'boolean', 'date', 'datetime', 'time', 'select', 'status', + 'lookup', 'master_detail', 'user', +]; + +function offeredAcrossBuckets(extra: readonly string[]): string[] { + const ids = new Set(); + for (const type of PROBE_FIELD_TYPES) for (const op of operatorsForFieldType(type, extra)) ids.add(op.value); + return [...ids].sort(); +} + +/** What the dataset inspector's filter popover offers. */ +const OFFERED = offeredAcrossBuckets([]); + +/** + * Offered, and deliberately NOT expressible by this bridge today. + * + * Each one drops on commit, and a drop of the last surviving row erases the + * stored filter — the same mechanism objectui#9363 fixed for the null pair. + * They are listed rather than fixed here because mapping them is a separate + * decision per operator (`between` needs a both-bounds-present rule before it + * can be emitted at all), and a blanket "stop dropping" would emit filters that + * mean something else. Mapping one is what makes this list shrink — and this + * assertion go red until it is updated. + */ +const DECLARED_UNEXPRESSIBLE = ['between', 'endsWith', 'notContains', 'startsWith']; + +/** A value that keeps a row from being dropped as INCOMPLETE, per operator. */ +function probeValue(operator: string): unknown { + if (VALUELESS_FILTER_BUILDER_OPERATORS.has(operator)) return ''; + if (operator === 'in' || operator === 'notIn') return ['a', 'b']; + if (operator === 'between') return [1, 5]; + return 'x'; +} + +describe('every operator this inspector OFFERS is either expressible or declared (objectui#9363)', () => { + it('the probe types cover every bucket, so the offering below is the whole dropdown', () => { + // Granting every id as an opt-in yields the full drawable vocabulary only + // if the probe list reaches every bucket. Without this, a bucket added + // later would silently shrink what the partition below is asserted over. + expect(offeredAcrossBuckets(FILTER_BUILDER_OPERATORS)).toEqual([...FILTER_BUILDER_OPERATORS].sort()); + }); + + it('partitions the offering exactly — no operator is silently unhandled', () => { + const expressible: string[] = []; + const dropped: string[] = []; + for (const operator of OFFERED) { + (groupToCondition(row(operator, probeValue(operator))) === undefined ? dropped : expressible) + .push(operator); + } + expect(dropped.sort()).toEqual(DECLARED_UNEXPRESSIBLE); + // The other half of the equality: every remaining offered id serializes. + expect(expressible.sort()).toEqual(OFFERED.filter((o) => !DECLARED_UNEXPRESSIBLE.includes(o))); + // And the null pair is on the expressible side — the card's defect, stated + // as a fact about the offering rather than about two literals. + expect(expressible).toContain('isNull'); + expect(expressible).toContain('isNotNull'); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.ts b/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.ts index 314d05c5b0..1cd8052cd4 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.ts @@ -11,6 +11,11 @@ * (nested groups, `$or`, multi-operator objects, unmapped operators) is reported * as NOT representable so the caller can fall back to the source editor instead * of silently corrupting the author's filter. + * + * The value-less operators are the exception to "field op value": the builder + * draws no input for them, so the row is complete without one. Both pairs the + * spec's vocabulary carries — `$exists` (is empty) and `$null` (is null) — are + * bridged here, in {@link VALUELESS_TO_MONGO}. */ /** FilterBuilder camelCase operator → FilterCondition Mongo operator. */ @@ -26,6 +31,32 @@ const MONGO_TO_OP: Record = { $contains: 'contains', $in: 'in', $nin: 'notIn', }; +/** + * Value-less builder operators, and the predicate each one lowers to. + * + * A row carrying one of these is COMPLETE without a value — the builder draws + * no input for it — so they are matched ahead of the value-completeness check + * in {@link groupToCondition}, not after it. + * + * `isNull` / `isNotNull` are not a spelling of `isEmpty` / `isNotEmpty`. The + * dropdown offers both pairs as their own rows and the spec's filter vocabulary + * carries both `$null` and `$exists`, so they stay distinct in both directions; + * collapsing them would draw two labels for one wire predicate and rewrite the + * author's choice when the filter is read back. + * + * objectui#9363: the null pair was missing here, so an `Is null` row — an + * ordinary entry in this inspector's menu, drawn as a finished row — fell + * through to the unmapped-operator `continue` below and was dropped. Dropping + * the last surviving row makes this function return `undefined`, and the + * inspector commits that as `{ filter: undefined }`, the same patch shape used + * to CLEAR the filter. So picking the entry erased the author's stored filter, + * with no error and the condition still on screen. + */ +const VALUELESS_TO_MONGO: Record> = { + isEmpty: { $exists: false }, isNotEmpty: { $exists: true }, + isNull: { $null: true }, isNotNull: { $null: false }, +}; + export interface BuilderCondition { id?: string; field: string; operator: string; value?: unknown } export interface BuilderGroup { id?: string; logic: 'and' | 'or'; conditions: BuilderCondition[] } @@ -53,10 +84,17 @@ export function groupToCondition(group: BuilderGroup | undefined): FilterConditi const conds = (group?.conditions ?? []).filter((c) => c && c.field); const parts: FilterCondition[] = []; for (const c of conds) { - if (c.operator === 'isEmpty') { parts.push({ [c.field]: { $exists: false } }); continue; } - if (c.operator === 'isNotEmpty') { parts.push({ [c.field]: { $exists: true } }); continue; } + const valueless = VALUELESS_TO_MONGO[c.operator]; + if (valueless) { parts.push({ [c.field]: { ...valueless } }); continue; } const mop = OP_TO_MONGO[c.operator]; - if (!mop) continue; // unmapped (e.g. notContains/between) — drop rather than emit a bad filter + // Still dropped rather than emitted in a spelling that means something + // else. ⚠️ The drop is not free: it is what erases the stored filter when + // no other row survives (see VALUELESS_TO_MONGO), and this menu offers + // `notContains` / `between` / `startsWith` / `endsWith`, none of which this + // table maps. Mapping one is a per-operator decision — `between` needs a + // both-bounds-present rule before it can be emitted at all — so they are + // declared, and pinned, in `datasetFilterCondition.nullOperators-9363`. + if (!mop) continue; // Skip incomplete rows (no value typed yet) — emitting `{field:{$op:''}}` would // be a silently-wrong filter (matches only empty), not "no filter". const v = c.value; @@ -96,6 +134,13 @@ export function conditionToGroup(cond: FilterCondition | undefined | null): { gr const mop = opKeys[0]; if (mop === '$exists') { conditions.push({ id: `c${i}`, field, operator: v.$exists ? 'isNotEmpty' : 'isEmpty', value: '' }); + } else if (mop === '$null') { + // The inverse of the write half: `$null: false` is "is not null", so + // the boolean picks the operator rather than becoming the row's value. + // Without this arm a filter this bridge now WRITES would read back as + // non-representable, sending the author to the Source tab for a row the + // builder can draw. + conditions.push({ id: `c${i}`, field, operator: v.$null ? 'isNull' : 'isNotNull', value: '' }); } else { const op = MONGO_TO_OP[mop]; if (!op) return { group: empty, representable: false };