diff --git a/.changeset/update-addressing-id-not-a-dropped-field.md b/.changeset/update-addressing-id-not-a-dropped-field.md new file mode 100644 index 0000000000..ea36c21128 --- /dev/null +++ b/.changeset/update-addressing-id-not-a-dropped-field.md @@ -0,0 +1,70 @@ +--- +"@objectstack/objectql": patch +"@objectstack/spec": patch +--- + +fix(objectql): a single-record update no longer reports the addressed row's own primary key as a dropped field (#8093) + +`droppedFields` / `onFieldsDropped` has one declared meaning: fields the CALLER +SUPPLIED and the engine REFUSED. On the by-id branch a payload `id` that equals +the row the call is bound to is the write's ADDRESS, not part of its payload — +it was refused nothing — and it was being reported as a `readonly` drop on every +object that declares `id` as `readonly: true`, which is every platform object. + +**Measured through the real ingress before fixing anything**, because the +report's own premise was an inference off the client source rather than a wire +capture. A real `ObjectQL` + a real `ObjectStackProtocolImplementation`, driven +with a body that provably has no `id` key +(`hasOwnProperty(body,'id') === false`, body keys `["value"]`): + +``` +PATCH /data/sys_user_preference/4mekbFDEhx0QgC85 body: {"value":[…]} +→ 200 droppedFields:[{object:"sys_user_preference",fields:["id"],reason:"readonly"}] +``` + +The server manufactures the key the caller never sent. `metadata-protocol`'s +`updateData` folds the path id INTO the write payload (`{ ...request.data, id: +request.id }`, #6479 — so a body `id` cannot bind a row other than the one the +URL, the `If-Match` check and the receipt all name). That fold is correct and is +unchanged here; it simply lands in `data` BEFORE the engine snapshots +`suppliedValues`, after which the address is indistinguishable from something +the caller typed, and the static-`readonly` strip (#2948) drops and reports it. + +**The cost was not cosmetic, which is why this is worth a round.** The console's +internal "recent items" trace runs on every org switch, so every org switch +popped a user-facing amber warning toast naming a field the user never touched. +The damage is that the warning channel gets TRAINED TO BE IGNORED — a user who +learns the amber toast is noise will ignore the one that matters. The identical +failure mode is already on record one field over: #3431 / #3794 stopped +`userState.ts` sending `updated_at` because doing so "made every +recents/favorites write pop a scary warning about a field the user never +touched, drowning the real signal the toast exists for." + +**This narrows the REPORT, not the strip.** `id` still 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 payload handed to the driver is byte-identical before and +after — pinned in both test files. + +**Scope, self-enforced by construction.** The exclusion is keyed on equality +with the BOUND key, so it reaches only single-record update: a predicate/multi +write addresses nothing by key and still reports a caller-supplied `id` in full, +and the `primary_key` strips (#6437) cannot collide with it, since those fire +only when the dispatch has already RULED the value is not an identifier — +exactly when it cannot equal the bound key. + +⚠️ **`strictReadonlyWrites` moves with it, and that is the contract rather than +a side effect.** The option covers "every drop `onFieldsDropped` reports" — +coverage DERIVED from the reported set (#6437) — so a non-drop must not be a +refusal either. A strict caller doing a single-record update of a platform +object previously got `ERR_READONLY_FIELD_REJECTED` for its own row's address, +and a strict refusal for a genuinely forged read-only field previously listed +`id` beside it in `drops`. Both are corrected. The reverse verification measured +this directly: reverting the report also restores the refusal — the quiet and +loud halves cannot move apart. + +The invariant is now stated where the next reader looks for it — +`WriteObservabilityOptions` in `spec/src/contracts/data-engine.ts` — including +what is deliberately still reported, so the boundary is not re-derived from the +implementation next time. diff --git a/packages/objectql/src/engine-update-addressing-id-not-dropped.test.ts b/packages/objectql/src/engine-update-addressing-id-not-dropped.test.ts new file mode 100644 index 0000000000..8113d9cf77 --- /dev/null +++ b/packages/objectql/src/engine-update-addressing-id-not-dropped.test.ts @@ -0,0 +1,290 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// objectstack#8093 — a single-record update must not report the row's OWN +// primary key as a field the caller supplied and the engine refused. +// +// ## What was wrong +// +// `droppedFields` / `onFieldsDropped` has one declared meaning: fields the +// CALLER SUPPLIED and the engine REFUSED. On the by-id branch an `id` that +// names the row being written was refused nothing — it is the ADDRESS of the +// write, not part of its payload — and it was being reported as a `readonly` +// drop on every object whose `id` carries `readonly: true` (every platform +// object). +// +// The reported route in: the REST ingress folds the path id into the write +// payload (`metadata-protocol`'s `updateData`, #6479, so a body `id` cannot +// bind a row other than the one the URL / OCC / receipt all name). That fold +// lands in `data` before the engine's `suppliedValues` snapshot, so the address +// became indistinguishable from something the caller typed. Measured on `main` +// through the real ingress, with a body containing no `id` key at all: +// +// PATCH /data/sys_user_preference/4mekbFDEhx0QgC85 body: {"value":[…]} +// → 200 droppedFields:[{object:…,fields:["id"],reason:"readonly"}] +// +// The console's recents trace runs on every org switch, so that answer popped a +// user-facing amber warning toast on every org switch, about a field the user +// never touched. The cost is not the toast — it is that the warning channel is +// trained to be ignored, so the drop that DOES matter is ignored with it. Same +// failure mode as #3431 / #3794 one field over (`updated_at`). +// +// ## Predicted table, written BEFORE the first run +// +// | case | event | driver SET clause | +// |--------------------------------------------------------|-------------------------------|-----------------------| +// | by-id, readonly `id` = the addressed row | NONE | no `id` (unchanged) | +// | canonical `update(obj,{id,…})` spelling, readonly `id` | NONE | no `id` (unchanged) | +// | by-id, a readonly field the caller really supplied | {fields:['locked_note'],readonly} | that field absent | +// | both at once | {fields:['locked_note']} ONLY | neither in SET | +// | by-id, ruled-non-id `data.id` (#6262/#6435 strip) | {fields:['id'],primary_key} | no `id` | +// | MULTI, ruled-non-id `data.id` | {fields:['id'],primary_key} | no `id` | +// | object whose `id` is NOT readonly | NONE (as before) | `id` PRESENT | +// | strict + addressing id only | no refusal | write happens | +// | strict + a really-supplied readonly field | REFUSED, unchanged | no driver call | +// +// The last three rows are the ones that make this file able to fail. The fix +// NARROWS what is reported, so without a case that still demands a report — and +// one that pins the strip's own behaviour as untouched — "stop reporting +// dropped fields" and "stop reporting the address" look identical. +// +// ## Reverse verification — prediction, then what actually happened +// +// PREDICTED, before the run: delete the `!(idAddressesThisRow && k === 'id')` +// term from `reportDroppedFields` in `engine.ts` and the four "NONE / ONLY" +// rows above gain an `id` entry and go red, while every `primary_key` row, 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 the way it was wrong is worth keeping. Both went red too: +// +// * "does NOT refuse a write whose only drop was the address" — reverting the +// report ALSO restores a refusal, so under `strictReadonlyWrites` the write +// throws instead of committing; +// * "still refuses a really-supplied read-only field" — the refusal survives, +// but its `drops` breakdown comes back as `['locked_note','id']`. +// +// That is the #6437 derived-coverage contract doing exactly what it says: +// `strictReadonlyWrites` covers "every drop `onFieldsDropped` reports", so the +// quiet and loud halves cannot move apart — un-reporting the address un-refuses +// it in the same edit. Predicting them independent was the error. The 4 rows +// that DID stay green are the ones the fix claims not to touch: both +// `primary_key` strips, the predicate/multi report, and the object whose `id` +// is not readonly. + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; +import type { DroppedFieldsEvent } from '@objectstack/spec/data'; + +const silentLogger: any = (() => { + const l: any = { + debug() {}, info() {}, error() {}, trace() {}, fatal() {}, warn() {}, + child() { return l; }, + }; + return l; +})(); + +interface DriverWrite { + readonly fn: 'update' | 'updateMany'; + readonly id?: unknown; + /** A COPY — the engine keeps mutating its own payload after the call. */ + readonly data: Record; +} + +function makeRecordingDriver() { + const writes: DriverWrite[] = []; + const row = { id: 'rec_1', value: 'v0', locked_note: 'n0', title: 't0' }; + const driver: any = { + name: 'recording', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find() { return [{ ...row }]; }, + async findOne() { return { ...row }; }, + async create(_o: string, data: Record) { return { id: 'rec_1', ...data }; }, + async update(_o: string, id: string, data: Record) { + writes.push({ fn: 'update', id, data: { ...data } }); + return { ...row, ...data, id }; + }, + async updateMany(_o: string, _ast: unknown, data: Record) { + writes.push({ fn: 'updateMany', data: { ...data } }); + return 2; + }, + async delete() { return true; }, + async deleteMany() { return 0; }, + async count() { return 1; }, + async bulkCreate() { return []; }, async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, writes }; +} + +/** + * `readonlyId: true` mirrors every platform object — `sys_user_preference`'s + * `id` is `Field.text({ label: 'Preference ID', required: true, readonly: true })`, + * and "Preference ID" is the label the amber toast rendered. + */ +async function makeEngine(readonlyId: boolean) { + const engine = new ObjectQL({ logger: silentLogger }); + const { driver, writes } = makeRecordingDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject({ + name: 'pref', + fields: { + id: { name: 'id', label: 'Preference ID', type: 'text', primaryKey: true, ...(readonlyId ? { readonly: true } : {}) }, + value: { name: 'value', type: 'text' }, + locked_note: { name: 'locked_note', type: 'text', readonly: true }, + title: { name: 'title', type: 'text' }, + }, + } as any, 'test'); + return { engine, writes }; +} + +/** Run a write with a listener attached; return the events and driver writes. */ +async function observe( + data: unknown, + options: Record = {}, + opts: { readonlyId?: boolean } = {}, +) { + const { engine, writes } = await makeEngine(opts.readonlyId !== false); + const events: DroppedFieldsEvent[] = []; + await engine.update('pref', data as any, { + ...options, + onFieldsDropped: (e: DroppedFieldsEvent) => events.push(e), + } as any); + return { events, writes }; +} + +describe('#8093 — the addressed row\'s primary key is not a dropped field', () => { + it('the REST ingress shape reports NOTHING: {…, id} + where.id, `id` readonly', async () => { + // Byte-for-byte what `metadata-protocol`'s `updateData` builds for + // `PATCH /data/pref/rec_1` with the body `{ value: 'v1' }`: + // `{ ...request.data, id: request.id }` plus `where: { id: request.id }`. + const { events, writes } = await observe( + { value: 'v1', id: 'rec_1' }, + { where: { id: 'rec_1' } }, + ); + expect(events).toEqual([]); + // The strip is UNCHANGED — this fix narrows the report, never the write. + // `id` still never reaches the SET clause (a same-value primary-key write + // is a no-op on SQL but a rejection on stores with immutable keys). + expect(writes.map((w) => w.fn)).toEqual(['update']); + expect(writes[0].data).toEqual({ value: 'v1' }); + expect(writes[0].id).toBe('rec_1'); + }); + + it('the canonical ObjectQL by-id spelling `update(obj, { id, ...fields })` reports nothing either', async () => { + // The engine's own documented by-id call (quoted in the #6435 remedy + // prose). Its `id` is the address by definition, whatever `where` says. + const { events, writes } = await observe({ id: 'rec_1', value: 'v1' }); + expect(events).toEqual([]); + expect(writes[0].data).toEqual({ value: 'v1' }); + }); + + it('a read-only field the caller REALLY supplied is still reported, unchanged', async () => { + // The counter-direction. Without this the fix is indistinguishable from + // "stop reporting dropped fields". + const { events, writes } = await observe( + { value: 'v1', locked_note: 'forged', id: 'rec_1' }, + { where: { id: 'rec_1' } }, + ); + expect(events).toEqual([{ object: 'pref', fields: ['locked_note'], reason: 'readonly' }]); + expect(writes[0].data).toEqual({ value: 'v1' }); + }); + + it('a real refusal does not drag the address into its field list', async () => { + const { events } = await observe( + { locked_note: 'forged', id: 'rec_1' }, + { where: { id: 'rec_1' } }, + ); + expect(events.flatMap((e) => e.fields)).not.toContain('id'); + expect(events.flatMap((e) => e.fields)).toEqual(['locked_note']); + }); + + it('an object whose `id` is NOT readonly is unaffected in both channels', async () => { + // Nothing was ever stripped or reported here; the fix must not invent a + // strip. The `id` still rides into the SET clause exactly as before — + // widening the strip to that case is #6435's explicitly separate decision. + const { events, writes } = await observe( + { value: 'v1', id: 'rec_1' }, + { where: { id: 'rec_1' } }, + { readonlyId: false }, + ); + expect(events).toEqual([]); + expect(writes[0].data).toEqual({ value: 'v1', id: 'rec_1' }); + }); + + it('the #6437 `primary_key` strip is untouched on the by-id branch', async () => { + // A ruled-non-id `data.id` is NOT an address — the dispatch said so — and + // the exclusion cannot reach it: it is keyed on equality with the bound + // key, which a ruled-non-id value never has. + const { events, writes } = await observe( + { id: { $in: ['a', 'b'] }, value: 'v1' }, + { where: { id: 'rec_1' } }, + ); + expect(events).toEqual([{ object: 'pref', fields: ['id'], reason: 'primary_key' }]); + expect(writes[0].data).toEqual({ value: 'v1' }); + }); + + it('the #6437 `primary_key` strip is untouched on the MULTI branch', async () => { + const { events, writes } = await observe( + { id: { $in: ['a', 'b'] }, value: 'v1' }, + { multi: true }, + ); + expect(events).toEqual([{ object: 'pref', fields: ['id'], reason: 'primary_key' }]); + expect(writes.map((w) => w.fn)).toEqual(['updateMany']); + }); + + it('a predicate write still reports a really-supplied read-only field', async () => { + // The multi branch addresses rows by predicate, so nothing there is an + // address-in-the-payload; its accounting must not have moved. + const { events } = await observe( + { locked_note: 'forged', value: 'v1' }, + { multi: true, where: { title: 't0' } }, + ); + expect(events).toEqual([{ object: 'pref', fields: ['locked_note'], reason: 'readonly' }]); + }); +}); + +describe('#8093 — strictReadonlyWrites stays consistent with what is reported', () => { + it('does NOT refuse a write whose only "drop" was the address', async () => { + // `strictReadonlyWrites` is contracted as covering "every drop + // `onFieldsDropped` reports" — a set DERIVED from the reported set (#6437). + // So a non-drop must not be a refusal either, or the loud half would refuse + // every single-record PATCH of a platform object. + const { engine, writes } = await makeEngine(true); + const res = await engine.update( + 'pref', + { value: 'v1', id: 'rec_1' } as any, + { where: { id: 'rec_1' }, strictReadonlyWrites: true } as any, + ); + expect(res).toBeTruthy(); + expect(writes.map((w) => w.fn)).toEqual(['update']); + expect(writes[0].data).toEqual({ value: 'v1' }); + }); + + it('still refuses a really-supplied read-only field, before any driver call', async () => { + const { engine, writes } = await makeEngine(true); + let err: any; + try { + await engine.update( + 'pref', + { value: 'v1', locked_note: 'forged', id: 'rec_1' } as any, + { where: { id: 'rec_1' }, strictReadonlyWrites: true } as any, + ); + } catch (e) { err = e; } + expect(err).toBeDefined(); + // Asserted on the ENVELOPE, not on the throw. The pair this class actually + // contracts is `code` + `drops`, not `code` + `status`: `strictReadonlyWrites` + // lives on `WriteObservabilityOptions`, which is not the serializable options + // bag, so no wire caller can reach this refusal and it carries no HTTP status + // to assert (`contracts/data-engine.ts`, "In-process only — what a REMOTE + // caller observes"). `drops` is the documented breakdown a caller reads after + // catching the one stable code (#6437). + expect(err.code).toBe('ERR_READONLY_FIELD_REJECTED'); + expect(err.drops).toEqual([{ object: 'pref', fields: ['locked_note'], reason: 'readonly' }]); + // The refusal names the field the caller really sent — and not the address. + expect(String(err.message)).toContain('locked_note'); + expect(String(err.message)).not.toContain("'id'"); + expect(writes).toHaveLength(0); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index cc44c88834..7979c9b413 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -8149,9 +8149,62 @@ export class ObjectQL implements IObjectQLEngine { // read-only ones are (don't half-apply my payload), and the refusal error // composes its wording from `drops` so it never calls a stripped `id` // read-only. Route a new strip through here ⇒ own both halves. + // + // [#8093] ...and one thing is NOT a drop: the row's own primary key, when + // it is the address of the row this call is already writing. `droppedFields` + // has one declared meaning — fields the CALLER SUPPLIED and the engine + // REFUSED. An `id` that names the targeted row was refused nothing; it did + // its job. ADDRESSING IS NOT PAYLOAD. + // + // How a caller who sent no `id` gets one reported anyway: the REST ingress + // folds the path id INTO the write payload (`metadata-protocol`'s + // `updateData`: `{ ...request.data, id: request.id }`, #6479 — so a body + // `id` can no longer bind a different row than the one the URL, the OCC + // check and the receipt all name). That fold is correct and stays. But it + // lands in `data` BEFORE the `suppliedValues` snapshot above, so from here + // down the address is indistinguishable from something the caller typed — + // and on an object whose `id` is declared `readonly: true` (as platform + // objects' are), the static-`readonly` strip below then drops it and + // reports it. Measured on `main` through the real ingress: + // `PATCH /data/sys_user_preference/` with the body `{"value":[...]}` — + // no `id` key in it — answered 200 carrying + // `droppedFields:[{fields:["id"],reason:"readonly"}]`. + // + // What that cost is not cosmetic. The console's internal "recent items" + // trace runs on every org switch, so every org switch popped a user-facing + // amber warning toast naming a field the user never touched. The damage is + // that the warning channel gets TRAINED TO BE IGNORED — a user who learns + // the amber toast is noise will ignore the one that matters. The identical + // failure mode is already on record one field over: #3431 / #3794 stopped + // `userState.ts` sending `updated_at` because doing so "made every + // recents/favorites write pop a scary warning about a field the user never + // touched, drowning the real signal the toast exists for." + // + // Deliberately the REPORT and not the strip. `id` still 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 payload handed to + // the driver is byte-identical before and after this change. + // + // Self-scoping to SINGLE-RECORD update by construction: `id` is bound only + // on the by-id branch, so a predicate/multi write — where nothing addresses + // a row by key — is untouched, and a caller-supplied `id` there is still + // reported. It cannot collide with the `primary_key` strips either: those + // fire only when the dispatch has ALREADY RULED the value is not a primary + // key, which is exactly when it cannot equal the bound key. + // + // Asked of `suppliedValues`, never of the live payload: this is a question + // about what the CALLER submitted, and the answer must survive a hook + // rewriting the key mid-write — the same reason that snapshot carries + // values at all (#5591). const onFieldsDropped = options?.onFieldsDropped; const strictReadonlyWrites = options?.strictReadonlyWrites === true; const strictDrops: DroppedFieldsEvent[] = []; + const idAddressesThisRow = + id !== undefined && id !== null + && Object.prototype.hasOwnProperty.call(suppliedValues, 'id') + && Object.is(suppliedValues.id, id); const reportDroppedFields = ( before: Record | null | undefined, after: Record | null | undefined, @@ -8159,7 +8212,10 @@ export class ObjectQL implements IObjectQLEngine { ): void => { if ((!onFieldsDropped && !strictReadonlyWrites) || before === after || !before) return; const afterObj = (after ?? {}) as Record; - const fields = Object.keys(before).filter((k) => !(k in afterObj)); + const fields = Object.keys(before).filter( + // [#8093] The address the caller wrote to is not a field it lost. + (k) => !(k in afterObj) && !(idAddressesThisRow && k === 'id'), + ); if (fields.length === 0) return; if (strictReadonlyWrites) { strictDrops.push({ object, fields, reason }); diff --git a/packages/rest/src/rest-update-path-id-not-a-dropped-field.test.ts b/packages/rest/src/rest-update-path-id-not-a-dropped-field.test.ts new file mode 100644 index 0000000000..50b323eb28 --- /dev/null +++ b/packages/rest/src/rest-update-path-id-not-a-dropped-field.test.ts @@ -0,0 +1,266 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#8093] `PATCH /data/:object/:id` must not report the row's OWN primary key +// as a field the caller supplied and the engine refused. +// +// ## The reported symptom +// +// Every org switch popped a user-facing amber toast — 「已保存，但部分字段未生效 +// / 以下字段为只读，未生效: 偏好设置 ID」 — behind the console's internal +// `ui.recent` preference write: +// +// PATCH /api/v1/data/sys_user_preference/4mekbFDEhx0QgC85 → 200 +// { …, "droppedFields": [ { "fields": ["id"], "reason": "readonly" } ] } +// +// `id` is the record's primary key, carried in the URL PATH. The client's body +// is `{ value: items }` and carries no `id` at all. +// +// ## Why this file drives the whole ingress instead of unit-testing the engine +// +// The card named a FORK it could not settle from the client source alone: the +// request body was never captured on the wire, so "the server folds the path id +// into the candidate write set" was an INFERENCE, and the alternative — the +// running build's client actually sending `id` — would have made this a client +// card in another repo. A fake engine cannot answer that; only executing the +// real ingress can. So: a REAL `ObjectQL`, a REAL +// `ObjectStackProtocolImplementation`, the REAL registered PATCH route, and a +// body that provably contains no `id` because this file writes it. +// +// Measured on unfixed `main` (recorded in the PR body): the body below carries +// no `id`, and the response still comes back with +// `droppedFields:[{fields:['id'],reason:'readonly'}]`. The server manufactures +// it — `updateData` folds `request.id` into the write payload (#6479, so a body +// `id` cannot bind another row), the engine snapshots that payload as +// "caller-supplied", and the static-`readonly` strip then reports the row's own +// address as a refused write. The client half of the fork is disproved: the +// defect reproduces with a body that never had an `id` in it. +// +// ## Both directions are pinned +// +// The fix NARROWS what gets reported, so the counter-case is not optional: a +// read-only field the caller really did supply must still be reported. Without +// it this file cannot tell the fix apart from "stop reporting dropped fields". + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from './rest-server.js'; + +const DATA_COLLECTION = '/api/v1/data/:object'; +const DATA_ITEM = '/api/v1/data/:object/:id'; + +/** + * `sys_user_preference`'s real shape, field-for-field on the parts that matter + * (`packages/platform-objects/src/identity/sys-user-preference.object.ts`): + * `id` is `readonly: true` with label "Preference ID" — the label the toast + * rendered as 「偏好设置 ID」 — and `created_at` / `updated_at` are read-only + * too. `value` is the JSON column the recents trace actually writes. + */ +const PREFERENCE = { + name: 'rp_user_preference', + label: 'User Preference', + fields: { + id: { name: 'id', label: 'Preference ID', type: 'text', primaryKey: true, required: true, readonly: true }, + created_at: { name: 'created_at', label: 'Created At', type: 'datetime', readonly: true }, + user_id: { name: 'user_id', label: 'User', type: 'text', required: true }, + key: { name: 'key', label: 'Key', type: 'text', required: true }, + value: { name: 'value', label: 'Value', type: 'json' }, + }, +}; + +function createMockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined), + }; +} + +function makeRes() { + const res: Record = { statusCode: 200, body: undefined, headers: {} as Record }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: unknown) => { res.body = b; return res; }); + res.header = vi.fn((k: string, v: string) => { (res.headers as Record)[k] = v; return res; }); + res.setHeader = vi.fn(); res.write = vi.fn(); res.end = vi.fn(); res.send = vi.fn(); + return res; +} + +/** In-memory driver with `RETURNING *` write semantics and copy-on-read. */ +function memoryDriver() { + const rows = new Map>>(); + const table = (o: string) => { + let t = rows.get(o); + if (!t) { t = new Map(); rows.set(o, t); } + return t; + }; + /** Every payload the driver was handed, so the SET clause is inspectable. */ + const writes: Array<{ id: string; data: Record }> = []; + let seq = 0; + const matches = (row: Record, where: unknown) => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where as Record)) { + if (k.startsWith('$')) continue; + const want = (v && typeof v === 'object' && '$eq' in (v as Record)) + ? (v as Record).$eq + : v; + if ((row[k] ?? null) !== (want ?? null)) return false; + } + return true; + }; + const driver = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: { where?: unknown }) { + return Array.from(table(object).values()).filter((r) => matches(r, ast?.where)).map((r) => ({ ...r })); + }, + async findOne(object: string, ast: { where?: unknown }) { + for (const r of table(object).values()) if (matches(r, ast?.where)) return { ...r }; + return null; + }, + async create(object: string, data: Record) { + seq += 1; + const id = (data.id as string) ?? `r_${seq}`; + const row = { ...data, id }; + table(object).set(id, row); + return { ...row }; + }, + async update(object: string, id: string, data: Record) { + writes.push({ id, data: { ...data } }); + const t = table(object); + const cur = t.get(id); + if (!cur) return null; + const next = { ...cur, ...data, id }; + t.set(id, next); + return { ...next }; + }, + async delete(object: string, id: string) { return table(object).delete(id); }, + async upsert(object: string, data: Record) { return this.create(object, data); }, + async count(object: string, ast: { where?: unknown }) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, list: Record[]) { + const out = []; + for (const r of list) out.push(await this.create(object, r)); + return out; + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, writes }; +} + +async function bootRest() { + const engine = new ObjectQL(); + const { driver, writes } = memoryDriver(); + engine.registerDriver(driver as never, true); + await engine.init(); + engine.registry.registerObject(PREFERENCE as never, 'test'); + const protocol = new ObjectStackProtocolImplementation(engine as never); + const rest = new RestServer( + createMockServer() as never, + protocol as never, + { api: { requireAuth: false } } as never, + ); + // A NON-system caller: the console's browser session, i.e. the one the + // static-`readonly` strip actually runs for. + (rest as unknown as { resolveExecCtx: () => Promise }).resolveExecCtx = + async () => ({ userId: 'u1' }); + rest.registerRoutes(); + return { rest, writes }; +} + +async function call( + rest: Awaited>['rest'], + method: string, + path: string, + req: Record, +) { + const route = (rest.getRoutes() as Array<{ method: string; path: string; handler: (rq: unknown, rs: unknown) => Promise }>) + .find((r) => r.method === method && r.path === path); + if (!route) throw new Error(`${method} ${path} route not registered`); + const res = makeRes(); + await route.handler({ method, params: {}, query: {}, body: {}, headers: {}, ...req }, res); + return res as unknown as { statusCode: number; body: Record; headers: Record }; +} + +/** Seed one preference row and return its server-issued id. */ +async function seed(rest: Awaited>['rest']) { + const created = await call(rest, 'POST', DATA_COLLECTION, { + params: { object: 'rp_user_preference' }, + body: { user_id: 'u1', key: 'ui.recent', value: [{ object: 'account', id: 'a1' }] }, + }); + expect(created.statusCode).toBe(201); + return String(created.body.id); +} + +type Dropped = Array<{ object: string; fields: string[]; reason: string }>; + +describe('[#8093] the path id is addressing, not a dropped field', () => { + it('a body that carries no `id` reports NO droppedFields at all', async () => { + const { rest, writes } = await bootRest(); + const id = await seed(rest); + writes.length = 0; + + // The console's recents trace, verbatim: `{ value: items }`. There is + // no `id` key in this object — that is the whole point of the case. + const body = { value: [{ object: 'account', id: 'a2' }] }; + expect(Object.prototype.hasOwnProperty.call(body, 'id')).toBe(false); + + const patched = await call(rest, 'PATCH', DATA_ITEM, { + params: { object: 'rp_user_preference', id }, + body, + }); + + expect(patched.statusCode).toBe(200); + // The card's invariant: `droppedFields` reports fields the CALLER + // SUPPLIED and the engine refused. Nothing here was supplied and + // refused, so the key must be absent entirely (the omit-when-empty + // shape every client reads). + expect(patched.body.droppedFields).toBeUndefined(); + // ...and the header the toast's sibling channel reads stays unset. + expect(patched.headers['X-ObjectStack-Dropped-Fields']).toBeUndefined(); + + // The write itself is unchanged in every other respect: it committed, + // and the primary key never reached the driver's SET clause (a strip + // this fix deliberately does NOT undo — see the PR body). + expect((patched.body.record as Record).value) + .toEqual([{ object: 'account', id: 'a2' }]); + expect(writes).toHaveLength(1); + expect(Object.prototype.hasOwnProperty.call(writes[0].data, 'id')).toBe(false); + expect(writes[0].id).toBe(id); + }, 60_000); + + it('a read-only field the caller DID supply is still reported, unchanged', async () => { + // The counter-direction. Without this case the fix above is + // indistinguishable from "stop reporting dropped fields". + const { rest } = await bootRest(); + const id = await seed(rest); + + const patched = await call(rest, 'PATCH', DATA_ITEM, { + params: { object: 'rp_user_preference', id }, + body: { value: ['x'], created_at: '2020-01-01T00:00:00.000Z' }, + }); + + expect(patched.statusCode).toBe(200); + expect(patched.body.droppedFields).toEqual([ + { object: 'rp_user_preference', fields: ['created_at'], reason: 'readonly' }, + ]); + // The header channel still carries it too. + expect(patched.headers['X-ObjectStack-Dropped-Fields']).toBe('created_at;reason=readonly'); + }, 60_000); + + it('a supplied read-only field is reported WITHOUT the path id riding along', async () => { + // The mixed case is where a whole-set report would leak the address + // back in: one real refusal must not drag `id` into the list. + const { rest } = await bootRest(); + const id = await seed(rest); + + const patched = await call(rest, 'PATCH', DATA_ITEM, { + params: { object: 'rp_user_preference', id }, + body: { value: ['y'], created_at: '2020-01-01T00:00:00.000Z' }, + }); + + const dropped = patched.body.droppedFields as Dropped; + expect(dropped.flatMap((d) => d.fields)).not.toContain('id'); + expect(dropped.flatMap((d) => d.fields)).toEqual(['created_at']); + }, 60_000); +}); diff --git a/packages/spec/src/contracts/data-engine.ts b/packages/spec/src/contracts/data-engine.ts index 34dedbdea5..0cf1b357f0 100644 --- a/packages/spec/src/contracts/data-engine.ts +++ b/packages/spec/src/contracts/data-engine.ts @@ -26,6 +26,32 @@ import type { IDataDriver } from './data-driver.js'; * surface a warning instead of a silent success (#3356's masked stage * write-backs). * + * ## What is NOT a drop: the address of a single-record write (#8093) + * + * A drop is a field the CALLER SUPPLIED and the engine REFUSED. On a by-id + * update the payload's `id`, when it equals the row the call is bound to, is + * the write's ADDRESS rather than part of its payload — it was refused nothing + * — so it is never reported here, even on the many objects that declare `id` + * as `readonly: true` and even though the strip does still remove it from the + * SET clause. Callers that FOLD an address into the payload get the same + * answer: `metadata-protocol`'s `updateData` appends the path id so a body + * `id` cannot bind a row other than the one the URL and the OCC check name + * (#6479), and that fold must not read back as a refused field. + * + * This is a report boundary, not a strip boundary, and the distinction is + * load-bearing in both directions: an `id` the update dispatch has RULED is + * not an identifier is a real drop and is still reported (`primary_key`, + * #6437), and a predicate/multi write — which addresses nothing by key — still + * reports a caller-supplied `id` in full. + * + * Why it matters more than one spurious warning: consumers render these events + * to end users, so an event nobody can act on teaches users that the channel + * is noise. #8093 was measured as a warning toast on every org switch, from an + * internal preference write; #3431 / #3794 were the same failure one field over + * (`updated_at`), and the lesson recorded there is the one that applies — + * a warning about a field the user never touched drowns the real signal the + * warning exists for. + * * Lives on the TS contract — NOT in the serializable Zod options schemas * (`EngineUpdateOptionsSchema` etc.): a function is unrepresentable in JSON * Schema and cannot cross the RPC (Virtual Data Engine) boundary, so remote