diff --git a/.changeset/getviewsbyobject-expands-view-containers.md b/.changeset/getviewsbyobject-expands-view-containers.md new file mode 100644 index 0000000000..82647e0a25 --- /dev/null +++ b/.changeset/getviewsbyobject-expands-view-containers.md @@ -0,0 +1,32 @@ +--- +"@objectstack/metadata": patch +--- + +fix(metadata): `getViewsByObject()` expands aggregated view containers instead of answering empty (#13913) + +`MetadataManager.getViewsByObject()` reads `this.list('view')` — the manager's +own registry + loader store, which is a completely different store from the +`sys_metadata` rows the REST route (`GET /meta/view?object=`) reads through +`getMetaItems`. #13407 taught that route to expand a runtime-authored aggregated +`defineView` container inline; this exit never called it and had no equivalent +step, so a container the REST route now serves still answered **empty** here — +for every internal/SDK caller that uses this entry point rather than the route. + +Getting the container into the store was never enough on its own: the filter +also requires `viewKind`, and a container has none. Relaxing that requirement +would answer with the container itself as a view — the behaviour #7163 ruled +wrong — so the repair adds the container's **expansion**, whose items each carry +the `viewKind` + `object` pair this filter has always tested. The filter is +untouched; it reads the top-level `object` exactly as `ViewSchema.object` +declares. + +The expansion is registry-free and per-read, mirroring #13407's choice at the +other exit and for the same reason: the registry is process-wide, so a read must +not graft rows into it. Already-present names win, so a container whose expanded +ViewItems were registered by a source registrar (the ObjectQL boot loop, the +artifact/HMR loader) still answers with those registered, fully-enriched items +and gains nothing new. + +The object-derivation chain (`object` → `list.data.object` → `form.data.object` +→ the row's own `name`) now has one spelling for this package, in the new +`view-container-expansion.ts`, rather than a third private copy to fall behind. diff --git a/packages/metadata/src/metadata-manager-views-by-object-container.test.ts b/packages/metadata/src/metadata-manager-views-by-object-container.test.ts new file mode 100644 index 0000000000..7cde4e22f4 --- /dev/null +++ b/packages/metadata/src/metadata-manager-views-by-object-container.test.ts @@ -0,0 +1,281 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #13913 — `MetadataManager.getViewsByObject()` answered EMPTY for an + * aggregated view container living in this manager's own backing store. + * + * --------------------------------------------------------------------------- + * The defect, and why it survived #13407 + * --------------------------------------------------------------------------- + * There are two independent object-bound readers. The REST route + * (`GET /meta/view?object=`) reads through `ObjectStackProtocolImplementation. + * getMetaItems` over `sys_metadata` rows; #13407 taught THAT reader to expand a + * runtime-authored container inline. `getViewsByObject()` reads + * `this.list('view')` — `MetadataManager`'s OWN registry + loader store, a + * completely separate store — never calls `getMetaItems`, and had no equivalent + * step. So the very container #13407 made visible on the wire still answered + * empty through this entry point. + * + * The filter here was never the bug: it reads the top-level `object`, exactly + * as `ViewSchema.object` declares. Two conditions have to hold at once, and the + * second is the one that decides the shape of the fix — the filter ALSO + * requires `viewKind`, and a container has none. Getting the container into the + * store is therefore not enough; the container's EXPANSION has to be what the + * read sees. Relaxing the `viewKind` requirement instead would answer with the + * container itself as a view — the behaviour #7163 ruled wrong — which is why + * `answers with EXPANDED items and never the container itself` below is a pin + * against that regression and not a restatement of the fix. + * + * --------------------------------------------------------------------------- + * What is driven, and why it is NOT the REST route + * --------------------------------------------------------------------------- + * Every assertion goes through `manager.getViewsByObject(...)` itself. A pin + * written against `GET /meta/view?object=` would have gone green on `main` + * without touching this bug at all — #13929 already repaired that exit. + * + * The container fixture is the card's own shape: a top-level `object` and NO + * `list.data.object`. That combination is what #13407's corrected derivation + * chain exists for, and it is why `container.object` has to be consulted first. + * + * --------------------------------------------------------------------------- + * The controls, and what the first ablation corrected about them + * --------------------------------------------------------------------------- + * The two `CONTROL:` cases are green in BOTH directions on purpose: this change + * must not move what the exit already answered, so a case going red there would + * report a regression rather than this fix. Neither may therefore depend on the + * expansion existing. + * + * The dedupe case was originally written as ONE control asserting both the full + * answer AND the identity of the registered item. The first ablation falsified + * that: it went RED, because pre-fix the answer is the registered item alone. + * A control that goes red under ablation is not a control, so it is split here + * — the SET assertion is an ordinary case (`contributes only the names the + * store does not already hold`, red under ablation), and the both-directions + * half is the object-IDENTITY assertion, which holds either way. + * + * Reverse verification, direction re-predicted after that split and recorded in + * the PR body as measured: reverting `getViewsByObject` to its pre-#13913 body + * turns the four container/expansion cases RED and leaves all three of the + * remaining cases GREEN. + * + * `answers with EXPANDED items and never the container itself (#7163)` is one + * of those three, and it is green under that reversion only VACUOUSLY — the + * pre-fix answer is empty, so nothing can be a container. It is a guard against + * a DIFFERENT mutation, and it was ablated separately against exactly that one: + * dropping `v.viewKind &&` from the filter (the tempting one-line "fix", which + * makes the container answer as a view) turns it RED. Measured, so the guard is + * known to fire rather than assumed to. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { IDataDriver } from '@objectstack/spec/contracts'; +import { MetadataManager } from './metadata-manager.js'; +import { DatabaseLoader } from './loaders/database-loader.js'; + +// The manager logs on some paths; keep the run quiet and stable. +const logger = vi.hoisted(() => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), +})); + +vi.mock('@objectstack/core', async (orig) => ({ + ...((await orig()) as object), + createLogger: () => logger, +})); + +/** + * The card's container shape: the binding lives ONLY in the top-level `object` + * field. `list.data` deliberately carries no `object`, so the pre-#13407 + * two-deep derivation chain (`list.data.object` -> `form.data.object`) cannot + * find it and the fallback to the row's own name is not available either. + */ +const runtimeContainer = { + object: 'crm_lead', + list: { + label: 'All Leads', + type: 'grid', + data: { provider: 'object' }, + columns: [{ field: 'name' }, { field: 'company' }], + }, + listViews: { + pipeline: { + label: 'Lead Pipeline', + type: 'kanban', + data: { provider: 'object' }, + columns: ['name', 'company'], + kanban: { groupByField: 'status' }, + }, + }, + // Deliberately NOT keyed `default`: the container's default `list` already + // claims `.default`, and a form competing for that name is renamed + // to `…_2` with a diagnostic. That rename is real expansion behaviour, but + // pinning it here would make this file a test of collision handling rather + // than of the exit under repair. + formViews: { + edit: { type: 'simple', sections: [{ label: 'Info', fields: [{ field: 'name' }] }] }, + }, +}; + +/** What `runtimeContainer` expands to, sorted — the whole expected answer. */ +const EXPANDED = ['crm_lead.default', 'crm_lead.edit', 'crm_lead.pipeline']; + +/** An already-independent ViewItem — the shape a source registrar produces. */ +const independentViewItem = { + name: 'crm_lead.legacy', + object: 'crm_lead', + viewKind: 'list', + config: { type: 'grid', columns: [{ field: 'name' }] }, + order: 0, + scope: 'package', +}; + +const names = (items: unknown[]): string[] => + (items as { name: string }[]).map((i) => i.name).sort(); + +/** + * A `sys_metadata` store serving the rows it is handed. Minimal on purpose: + * `DatabaseLoader.loadMany()` only reaches `syncSchema` and `find`. + */ +function storeServing(rows: Record[]): IDataDriver { + return { + name: 'mock', + version: '1.0.0', + supports: {}, + connect: async (): Promise => {}, + disconnect: async (): Promise => {}, + syncSchema: async (): Promise => {}, + find: async (): Promise[]> => rows, + } as unknown as IDataDriver; +} + +function managerWithRegistryContainer(): MetadataManager { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + // How a container is keyed everywhere in this repo: under the bare object + // name, with no top-level `name` of its own. + manager.registerInMemory('view', 'crm_lead', runtimeContainer); + return manager; +} + +/** + * What every SOURCE registrar leaves behind: the container under the bare + * object key, PLUS each expanded ViewItem under `.`. Only one + * of the three is registered here, so both halves are observable in one store — + * the registered name must not be re-minted, and the two absent ones must be. + */ +function managerWithContainerAndRegisteredItem(): { + manager: MetadataManager; + registeredPipeline: Record; +} { + const manager = managerWithRegistryContainer(); + const registeredPipeline = { + name: 'crm_lead.pipeline', + object: 'crm_lead', + viewKind: 'list', + config: { type: 'kanban' }, + order: 0, + scope: 'package', + _packageId: 'crm', + }; + manager.registerInMemory('view', 'crm_lead.pipeline', registeredPipeline); + return { manager, registeredPipeline }; +} + +describe('#13913 getViewsByObject() expands aggregated view containers', () => { + it('answers with the container EXPANSION, not empty', async () => { + const manager = managerWithRegistryContainer(); + + const views = await manager.getViewsByObject('crm_lead'); + + // Pre-fix this was `[]`: the container carries no `viewKind`, so the filter + // rejected the only row in the store. + expect(views.length).toBeGreaterThan(0); + expect(names(views)).toEqual(EXPANDED); + }); + + it('answers with EXPANDED items and never the container itself (#7163)', async () => { + const manager = managerWithRegistryContainer(); + + const views = (await manager.getViewsByObject('crm_lead')) as Record[]; + + // Every answer is an independent ViewItem bound to the requested object. + for (const v of views) { + expect(v.viewKind === 'list' || v.viewKind === 'form').toBe(true); + expect(v.object).toBe('crm_lead'); + } + // The container is keyed `crm_lead` and has neither `viewKind` nor a + // top-level `name`; loosening the filter to admit it is the regression this + // pins against. + expect(names(views)).not.toContain('crm_lead'); + expect(views.some((v) => v.list !== undefined || v.listViews !== undefined)).toBe(false); + }); + + it('derives the binding from the top-level `object` when `list.data.object` is absent', async () => { + const manager = managerWithRegistryContainer(); + + // Nothing binds to the container's registry KEY by accident: ask for an + // object the container does not name and the answer stays empty. + expect(await manager.getViewsByObject('crm_account')).toEqual([]); + expect(names(await manager.getViewsByObject('crm_lead'))).toEqual(EXPANDED); + }); + + it('reaches a container that arrived through a LOADER, not only the registry', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + manager.registerLoader( + new DatabaseLoader({ + driver: storeServing([ + { + id: 'r1', + name: 'crm_lead', + type: 'view', + // A runtime-authored row: the stored body is the container itself. + metadata: JSON.stringify({ name: 'crm_lead', ...runtimeContainer }), + }, + ]), + cache: { enabled: false }, + }), + ); + + expect(names(await manager.getViewsByObject('crm_lead'))).toEqual(EXPANDED); + }); + + it('contributes only the names the store does not already hold', async () => { + const { manager } = managerWithContainerAndRegisteredItem(); + + const views = (await manager.getViewsByObject('crm_lead')) as Record[]; + + // One entry per named view — the registered `crm_lead.pipeline` is NOT + // joined by a second, freshly-expanded copy of itself. + expect(names(views)).toEqual(EXPANDED); + expect(views.filter((v) => v.name === 'crm_lead.pipeline')).toHaveLength(1); + }); + + // ------------------------------------------------------------------ + // Controls — green in BOTH directions, and measured so. + // + // These pin what this change must NOT move. A case going red here would be + // reporting a regression, not this fix, so neither may depend on the + // expansion existing. + // ------------------------------------------------------------------ + + it('CONTROL: a non-container ViewItem still resolves unchanged', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + manager.registerInMemory('view', 'crm_lead.legacy', independentViewItem); + + const views = await manager.getViewsByObject('crm_lead'); + + expect(views).toEqual([independentViewItem]); + }); + + it('CONTROL: a registered expanded item is returned BY IDENTITY, never shadowed', async () => { + const { manager, registeredPipeline } = managerWithContainerAndRegisteredItem(); + + const views = (await manager.getViewsByObject('crm_lead')) as Record[]; + + // Object identity, not deep equality: the store's own fully-enriched copy + // (it carries `_packageId`) is what comes back, not an expansion-minted + // stand-in that happens to share a name. + expect(views.find((v) => v.name === 'crm_lead.pipeline')).toBe(registeredPipeline); + }); +}); diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index 60c509d0a7..14ffa5d9a9 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -78,6 +78,9 @@ import { EndpointMatcher } from './endpoint-matcher.js'; // [#5309] The stored ENVELOPE / authored BODY split — peeled before any spec // schema parses a stored row. See `stored-envelope.ts`. import { peelStoredEnvelope } from './stored-envelope.js'; +// [#13913] Registry-free expansion of an aggregated view container reached +// through THIS manager's own store — see `getViewsByObject`. +import { expandRuntimeViewContainer } from './view-container-expansion.js'; import type { ApiEndpointMatch } from '@objectstack/spec/contracts'; /** @@ -1579,19 +1582,65 @@ export class MetadataManager implements IMetadataService { * Runtime-authored `shared` / `personal` views (`sys_view_definition`) are * merged in by the REST layer; this method returns the `package` layer that * was registered from source. + * + * ## [#13913] Aggregated containers are expanded inline, per read + * + * `this.list('view')` is `MetadataManager`'s OWN loader-based store — the + * in-memory registry plus every registered loader — and is a completely + * different store from the `sys_metadata` rows `getMetaItems` reads. #13407 + * taught `getMetaItems` to expand a runtime-authored aggregated container + * inline; this exit never called it and had no equivalent step, so a + * container that `GET /meta/view?object=` now serves still answered **empty** + * here. + * + * Merely getting the container into the store would not have helped: the + * filter also requires `viewKind`, and a container has none. Loosening that + * requirement is NOT the repair — it would answer with the container itself + * as a view, the behaviour #7163 ruled wrong — so what is added below is the + * container's **expansion**, whose items each carry the `viewKind` + `object` + * pair this filter has always tested. The filter itself is untouched: it + * reads the top-level `object`, exactly as `ViewSchema.object` declares. + * + * Registry-free and per-read, mirroring #13407's choice at the other exit and + * for the same reason — the registry is process-wide, so a read must not + * graft rows into it (see `view-container-expansion.ts`'s header, which also + * records why the protocol's copy of this logic cannot be imported). + * + * Already-present items win: an expansion contributes only names the store + * does not already hold, so a container whose expanded ViewItems were + * registered by a source registrar (the ObjectQL boot loop, the artifact/HMR + * loader) still answers with those registered, fully-enriched items and this + * step adds nothing. */ async getViewsByObject(object: string): Promise { const views = await this.list('view'); - return views - .filter( - (v: any) => - v && typeof v === 'object' && v.viewKind && v.object === object, - ) - .sort( - (a: any, b: any) => - (a.order ?? 0) - (b.order ?? 0) || - String(a.name).localeCompare(String(b.name)), - ); + const matches = views.filter( + (v: any) => + v && typeof v === 'object' && v.viewKind && v.object === object, + ); + + // Every name the store already holds — containers included, though a + // container is keyed by the bare `` and can never collide with the + // `.` names an expansion mints. + const known = new Set(); + for (const v of views as any[]) { + if (v && typeof v === 'object' && typeof v.name === 'string') known.add(v.name); + } + + for (const v of views) { + for (const item of expandRuntimeViewContainer(v)) { + if (!item.viewKind || item.object !== object) continue; + if (known.has(item.name)) continue; + known.add(item.name); + matches.push(item); + } + } + + return matches.sort( + (a: any, b: any) => + (a.order ?? 0) - (b.order ?? 0) || + String(a.name).localeCompare(String(b.name)), + ); } /** diff --git a/packages/metadata/src/view-container-expansion.ts b/packages/metadata/src/view-container-expansion.ts new file mode 100644 index 0000000000..fcb31c2c98 --- /dev/null +++ b/packages/metadata/src/view-container-expansion.ts @@ -0,0 +1,108 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#13913] Registry-free expansion of an aggregated `defineView` container + * that reached `MetadataManager`'s OWN backing store. + * + * --------------------------------------------------------------------------- + * Why this lives in `packages/metadata` and does NOT import the protocol's copy + * --------------------------------------------------------------------------- + * `packages/metadata-protocol` grew `expandRuntimeViewContainer` (#13407) for + * `getMetaItems`' own inline expansion, and the obvious move would be to share + * it. **The dependency edge forbids it**: `@objectstack/metadata-protocol` + * declares `@objectstack/metadata` as a dependency, so an import in this + * direction inverts an existing edge and closes a cycle. Promoting the + * protocol's private method to a public export would not help either — it + * would widen that package's surface *and* still be unreachable from here. + * + * The reusable substance is therefore taken from where it already is: the + * canonical expansion primitives (`isAggregatedViewContainer`, + * `expandViewContainer`) live in `@objectstack/spec`, one level BELOW both + * packages, and this package already imports them (`plugin.ts`). Nothing is + * duplicated except the ~6-line object-derivation chain — which is exactly the + * part that has silently drifted three ways (the ObjectQL boot loop keys off + * the registration name, `plugin.ts` walks two levels, and `protocol.ts` walks + * four since #13407). {@link deriveViewContainerObject} is the one spelling of + * it for this package, so the drift has a single place to be repaired rather + * than a third private copy to fall behind. + * + * --------------------------------------------------------------------------- + * Registry-free, on purpose + * --------------------------------------------------------------------------- + * Nothing here REGISTERS. `protocol.ts`'s own header records why its + * registry-MUTATING path (`hydrateExpandedViewItems`) could not simply be + * widened — it is gated off for every org-scoped row (ADR-0005: the registry + * is shared by every org a kernel serves) — and why #13407's actual repair was + * a separate, registry-free, per-request expansion. The same reasoning applies + * one exit over: `MetadataManager`'s registry is process-wide too, so the + * repair for {@link MetadataManager.getViewsByObject} is a pure function over + * what that one read already holds. + */ + +import { + expandViewContainer, + isAggregatedViewContainer, + type ExpandedViewItem, +} from '@objectstack/spec'; +import { applyProtection } from '@objectstack/spec/shared'; + +/** + * Which object an aggregated view container binds to. + * + * The container's OWN top-level `object` field — `ViewSchema.object`, + * documented there as "how a stack-level `views: [...]` entry says which object + * its views belong to; read by `getViewsByObject()` / `GET /meta/view?object=`" + * — is the authorial, explicit signal and is consulted FIRST (#13407). The + * three-deep fallback below it is kept unchanged for every container written + * before that field was read here: `list.data.object`, then `form.data.object`, + * then the row's own `name` — which is the bound object only by convention, and + * is why a container that set the top-level field but not `list.data.object` + * used to bind under the wrong key or not at all. + * + * Returns `undefined` when no binding can be derived; every caller treats that + * as "no expansion" rather than an error. + */ +export function deriveViewContainerObject(container: unknown): string | undefined { + if (!container || typeof container !== 'object') return undefined; + const c = container as Record; + const own = typeof c.object === 'string' && c.object ? c.object : undefined; + const byName = typeof c.name === 'string' && c.name ? c.name : undefined; + return own ?? c?.list?.data?.object ?? c?.form?.data?.object ?? byName; +} + +/** + * Expand an aggregated `defineView` container into the same independent + * ViewItems the source registrars produce — object binding and package + * provenance applied, but **nothing registered**. + * + * Returns `[]` (never throws) for a body that is not a container and for a + * container with no derivable object binding — the two "no expansion" outcomes + * callers already decide on independently. Every returned item carries a + * `viewKind`, which is what makes it visible to the object-bound filters; the + * container itself is never returned, because a container surfaced as a view is + * the behaviour #7163 ruled wrong. + * + * The items are fresh objects from `expandViewContainer`, so stamping package + * coords on them cannot mutate the stored container. + */ +export function expandRuntimeViewContainer(data: unknown): ExpandedViewItem[] { + if (!isAggregatedViewContainer(data)) return []; + const container = data as Record; + const viewObject = deriveViewContainerObject(container); + if (!viewObject) return []; + + const out: ExpandedViewItem[] = []; + for (const vi of expandViewContainer(viewObject, container)) { + // Carry the container's package provenance onto each expanded item so the + // package-disable filter and ADR-0048 artifact scoping judge them by the + // same owner the container has. `applyProtection` stamps nothing when the + // container has no package coords, which keeps runtime/DB-authored items + // free of an unexpected `_provenance` (its own documented boundary). + applyProtection(vi as unknown as Record, { + packageId: container._packageId, + packageVersion: container._packageVersion, + }); + out.push(vi); + } + return out; +}