Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions .changeset/list-filter-existence-operators-withheld.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
---
'@object-ui/components': patch
'@object-ui/fields': patch
'@object-ui/plugin-list': patch
'@object-ui/app-shell': patch
---

The list filter builder no longer offers `Is set` / `Is not set`, which its query dialects cannot express.

**User-visible before/after.** The operator dropdown in the list toolbar's
filter popover — and in the Studio view/tab/page filter inspectors, the dataset
inspector and the generic `filter` config field — loses two rows: **"Is set"**
and **"Is not set"**. The sharing-rule criteria builder (`FilterConditionField`)
keeps both, unchanged. `Is null` / `Is not null` and `Is empty` / `Is not empty`
are untouched everywhere and remain the way to filter on a missing value from
the list.

Nothing that worked stops working. Every save path behind those two rows was
already broken, in three different ways depending on the surface:

- **Live grid** — `ListView.mapOperator` had no row for either id, so its
`default:` arm returned the id verbatim and the query went out as
`['name', 'exists', 'x']`. `exists` is not a member of the spec's
`VALID_AST_OPERATORS`, so `isFilterAST()` rejects the shape: an unfiltered
read or a 400, never the filter the user asked for.
- **Save as view** — `foldFilterGroupToSpecRules` normalizes through the spec's
`normalizeFilterOperator`, which does not know the pair, and
`ViewFilterRuleSchema`'s enum then refuses the rule.
- **Dataset inspector** — `groupToCondition` has no row either and drops the
condition silently, so the filter simply never applied.

**Why withheld rather than mapped.** Measured on `@objectstack/spec`
17.0.0-rc.6: neither `VIEW_FILTER_OPERATORS` nor `VALID_AST_OPERATORS` contains
an existence operator, under any spelling — both sets have zero members matching
`/exist/`. Only the MongoDB-style `FieldOperatorsSchema` criteria carries
`$exists`, and that is precisely the dialect `FilterConditionField` writes, so
the pair moves behind the existing `OPT_IN_OPERATORS` gate and that widget opts
in. Collapsing them onto `isNotNull` / `isNull` was rejected: the builder
already draws those as their own rows, the round trip is lossy (a saved
`exists` reads back as `isNotNull`), and the spec's own note records `$exists` =
has-value as still unsettled across drivers — `driver-memory`'s live mingo path
and `driver-mongodb` read key-presence.

