Skip to content

Commit 4fedb11

Browse files
os-zhuangclaude
andauthored
fix(objectql): retire sys_fetch_previous_delete so the delete-path per-object gate is honest (#5929) (#6794)
* fix(objectql): retire `sys_fetch_previous_delete` so the delete-path per-object gate is honest (#5929) `delete()`'s prior-row read is gated per object on `hasHooksFor('beforeDelete', object) || hasHooksFor('afterDelete', object) || getSummaryDescriptors(object).length > 0`. On any kernel-hosted engine the first term was constant true, because `ObjectQLPlugin` registered its own `sys_fetch_previous_delete` builtin with `object: '*'` — so the per-object skip the gate exists to perform never happened outside the bare engines unit tests boot. The builtin could not use what it held open. Since #5272 (by-id) and #6697 (predicate path, per matched row) the engine reads the pre-image and binds `previous` before `beforeDelete` dispatches, so its `!ctx.previous` guard was permanently false and it issued no read. Its only remaining effect was holding open the gate that made it redundant. Retired under ADR-0049 enforce-or-remove; the measurement #5846 recorded in `plugin.ts` was re-verified on this branch rather than taken on trust. The gate's three terms are unchanged — no term was added or removed. What changed is that term 1 now reflects real hooks. `engine.ts` gains the enumeration of the delete-phase hooks that still register globally (plugin-auth, plugin-sharing, service-storage; plugin-audit narrows at the engine face with `excludeObjects`), so nobody reads a skip into a trace that will not show one. New `engine-delete-prior-read-scope.test.ts` pins the three terms per object, the `excludeObjects` subtraction on both phases, the predicate path's twin gate, and — on a real `ObjectKernel` + `ObjectQLPlugin`, the only configuration where the defect was observable — the zero-read skip and the still-bound `previous`. It replays the retired builtin's own shape and measures its guard short-circuiting, so "the guard can no longer be true" stays a measurement. The by-id `beforeDelete` REPOINT behaviour is deliberately untouched (#6752). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNT8SWDEsDQp2TrmSBizKq * test(objectql): keep the new delete-gate pins inside the erasure ratchets (#5929) Two lint.yml gates caught the new test file on the first full run: * `check:slot-lookup` / `no-restricted-syntax` — `kernel.getService('objectql') as any` erased the slot's contract for every `engine.*` call in the kernel section, which is where the measurement lives. Typed as `getService<ObjectQL>('objectql')`. * `check:query-options-erasure` — three `count(obj, {} as any)` calls grew the test-surface count 263 → 266. The empty options bag is already on contract; the assertion was never needed. No assertion changed; the file still passes 16/16. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNT8SWDEsDQp2TrmSBizKq --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f012f55 commit 4fedb11

5 files changed

Lines changed: 713 additions & 47 deletions

File tree

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
---
2+
"@objectstack/objectql": patch
3+
---
4+
5+
fix(objectql): retire `sys_fetch_previous_delete`, so `delete()`'s per-object prior-row gate is a real question (#5929)
6+
7+
`delete()` reads the doomed row's pre-image only when something on **this
8+
object** consumes it — a delete-side hook in either phase, or a roll-up summary
9+
aggregating it. That gate has been per object since #5272, and on a
10+
kernel-hosted engine it was **constant true for every object that has ever
11+
existed**, so the skip it exists to perform never happened outside the unit
12+
tests' bare engines.
13+
14+
The reason was `ObjectQLPlugin`'s own builtin. `sys_fetch_previous_delete`
15+
registered on `beforeDelete` with `object: '*'`, which made the gate's first
16+
term true everywhere — and it could not use what it held open: since #5272 (and
17+
#6697, which extended the same ordering to the predicate path, per matched row)
18+
the engine reads the pre-image and binds `previous` **before** `beforeDelete`
19+
dispatches, so the builtin's own `if (input.id && !ctx.previous)` guard was
20+
permanently false and it issued no read. Its only remaining effect was holding
21+
open the gate that made it redundant. Retired under ADR-0049
22+
enforce-or-remove.
23+
24+
**What changes for you, in both directions:**
25+
26+
- An object with **no** delete-side hook and no roll-up summary now performs
27+
**no prior-row read** on a by-id `delete()`. Previously it always did, on any
28+
kernel.
29+
- Where a delete-side hook **does** exist, `previous` still arrives bound,
30+
identically — from the engine's own read, which was already the only producer.
31+
Hook handlers, declarative `condition`s reading `previous`, the record-change
32+
trigger, the audit diff and the summary recompute are all unaffected.
33+
- The residual shape the retired guard could still have been true for — the
34+
engine's read found nothing because the row is already gone — is one where the
35+
builtin's read found nothing either, so it binds nothing there too. `previous`
36+
stays **unbound** rather than fabricated as `{}`, unchanged (#4649/#4775).
37+
38+
Nothing about hook dispatch, the by-id repoint re-resolution, or the delete
39+
dispatch ladder changes. The gate's three terms are unchanged — what changed is
40+
that its first term now reflects **real** hooks.
41+
42+
One caveat worth stating so nobody reads a skip into a trace that will not show
43+
one: a kernel that also loads plugin-auth, plugin-sharing or service-storage
44+
still has the gate held open on every object, because each registers a
45+
delete-phase hook with no `object` and decides applicability inside its handler.
46+
Those are real consumers, not circular ones. plugin-audit is the exception and
47+
the worked example — it narrows at the engine face with `excludeObjects`
48+
(#5860), so an excluded object really does skip the read. The full enumeration
49+
is in `engine.ts` beside `wantsPreImage`.

0 commit comments

Comments
 (0)