diff --git a/.changeset/8415-filter-builder-condition-id.md b/.changeset/8415-filter-builder-condition-id.md new file mode 100644 index 0000000000..be983b0ec9 --- /dev/null +++ b/.changeset/8415-filter-builder-condition-id.md @@ -0,0 +1,118 @@ +--- +'@object-ui/types': minor +--- + +**Breaking for authored metadata:** a `filter-builder` CONDITION must now declare +`id` (objectui#8415). It is declared on both published faces — the TypeScript +interface `FilterBuilderCondition` in `complex.ts` and the Zod mirror +`FilterBuilderConditionSchema` in `zod/complex.zod.ts` — so a key the renderer +has always required is finally validated instead of silently discarded. + +**What was measured, on this branch's base (`0203a29e`).** The mirror declared a +condition as `{ field, operator, value? }` and it is a plain `z.object`, which +STRIPS undeclared keys. So an author who correctly wrote `id` had it removed: +`FilterBuilderConditionSchema.safeParse({ id: 'c1', field, operator, value })` +returned `success: true` with an output whose keys were `field`, `operator`, +`value` — no `id`. The document then validated and the row rendered, and from +that point on the row had no individual identity: every affordance on it is +handed `undefined`, so it acts on every OTHER id-less row along with it. The +measurement is below. + +Re-derived from `packages/components/src/custom/filter-builder.tsx` rather than +inherited: `id` has **sixteen** condition-side read sites — the four MATCH sites +that decide which row a mutation lands on (`removeCondition`'s +`c.id !== conditionId`, `updateCondition`'s and `changeOperator`'s +`c.id === conditionId`, `changeField`'s `c.id !== conditionId`), the React `key` +on the row, and eleven call sites that hand `condition.id` to one of those four. + +**What that does when `id` is stripped**, simulated on the four helper bodies +transcribed verbatim, over three id-less rows and one `crypto.randomUUID()` row +(the realistic mix: the mirror strips every AUTHORED row's `id`, while rows the +user adds in-session are born with one). Both sides of every comparison are +`undefined`, and `undefined === undefined` is TRUE, so each helper matches EVERY +id-less row rather than none: + +- `removeCondition(undefined)` keeps only rows where `c.id !== undefined`, so it + deletes all three id-less rows in a single click — the clicked one included — + and leaves the uuid row standing. Measured: 3 of 4 rows removed. +- `updateCondition(undefined, …)`, `changeOperator(undefined, …)` and + `changeField(undefined, …)` each apply the edit to all three at once. + Measured: 3 of 4 rows moved, each time. +- `key={condition.id}` becomes `key={undefined}`, which React reads as NO key at + all rather than as a duplicate one. Measured on React 19.2.8: `element.key` is + `null` for every such row, the list falls back to index reconciliation, and + React logs `Each child in a list should have a unique "key" prop.` + +So the defect is **loss of individual identity** — every affordance acts on all +the id-less rows en bloc, and a row cannot be edited or removed on its own. It +is not "matches none", and it is the more severe of the two readings. The +component's own exported `FilterBuilderCondition` has always declared +`id: string`, and `addCondition` emits `crypto.randomUUID()`. + +**Who is affected — a condition authored without `id`:** + +```json +{ "type": "filter-builder", "name": "f", + "fields": [{ "value": "a", "label": "A", "type": "text" }], + "value": { "id": "root", "logic": "and", + "conditions": [{ "field": "a", "operator": "equals", "value": "x" }] } } +``` + +now fails validation. **Where it is REPORTED is not where it logically is.** +`value.conditions.0.id` is the LOGICAL location — the concatenation of the paths +down the arm tree. The issue `safeValidateSchema` actually reports is a single +root `invalid_union` at `path: []`, across **13** arms; the `id` failure sits +three nested unions further down, inside arm 8: `invalid_union` at `["value"]` +→ arm 1 → `invalid_union` at `["conditions", 0]` → arm 0 → `invalid_type` at +`["id"]`, *"Invalid input: expected string, received undefined"*. Parsed against +`FilterBuilderConditionSchema` directly, the same refusal is reported flat, at +`path: ["id"]`. Both are stated because a consumer that reads `issue.path` off +the document-level result will not find `id` there. + +**The compile-time face, which lands before any document is parsed.** `id` is +declared on the TypeScript interface as well, so a TypeScript author is refused +by `tsc`, not only by the validator. An object literal typed +`FilterBuilderCondition`, or `FilterGroup['conditions'][number]`, or a condition +written inside a `FilterGroup` / `FilterBuilderSchema['value']` / `defaultValue` +literal, now fails type-check with *"Property 'id' is missing in type … but +required in type 'FilterBuilderCondition'"* — measured on all four spellings. +The GROUP's own `id` stays optional, so `{ logic: 'and', conditions: [] }` still +compiles. Stated explicitly for the reason objectui#7774 stated its compile-time +face (`groupField?: never`, "refused at compile time"): it is the half a +TypeScript author meets first, and a runtime-only description hides it. + +**Migration:** give each condition a stable unique string — any value the rest of +the document does not reuse; the component generates `crypto.randomUUID()` for +rows the user adds. + +⭐ **The narrowing refuses only what was ALREADY broken.** A condition with no +`id` renders today but cannot be edited or removed INDIVIDUALLY — as measured +above, one click removes every id-less row at once and one edit fans out across +all of them — so nothing that works stops working; the state this refuses is +*accepted-and-discarded*, the class objectui#6150 closed for `tree-view.title`. +Measured across `apps/**`, `examples/**`, `content/**` and `packages/**`: +**every** authored `filter-builder` condition in this repository already carries +`id` — 7 of 7, across the three schema-catalog entries that author rows — so no +shipped document in this repo changes verdict on this key. + +**What else now refuses, and it is the second thing declaring a key buys:** +`id: 42`. An UNDECLARED key gets no type check at all, so a wrong-typed identity +parsed clean at base and was then dropped. It is now refused — reported flat at +`path: ["id"]` on the condition schema (*"Invalid input: expected string, +received number"*), and through the document at the same place in the arm tree +as the missing key above. + +**Who is NOT affected.** ⛔ The GROUP's `id` is untouched and stays OPTIONAL +(objectui#7560): `isValidGroup` never consults it, nothing reads +`filterGroup.id`, and deleting it from an authored group renders +byte-identically — the opposite reading, on a member that looks identical. +`{ logic: 'and', conditions: [] }` still validates. The renderer is unchanged; +`FilterOperatorSchema`, `FilterFieldSchema` and `FilterBuilderSchema`'s own keys +are byte-identical, so the three items parked on objectui#7562 and the operator +vocabulary of objectui#7561 are neither addressed nor moved here. + +Graded `minor`, not `patch`: this narrows the accepted input set, which is +breaking for any author who wrote a condition without an identity. It is not +`major` per this repo's fixed-group convention (objectui's own breaking changes +ship as `minor`; the group's major tracks `@objectstack` — AGENTS.md 版本号策略, +mechanically enforced by `scripts/check-changeset-no-major.mjs`). diff --git a/packages/types/src/__tests__/filter-builder-condition-id-8415.test.ts b/packages/types/src/__tests__/filter-builder-condition-id-8415.test.ts new file mode 100644 index 0000000000..3d1c1ffa59 --- /dev/null +++ b/packages/types/src/__tests__/filter-builder-condition-id-8415.test.ts @@ -0,0 +1,191 @@ +/** + * 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#8415 — `FilterBuilderConditionSchema` declares `id`. + * + * ## What was wrong + * + * The mirror declared a condition as `{ field, operator, value? }`. The + * component's identity for a row is `id`, and because the mirror is a plain + * `z.object`, an author who wrote `id` correctly had it STRIPPED in silence. + * The document then validated and the row rendered — and from then on the row + * had no individual identity. All four mutation helpers match on `c.id`, every + * one of them is handed `undefined`, and `undefined === undefined` is TRUE, so + * each matches EVERY id-less row: `removeCondition` deletes them all in one + * click (the clicked row included; only uuid-bearing rows survive), and + * `updateCondition` / `changeOperator` / `changeField` fan one edit out across + * all of them. `key={condition.id}` becomes `key={undefined}`, which React + * reads as no key at all rather than as a duplicate one. + * + * ⛔ Not "matches none" — the failure is EN BLOC, and it is the more severe of + * the two readings. + * + * Measured on the base commit, through `FilterBuilderConditionSchema.safeParse`: + * a condition carrying `id` parsed successfully and the parsed OUTPUT did not + * carry the key. That is the `accepted-and-discarded` class objectui#6150 + * closed for `tree-view.title`, not a refusal. + * + * ## Why REQUIRED, and why that is not the answer the GROUP got + * + * ⛔ The two `id`s look alike and take OPPOSITE answers — do not unify them. + * + * - `FilterGroupSchema.id` is declared OPTIONAL (objectui#7560): `isValidGroup` + * never consults it, nothing reads `filterGroup.id`, and deleting it from an + * authored group renders byte-identically. Requiring it would invent a + * refusal the renderer does not make. + * - A CONDITION's `id` is the opposite reading. `assertion the four match + * sites and the React key are live` below re-derives it from the component + * source rather than quoting a count. + * + * ⭐ The narrowing therefore refuses only what is ALREADY broken: a condition + * with no `id` renders today but cannot be edited or removed INDIVIDUALLY — + * every affordance on it acts on all the id-less rows at once. Nothing that + * works stops working. + */ +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; + +import { FilterBuilderConditionSchema, FilterGroupSchema } from '../zod/complex.zod'; +import { safeValidateSchema } from '../zod/index.zod'; +import type { FilterBuilderCondition as TsCondition, FilterGroup as TsGroup } from '../complex'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = join(HERE, '..', '..', '..', '..'); +const READER = 'packages/components/src/custom/filter-builder.tsx'; +const readerSource = readFileSync(join(REPO_ROOT, READER), 'utf8'); + +const FIELDS = [{ value: 'a', label: 'A', type: 'text' }]; +const WELL_FORMED = { id: 'c1', field: 'a', operator: 'equals', value: 'x' }; +const NO_ID = { field: 'a', operator: 'equals', value: 'x' }; +const doc = (value: unknown) => ({ type: 'filter-builder', name: 'f', fields: FIELDS, value }); +const group = (conditions: unknown[]) => ({ id: 'root', logic: 'and', conditions }); + +/* ── Type-level pins: the TS twin moved WITH the mirror ───────────────────── */ + +type Equal = + (() => T extends A ? 1 : 2) extends (() => T extends B ? 1 : 2) ? true : false; +function expectType(_: T = true as T): void { /* compile-time only */ } + +expectType>(); +// `id` is REQUIRED, not merely typed `string`: an object that OMITS the key must +// be rejected, and only this annotation proves it. The `@ts-expect-error` IS the +// assertion — delete the `id` declaration and this line stops erroring, which +// fails the compile. +// @ts-expect-error `FilterBuilderCondition.id` is required (objectui#8415) +const conditionWithoutId: TsCondition = { field: 'a', operator: 'equals' }; +// …and the GROUP's stayed optional, so the two faces cannot be "unified" by a +// future editor without one of these two lines going red. +const groupWithoutId: TsGroup = { logic: 'and', conditions: [] }; + +describe('objectui#8415 — the condition `id` is DECLARED, so it is no longer stripped', () => { + it('the repair itself: `id` survives the parse output', () => { + // The base measurement this moves away from: `success` was already `true`, + // and the key was GONE from `.data`. Asserting only `success` would have + // been green before and after. + const parsed = FilterBuilderConditionSchema.safeParse(WELL_FORMED); + expect(parsed.success).toBe(true); + expect(parsed.success && Object.keys(parsed.data as object).sort()) + .toEqual(['field', 'id', 'operator', 'value']); + expect(parsed.success && (parsed.data as { id?: unknown }).id).toBe('c1'); + }); + + it('REFUSES a condition with no `id`, on the direct arm and through a group', () => { + expect(FilterBuilderConditionSchema.safeParse(NO_ID).success).toBe(false); + expect(FilterGroupSchema.safeParse(group([NO_ID])).success).toBe(false); + expect(FilterGroupSchema.safeParse(group([{ id: 'g2', logic: 'or', conditions: [NO_ID] }])).success) + .toBe(false); + }); + + it('REFUSES it through the authored document too — both entry paths on `FilterBuilderSchema`', () => { + // `value` and `defaultValue` are each `union([condition, group])`, so a + // condition reaches the mirror two ways and the union must not launder it + // through the group arm. + expect(safeValidateSchema(doc(group([NO_ID]))).success).toBe(false); + expect(safeValidateSchema(doc(NO_ID)).success).toBe(false); + expect(safeValidateSchema({ type: 'filter-builder', name: 'f', fields: FIELDS, defaultValue: group([NO_ID]) }).success) + .toBe(false); + }); + + it('type-checks `id` now that it is declared — `id: 42` was ACCEPTED before', () => { + // The second thing declaring a key buys, and the one an author never sees: + // a `z.object` gives an UNDECLARED key no check at all, so `id: 42` parsed + // clean at base and was then dropped. + expect(FilterBuilderConditionSchema.safeParse({ ...WELL_FORMED, id: 42 }).success).toBe(false); + }); +}); + +describe('objectui#8415 — the negative controls that must NOT have moved', () => { + it('still ACCEPTS a well-formed condition (anti-vacuity)', () => { + expect(FilterBuilderConditionSchema.safeParse(WELL_FORMED).success).toBe(true); + expect(FilterGroupSchema.safeParse(group([WELL_FORMED])).success).toBe(true); + expect(safeValidateSchema(doc(group([WELL_FORMED]))).success).toBe(true); + expect(safeValidateSchema(doc(group([]))).success).toBe(true); + }); + + it('still REFUSES a bad operator — and the fixture carries `id` so THIS is what refuses it', () => { + // Without the `id`, this assertion would stay green with + // `FilterOperatorSchema` deleted outright: the row would be refused for the + // missing key instead. Carrying `id` isolates the operator. + expect(FilterBuilderConditionSchema.safeParse({ ...WELL_FORMED, operator: 'not_a_real_operator' }).success) + .toBe(false); + expect(FilterBuilderConditionSchema.safeParse({ id: 'c1', operator: 'equals', value: 'x' }).success) + .toBe(false); + }); + + it('the GROUP `id` is UNTOUCHED — still optional, still type-checked (objectui#7560)', () => { + // ⛔ The trap this card was split out to avoid. The group's `id` has zero + // read sites; requiring it would refuse a document that renders perfectly. + expect(FilterGroupSchema.safeParse({ logic: 'and', conditions: [WELL_FORMED] }).success).toBe(true); + expect(FilterGroupSchema.safeParse({ id: 42, logic: 'and', conditions: [] }).success).toBe(false); + expect(FilterGroupSchema.safeParse({ operator: 'and', conditions: [] }).success).toBe(false); + expect(groupWithoutId.conditions).toEqual([]); + expect(conditionWithoutId.field).toBe('a'); + }); +}); + +describe('objectui#8415 — the enforcement the declaration now matches, re-derived from the reader', () => { + it('the four MATCH sites are live in the component', () => { + // Re-derived from source rather than quoted as a count: if a refactor moves + // a row's identity off `id`, this reddens and the REQUIRED declaration has + // to be re-argued rather than silently outliving its reason. + const matchSites = [ + // removeCondition + 'conditions: filterGroup.conditions.filter((c) => c.id !== conditionId)', + // updateCondition + 'c.id === conditionId ? { ...c, ...updates } : c', + // changeOperator + 'c.id === conditionId', + // changeField + 'if (c.id !== conditionId) return c', + ]; + for (const site of matchSites) expect(readerSource).toContain(site); + }); + + it('the React `key` on the row is the condition `id`', () => { + expect(readerSource).toContain('key={condition.id}'); + }); + + it('a new row is BORN with an `id` — the producer agrees with the declaration', () => { + expect(readerSource).toContain('id: crypto.randomUUID(),'); + }); + + it('the component declares `id` non-optional on its own condition type', () => { + // The renderer half of `declared = enforced`. `id?: string` here would mean + // the component tolerates its absence, and the required mirror would be + // narrower than the thing it mirrors. + expect(readerSource).toContain('export interface FilterBuilderCondition {\n id: string\n'); + }); + + it('anti-vacuity for the source probes: a spelling that is NOT there reads false', () => { + // The instrument above is `String.prototype.includes`; a probe that matches + // nothing would make every assertion in this block unfalsifiable. + expect(readerSource).not.toContain('c.zzzNotAKey === conditionId'); + expect(readerSource.length).toBeGreaterThan(1000); + }); +}); diff --git a/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts b/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts index e2a9e3b396..ef24f1f66a 100644 --- a/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts +++ b/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts @@ -257,20 +257,28 @@ describe('objectui#7918 · z.lazy getter identity', () => { }); it('FilterBuilderConditionSchema still accepts a condition and refuses a bad operator', () => { + // ⚠️ Both fixtures carry `id` since objectui#8415 declared it REQUIRED on + // the condition. The NEGATIVE one carries it for a reason that is not + // cosmetic: without it the row would be refused for the MISSING KEY, and + // this assertion — whose subject is `FilterOperatorSchema` — would stay + // green with the operator vocabulary deleted outright. Carrying `id` + // isolates the operator as the only thing left to refuse it. expect(FilterBuilderConditionSchema.safeParse( - { field: 'amount', operator: 'greater_than', value: 100 }, + { id: 'c1', field: 'amount', operator: 'greater_than', value: 100 }, ).success).toBe(true); expect(FilterBuilderConditionSchema.safeParse( - { field: 'amount', operator: 'not_a_real_operator' }, + { id: 'c1', field: 'amount', operator: 'not_a_real_operator' }, ).success).toBe(false); }); it('FilterGroupSchema still nests conditions and sub-groups through the memoised arm', () => { + // The CONDITION rows carry `id` (required since objectui#8415); the + // sub-GROUP's `id` stays what it always was — declared but optional. const group = { id: 'g1', logic: 'and', conditions: [ - { field: 'amount', operator: 'greater_than', value: 100 }, - { id: 'g2', logic: 'or', conditions: [{ field: 'stage', operator: 'equals', value: 'won' }] }, + { id: 'c1', field: 'amount', operator: 'greater_than', value: 100 }, + { id: 'g2', logic: 'or', conditions: [{ id: 'c2', field: 'stage', operator: 'equals', value: 'won' }] }, ], }; expect(FilterGroupSchema.safeParse(group).success).toBe(true); diff --git a/packages/types/src/complex.ts b/packages/types/src/complex.ts index 60cb9fac9e..9b7cc8c591 100644 --- a/packages/types/src/complex.ts +++ b/packages/types/src/complex.ts @@ -586,6 +586,26 @@ export type FilterBuilderOperator = * Filter condition */ export interface FilterBuilderCondition { + /** + * Row identity. + * + * REQUIRED, and deliberately asymmetric with `FilterGroup.id` below + * (objectui#8415). The group's `id` has zero read sites and is optional for + * that reason; a CONDITION's `id` is the identity every affordance on the row + * matches on — `removeCondition`, `updateCondition`, `changeOperator` and + * `changeField` in `packages/components/src/custom/filter-builder.tsx`, plus + * the React `key`. The component's own `FilterBuilderCondition` declares it + * `string`, and `addCondition` emits `crypto.randomUUID()`. + * + * Undeclared, it was STRIPPED by the `z.object` mirror in silence, so a + * correctly authored row validated and rendered with no individual identity: + * every affordance is handed `undefined`, `c.id === conditionId` is TRUE for + * every id-less row, and so one click removes all of them at once — the + * clicked row included — while one edit fans out across all of them. A row + * cannot be edited or removed on its own. (Not "matches none"; the failure is + * en bloc.) Declaring it is what makes `declared = enforced`. + */ + id: string; /** * Field to filter */ diff --git a/packages/types/src/zod/complex.zod.ts b/packages/types/src/zod/complex.zod.ts index ee9cf17f5d..aed0cb61dd 100644 --- a/packages/types/src/zod/complex.zod.ts +++ b/packages/types/src/zod/complex.zod.ts @@ -281,6 +281,41 @@ export const FilterOperatorSchema = z.enum([ * — read that before "fixing" any of them to match this one. */ const FilterBuilderConditionObject = z.object({ + // REQUIRED, and the asymmetry with `FilterGroupSchema.id` below is the whole + // point of declaring it here (objectui#8415). The group's `id` has ZERO read + // sites and is optional for that reason; a CONDITION's `id` is the identity + // every affordance on the row matches on, measured in + // `packages/components/src/custom/filter-builder.tsx`: + // + // - the four MATCH sites — `removeCondition` (`c.id !== conditionId`), + // `updateCondition` and `changeOperator` (`c.id === conditionId`) and + // `changeField` (`c.id !== conditionId`); + // - the React `key` on the row; + // - eleven call sites that hand `condition.id` to one of those four. + // + // The component's own exported `FilterBuilderCondition` declares it `string`, + // not `string | undefined`, and `addCondition` emits `crypto.randomUUID()`. + // + // ⭐ The narrowing refuses only what is ALREADY broken. Because a plain + // `z.object` STRIPS undeclared keys, an author who correctly wrote `id` had + // it discarded in silence: the document validated, the row rendered, and the + // row then had no individual identity. Both sides of every comparison listed + // above are `undefined`, and `undefined === undefined` is TRUE, so each + // helper matches EVERY id-less row rather than none: + // + // - `removeCondition(undefined)` deletes them ALL in one click — the + // clicked row included; only rows carrying a real id survive it; + // - `updateCondition`, `changeOperator` and `changeField` fan a single + // edit out across all of them; + // - `key={condition.id}` becomes `key={undefined}`, which React reads as + // NO key at all rather than as a duplicate one, so the rows reconcile by + // index and React warns about the missing key. + // + // ⛔ Not "matches none": the failure is EN BLOC, and it is the more severe + // reading — a row cannot be edited or removed on its own. Nothing that worked + // stops working; the state this refuses is accepted-and-discarded, the class + // objectui#6150 closed for `tree-view.title`. + id: z.string().describe('Row identity — matched by `removeCondition` / `updateCondition` / `changeOperator` / `changeField`, and the React key'), field: z.string().describe('Field name'), operator: FilterOperatorSchema.describe('Filter operator'), value: z.any().optional().describe('Filter value'),