From d702f98f144deda3e5f9878ee03d5a3b996fa1c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 13:56:44 +0000 Subject: [PATCH 1/3] wip: reorder uninstallPackage refusal ahead of mutations (#7970) --- packages/objectql/src/registry.test.ts | 89 ++++++++++++++++++++++++++ packages/objectql/src/registry.ts | 70 ++++++++++++++------ 2 files changed, 141 insertions(+), 18 deletions(-) diff --git a/packages/objectql/src/registry.test.ts b/packages/objectql/src/registry.test.ts index 709eb853d0..d492dc3c03 100644 --- a/packages/objectql/src/registry.test.ts +++ b/packages/objectql/src/registry.test.ts @@ -357,6 +357,37 @@ describe('SchemaRegistry', () => { registry.unregisterObjectsByPackage('com.owner', true); }).not.toThrow(); }); + + it('[#7970] refuses before removing anything — a free sibling object survives', () => { + // `free` is walked FIRST (Map insertion order) and used to be spliced + // out on the way to refusing over `important`, so the refusal that + // exists to keep the registry whole half-tore it down instead. + registry.registerObject({ name: 'free', fields: {} }, 'com.owner', 'base', 'own'); + registry.registerObject({ name: 'important', fields: {} }, 'com.owner', 'base', 'own'); + registry.registerObject({ name: 'important', fields: {} }, 'com.ext', undefined, 'extend'); + + expect(() => { + registry.unregisterObjectsByPackage('com.owner'); + }).toThrow(/object "important" is extended by com\.ext/); + + expect(registry.getObject('free')).toBeDefined(); + expect(registry.getObject('important')).toBeDefined(); + }); + + it('[#7970] force still removes the owner even with a free sibling ahead of it', () => { + registry.registerObject({ name: 'free', fields: {} }, 'com.owner', 'base', 'own'); + registry.registerObject({ name: 'important', fields: {} }, 'com.owner', 'base', 'own'); + registry.registerObject({ name: 'important', fields: {} }, 'com.ext', undefined, 'extend'); + + registry.unregisterObjectsByPackage('com.owner', true); + + // The refusal pass is skipped under `force`, and the mutation pass is + // unchanged: both of the package's contributions are gone, and the + // extender's own contribution is left where it was. + expect(registry.getObject('free')).toBeUndefined(); + expect(registry.getObjectOwner('important')).toBeUndefined(); + expect(registry.getObjectContributors('important')).toHaveLength(1); + }); }); // ========================================== @@ -427,6 +458,64 @@ describe('SchemaRegistry', () => { expect(registry.getNamespaceOwner('test')).toBeUndefined(); }); + /** + * [#7970] The uninstall's ONE refusable step is `unregisterObjectsByPackage` + * (ADR-0029: you may not uninstall the owner of an object another package + * extends). It now runs before every mutation, so reaching that refusal + * costs nothing. The namespace is the limb that measured this: the release + * used to run FIRST, so a refused uninstall left the package installed — + * record, objects and items all intact — while its namespace no longer + * resolved, for the life of the process. No test refused and then inspected + * the namespace, which is exactly why the defect was invisible. + * + * Latent by grade: no in-tree caller reaches the refusal path today. + */ + it('[#7970] a refused uninstall leaves the namespace still resolving', () => { + registry.installPackage({ id: 'com.crm', name: 'CRM', namespace: 'crm', version: '1.0.0' } as any); + registry.registerObject({ name: 'contact', fields: {} }, 'com.crm', 'crm', 'own'); + registry.registerObject({ name: 'contact', fields: {} }, 'com.analytics', undefined, 'extend'); + + expect(registry.getNamespaceOwner('crm')).toBe('com.crm'); + + expect(() => registry.uninstallPackage('com.crm')).toThrow( + /Cannot uninstall package "com\.crm".*extended by com\.analytics/, + ); + + // The assertion the card names: refused ⇒ the namespace still resolves. + expect(registry.getNamespaceOwner('crm')).toBe('com.crm'); + expect(registry.getNamespaceOwners('crm')).toEqual(['com.crm']); + }); + + it('[#7970] a refused uninstall leaves the whole package intact, not just the namespace', () => { + registry.installPackage({ id: 'com.crm', name: 'CRM', namespace: 'crm', version: '1.0.0' } as any); + // Registered ahead of the extended object, so the object walk reaches + // this one before it can refuse. + registry.registerObject({ name: 'account', fields: {} }, 'com.crm', 'crm', 'own'); + registry.registerObject({ name: 'contact', fields: {} }, 'com.crm', 'crm', 'own'); + registry.registerObject({ name: 'contact', fields: {} }, 'com.analytics', undefined, 'extend'); + registry.registerItem('page', { name: 'home' }, 'name', 'com.crm'); + + expect(() => registry.uninstallPackage('com.crm')).toThrow(/extended by com\.analytics/); + + // Every limb `uninstallPackage` mutates, in the order it mutates them. + expect(registry.getNamespaceOwner('crm')).toBe('com.crm'); + expect(registry.getObject('account')).toBeDefined(); + expect(registry.getObject('contact')).toBeDefined(); + expect(registry.getItem('page', 'home')).toMatchObject({ name: 'home' }); + expect(registry.getPackage('com.crm')).toBeDefined(); + }); + + it('[#7970] the successful path still releases the namespace after the object verb', () => { + registry.installPackage({ id: 'com.crm', name: 'CRM', namespace: 'crm', version: '1.0.0' } as any); + registry.registerObject({ name: 'contact', fields: {} }, 'com.crm', 'crm', 'own'); + + expect(registry.uninstallPackage('com.crm')).toBe(true); + + expect(registry.getNamespaceOwner('crm')).toBeUndefined(); + expect(registry.getObject('contact')).toBeUndefined(); + expect(registry.getPackage('com.crm')).toBeUndefined(); + }); + it('updatePackageManifest merges editable fields, preserving lifecycle state', () => { registry.installPackage({ id: 'com.test', name: 'Old', version: '1.0.0' } as any); registry.disablePackage('com.test'); // lifecycle state that must survive an edit diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index e8e6996ec4..bca5d0725c 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -1774,28 +1774,53 @@ export class SchemaRegistry { /** * Unregister all objects contributed by a package. - * + * + * [#7970] **Refuses before it mutates.** If any object this package owns is + * extended by another package (ADR-0029), the call throws having removed + * nothing — the refusal is decided across every object first. Callers may + * therefore treat a throw as a no-op, which is what lets + * {@link uninstallPackage} run this verb ahead of its own mutations. + * * @throws Error if trying to uninstall an owner that has extenders */ unregisterObjectsByPackage(packageId: string, force: boolean = false): void { + // [#7970] REFUSAL PASS — the whole decision, taken before a single + // contribution is removed. This check used to live inline in the mutation + // walk below, one object at a time, so a package owning `account` (free) + // and `contact` (extended by another package) lost `account` on the way to + // refusing over `contact`: the guard that exists to keep a registry whole + // was itself reached through a mutation, and nothing rolled it back. Same + // predicate and same iteration order as the inline check it replaces, so + // the same object still refuses with the same message — what changed is + // only that no removal precedes the throw. + if (!force) { + for (const [fqn, contributors] of this.objectContributors.entries()) { + const ownedHere = contributors.some( + c => c.packageId === packageId && c.ownership === 'own' + ); + if (!ownedHere) continue; + // Extenders from other packages + const otherExtenders = contributors.filter( + c => c.packageId !== packageId && c.ownership === 'extend' + ); + if (otherExtenders.length > 0) { + throw new Error( + `Cannot uninstall package "${packageId}": object "${fqn}" is extended by ` + + `${otherExtenders.map(c => c.packageId).join(', ')}. Uninstall extenders first.` + ); + } + } + } + + // MUTATION PASS — carries no refusal of its own; the pass above already + // proved every removal below is allowed. Keep it that way: a second copy of + // the predicate here is the two-places-that-must-agree shape this ordering + // fix was chosen over. for (const [fqn, contributors] of this.objectContributors.entries()) { // Find this package's contributions const packageContribs = contributors.filter(c => c.packageId === packageId); - - for (const contrib of packageContribs) { - if (contrib.ownership === 'own' && !force) { - // Check if there are extenders from other packages - const otherExtenders = contributors.filter( - c => c.packageId !== packageId && c.ownership === 'extend' - ); - if (otherExtenders.length > 0) { - throw new Error( - `Cannot uninstall package "${packageId}": object "${fqn}" is extended by ` + - `${otherExtenders.map(c => c.packageId).join(', ')}. Uninstall extenders first.` - ); - } - } + for (const contrib of packageContribs) { // Remove contribution const idx = contributors.indexOf(contrib); if (idx !== -1) { @@ -2640,14 +2665,23 @@ export class SchemaRegistry { return false; } + // [#7970] Unregister objects FIRST — this is the one step that can REFUSE + // (ADR-0029: the package owns an object another package `extend`s), so + // every mutation below is downstream of the refusal point and a refused + // uninstall removes nothing at all. The namespace release used to sit + // ABOVE this line, which meant reaching the guard that exists to keep a + // registry whole cost the namespace on the way in: the package stayed + // installed with its objects and its record intact, while its namespace no + // longer resolved, for the life of the process. Safe as the first step + // because the verb reads `objectContributors` only — it depends on nothing + // the steps below establish. + this.unregisterObjectsByPackage(id); + // Unregister namespace if (pkg.manifest.namespace) { this.unregisterNamespace(pkg.manifest.namespace, id); } - // Unregister objects (will throw if extenders exist) - this.unregisterObjectsByPackage(id); - // [#7221] …and everything else the package shipped. The object verb above // reaches `objectContributors` only, so without this an uninstall dropped // the package record while its `page`/`view`/`flow`/`app`/`api` entries From df0f84fd901cf369eec27ef4be36be1e666a6f29 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 15:02:18 +0000 Subject: [PATCH 2/3] changeset --- .../uninstall-refuses-before-mutating.md | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 .changeset/uninstall-refuses-before-mutating.md diff --git a/.changeset/uninstall-refuses-before-mutating.md b/.changeset/uninstall-refuses-before-mutating.md new file mode 100644 index 0000000000..73b7b6b625 --- /dev/null +++ b/.changeset/uninstall-refuses-before-mutating.md @@ -0,0 +1,35 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): a refused package uninstall now mutates nothing (#7970) + +`SchemaRegistry.uninstallPackage` has one step that can **refuse**: +`unregisterObjectsByPackage`, which throws when the package owns an object +another package `extend`s (ADR-0029 — the refusal that tells an operator to +uninstall the extenders first). That guard exists to keep a registry whole, and +it was reached **through** mutations, so exercising it half-tore down the very +package it was protecting. Two limbs were exposed: + +- **The namespace.** `uninstallPackage` released it before calling the refusing + verb. A refused uninstall therefore left the package installed — record, + objects and items all intact — while its namespace no longer resolved, for the + life of the process. The invariant was already written three lines below the + defect ("a refused uninstall must remove nothing at all"); the code above it + did the opposite. +- **The package's other objects.** `unregisterObjectsByPackage` decided the + refusal one object at a time, inside the walk that removes them, so a package + owning `account` (free) and `contact` (extended) lost `account` on its way to + refusing over `contact`. + +Both are fixed by ordering, not by a transaction or a second probe: the refusing +verb runs first in `uninstallPackage`, and the verb itself now decides the +refusal across every object before removing any. A throw from +`unregisterObjectsByPackage` is a no-op, which is what lets its callers run it +ahead of their own mutations. + +Unchanged: the refusal's message and the object it names, the `force: true` path +(which skips the refusal and removes exactly what it removed before), and the +successful uninstall's observable outcome. Grade is **latent** — no in-tree +caller reaches the refusal path today, so no shipped behaviour was +observably broken; this restores the invariant before one does. From 3addbc8a010efbc09d7fa75a9356eaaf029a9f15 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 15:06:03 +0000 Subject: [PATCH 3/3] test: pin refusal message identity under multiple extended objects (#7970) --- packages/objectql/src/registry.test.ts | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/objectql/src/registry.test.ts b/packages/objectql/src/registry.test.ts index e20195348d..b876ceb300 100644 --- a/packages/objectql/src/registry.test.ts +++ b/packages/objectql/src/registry.test.ts @@ -374,6 +374,29 @@ describe('SchemaRegistry', () => { expect(registry.getObject('important')).toBeDefined(); }); + /** + * [#7970] MESSAGE IDENTITY, the half the reorder must not disturb. The + * refusal pass replaces an inline check, so it must name the SAME object + * and the SAME extenders as before — with two refusable objects the + * first one walked still wins, and an object's extenders are still + * listed in registration order. This test is deliberately written to + * pass BOTH before and after the fix: run it against the pre-fix + * `registry.ts` and it stays green, which is what proves the message did + * not move (only the mutations that used to precede it are gone). + */ + it('[#7970] the refusal still names the first refusable object and all its extenders', () => { + registry.registerObject({ name: 'alpha', fields: {} }, 'com.owner', 'base', 'own'); + registry.registerObject({ name: 'beta', fields: {} }, 'com.owner', 'base', 'own'); + registry.registerObject({ name: 'alpha', fields: {} }, 'com.ext1', undefined, 'extend'); + registry.registerObject({ name: 'alpha', fields: {} }, 'com.ext2', undefined, 'extend'); + registry.registerObject({ name: 'beta', fields: {} }, 'com.ext3', undefined, 'extend'); + + expect(() => registry.unregisterObjectsByPackage('com.owner')).toThrow( + 'Cannot uninstall package "com.owner": object "alpha" is extended by ' + + 'com.ext1, com.ext2. Uninstall extenders first.', + ); + }); + it('[#7970] force still removes the owner even with a free sibling ahead of it', () => { registry.registerObject({ name: 'free', fields: {} }, 'com.owner', 'base', 'own'); registry.registerObject({ name: 'important', fields: {} }, 'com.owner', 'base', 'own');