|
| 1 | +--- |
| 2 | +"@objectstack/objectql": patch |
| 3 | +"@objectstack/core": patch |
| 4 | +"@objectstack/metadata-protocol": patch |
| 5 | +"@objectstack/runtime": patch |
| 6 | +"@objectstack/plugin-audit": patch |
| 7 | +"@objectstack/plugin-auth": patch |
| 8 | +--- |
| 9 | + |
| 10 | +fix(objectql): a by-id `update()`/`delete()` against a nonexistent record answers 404 `RECORD_NOT_FOUND` instead of a 400 from further down the pipeline (#7867) |
| 11 | + |
| 12 | +Nothing on the action-body write path ever asked whether the target row existed. |
| 13 | +`ctx.api.object(name).update({ id, … })` reached `ObjectQL.update()`'s by-id |
| 14 | +branch through `buildSandboxApi` → `ObjectRepository`, and that branch had **no |
| 15 | +existence gate at all**: `engine.update()` on a ghost id was a silent no-op that |
| 16 | +resolved `null`, so the write ran on into validation, the driver and the hook |
| 17 | +chain and died on whichever complained first. |
| 18 | + |
| 19 | +**Which one it died on varied with the object's declarations**, which is why the |
| 20 | +defect read as several unrelated bugs: |
| 21 | + |
| 22 | +- a **hooked** object → `400` `HookConditionError`, from an `afterUpdate` |
| 23 | + condition reading `previous` on a row nobody read; |
| 24 | +- an **unhooked** object → `400` `VALIDATION_FAILED` "X is required", because |
| 25 | + with no prior row a PATCH is validated as if it were a whole record. |
| 26 | + |
| 27 | +The 400 class varied; the missing 404 was the constant. Measured on one showcase |
| 28 | +stack, same id, same object, same second: `POST /actions/showcase_task/ |
| 29 | +showcase_mark_done/<ghost>` answered 400 while `PATCH /data/showcase_task/ |
| 30 | +<ghost>` answered 404. Both answer **404 `RECORD_NOT_FOUND`** now. |
| 31 | + |
| 32 | +`delete()` had the same shape and was the worse of the two: with no gate it |
| 33 | +reported success for a row that was never there, so a typo'd id, an |
| 34 | +already-deleted row and a real deletion were indistinguishable. |
| 35 | + |
| 36 | +**This is not a `previous`-binding bug.** `if (priorRecord) hookContext.previous |
| 37 | += …` is correct and is untouched — ADR-0058 Addendum II / #4649 require that an |
| 38 | +absent row leave `previous` UNBOUND rather than fabricated. It was behaving |
| 39 | +correctly on a path that should never have been entered, so the fix removes the |
| 40 | +producer rather than specializing what it produced. |
| 41 | + |
| 42 | +**Where the gate went, and why there.** At the engine, in the by-id branches of |
| 43 | +`update()` and `delete()` — the one point all three action-body write faces |
| 44 | +funnel through (`ctx.api.object()`, its context-less repo-facade fallback, and |
| 45 | +`ctx.engine.update()`). A repository-level gate would have closed one of the |
| 46 | +three and made `ql.update(o, { id })` and `ctx.api.object(o).update({ id })` |
| 47 | +answer one ghost id two different ways. Two sibling paths already gated |
| 48 | +correctly — `protocol.updateData`/`deleteData` (#4435) and `callData`'s ObjectQL |
| 49 | +fallback (#5138) — and all three now throw the **same** `recordNotFoundError`, |
| 50 | +which moved to `@objectstack/core` so the engine can reach it without importing |
| 51 | +`@objectstack/metadata-protocol` (forbidden in the `/core` closure by ADR-0076 |
| 52 | +D2's boundary ratchet). `@objectstack/metadata-protocol` re-exports it unchanged. |
| 53 | + |
| 54 | +Existence is asked with a pre-write read, never off the write's own result: |
| 55 | +`IDataDriver.update` declares no not-found signal, and the engine's post-write |
| 56 | +readback is `null` for a second reason (a write that moves the row out of the |
| 57 | +caller's row scope), so reading either would answer 404 to a write that landed. |
| 58 | + |
| 59 | +**Behaviour change worth knowing about — the by-id prior-row read is now |
| 60 | +unconditional.** #5284 (update) and #5929 (delete) had narrowed it to "does |
| 61 | +anything CONSUME the prior row?", skipping the read for objects with no hook, no |
| 62 | +prior-reading validation rule and no roll-up. Existence is a consumer that |
| 63 | +demand list never enumerated and the one consumer every by-id write has, and no |
| 64 | +cheaper question answers it — so the skip and the gate are mutually exclusive. |
| 65 | +The measured cost is small: #5929's own record enumerates the global hook |
| 66 | +registrants (plugin-sharing, service-storage, plugin-auth, plugin-audit), so on |
| 67 | +any kernel that loads them the demand was already true for every object and the |
| 68 | +narrowing skipped nothing. The read is genuinely new only for a bare |
| 69 | +`@objectstack/objectql/core` embedder — which is buying a 404 it did not have. |
| 70 | + |
| 71 | +Three read-count pins measured the old skip and now measure the read, each |
| 72 | +recording what changed and why at its own site: #5284's and #5929's in |
| 73 | +`packages/objectql`, and #5860's `sys_job_queue` case in `@objectstack/plugin-audit`. |
| 74 | +The DISPATCH half all three are actually about — the per-object `hasHooksFor` |
| 75 | +question, the `excludeObjects` subtraction, and the retired |
| 76 | +`sys_fetch_previous_*` builtins — is untouched and still pinned. |
| 77 | + |
| 78 | +One further case encoded the old silent no-op as correct: `@objectstack/plugin-auth`'s |
| 79 | +#5941 last-admin-guard test deleted a `sys_account` id that was never seeded and |
| 80 | +asserted it RESOLVED, to show the guard does not write-guard that object. It now |
| 81 | +deletes a REAL row — which states the same thing more strongly — and separately |
| 82 | +pins that a ghost id there is refused by the ENGINE rather than by the guard. |
| 83 | + |
| 84 | +**Scope.** By-id only. A `multi: true` predicate write matching zero rows still |
| 85 | +resolves "0 rows affected" — the same line both sibling paths draw. |
| 86 | + |
| 87 | +`@objectstack/runtime`: the sandbox error passthrough now also carries `status` |
| 88 | +alongside `code` and `fields`, so an error that names its own HTTP status keeps |
| 89 | +it across the QuickJS boundary. Without it the action surface answered the right |
| 90 | +diagnosis at the wrong status (`{ code: 'RECORD_NOT_FOUND', httpStatus: 400 }`); |
| 91 | +`domains/actions.ts` already honoured `.status` first — the number simply never |
| 92 | +arrived. A permission refusal thrown inside a body likewise keeps its 403 now |
| 93 | +instead of flattening to 400. |
0 commit comments