fix(core): lower the TRUE-identity combinators to "no constraint" instead of handing back the caller's object

.changeset/8770-true-identity-groups-no-constraint.md

@@ -0,0 +1,53 @@
+---
+'@object-ui/core': minor
+'@object-ui/data-objectstack': minor
+---
+
+Lower the TRUE-identity filter combinators to "no constraint" instead of handing the
+caller's own object back (objectui#8770).
+
+`convertFiltersToAST({ $and: [] })`, `{ $or: [{}] }` and `{ $and: [{}] }` returned the
+INPUT OBJECT unchanged. `lowerLogicalGroup` correctly answers `undefined` for a group
+that reduces to the TRUE identity — objectstack#5322 rules all three "every row", and a
+childless `['and']` would be `isFilterAST` FALSE — but when such a group was the only
+thing in the filter, that `undefined` fell through to the general tail
+(`if (conditions.length === 0) return filter`) and the group reappeared one level up, in
+the `$` dialect, in the slot the AST occupies. The same function already lowered the
+fourth identity, `{ $or: [] }`, correctly, so this was an internal inconsistency rather
+than an open question; the consumer half was settled by objectui#8513.
+
+**This widens what those three filters return, and that is the point.** Measured against
+`@objectstack/spec` 17.4.0 and `@objectstack/client` 17.4.0 before the change: the
+returned object is not sent as a filter and refused — `client.data.find()` tests the
+value with `isFilterAST` and its else branch spreads a plain object's entries as query
+parameters, so `{ $and: [] }` left as `?$and=` with **no `filter` parameter at all**, and
+the server answered `400 UNSUPPORTED_QUERY_PARAM` for the unknown `$`-prefixed
+parameter. A filter whose ruled answer is EVERY ROW was a **failed list**, not a narrowed
+one — so nothing could have been relying on it to scope data. On the sibling
+`$expand` / `$search` route the same object travelled as `filter={"$and":[]}`, which the
+server accepts as a `FilterCondition` and already answers with every row; the two routes
+disagreed about one filter and now agree.
+
+**`@object-ui/core`.** `convertFiltersToAST`'s declared return type gains `undefined`,
+which is what `toFilterNode`, `mergeFilterNodes` and `data-objectstack`'s
+`translateFilterToAST` already mean by "no filter, skip the slot". Every call site
+already acted on it. The fold is scoped to a filter whose EVERY key is such a group:
+the same tail also serves `{}`, an all-null filter and an empty operator map, and those
+keep the object they always returned — a null-valued key is this converter's own
+tolerance rather than a ruled identity, and the object it hands back reaches the server
+as a REAL `a IS NULL` predicate on the `$expand` route, so folding it in would return
+more rows on a path the ruling said nothing about.
+
+`{ $or: [] }` is untouched: FALSE is not "no constraint", the AST has no contradiction
+literal, and its `['$or', '=', []]` leaf answers FALSE at both consumers.
+
+**`@object-ui/data-objectstack`.** `convertQueryParams` skips the `filters` slot when the
+lowering answers `undefined`, the same answer the raw-GET route's
+`if (translated !== undefined)` already gave, so the two `find()` routes cannot disagree
+about one filter.
+
+**Migration.** A TypeScript caller that stored `convertFiltersToAST(...)` in a
+`FilterNode | Record<string, any>` slot must widen it with `| undefined` and skip the
+filter when it is absent — the same handling `toFilterNode` has always needed. At
+runtime, a filter that is nothing but TRUE-identity combinators now returns every row
+(what objectstack#5322 rules) instead of failing the request.

packages/core/src/adapters/__tests__/ValueDataSource.filterLogicConformance-8513.test.ts

@@ -189,27 +189,38 @@ describe('objectui#8513 — `$not` is out of scope, and stays refused', () => {
  * `engine.find`, and it never passes through this adapter.
  *
  * The producer that does reach here is this repo's own single filter sink.
- * `convertFiltersToAST` returns the ORIGINAL OBJECT when a filter lowers to no
- * conditions, and the identity groups are exactly that case — so `toFilterNode`
- * hands `{ $and: [] }` and `{ $or: [{}] }` back UNLOWERED, and every consumer
- * on that chain (`ObjectGrid`'s `schemaFilter`, `plugin-list`'s
- * `buildEffectiveFilter`, `plugin-view`'s `ObjectView`) drops them straight
+ * `convertFiltersToAST` returned the ORIGINAL OBJECT when a filter lowered to
+ * no conditions, and the identity groups were exactly that case — so
+ * `toFilterNode` handed `{ $and: [] }` and `{ $or: [{}] }` back UNLOWERED, and
+ * every consumer on that chain (`ObjectGrid`'s `schemaFilter`, `plugin-list`'s
+ * `buildEffectiveFilter`, `plugin-view`'s `ObjectView`) dropped them straight
  * onto `$filter`. Before this change the matcher answered ZERO ROWS for both,
  * on filters whose ruled answer is EVERY row — the worst direction, on the
  * reachable path.
+ *
+ * ⚠️ The PRODUCER half moved afterwards: objectui#8770 made `convertFiltersToAST`
+ * answer `undefined` — "no constraint" — for a filter that is nothing but
+ * TRUE-identity combinators, so this matcher is no longer handed those objects
+ * BY THAT CHAIN. The matcher's own answer is asserted below all the same, and
+ * deliberately from the object literals rather than through `toFilterNode`: it
+ * is a `DataSource` with a public `find`, the object dialect is what
+ * `QuerySchema.where` declares, and #8513's ruling is about what this adapter
+ * answers — not about which producer happens to call it this month.
  */
-describe('objectui#8513 — the lowering chain hands this matcher an object', () => {
-  it('`toFilterNode` leaves the TRUE identities in the object dialect', () => {
-    // Not asserting this is right — asserting it is what happens, because it
-    // is why the matcher has to answer correctly for these shapes.
-    expect(toFilterNode({ $and: [] })).toEqual({ $and: [] });
-    expect(toFilterNode({ $or: [{}] })).toEqual({ $or: [{}] });
+describe('objectui#8513 — this matcher answers the object dialect directly', () => {
+  it('the lowering chain no longer hands the identities down in that dialect', () => {
+    // objectui#8770: the producer stopped re-emitting the caller's object for a
+    // filter it read as "no constraint". Pinned here because the paragraph
+    // above rests on it, and a silent revert would put an object back on
+    // `$filter` where the wire refuses it.
+    expect(toFilterNode({ $and: [] })).toBeUndefined();
+    expect(toFilterNode({ $or: [{}] })).toBeUndefined();
   });
 
-  it('and the matcher now answers them the way the ruling says', async () => {
+  it('and the matcher answers the objects themselves the way the ruling says', async () => {
     const allIds = FILTER_LOGIC_ROWS.map((r) => r.id);
-    expect(await selectedIds(toFilterNode({ $and: [] }))).toEqual(allIds);
-    expect(await selectedIds(toFilterNode({ $or: [{}] }))).toEqual(allIds);
+    expect(await selectedIds({ $and: [] })).toEqual(allIds);
+    expect(await selectedIds({ $or: [{}] })).toEqual(allIds);
   });
 
   it('a group that DOES lower still goes down the AST arm, unchanged', async () => {

packages/core/src/utils/__tests__/filter-true-identity-8770.test.ts

@@ -0,0 +1,237 @@
+/**
+ * ObjectUI
+ * Copyright (c) 2024-present ObjectStack Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+/**
+ * The TRUE-identity combinators reach the wire as "no constraint" —
+ * objectui#8770.
+ *
+ * ## What was wrong
+ *
+ * `lowerLogicalGroup` answers `undefined` for a group that reduces to the TRUE
+ * identity, and that is correct and deliberate (see its docblock: a childless
+ * `['and']` is `isFilterAST` FALSE, which reads as no filter at all). But when
+ * such a group was the ONLY thing in the filter, that `undefined` fell through
+ * to `convertFiltersToAST`'s general tail — `if (conditions.length === 0)
+ * return filter` — and the CALLER'S ORIGINAL OBJECT came back. So the group did
+ * not disappear at that level after all; it reappeared one level up, in the `$`
+ * dialect, in a slot the AST is expected to occupy.
+ *
+ * Three of the four identities went out that way and one did not, which is what
+ * made this an internal inconsistency rather than an open question: the same
+ * function already lowered `{ $or: [] }` correctly.
+ *
+ * ## Why the assertions are shaped the way they are
+ *
+ * Two halves, because either alone passes on something worse than the bug:
+ *
+ *   - a SHAPE assertion (`toBeUndefined`) alone passes on a converter that has
+ *     stopped emitting anything at all;
+ *   - a ROW-SET assertion alone passes on a filter that never ran — `$filter:
+ *     undefined` returns EVERY row, and so does a correct TRUE identity, so the
+ *     row set cannot tell "honoured" from "dropped".
+ *
+ * So the shape half is asserted against the spec's own door (`isFilterAST`)
+ * rather than against a literal, the row-set half names exact ids, and the
+ * FALSE identity `{ $or: [] }` is carried through every section as a control:
+ * it must keep answering NO rows. A change that flattened the identities into
+ * one arm would take the control with it.
+ *
+ * `FILTER_LOGIC_ROWS` (`@objectstack/spec/data`) is the published cross-backend
+ * fixture the identity ruling (objectstack#5322, merged objectstack#5365) is
+ * held to, so the row sets here are the platform's own and not a local
+ * restatement of them.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { FILTER_LOGIC_ROWS, isFilterAST, parseFilterAST } from '@objectstack/spec/data';
+import { convertFiltersToAST, toFilterNode, mergeFilterNodes } from '../filter-converter';
+import { ValueDataSource } from '../../adapters/ValueDataSource';
+
+const ALL_IDS = FILTER_LOGIC_ROWS.map((r) => String(r.id));
+
+async function selectedIds(filter: unknown): Promise<string[]> {
+  const ds = new ValueDataSource({ items: FILTER_LOGIC_ROWS as any[] });
+  const result = await ds.find('rows', { $filter: filter as any });
+  return result.data.map((r) => String(r.id));
+}
+
+/** The three groups objectstack#5322 rules TRUE — "every row". */
+const TRUE_IDENTITIES: ReadonlyArray<[string, Record<string, any>]> = [
+  ['{ $and: [] }', { $and: [] }],
+  ['{ $or: [{}] }', { $or: [{}] }],
+  ['{ $and: [{}] }', { $and: [{}] }],
+];
+
+// ---------------------------------------------------------------------------
+// 0. The harness has to be able to fail
+// ---------------------------------------------------------------------------
+
+describe('objectui#8770 — harness', () => {
+  it('every row and no rows are distinguishable answers here', async () => {
+    expect(ALL_IDS).toEqual(['1', '2', '3', '4']);
+    // A filter that really constrains lands strictly between the two, so
+    // "every row" below is a measurement and not the fixture's only answer.
+    expect(await selectedIds(['a', '=', 'x'])).toEqual(['1', '2']);
+    expect(await selectedIds(undefined)).toEqual(ALL_IDS);
+  });
+});
+
+// ---------------------------------------------------------------------------
+// 1. The card's own table, re-measured
+// ---------------------------------------------------------------------------
+
+describe('objectui#8770 — the four identity groups', () => {
+  it.each(TRUE_IDENTITIES)('%s lowers to "no constraint"', (_label, filter) => {
+    // Not `['and']` and not the caller's object: the absence of the slot is how
+    // this dialect says TRUE, which is exactly what `lowerLogicalGroup` decided
+    // one level down and what the tail used to throw away.
+    expect(convertFiltersToAST(filter)).toBeUndefined();
+  });
+
+  it('CONTROL — `{ $or: [] }` is FALSE and keeps the leaf it always had', () => {
+    // The one identity that was already right. It must not be swept into the
+    // fold above: FALSE is not "no constraint", and the AST has no
+    // contradiction literal, so the pre-existing leaf is still the emission.
+    const node = convertFiltersToAST({ $or: [] });
+    expect(node).toEqual(['$or', '=', []]);
+    expect(parseFilterAST(node)).toEqual({ $or: [] });
+  });
+});
+
+// ---------------------------------------------------------------------------
+// 2. The invariant that actually broke — what reaches the wire
+// ---------------------------------------------------------------------------
+
+describe('objectui#8770 — nothing unreadable is handed back', () => {
+  it.each([...TRUE_IDENTITIES, ['{ $or: [] }', { $or: [] }] as const])(
+    '%s is either absent or a node the spec can read',
+    (_label, filter) => {
+      // The property, stated once for all four: a value this function returns
+      // is either `undefined` — no filter — or something `isFilterAST`
+      // accepts. Before the fix three of the four returned a plain OBJECT
+      // instead, which is neither: `client.data.find()` spreads such an object's
+      // entries as query parameters (`?$and=`), so no `filter` parameter was
+      // sent and the server refused the unknown `$`-prefixed one with `400
+      // UNSUPPORTED_QUERY_PARAM`.
+      const out = convertFiltersToAST(filter as Record<string, any>);
+      expect(out === undefined || isFilterAST(out)).toBe(true);
+    },
+  );
+});
+
+// ---------------------------------------------------------------------------
+// 3. The ruled row sets, through a real matcher
+// ---------------------------------------------------------------------------
+
+/**
+ * ⚠️ This section does NOT witness this card's fix, and is kept anyway — say so
+ * rather than let the next reader assume it does. Both legs of the ablation
+ * (fold removed / fold restored) leave every case here GREEN, because
+ * objectui#8513 already taught `ValueDataSource`'s matcher to answer the object
+ * dialect correctly: the unlowered `{ $and: [] }` and the absent filter select
+ * the same four rows. What it pins is the direction the triage ruling names —
+ * the producer aligning to the consumer — so a fix that moved the two apart
+ * again, or that started answering these identities FALSE, reddens here.
+ */
+describe('objectui#8770 — the ruling reaches the consumer', () => {
+  it.each(TRUE_IDENTITIES)('%s selects EVERY row after lowering', async (_label, filter) => {
+    expect(await selectedIds(toFilterNode(filter))).toEqual(ALL_IDS);
+  });
+
+  it('CONTROL — `{ $or: [] }` still selects NO row after lowering', async () => {
+    expect(await selectedIds(toFilterNode({ $or: [] }))).toEqual([]);
+  });
+});
+
+// ---------------------------------------------------------------------------
+// 4. Composition — the shape the two real sinks build
+// ---------------------------------------------------------------------------
+
+describe('objectui#8770 — composing with a filter that does constrain', () => {
+  it.each(TRUE_IDENTITIES)('%s contributes nothing to the merged `and`', async (_label, filter) => {
+    // `plugin-list`'s `buildEffectiveFilter` and `plugin-view`'s `ObjectView`
+    // both merge several sources here. Before the fix the object landed in AST
+    // CHILD position — `['and', { $and: [] }, ['a','=','x']]` — which
+    // `isFilterAST` refuses, so the whole list 400'd on a filter that was
+    // supposed to narrow nothing.
+    const merged = mergeFilterNodes(filter, ['a', '=', 'x']);
+    expect(merged).toEqual(['a', '=', 'x']);
+    expect(isFilterAST(merged)).toBe(true);
+    expect(await selectedIds(merged)).toEqual(['1', '2']);
+  });
+
+  it('a TRUE identity merged with a FALSE one is still FALSE', async () => {
+    const merged = mergeFilterNodes({ $and: [] }, { $or: [] });
+    expect(merged).toEqual(['$or', '=', []]);
+    expect(await selectedIds(merged)).toEqual([]);
+  });
+
+  it('and the two identities inside ONE filter reduce the same way', async () => {
+    // TRUE ∧ FALSE = FALSE, and the `$and` key must not drag the `$or` leaf out
+    // of the result on its way to being dropped.
+    const node = convertFiltersToAST({ $and: [], $or: [] });
+    expect(node).toEqual(['$or', '=', []]);
+    expect(await selectedIds(node)).toEqual([]);
+  });
+});
+
+// ---------------------------------------------------------------------------
+// 5. Nesting — an identity group inside an identity group
+// ---------------------------------------------------------------------------
+
+describe('objectui#8770 — nested identities fold all the way up', () => {
+  it('a TRUE-identity child drops out of an `$and` and absorbs an `$or`', () => {
+    expect(convertFiltersToAST({ $and: [{ $and: [] }] })).toBeUndefined();
+    expect(convertFiltersToAST({ $or: [{ $and: [] }] })).toBeUndefined();
+  });
+
+  it('a nested identity beside a real conjunct leaves the conjunct alone', () => {
+    expect(convertFiltersToAST({ $and: [{ $and: [] }, { a: 'x' }] })).toEqual([
+      'and',
+      ['a', '=', 'x'],
+    ]);
+  });
+});
+
+// ---------------------------------------------------------------------------
+// 6. The boundary this fix deliberately did NOT move
+// ---------------------------------------------------------------------------
+
+describe('objectui#8770 — the non-combinator tail is untouched', () => {
+  // The same tail serves inputs that are not combinators at all. Those are a
+  // different question and were left exactly as they were: a null-valued key is
+  // this converter's own tolerance rather than a ruled identity, and the object
+  // handed back travels the `$expand` / `$search` route as `filter={"a":null}`,
+  // which the server reads as a REAL `a IS NULL` predicate. Folding them into
+  // "no constraint" would return MORE rows on a path objectstack#5322 said
+  // nothing about — the one direction this file exists to avoid.
+
+  it('an all-null filter still returns the original object', () => {
+    expect(convertFiltersToAST({ a: null, b: undefined })).toEqual({ a: null, b: undefined });
+  });
+
+  it('an empty operator map still returns the original object', () => {
+    expect(convertFiltersToAST({ a: {} })).toEqual({ a: {} });
+  });
+
+  it('an identity group beside a NON-combinator key returns the original object', () => {
+    // The fold is keyed on "every key was such a group", not on "any key was".
+    // `a: null` is not one, so this filter is not the ruled family and keeps the
+    // answer it has always had.
+    expect(convertFiltersToAST({ $and: [], a: null })).toEqual({ $and: [], a: null });
+  });
+
+  it('an identity group beside a key that DOES lower is unchanged', async () => {
+    // Already correct before this card — the group was dropped and the sibling
+    // carried the filter. Pinned because the fold must not start swallowing the
+    // sibling.
+    const node = convertFiltersToAST({ $and: [], a: 'x' });
+    expect(node).toEqual(['a', '=', 'x']);
+    expect(await selectedIds(node)).toEqual(['1', '2']);
+  });
+});