diff --git a/.changeset/objectview-overlay-pair-completeness-5025.md b/.changeset/objectview-overlay-pair-completeness-5025.md new file mode 100644 index 000000000..85273d19a --- /dev/null +++ b/.changeset/objectview-overlay-pair-completeness-5025.md @@ -0,0 +1,33 @@ +--- +'@object-ui/app-shell': patch +--- + +`sanitizeViewOverride` now strips a half-filled `between` from a stored view +overlay, instead of handing it back to the merge (objectui#5025, +objectstack#8815). + +The overlay recovery pass asked "is this filter row filled in?" with the +shape-blind predicate objectstack#8815 retired — `value == null || value === '' +|| (Array.isArray(value) && value.length === 0)`. That is correct for `scalar` +and `list` and blind to `pair`: a `between` carrying one bound is +`['2024-01-01', '']`, an array of length 2, so the pass read it as a real +condition and kept it. The two write paths (`plugin-list`'s `ListView` and +app-shell's `viewFilterFold`) were converted to the builder's arity-aware +`isFilterValueComplete`; this read path was the third verbatim copy, and the one +whose job was to clean up exactly the rows the other two used to write. Until +now a stored half-range survived the pass, reached the query, and the server +refused the whole view (`400 INVALID_FILTER`) for every user on every later +read. + +The pass now delegates to the same `isFilterValueComplete` the write paths use, +on both of the at-rest shapes it handles (the spec `ViewFilterRule` object and +the legacy runtime triple). A complete range — bounds of `0` and `false` +included — is untouched, and the `scalar` and `list` families read exactly as +before, since the retired predicate was already right for them. One shape is +newly stripped beyond the half-filled array: a `between` whose value is a bare +scalar, which `ViewFilterRuleSchema` itself refuses. + +Which operators want no value at all is unchanged and still answered by +app-shell's `VALUELESS_FILTER_OPERATORS` — that set is already derived from the +builder's `VALUELESS_FILTER_BUILDER_OPERATORS` rather than being a private copy, +and its parity is pinned. diff --git a/packages/app-shell/src/views/ObjectView.overlayPairValue.test.ts b/packages/app-shell/src/views/ObjectView.overlayPairValue.test.ts new file mode 100644 index 000000000..abb7bb653 --- /dev/null +++ b/packages/app-shell/src/views/ObjectView.overlayPairValue.test.ts @@ -0,0 +1,205 @@ +/** + * 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. + */ + +/** + * objectui#5025 — the overlay RECOVERY pass must strip a half-filled `between`. + * + * The third site of objectstack#8815, and the one that matters most, because it + * is the pass that exists to CLEAN UP the shape the other two used to write. + * All three asked "is this filter row filled in?" with one shape-blind + * predicate: + * + * ```ts + * value == null || value === '' || (Array.isArray(value) && value.length === 0) + * ``` + * + * Right for `scalar` and `list`, blind to `pair`. A `between` row with one bound + * typed is `['2024-01-01', '']` — an array of length 2 — so it read as a real + * condition. The two WRITE paths were converted (`plugin-list/src/ListView.tsx`, + * `views/viewFilterFold.ts`); `sanitizeViewOverride` kept the verbatim copy, so + * the read path handed the half-range straight back to the merge and the view + * went on being refused (`400 INVALID_FILTER`) for every user on every load. + * + * WHY THIS IS REACHABLE, not dead-but-misleading code — the question the card + * asked to measure first. Two independent producers, neither of them the + * converted builder: + * + * 1. Installs that ran a pre-conversion build. `foldFilterGroupToSpecRules` + * persisted half-filled ranges into stored view bodies, and this pass is + * documented as the recovery half for exactly that class of row — "an + * install already carrying a poisoned row self-heals on the next load". + * It self-healed every arity except the one the sibling issue was about. + * 2. Any producer that is not the builder — hand-authored view JSON, an + * AI-authored metadata app, an import, a migration — because the SPEC + * accepts the shape. Measured below rather than asserted from the + * docstring: `ViewFilterRuleSchema` counts the two slots and does not ask + * what is in them, so authoring validation is green on a half-filled range + * and it only dies at query time. That makes this read-path pass the last + * guard standing, not a tidy-up. + * + * SUITE DIRECTION — stated before running. + * + * RED under the mutation leg that restores the old predicate at both branches + * of `sanitizeViewOverride`: every case in the two `between` blocks below. + * Those are the discriminating ones; each quotes a VALUE in its failure, not + * an import. + * + * GREEN under that same leg, by design: the whole `arities this change must + * not touch` block (the old predicate is CORRECT for `scalar` and `list`, so + * an assertion that passes on it there proves the change stayed scoped), plus + * the spec-acceptance case, which is a fact about `@objectstack/spec` and + * would pass in any world. It is recorded because it is the reason the pair + * cases matter, and it is labelled non-discriminating so nobody later reads + * it as a pin on OUR code. + * + * Those controls are shown capable of failing by a SECOND leg (see the PR + * body): making the sanitizer's predicate answer `false` unconditionally + * turns the control block red while leaving the pair block green — the two + * legs fail in disjoint directions, which is what makes each of them a + * measurement rather than a decoration. + */ + +import { describe, it, expect } from 'vitest'; +import { ViewFilterRuleSchema } from '@objectstack/spec/ui'; +import { sanitizeViewOverride } from './ObjectView'; + +/** The source-declared body an overlay must not be able to poison or erase. */ +const SOURCE_VIEW = { + label: 'All work orders', + columns: ['name', 'declare_date'], + filter: [{ field: 'status', operator: 'not_in', value: ['archived', 'deleted'] }], +}; + +const overlay = (filter: unknown[]) => ({ name: 'wo.all', object: 'work_order', filter }); + +describe('sanitizeViewOverride — a `between` needs both bounds (#5025)', () => { + it.each([ + ['upper bound missing', ['2024-01-01', '']], + ['lower bound missing', ['', '2024-03-01']], + ['both bounds missing', ['', '']], + // Not an array at all. The spec REFUSES a scalar under `between` + // (measured in the block below), so the server would refuse it too — + // stripping it is the same judgement, one layer earlier. + ['a scalar where a range belongs', '2024-01-01'], + ])('drops the whole `filter` key when the only rule has %s', (_name, value) => { + const sanitized = sanitizeViewOverride( + overlay([{ field: 'declare_date', operator: 'between', value }]), + ); + // The KEY, not an empty array: `{ ...source, ...override }` is key-wise, + // so `filter: []` would still blank the source declaration. + expect('filter' in sanitized).toBe(false); + expect({ ...SOURCE_VIEW, ...sanitized }.filter).toEqual(SOURCE_VIEW.filter); + }); + + it('drops it in the legacy runtime triple shape too', () => { + const sanitized = sanitizeViewOverride( + overlay([['declare_date', 'between', ['2024-01-01', '']]]), + ); + expect('filter' in sanitized).toBe(false); + }); + + it('drops only the half-filled range, keeping the complete rules beside it', () => { + expect( + sanitizeViewOverride( + overlay([ + { field: 'stage', operator: 'equals', value: 'won' }, + { field: 'declare_date', operator: 'between', value: ['2024-01-01', ''] }, + ]), + ).filter, + ).toEqual([{ field: 'stage', operator: 'equals', value: 'won' }]); + }); +}); + +describe('sanitizeViewOverride — a complete `between` survives untouched (#5025)', () => { + it('keeps a range with both bounds, and returns the row by identity', () => { + const row = overlay([ + { field: 'declare_date', operator: 'between', value: ['2024-01-01', '2024-03-01'] }, + ]); + expect(sanitizeViewOverride(row)).toBe(row); + }); + + it('keeps a bound of 0 — a real bound on a number column (objectui#4873)', () => { + const row = overlay([{ field: 'amount', operator: 'between', value: [0, 100] }]); + expect(sanitizeViewOverride(row)).toBe(row); + }); + + it('keeps a bound of `false`, which `!bound` would have read as unfilled', () => { + const row = overlay([{ field: 'flag', operator: 'between', value: [false, true] }]); + expect(sanitizeViewOverride(row)).toBe(row); + }); + + it('keeps a complete range in the legacy runtime triple shape', () => { + const row = overlay([['declare_date', 'between', ['2024-01-01', '2024-03-01']]]); + expect(sanitizeViewOverride(row)).toBe(row); + }); +}); + +describe('the arities this change must not touch (#5025)', () => { + // The retired predicate is CORRECT for `scalar` and `list`. These cases read + // identically before and after, and that is the point: a red here means the + // conversion reached past `pair`. + it('still drops an empty scalar and an empty list', () => { + expect( + sanitizeViewOverride( + overlay([ + { field: 'name', operator: 'equals', value: '' }, + { field: 'kind', operator: 'in', value: [] }, + { field: 'owner', operator: 'equals', value: null }, + ]), + ).filter, + ).toBeUndefined(); + }); + + it('still keeps a filled scalar and a non-empty list', () => { + const row = overlay([ + { field: 'name', operator: 'equals', value: 'acme' }, + { field: 'kind', operator: 'in', value: ['a'] }, + ]); + expect(sanitizeViewOverride(row)).toBe(row); + }); + + it('still keeps a value-less operator whose value slot is empty', () => { + // Both dialects: the builder id and the canonical spec spelling this + // layer additionally sees. + const row = overlay([ + { field: 'closed_at', operator: 'isEmpty', value: '' }, + { field: 'owner', operator: 'is_null', value: '' }, + ]); + expect(sanitizeViewOverride(row)).toBe(row); + }); +}); + +describe('why the read path is the last guard (#5025)', () => { + // NON-DISCRIMINATING by construction: this asserts a fact about + // `@objectstack/spec`, not about `sanitizeViewOverride`, so it is green in + // every world. It is here because it is the reason the cases above are a + // bug fix rather than a tidy-up — authoring validation cannot catch this + // shape, so nothing upstream of the recovery pass will. + it('the spec ACCEPTS a half-filled range, so authoring validation lets it through', () => { + expect( + ViewFilterRuleSchema.safeParse({ + field: 'declare_date', + operator: 'between', + value: ['2024-01-01', ''], + }).success, + ).toBe(true); + }); + + it('…while refusing the shapes that are merely malformed', () => { + for (const value of ['2024-01-01', ['2024-01-01']]) { + expect( + ViewFilterRuleSchema.safeParse({ + field: 'declare_date', + operator: 'between', + value, + }).success, + JSON.stringify(value), + ).toBe(false); + } + }); +}); diff --git a/packages/app-shell/src/views/ObjectView.tsx b/packages/app-shell/src/views/ObjectView.tsx index a7e288ca2..fe72f1a7f 100644 --- a/packages/app-shell/src/views/ObjectView.tsx +++ b/packages/app-shell/src/views/ObjectView.tsx @@ -35,6 +35,7 @@ import { EmptyTitle, EmptyDescription, NavigationOverlay, + isFilterValueComplete, } from '@object-ui/components'; import { Plus, Upload, Star, StarOff, Table as TableIcon, KanbanSquare, Calendar, LayoutGrid, Activity, GanttChart, MapPin, BarChart3 } from 'lucide-react'; import { useFavorites } from '../hooks/useFavorites.js'; @@ -325,6 +326,30 @@ export function defaultListColumnsFromObject( * objects (`{ field, operator, value }`, what the fold writes) and the legacy * runtime triple `[field, operator, value]` a source view may declare and which * `persistViewPatch` copies into the overlay along with the rest of the view. + * + * Whether a matched entry's VALUE counts as supplied is asked of the builder's + * own {@link isFilterValueComplete}, not of a local predicate (objectstack#8815, + * objectui#5025). This pass used to spell the question out here: + * + * ```ts + * value == null || value === '' || (Array.isArray(value) && value.length === 0) + * ``` + * + * — right for `scalar` and `list`, blind to `pair`. A `between` carrying one + * bound is `['2024-01-01', '']`, an array of length 2, so the recovery pass read + * it as a real condition and handed it back. That is the one shape this pass + * most needs to strip: `ViewFilterRuleSchema` ACCEPTS a half-filled range + * (measured on `@objectstack/spec` 17.1.0 — it counts the two slots, not what is + * in them), so authoring validation is green on it and it only dies at query + * time, where the server refuses the WHOLE view (`400 INVALID_FILTER`) for every + * user on every later read. The builder's two write paths were converted to the + * arity-aware reading; this is the read-path half, and it was the third verbatim + * copy of a predicate that had already been indicted twice. + * + * The split of duties is the helper's own: it answers the VALUE question only, + * while which operators want no value at all stays with + * {@link VALUELESS_FILTER_OPERATORS} above — this layer additionally sees the + * canonical spec spellings (`is_null`) that never reach the dropdown. */ export function sanitizeViewOverride(override: any): any { if (!override || typeof override !== 'object') return override; @@ -348,13 +373,13 @@ export function sanitizeViewOverride(override: any): any { if (entry.length < 2) return false; const [, operator, value] = entry; if (VALUELESS_FILTER_OPERATORS.has(String(operator))) return true; - return !(value == null || value === '' || (Array.isArray(value) && value.length === 0)); + return isFilterValueComplete(String(operator), value); } if (!entry || typeof entry !== 'object') return false; if (typeof entry.field !== 'string' || entry.field === '') return false; if (VALUELESS_FILTER_OPERATORS.has(String(entry.operator))) return true; const value = entry.value; - return !(value == null || value === '' || (Array.isArray(value) && value.length === 0)); + return isFilterValueComplete(String(entry.operator), value); }); // The empty-array case is checked FIRST: `filter: []` (Clear all's write)