Repository navigation
fix(rest)!: POST /batch answers an update or delete op whose id names no row with the missing-id 400, not a 500 - #24820
Conversation
…ing-id 400 The cross-object batch's up-front id check read `op.id == null && op.data?.id == null`, so an address the engine does not take as an id (the empty string, 0, a boolean, an object or an array in data.id) passed it and the engine's "requires an ID" throw answered 500. A delete naming its id only in data.id passed it too, though the delete arm never read data.id. The check now judges the address each arm hands the engine (batchOpRowAddress), with the engine's own by-id predicates, and both arms read that same address. Claude-Session: https://claude.ai/code/session_01LmjU2x1faLZ39DVTFyVca6 Co-authored-by: Claude <noreply@anthropic.com>
…owing Claude-Session: https://claude.ai/code/session_01LmjU2x1faLZ39DVTFyVca6 Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LmjU2x1faLZ39DVTFyVca6 Co-authored-by: Claude <noreply@anthropic.com>
📓 Docs Drift Check5 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. What this run could not see
Coarse fallback — 18 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 69459504060ff760f072257b064a5882ead74c0e && git checkout 69459504060ff760f072257b064a5882ead74c0e
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin c56115bf962cb301fb046c54680620d37e5ab2fb 0d4ea3e5b0f7e2109f5dc661b1ea02aede4c7acd && git checkout -B drift-repro c56115bf962cb301fb046c54680620d37e5ab2fb && git merge --no-ff 0d4ea3e5b0f7e2109f5dc661b1ea02aede4c7acd
node scripts/docs-audit/affected-docs.mjs --json c56115bf962cb301fb046c54680620d37e5ab2fb |
Contract reviewServed-tier: ① Derived judgmentsGate state at render (latest run per name on the head, 40 names).
Net diff vs The door and its twin. The new rule, read against the engine.
The PR body's
The unchanged list. Every item is right:
Other doors. Both are right and untouched by the diff:
Public surface.
Pins.
Noted, not a verdict item: ② Semver levelThe changeset is right on all three axes.
Level.
Disposition.
Prose. It states the shipped behaviour truly. The banner's "No request that succeeded before is refused now" is the ① invariant, and the whitespace-id, per-object-door and ordering sentences each match the cited code.
③ Boundary flags
Landing stays the owning seat's. This PASS arms nothing: the six Implemented-by: VERDICT: PASS Generated by Claude Code |
席位核对 · ACCEPT
Accepted deviations:
Out-of-scope dispositions (one line each):
Landing: through the merge queue by this seat once every check on this head reads Generated by Claude Code |
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 anupdate/deleteop names a row. But it tested only for an absent ornullid:op.id == null && op.data?.id == null. An id the engine does not take as one passed that check and reachedql.update/ql.delete. The engine'sUpdate requires an ID or options.multi=true/Delete requires an ID or options.multi=truethrow 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.updatereadsid, elsedata.id(the arm's ownop.id ?? data?.id).deletereadsidalone, 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/resolveEngineDeleteDispatchfrom@objectstack/metadata-core. These are the functionsObjectQL.update/ObjectQL.deletedispatch on, and@objectstack/restalready depends on that package. A by-id address is a truthy string or number.isBatchRefis now the one spelling of the{ $ref }test, shared with the ref resolver. An update whosedata.idreferences an earlier create is not judged up front; the resolver still refuses an unresolvable one withBATCH_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.
POST /batchupdate,"id": ""(the card)500 DATABASE_ERROR, "Internal data error"400 VALIDATION_FAILED,Operation 'update' on 'OBJECT' requires an idupdate, noid,data.idis"",0,false,true,{}or[]500 DATABASE_ERROR400update,"id": ""beside a realdata.id(idis read first)500 DATABASE_ERROR400delete,"id": ""(the card), with or without adata.id500 INTERNAL_ERROR, "Internal server error"400 VALIDATION_FAILED,Operation 'delete' on 'OBJECT' requires an iddelete, noid, the record's id only indata.id400 INVALID_FILTER, the engine's filter guard ("Filter comparand at where.id is undefined …")400 VALIDATION_FAILED"… requires an id"404/405, or another op in the batch)400. The id check runs before the exposure check and the transaction, as it already did for a missing idThese answers do not move, and the pins below hold them as controls:
nullid gets the same400.idor bydata.id, and a delete byid, write as before.data.idis{ "$ref": 0 }still resolves to the row created by op 0.404 RECORD_NOT_FOUND. So does a whitespace id (" "), which the engine looks up like any other id.atomic: falsestill answers400 BATCH_NOT_ATOMICfirst, because that check comes before this one. Withatomic: trueand withatomicomitted, the rule is the same.The delete-by-
data.idrow 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 acceptsdata.idfor 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*Dispatchand with the500s above onmain:"",0, booleans, objects and arrays are never a by-id address, so they are refused.404, so it is not refused here.nullindata.idwas already refused.op.idisz.string().optional()in the spec, so a non-stringidis 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,updateManyanddeleteManyhave no REST-level up-front id check. The protocol's row check isif (!record.id) throw rowRequiredIdError(…), a falsy test. Measured with the same real-engine harness:update/deleterows with"id": ""answer a per-rowVALIDATION_FAILED, "Record id is required for update" (or delete).deleteManywithids: [""]answers400 VALIDATION_FAILED.404.These doors do not share the gap, so there is no shared helper.
@objectstack/runtime's dispatcher has no/batchbranch.domains/data.tsroutes onlyquery, andhttp-dispatcher.tsreportstransactionalBatch: falsefor exactly that reason. There is no twin.Tests
The new
packages/rest/src/rest-batch-blank-id.test.tsruns a real engine (ObjectQL+ sqliteSqlDriver), the real protocol and the real route, withobjectQLProviderwired. It has:{ status, body }(status,codeand sentence), thatengine.transactionwas never called, and that the seeded row is unchanged.createin the same batch is not written.Red and green:
maina43405d59, the pin file gave 12 failed / 8 passed. Update rows answered500 DATABASE_ERROR, delete""rows500 INTERNAL_ERROR, and delete-by-data.id400 INVALID_FILTER.scripts/ablation-replace.mjsswapped the check back toop.id == null && op.data?.id == null. The landing was proven by the anchor count going 1 → 0 and the blob goingc2ad0e6dac7e→7be150e2b515. Result: 12 failed / 8 passed, the same rows.isBatchRefcarve-out deleted. Exactly the$refcontrol failed (expected 400 to be 200).git diff HEADempty.pnpm --filter @objectstack/rest exec vitest run --project local --maxWorkers=2on0d4ea3e5b(after mergingorigin/main307ce4c4b): 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 --commandsderived 67 commands for this diff against merge base307ce4c4b. All 67 ran and exited 0, with each exit code captured before any pipe. The--ranreconciliation 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(therepovitest project) was not run locally. The diff touches none of its five files, and CI'sTest Coreruns it.Changeset
@objectstack/rest: minor, carrying this PR's narrowing declaration line and an ADR-0087not-required (no-migration-prescription)disposition. The dispatch asked forpatch. Measured on a throwaway commit object (no branch or tree change):check-changeset-no-major.mjswith this PR's declaration line and apatchgrade on@objectstack/restexits 1 ("clause-② declaresno (narrowing)while no moved package is gradedminoror above"). Withminorit passes. A narrowing shipsminorin the launch window.Acceptance notes
Update requires an ID…/Delete requires…throws still answer500wherever another caller reaches them, withDATABASE_ERRORandINTERNAL_ERRORdisagreeing between the two verbs. That mapping is held off here: it is the door-parity region of A database NOT NULL refusal answers differently per door: the dispatcher /data twin answers 500 INTERNAL_ERROR, and REST batch/import/form/action rows name the column key (RestServer POST/PATCH /data answer 400 with the label after #24715) #24768, which carries it.CrossObjectBatchOperationSchema.idisz.string().optional()with no.min(1), so""passes the schema and reaches this check. Tightening the schema would move the refusal toInvalid batch requestwith a different sentence. That ispackages/spec, held off, and not a defect after this fix. Noted, not filed; no carrier.Generated by Claude Code