fix(objectql): a refused package uninstall mutates nothing (#7970)

.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.

packages/objectql/src/registry.test.ts

@@ -357,6 +357,60 @@ 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();
+ });
+
+ /**
+ * [#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');
+ 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 +481,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

packages/objectql/src/registry.ts

@@ -1838,28 +1838,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) {
@@ -2704,14 +2729,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