diff --git a/.changeset/23505-extension-refusal-409.md b/.changeset/23505-extension-refusal-409.md new file mode 100644 index 00000000000..3a809de4784 --- /dev/null +++ b/.changeset/23505-extension-refusal-409.md @@ -0,0 +1,17 @@ +--- +'@objectstack/objectql': patch +--- + +fix(objectql): an uninstall refused because another package extends one of its objects answers `409 RESOURCE_CONFLICT` with the way out, instead of `500 INTERNAL_ERROR` (ADR-0029) + +Clause-②: no + +`SchemaRegistry.assertPackageUninstallable` refuses to uninstall a package while another package extends an object it owns (a manifest's `objectExtensions`, ADR-0029). `unregisterObjectsByPackage` and `uninstallPackage` raise the same refusal, because they ask it. When the refusal happens does not change, and a refused uninstall still removes nothing. Only the error it throws changes. + +**FROM.** The error was a bare `Error` with no `code` and no `status`. Its message, `Cannot uninstall package "P": object "O" is extended by E. Uninstall extenders first.`, named only the first extended object. `DELETE /api/v1/packages/P` answered `500 INTERNAL_ERROR`, a server failure, although the caller can change the state that blocks the uninstall. + +**TO.** The error carries `code: 'RESOURCE_CONFLICT'` and `status: 409` (and `httpStatus: 409`), so `DELETE /api/v1/packages/P` answers `409 RESOURCE_CONFLICT`. The message starts with the way out: `Uninstall the package that extends an object of "P" first (E), then uninstall "P" again: …`. It then names every extended object with the packages that extend it: the first five objects, then a count of the rest. The thrown error also carries `packageId`, `extendedObjects` and `extenders`. + +**Unchanged.** A package is refused in the same cases as before. A refused uninstall changes nothing: the `sys_packages` row, the registry entry, the stored metadata rows and the disable record all stay. `unregisterObjectsByPackage(id, true)` still does not ask. + +**If you matched the old text.** Match `error.code === 'RESOURCE_CONFLICT'` (HTTP `409`) instead. The message still contains `Cannot uninstall package "P": object "O" is extended by E`, after the way out. The closing `Uninstall extenders first.` is gone. diff --git a/packages/objectql/src/registry-assert-package-uninstallable.test.ts b/packages/objectql/src/registry-assert-package-uninstallable.test.ts index 4eccbf2d751..bce59b5ca1b 100644 --- a/packages/objectql/src/registry-assert-package-uninstallable.test.ts +++ b/packages/objectql/src/registry-assert-package-uninstallable.test.ts @@ -10,20 +10,25 @@ * mutates when it does not refuse. The refusal pass (#7970) is now this one * method, and `unregisterObjectsByPackage` calls it: ONE predicate. * + * #23505 — the refusal carries the ADR-0112 envelope, `RESOURCE_CONFLICT` / + * 409, so the uninstall door answers a located client refusal instead of its + * `500 INTERNAL_ERROR` fallback; its message opens with the way out and names + * every extended object and every extending package. + * * Pinned here: - * 1. it refuses with the uninstall's exact sentence, and mutates nothing; - * 2. it answers normally for an uninstallable package, and mutates nothing; - * 3. `unregisterObjectsByPackage` still refuses with that same sentence, by - * calling this method (not a copy of its predicate), and `force` still - * means "do not ask". + * 1. it refuses with that envelope, the way out first, every extended object + * and extender named, and mutates nothing; + * 2. one extended object with one extender reads in the singular, and past + * five extended objects the rest are counted, never dropped silently; + * 3. it answers normally for an uninstallable package, and mutates nothing; + * 4. `unregisterObjectsByPackage` and `uninstallPackage` refuse with that + * same refusal, by calling it (not a copy of its predicate), and `force` + * still means "do not ask". */ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { SchemaRegistry } from './registry'; -const REFUSAL = - 'Cannot uninstall package "com.owner": object "alpha" is extended by com.ext1, com.ext2. Uninstall extenders first.'; - /** Every contributor of every object the fixture registers, as plain data. */ function contributorsOf(registry: SchemaRegistry) { return ['free', 'alpha', 'beta'].map((name) => ({ @@ -33,6 +38,16 @@ function contributorsOf(registry: SchemaRegistry) { })); } +/** The error `fn` throws — it must throw. */ +function thrownBy(fn: () => unknown): any { + try { + fn(); + } catch (e) { + return e; + } + throw new Error('expected a throw'); +} + describe('#21276 SchemaRegistry.assertPackageUninstallable', () => { let registry: SchemaRegistry; @@ -48,15 +63,63 @@ describe('#21276 SchemaRegistry.assertPackageUninstallable', () => { registry.registerObject({ name: 'beta', fields: {} } as any, 'com.ext3', undefined, 'extend'); }); - it('refuses with the uninstall\'s exact sentence, and mutates nothing', () => { + it('[#23505] refuses RESOURCE_CONFLICT / 409, the way out first, every extended object and extender named, and mutates nothing', () => { const before = contributorsOf(registry); - expect(() => registry.assertPackageUninstallable('com.owner')).toThrow(REFUSAL); + const err = thrownBy(() => registry.assertPackageUninstallable('com.owner')); + + expect({ code: err.code, status: err.status, httpStatus: err.httpStatus }).toEqual({ + code: 'RESOURCE_CONFLICT', + status: 409, + httpStatus: 409, + }); + expect(err.message.startsWith( + 'Uninstall the packages that extend objects of "com.owner" first (com.ext1, com.ext2, com.ext3), ' + + 'then uninstall "com.owner" again', + ), err.message).toBe(true); + // The named subjects, in walk order: the first refusable object first, + // each with its extenders in registration order. + expect(err.packageId).toBe('com.owner'); + expect(err.extendedObjects).toEqual([ + { objectName: 'alpha', extenders: ['com.ext1', 'com.ext2'] }, + { objectName: 'beta', extenders: ['com.ext3'] }, + ]); + expect(err.extenders).toEqual(['com.ext1', 'com.ext2', 'com.ext3']); + expect(err.message).toContain('"alpha"'); + expect(err.message).toContain('"beta"'); + expect(err.message).not.toContain('"free"'); expect(contributorsOf(registry)).toEqual(before); expect(before.every((o) => o.resolves)).toBe(true); }); + it('[#23505] one extended object with one extender reads in the singular', () => { + const r = new SchemaRegistry({ multiTenant: false }); + r.registerObject({ name: 'leave_request', fields: {} } as any, 'com.leave', 'base', 'own'); + r.registerObject({ name: 'leave_request', fields: {} } as any, 'com.addon', undefined, 'extend'); + + const err = thrownBy(() => r.assertPackageUninstallable('com.leave')); + + expect(err.message.startsWith( + 'Uninstall the package that extends an object of "com.leave" first (com.addon), then uninstall "com.leave" again', + ), err.message).toBe(true); + }); + + it('[#23505] past five extended objects the rest are counted, and every extender is still named', () => { + const r = new SchemaRegistry({ multiTenant: false }); + const names = ['o1', 'o2', 'o3', 'o4', 'o5', 'o6', 'o7']; + for (const name of names) r.registerObject({ name, fields: {} } as any, 'com.big', 'base', 'own'); + names.forEach((name, i) => r.registerObject({ name, fields: {} } as any, `com.x${i}`, undefined, 'extend')); + + const err = thrownBy(() => r.assertPackageUninstallable('com.big')); + + expect(err.extendedObjects).toHaveLength(7); + for (const name of names.slice(0, 5)) expect(err.message).toContain(`"${name}"`); + expect(err.message).not.toContain('"o6"'); + expect(err.message).toContain('2 more'); + for (let i = 0; i < 7; i += 1) expect(err.message).toContain(`com.x${i}`); + }); + it('answers normally for a package nothing extends, and mutates nothing', () => { registry.registerObject({ name: 'free', fields: {} } as any, 'com.ext1', undefined, 'extend'); const before = contributorsOf(registry); @@ -67,14 +130,29 @@ describe('#21276 SchemaRegistry.assertPackageUninstallable', () => { expect(contributorsOf(registry)).toEqual(before); }); - it('unregisterObjectsByPackage refuses with the same sentence BY calling it; force does not ask', () => { + it('unregisterObjectsByPackage and uninstallPackage refuse with the same refusal BY calling it; force does not ask', () => { + const asked = thrownBy(() => registry.assertPackageUninstallable('com.owner')); const ask = vi.spyOn(registry, 'assertPackageUninstallable'); - expect(() => registry.unregisterObjectsByPackage('com.owner')).toThrow(REFUSAL); + const viaUnregister = thrownBy(() => registry.unregisterObjectsByPackage('com.owner')); expect(ask).toHaveBeenCalledTimes(1); expect(ask).toHaveBeenCalledWith('com.owner'); expect(registry.getObject('free')).toBeDefined(); + registry.installPackage({ id: 'com.owner', name: 'Owner', version: '1.0.0' } as any); + ask.mockClear(); + const viaUninstall = thrownBy(() => registry.uninstallPackage('com.owner')); + expect(ask).toHaveBeenCalledTimes(1); + expect(registry.getPackage('com.owner')).toBeDefined(); + + for (const err of [viaUnregister, viaUninstall]) { + expect({ code: err.code, status: err.status, message: err.message }).toEqual({ + code: asked.code, + status: asked.status, + message: asked.message, + }); + } + ask.mockClear(); registry.unregisterObjectsByPackage('com.owner', true); expect(ask).not.toHaveBeenCalled(); diff --git a/packages/objectql/src/registry.test.ts b/packages/objectql/src/registry.test.ts index cd024dcc672..29e979e8407 100644 --- a/packages/objectql/src/registry.test.ts +++ b/packages/objectql/src/registry.test.ts @@ -383,18 +383,28 @@ describe('SchemaRegistry', () => { * 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). + * + * [#23505] The message did move, deliberately: it opens with the way + * out and goes on to name EVERY extended object, not only the first. + * The order this test holds is unchanged — the first refusable object + * walked is named first, with its extenders in registration order — so + * it reads that order off the structured refusal rather than off prose. */ - it('[#7970] the refusal still names the first refusable object and all its extenders', () => { + it('[#7970] the refusal still names the first refusable object first, with 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.', - ); + let err: any; + try { + registry.unregisterObjectsByPackage('com.owner'); + } catch (e) { + err = e; + } + expect(err?.extendedObjects?.[0]).toEqual({ objectName: 'alpha', extenders: ['com.ext1', 'com.ext2'] }); + expect(err.message.indexOf('"alpha"')).toBeLessThan(err.message.indexOf('"beta"')); }); it('[#7970] force still removes the owner even with a free sibling ahead of it', () => { diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 24e268fc253..69df6079e5a 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -2002,6 +2002,82 @@ export class ObjectOwnershipConflictError extends Error { } } +/** One object {@link PackageUninstallExtendedError} names, with the packages extending it. */ +interface ExtendedObjectOfUninstall { + /** The object's fully-qualified name, as the registry keys it. */ + readonly objectName: string; + /** Every OTHER package contributing an `extend` to it, in registration order. */ + readonly extenders: readonly string[]; +} + +/** How many extended objects the refusal spells out before it counts the rest. */ +const EXTENDED_OBJECTS_LISTED = 5; + +/** + * [#23505] The uninstall refusal of ADR-0029: a package cannot leave while + * another package `extend`s an object it owns (a manifest's + * `objectExtensions`, registered by `ObjectQL.registerApp`). Raised by + * {@link SchemaRegistry.assertPackageUninstallable}, and so by + * {@link SchemaRegistry.unregisterObjectsByPackage} and + * {@link SchemaRegistry.uninstallPackage}, which ask it. + * + * Carries the ADR-0112 envelope (`code` + `status`), like the registry's other + * refusals above. Before it carried one, this was a bare `Error`, and + * `DELETE /api/v1/packages/:id` — which reaches it through `deletePackage`'s + * pre-flight, or through `uninstallPackage` on a host with no persisted half — + * fell through `resolveThrownHttpError` to the door's `500` fallback: measured + * on the real stack as `500 INTERNAL_ERROR`, which tells a client, an AI author + * or an operator that the server failed, invites a retry, and changes nothing. + * The state is the caller's to change: uninstall the extending packages first. + * + * `RESOURCE_CONFLICT` / 409, no code minted: the standard catalog's member for + * a refusal that comes from environment state rather than the request (the + * same uninstall is accepted once the extenders are gone), as the uninstall + * verb's sibling refusals in `@objectstack/metadata-protocol` answer. The + * prescription comes first, because a 4xx message can be truncated at a REST + * boundary; the diagnostic sentence the refusal always carried follows it. + * + * Every extended object is named (the first {@link EXTENDED_OBJECTS_LISTED}, + * then a count), in the order the registry walks them, each with its extenders + * in registration order — and the prescription names every extending package, + * so one answer is the whole way out rather than the first step of several. + * + * ⛔ Unexported, like its siblings: a consumer compares `code`, never + * `instanceof` (see the note over the `*_CODE` constants). + */ +class PackageUninstallExtendedError extends Error { + readonly code = 'RESOURCE_CONFLICT'; + readonly status = 409; + /** The same number under ADR-0112 D5's spelling — what a consumer holding the THROWN error reads (the CLI `--json` envelope). `status` stays for the HTTP doors, which read it. */ + readonly httpStatus = 409; + /** The package whose uninstall this refusal stopped. */ + readonly packageId: string; + /** Every object of `packageId` another package extends, in walk order. */ + readonly extendedObjects: readonly ExtendedObjectOfUninstall[]; + /** Every extending package, once each, in the order the objects name them. */ + readonly extenders: readonly string[]; + + constructor(packageId: string, extendedObjects: readonly ExtendedObjectOfUninstall[]) { + const extenders = [...new Set(extendedObjects.flatMap((o) => o.extenders))]; + const listed = extendedObjects + .slice(0, EXTENDED_OBJECTS_LISTED) + .map((o) => `object "${o.objectName}" is extended by ${o.extenders.join(', ')}`); + const more = extendedObjects.length - listed.length; + super( + `Uninstall the ${extenders.length === 1 ? 'package that extends' : 'packages that extend'} ` + + `${extendedObjects.length === 1 ? 'an object' : 'objects'} of "${packageId}" first ` + + `(${extenders.join(', ')}), then uninstall "${packageId}" again: a package cannot be ` + + `uninstalled while another package extends an object it owns (objectExtensions, ADR-0029). ` + + `Cannot uninstall package "${packageId}": ${listed.join('; ')}` + + `${more > 0 ? `; and ${more} more extended object${more === 1 ? '' : 's'}` : ''}.`, + ); + this.name = 'PackageUninstallExtendedError'; + this.packageId = packageId; + this.extendedObjects = extendedObjects; + this.extenders = extenders; + } +} + // [commit fa5d137ab] `isTenantAuthored` and `isCodeArtifactBody` used to be defined here. // They now live in `@objectstack/metadata-core` // (`code-artifact-provenance.ts`), imported at the top of this file and @@ -4244,16 +4320,25 @@ export class SchemaRegistry { * 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 replaced, so the same object still refuses - * with the same message. + * first. + * + * [#23505] The refusal is a {@link PackageUninstallExtendedError} — + * `RESOURCE_CONFLICT` / 409, so the uninstall door answers a located client + * refusal instead of its `500` fallback — and it names EVERY extended object + * and every extending package, not only the first object the walk meets, so + * its prescription is the whole way out. The decision is unchanged: the + * package is refused exactly when the first such object exists. * * ⛔ ONE predicate: {@link unregisterObjectsByPackage} calls this method * rather than keeping its own copy, so the question asked ahead and the * refusal raised by the uninstall cannot disagree. `force` is not a * parameter here: forcing means not asking, and stays the caller's choice. * - * @throws Error if the package owns an object another package extends (ADR-0029) + * @throws PackageUninstallExtendedError (`RESOURCE_CONFLICT` / 409) if the + * package owns an object another package extends (ADR-0029) */ assertPackageUninstallable(packageId: string): void { + const extended: ExtendedObjectOfUninstall[] = []; for (const [fqn, contributors] of this.objectContributors.entries()) { const ownedHere = contributors.some( c => c.packageId === packageId && c.ownership === 'own' @@ -4264,12 +4349,10 @@ export class SchemaRegistry { 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.` - ); + extended.push({ objectName: fqn, extenders: otherExtenders.map(c => String(c.packageId)) }); } } + if (extended.length > 0) throw new PackageUninstallExtendedError(packageId, extended); } /** @@ -4282,7 +4365,8 @@ export class SchemaRegistry { * 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 + * @throws PackageUninstallExtendedError (`RESOURCE_CONFLICT` / 409) if trying + * to uninstall an owner that has extenders — {@link assertPackageUninstallable}'s */ unregisterObjectsByPackage(packageId: string, force: boolean = false): void { // [#7970] REFUSAL PASS — see {@link assertPackageUninstallable}, which holds diff --git a/packages/runtime/src/package-uninstall-store-refusal.integration.test.ts b/packages/runtime/src/package-uninstall-store-refusal.integration.test.ts index d17c8737893..c01185e1a20 100644 --- a/packages/runtime/src/package-uninstall-store-refusal.integration.test.ts +++ b/packages/runtime/src/package-uninstall-store-refusal.integration.test.ts @@ -24,10 +24,14 @@ * 4. The registry's own uninstall refusal — another package extends an object * this one owns (ADR-0029) — is decided BEFORE the store delete * (`SchemaRegistry.assertPackageUninstallable`, asked by `deletePackage`): - * the door answers `500` and nothing changes — the stored rows, the - * registry entry and the disable record are all intact, in the same - * process and after a restart. That is the envelope the door gave when it - * withdrew the package itself first. + * nothing changes — the stored rows, the registry entry and the disable + * record are all intact, in the same process and after a restart. + * [#23505] The door answers it as the client refusal it is: `409 + * RESOURCE_CONFLICT`, its message opening with the way out (uninstall the + * extending package first) and naming the object and its extender. It + * answered `500 INTERNAL_ERROR` while the registry threw a bare `Error`. + * 5. CONTROL for 4: the extending package uninstalled through the same door, + * the same uninstall succeeds — the prescription is a real way out. * * ## The composition — the shipped pieces, booted twice over one database file * @@ -249,20 +253,36 @@ describe('#21276 CONTROL — an ordinary delete removes the package, and a resta }); }); +/** + * An object `PKG` owns, extended by `OTHER_PKG` — registered exactly as + * `ObjectQL.registerApp` registers a manifest's `objects` and its + * `objectExtensions`. In memory only: nothing about it is stored. + */ +function extendAnObjectOfPkg(p: Process): string { + p.engine.registry.registerObject({ name: 'leave_request', fields: { title: { type: 'text' } } } as any, PKG, 'leave', 'own'); + const fqn = p.engine.registry.getAllObjects(PKG).map((o: any) => o.name).find((n: string) => n.endsWith('leave_request')); + p.engine.registry.registerObject({ name: fqn, fields: { note: { type: 'text' } } } as any, OTHER_PKG, undefined, 'extend'); + return fqn; +} + describe('#21276 the registry\'s uninstall refusal (an ADR-0029 extender) is decided before the store delete', () => { - it('500, and nothing changes: stored rows, registry entry and disable record intact, in the same process and after a restart', async () => { + it('[#23505] 409 RESOURCE_CONFLICT naming the way out, and nothing changes: stored rows, registry entry and disable record intact, in the same process and after a restart', async () => { const dir = newDir(); const first = await boot(dir); await seed(first); // Another package extends an object this one owns, so the registry refuses - // the uninstall. Registered in memory only: nothing about it is stored. - first.engine.registry.registerObject({ name: 'leave_request', fields: { title: { type: 'text' } } } as any, PKG, 'leave', 'own'); - const fqn = first.engine.registry.getAllObjects(PKG).map((o: any) => o.name).find((n: string) => n.endsWith('leave_request')); - first.engine.registry.registerObject({ name: fqn, fields: { note: { type: 'text' } } } as any, OTHER_PKG, undefined, 'extend'); + // the uninstall. + const fqn = extendAnObjectOfPkg(first); const answer = await first.call('DELETE', `/${PKG}`); - expect({ status: answer.status, code: answer.code }).toEqual({ status: 500, code: 'INTERNAL_ERROR' }); + expect({ status: answer.status, code: answer.code }).toEqual({ status: 409, code: 'RESOURCE_CONFLICT' }); + // The way out comes first (a 4xx message can be truncated), then what it is about. + const message: string = answer.body?.error?.message; + expect(message.startsWith( + `Uninstall the package that extends an object of "${PKG}" first (${OTHER_PKG}), then uninstall "${PKG}" again`, + ), message).toBe(true); + expect(message).toContain(`"${fqn}"`); const detail = await first.call('GET', `/${PKG}`); expect(detail.status).toBe(200); expect(detail.body?.data).toMatchObject({ enabled: false, status: 'disabled' }); @@ -278,4 +298,32 @@ describe('#21276 the registry\'s uninstall refusal (an ADR-0029 extender) is dec expect(after.body?.data).toMatchObject({ enabled: false, status: 'disabled' }); expect(await restarted.metadataRows()).toEqual(['view/leave_list']); }); + + it('[#23505] CONTROL — the extending package uninstalled through the same door, the same uninstall succeeds', async () => { + const dir = newDir(); + const first = await boot(dir); + await seed(first); + // The extender is an installed package this time, so the way out the + // refusal names can be taken through the door. + await first.protocol.installPackage({ manifest: { id: OTHER_PKG, name: 'Addon', version: '1.0.0', type: 'app' } } as any); + const fqn = extendAnObjectOfPkg(first); + expect((await first.call('DELETE', `/${PKG}`)).status, 'precondition: refused while extended').toBe(409); + + const extenderGone = await first.call('DELETE', `/${OTHER_PKG}`); + expect(extenderGone.status).toBe(200); + + const answer = await first.call('DELETE', `/${PKG}`); + + expect(answer.status).toBe(200); + expect(answer.body?.data).toMatchObject({ packageId: PKG, success: true, registryRemoved: true }); + expect((await first.call('GET', `/${PKG}`)).status).toBe(404); + expect(first.engine.registry.getObject(fqn)).toBeUndefined(); + expect(await first.metadataRows()).toEqual([]); + expect(loadDisabledPackageIds(ENV).has(PKG)).toBe(false); + await first.destroy(); + + const restarted = await boot(dir); + + expect((await restarted.call('GET', `/${PKG}`)).status).toBe(404); + }); });