fix(objectql): the addressed row's primary key is not a dropped field (#8093) - #8143
Merged
Merged
Conversation
…#8093) `droppedFields` / `onFieldsDropped` reports fields the CALLER SUPPLIED and the engine REFUSED. On a by-id update a payload `id` equal to the bound row is the write's ADDRESS, not payload — it was refused nothing — yet it was reported as a `readonly` drop on every object declaring `id` as `readonly: true`, i.e. every platform object. Measured through the real ingress before fixing anything, because the report's premise was an inference off client source rather than a wire capture: a real ObjectQL + a real ObjectStackProtocolImplementation, driven with a body that provably has no `id` key, still answered `droppedFields:[{fields:["id"],reason:"readonly"}]`. The server manufactures it — `updateData` folds the path id into the payload (#6479) before the engine snapshots `suppliedValues`. The cost was not the toast: the console's recents trace runs on every org switch, so the warning channel was being trained to be ignored — the same failure #3431/#3794 fixed one field over (`updated_at`). Narrows the REPORT, not the strip: `id` still leaves the SET clause, and must, since a same-value primary-key write is rejected outright by stores with immutable primary keys. Self-scopes to single-record update by keying on equality with the bound key, so predicate/multi writes and the #6437 `primary_key` strips are untouched. `strictReadonlyWrites` moves with it by the #6437 derived-coverage contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014C8pAprWdmtecFsEprZax4
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 109 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
github-actions
Bot
added
documentation
This was referenced Aug 12, 2026
os-zhuang
marked this pull request as ready for review
August 12, 2026 18:14
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
2 participants
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 #8093
Step 1 — the card's own fork, settled by execution
The card named a fork in its own body and refused to guess past it:
That fork decides which repo eats the fix, so it was measured before any fix was written, by driving the real ingress rather than by reading source: a real
ObjectQL, a realObjectStackProtocolImplementation, and a request body this side constructs, so its contents are not in question.Verdict: the server half is real. The client half of the fork is disproved. The body carries no
idkey at all and the response still reproduces the card's payload byte for byte, includingreason: "readonly". Nothing needs to move torepo:objectui; the reporter's inference was correct and this isdomain:engine-core's to fix. The reporter's premise stands (premise_still_valid: true).The third block is the other half of the measurement: the driver's SET clause never contained
id. The strip is doing its job; only the report is untrue. That is what made a report-only fix the correct shape.The mechanism, end to end
{ value: items }—packages/data-objectstack/src/userState.ts:169, noid.metadata-protocol'supdateDatafolds the path id into the write payload:{ ...request.data, id: request.id }(RESTPATCH /data/:object/:id:请求体里的标量id压过路径:id,存在性探测/OCC 判在一行、写落在另一行、响应报第三个说法 #6479, so a bodyidcan no longer bind a row other than the one the URL, theIf-Matchcheck and the receipt all name). This fold is correct and is unchanged here.suppliedValuesfrom that payload — after the fold — so the address is now indistinguishable from something the caller typed.sys_user_preference.idisField.text({ label: 'Preference ID', required: true, readonly: true }), so the static-readonlystrip (security: 服务端 readonly 字段在 UPDATE 时未强制（可被用户上下文覆盖） #2948) removes it andreportDroppedFieldsreports it. "Preference ID" is the label the toast rendered as 「偏好设置 ID」.Why this is worth a round, and not a cosmetic toast fix
The file that triggers it already documents this exact failure mode as previously fixed.
userState.tsrecords whyupdated_atis no longer sent (#3431/#3794):This is that same drowning, one field over. The console's internal recents trace runs on every org switch, so the cost is not the toast — it is that the warning channel is being trained to be ignored. A user who learns the amber toast is noise will ignore the one that matters. The same file also states the invariant being violated: "
save()resolves without throwing, so UI mutations never surface a toast."The fix
Two lines of behaviour in
packages/objectql/src/engine.ts, under a comment that carries the reasoning:...consumed by the one place that decides what a drop is:
It narrows the report, not the strip.
idstill leaves the SET clause and must: a same-value primary-key write is a harmless no-op on SQL but an outright rejection on stores with immutable primary keys, and #6435's block already ruled that widening the strip to the truthy-scalar case "is a separate decision, not a rider here". The driver payload is byte-identical before and after, pinned in both test files.It self-scopes to single-record update by construction.
idis bound only on the by-id branch, so a predicate/multi write — which addresses nothing by key — still reports a caller-suppliedidin full. It cannot collide with the #6437primary_keystrips either: those fire only when the dispatch has already RULED the value is not an identifier, which is exactly when it cannot equal the bound key.Asked of
suppliedValues, not of the live payload, so the answer survives a hook rewriting the key mid-write — the same reason that snapshot carries values at all (#5591).strictReadonlyWritesmoves with it, and that is the contract rather than a side effect: the option covers "every droponFieldsDroppedreports", coverage DERIVED from the reported set (#6437), so a non-drop must not be a refusal. A strict caller doing a single-record update of a platform object previously gotERR_READONLY_FIELD_REJECTEDfor its own row's address.The invariant is now written down where the next reader looks for it —
WriteObservabilityOptionsinpackages/spec/src/contracts/data-engine.ts— including what is deliberately still reported, so the boundary is not re-derived from the implementation next time.Tests — both directions, because this is a narrowing
packages/rest/src/rest-update-path-id-not-a-dropped-field.test.ts(3 cases, the whole ingress: real engine, real protocol, real registered PATCH route) andpackages/objectql/src/engine-update-addressing-id-not-dropped.test.ts(10 cases, the engine seam).droppedFieldskey, noX-ObjectStack-Dropped-Fieldsheaderiddoes not ride alongprimary_keystrip, by-id and multiidis not readonlyidstill reaches the driverstrictReadonlyWritesdropsno longer listsidWithout the second row this change would be indistinguishable from "stop reporting dropped fields".
Reverse verification — prediction, then what actually happened
Predicted, written before the run: delete the
!(idAddressesThisRow && k === 'id')term and the four "NONE / ONLY" rows go red, while everyprimary_keyrow, the not-readonly row and both strict rows stay green.Measured: 6 failed / 4 passed — the prediction was wrong about the two strict rows, and how it was wrong is worth keeping. Both went red as well: reverting the report also restores the refusal, and the strict refusal's
dropsbreakdown comes back as['locked_note','id']. That is the #6437 derived-coverage contract behaving exactly as documented — the quiet and loud halves of the seam cannot move apart, so un-reporting the address un-refuses it in the same edit. Predicting them independent was the error, not the code. The 4 rows that did stay green are precisely the ones this change claims not to touch: bothprimary_keystrips, the predicate/multi report, and the object whoseidis not readonly.Verification
@objectstack/objectqlfull suite@objectstack/restfull suite@objectstack/specfull suite@objectstack/service-automation(the named consumer of this seam)typecheck(objectql, rest, spec)pnpm check:query-options-erasurepnpm check:type-check-debt --re-measurecheck:type-check-debt --re-measurefirst came back red at +1 (objectql) and +2 (rest) from the two new test files. Fixed at the source rather than by moving a baseline — a missingpackageIdargument toregistry.registerObjectin each, plus a missing.jsextension on a relative import undernodenext. No baseline was raised and--lowerwas not run, although the gate reported nine entries that could be lowered.Gates derived at implementation time from the real file surface via
node scripts/pm/dispatch-gates.mjs, all green:check:adr-anchors,check:authz-resolver,check:changeset-gate-self-tests,check:cross-package-test-inputs,check:doc-formula-expressions,check:docs-audit-scope,check:durability-log-level,check:engine-double-contract,check:i18n,check:merge-driver,check:meta-type-normalized,check:objectui-changeset,check:release-body,check:spec-parsed-alias,check:stack-collection-maps,check:nul-bytes,check-changeset-no-major.mjs,check-engine-split-ratio.mjs.One derived gate is red and is not from this branch:
check:objectui-pin-freshreports.objectui-shastale relative to objectui main. That file is not in this diff (git diff --name-only origin/maindoes not list it; it was last touched by #7916), and the gate matched only because this PR adds a changeset. Its remedy is a maintainer pin bump (scripts/bump-objectui.sh), the recurring task last filed as the now-closed #6089.Scope
The card's suggested fix had two halves and only the first is here:
Filed rather than fixed here: #8141 — the read-only strip's server-side
warnstill calls the same addressingida forged caller write, on every single-record update of a platform object. Measured after this fix (droppedFields: null,warn count: 1). It is the same untruth in the log channel, but fixing it means changing a strip helper shared by three call sites, and the honest alternative is #6435's explicitly separate decision — so it goes to triage with options rather than riding along here.Cross-seat note
The producer of the
droppedFieldspayload ispackages/objectql/src/engine.ts—domain:engine-core's own package, located by symbol as the card required. It is notpackages/objectql/src/metadata-facade.ts, so there is no collision with sibling card #7378.packages/metadata-protocol/src/protocol.ts(seat #6367,domain:metadata) is the ingress that folds the path id into the payload and is therefore the trigger — but it is not modified by this PR, deliberately: that fold is #6479's cross-row-write fix and is correct. The engine is where the accounting is wrong, and fixing it there also fixes every other ingress (batch, GraphQL, flowupdate_record) in one place. The only other package touched ispackages/spec, and only by a doc comment on the contract this change enforces.Generated by Claude Code