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
50 changes: 50 additions & 0 deletions .changeset/filter-builder-between-pair-and-operator-labels.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
---
'@object-ui/components': patch
'@object-ui/plugin-list': patch
'@object-ui/app-shell': patch
'@object-ui/i18n': patch
---

Console list filters: a `between` range is submitted only when both bounds are filled, and six operator labels stop rendering as raw i18n keys.

Two defects in the list-view filter panel (objectstack#8815), both in the Console
render layer, with no workaround available downstream.

**A half-filled range no longer refuses the whole view.** Picking a date column
and 「介于」 draws two inputs — that part landed in objectui#3958 — but typing
only one bound produced `["2024-01-01", ""]`, and both write paths read "is this
row filled in?" with one shape-blind predicate (`null` / `''` / empty array).
An array of length 2 passed it, so the empty bound went to the server, which
refuses the query outright (`400 INVALID_FILTER`): the list showed
「该视图的查询被拒绝」 and the filters the user had already applied stopped
applying too. The saved-view fold persisted the same half-range, so the refusal
came back on every later read of that view, for every user of it.

The spec cannot intercept this — `ViewFilterRuleSchema` accepts
`["2024-01-01", ""]` because it counts the two slots rather than what is in
them, while refusing a scalar or a one-element array. Authoring validation is
therefore green on exactly the shape that fails at query time, which makes not
emitting it the producer's job. `@object-ui/components` now exports
`isFilterValueComplete(operator, value)` — arity-aware, so a `pair` row needs
both bounds — and the two consumers that had each kept a copy of the old
predicate (`plugin-list`'s `convertFilterGroupToAST`, `app-shell`'s
`foldFilterGroupToSpecRules`) read it instead. A half-filled range is now
dropped exactly as a half-typed `equals` row already was: no filter, rather than
a filter the server will reject. Bounds of `0` and `false` stay real bounds.

**Six operator labels are translated in all ten locale packs.**
`startsWith`, `endsWith`, `isNull`, `isNotNull`, `exists` and `notExists` were
missing from every pack, so i18next resolved them to the raw key and the dropdown
showed `filterBuilder.operators.isNull` beside translated entries. The
component's own defaults table could not cover it: that table serves only the
no-provider path, and the Console mounts a provider. The report named four —
a `date` column's bucket offers the four nullness operators; a `text` column
showed all six.

Because the label key is built dynamically (`t(\`filterBuilder.operators.${op}\`)`),
no existing gate could see the gap: the call-site checker classifies a template
key as `missing-prefix` and only asks whether the prefix resolves, and
cross-pack parity is satisfied when all ten packs are missing a key together.
A new parity test pins the packs against `FILTER_BUILDER_OPERATORS` in both
directions, so an operator added to the dropdown now fails loudly until every
pack labels it.
130 changes: 130 additions & 0 deletions packages/app-shell/src/views/viewFilterFold.pairValue.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/**
* 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.
*/

/**
* objectstack#8815 — a HALF-FILLED `between` must never be persisted.
*
* The sibling of objectui#4155, one arity over. That issue made this fold agree
* with the live query about the unfilled row; both then asked the question with
* the same shape-blind predicate:
*
* ```ts
* value == null || value === '' || (Array.isArray(value) && value.length === 0)
* ```
*
* Correct for `scalar` and `list`, wrong for `pair`. A `between` row with only
* the start typed is `["2024-01-01", ""]` — an array of length 2 — so both
* halves agreed it was complete. They were agreed and both wrong, which is the
* failure mode a shared predicate is supposed to prevent and a COPIED one does
* not: the two sides matched because the copy matched, not because either was
* right.
*
* Persisting it is the worse half of the damage. The live query at least fails
* in front of the user who typed it; a stored half-range makes the view refuse
* (`400 INVALID_FILTER`) for EVERY user on every later read, with nothing in the
* panel to explain it — the same shape objectui#4155 closed, arriving through
* the operator's arity instead of through an empty string.
*
* The spec does not catch it either. `ViewFilterRuleSchema` on
* `@objectstack/spec` 17.0.0 ACCEPTS `['2024-01-01', '']` — it counts two slots
* and does not ask what is in them — while refusing a scalar or a one-element
* array. So authoring validation is green on exactly the shape that fails at
* query time, which is why the producer has to refuse it.
*
* SUITE DIRECTION, predicted before running: RED against `origin/main`'s fold on
* every half-filled case (it returns the rule verbatim, empty bound and all),
* green on the paired and value-less cases both before and after.
*/
import { describe, it, expect } from 'vitest';
import { ViewFilterRuleSchema } from '@objectstack/spec/ui';
import { foldFilterGroupToSpecRules } from './viewFilterFold';

const group = (value: unknown) => ({
id: 'root',
logic: 'and' as const,
conditions: [{ id: 'c1', field: 'declare_date', operator: 'between', value }],
});

describe('foldFilterGroupToSpecRules — `between` needs both bounds', () => {
it('persists a complete range', () => {
const result = foldFilterGroupToSpecRules(group(['2024-01-01', '2024-03-01']));
expect(result).toEqual({
ok: true,
rules: [
{ field: 'declare_date', operator: 'between', value: ['2024-01-01', '2024-03-01'] },
],
});
});

it('the persisted range is a rule the spec accepts', () => {
const result = foldFilterGroupToSpecRules(group(['2024-01-01', '2024-03-01']));
if (!result.ok) throw new Error('fold refused a complete range');
// Asserted against the schema itself rather than a hand-written shape:
// what "well-formed" means here is the spec's answer, not this file's.
const parsed = ViewFilterRuleSchema.safeParse(result.rules[0]);
expect(parsed.success).toBe(true);
});

it.each([
['upper bound missing', ['2024-01-01', '']],
['lower bound missing', ['', '2024-03-01']],
['both bounds missing', ['', '']],
['nothing filled in', []],
])('drops the row when %s', (_name, value) => {
expect(foldFilterGroupToSpecRules(group(value))).toEqual({ ok: true, rules: [] });
});

it('drops only the half-filled range, keeping the complete rules beside it', () => {
const result = foldFilterGroupToSpecRules({
id: 'root',
logic: 'and' as const,
conditions: [
{ id: 'c1', field: 'stage', operator: 'equals', value: 'won' },
{ id: 'c2', field: 'declare_date', operator: 'between', value: ['2024-01-01', ''] },
],
});
expect(result).toEqual({
ok: true,
rules: [{ field: 'stage', operator: 'equals', value: 'won' }],
});
});

it('keeps a bound of 0 — a real bound, not an empty one', () => {
const result = foldFilterGroupToSpecRules({
id: 'root',
logic: 'and' as const,
conditions: [{ id: 'c1', field: 'amount', operator: 'between', value: [0, 100] }],
});
expect(result).toEqual({
ok: true,
rules: [{ field: 'amount', operator: 'between', value: [0, 100] }],
});
});

it('leaves the scalar and list families reading exactly as they did', () => {
// The change is scoped to `pair`. A red here means it reached further.
expect(
foldFilterGroupToSpecRules({
id: 'root',
logic: 'and' as const,
conditions: [
{ id: 'c1', field: 'stage', operator: 'equals', value: '' },
{ id: 'c2', field: 'kind', operator: 'in', value: [] },
{ id: 'c3', field: 'name', operator: 'equals', value: 'acme' },
{ id: 'c4', field: 'kind', operator: 'in', value: ['a'] },
],
}),
).toEqual({
ok: true,
rules: [
{ field: 'name', operator: 'equals', value: 'acme' },
{ field: 'kind', operator: 'in', value: ['a'] },
],
});
});
});
12 changes: 11 additions & 1 deletion packages/app-shell/src/views/viewFilterFold.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,21 @@ describe('foldFilterGroupToSpecRules — flat AND group → ViewFilterRule[]', (
'greaterOrEqual', 'lessOrEqual', 'before', 'after', 'between',
'in', 'notIn', 'startsWith', 'endsWith', 'isNull', 'isNotNull',
];
// `'x'` is filler for every operator whose value is a SCALAR, which is
// all of them but one: `between` takes a `[min, max]` pair, and since
// objectstack#8815 the fold drops a range that is not filled in — a bare
// scalar included. Giving it a real pair keeps this test asserting what
// it is named for (camelCase → canonical SPELLING) instead of quietly
// depending on a malformed range surviving the fold. Not a workaround
// for the guard: `ViewFilterRuleSchema` refuses `between: 'x'` outright,
// so the row this fixture used to produce was never spec-valid.
const valueFor = (operator: string) =>
operator === 'between' ? ['x', 'y'] : 'x';
const result = foldFilterGroupToSpecRules({
id: 'root',
logic: 'and',
conditions: builderOperators.map((operator, i) => ({
id: `c${i}`, field: 'f', operator, value: 'x',
id: `c${i}`, field: 'f', operator, value: valueFor(operator),
})),
});
expect(result.ok).toBe(true);
Expand Down
29 changes: 24 additions & 5 deletions packages/app-shell/src/views/viewFilterFold.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

import { normalizeFilterOperator } from '@objectstack/spec/ui';
import type { ViewFilterRule } from '@objectstack/spec/ui';
import { VALUELESS_FILTER_BUILDER_OPERATORS } from '@object-ui/components';
import { VALUELESS_FILTER_BUILDER_OPERATORS, isFilterValueComplete } from '@object-ui/components';

/** Why a group could not be folded to a flat spec rule list. */
export type FilterFoldRefusal =
Expand Down Expand Up @@ -88,9 +88,28 @@ export const VALUELESS_FILTER_OPERATORS: ReadonlySet<string> = new Set([
'is_empty', 'is_not_empty', 'is_null', 'is_not_null',
]);

/** A value the user has not supplied yet — the same predicate the live query uses. */
function isMissingValue(value: unknown): boolean {
return value == null || value === '' || (Array.isArray(value) && value.length === 0);
/**
* A value the user has not supplied yet — the same predicate the live query
* uses, which is now the builder's own {@link isFilterValueComplete}
* (objectstack#8815).
*
* It used to be a local copy of the shape-blind reading (`== null || === '' ||
* empty array`), and "the same predicate the live query uses" was true only
* because the live query held an identical copy. Both were blind to the
* operator's ARITY: a `between` row with one bound typed is an array of length
* 2, so both read it as complete — the grid queried a half-open range the server
* refuses (`400 INVALID_FILTER`) and this fold PERSISTED it, so the refusal
* returned on every later read of that view.
*
* Reading the builder's export keeps the promise this comment always made: what
* is not applied is not persisted, decided in one place instead of two copies
* that agreed by luck.
*/
function isMissingValue(operator: unknown, value: unknown): boolean {
return !isFilterValueComplete(
String(operator ?? ''),
value as Parameters<typeof isFilterValueComplete>[1],
);
}

function isGroupLike(value: unknown): value is FilterGroupLike {
Expand Down Expand Up @@ -178,7 +197,7 @@ export function foldFilterGroupToSpecRules(group: unknown): FilterFoldResult {
// the next read.
const takesValue = !VALUELESS_FILTER_OPERATORS.has(String(c.operator))
&& !VALUELESS_FILTER_OPERATORS.has(String(operator));
if (takesValue && isMissingValue(c.value)) continue;
if (takesValue && isMissingValue(c.operator, c.value)) continue;
const rule: ViewFilterRule = {
field: c.field,
operator,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/**
* Every operator the FilterBuilder dropdown can DRAW carries a label in every
* locale pack (objectstack#8815).
*
* ## The hole this closes
*
* The operator label is looked up with a key the component BUILDS:
*
* ```tsx
* {t(`filterBuilder.operators.${op.value}`)}
* ```
*
* Nothing upstream can check that. `scripts/check-i18n-call-site-keys.mjs`
* classifies a template key as `missing-prefix` and asks only whether the
* PREFIX (`filterBuilder.operators`) resolves — it does, sixteen members deep —
* because there is no static way to know which members a dynamic key needs.
* `all-locales-key-parity.test.ts` asks whether the ten packs AGREE, and its own
* header records why that is green here by construction: "Ten packs identically
* missing it is full parity."
*
* So six operators — `startsWith`, `endsWith`, `isNull`, `isNotNull`, `exists`,
* `notExists` — were absent from ALL TEN packs at once and every existing gate
* was green. What the user saw was the raw key: i18next resolves a missing key
* to the key itself, and the component's `createSafeTranslation` defaults table
* cannot save it — that table serves only the NO-PROVIDER path, and the Console
* mounts a provider, so the pack's answer is the one that renders.
*
* The reported repro (a `date` column) shows exactly four of them, because a
* date field's bucket offers the four nullness operators and not
* `startsWith`/`endsWith`; a `text` column shows all six. That is why this test
* asserts over the whole DRAWABLE vocabulary rather than the four that were
* reported — the defect was never about which bucket the reporter opened.
*
* ## Why the assertion is against `FILTER_BUILDER_OPERATORS`
*
* That export is the vocabulary the dropdown draws, derived from the operators
* the component actually renders — so a new operator added to `defaultOperators`
* lands here as a red test naming the packs it still needs, instead of shipping
* as a raw key. It includes the opt-in ids (`containsCaseInsensitive`,
* `exists`, `notExists`): `OPT_IN_OPERATORS` governs which CONSUMERS are offered
* an operator, not whether the builder can draw it, and `FilterConditionField`
* grants all three — a drawable operator needs a label.
*
* Placement is forced by the dependency graph: `@object-ui/components` depends
* on `@object-ui/i18n`, so this is the side that can see both the vocabulary and
* the packs. The reverse import would be a cycle, which is why this guard cannot
* live beside the other locale-parity tests in `packages/i18n`.
*/
import { describe, it, expect } from 'vitest';
import { builtInLocales } from '@object-ui/i18n';

import { FILTER_BUILDER_OPERATORS } from '../custom/filter-builder';

type LocaleCode = keyof typeof builtInLocales;

const LOCALES = Object.keys(builtInLocales) as LocaleCode[];

/** Read a dot-path out of a nested translation node. */
function readPath(node: unknown, path: string): unknown {
return path.split('.').reduce<unknown>(
(acc, seg) =>
acc && typeof acc === 'object' ? (acc as Record<string, unknown>)[seg] : undefined,
node,
);
}

const operatorsNode = (code: LocaleCode): unknown =>
readPath(builtInLocales[code], 'filterBuilder.operators');

describe('filterBuilder.operators locale parity', () => {
it('every pack defines the namespace', () => {
const missing = LOCALES.filter((code) => !operatorsNode(code));
expect(missing).toEqual([]);
});

it.each(LOCALES)('%s labels every operator the dropdown can draw', (code) => {
const node = operatorsNode(code) as Record<string, unknown>;
// Reported as the full list rather than one `toBeDefined()` per operator:
// these went missing six at a time, and a failure that names all six is the
// difference between one fix and six rounds of it.
const missing = FILTER_BUILDER_OPERATORS.filter((op) => {
const label = node?.[op];
return typeof label !== 'string' || label.trim() === '';
});
expect(missing).toEqual([]);
});

it.each(LOCALES)('%s defines no operator label the dropdown cannot draw', (code) => {
// The other direction: a label for an operator that no longer exists is
// dead weight that reads as coverage. Keeps the two sides converging.
const node = operatorsNode(code) as Record<string, unknown>;
const orphaned = Object.keys(node ?? {}).filter(
(key) => !(FILTER_BUILDER_OPERATORS as readonly string[]).includes(key),
);
expect(orphaned).toEqual([]);
});

it('no pack renders an operator label as its own raw key', () => {
// The exact user-visible symptom: `t('filterBuilder.operators.isNull')`
// returning `'filterBuilder.operators.isNull'`. A pack that "defines" the
// key as the key would satisfy the presence assertions above while showing
// the user the same machine name.
const offenders: string[] = [];
for (const code of LOCALES) {
const node = operatorsNode(code) as Record<string, unknown>;
for (const op of FILTER_BUILDER_OPERATORS) {
const label = node?.[op];
if (typeof label === 'string' && label.includes('filterBuilder.operators')) {
offenders.push(`${code}.${op}`);
}
}
}
expect(offenders).toEqual([]);
});
});
Loading
Loading