Repository navigation
Commit 5fcc794
fix(rest)!: POST /batch answers an update or delete op whose id names no row with the missing-id 400, not a 500 (#24820)
Fixes #24781
Clause-②: no (narrowing)
## What changed
`POST /api/v1/batch` (the cross-object transactional batch, and its
environment-scoped twin from the same registrar) checked up front that
an `update` / `delete` op names a row. But it tested only for an absent
or `null` id: `op.id == null && op.data?.id == null`. An id the engine
does not take as one passed that check and reached `ql.update` /
`ql.delete`. The engine's `Update requires an ID or options.multi=true`
/ `Delete requires an ID or options.multi=true` throw then answered as a
server fault.
The fix is in `packages/rest/src/batch-routes.ts`:
- `batchOpRowAddress(op, data)` is the one reading of the row an op
addresses. `update` reads `id`, else `data.id` (the arm's own `op.id ??
data?.id`). `delete` reads `id` alone, which is all its arm ever sent.
The up-front check and both write arms read it, so the check can no
longer pass an op that its arm then sends with no address.
- `namesNoRow(action, address)` judges that address with the engine's
own by-id predicates, `resolveEngineUpdateDispatch` /
`resolveEngineDeleteDispatch` from `@objectstack/metadata-core`. These
are the functions `ObjectQL.update` / `ObjectQL.delete` dispatch on, and
`@objectstack/rest` already depends on that package. A by-id address is
a truthy string or number.
- `isBatchRef` is now the one spelling of the `{ $ref }` test, shared
with the ref resolver. An update whose `data.id` references an earlier
create is not judged up front; the resolver still refuses an
unresolvable one with `BATCH_UNRESOLVED_REF`.
The refusal's code and sentence are the ones a missing id already got:
`400 VALIDATION_FAILED`, `Operation 'update' on 'OBJECT' requires an id`
(or `'delete'`).
## Contract
These are all the inputs whose answer changes. Before, the transaction
opened, the engine threw and the batch rolled back, so nothing was
written. Now the batch is refused before the transaction opens, and
nothing is written.
| op on cross-object `POST /batch` | before | after |
|---|---|---|
| `update`, `"id": ""` (the card) | `500 DATABASE_ERROR`, "Internal data
error" | `400 VALIDATION_FAILED`, `Operation 'update' on 'OBJECT'
requires an id` |
| `update`, no `id`, `data.id` is `""`, `0`, `false`, `true`, `{}` or
`[]` | `500 DATABASE_ERROR` | the same `400` |
| `update`, `"id": ""` beside a real `data.id` (`id` is read first) |
`500 DATABASE_ERROR` | the same `400` |
| `delete`, `"id": ""` (the card), with or without a `data.id` | `500
INTERNAL_ERROR`, "Internal server error" | `400 VALIDATION_FAILED`,
`Operation 'delete' on 'OBJECT' requires an id` |
| `delete`, no `id`, the record's id only in `data.id` | `400
INVALID_FILTER`, the engine's filter guard ("Filter comparand at
where.id is undefined …") | the same `400 VALIDATION_FAILED` "… requires
an id" |
| any of the above, where a later step used to answer first (the
per-object API-exposure gate's `404` / `405`, or another op in the
batch) | that later step's answer | this `400`. The id check runs before
the exposure check and the transaction, as it already did for a missing
id |
These answers do not move, and the pins below hold them as controls:
- A missing or `null` id gets the same `400`.
- An update by `id` or by `data.id`, and a delete by `id`, write as
before.
- An update whose `data.id` is `{ "$ref": 0 }` still resolves to the row
created by op 0.
- An unknown id answers `404 RECORD_NOT_FOUND`. So does a whitespace id
(`" "`), which the engine looks up like any other id.
- `atomic: false` still answers `400 BATCH_NOT_ATOMIC` first, because
that check comes before this one. With `atomic: true` and with `atomic`
omitted, the rule is the same.
The delete-by-`data.id` row is fixed in place under the bounded
exemption. It is the same defect class (the up-front check passed an op
that its arm sent with no address), in the same check, the same file and
the same gate family. Leaving it out would mean the check knowingly
accepts `data.id` for a delete while the arm ignores it.
## Which ids count as missing
The rule is "refuse what can never address a record", judged by the
engine. Measured with `resolveEngine*Dispatch` and with the `500`s above
on `main`:
- `""`, `0`, booleans, objects and arrays are never a by-id address, so
they are refused.
- A whitespace string is a by-id address. The engine and the driver look
it up, and the answer is an honest `404`, so it is not refused here.
- `null` in `data.id` was already refused.
- `op.id` is `z.string().optional()` in the spec, so a non-string `id`
is still refused by the schema (`Invalid batch request`) before this
step.
This is not id-format validation.
## Other doors that serve the operation
- The per-object `POST /data/:object/batch`, `updateMany` and
`deleteMany` have no REST-level up-front id check. The protocol's row
check is `if (!record.id) throw rowRequiredIdError(…)`, a falsy test.
Measured with the same real-engine harness:
- `update` / `delete` rows with `"id": ""` answer a per-row
`VALIDATION_FAILED`, "Record id is required for update" (or delete).
- `deleteMany` with `ids: [""]` answers `400 VALIDATION_FAILED`.
- A whitespace id answers a per-row `404`.
These doors do not share the gap, so there is no shared helper.
- `@objectstack/runtime`'s dispatcher has no `/batch` branch.
`domains/data.ts` routes only `query`, and `http-dispatcher.ts` reports
`transactionalBatch: false` for exactly that reason. There is no twin.
## Tests
The new `packages/rest/src/rest-batch-blank-id.test.ts` runs a real
engine (`ObjectQL` + sqlite `SqlDriver`), the real protocol and the real
route, with `objectQLProvider` wired. It has:
- 11 refusal rows. Each asserts the whole `{ status, body }` (status,
`code` and sentence), that `engine.transaction` was never called, and
that the seeded row is unchanged.
- One row proving that an earlier `create` in the same batch is not
written.
- 8 controls, the ones listed above.
Red and green:
- On `main` `a43405d59`, the pin file gave **12 failed / 8 passed**.
Update rows answered `500 DATABASE_ERROR`, delete `""` rows `500
INTERNAL_ERROR`, and delete-by-`data.id` `400 INVALID_FILTER`.
- Ablation 1, with the fix committed: `scripts/ablation-replace.mjs`
swapped the check back to `op.id == null && op.data?.id == null`. The
landing was proven by the anchor count going 1 → 0 and the blob going
`c2ad0e6dac7e` → `7be150e2b515`. Result: **12 failed / 8 passed**, the
same rows.
- Ablation 2: the `isBatchRef` carve-out deleted. Exactly the `$ref`
control failed (`expected 400 to be 200`).
- Both ablations were restored with blob == HEAD and `git diff HEAD`
empty.
- After the fix: pin file 20/20. `pnpm --filter @objectstack/rest exec
vitest run --project local --maxWorkers=2` on `0d4ea3e5b` (after merging
`origin/main` `307ce4c4b`): **305 files passed, 6144 tests passed, 326
skipped**. `pnpm --filter @objectstack/rest typecheck` (tsc plus the
test-layer check) exited 0.
## Gates (local, on `0d4ea3e5b`)
- `node scripts/pm/dispatch-gates.mjs --commands` derived 67 commands
for this diff against merge base `307ce4c4b`. All 67 ran and exited 0,
with each exit code captured before any pipe. The `--ran` reconciliation
reports 67 derived, 67 run, 0 NOT-MEASURED, 0 UNRUN.
- `pnpm lint` (`eslint . --no-inline-config`), the family the derivation
does not name: exit 0.
- `test:repo` (the `repo` vitest project) was not run locally. The diff
touches none of its five files, and CI's `Test Core` runs it.
## Changeset
`@objectstack/rest`: **minor**, carrying this PR's narrowing declaration
line and an ADR-0087 `not-required (no-migration-prescription)`
disposition. The dispatch asked for `patch`. Measured on a throwaway
commit object (no branch or tree change): `check-changeset-no-major.mjs`
with this PR's declaration line and a `patch` grade on
`@objectstack/rest` exits 1 ("clause-② declares `no (narrowing)` while
no moved package is graded `minor` or above"). With `minor` it passes. A
narrowing ships `minor` in the launch window.
## Acceptance notes
- The engine's `Update requires an ID…` / `Delete requires…` throws
still answer `500` wherever another caller reaches them, with
`DATABASE_ERROR` and `INTERNAL_ERROR` disagreeing between the two verbs.
That mapping is held off here: it is the door-parity region of #24768,
which carries it.
- The spec's `CrossObjectBatchOperationSchema.id` is
`z.string().optional()` with no `.min(1)`, so `""` passes the schema and
reaches this check. Tightening the schema would move the refusal to
`Invalid batch request` with a different sentence. That is
`packages/spec`, held off, and not a defect after this fix. Noted, not
filed; no carrier.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01LmjU2x1faLZ39DVTFyVca6)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 99e4eaa commit 5fcc794
3 files changed
Lines changed: 280 additions & 7 deletions
File tree
- .changeset
- packages/rest/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
53 | 53 | | |
54 | 54 | | |
55 | 55 | | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
56 | 59 | | |
57 | 60 | | |
58 | 61 | | |
| |||
81 | 84 | | |
82 | 85 | | |
83 | 86 | | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
84 | 145 | | |
85 | 146 | | |
86 | 147 | | |
| |||
185 | 246 | | |
186 | 247 | | |
187 | 248 | | |
188 | | - | |
| 249 | + | |
189 | 250 | | |
190 | 251 | | |
191 | 252 | | |
| |||
201 | 262 | | |
202 | 263 | | |
203 | 264 | | |
204 | | - | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
205 | 268 | | |
206 | | - | |
| 269 | + | |
207 | 270 | | |
208 | 271 | | |
209 | 272 | | |
| |||
240 | 303 | | |
241 | 304 | | |
242 | 305 | | |
243 | | - | |
244 | | - | |
| 306 | + | |
| 307 | + | |
245 | 308 | | |
246 | 309 | | |
247 | 310 | | |
| |||
309 | 372 | | |
310 | 373 | | |
311 | 374 | | |
312 | | - | |
| 375 | + | |
313 | 376 | | |
314 | 377 | | |
315 | 378 | | |
| |||
330 | 393 | | |
331 | 394 | | |
332 | 395 | | |
333 | | - | |
| 396 | + | |
334 | 397 | | |
335 | 398 | | |
336 | 399 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
0 commit comments