diff --git a/.changeset/hook-ctx-doc-alias-reads-removed.md b/.changeset/hook-ctx-doc-alias-reads-removed.md new file mode 100644 index 0000000000..76530579b0 --- /dev/null +++ b/.changeset/hook-ctx-doc-alias-reads-removed.md @@ -0,0 +1,25 @@ +--- +'@objectstack/service-storage': patch +'@objectstack/plugin-sharing': patch +'@objectstack/runtime': patch +--- + +hooks: drop the last three `doc` / `previousDoc` alias reads on a hook context — read the engine's own keys only + +Behaviour is unchanged: every one of these limbs guarded against a producer that +has never existed, so none of them could be reached. + +- `service-storage` attachment lifecycle read `ctx.result ?? ctx.input.doc ?? ctx.input.data` +- `plugin-sharing` primary-BU projection read `(ctx.input.data ?? ctx.input.doc).user_id` +- `runtime`'s hook sandbox read `engineCtx.input ?? engineCtx.doc` and `engineCtx.previous ?? engineCtx.previousDoc` + +Every ObjectQL write context spells the payload `data` — measured and pinned by +`hook-input-shape-contract.test.ts` in `@objectstack/objectql` ("insert carries +`data` — never `doc`", #5273). The top-level pair is the same family one level +up: `HookContextSchema` declares `input` / `result` / `previous` and neither a +`doc` nor a `previousDoc`, and `engine.ts` — the sole producer of a HookContext +— builds neither. The limbs survived only because the old `HookContext.input` +contract table documented insert as `{ doc, options }`; that table was corrected +in #5668, and the same alias was removed from `trigger-record-change` in #5671. +These are the remainder (#5906), removed rather than left as a second de-facto +contract (PD #12). diff --git a/packages/objectql/src/hook-input-shape-contract.test.ts b/packages/objectql/src/hook-input-shape-contract.test.ts index 9695fcbbbf..95ef831a7e 100644 --- a/packages/objectql/src/hook-input-shape-contract.test.ts +++ b/packages/objectql/src/hook-input-shape-contract.test.ts @@ -19,9 +19,14 @@ * filters bind the driver call where no handler can widen them. So the one * field the docs pointed at resolved `undefined`. * - `insert` was documented as `{ doc: Record, ... }`; the engine builds - * `{ data: row, ... }`. (`trigger-record-change` still carries a defensive - * `input.doc` alias read for that reason — filed separately, not fixed - * here.) + * `{ data: row, ... }`. That wrong sentence was the whole reason consumers + * carried defensive `input.doc` alias reads — branches guarding against a + * producer that has never existed. All of them are now gone: + * `trigger-record-change` in #5671, and the last three — service-storage's + * attachment lifecycle, plugin-sharing's primary-BU projection, and the + * runtime hook sandbox (which aliased the top-level `doc`/`previousDoc` of + * the same family) — in #5906. No consumer defends against this key today, + * so the assertions below are what keeps it that way. * * The table was also silent about #5038: since ADR-0058's bulk-write addendum * the `after*` events on a bulk write fire PER MATCHED ROW on a diff --git a/packages/plugins/plugin-sharing/src/primary-bu-projection.ts b/packages/plugins/plugin-sharing/src/primary-bu-projection.ts index e7f2db9a45..27f23acde6 100644 --- a/packages/plugins/plugin-sharing/src/primary-bu-projection.ts +++ b/packages/plugins/plugin-sharing/src/primary-bu-projection.ts @@ -77,7 +77,12 @@ function collectUserIds(ctx: any): string[] { const add = (v: unknown) => { if (v != null && v !== '') ids.add(String(v)); }; add(ctx?.result?.user_id); add(ctx?.previous?.user_id); - add((ctx?.input?.data ?? ctx?.input?.doc)?.user_id); + // `input.data` is the ONE key a write hook's payload arrives under — measured + // and pinned by objectql's `hook-input-shape-contract.test.ts` ("insert carries + // `data` — never `doc`", #5273). An `input.doc` alias limb sat below this read + // for a producer that never existed; removed in #5906 (same family as #5671) + // rather than left as a second de-facto contract (PD #12). + add(ctx?.input?.data?.user_id); add(ctx?.[STASH_KEY]); return [...ids]; } diff --git a/packages/runtime/src/sandbox/body-runner.test.ts b/packages/runtime/src/sandbox/body-runner.test.ts index 0d84e489fc..7fd077b1b0 100644 --- a/packages/runtime/src/sandbox/body-runner.test.ts +++ b/packages/runtime/src/sandbox/body-runner.test.ts @@ -156,6 +156,52 @@ describe('hookBodyRunnerFactory', () => { await fn!(engineCtx); expect(backing.website).toBe('https://acme.com'); }); + + // [#5906] `input` and `previous` are the engine's own spellings, and the only + // ones: `HookContextSchema` declares no top-level `doc`/`previousDoc`, and + // objectql's `engine.ts` — the sole producer of a HookContext — builds neither. + // Alias limbs for both used to sit in `buildSandboxContext`; these two pin that + // the sandbox now seeds from the truth keys and from nothing else. The NEGATIVE + // one carries the weight: the truth keys sit FIRST in both reads, so the + // positive case would stay green if either alias limb were put back. + describe('seeds ctx.input / ctx.previous from the engine keys only', () => { + /** A `ql` whose single write records what the body observed. */ + const probingQl = (seen: Array>) => ({ + object: () => ({ insert: async (data: any) => { seen.push(data); return data; } }), + }); + + const probeHook = (seen: Array>) => + hookBodyRunnerFactory(runner, { ql: probingQl(seen), appId: 'crm' })({ + name: 'probe', + object: 'contact', + events: ['beforeUpdate'], + body: { + language: 'js', + source: + "await ctx.api.object('probe').insert({" + + ' input: JSON.stringify(ctx.input),' + + ' previous: JSON.stringify(ctx.previous ?? null) });', + capabilities: ['api.write'], + }, + } as any); + + it('reads `input` and `previous`', async () => { + const seen: Array> = []; + await probeHook(seen)!({ input: { email: 'new@x.io' }, previous: { email: 'old@x.io' } } as any); + expect(seen[0]).toEqual({ + input: '{"email":"new@x.io"}', + previous: '{"email":"old@x.io"}', + }); + }); + + it('does NOT read a `doc` / `previousDoc` alias — no engine path produces either', async () => { + const seen: Array> = []; + // The spellings the deleted limbs defended, and nothing else on the context: + // with them unread the body sees an empty input and no previous at all. + await probeHook(seen)!({ doc: { email: 'new@x.io' }, previousDoc: { email: 'old@x.io' } } as any); + expect(seen[0]).toEqual({ input: '{}', previous: 'null' }); + }); + }); }); describe('actionBodyRunnerFactory', () => { diff --git a/packages/runtime/src/sandbox/body-runner.ts b/packages/runtime/src/sandbox/body-runner.ts index 6e6d042489..3b8be04276 100644 --- a/packages/runtime/src/sandbox/body-runner.ts +++ b/packages/runtime/src/sandbox/body-runner.ts @@ -305,8 +305,14 @@ function buildSandboxApi(engineCtx: any, ql: any, errLabel: string) { } function buildSandboxContext(engineCtx: any, ql: any): ScriptContext { - const inputSnapshot = unwrapProxyToPlain(engineCtx?.input ?? engineCtx?.doc); - const previousRaw = engineCtx?.previous ?? engineCtx?.previousDoc; + // `input` and `previous` are the engine's own spellings, and the only ones: + // `HookContextSchema` (`packages/spec/src/data/hook.zod.ts`) declares neither a + // top-level `doc` nor a `previousDoc`, and objectql's `engine.ts` — the sole + // producer of a HookContext — builds neither. Alias limbs for both sat here for + // producers that never existed; removed in #5906 (same family as #5671) rather + // than left as a second de-facto contract (PD #12). + const inputSnapshot = unwrapProxyToPlain(engineCtx?.input); + const previousRaw = engineCtx?.previous; return { input: inputSnapshot ?? {}, // Preserve `undefined` for `previous` on insert events so hooks can diff --git a/packages/services/service-storage/src/attachment-lifecycle.test.ts b/packages/services/service-storage/src/attachment-lifecycle.test.ts index bd1a2d2b6c..8fda3dc27e 100644 --- a/packages/services/service-storage/src/attachment-lifecycle.test.ts +++ b/packages/services/service-storage/src/attachment-lifecycle.test.ts @@ -167,6 +167,52 @@ describe('installAttachmentLifecycleHooks — tombstoning', () => { expect(engine.updates[0].data).toMatchObject({ id: 'f1', status: 'committed', deleted_at: null }); }); + // [#5906] `input.data` is the ONE key an insert payload arrives under — measured + // on the real engine by objectql's `hook-input-shape-contract.test.ts` ("insert + // carries `data` — never `doc`", #5273). The fixture above cannot pin that: it + // supplies `result`, which sits FIRST in the handler's read, so it stays green + // whatever the limbs below it spell. These two carry the weight instead, and the + // NEGATIVE one is the load-bearing half — it goes red the moment the deleted + // `input.doc` alias limb is put back (that limb sat ahead of `data`, so a + // `doc`-only context would be read again). + const tombstonedFile = () => ({ + id: 'f1', + key: 'attachments/f1.bin', + scope: 'attachments', + status: 'deleted', + deleted_at: '2026-01-01T00:00:00Z', + }); + + it('un-tombstones from input.data when the context carries no result', async () => { + const engine = fakeEngine({ attachments: [], files: [tombstonedFile()] }); + installAttachmentLifecycleHooks(engine, silentLogger()); + + await engine.trigger('afterInsert', { + object: 'sys_attachment', + event: 'afterInsert', + input: { data: { file_id: 'f1' } }, + }); + + expect(engine.updates).toHaveLength(1); + expect(engine.updates[0].data).toMatchObject({ id: 'f1', status: 'committed', deleted_at: null }); + }); + + it('does NOT read an `input.doc` alias — no engine path produces that key', async () => { + const engine = fakeEngine({ attachments: [], files: [tombstonedFile()] }); + installAttachmentLifecycleHooks(engine, silentLogger()); + + await engine.trigger('afterInsert', { + object: 'sys_attachment', + event: 'afterInsert', + // The spelling the deleted limb defended. With it gone the handler finds no + // `file_id` at all, so the tombstone stands. + input: { doc: { file_id: 'f1' } }, + }); + + expect(engine.updates).toHaveLength(0); + expect(engine.tables.sys_file[0].status).toBe('deleted'); + }); + it('a failing lookup never blocks the delete (best-effort)', async () => { const engine = fakeEngine({ attachments: [], files: [] }); engine.findOne = async () => { diff --git a/packages/services/service-storage/src/attachment-lifecycle.ts b/packages/services/service-storage/src/attachment-lifecycle.ts index 72b96ea088..6b8753401f 100644 --- a/packages/services/service-storage/src/attachment-lifecycle.ts +++ b/packages/services/service-storage/src/attachment-lifecycle.ts @@ -171,7 +171,14 @@ export function installAttachmentLifecycleHooks( 'afterInsert', async (ctx: any) => { try { - const row: any = ctx?.result ?? ctx?.input?.doc ?? ctx?.input?.data; + // An after-insert context carries the stored row on `ctx.result`, and the + // written payload under `input.data` — `data` is the ONLY spelling any + // engine path produces, measured and pinned by objectql's + // `hook-input-shape-contract.test.ts` ("insert carries `data` — never + // `doc`", #5273). An `input.doc` alias limb used to sit between these two + // for a producer that never existed; removed in #5906 (same family as + // #5671) rather than left as a second de-facto contract (PD #12). + const row: any = ctx?.result ?? ctx?.input?.data; const fileId = row?.file_id; if (!fileId) return; const file = await engine.findOne('sys_file', { where: { id: String(fileId) }, context: { ...SYSTEM_CTX } });