**The class is now closed by an assertion, not by discipline.** objectui's three
existing operator-parity guards all sweep spec vocabulary → objectui; none asked
whether an id the dropdown draws is an id the consumer can persist, which is the
direction that broke. `plugin-list`'s new
`list-offered-operator-expressible-parity.test.ts` forces the set the list
toolbar offers to **equal** the set its two dialects can express, in both
directions — so an unexpressible operator cannot be offered, and an operator
that becomes expressible upstream cannot stay needlessly withheld.
19 changes: 14 additions & 5 deletions packages/app-shell/src/views/viewFilterFold.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -56,11 +56,20 @@ interface FilterGroupLike {
*
* The builder renders no value input for these (`needsValueInput` in
* `@object-ui/components`'s `filter-builder.tsx`), so "no value" is the row's
* finished state, not an unfinished one. `exists` / `notExists` have no
* canonical spec spelling and pass through verbatim for the server's enum to
* reject loudly (see the operator note above) — they are listed so that
* rejection stays the reason they fail, rather than being silently dropped here
* as incomplete.
* finished state, not an unfinished one.
*
* `exists` / `notExists` are still listed, and no longer for the reason they
* used to be. They have no canonical spec spelling — `VIEW_FILTER_OPERATORS`
* has no existence operator — so a rule carrying one is refused by
* `ViewFilterRuleSchema`. That used to be their whole story here: they reached
* this fold from the shared dropdown, and listing them kept the server's loud
* rejection as the reason they failed rather than a silent drop as incomplete.
* Since objectui#4736 the dropdown no longer OFFERS them to any consumer that
* folds through here (they moved behind `OPT_IN_OPERATORS`, and only
* `FilterConditionField` — which writes MongoDB-style criteria, never a view
* rule — opts in), so the live path stopped producing them. They stay because
* `needsValueInput` still draws them value-less for that widget, and because
* stored metadata may carry one; the parity test below is what forces that.
*
* `viewFilterFold.emptyValue.test.ts` pins this set against the builder's own
* list so the two cannot drift apart unnoticed.
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,21 +7,34 @@
*/

/**
* The opt-in half of the FilterBuilder's operator vocabulary (objectui#4023).
* The opt-in half of the FilterBuilder's operator vocabulary (objectui#4023,
* objectui#4736).
*
* `containsCaseInsensitive` authors the spec's `$icontains`, which the MongoDB-
* style `FieldOperatorsSchema` dialect carries and the OTHER two dialects this
* one dropdown feeds do not: `VIEW_FILTER_OPERATORS` (what a saved view stores)
* and `VALID_AST_OPERATORS` (what the live grid sends) have no case-insensitive
* contains at all. Offering it unconditionally would hand users a filter two of
* three consumers cannot execute — the same hazard that held objectui#4023
* blocked while no driver implemented the operator, moved from the drivers to
* this repo's own bridges.
* This one dropdown feeds three at-rest dialects and they do not accept the
* same operators. Two families are carried only by the MongoDB-style
* `FieldOperatorsSchema` criteria:
*
* - `containsCaseInsensitive` authors `$icontains`. `VIEW_FILTER_OPERATORS`
* (what a saved view stores) and `VALID_AST_OPERATORS` (what the live grid
* sends) have no case-insensitive contains at all.
* - `exists` / `notExists` author `$exists`. Neither of those two vocabularies
* has an existence operator either, under any spelling.
*
* Offering such an operator unconditionally hands users a filter two of three
* consumers cannot execute — the same hazard that held objectui#4023 blocked
* while no driver implemented the operator, moved from the drivers to this
* repo's own bridges. objectui#4736 is that hazard realised: the existence pair
* shipped to every consumer in objectui#2942 and the list toolbar sent it to
* the wire verbatim.
*
* So the default answer is "not offered", and these tests pin BOTH directions:
* withheld unless asked for, and actually reachable once asked for. A gate that
* only checked the second would go green on a component that offers everything
* to everybody.
*
* What these tests do NOT decide is which ids belong in `OPT_IN_OPERATORS` —
* this package cannot see the dialects. That equality is forced per consumer:
* `plugin-list`'s `list-offered-operator-expressible-parity.test.ts`.
*/
import { describe, it, expect } from 'vitest';
import { FILTER_BUILDER_OPERATORS, operatorsForFieldType } from '../custom/filter-builder';
Expand DownExpand Up@@ -66,6 +79,44 @@ describe('FilterBuilder opt-in operators', () => {
// opt-in operator is drawable, so leaving it out would understate the
// vocabulary and let a future spec operator look unreachable when it is not.
expect(FILTER_BUILDER_OPERATORS).toContain('containsCaseInsensitive');
expect(FILTER_BUILDER_OPERATORS).toContain('exists');
expect(FILTER_BUILDER_OPERATORS).toContain('notExists');
expect(new Set(FILTER_BUILDER_OPERATORS).size).toBe(FILTER_BUILDER_OPERATORS.length);
});

// objectui#4736. The pair is drawable but no longer unconditional, so the
// list toolbar and the Studio view/tab inspectors — neither of which passes
// `extraOperators` — stop drawing an operator their dialects cannot store.
it('withholds exists / notExists from a consumer that did not ask', () => {
for (const type of [undefined, 'text', 'number', 'date', 'select', 'lookup']) {
const ids = idsFor(type);
expect(ids, String(type)).not.toContain('exists');
expect(ids, String(type)).not.toContain('notExists');
}
});

it('offers exists / notExists once the consumer opts in', () => {
const extra = ['exists', 'notExists'];
// Reachable on every bucket that had them before the withdrawal — the
// criteria builder filters lookups and dates by presence too, so scoping
// this to text would silently narrow what objectui#2942 made authorable.
for (const type of [undefined, 'text', 'number', 'date', 'select', 'lookup']) {
const ids = idsFor(type, extra);
expect(ids, String(type)).toContain('exists');
expect(ids, String(type)).toContain('notExists');
// Beside the null predicates, never instead of them: `$exists` and
// `$null` are distinct spec operators and both keep their own rows.
expect(ids, String(type)).toContain('isNull');
expect(ids, String(type)).toContain('isNotNull');
}
});

it('grants each opt-in id independently', () => {
// A consumer that can store `$exists` but not `$icontains` (or the reverse)
// must not get the other for free — that would make `extraOperators` a
// single "unlock everything" switch and put the whole opt-in gate back.
expect(idsFor('text', ['exists'])).not.toContain('containsCaseInsensitive');
expect(idsFor('text', ['containsCaseInsensitive'])).not.toContain('exists');
expect(idsFor('text', ['exists'])).not.toContain('notExists');
});
});
85 changes: 63 additions & 22 deletions packages/components/src/custom/filter-builder.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -118,40 +118,81 @@ const defaultOperators = [
* `extraOperators`. Every other id in `defaultOperators` is offered to every
* consumer.
*
* ## Why an operator would ever be opt-in (objectui#4023)
* ## Why an operator would ever be opt-in (objectui#4023, objectui#4736)
*
* This one dropdown feeds three different at-rest dialects, and they do not
* accept the same operators:
*
* - **MongoDB-style `FieldOperatorsSchema` criteria** — what
* `FilterConditionField` writes into a sharing rule's `criteria_json`,
* evaluated by the server's engine. It has `$icontains`, and every driver
* and evaluation face the platform ships executes it
* (objectstack#5702 + objectstack#6520).
* - **`ViewFilterRule[]`** — what `viewFilterFold` persists for a saved view.
* Its vocabulary is the spec's `VIEW_FILTER_OPERATORS`, which has **no**
* case-insensitive contains, so the rule is refused by the schema's enum.
* evaluated by the server's engine. Its vocabulary is the spec's
* `FILTER_OPERATORS`, the widest of the three.
* - **`ViewFilterRule[]`** — what `viewFilterFold` persists for a saved view,
* and what the Studio inspector's `FilterBuilderField` writes. Its
* vocabulary is the spec's `VIEW_FILTER_OPERATORS`; a rule outside that
* enum is refused by `ViewFilterRuleSchema`.
* - **The array/triplet filter AST** — what `ListView` sends for the live
* grid, via `mapOperator`. Its vocabulary is the spec's
* `VALID_AST_OPERATORS`, which also has no case-insensitive contains: an
* unmapped operator is emitted verbatim and the query comes back broken
* (see `packages/data-objectstack/src/filter-operator-ast-parity.test.ts`
* for what "broken" costs — an unfiltered read or a 400, either way not the
* `VALID_AST_OPERATORS`; an unmapped operator is emitted verbatim and the
* query comes back broken (see
* `packages/data-objectstack/src/filter-operator-ast-parity.test.ts` for
* what "broken" costs — an unfiltered read or a 400, either way not the
* filter the user asked for).
*
* So offering `containsCaseInsensitive` to every FilterBuilder would put a
* filter in users' hands that two of the three consumers cannot execute —
* which is the exact hazard objectui#4023 was held blocked over, relocated from
* the drivers to this repo's own bridges. Mapping it onto plain `contains`
* there is NOT the alternative: that is a different question silently answered
* (the same reason `view-operator-builder-parity.test.ts` refuses to map
* `is_null` onto `isEmpty`).
* So an operator only one dialect can carry is offered only by the consumer
* that speaks that dialect. Mapping it onto a near-equivalent in the others is
* NOT the alternative: that is a different question silently answered — the
* same reason `view-operator-builder-parity.test.ts` refuses to map `is_null`
* onto `isEmpty`.
*
* The lasting fix is upstream — the spec's view and AST vocabularies gaining a
* case-insensitive contains — at which point the entry below is deleted and the
* operator becomes ordinary. Until then the consumer that CAN store it says so.
* Which side of the line an id falls on is not left to this comment.
* `plugin-list`'s `list-offered-operator-expressible-parity.test.ts` forces the
* set the list toolbar offers to EQUAL the set its two dialects can express, in
* both directions: an unexpressible id that stays offered fails, and an id that
* becomes expressible while still withheld fails too.
*
* ### `containsCaseInsensitive` — the spec's `$icontains` (objectui#4023)
*
* Carried by the Mongo criteria dialect, where every driver and evaluation face
* the platform ships executes it (objectstack#5702 + objectstack#6520). The
* other two vocabularies have no case-insensitive contains at all, so offering
* it everywhere is the exact hazard objectui#4023 was held blocked over,
* relocated from the drivers to this repo's own bridges.
*
* ### `exists` / `notExists` — the spec's `$exists` (objectui#4736)
*
* Added by objectui#2942 to make `$exists` reachable from
* `FilterConditionField`, but offered to every consumer, including the two that
* cannot store them. Measured on `@objectstack/spec` 17.0.0-rc.6: neither
* `VIEW_FILTER_OPERATORS` nor `VALID_AST_OPERATORS` contains an existence
* operator — not under this spelling, not under any other; both sets have ZERO
* members matching `/exist/`. So there was nothing to map onto and the id went
* out verbatim.
*
* Collapsing them onto `isNotNull` / `isNull` would be a semantic claim, not a
* bridge, and it is refused on three counts:
*
* 1. The builder already offers `isNull` / `isNotNull` as their own rows, so
* the collapse would draw two labels for one wire predicate.
* 2. The round trip is lossy: a saved `exists` reads back through
* `specToBuilderOperator('is_not_null')` as `isNotNull`, silently
* rewriting the author's choice on reopen.
* 3. The equivalence is not this repo's to declare. `@objectstack/spec`'s own
* `data/index.d.ts` records `$exists` = has-value (`!= null`) as settled
* and shipped on `formula` and `driver-memory`'s reference matcher, while
* `driver-memory`'s live mingo path and `driver-mongodb` still read
* KEY-PRESENCE, both frozen by objectstack#5499 — which is why upstream
* cannot enrol a `$exists` conformance row yet either.
*
* The lasting fix for both families is upstream — the view and AST vocabularies
* gaining the operator — at which point the entry below is deleted, the parity
* test flips it back on by itself, and the operator becomes ordinary.
*/
const OPT_IN_OPERATORS = new Set<string>(["containsCaseInsensitive"])
const OPT_IN_OPERATORS = new Set<string>([
"containsCaseInsensitive",
"exists",
"notExists",
])

/**
* The FilterBuilder's own operator vocabulary — every id its dropdown can
Expand Down
24 changes: 19 additions & 5 deletions packages/fields/src/widgets/FilterConditionField.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -48,22 +48,36 @@ interface BuilderGroup {
const EMPTY_GROUP: BuilderGroup = { id: 'root', logic: 'and', conditions: [] };

/**
* Opt-in FilterBuilder operators this widget offers (objectui#4023).
* Opt-in FilterBuilder operators this widget offers (objectui#4023,
* objectui#4736).
*
* The shared dropdown withholds these because two of its three consumers
* persist into dialects that cannot carry them (see `OPT_IN_OPERATORS` in
* `@object-ui/components`'s `filter-builder.tsx`). THIS widget can: its value
* is a MongoDB-style `FieldOperatorsSchema` criteria that the server's engine
* evaluates directly — never lowered through the array/triplet AST — and
* `$icontains` is executable on every driver and evaluation face the platform
* ships (objectstack#5702 + objectstack#6520).
* evaluates directly — never lowered through the array/triplet AST and never
* folded into a `ViewFilterRule` — so the spec's `FILTER_OPERATORS` is the only
* vocabulary it has to satisfy.
*
* - `containsCaseInsensitive` authors `$icontains`, executable on every
* driver and evaluation face the platform ships (objectstack#5702 +
* objectstack#6520).
* - `exists` / `notExists` author `$exists`, which `condToMongo` has emitted
* and `kvToCondition` has read back since objectui#2942. Naming them here
* is what KEEPS them reachable now that the shared dropdown no longer
* offers them to the list and view surfaces, whose dialects have no
* existence operator at all (objectui#4736).
*
* Module scope, not an inline literal: a fresh array each render would reset
* `FilterBuilder`'s memo inputs on every keystroke.
*
* @internal exported for tests
*/
export const FILTER_CONDITION_EXTRA_OPERATORS: readonly string[] = ['containsCaseInsensitive'];
export const FILTER_CONDITION_EXTRA_OPERATORS: readonly string[] = [
'containsCaseInsensitive',
'exists',
'notExists',
];

/** Field types that are not meaningfully filterable in a simple builder. */
const NON_FILTERABLE = new Set([
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -166,6 +166,37 @@ describe('every spec field operator is reachable from the builder (#2942)', () =
expect(offered).toContain('containsCaseInsensitive');
expect(offered).toContain('contains');
});

/**
* The same half for `$exists` (objectui#4736). The pair became opt-in when the
* list and view surfaces — whose dialects have no existence operator — were
* found offering it, so THIS widget naming it is now the only thing keeping
* `$exists` authorable at all.
*
* The sweep above cannot notice if that opt-in is dropped: it feeds
* `FILTER_BUILDER_OPERATORS` (every DRAWABLE id) straight to `condToMongo`,
* which answers to any string it is handed, so `$exists` would still count as
* emitted while no admin could click a row that emits it — exactly the state
* objectui#4023 found for `$icontains`.
*/
it('the $exists operator is one this widget still offers, post-withdrawal', () => {
expect(SPEC_OPERATORS.has('$exists'), '$exists is no longer a spec operator').toBe(true);
expect(FILTER_BUILDER_OPERATORS).toContain('exists');
expect(FILTER_BUILDER_OPERATORS).toContain('notExists');
expect(FILTER_CONDITION_EXTRA_OPERATORS).toContain('exists');
expect(FILTER_CONDITION_EXTRA_OPERATORS).toContain('notExists');

for (const type of ['text', 'number', 'date', 'select', 'lookup']) {
const offered = operatorsForFieldType(type, FILTER_CONDITION_EXTRA_OPERATORS)
.map((o) => o.value);
expect(offered, `${type} lost the existence pair`).toContain('exists');
expect(offered, `${type} lost the existence pair`).toContain('notExists');
// Distinct rows from the null predicates, which author `$null`. The two
// are different spec operators and `condToMongo` keeps them apart.
expect(offered, type).toContain('isNull');
expect(offered, type).toContain('isNotNull');
}
});
});

describe('kvToCondition round-trips what condToMongo writes', () => {
Expand Down
Loading
Loading