diff --git a/.changeset/registry-merge-object-definitions-docblock-closed-set.md b/.changeset/registry-merge-object-definitions-docblock-closed-set.md new file mode 100644 index 0000000000..d8509261d3 --- /dev/null +++ b/.changeset/registry-merge-object-definitions-docblock-closed-set.md @@ -0,0 +1,51 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): correct `mergeObjectDefinitions`'s docblock to the real, closed merge set (#12680) + +The docblock on `mergeObjectDefinitions` (`packages/objectql/src/registry.ts`) +said: + +> Fields are merged additively. **Other props: later value wins.** + +The implementation has never done the second half. It merges exactly: +`fields` (additively), `validations` (additively), `indexes` (additively), +and the three guarded scalars `label` / `pluralLabel` / `description` +(last-writer-wins, subject to the `tenantAuthored` yield rule). **Every +other top-level prop an `extend` contributor carries is silently +discarded** — `merged` starts as `{ ...base }` and nothing outside that list +is ever copied onto it. + +This is a **docs-only correction plus a regression pin — zero runtime +behaviour changed.** `mergeObjectDefinitions` is not exported; nothing about +what it does was touched, only what the comment above it claims. Shipped as +`patch` rather than omitted because the file's behaviour is now backed by an +enforced pin (see below) where before it was backed by nothing but an +inaccurate comment — that is a real (if internal-only) improvement to the +package worth a version bump, and this repo's convention reserves "no +changeset" for changes with no user-facing effect of any kind, not for +"no runtime diff." There is no public API surface to widen or narrow (the +function is module-private), so there is nothing here for `check:*` gates +that watch exported shapes to see. + +Why this matters: an author reading the old docblock and shipping an +`objectExtensions` entry carrying, say, `tenancy: { enabled: false }` would +have gotten a **silent no-op** on a security-relevant key — no error, no +warning, the base's existing value simply wins as if the extension had never +named the key. That near-miss is the reason this card exists (found while +resolving cloud#1653's investigation into exactly that override path). The +corrected docblock says the discard out loud; a new pin +(`registry-object-extension-nonenumerated-prop-discard.test.ts`) hands +`mergeObjectDefinitions` (via the public `SchemaRegistry` API) an `extend` +contributor carrying a non-enumerated top-level prop and asserts the merged +result does not carry it, with a guarded scalar as a positive control in the +same fold. `icon` is the pin's fixture — a real, spec-legal, security-neutral +top-level prop — deliberately **not** `tenancy`, so the pin does not read as +license to special-case that key elsewhere. + +⛔ Out of scope, explicitly: implementing "later value wins" for the +undocumented remainder (making `tenancy` / `permissions` extender-writable is +a separate, much larger decision triage fenced off this card) and adding a +runtime warning on the silent drop (a real question, filed separately rather +than folded in here). diff --git a/packages/objectql/src/registry-object-extension-nonenumerated-prop-discard.test.ts b/packages/objectql/src/registry-object-extension-nonenumerated-prop-discard.test.ts new file mode 100644 index 0000000000..0a95138385 --- /dev/null +++ b/packages/objectql/src/registry-object-extension-nonenumerated-prop-discard.test.ts @@ -0,0 +1,98 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#12680] `mergeObjectDefinitions`'s docblock used to promise "other props: + * later value wins" while the implementation only ever merged `fields` / + * `validations` / `indexes` (additively) and the three guarded scalars + * `label` / `pluralLabel` / `description` (last-writer-wins). Every other + * top-level prop an `extend` contributor carries was — and still is — + * silently discarded: `merged = { ...base }` and nothing else is copied. + * + * That mismatch is what let cloud#1653's investigation nearly conclude that a + * host could override a framework object's `tenancy` declaration through the + * extension seam — the docblock said so. The docblock was corrected (this + * PR); THIS FILE pins the closed merge set as a fact of the implementation so + * prose and behaviour cannot silently drift apart again. + * + * `icon` is the fixture: a real, spec-legal top-level `ServiceObject` prop + * with no security weight (`tenancy` — the motivating example — is + * deliberately NOT baked into a test, to keep this pin from reading as + * license to write a `tenancy`-specific test elsewhere). + * + * `label` is the positive control: it IS in the guarded-scalar set, so it + * must change under the very same extend-fold. Without it, a fold that did + * nothing at all would pass the negative assertion for the wrong reason. + */ + +import { describe, it, expect } from 'vitest'; +import { SchemaRegistry } from './registry.js'; + +const OWNER_PKG = 'app.owner'; +const EXTENDER_PKG = 'app.extender'; +const OBJECT_NAME = 'merge_fold_nonenumerated_prop_probe'; + +function silentRegistry(): SchemaRegistry { + const r = new SchemaRegistry({ multiTenant: false }); + r.logLevel = 'silent'; + return r; +} + +describe('mergeObjectDefinitions — closed merge set (#12680)', () => { + it('discards a non-enumerated top-level prop (`icon`) from an extend contributor silently', () => { + const r = silentRegistry(); + r.registerObject( + { + name: OBJECT_NAME, + label: 'Base Label', + icon: 'base-icon', + fields: { name: { name: 'name', type: 'text', label: 'Name' } }, + } as any, + OWNER_PKG, + ); + r.registerObject( + { + name: OBJECT_NAME, + // Enumerated scalar — must win (positive control). + label: 'Extender Label', + // NON-enumerated top-level prop — not in the closed merge set, must + // NOT survive the fold, and no error/warning marks the drop. + icon: 'extender-icon', + } as any, + EXTENDER_PKG, + undefined, + 'extend', + ); + + const resolved = r.getObject(OBJECT_NAME) as any; + + // Positive control: the guarded scalar DID fold — proves the extend + // contributor was actually applied, not skipped for some unrelated reason. + expect(resolved.label).toBe('Extender Label'); + + // The pin: the non-enumerated prop was silently discarded. The base's + // value survives untouched — the extension's value never lands. + expect(resolved.icon).toBe('base-icon'); + }); + + it('discards a non-enumerated prop the BASE never declared, rather than materializing it from the extension', () => { + const r = silentRegistry(); + r.registerObject( + { + name: OBJECT_NAME, + label: 'Base Label', + fields: { name: { name: 'name', type: 'text', label: 'Name' } }, + // no `icon` at all on the base + } as any, + OWNER_PKG, + ); + r.registerObject( + { name: OBJECT_NAME, icon: 'extender-icon' } as any, + EXTENDER_PKG, + undefined, + 'extend', + ); + + const resolved = r.getObject(OBJECT_NAME) as any; + expect(resolved.icon).toBeUndefined(); + }); +}); diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 2b289e739f..e931dfd694 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -126,11 +126,49 @@ type ObjectFoldScalarKey = (typeof OBJECT_FOLD_SCALAR_KEYS)[number]; /** * Deep merge two ServiceObject definitions. - * Fields are merged additively. Other props: later value wins. * - * [#8460] …except that "later value wins" is now conditional for the three - * SCALARS. `tenantAuthored` names the scalars the fold's BASE has authored away - * from the packaged owner's value; an extender yields on those. See + * The merge set is CLOSED and enumerable — this is NOT "every prop, later + * value wins": + * - `fields`, `validations`, `indexes` are merged ADDITIVELY. + * - The three {@link OBJECT_FOLD_SCALAR_KEYS} (`label`, `pluralLabel`, + * `description`) are overridden last-writer-wins, subject to the + * `tenantAuthored` yield rule below. + * + * **Every other top-level prop on `extension` is silently discarded.** + * `merged` starts as `{ ...base }` and nothing outside the list above is ever + * copied onto it — a caller that hands this function an extension body + * carrying an undeclared key, e.g. `tenancy: { enabled: false }`, gets a + * valid-but-inert no-op HERE: no error, no warning from this function, the + * base's existing value simply wins as if the extension had never named the + * key (issue #12680). + * + * Declarative authors never reach that silence, though: `ObjectExtensionSchema` + * (`packages/spec/src/data/object.zod.ts`) is `.strict()` — its shape is + * exactly `extend` / `fields` / `label` / `pluralLabel` / `description` / + * `validations` / `indexes` / `priority`, so an `objectExtensions` entry + * naming `tenancy`, `permissions`, or any other key outside that list fails + * LOUDLY at authoring time with a prescription (#4001; the same rule is + * documented at `content/docs/data-modeling/object-extensions.mdx`). This + * function's discard is reachable only by a caller that bypasses that + * schema — a direct, programmatic `registry.registerObject(def, pkg, + * undefined, 'extend', …)` call, which is how this package's own pin + * (`registry-object-extension-nonenumerated-prop-discard.test.ts`) exercises + * the rule. Extender-writable `tenancy`/`permissions` would in any case be a + * separate, much bigger decision that has NOT been made. + * + * Unlike `tenancy`/`permissions`, this one is not blocked by schema: + * `_provenance` is a real top-level prop an `extend` contributor carries + * TODAY. Copying it through (as "later value + * wins" would) lets a third-party extender flip a tenant-authored object's + * `_provenance` to the extending package's — reaching, by a different route, + * the exact outcome ADR-0029 D9.3's priority-reranking guard exists to + * prevent. `registry-object-overlay-layer.test.ts` ("an extender declaring + * priority 140 does not become the base layer") fails if this merge set is + * widened to copy it through. + * + * [#8460] …the SCALAR override above is conditional. `tenantAuthored` names + * the scalars the fold's BASE has authored away from the packaged owner's + * value; an extender yields on those. See * {@link SchemaRegistry.tenantAuthoredScalars} for why the set is computed once * over the base rather than re-derived from the running `merged`. */