fix(objectql): 单记录 delete 绑定 hookContext.previous —— 让引擎符合契约已声明的「for update/delete」 (#5272) - #5283
Merged
Merged
Conversation
…5272) `HookContext.previous` is documented "for update/delete", and `update()` has bound it all along — `delete()` never did. `previous` was `undefined` in both `beforeDelete` and `afterDelete` for every single-record delete, so a legal delete-side condition (`previous.status == 'done'`) was unevaluable and, since #4775, rejected the whole operation — through the generic branch, which reads as an author typo when the engine was simply not binding the key. #5038 inverted the asymmetry: a predicate bulk delete already binds each doomed row's pre-image on its per-row `afterDelete`, so the single-record path was strictly worse than the bulk one — the opposite of the #4800/#4862 ruling that single and bulk mean the same thing. `delete()` now reads the doomed row once, before `beforeDelete` fires, and binds it for both phases. The gate is demand-driven like `update()`'s: a delete-side hook in either phase, or a roll-up summary aggregating this object. The roll-up path's own later pre-image fetch is folded into that same read, so an object with both pays one read, not two — and it is now the raw driver read `update()` already hands `recomputeSummaries`. A missing row leaves `previous` unbound rather than fabricating `{}`/`null`, and a `beforeDelete` hook that repoints or clears the target id re-reads or drops the binding so a stale pre-image never rides into `afterDelete`. The pin that hid this — `hook-condition-previous-scope.test.ts`'s "a delete-shaped context evaluates `previous` against the pre-image" — built `previous` by hand and asserted the wrapper read it, greenlighting behaviour the engine never produced. Replaced with end-to-end cases that drive a real `engine.delete()`: both phases receive the stored pre-image, a `previous.*` condition evaluates instead of rejecting, declared-field materialisation and no-leak hold on the delete side, the read happens exactly once for the two phases, an object with no delete-side hook pays no read at all, and a missing row leaves `previous` unbound. Fixes #5272
…gle-delete-previous
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 13 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
Contributor
Author
|
范围外清单补记(正文第五节写这两条时尚未立项,现更新):
Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5272
按 issue 的 A 方向实施(PM 已拍板):
delete()复用update()同款的 demand-driven 前置行门,在单 id 路径取一次前置行并赋给hookContext.previous,覆盖beforeDelete/afterDelete两个阶段。⛔ 未触碰packages/spec/**、content/docs/releases/**、metadata-protocol/src/protocol.ts、core/src/kernel.ts,也未改 plugin-audit 的__previous兜底。一、STALE-PREMISE 事实核对(基于合并 #5270 之后的现场)
worktree 基于
3905c0064(即 PR #5270 的合入点),逐条核对 issue 正文引用的行为:hookContext.previous的赋值只有一处,在update()单 id 分支update()单 id 分支(engine.ts原 5300 行),以及 #5270 新增的buildPerRowAfterContexts()(原 1225 行),后者为批量 update 和批量 delete 的每行 after 上下文赋previousprevious的写入路径delete()全程不给hookContext.previous赋值grep previous在delete()体内零命中)summaryPrev只喂recomputeSummaries,不进上下文复现原文(在 stash 掉本 PR 的
engine.ts改动、只跑新测试时打印):补充一条 issue 未提、实施中确认的事实:
summaryPrev走的是引擎级this.findOne()(过中间件、RLS、formula 投影),而update()喂给recomputeSummaries的priorRecord是裸 driver 读。同一件事两条路两种语义 —— 合并成一次读时按update()的口径统一(见下)。二、实现摘要(
packages/objectql/src/engine.ts)前置行读取放在
triggerHooks('beforeDelete')之前:delete 的 before 阶段是唯一「除了 id 什么都没有」的阶段(input无data),而前像本来就必须在删除动作前取,一份数据两个阶段共用。门(demand-driven):
hasHooksFor(event, object)(按对象)而不是update()那条this.hooks.get('afterUpdate')?.length > 0(全局),与 [17.x] 批量写按行语义实现:hook 按行触发 + record-change trigger 按行绑定 previous/record(#4800/#4862 拍板 A) #5038 批量 delete 侧同款,更省。needsPriorRecord(schema)放进门,虽然update()的孪生门带着它:对象校验规则只在 insert/update 求值,delete()一条都不跑 —— 带上它就是买一次没有读者的读。这是对 PM 指令中「同款条件」的一处有意收窄,理由记在代码注释里。合并为一次读:原先 roll-up summary 的
summaryPrev是另一次、更晚的读(在beforeDelete之后)。现已删除,recomputeSummaries直接吃这一次前置行。口径按update()统一为裸driver.findOne——recomputeSummaries只取p?.[desc.fkField](存储列),裸读足够,且不会因调用方读权限被 RLS 收窄而静默跳过父行重算。租户/事务安全:裸 driver 读经
buildDriverOptions(object, opCtx.context, hookContext.input.options)取得 options —— 这正是把开启中的事务和tenantId带到裸读上的那一层,update()的前置读同理。少了它会读到事务外、甚至跨租户。不臆造:行不存在 →
previous保持 unbound(而非{}/null),条件读它仍按 #4649/#4775 大声失败,而不是替一条没人读过的记录作答。id 被改写/清空的收尾:
beforeDeletehook 可以改写甚至清空input.id(#4550 的 reject 判定就是为此再问一次)。hook 跑完后若input.id !== id,则重读(改写)或丢弃(清空)绑定 —— 陈旧前像绝不进入afterDelete或 summary 重算;清空后落到 predicate 分支,而批量派发按契约本就不该带任何单行前像(hook-wrappers正是靠这个「两者皆无」来诊断批量派发)。未动:#5038 的批量 predicate delete 按行绑定路径逐字未改。
三、测试
改造既有「开绿灯」的 pin(本单硬性要求)
hook-condition-previous-scope.test.ts的a delete-shaped context evaluates 'previous' against the pre-image手工构造previous,断言的是 wrapper 读没读它 —— 而引擎从不产出这份数据,于是它替一个不存在的行为发了绿灯,真正的故障(每一次带previous.*条件的单记录 delete 都被 #4775 打回)就藏在它后面。已删除,原位留下说明注释,替换为走真实引擎的端到端用例。新增 8 例(全部 insert → 真实
engine.delete()→ 断言 hook 拿到什么)hands 'beforeDelete' the stored pre-imagectx.previous等于数据库前像,不是undefined、也不是 deleteinput的裸{id}hands 'afterDelete' the same pre-image — by then the row is goneevaluates a 'previous.*' delete-side condition instead of rejecting the delete (#4775)previous.status == 'done':两次 delete 都不被拒,且条件有选择性(只对 done 的那条触发)is TOTAL over declared fields on a delete toodoes not leak materialised nulls into what the delete hook observesctx.previous不会多出该行从未有过的列reads the pre-image ONCE for both phasesfindOne增量 = 1(合并读的成本护栏)reads nothing at all when the object has no delete-side hookleaves 'previous' UNBOUND when the row is not thereundefined,不臆造反向验证(证明这些是真 pin,不是重言式):把
engine.ts的改动 stash 掉单跑该文件 —— 6 failed | 17 passed,失败原因正是 issue 描述的两类(expected undefined to deeply equal {...}、HookConditionError ... Unknown variable: previous);另 2 例(无 hook 不读、行不存在不绑)修复前后都应成立,故不失败。恢复改动后 23 例全绿。验证记录
pnpm --filter @objectstack/objectql testTest Files 116 passed (116)/Tests 1875 passed (1875)pnpm --filter @objectstack/objectql typechecktsc --noEmit,无输出(通过)pnpm exec eslint packages/objectql/src/engine.ts packages/objectql/src/hook-condition-previous-scope.test.tsturbo run test --filter @objectstack/plugin-audit --filter @objectstack/trigger-record-changeplugin-audit108 passed;trigger-record-change55 passed(两者都读ctx.previous,是本改动的直接下游)summary-rollup.test.ts(含recomputes when a child is deleted)与engine-summary-retry.test.ts覆盖了被合并掉的那次读,均绿。重的构建/测试全程持
flock /tmp/os-heavy-verify.lock并带NODE_OPTIONS=--max-old-space-size=4096,--maxWorkers=2/--concurrency=2。四、变更记录
.changeset/single-delete-binds-previous.md(@objectstack/objectql: patch),写明行为变化与升级影响:原本因 #4775 被打回的 delete 侧previous.*条件现在正常求值;delete 侧 handler 开始收到ctx.previous;若有人靠ctx.previous == null来判别「这是一次 delete」,该判据失效,应改读ctx.event。未改content/docs/releases/**。五、范围外(未在本 PR 修,留给 PM 分诊)
(ctx as any).__previous兜底可退役(PM 已记为后续 finding,本 PR 按指令不动)。audit-writers.ts的captureBefore在beforeDelete里自己再读一次前像塞进__previous;单记录 delete 的previous落地后,这次读在 delete 路径上已成重复(update 路径本来就重复)。退役它可省掉一次读,但属独立面。update()的前置行门仍是全局的(this.hooks.get('afterUpdate')?.length > 0),而 delete 侧用的是按对象的hasHooksFor。全局门意味着任一对象注册了afterUpdate,所有对象的单 id update 都多付一次读。属可收窄的性能面,非缺陷,未在本单扩面。以上两条尚未开 issue —— 均为 PM 已知/已记项与纯优化面,按 #4949 的立项纪律先在此列明,若需要我可另开
finding标签的观察类 issue。Generated by Claude Code