diff --git a/.changeset/7839-tree-arm-own-reference.md b/.changeset/7839-tree-arm-own-reference.md new file mode 100644 index 0000000000..c91d9b08c5 --- /dev/null +++ b/.changeset/7839-tree-arm-own-reference.md @@ -0,0 +1,29 @@ +--- +'@object-ui/plugin-tree': patch +--- + +`ObjectTree`'s parent-pointer auto-detection accepts a `tree` field only when it is +this object's own (objectui#7839, objectstack#14892 follow-up). + +A hierarchy is parent/child **within one object**, so `@objectstack/spec` refuses a +`tree` field whose `reference` names any other object (`refuseForeignTreeReference`); +`reference` stays optional on a `tree`, where it is a redundant self-annotation. +`detectParentField` returned the first `type: 'tree'` field whatever its `reference` +said, so a foreign-shaped one was silently picked as the parent pointer and the forest +was grouped on a pointer into a table it does not point at. It now mirrors the spec's +own kernel predicate `hasDetectableParentField` term for term — accept when `reference` +is absent, or when it equals the bound `objectName` — and the `objectName` guard comes +with it: an object whose name we do not know cannot be self-referenced. + +Skipping rather than returning also removes a masking bug the old early-return hid: a +foreign `tree` declared before a self-referencing `lookup` used to win by position and +the lookup never got its turn. The `lookup` / `master_detail` arm is otherwise +unchanged. + +Reachability is narrow and stated rather than oversold: the refused shape does not +survive `ObjectSchema.parse` on a spec carrying that rule, so it is unreachable from +parsed metadata and reachable from a hand-built one — `getObjectSchema` is a required +member of the published `DataSource` interface, so a third-party implementation reaches +this reader raw. Note also that the renderer tightens **ahead** of the spec copy this +repo installs: `@objectstack/spec@17.2.0` still accepts the foreign shape at parse +(measured), so until that pin moves this function is the only door. diff --git a/packages/core/src/utils/__tests__/expand-fields.test.ts b/packages/core/src/utils/__tests__/expand-fields.test.ts index 0d00686fa9..a4bb0e0748 100644 --- a/packages/core/src/utils/__tests__/expand-fields.test.ts +++ b/packages/core/src/utils/__tests__/expand-fields.test.ts @@ -131,7 +131,14 @@ describe('buildExpandFields', () => { name: { type: 'text', label: 'Name' }, f_lookup: { type: 'lookup', label: 'Account', reference: 'showcase_account' }, f_master_detail: { type: 'master_detail', label: 'Project', reference: 'showcase_project' }, - f_tree: { type: 'tree', label: 'Category', reference: 'showcase_category' }, + // No `reference`: a `tree` field's `reference` is OPTIONAL and, when + // present, must name the DECLARING object — a hierarchy is parent/child + // within one object (objectstack#14892, `refuseForeignTreeReference`). + // This map declares no object, so there is no name to point at, and + // `buildExpandFields` reads `type` and never the target anyway. It used + // to carry a foreign one copied from the showcase zoo, which is exactly + // how it went stale when upstream corrected the zoo (objectui#7839). + f_tree: { type: 'tree', label: 'Category' }, f_user: { type: 'user', label: 'Assignee', reference: 'sys_user' }, status: { type: 'select', label: 'Status' }, cover: { type: 'image', label: 'Cover' }, diff --git a/packages/core/src/utils/__tests__/predicate-record.test.ts b/packages/core/src/utils/__tests__/predicate-record.test.ts index 20247a813f..616698e952 100644 --- a/packages/core/src/utils/__tests__/predicate-record.test.ts +++ b/packages/core/src/utils/__tests__/predicate-record.test.ts @@ -26,7 +26,11 @@ const FIELDS = { account: { type: 'lookup', reference: 'showcase_account' }, accounts: { type: 'lookup', reference: 'showcase_account', multiple: true }, owner: { type: 'user' }, - parent: { type: 'tree', reference: 'showcase_category' }, + // No `reference` — optional on a `tree`, and must name the declaring + // object when present (objectstack#14892). This map declares none, and + // `toPredicateRecord` collapses by declared TYPE, never by target + // (objectui#7839). + parent: { type: 'tree' }, config: { type: 'json' }, }; diff --git a/packages/plugin-dashboard/src/__tests__/expandableFamily.identity-5692.test.ts b/packages/plugin-dashboard/src/__tests__/expandableFamily.identity-5692.test.ts index ac44e58af3..148b03a9dd 100644 --- a/packages/plugin-dashboard/src/__tests__/expandableFamily.identity-5692.test.ts +++ b/packages/plugin-dashboard/src/__tests__/expandableFamily.identity-5692.test.ts @@ -110,7 +110,10 @@ const objectSchema = () => ({ account: { type: 'lookup', reference: 'accounts' }, parent_case: { type: 'master_detail', reference: 'cases' }, assignee: { type: 'user' }, - parent_node: { type: 'tree', reference: 'nodes' }, + // No `reference` — optional on a `tree`, and must name the declaring + // object when present (objectstack#14892). This schema declares no + // `name`, and every rule under test reads `type` (objectui#7839). + parent_node: { type: 'tree' }, legacy_ref: { type: 'reference', reference: 'accounts' }, }, }); diff --git a/packages/plugin-detail/src/__tests__/expandableFamily.identity-5874.test.tsx b/packages/plugin-detail/src/__tests__/expandableFamily.identity-5874.test.tsx index 7fc151c81d..f75b871c0c 100644 --- a/packages/plugin-detail/src/__tests__/expandableFamily.identity-5874.test.tsx +++ b/packages/plugin-detail/src/__tests__/expandableFamily.identity-5874.test.tsx @@ -116,7 +116,13 @@ const objectSchema = { account: { type: 'lookup', label: 'Account', reference_to: 'accounts' }, parent_deal: { type: 'master_detail', label: 'Parent', reference_to: 'deals' }, assignee: { type: 'user', label: 'Assignee' }, - parent_node: { type: 'tree', label: 'Parent node', reference_to: 'deals' }, + // No target key. It carried the retired snake_case one, which + // `FieldSchema` refuses BY NAME — so the line asserted nothing, and + // renaming it would have turned a refused key into an accepted + // self-annotation this fixture never made. On a `tree` the key is + // optional and must be the declaring object when present + // (objectstack#14892); the readonly rule reads `type` (objectui#7839). + parent_node: { type: 'tree', label: 'Parent node' }, stage: { type: 'select', label: 'Stage' }, notes: { type: 'textarea', label: 'Notes' }, }, diff --git a/packages/plugin-tree/src/ObjectTree.referenceArms-6837.test.tsx b/packages/plugin-tree/src/ObjectTree.referenceArms-6837.test.tsx index fa63beab39..37f0355cf9 100644 --- a/packages/plugin-tree/src/ObjectTree.referenceArms-6837.test.tsx +++ b/packages/plugin-tree/src/ObjectTree.referenceArms-6837.test.tsx @@ -160,8 +160,13 @@ const RECORDS = [ /** * Every probe is a self-referencing `lookup`, so only the target SPELLING - * varies. ⚠️ `lookup` and not `tree`: `detectParentField` returns a `tree` field - * before it ever reads a target, which would make the chain unobservable. + * varies. ⚠️ `lookup` and not `tree`, and objectui#7839 made that choice + * STRICTLY more load-bearing rather than retiring it. The `tree` arm now reads + * a target too (absent, or this object's own name), but it reads only the ONE + * declared spelling: a `tree` carrying just a refused spelling has, to that + * arm, no `reference` AT ALL, so it is accepted as the parent pointer and every + * refusal below would invert to green. On a `lookup` the same def is simply + * not a self-reference, which is what keeps the chain observable. */ const FIELD_DEFS: Record> = { // Live arm — the ONE spelling the protocol declares. objectui#6837 half 2 diff --git a/packages/plugin-tree/src/ObjectTree.treeArmOwnReference-7839.test.tsx b/packages/plugin-tree/src/ObjectTree.treeArmOwnReference-7839.test.tsx new file mode 100644 index 0000000000..7bf2215814 --- /dev/null +++ b/packages/plugin-tree/src/ObjectTree.treeArmOwnReference-7839.test.tsx @@ -0,0 +1,256 @@ +/** + * 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#7839 — `detectParentField`'s `tree` arm accepts a `tree` field only + * when its `reference` is ABSENT or names THIS object, mirroring the rule the + * parse door already enforces. + * + * Before: `if (def?.type === 'tree') return key;` + * After: the same, gated on `ref === undefined || ref === objectName`. + * + * Form copied from `ObjectTree.referenceArms-6837.test.tsx` (the sibling pin on + * the same function). ⛔ Do not invent a second form. + * + * ## 1. The contract being mirrored — read, not paraphrased + * + * `@objectstack/spec`'s `refuseForeignTreeReference` + * (`packages/spec/src/data/object.zod.ts`, objectstack#14892 / #15979) skips a + * field unless it is foreign-shaped: + * + * if (type !== 'tree' || reference === undefined || reference === ownName) continue; + * + * So the accept set is exactly {absent, own name} — `reference` stays OPTIONAL + * on a `tree` (under this rule it is a redundant self-annotation), and only a + * value naming ANOTHER object is refused. The spec's kernel predicate + * `hasDetectableParentField` (`packages/spec/src/kernel/ + * functional-completeness.ts`) reads the identical rule and carries the + * `ownName` guard as `own !== undefined && def.reference === own`; its docblock + * named THIS reader as the one place still out of step ("On that arm this + * predicate is STRICTER than objectui's `detectParentField` … tightening the + * renderer is an objectui follow-up"). This file is that follow-up's pin. + * + * ## 2. ⚠️ What this pin deliberately does NOT assert — measured, not assumed + * + * The obvious extra control would be a live two-directional probe that + * `ObjectSchema.safeParse` REFUSES the foreign shape, proving the renderer + * mirrors a running contract rather than a remembered one. It is not here + * because it would be RED, and the reason is worth recording rather than + * discovering twice. Probed on the copy this branch actually installs + * (`@objectstack/spec@17.2.0`, resolved through `packages/plugin-tree`): + * + * | shape on an object named `business_unit` | 17.2.0 verdict | + * |-------------------------------------------------|----------------| + * | `{ type: 'tree' }` | ACCEPT | + * | `{ type: 'tree', reference: 'business_unit' }` | ACCEPT | + * | `{ type: 'tree', reference: 'other_object' }` | **ACCEPT** | + * + * `refuseForeignTreeReference` landed on objectstack `main` after 17.2.0 was + * cut, and objectui tracks the spec by npm semver, not by SHA. So the renderer + * is tightening AHEAD of its installed parse door — which is the whole reason + * the tightening is worth doing at the renderer at all: until the pin moves, + * this function is the only thing standing between a foreign-shaped `tree` and + * a forest grouped on a pointer into a table it does not point at. The one + * spec-anchored control below therefore pins only the half 17.2.0 already + * answers, and that half is the one the live arms rest on: `reference` is + * OPTIONAL on a `tree`, so the accept-when-absent arm is not blessing a shape + * the schema rejects. + * + * ⛔ When the pin moves, do not delete that control — add the refusal leg + * beside it. A green "absence is accepted" is what stops a future reading of + * this rule from drifting to "a `tree` must self-annotate". + * + * ## 3. Reachability, stated so the pin is not mistaken for a live bug fix + * + * From PARSED metadata the refused shape is unreachable on a spec at or past + * `refuseForeignTreeReference`. It is reachable from a HAND-BUILT schema: + * `getObjectSchema` is a required member of the published `DataSource` + * interface and `useSettledSchema` calls it on the generic `dataSource`, so a + * third-party implementation reaches this reader raw — the same + * door the sibling pin names. Defence in depth, not a hot path. + * + * ## 4. Ablation direction, predicted before running + * + * Restore `if (def?.type === 'tree') return key;` on the committed tree and the + * two refusal cases below go RED (the foreign `tree` is picked again, so the + * masked lookup never gets its turn) while every live arm and every `lookup` + * control stays GREEN — that contrast is what makes the controls controls + * rather than duplicates of the pins. MODULE RESOLUTION: this file imports the + * component by RELATIVE SOURCE PATH (`./ObjectTree`), so both legs resolve to + * SOURCE — no package `exports` hop, no `dist`, and therefore NO REBUILD LEG to + * get wrong. + */ +import React from 'react'; +import { render, waitFor, cleanup } from '@testing-library/react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { FieldSchema } from '@objectstack/spec/data'; +import { ObjectTree } from './ObjectTree'; + +afterEach(cleanup); + +/** The object every probe below declares itself to be. */ +const OWN = 'business_unit'; +const FOREIGN = 'other_object'; + +/** + * A two-node self-referencing hierarchy. `Engineering` nests under `Acme` ONLY + * if the parent pointer is auto-detected — which is the whole observable here, + * because the schema below deliberately omits `parentField`. + */ +const RECORDS = [ + { id: '1', name: 'Acme', parent_id: null }, + { id: '2', name: 'Engineering', parent_id: '1' }, +]; + +/** + * The field maps under test. Every one of them still declares `parent_id` as + * an EXPANDABLE type, so the settle signal in `mount` is independent of which + * arm the detector takes — a refusal probe cannot be satisfied by a schema + * that never arrived. + */ +const FIELD_MAPS: Record> = { + // Live arms — the accept set the spec declares. + tree_no_reference: { name: { type: 'text' }, parent_id: { type: 'tree' } }, + tree_own_reference: { name: { type: 'text' }, parent_id: { type: 'tree', reference: OWN } }, + // The refusal — the one shape this card removed from the accept set. + tree_foreign_reference: { name: { type: 'text' }, parent_id: { type: 'tree', reference: FOREIGN } }, + // The masking case: before the tightening the foreign `tree` won BY POSITION + // and the self-referencing lookup after it never got its turn. + foreign_tree_masking_a_self_lookup: { + name: { type: 'text' }, + decoy: { type: 'tree', reference: FOREIGN }, + parent_id: { type: 'lookup', reference: OWN }, + }, + // `lookup` controls — this arm did not move, and must stay green through + // BOTH ablation legs. + lookup_own_reference: { name: { type: 'text' }, parent_id: { type: 'lookup', reference: OWN } }, + lookup_foreign_reference: { name: { type: 'text' }, parent_id: { type: 'lookup', reference: FOREIGN } }, +}; + +function makeDataSource(fields: Record) { + return { + getObjectSchema: vi.fn().mockResolvedValue({ name: OWN, fields }), + find: vi.fn(async () => RECORDS), + } as any; +} + +/** No `parentField` — auto-detection is what is under test. */ +const TREE_SCHEMA = { + type: 'object-tree', + objectName: OWN, + labelField: 'name', + fields: ['name'], +} as any; + +/** + * Mount over one field map and wait for the SCHEMA-DEPENDENT commit to happen. + * + * The settle signal is deliberately ARM-independent: the record query is gated + * on the settled schema and carries `$expand`, and `buildExpandFields` decides + * that from the field's `type` alone — the `reference` target is irrelevant to + * it. So a `find` carrying `$expand: ['parent_id']` proves the component + * consumed this schema for the refusal probes just as much as for the live + * ones, which is what stops a refusal from being satisfiable by a schema that + * never arrived. + */ +async function mount(fields: Record) { + const ds = makeDataSource(fields); + const view = render(); + await waitFor(() => + expect( + ds.find.mock.calls.some((c: any[]) => c[1]?.$expand?.includes('parent_id')), + ).toBe(true), + ); + await waitFor(() => expect(view.getAllByTestId('object-tree-row').length).toBe(2)); + return { ds, view }; +} + +/** The rendered depth of one node — 1 once the parent pointer is detected, 0 while it is not. */ +function depthOf(view: ReturnType, label: string) { + const row = view + .getAllByTestId('object-tree-row') + .find((r) => r.textContent?.includes(label)); + return row?.getAttribute('data-depth'); +} + +describe('ObjectTree auto-detects a `tree` parent pointer only when it is this object\'s own (objectui#7839)', () => { + describe('live arms — the accept set the spec declares (without these, a detector that stopped detecting anything would pass the refusal too)', () => { + it('accepts a `tree` with NO `reference` — the spec keeps the key optional', async () => { + const { view } = await mount(FIELD_MAPS.tree_no_reference); + expect(depthOf(view, 'Engineering')).toBe('1'); + }); + + it('accepts a `tree` whose `reference` names THIS object', async () => { + const { view } = await mount(FIELD_MAPS.tree_own_reference); + expect(depthOf(view, 'Engineering')).toBe('1'); + }); + + it('the root stays a root either way, so depth is reading the hierarchy and not the row order', async () => { + const { view } = await mount(FIELD_MAPS.tree_own_reference); + expect(depthOf(view, 'Acme')).toBe('0'); + }); + }); + + describe('the refusal — a foreign-referencing `tree` is not a parent pointer', () => { + it('does NOT pick a `tree` whose `reference` names another object', async () => { + const { view } = await mount(FIELD_MAPS.tree_foreign_reference); + expect(depthOf(view, 'Engineering')).toBe('0'); + }); + + it('and still renders every record as a root rather than rendering nothing', async () => { + // Guards the refusal above against the degenerate pass: a tree that + // rendered no rows at all would also report no nested child. + const { view } = await mount(FIELD_MAPS.tree_foreign_reference); + expect(view.getAllByTestId('object-tree-row').length).toBe(2); + expect(depthOf(view, 'Acme')).toBe('0'); + expect(view.getByText('Engineering')).toBeTruthy(); + }); + + it('and no longer MASKS a self-referencing lookup declared after it', async () => { + // The behavioural half a pure "is it skipped?" assertion cannot see: + // skipping is only correct if the loop then keeps looking. Before the + // tightening the foreign `tree` won by position and this rendered flat. + const { view } = await mount(FIELD_MAPS.foreign_tree_masking_a_self_lookup); + expect(depthOf(view, 'Engineering')).toBe('1'); + }); + }); + + describe('the `lookup` arm did not move — regression control', () => { + // These must stay green through BOTH ablation legs. If they move, the + // tightening took the whole detector with it and the pins above are + // reporting on rubble rather than on a narrowed arm. + it('still detects a self-referencing `lookup`', async () => { + const { view } = await mount(FIELD_MAPS.lookup_own_reference); + expect(depthOf(view, 'Engineering')).toBe('1'); + }); + + it('still refuses a foreign `lookup`', async () => { + const { view } = await mount(FIELD_MAPS.lookup_foreign_reference); + expect(depthOf(view, 'Engineering')).toBe('0'); + }); + }); + + describe('the accept-when-absent arm is not blessing a shape the schema rejects', () => { + // The only spec-anchored control 17.2.0 can answer today; see §2 of the + // module docblock for the leg that is deliberately absent and why. + it('`FieldSchema` accepts a `tree` field with no `reference`', () => { + const parsed = FieldSchema.safeParse({ name: 'parent_id', type: 'tree', label: 'Parent' }); + expect(parsed.success).toBe(true); + }); + + it('and accepts one that self-annotates, so both live arms are spec-legal shapes', () => { + const parsed = FieldSchema.safeParse({ + name: 'parent_id', + type: 'tree', + label: 'Parent', + reference: OWN, + }); + expect(parsed.success).toBe(true); + }); + }); +}); diff --git a/packages/plugin-tree/src/ObjectTree.tsx b/packages/plugin-tree/src/ObjectTree.tsx index f41d74b2ee..35367b1aff 100644 --- a/packages/plugin-tree/src/ObjectTree.tsx +++ b/packages/plugin-tree/src/ObjectTree.tsx @@ -122,15 +122,39 @@ function getTreeConfig(schema: any): TreeConfig { /** * Auto-detect the single-parent pointer field from the object schema: - * the first field declared as `tree`, or a lookup/master_detail whose - * reference points back at this same object. + * a field declared `tree` whose `reference` is absent or names THIS object, or + * a lookup/master_detail whose reference points back at this same object. + * + * ## The `tree` arm mirrors the parse door, it does not out-guess it + * + * A hierarchy is parent/child WITHIN one object, so `@objectstack/spec` refuses + * a `tree` field whose `reference` names any other object + * (`refuseForeignTreeReference` in `packages/spec/src/data/object.zod.ts`, + * applied at both doors that carry a field map — `ObjectSchema` with `name` as + * the own name and `ObjectExtensionSchema` with `extend`). `reference` stays + * OPTIONAL on a `tree` — under that rule it is a redundant self-annotation — so + * ABSENCE is accepted and only a FOREIGN value is refused. The spec's own + * kernel predicate reads the identical rule (`hasDetectableParentField` in + * `packages/spec/src/kernel/functional-completeness.ts`), and its docblock + * named this reader as the one place still out of step; objectui#7839 closes + * that gap. Mirrored here term for term, including the `objectName` guard: an + * object whose own name we do not know cannot be self-referenced, so a `tree` + * that DOES name a target is not matchable against a name that is not there. + * + * ⛔ Not `??`-style tolerance in either direction. A foreign-referencing `tree` + * is skipped rather than returned, so it can no longer mask a self-referencing + * lookup declared after it — before this it won by position, and the tree view + * then grouped records under a pointer into a table it does not point at. + * Unreachable from parsed metadata (the parse door refuses the shape) and + * reachable from a hand-built schema, which a third-party `DataSource` may + * hand straight to `useSettledSchema`. Pinned in + * `ObjectTree.treeArmOwnReference-7839.test.tsx`. */ function detectParentField(objectSchema: any, objectName?: string): string | undefined { const fields = objectSchema?.fields; if (!fields || typeof fields !== 'object') return undefined; let firstSelfRef: string | undefined; for (const [key, def] of Object.entries(fields)) { - if (def?.type === 'tree') return key; // ONE arm: `reference`, the only target spelling the protocol declares. // `FieldSchema` refuses `reference_to` / `referenceTo` / `target` by name. // `referenceTo` went in objectui#6837 slice 2, `reference_to` in half 2 @@ -139,6 +163,11 @@ function detectParentField(objectSchema: any, objectName?: string): string | und // canonicalised once at the ingestion choke point, never here. Pinned in // `ObjectTree.referenceArms-6837.test.tsx`. const ref = def?.reference; + // A `tree` still wins over a lookup found earlier — the precedence is + // unchanged; what changed is that it must first BE a self-reference. + if (def?.type === 'tree' && (ref === undefined || (!!objectName && ref === objectName))) { + return key; + } if ( !firstSelfRef && (def?.type === 'lookup' || def?.type === 'master_detail') &&