diff --git a/.changeset/9372-unmapped-operator-inert.md b/.changeset/9372-unmapped-operator-inert.md new file mode 100644 index 0000000000..c6da2950c9 --- /dev/null +++ b/.changeset/9372-unmapped-operator-inert.md @@ -0,0 +1,47 @@ +--- +'@object-ui/app-shell': patch +--- + +Stop the Studio dataset-filter bridge from ERASING a stored filter when an edit +cannot be serialized (objectui#9372). Behaviour change, not just a fix: three more +operators are now STORED where they used to be dropped. + +**The erase.** `groupToCondition` answers `undefined` both when the author CLEARED the +filter and when nothing survived serialization, and the inspector — which commits on +every change — treated the two the same. The host applies patches as +`{ ...draft, ...patch }`, so that commit SET `dataset.filter` (or a `measure.filter`) +to `undefined`, which is exactly the patch shape `objectChangePatch` uses deliberately +to erase it. Nothing errored. + +Two ordinary gestures reached it. Switching the only condition's operator to one this +bridge did not map — `notContains`, `startsWith`, `endsWith` and `between`, all four +ordinary entries in this inspector's menu, none of them opt-in. And, needing no +operator at all, simply BLANKING the value of the only row: an incomplete row is +dropped by the same path, the last part goes with it, and the answer is `undefined`. + +**The fix, unconditional and ahead of any per-operator question.** The two meanings are +now distinguished: a group that still holds rows commits NOTHING and the stored filter +is left alone; only a group with no rows — Clear all, or the last row removed — still +commits `undefined`, because that is the author's own gesture. An operator this bridge +cannot express is therefore inert, whichever operators it maps. + +⚠️ Deliberately not "emit something anyway". A filter emitted in a spelling that means +something else is worse than one that was dropped, so the unmapped arm still drops. + +**And three of the four are no longer unmapped.** `notContains`, `startsWith` and +`endsWith` now serialize to the spec's own `$notContains` / `$startsWith` / `$endsWith` +and read back as the operator the author picked. The comment calling them operators +"this dialect genuinely cannot express" was stale: `FILTER_OPERATORS` carries all four. +Each is backed by a conformance reading rather than a guess — the Filter Protocol's +canonical `FILTER_TEXT_CASES` covers all three, the spec's declared-type door passes +them over `text` and refuses them over `number` / `date` / `boolean`, and this builder +offers them only on its text bucket. + +`between` stays unmapped, for a reason about this bridge rather than the vocabulary: +the builder pads a half-typed pair with an empty bound and the spec's comparand door +accepts `[1, '']`, so emitting it needs a both-bounds-present rule first. It is now +unmapped and inert instead of unmapped and destructive. + +Forward note for anyone pinning stored filters: a dataset filter written by this +version may carry `$notContains` / `$startsWith` / `$endsWith`, which an older +app-shell reads as non-representable and degrades to "edit it in the Source tab". diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.filterInert-9372.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.filterInert-9372.test.tsx new file mode 100644 index 0000000000..b42d5fc5bb --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.filterInert-9372.test.tsx @@ -0,0 +1,116 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The inspector does not COMMIT an erase (objectui#9372). + * + * ## Why this file exists next to the pure one + * + * `datasetFilterCondition.unmappedInert-9372.test.ts` pins the decision — + * {@link isClearedGroup} tells the author's CLEAR gesture apart from a + * serialization that produced nothing. A decision nobody consults is a channel + * with no reader, and the pure file cannot tell the difference: it would stay + * green with the guard sitting unused beside the old `onCommit(...)` call. + * + * This file drives the real component, with the real `FilterBuilder` mounted + * inside it, and watches the ONE thing that caused the data loss — the patch. + * + * ## The gesture, and why it is the value and not the operator menu + * + * The card's route is an operator pick, but the same defect is reachable by + * blanking the VALUE of the only row, with no operator involved at all: the + * incomplete-row `continue` drops it, the last part goes, and the commit is + * `undefined`. The host applies patches as `{ ...draft, ...patch }`, so that + * commit SETS `filter` to `undefined` — the patch shape `objectChangePatch` + * uses deliberately to erase it. Blanking a text input is also the one gesture + * that needs no Radix listbox interaction, so this pin holds without driving a + * select open in a headless DOM. + * + * ## The control + * + * "`onPatch` was not called" is also what an unopened popover, a mis-queried + * input and a dead handler all look like. So the same file types a REAL value + * through the same input and asserts the patch that produces — if that control + * stops firing, the absence below stops meaning anything. + */ +import { describe, it, expect, vi, afterEach, type Mock } from 'vitest'; +import { render, screen, fireEvent, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; + +// Stub the catalog hooks so the inspector renders without a MetadataClient / +// network, but with ONE text field so the filter popover has something to draw. +vi.mock('./useDatasetFields', () => ({ + useObjectOptions: () => ({ options: [], loading: false }), + useDatasetFieldCatalog: () => ({ + relationships: [], + fieldOptions: [{ value: 'name', label: 'Name', type: 'text' }], + loading: false, + }), + useDatasetUsage: () => ({ reports: 0, dashboards: 0, loading: false }), + fieldTypeToDimensionType: (t: string) => (t === 'date' ? 'date' : 'string'), +})); + +import { DatasetDefaultInspector } from './DatasetDefaultInspector'; + +afterEach(cleanup); + +const baseProps = { type: 'dataset', name: 'sales', locale: 'en-US' as const }; + +/** A dataset whose filter is ALREADY stored — the thing that got destroyed. */ +const draft = { + name: 'sales', + label: 'Sales', + object: 'opportunity', + include: [], + dimensions: [], + measures: [], + filter: { name: { $eq: 'acme' } }, +}; + +/** The inspector's patch channel, typed as the component declares it. */ +type PatchSpy = Mock<(patch: Record) => void>; +const patchSpy = (): PatchSpy => vi.fn<(patch: Record) => void>(); + +/** Render, open the Scope filter popover, and hand back the row's value input. */ +function openScopeFilter(onPatch: PatchSpy) { + render(); + // The trigger summarises the stored filter; seeing it at all is already a + // reading that `conditionToGroup` found the stored shape representable. + fireEvent.click(screen.getByText('1 condition')); + return screen.getByDisplayValue('acme') as HTMLInputElement; +} + +describe('DatasetDefaultInspector — a filter edit that cannot be stored commits nothing (objectui#9372)', () => { + it('CONTROL: typing a real value still commits it, so the absence below is about the blank', () => { + const onPatch = patchSpy(); + const input = openScopeFilter(onPatch); + fireEvent.change(input, { target: { value: 'contoso' } }); + expect(onPatch).toHaveBeenCalledWith({ filter: { name: { $eq: 'contoso' } } }); + }); + + it('THE DEFECT: blanking the only row\'s value does NOT patch `filter` to undefined', () => { + const onPatch = patchSpy(); + const input = openScopeFilter(onPatch); + fireEvent.change(input, { target: { value: '' } }); + // Before the repair this called `onPatch({ filter: undefined })`, which the + // host spreads over the draft — the stored filter destroyed, nothing shown + // to the author, and `JSON.stringify` then omits the key on save. + for (const [patch] of onPatch.mock.calls) { + expect( + patch, + 'the inspector committed a patch carrying `filter`; if it is undefined, that ERASES the stored filter', + ).not.toHaveProperty('filter'); + } + }); + + it('and the author\'s own CLEAR gesture still reaches the draft', () => { + // The other half: "Clear all" empties the group, which IS the clear + // gesture, and must still commit `undefined`. Without this the repair + // could have been a blanket "never commit undefined", stranding the filter + // an author asked to remove. + const onPatch = patchSpy(); + render(); + fireEvent.click(screen.getByText('1 condition')); + fireEvent.click(screen.getByText('Clear all')); + expect(onPatch).toHaveBeenCalledWith({ filter: undefined }); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx index b16666d8d6..15276fb767 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx @@ -34,7 +34,7 @@ import { InspectorComboField, type InspectorComboOption } from './InspectorCombo import { toFieldName } from '../previews/object-fields-io.js'; import { formatMeasure } from '@object-ui/core'; import { useDisplayLocale } from '@object-ui/i18n'; -import { conditionToGroup, groupToCondition, type FilterCondition } from './datasetFilterCondition.js'; +import { conditionToGroup, groupToCondition, isClearedGroup, type BuilderGroup, type FilterCondition } from './datasetFilterCondition.js'; import { useObjectOptions, useDatasetFieldCatalog, @@ -244,6 +244,31 @@ function DatasetFilterField({ label, help, value, onCommit, fields, disabled }: }) { const { group, representable } = conditionToGroup(value); const count = group.conditions.length; + /** + * Commit an edit — unless nothing survived serialization while rows are + * still on screen (objectui#9372). + * + * `groupToCondition` answers `undefined` both when the author CLEARED the + * filter and when every row was dropped, and this commit is what turns the + * second one into data loss: `onCommit` lands as `onPatch({ filter })`, the + * host applies it as `{ ...draft, ...patch }`, so `filter` is SET to + * `undefined` — the very patch shape `objectChangePatch` uses to erase it. + * An unmapped operator (`between`) or a blanked value on the only row would + * therefore destroy a working stored filter, silently. + * + * Holding the patch leaves the stored value alone, which is the whole + * requirement. ⛔ It is deliberately not "emit something anyway": a filter in + * a spelling that means something else is worse than one that was dropped. + * ⚠️ Known and accepted: the builder re-seeds its own state from `value` + * whenever the two differ, so an unexpressible row is lost from the panel on + * the next render the inspector happens to do. Losing an edit the bridge + * could never have stored is not in the same class as destroying one it had. + */ + const commitFilterGroup = (g: BuilderGroup) => { + const next = groupToCondition(g); + if (next === undefined && !isClearedGroup(g)) return; + onCommit(next); + }; return (
@@ -263,7 +288,7 @@ function DatasetFilterField({ label, help, value, onCommit, fields, disabled }: {fields.length === 0 ? (

Pick a base object to add filter conditions.

) : ( - onCommit(groupToCondition(g))} /> + )} 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 index 382b4a4f5a..4c8f8735f3 100644 --- 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 @@ -108,13 +108,14 @@ describe('groupToCondition — the null predicates this inspector offers (object }); 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(); + // Deliberate, and kept: see this file's header. + // + // objectui#9372 took the other three of the four this listed — the + // `notContains` / `startsWith` / `endsWith` rows are asserted as EMITTED + // in `datasetFilterCondition.unmappedInert-9372`, with the conformance + // reading behind each — and made the remaining drop inert. `between` is + // what is left: still offered, still dropped, and no longer destructive. 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', () => { @@ -191,15 +192,21 @@ 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. + * Each one drops on commit. ⚠️ That drop used to ERASE the stored filter when + * no other row survived — the same mechanism objectui#9363 fixed for the null + * pair — and objectui#9372 ended that: the caller now tells "nothing survived" + * apart from "the author cleared", so a drop is inert + * (`datasetFilterCondition.unmappedInert-9372`). Being on this list is now a + * missing capability, not data loss. + * + * objectui#9372 also took three of the four this listed. `between` is what + * remains, and it remains for a reason that is about THIS bridge rather than + * the spec's vocabulary: the builder pads a half-typed pair with `''` and the + * spec's comparand door accepts `[1, '']`, so it needs a both-bounds-present + * rule before it can be emitted at all. Mapping it is what makes this list + * shrink — and this assertion go red until it is updated. */ -const DECLARED_UNEXPRESSIBLE = ['between', 'endsWith', 'notContains', 'startsWith']; +const DECLARED_UNEXPRESSIBLE = ['between']; /** A value that keeps a row from being dropped as INCOMPLETE, per operator. */ function probeValue(operator: string): unknown { diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.test.ts b/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.test.ts index b26d609ab4..65f5536a20 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.test.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.test.ts @@ -21,7 +21,12 @@ describe('datasetFilterCondition', () => { }); it('drops unmapped operators rather than emitting a bad filter', () => { - expect(groupToCondition({ logic: 'and', conditions: [{ field: 'x', operator: 'notContains', value: 'a' }] })) + // The claim is unchanged; the FIXTURE moved. `notContains` stopped being + // an unmapped operator in objectui#9372 (it is bridged to `$notContains`, + // asserted there), so keeping it here would have pinned a branch it no + // longer reaches — an assertion that passes because nothing is produced. + // `between` is the operator this bridge still declines to emit. + expect(groupToCondition({ logic: 'and', conditions: [{ field: 'x', operator: 'between', value: [1, 5] }] })) .toBeUndefined(); }); 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 1cd8052cd4..a0389a42ed 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.ts @@ -16,6 +16,10 @@ * 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}. + * + * An operator that is NOT bridged is dropped, and dropping is where the danger + * used to be: see {@link isClearedGroup} for why an unmapped operator is now + * inert rather than destructive (objectui#9372). */ /** FilterBuilder camelCase operator → FilterCondition Mongo operator. */ @@ -24,11 +28,19 @@ const OP_TO_MONGO: Record = { greaterThan: '$gt', greaterOrEqual: '$gte', lessThan: '$lt', lessOrEqual: '$lte', after: '$gt', before: '$lt', contains: '$contains', in: '$in', notIn: '$nin', + // objectui#9372. The builder offers these three only on its TEXT bucket, + // which is the side the spec's declared-type door passes them on + // (`TEXT_OPERATOR_DOOR_CASES`: `passes` over `text`, `door-refusal` over + // `number` / `date` / `boolean`), and every filter backend answers them + // against the same canonical table (`FILTER_TEXT_CASES`). So mapping them is + // a bridge to a predicate the platform already agrees on, not a new claim. + notContains: '$notContains', startsWith: '$startsWith', endsWith: '$endsWith', }; const MONGO_TO_OP: Record = { $eq: 'equals', $ne: 'notEquals', $gt: 'greaterThan', $gte: 'greaterOrEqual', $lt: 'lessThan', $lte: 'lessOrEqual', $contains: 'contains', $in: 'in', $nin: 'notIn', + $notContains: 'notContains', $startsWith: 'startsWith', $endsWith: 'endsWith', }; /** @@ -79,21 +91,81 @@ export type { FilterCondition } from '@objectstack/spec/data'; import type { FilterCondition } from '@objectstack/spec/data'; +/** + * The rows this bridge will even look at. A row with no field picked is not + * yet a row — the builder seeds one the moment "Add condition" is clicked — + * so it is neither serialized nor counted as something the author typed. + * + * One definition, two readers: {@link groupToCondition} filters by it and + * {@link isClearedGroup} counts it. Two copies of this predicate is exactly + * how "the group is empty" and "the group serialized to nothing" could drift + * apart again. + */ +function liveRows(group: BuilderGroup | undefined): BuilderCondition[] { + return (group?.conditions ?? []).filter((c) => c && c.field); +} + +/** + * Is an `undefined` answer from {@link groupToCondition} the author CLEARING + * the filter (objectui#9372)? + * + * ## The conflation this exists to end + * + * `undefined` out of {@link groupToCondition} meant two different things — + * *"the author cleared the filter"* and *"nothing survived serialization"* — + * and the only caller treated both as clear. Since the inspector commits on + * every change, and the host applies patches as `{ ...draft, ...patch }`, that + * commit SETS `filter` to `undefined`: the same patch shape + * `objectChangePatch` uses deliberately to erase it. So a serialization that + * produced nothing destroyed the author's stored filter. + * + * Reachable two ways, and both are the same defect: + * + * - switching the only row to an operator this bridge does not map + * (objectui#9363 closed `isNull` / `isNotNull`; `between` is still one); + * - blanking the VALUE of the only row, which needs no operator at all — the + * incomplete-row `continue` drops it and the last part goes with it. + * + * ## What the caller does with the answer + * + * `false` means "rows are still on screen": the caller must patch NOTHING and + * leave the stored value alone. `true` — no rows at all, i.e. Clear all, or + * the last row removed — is the author's own gesture and still commits + * `undefined`. + * + * ⛔ Deliberately not "emit something for the unmapped operator". A filter + * emitted in a spelling that means something else is worse than a dropped one, + * which is the whole reason the unmapped arm exists; this makes the drop inert, + * it does not stop it dropping. + */ +export function isClearedGroup(group: BuilderGroup | undefined): boolean { + return liveRows(group).length === 0; +} + /** Serialize the visual group → a spec FilterCondition (flat `$and`). */ export function groupToCondition(group: BuilderGroup | undefined): FilterCondition | undefined { - const conds = (group?.conditions ?? []).filter((c) => c && c.field); + const conds = liveRows(group); const parts: FilterCondition[] = []; for (const c of conds) { const valueless = VALUELESS_TO_MONGO[c.operator]; if (valueless) { parts.push({ [c.field]: { ...valueless } }); continue; } const mop = OP_TO_MONGO[c.operator]; // 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`. + // else — that decision is the reason this arm exists and it is unchanged. + // + // What changed (objectui#9372) is the COST of the drop. It used to erase + // the author's stored filter whenever no other row survived; now + // {@link isClearedGroup} lets the caller tell that apart from a real clear, + // so an unmapped operator is inert. ⚠️ Do not read the drop as "this + // dialect cannot express it": the spec's `FILTER_OPERATORS` carries + // `$notContains`, `$startsWith`, `$endsWith` AND `$between`. The three text + // ones are mapped above. `between` is the one still offered here (on the + // date bucket) and still unmapped, for a reason that is about THIS bridge + // rather than the vocabulary: the builder pads a half-typed pair with `''` + // and the spec's comparand door accepts `[1, '']`, so emitting it needs a + // both-bounds-present rule first. The partition is pinned in + // `datasetFilterCondition.nullOperators-9363`, the inertness in + // `datasetFilterCondition.unmappedInert-9372`. 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". diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.unmappedInert-9372.test.ts b/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.unmappedInert-9372.test.ts new file mode 100644 index 0000000000..17f74844b3 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.unmappedInert-9372.test.ts @@ -0,0 +1,282 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * An operator this bridge cannot express must be INERT, never destructive + * (objectui#9372). + * + * ## The defect, and the split that carries it + * + * objectui#9363 repaired `isNull` / `isNotNull` one operator at a time. This + * card is the same destruction four more times — `between`, `endsWith`, + * `notContains`, `startsWith`, every one of them an ordinary entry in this + * inspector's menu — and it separates two questions the card's own option list + * ran together: + * + * (i) WHICH operators map to the dialect — a per-operator conformance + * question, answered below for three of the four. + * (ii) What happens when one does NOT map — not an open question. Erasing + * the author's stored filter is wrong whatever (i) answers. + * + * (ii) is what this file exists for, and it is fixed unconditionally: the + * mechanism is that `undefined` out of {@link groupToCondition} means BOTH + * "the author cleared the filter" AND "nothing survived serialization", and the + * caller treated both as clear. That conflation is what makes an unmapped + * operator destructive rather than inert. + * + * ## The second route, which needs no operator at all + * + * The conflation is reachable by blanking the VALUE of the only row — an + * incomplete row is dropped by the same `continue`, the last part disappears, + * and the function answers `undefined`. Pinned below beside the operator + * route, because it is the same defect and a repair aimed only at operators + * would leave it standing. + * + * ## What "leave it alone" is, and what it is deliberately not + * + * {@link isClearedGroup} answers the one question the caller could not ask + * before: was `undefined` the author's CLEAR gesture? Only then is `undefined` + * committed. Otherwise the caller patches nothing and the stored filter is + * untouched. + * + * ⚠️ NOT "emit something". Emitting a filter in a spelling that means something + * else is worse than dropping — that is the whole reason the unmapped arm + * exists, and it survives this repair intact. `between` is still dropped + * below, and now dropped inertly. + * + * ## Red-first + * + * Predicted before running, on the unmodified tree: the (ii) block fails at + * `isClearedGroup` not being a function, and the three (i) mappings fail with + * `undefined`, while the `equals` control in the SAME run passes — a table of + * all-`undefined` answers and a dead function look identical otherwise. + */ +import { describe, it, expect } from 'vitest'; +import { + FILTER_OPERATORS, + FieldOperatorsSchema, + FILTER_TEXT_CASES, + TEXT_OPERATOR_DOOR_CASES, +} from '@objectstack/spec/data'; +import { filterValueArity, operatorsForFieldType } from '@object-ui/components'; +import { groupToCondition, conditionToGroup, isClearedGroup } 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: 'name', operator, value }], +}); + +/** The author's stored filter, before they touch anything. */ +const STORED = { name: { $eq: 'acme' } }; + +/** + * The commit decision, spelled exactly as `DatasetFilterField` spells it. + * + * Returned rather than asserted inline so each gesture below reads as "what + * would this commit", and so the HOLD case is a value rather than the absence + * of a call. + */ +function commitFor(group: BuilderGroup): { hold: true } | { hold: false; filter: unknown } { + const next = groupToCondition(group); + if (next === undefined && !isClearedGroup(group)) return { hold: true }; + return { hold: false, filter: next }; +} + +describe('(ii) an operator this bridge cannot express is inert, not destructive (objectui#9372)', () => { + it('CONTROL: a mapped operator still serializes, so an empty answer below is about that operator', () => { + expect(groupToCondition(row('equals', 'acme'))).toEqual({ name: { $eq: 'acme' } }); + }); + + it('tells the author\'s CLEAR gesture apart from a serialization that produced nothing', () => { + // No rows at all is the clear gesture — "Clear all", or removing the last + // row. `undefined` is the right commit for it and stays that way. + expect(isClearedGroup({ id: 'g', logic: 'and', conditions: [] })).toBe(true); + expect(isClearedGroup(undefined)).toBe(true); + // A row with no field picked is not yet a row — `groupToCondition` filters + // it out before anything else, so the two must agree here. + expect(isClearedGroup(row('equals', 'acme'))).toBe(false); + expect(isClearedGroup({ id: 'g', logic: 'and', conditions: [{ id: 'c1', field: '', operator: 'equals', value: 'x' }] })).toBe(true); + }); + + it('THE GESTURE, operator route: switching the only row to an unmapped operator commits NOTHING', () => { + // The exact author gesture the card describes: a dataset that already has + // a filter, opened in the inspector, one operator change. Before this + // repair the commit was `undefined`, which the host spreads over the draft + // as `{ filter: undefined }` — the same patch shape `objectChangePatch` + // uses deliberately to CLEAR the filter. + const { group, representable } = conditionToGroup(STORED); + expect(representable).toBe(true); + const edited: BuilderGroup = { + ...group, + conditions: [{ ...group.conditions[0], operator: 'between', value: [1, 5] }], + }; + expect(groupToCondition(edited)).toBeUndefined(); + expect( + commitFor(edited), + 'this gesture used to commit `undefined`, which ERASES the stored filter', + ).toEqual({ hold: true }); + }); + + it('THE GESTURE, blank-value route: blanking the only row\'s value commits NOTHING — no operator needed', () => { + // The same defect reached without touching the operator menu at all: an + // incomplete row is dropped by the same `continue`, the last part goes, + // and the answer is `undefined`. + const { group } = conditionToGroup(STORED); + const blanked: BuilderGroup = { + ...group, + conditions: [{ ...group.conditions[0], value: '' }], + }; + expect(groupToCondition(blanked)).toBeUndefined(); + expect( + commitFor(blanked), + 'blanking the only row used to erase the stored filter, with no operator involved', + ).toEqual({ hold: true }); + }); + + it('a partly-edited group still commits the rows that DID survive', () => { + // Holding is only for "nothing survived". One good row and one blank one + // must still commit the good row, exactly as before. + const mixed: BuilderGroup = { + id: 'g', + logic: 'and', + conditions: [ + { id: 'c1', field: 'stage', operator: 'equals', value: 'won' }, + { id: 'c2', field: 'name', operator: 'between', value: [1, 5] }, + ], + }; + expect(commitFor(mixed)).toEqual({ hold: false, filter: { stage: { $eq: 'won' } } }); + }); + + it('the author CLEARING the filter still clears it — the repair does not strand a stale filter', () => { + // The other half of the equality, and the reason this is a disambiguation + // rather than a blanket "never commit undefined": with no rows left there + // is no edit to preserve, and `undefined` is the author's own gesture. + expect(commitFor({ id: 'g', logic: 'and', conditions: [] })).toEqual({ hold: false, filter: undefined }); + }); +}); + +describe('(i) the three text operators this bridge now expresses (objectui#9372)', () => { + const MAPPED: ReadonlyArray = [ + ['notContains', '$notContains'], + ['startsWith', '$startsWith'], + ['endsWith', '$endsWith'], + ]; + + it('serializes each one to the spec\'s own token', () => { + for (const [operator, token] of MAPPED) { + expect(groupToCondition(row(operator, 'ac')), `${operator} serialized to nothing`) + .toEqual({ name: { [token]: 'ac' } }); + } + }); + + it('reads each one back as the operator the author picked', () => { + for (const [operator, token] of MAPPED) { + const { group, representable } = conditionToGroup({ name: { [token]: 'ac' } }); + expect(representable, `${token} fell back to the Source tab`).toBe(true); + expect(group.conditions[0].operator).toBe(operator); + expect(groupToCondition(group)).toEqual({ name: { [token]: 'ac' } }); + } + }); + + it('PREMISE, re-measured: the dialect CAN express all four — the file\'s comment was stale', () => { + // The `unmapped (e.g. notContains/between)` comment read as "operators this + // dialect genuinely cannot express". Measured against the pinned spec, all + // four are members of its filter vocabulary, so the premise is false for + // every one of them: they were not inexpressible, they were unmapped. + for (const token of ['$notContains', '$startsWith', '$endsWith', '$between']) { + expect(FILTER_OPERATORS).toContain(token); + } + // Negative control: membership is a real reading, not a list that contains + // everything. A plausible spelling this bridge could have invented is not + // in it. + expect(FILTER_OPERATORS).not.toContain('$beginsWith'); + }); + + it('CONFORMANCE: each one carries canonical driver cases, and `$between` is not in that table', () => { + // The reading `$null` has and these were said to lack. `FILTER_TEXT_CASES` + // is the Filter Protocol's text-operator standard — the table every filter + // backend is checked against — and it carries rows for all three. + const covered = new Set(); + for (const c of FILTER_TEXT_CASES) { + for (const ops of Object.values(c.filter as Record)) { + if (ops && typeof ops === 'object') for (const k of Object.keys(ops)) covered.add(k); + } + } + for (const [, token] of MAPPED) expect(covered, `${token} has no text-conformance rows`).toContain(token); + // Negative control: this is a reading of one table, not of "every operator + // is covered". `$between` is a range operator and is NOT in it — which is + // why the conformance answer for `between` has to be sought elsewhere, and + // is not what this assertion supplies. + expect(covered).not.toContain('$between'); + }); + + it('CONFORMANCE: the spec\'s declared-type door passes all three over text and refuses them over number', () => { + // The authoring half. This bridge only ever emits these three from the + // builder's TEXT bucket (asserted below), which is the side the door + // passes; the refusals are what make the pass a reading rather than a + // table that says yes to everything. + for (const [, token] of MAPPED) { + const forText = TEXT_OPERATOR_DOOR_CASES.filter((c) => c.operator === token && c.declaredType === 'text'); + expect(forText.length, `${token} has no door case over text`).toBeGreaterThan(0); + for (const c of forText) expect(c.verdict, `${token} over text`).toBe('passes'); + const forNumber = TEXT_OPERATOR_DOOR_CASES.filter((c) => c.operator === token && c.declaredType === 'number'); + expect(forNumber.length, `${token} has no door case over number`).toBeGreaterThan(0); + for (const c of forNumber) expect(c.verdict, `${token} over number`).toBe('door-refusal'); + } + }); + + it('CONFORMANCE: the comparand door accepts the string this builder types and refuses a number', () => { + for (const [, token] of MAPPED) { + expect(FieldOperatorsSchema.safeParse({ [token]: 'ac' }).success, `${token} refused a string`).toBe(true); + // Negative control: this door judges the VALUE, so without this leg the + // assertion above would pass for a schema that accepts anything. + expect(FieldOperatorsSchema.safeParse({ [token]: 5 }).success, `${token} accepted a number`).toBe(false); + } + }); + + it('the builder only OFFERS these three on text-like fields, which is the side the door passes', () => { + // The two halves have to meet: the door refuses these operators over + // `number` / `date` / `boolean`, so mapping them is only safe while the + // dropdown never offers them there. Read from the builder's own bucket + // function rather than restated. + const offered = (type: string | undefined) => operatorsForFieldType(type, []).map((o) => o.value); + for (const [operator] of MAPPED) { + expect(offered('text'), `${operator} is not offered on text`).toContain(operator); + for (const type of ['number', 'currency', 'percent', 'rating', 'date', 'datetime', 'time', 'boolean']) { + expect(offered(type), `${operator} is offered on ${type}, where the spec's door refuses it`) + .not.toContain(operator); + } + } + }); +}); + +describe('`between` stays unmapped — and is now unmapped INERT (objectui#9372)', () => { + it('is still dropped rather than emitted', () => { + expect(groupToCondition(row('between', [1, 5]))).toBeUndefined(); + }); + + it('the reason it stays out, measured: nothing downstream catches a half-filled pair', () => { + // The builder pads a pair with `""` when only one bound is typed + // (`reshapeFilterValue`), and the row is two entries long, so this bridge's + // completeness check — which only rejects `null` / `''` / `[]` — would let + // it through. The spec's comparand door does not catch it either: a bound + // of `''` parses. So emitting `between` today would emit a filter that + // means something the author did not ask for, which is exactly what the + // unmapped arm exists to prevent. A both-bounds-present rule is the + // precondition, and it is a separate decision. + expect(filterValueArity('between')).toBe('pair'); + expect(FieldOperatorsSchema.safeParse({ $between: [1, 5] }).success).toBe(true); + expect( + FieldOperatorsSchema.safeParse({ $between: [1, ''] }).success, + 'if the spec refused a half-filled pair, this bridge could lean on it instead of a local rule', + ).toBe(true); + }); + + it('but picking it no longer erases the stored filter', () => { + // The whole point of the (i)/(ii) split: an operator can stay unmapped + // without staying destructive. + expect(commitFor(row('between', [1, 5]))).toEqual({ hold: true }); + }); +});