diff --git a/.changeset/28835-redeclared-extension-field-named.md b/.changeset/28835-redeclared-extension-field-named.md new file mode 100644 index 00000000000..848e3818708 --- /dev/null +++ b/.changeset/28835-redeclared-extension-field-named.md @@ -0,0 +1,16 @@ +--- +'@objectstack/objectql': patch +--- + +fix(objectql): the object fold names, once, a field that two contributors declare in `fields`, and which definition it serves (#28835) + +Clause-②: no +Metadata-protocol: none + +The registry fold every object read serves (`getObject`, the by-name and layered reads through `foldObjectExtendersOnto`, the code layer) merges each extension's `fields` by spreading them. So when two extensions of one object both declare a field name, or an extension's `fields` redeclares a field the owner declares, the last contribution the fold applies is served: extensions fold in ascending `priority`, and at equal priority the one registered last wins. Nothing said so. This is reachable from two vendor dependents, from two `.custom` pairs where one base depends on the other, and, since the `object_extension` item door, from Studio. + +- **Now.** The first time a fold sees such a field, it writes one `console.warn` line per object, field and set of declaring packages. The line names the object, the field, every declaring package in fold order (the owner first when it declares the field), and the package whose definition is served. Repeated reads in one registry do not repeat it; a second registry says it again. It sits beside the existing line for a field patch that misses its target, in the same style. +- **Unchanged.** The served definition, byte for byte: the same winner as before. No door refuses anything new, and `os validate` and the save and publish doors are not touched. +- **Not counted.** `fieldPatches` entries (they are patches, not declarations, and a patch that misses is already named). A `.custom` pair's `__c` field never shares a name with a dependent's plain field, so the two never meet here. +- **Docs.** `content/docs/data-modeling/object-extensions.mdx` no longer says nothing warns. +- **Measured.** No shipped package or example declares the same field from two extensions, or redeclares an owner field in an extension's `fields`: the two shipped `objectExtensions` entries (`examples/app-showcase` on `showcase_account`, `plugin-auth` on `sys_sso_provider`) each add fields the owner does not declare, and HotCRM declares none. diff --git a/content/docs/data-modeling/object-extensions.mdx b/content/docs/data-modeling/object-extensions.mdx index 16c07e2e641..a35dac5793d 100644 --- a/content/docs/data-modeling/object-extensions.mdx +++ b/content/docs/data-modeling/object-extensions.mdx @@ -158,9 +158,12 @@ it does not walk `objectExtensions`, and it could not — the target's name belo its owner, not to you. Nothing therefore stops two packages from contributing the same field name to the same -object, and nothing warns when they do: the merge is last-writer-wins by `priority`, so -one of the two silently disappears. Name defensively — prefix added fields with -something specific to your package when the name is at all generic. +object: the merge is last-writer-wins by `priority`, so one of the two definitions is +not served. The same goes for an extension whose `fields` redeclares a field the owner +declares. Neither is refused; at boot the registry names each such field once in the log, +with every package that declares it and the one whose definition is served. Name +defensively — prefix added fields with something specific to your package when the +name is at all generic. --- @@ -172,7 +175,7 @@ something specific to your package when the name is at all generic. last-writer-wins, validations and indexes accumulate. So a **higher `priority` is applied later and wins** a conflict. Two extensions with the -same priority are folded in registration order, which is not a guarantee to build on — +same priority are folded in registration order (the one registered last wins), which is not a guarantee to build on — give competing extensions distinct priorities. Merging is idempotent: re-registering a package (a metadata rebuild, a dev-server diff --git a/packages/objectql/src/registry-extension-redeclared-field.test.ts b/packages/objectql/src/registry-extension-redeclared-field.test.ts new file mode 100644 index 00000000000..445b0e83466 --- /dev/null +++ b/packages/objectql/src/registry-extension-redeclared-field.test.ts @@ -0,0 +1,202 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#28835] A field name that two contributors of one object both declare in + * `fields` is named once at the fold, with the definition it serves. + * + * `mergeObjectDefinitions` spreads `fields`, so when two extensions of one + * object both add `x`, or an extension's `fields` redeclares a field the owner + * declares, the last contribution the fold applies wins: ascending `priority`, + * and at equal priority the one registered last. That stays the answer (the + * served definition is not changed here); the fold now says so, once per + * object, field and set of packages, in the style of the field-patch miss + * report. `fieldPatches` is not a declaration, and a `.custom` pair's `__c` + * field never shares a name with a dependent's plain one. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { SchemaRegistry } from './registry.js'; + +const OWNER_PKG = 'com.vendor.crm'; +const DEP_A = 'com.acme.crm-territory'; +const DEP_B = 'com.globex.crm-regions'; +const OBJ = 'crm_account'; + +function vendorAccount(): any { + return { + name: OBJ, + label: 'Account', + fields: { + name: { name: 'name', type: 'text', label: 'Name' }, + industry: { + name: 'industry', + type: 'select', + label: 'Industry', + options: [ + { value: 'tech', label: 'Tech' }, + { value: 'retail', label: 'Retail' }, + ], + }, + }, + }; +} + +function withVendor(): SchemaRegistry { + const reg = new SchemaRegistry({ multiTenant: false, searchCompanion: false } as never); + reg.logLevel = 'silent'; + reg.registerObject(vendorAccount(), OWNER_PKG, undefined, 'own'); + return reg; +} + +function extendWith(reg: SchemaRegistry, pkg: string, body: Record, priority = 200): void { + reg.registerObject({ name: OBJ, ...body } as any, pkg, undefined, 'extend', priority); +} + +const regionOf = (pkg: string) => ({ name: 'region', type: 'text', label: `Region (${pkg})` }); + +/** The fold's lines about a field declared more than once. */ +function redeclaredLines(warn: { mock: { calls: unknown[][] } }): string[] { + return warn.mock.calls.map((c) => String(c[0])).filter((l) => l.includes('is declared in `fields` by')); +} + +/** Every read the fold serves, three times over, with the cache dropped between. */ +function readEverywhere(reg: SchemaRegistry): void { + for (let i = 0; i < 3; i += 1) { + reg.invalidateAll(); + reg.getObject(OBJ); + reg.foldObjectExtendersOnto(OBJ, vendorAccount()); + reg.getArtifactItem('object', OBJ); + } +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('#28835 — a field two contributors declare in `fields` is named once at the fold, with the definition served', () => { + it('two dependents both add `region` at equal priority: the one registered last is served, and one line names both and the served one', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const reg = withVendor(); + extendWith(reg, DEP_A, { fields: { region: regionOf(DEP_A) } }); + extendWith(reg, DEP_B, { fields: { region: regionOf(DEP_B) } }); + + const served = reg.getObject(OBJ) as any; + expect(served.fields.region).toEqual(regionOf(DEP_B)); + readEverywhere(reg); + + const lines = redeclaredLines(warn); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain(`Field "region" of object "${OBJ}"`); + expect(lines[0]).toContain(`extensions from "${DEP_A}", "${DEP_B}"`); + expect(lines[0]).toContain(`the definition served is the one from "${DEP_B}"`); + expect(lines[0]).not.toContain(OWNER_PKG); + }); + + it('…by priority, not registration order: the higher-priority dependent registered first is the one served and named', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const reg = withVendor(); + extendWith(reg, DEP_A, { fields: { region: regionOf(DEP_A) } }, 300); + extendWith(reg, DEP_B, { fields: { region: regionOf(DEP_B) } }, 200); + + expect((reg.getObject(OBJ) as any).fields.region).toEqual(regionOf(DEP_A)); + const lines = redeclaredLines(warn); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain(`extensions from "${DEP_B}", "${DEP_A}"`); + expect(lines[0]).toContain(`the definition served is the one from "${DEP_A}"`); + }); + + it('an extension redeclaring a field the owner declares: the extension\'s definition is served, and one line names the owner, the extension and the served one', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const reg = withVendor(); + const takeover = { name: 'industry', type: 'text', label: 'Industry (free text)' }; + extendWith(reg, DEP_A, { fields: { industry: takeover } }); + + expect((reg.getObject(OBJ) as any).fields.industry).toEqual(takeover); + readEverywhere(reg); + + const lines = redeclaredLines(warn); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain(`Field "industry" of object "${OBJ}"`); + expect(lines[0]).toContain(`the owner "${OWNER_PKG}" and by extensions from "${DEP_A}"`); + expect(lines[0]).toContain(`the definition served is the one from "${DEP_A}"`); + }); + + it('the owner and two extensions on one field: one line naming all three in fold order', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const reg = withVendor(); + extendWith(reg, DEP_A, { fields: { industry: { name: 'industry', type: 'text', label: 'A' } } }); + extendWith(reg, DEP_B, { fields: { industry: { name: 'industry', type: 'text', label: 'B' } } }); + + expect((reg.getObject(OBJ) as any).fields.industry.label).toBe('B'); + const lines = redeclaredLines(warn); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain(`the owner "${OWNER_PKG}" and by extensions from "${DEP_A}", "${DEP_B}"`); + expect(lines[0]).toContain(`the definition served is the one from "${DEP_B}"`); + }); + + it('two `.custom` pairs (one base depending on the other) both adding `region__c`: named like any two extensions', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const reg = withVendor(); + extendWith(reg, `${OWNER_PKG}.custom`, { fields: { region__c: { name: 'region__c', type: 'text', label: 'One' } } }); + extendWith(reg, `${DEP_A}.custom`, { fields: { region__c: { name: 'region__c', type: 'text', label: 'Two' } } }); + + expect((reg.getObject(OBJ) as any).fields.region__c.label).toBe('Two'); + const lines = redeclaredLines(warn); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain(`the definition served is the one from "${DEP_A}.custom"`); + }); + + it('a second registry names the same collision again', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + for (let i = 0; i < 2; i += 1) { + const reg = withVendor(); + extendWith(reg, DEP_A, { fields: { region: regionOf(DEP_A) } }); + extendWith(reg, DEP_B, { fields: { region: regionOf(DEP_B) } }); + readEverywhere(reg); + } + expect(redeclaredLines(warn)).toHaveLength(2); + }); + + describe('controls: nothing is named', () => { + it('one extension adding a field of its own', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const reg = withVendor(); + extendWith(reg, DEP_A, { fields: { region: regionOf(DEP_A) } }); + readEverywhere(reg); + expect((reg.getObject(OBJ) as any).fields.region).toEqual(regionOf(DEP_A)); + expect(redeclaredLines(warn)).toEqual([]); + }); + + it('a pair\'s `__c` field beside a dependent\'s plain field of the same stem', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const reg = withVendor(); + extendWith(reg, `${OWNER_PKG}.custom`, { fields: { region__c: { name: 'region__c', type: 'text', label: 'Mine' } } }); + extendWith(reg, DEP_A, { fields: { region: regionOf(DEP_A) } }); + readEverywhere(reg); + const served = reg.getObject(OBJ) as any; + expect(served.fields.region__c.label).toBe('Mine'); + expect(served.fields.region).toEqual(regionOf(DEP_A)); + expect(redeclaredLines(warn)).toEqual([]); + }); + + it('a `fieldPatches` entry of an owner field, beside an extension adding a field', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const reg = withVendor(); + extendWith(reg, DEP_A, { fieldPatches: { industry: { label: 'Sector' } } }); + extendWith(reg, DEP_B, { fields: { region: regionOf(DEP_B) }, fieldPatches: { industry: { label: 'Market' } } }); + readEverywhere(reg); + expect((reg.getObject(OBJ) as any).fields.industry.label).toBe('Market'); + expect(redeclaredLines(warn)).toEqual([]); + }); + + it('a base that already carries the extensions\' fields (a folded body handed back to the fold)', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const reg = withVendor(); + extendWith(reg, DEP_A, { fields: { region: regionOf(DEP_A) } }); + const folded = reg.getObject(OBJ) as any; + reg.foldObjectExtendersOnto(OBJ, structuredClone(folded)); + reg.foldObjectExtendersOnto(OBJ, structuredClone(folded)); + expect(redeclaredLines(warn)).toEqual([]); + }); + }); +}); diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 8827e579eb5..c47d8f140f3 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -513,6 +513,9 @@ type ObjectFieldPatches = NonNullable; /** An `extend` contributor's body: a partial object that may carry {@link ObjectFieldPatches}. */ type ObjectExtensionBody = Partial & { fieldPatches?: ObjectFieldPatches }; +/** [#28835] One extension's `fields` as the fold merged them, with the package that declared them. */ +type FoldedExtensionFields = { packageId: string | undefined; fields: unknown }; + const isPlainRecord = (v: unknown): v is Record => v !== null && typeof v === 'object' && !Array.isArray(v); @@ -4627,6 +4630,8 @@ export class SchemaRegistry { let merged = tenantRules ? this.withPackagedOwnerRules(contributors, baseDefinition, tenantRules) : { ...baseDefinition }; + // [#28835] Each extension's `fields` as the merge took them, in fold order. + const folded: FoldedExtensionFields[] = []; for (const contrib of contributors) { if (contrib.ownership === 'extend') { let extension = governAudit ? applyAuditFieldGovernance(contrib.definition) : contrib.definition; @@ -4664,8 +4669,10 @@ export class SchemaRegistry { ) : []; this.reportFieldPatchMisses([...misses, ...residual], contrib.packageId); + folded.push({ packageId: contrib.packageId, fields: extension.fields }); } } + this.reportRedeclaredFields(String(merged.name ?? baseDefinition.name ?? ''), contributors, ownFields, folded); if (tenantRules) this.reportChangedRuleCopies(contributors, baseDefinition); // A picklist-bound field is served with its list's options resolved onto // it — here, in the fold every object read and the write door share, so @@ -4680,6 +4687,75 @@ export class SchemaRegistry { */ private fieldPatchMissesWarned = new Set(); + /** + * [#28835] Field names already named as declared by more than one + * contributor, keyed `object|field|packages` (the packages in fold order). + * Per registry, for the reason {@link shadowedExtensionQueueWarned} gives. + */ + private redeclaredFieldsWarned = new Set(); + + /** + * [#28835] Name, once, each field name of the object that more than one + * contributor declares in `fields`: two extensions both adding it, or an + * extension redeclaring a field the owner declares (one of `ownFields`). The + * merge spreads `fields`, so the definition served is the last one the fold + * applied: contributions fold in ascending `priority`, and at equal priority + * in the order they registered, so the one registered last. The line names + * the object, the field, every declaring package in that order, and the one + * whose definition is served. + * + * Judged from the contributors' own declarations (`folded`, as the merge + * took them), never from the running body, so a base that already carries + * the extensions' fields (a folded body handed to + * {@link foldObjectExtendersOnto}, a stored overlay row) is not read as a + * redeclaration. `fieldPatches` is not a declaration and is not counted + * ({@link reportFieldPatchMisses} names a patch that misses). A `.custom` + * pair's `__c` field and a dependent's plain one never share a name. + * + * ⛔ Never a refusal, and the served definition is unchanged: the same + * reasons as {@link reportFieldPatchMisses} (the fold runs on every read + * while a boot is still registering packages). At `warn`: one package's + * definition of the field is not served. + */ + private reportRedeclaredFields( + object: string, + contributors: readonly ObjectContributor[], + ownFields: ReadonlySet | undefined, + folded: readonly FoldedExtensionFields[], + ): void { + if (folded.length === 0) return; + const owner = contributors.find((c) => c.ownership === 'own'); + const declarers = new Map>(); + for (const { packageId, fields } of folded) { + if (!isPlainRecord(fields)) continue; + for (const name of Object.keys(fields)) { + let list = declarers.get(name); + if (!list) { + list = owner && ownFields?.has(name) ? [{ packageId: owner.packageId, owner: true }] : []; + declarers.set(name, list); + } + list.push({ packageId, owner: false }); + } + } + const named = (packageId: string | undefined) => (packageId ? `"${packageId}"` : '(no package)'); + for (const [field, list] of declarers) { + if (list.length < 2) continue; + const key = `${object}|${field}|${list.map((d) => d.packageId ?? '').join(',')}`; + if (this.redeclaredFieldsWarned.has(key)) continue; + this.redeclaredFieldsWarned.add(key); + const extenders = list.filter((d) => !d.owner).map((d) => named(d.packageId)); + const declaredBy = list[0]!.owner + ? `the owner ${named(list[0]!.packageId)} and by extensions from ${extenders.join(', ')}` + : `extensions from ${extenders.join(', ')}`; + console.warn( + `[Registry] Field "${field}" of object "${object}" is declared in \`fields\` by ${declaredBy}; ` + + `the definition served is the one from ${named(list[list.length - 1]!.packageId)}, the last the fold applies ` + + '(ascending `priority`; at equal priority, the one registered last). Rename the field in one extension, ' + + 'or patch an owner field\'s display properties instead of redeclaring it.', + ); + } + } + /** * [ADR-0146 D10, second rule] Name, once, each patch the fold did not apply * because the judge missed it: a field that is not one of the owner's own