diff --git a/.changeset/adr-0116-followups.md b/.changeset/adr-0116-followups.md new file mode 100644 index 0000000000..7e0189584a --- /dev/null +++ b/.changeset/adr-0116-followups.md @@ -0,0 +1,42 @@ +--- +"@objectstack/plugin-auth": patch +"@objectstack/plugin-webhooks": patch +--- + +fix(plugin-auth,plugin-webhooks): retire a dead degrade branch and an implicit transitive dependency (ADR-0116 follow-ups, #4187) + +Two concrete findings from the ADR-0116 consumer-side audit, plus the +authoring rule that would have prevented both. + +**`plugin-auth` claimed a fallback it did not have.** `init()` ran +`const dataEngine = ctx.getService('data'); if (!dataEngine) { warn('No data +engine service found - auth will use in-memory storage') }`. That branch could +never execute: `getService` **throws** for an unregistered service rather than +returning `undefined`, and this plugin declares a hard dependency on ObjectQL +(which registers `data` unconditionally), so a kernel without the engine fails +even earlier with `Dependency … not found`. The branch is removed and the real +contract is declared — `requiresServices: ['data', 'manifest']` — which also +replaces a trailing `// manifest service required` comment with the +machine-checked form of the same claim. `AuthManager` keeps its own optional +`dataEngine` guards: it is usable outside the plugin. + +**`plugin-webhook-outbox` was protected only transitively.** It resolves +`manifest` in `init()` with no fallback while depending on +`com.objectstack.service.messaging`, which in turn depends on ObjectQL, the +actual provider. That works today and would have broken silently the day +messaging stopped depending on the engine — surfacing as a crash inside an +unrelated plugin's init. It now declares `requiresServices: ['manifest']` +directly. + +Neither change alters ordering or boot outcomes on any current composition: +both plugins were already ordered correctly. What changes is what a broken +composition *says*, and that the guarantees are now checked rather than +inherited. + +Docs: `content/docs/plugins/anatomy.mdx` gains the three ADR-0116 fields and +the decision rule for resolving a service inside `init()` (hard dependency vs +`optionalDependencies` + `requiresServices`), including the two traps behind +these fixes — don't rely on a transitive provider, and don't write an +`if (!svc)` fallback after a bare `getService`. The api-registry example +declares the contract on all seven of its plugins instead of relying on +`kernel.use()` order. diff --git a/content/docs/plugins/anatomy.mdx b/content/docs/plugins/anatomy.mdx index 7ea9469a29..a7525c3c03 100644 --- a/content/docs/plugins/anatomy.mdx +++ b/content/docs/plugins/anatomy.mdx @@ -131,9 +131,32 @@ export interface Plugin { * Dependencies (Optional) * List of other plugin names that this plugin depends on. * The kernel ensures these plugins are initialized before this one. + * A name that is not composed on the kernel fails the boot. */ dependencies?: string[]; + /** + * Soft dependencies (Optional) — order-if-present, ADR-0116. + * Hoisted ahead exactly like `dependencies` when composed, silently + * skipped when absent. For plugins that degrade without the dependency + * but must never initialize before it. + */ + optionalDependencies?: string[]; + + /** + * Services this plugin resolves SYNCHRONOUSLY during init() (ADR-0116). + * Validated before Phase 1 and again immediately before this plugin's + * init, so a misordered composition fails with a named error instead of + * crashing inside your init. + */ + requiresServices?: string[]; + + /** + * Services this plugin's init() registers UNCONDITIONALLY (ADR-0116). + * Never declare an option-gated registration. + */ + providesServices?: string[]; + /** * Init Phase (REQUIRED) * Called when the kernel is initializing. Use this to: @@ -242,6 +265,47 @@ Plugins follow a strict three-phase lifecycle managed by the kernel: 2. **start()** — Called after *all* plugins have initialized. Start servers, connect to databases, or execute main logic. 3. **destroy()** — Called during shutdown, in reverse order. Clean up connections, timers, and resources. +### Resolving a service inside `init()` — declare it + +`kernel.use()` **registration order is not a contract**. Init order comes from +the dependency graph, so "it works because I registered it later" is luck, and +it is the recipe behind two production bugs (a server that booted with no +tables, then [#4085](https://github.com/objectstack-ai/objectstack/issues/4085)). +Anything you resolve in `start()` needs no declaration — Phase 1 completes +before any `start()` runs. For `init()`, pick one of two: + +| Your plugin… | Declare | +| :--- | :--- | +| **cannot work** without the provider | `dependencies: ['']` — plus `requiresServices` to say which service, and why | +| **degrades** without it, but must init after it when present | `optionalDependencies: ['']` + `requiresServices: ['']` | + +Resolving a service in `init()` while declaring **neither** is the bug shape: +it works until someone composes the stack in a different order, and then fails +inside your `init()` with an error that names the service but not the cause. + +```ts +class MyPlugin implements Plugin { + // Hard: the engine is always composed with this plugin. + dependencies = ['com.objectstack.engine.objectql']; + requiresServices = ['manifest']; // what the dependency is FOR + + async init(ctx: PluginContext) { + ctx.getService<{ register(m: any): void }>('manifest').register(mySchema); + } +} +``` + +Two rules worth stating explicitly, because both have already caused bugs: + +- **Declare the requirement directly, not transitively.** Depending on a plugin + that *happens* to depend on the real provider works until that plugin's own + dependencies change, and the breakage then surfaces in your `init()`. +- **A service you probe behind `try`/`catch` is not a requirement.** Put only + hard, no-fallback needs in `requiresServices` — and if you write a fallback + branch, make sure it can actually run: `getService` **throws** for a missing + service, it never returns `undefined`, so `if (!svc) { /* degrade */ }` after + a bare `getService` is dead code advertising a mode you do not have. + ## Next Steps - **[Plugin System](/docs/plugins)** — Module overview: architecture, configuration, and built-in plugins diff --git a/packages/core/examples/api-registry-example.ts b/packages/core/examples/api-registry-example.ts index 2bbf732af3..83baa95561 100644 --- a/packages/core/examples/api-registry-example.ts +++ b/packages/core/examples/api-registry-example.ts @@ -24,6 +24,12 @@ async function example1_BasicApiRegistration() { const customerPlugin: Plugin = { name: 'customer-plugin', version: '1.0.0', + // init() resolves `api-registry` synchronously with no fallback, so the + // ordering requirement is DECLARED rather than left to the `kernel.use()` + // order above (ADR-0116). Registration order is not a contract — the + // kernel orders from this graph. + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -163,6 +169,8 @@ async function example2_MultiPluginDiscovery() { // Data Plugin - REST APIs const dataPlugin: Plugin = { name: 'data-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -212,6 +220,8 @@ async function example2_MultiPluginDiscovery() { // Analytics Plugin - Beta API const analyticsPlugin: Plugin = { name: 'analytics-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -282,6 +292,8 @@ async function example3_ConflictResolution() { // Core Plugin - High priority const corePlugin: Plugin = { name: 'core-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -310,6 +322,8 @@ async function example3_ConflictResolution() { // Custom Plugin - Medium priority const customPlugin: Plugin = { name: 'custom-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -361,6 +375,8 @@ async function example4_CustomProtocol() { const websocketPlugin: Plugin = { name: 'websocket-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -431,6 +447,8 @@ async function example5_DynamicSchemas() { const dynamicPlugin: Plugin = { name: 'dynamic-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 8d034ab7d3..a34867707b 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -144,8 +144,19 @@ export class AuthPlugin implements Plugin { providesServices = ['auth', 'tenancy']; type = 'standard'; version = '1.0.0'; - dependencies: string[] = ['com.objectstack.engine.objectql']; // manifest service required - + dependencies: string[] = ['com.objectstack.engine.objectql']; + /** + * What that dependency is FOR, stated so the kernel can check it (#4187). + * `init()` resolves both synchronously with no fallback: `data` builds the + * AuthManager config, `manifest` registers the auth objects. ObjectQL + * registers both unconditionally and the hard dependency above hoists it + * ahead, so this never changes whether the boot succeeds — it changes what + * a broken composition SAYS, and replaces the trailing `// manifest service + * required` comment with the machine-checked form of the same claim. + */ + requiresServices = ['data', 'manifest']; + + private options: AuthPluginOptions; private authManager: AuthManager | null = null; /** ADR-0093 D4 — the tenancy service registered in init(); reused at kernel:ready. */ @@ -199,11 +210,19 @@ export class AuthPlugin implements Plugin { throw new Error('AuthPlugin: secret is required'); } - // Get data engine service for database operations + // Get data engine service for database operations. This plugin declares a + // hard dependency on ObjectQL (which registers `data` unconditionally), so + // the service is always present here. + // + // There used to be an `if (!dataEngine) warn('…auth will use in-memory + // storage')` guard under this line. It could never fire and the fallback it + // named did not exist: `getService` THROWS for an unregistered service, it + // does not return undefined, and the hard dependency means a kernel without + // the engine fails earlier still with "Dependency … not found". A branch + // that advertises a tolerance the composition forbids is worse than no + // branch — it reads as a supported degraded mode (#4187). AuthManager keeps + // its own `dataEngine?` guards because it is usable outside this plugin. const dataEngine = ctx.getService('data'); - if (!dataEngine) { - ctx.logger.warn('No data engine service found - auth will use in-memory storage'); - } const authConfig: AuthManagerOptions & AuthPluginOptions = { ...this.options, diff --git a/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts b/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts index bca364208b..929aba428d 100644 --- a/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts +++ b/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts @@ -62,6 +62,16 @@ export class WebhookOutboxPlugin implements Plugin { version = '2.0.0'; type = 'standard' as const; dependencies = ['com.objectstack.service.messaging']; + /** + * `init()` registers this plugin's schema through `manifest` with no + * fallback. Until #4187 that was safe only TRANSITIVELY — messaging happens + * to depend on ObjectQL, which provides `manifest` — so the guarantee would + * have evaporated silently the day messaging stopped depending on the + * engine, and the failure would have surfaced as an unrelated plugin's + * init crash. Declaring the requirement directly makes the kernel check it + * regardless of what messaging depends on. + */ + requiresServices = ['manifest']; private autoEnqueuer: AutoEnqueuer | undefined; /** Engine the provenance hook was bound to, so `dispose()` can unbind it. */