diff --git a/.changeset/dropped-fields-primary-key-reason.md b/.changeset/dropped-fields-primary-key-reason.md new file mode 100644 index 0000000000..22511178ce --- /dev/null +++ b/.changeset/dropped-fields-primary-key-reason.md @@ -0,0 +1,63 @@ +--- +"@objectstack/spec": minor +"@objectstack/objectql": minor +"@objectstack/service-automation": patch +--- + +feat(spec,objectql): `DroppedFieldsEvent.reason` names the dispatch-ruled id strip (#6437) + +The write path's strip-observability seam declared a narrower vocabulary than +the strips it reports on. `DroppedFieldsEvent.reason` was a closed enum over the +two READ-ONLY strips (`readonly` #2948 / `readonly_when` #3042), so the +primary-key strip added by #6262 / PR #6433 (multi branch) and #6435 (by-id +branch) — a `data.id` the update dispatch has ALREADY RULED is not a primary +key, removed from the SET payload before it can overwrite the targeted rows' +identity — was invisible to `onFieldsDropped` and to `strictReadonlyWrites`. +Both PRs were right to refuse the alternative: force-fitting `readonly` would +make `reason` lie, which is worse than silence. This adds the value instead. + +**New reason: `primary_key`.** It names the FIELD's role, not the offending +value's shape, so it stays true if the strip ever widens to the same-value +truthy-scalar no-op the engine deliberately leaves alone today — +`not_a_primary_key` would describe the value and become false that day. The +house rule it follows is #5503's, applied in the other direction: a new arm is +warranted exactly when no existing arm is truthful. #5503 reported the +implicitly-readonly runtime-owned strip as plain `readonly` because that *was* +true of it; `readonly` is not true of an `id` (a truthy scalar `id` writes +fine), so this one gets its own value. + +**⚠️ Behaviour change, deliberate and measured: `strictReadonlyWrites` gains a +new refusal.** The option's contract says it covers "every drop +`onFieldsDropped` reports" — coverage DERIVED from the reported set, never an +enumeration frozen at #5126, and confirmed by reading `reportDroppedFields` on +`main`, whose `strictDrops.push` applies no reason-class filter. So reporting a +new reason necessarily refuses it. A caller that passes +`strictReadonlyWrites: true` **and** puts a ruled-non-key value in `data.id` now +gets `ERR_READONLY_FIELD_REJECTED` where it previously got a success whose `id` +had been silently dropped. That is the option's whole promise ("don't +half-apply my payload") reaching one more strip class, and it is the outcome the +flag's own doc now states. Nothing else moves: default-mode callers still get a +successful write plus an event, the strip itself is unchanged, and +`strictReadonlyWrites` is in-process only (`WriteObservabilityOptions`), so no +REST/wire caller can reach either behaviour. + +**The refusal error no longer describes every rejection as read-only.** +`ReadonlyFieldRejectedError` composed one sentence ("… are read-only and would +have been stripped", remedied by `{ context: { isSystem: true } }`) that is +false for a `primary_key` drop — `isSystem` does not exempt that strip. The +message is now built from the `drops` breakdown the error already carried, so it +names each reason against its own fields and offers the right remedy. The +**read-only-only message is byte-identical** to #5126's / #5503's text (pinned +directly), the error `code` is unchanged, and adding a reason deliberately does +not add an error code: callers catch one code and read `drops`. + +Consumers that branch on `reason` were swept. `service-automation`'s flow-step +warning map is a `Record`, so tsc demanded +the new wording — the loud shape, kept that way on purpose. The protocol +responses that carry `droppedFields` (`api/batch.zod.ts`, `api/protocol.zod.ts` +×3, plus the cross-object batch extension) all derive from +`DroppedFieldsEventSchema` and widen transitively; REST's +`X-ObjectStack-Dropped-Fields` header is generic over the reason and needed no +change. One consumer does NOT widen safely and is filed rather than fixed here: +objectui's `writeWarningToast` picks its wording with a binary ternary whose +`else` arm would announce a stripped `id` as "Read-only" (objectui#3935). diff --git a/content/docs/kernel/contracts/data-engine.mdx b/content/docs/kernel/contracts/data-engine.mdx index 3ac963dea1..01fa785edf 100644 --- a/content/docs/kernel/contracts/data-engine.mdx +++ b/content/docs/kernel/contracts/data-engine.mdx @@ -282,9 +282,11 @@ interface WriteObservabilityOptions { } interface DroppedFieldsEvent { - object: string; // resolved object name - fields: string[]; // caller-supplied fields that were dropped - reason: 'readonly' | 'readonly_when'; // why they were dropped + object: string; // resolved object name + fields: string[]; // caller-supplied fields that were dropped + // why they were dropped — an OPEN vocabulary that grows with the write + // path's legal strips; branch on it exhaustively, never with a binary test + reason: 'readonly' | 'readonly_when' | 'primary_key'; } ``` diff --git a/content/docs/protocol/objectql/security.mdx b/content/docs/protocol/objectql/security.mdx index d446964c81..45707cb445 100644 --- a/content/docs/protocol/objectql/security.mdx +++ b/content/docs/protocol/objectql/security.mdx @@ -325,7 +325,12 @@ await data.update('attendance', { id, work_duration: 480 }, { ``` `reason` is `'readonly'` for this static lock and `'readonly_when'` for a conditional -[`readonlyWhen`](/docs/references/data/field) predicate. The listener is an in-process +[`readonlyWhen`](/docs/references/data/field) predicate. A third value, +`'primary_key'`, reports the one legal strip that is **not** a read-only lock: an +`update` payload whose `id` the engine has already ruled is not an identifier is +dropped rather than written over the targeted row's primary key. The vocabulary is +open — it grows as the write path gains legal strips — so branch on `reason` +exhaustively rather than treating "not `readonly_when`" as "read-only". The listener is an in-process callback: it is delivered by the local engine, and does **not** cross the RPC / Virtual Data Engine boundary, so a remote caller never receives these events. Without a listener, the only trace is a server-side `WARN` naming the object, the field, and both remedies. diff --git a/content/docs/references/api/batch.mdx b/content/docs/references/api/batch.mdx index 0a50d71614..2b694b29fe 100644 --- a/content/docs/references/api/batch.mdx +++ b/content/docs/references/api/batch.mdx @@ -58,7 +58,7 @@ const result = BatchConfigSchema.parse(data); | **errors** | `{ code: Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| … +257 more>; message: string; category?: string; httpStatus?: integer; … }[]` | optional | Array of errors if operation failed. Branch on `errors[0].code` — an atomic batch that rolled back marks rows that were written then undone with code ROLLED_BACK and rows never reached with NOT_ATTEMPTED, while the causal row keeps its own error (#4793). | | **data** | `Record` | optional | Full record data (if returnRecords=true) | | **index** | `number` | optional | Index of the record in the request array | -| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when'> }[]` | optional | Write-observability (#3407/#3431/#3455): caller-supplied fields LEGALLY stripped from THIS row before it was written — static `readonly` (#2948) / TRUE `readonlyWhen` (#3042) on update, or the #3043 create-ingress strip. Per-row because a batch can drop different fields on different rows (`readonlyWhen` is record-state-dependent). Present ONLY when ≥1 field was dropped for this row; the row still succeeded (success unchanged). A single response header cannot express per-row drops, so this body field is the canonical bulk channel — REST does not emit `X-ObjectStack-Dropped-Fields` for batches. Optional — omit-when-empty keeps the shape backward-compatible. | +| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when' \| 'primary_key'> }[]` | optional | Write-observability (#3407/#3431/#3455): caller-supplied fields LEGALLY stripped from THIS row before it was written — static `readonly` (#2948) / TRUE `readonlyWhen` (#3042) on update, or the #3043 create-ingress strip. Per-row because a batch can drop different fields on different rows (`readonlyWhen` is record-state-dependent). Present ONLY when ≥1 field was dropped for this row; the row still succeeded (success unchanged). A single response header cannot express per-row drops, so this body field is the canonical bulk channel — REST does not emit `X-ObjectStack-Dropped-Fields` for batches. Optional — omit-when-empty keeps the shape backward-compatible. | --- @@ -143,7 +143,7 @@ A cross-object batch strip event: dropped fields plus the operation index | :--- | :--- | :--- | :--- | | **object** | `string` | ✅ | Object the write targeted (resolved object name) | | **fields** | `string[]` | ✅ | Caller-supplied field names the engine removed from the write payload | -| **reason** | `Enum<'readonly' \| 'readonly_when'>` | ✅ | Why the fields were dropped: static readonly (#2948) or a TRUE readonlyWhen predicate (#3042) | +| **reason** | `Enum<'readonly' \| 'readonly_when' \| 'primary_key'>` | ✅ | Why the fields were dropped: static readonly (#2948), a TRUE readonlyWhen predicate (#3042), or the primary-key strip of a payload id the engine ruled is not an identifier (#6437) | | **index** | `integer` | ✅ | Index of the operation in the request `operations` array | @@ -182,7 +182,7 @@ A cross-object batch strip event: dropped fields plus the operation index | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | | **results** | `any[]` | ✅ | Per-operation result, index-aligned with the request operations | -| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when'>; index: integer }[]` | optional | Write-observability (#3407/#3431/#3455/#3794): caller-supplied fields the engine LEGALLY stripped from an operation before it was written — static `readonly` (#2948) or a TRUE `readonlyWhen` predicate (#3042). This endpoint is the console record form's save path (master-detail writes parent + children in one transaction), so without it the ONE surface where a user edits a `readonlyWhen` field reported plain success while the value never landed. Each event carries the `index` of its operation. Present ONLY when ≥1 field was dropped; the batch still committed without them (results/success semantics unchanged). Optional — omit-when-empty keeps the shape backward-compatible. | +| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when' \| 'primary_key'>; index: integer }[]` | optional | Write-observability (#3407/#3431/#3455/#3794): caller-supplied fields the engine LEGALLY stripped from an operation before it was written — static `readonly` (#2948) or a TRUE `readonlyWhen` predicate (#3042). This endpoint is the console record form's save path (master-detail writes parent + children in one transaction), so without it the ONE surface where a user edits a `readonlyWhen` field reported plain success while the value never landed. Each event carries the `index` of its operation. Present ONLY when ≥1 field was dropped; the batch still committed without them (results/success semantics unchanged). Optional — omit-when-empty keeps the shape backward-compatible. | --- diff --git a/content/docs/references/api/protocol.mdx b/content/docs/references/api/protocol.mdx index e29c7a1b60..86c6773c45 100644 --- a/content/docs/references/api/protocol.mdx +++ b/content/docs/references/api/protocol.mdx @@ -351,7 +351,7 @@ const result = AiAgentCapabilitiesSchema.parse(data); | **object** | `string` | ✅ | The object name. | | **id** | `string` | ✅ | The ID of the newly created record. | | **record** | `Record` | ✅ | The created record, including server-generated fields (created_at, owner). | -| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when'> }[]` | optional | Write-observability (#3407/#3431): caller-supplied fields that were LEGALLY stripped before the record was written — a non-system create cannot seed a static `readonly` column (#3043 ingress strip), so those keys are dropped and the field re-derives its default. Present ONLY when ≥1 field was dropped; the create still succeeded without them (status/success semantics unchanged). REST additionally surfaces this as the `X-ObjectStack-Dropped-Fields` response header. Optional — omit-when-empty keeps the shape backward-compatible for existing clients. | +| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when' \| 'primary_key'> }[]` | optional | Write-observability (#3407/#3431): caller-supplied fields that were LEGALLY stripped before the record was written — a non-system create cannot seed a static `readonly` column (#3043 ingress strip), so those keys are dropped and the field re-derives its default. Present ONLY when ≥1 field was dropped; the create still succeeded without them (status/success semantics unchanged). REST additionally surfaces this as the `X-ObjectStack-Dropped-Fields` response header. Optional — omit-when-empty keeps the shape backward-compatible for existing clients. | --- @@ -377,7 +377,7 @@ const result = AiAgentCapabilitiesSchema.parse(data); | **object** | `string` | ✅ | Object name | | **records** | `Record[]` | ✅ | Created records | | **count** | `number` | ✅ | Number of records created | -| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when'> }[]` | optional | Write-observability (#3407/#3431/#3455): caller-supplied `readonly` fields the #3043 create-ingress strip removed before the rows were written. AGGREGATED across the batch (one event per object/reason with the union of dropped field names) rather than per-row, because the insert-time strip is static-`readonly` only — schema-uniform, so every row drops the same set. Present ONLY when ≥1 field was dropped; the creates still succeeded without them (count/success unchanged). Optional — omit-when-empty keeps the shape backward-compatible. (The per-row `insertMany`/`batch` paths carry per-row `droppedFields` on each result instead — see BatchOperationResultSchema.) | +| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when' \| 'primary_key'> }[]` | optional | Write-observability (#3407/#3431/#3455): caller-supplied `readonly` fields the #3043 create-ingress strip removed before the rows were written. AGGREGATED across the batch (one event per object/reason with the union of dropped field names) rather than per-row, because the insert-time strip is static-`readonly` only — schema-uniform, so every row drops the same set. Present ONLY when ≥1 field was dropped; the creates still succeeded without them (count/success unchanged). Optional — omit-when-empty keeps the shape backward-compatible. (The per-row `insertMany`/`batch` paths carry per-row `droppedFields` on each result instead — see BatchOperationResultSchema.) | --- @@ -1469,7 +1469,7 @@ Uninstall package response | **object** | `string` | ✅ | Object name | | **id** | `string` | ✅ | Updated record ID | | **record** | `Record` | ✅ | Updated record | -| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when'> }[]` | optional | Write-observability (#3407/#3431): caller-supplied fields the engine LEGALLY stripped from the write before persisting — static `readonly` (#2948) or a TRUE `readonlyWhen` predicate (#3042). Present ONLY when ≥1 field was dropped; the update still succeeded without them (status/success semantics unchanged — stripping is legitimate, not an error). REST additionally surfaces this as the `X-ObjectStack-Dropped-Fields` response header. Optional — omit-when-empty keeps the shape backward-compatible for existing clients that only read `record`. | +| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when' \| 'primary_key'> }[]` | optional | Write-observability (#3407/#3431): caller-supplied fields the engine LEGALLY stripped from the write before persisting — static `readonly` (#2948) or a TRUE `readonlyWhen` predicate (#3042). Present ONLY when ≥1 field was dropped; the update still succeeded without them (status/success semantics unchanged — stripping is legitimate, not an error). REST additionally surfaces this as the `X-ObjectStack-Dropped-Fields` response header. Optional — omit-when-empty keeps the shape backward-compatible for existing clients that only read `record`. | --- diff --git a/content/docs/references/data/data-engine.mdx b/content/docs/references/data/data-engine.mdx index 5c8a97bb9a..2da68e20b1 100644 --- a/content/docs/references/data/data-engine.mdx +++ b/content/docs/references/data/data-engine.mdx @@ -451,7 +451,7 @@ A write-path strip event: caller-supplied fields legally dropped from the payloa | :--- | :--- | :--- | :--- | | **object** | `string` | ✅ | Object the write targeted (resolved object name) | | **fields** | `string[]` | ✅ | Caller-supplied field names the engine removed from the write payload | -| **reason** | `Enum<'readonly' \| 'readonly_when'>` | ✅ | Why the fields were dropped: static readonly (#2948) or a TRUE readonlyWhen predicate (#3042) | +| **reason** | `Enum<'readonly' \| 'readonly_when' \| 'primary_key'>` | ✅ | Why the fields were dropped: static readonly (#2948), a TRUE readonlyWhen predicate (#3042), or the primary-key strip of a payload id the engine ruled is not an identifier (#6437) | --- diff --git a/packages/objectql/src/engine-dropped-fields-primary-key.test.ts b/packages/objectql/src/engine-dropped-fields-primary-key.test.ts new file mode 100644 index 0000000000..521489c638 --- /dev/null +++ b/packages/objectql/src/engine-dropped-fields-primary-key.test.ts @@ -0,0 +1,321 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// objectstack#6437 — `DroppedFieldsEvent.reason` gains `primary_key`, so the +// write path's primary-key strip becomes visible to BOTH halves of the #3407 / +// #5126 seam instead of only to the server log. +// +// ## What was wrong +// +// #6262 / PR #6433 (multi branch) and #6435 (by-id branch) each added a legal +// strip: a `data.id` the update dispatch has ALREADY RULED is not a primary key +// is removed from the SET payload, because writing it would overwrite the +// identity of the very rows the call targets. Both were caller-supplied fields +// legally dropped from a write — the exact thing `onFieldsDropped` exists to +// report — but `reason` was a closed enum over the two READ-ONLY strips, so +// both blocks emitted a `warn` and deliberately reported nothing. Neither PR +// was willing to force-fit `readonly`: `id` is not read-only (a truthy scalar +// `id` writes fine), and a `reason` that lies is worse than silence. +// +// The consequence this file pins away: a caller subscribed to +// `onFieldsDropped` saw nothing, and a caller that had opted into +// `strictReadonlyWrites` — whose whole promise is "refuse rather than commit a +// payload I did not write" — still got a success whose `id` had been dropped. +// +// ## Predicted table, written BEFORE the first run +// +// | case | default mode | strictReadonlyWrites: true | +// |---------------------------------------|---------------------------------------|-------------------------------------------| +// | multi update, ruled-non-id `data.id` | event {fields:['id'],'primary_key'}, | REFUSED, ERR_READONLY_FIELD_REJECTED, | +// | | write SUCCEEDS | drops[].reason='primary_key', no driver | +// | by-id update, ruled-non-id `data.id` | same event, write SUCCEEDS | REFUSED, same | +// | readonly / readonly_when strip | UNCHANGED `readonly`/`readonly_when` | REFUSED, byte-identical #5126 message | +// | truthy scalar `data.id` (not stripped)| NO event | no throw | +// | write carrying no `id` at all | NO event | no throw | +// +// The strict column is the deliberate part. `strictReadonlyWrites` covers +// "every drop `onFieldsDropped` reports" by its own contract sentence +// (`spec/src/contracts/data-engine.ts`), and that coverage is DERIVED from the +// reported set rather than enumerated — measured on `main` in +// `reportDroppedFields`, whose `strictDrops.push` has no reason-class filter. +// So reporting a new reason NECESSARILY adds a new refusal. That is stated in +// the option's doc and pinned here, in both directions, rather than left for a +// caller to discover. +// +// ## Reverse verification, direction predicted first +// +// Delete the two `reportDroppedFields(..., 'primary_key')` call sites in +// `engine.ts` and: +// - every `primary_key` case below goes RED (no event / the write resolves); +// - every `readonly` / `readonly_when` case stays GREEN — the read-only seam +// is untouched, which is the claim the changeset makes; +// - the byte-identity pin stays GREEN. +// Revert `readonly-strict-errors.ts` instead and only the wording pins move: +// the `primary_key` message assertions go RED while the byte-identity pin +// stays GREEN. Measured results are recorded in the PR body. + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; +import type { DroppedFieldsEvent } from '@objectstack/spec/data'; + +interface RecordedCall { + readonly fn: 'update' | 'updateMany'; + readonly id?: unknown; + /** A COPY — the engine may keep mutating its own payload after the call. */ + readonly data: Record; +} + +const silentLogger: any = (() => { + const l: any = { + debug() {}, info() {}, error() {}, trace() {}, fatal() {}, warn() {}, + child() { return l; }, + }; + return l; +})(); + +function makeRecordingDriver() { + const calls: RecordedCall[] = []; + const rows = new Map>(); + 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 [...rows.values()]; }, + async findOne() { return rows.values().next().value ?? null; }, + async create(_o: string, data: Record) { return { id: 'r1', ...data }; }, + async update(_o: string, id: string, data: Record) { + calls.push({ fn: 'update', id, data: { ...data } }); + return { id, ...data }; + }, + async updateMany(_o: string, _ast: unknown, data: Record) { + calls.push({ fn: 'updateMany', data: { ...data } }); + return 2; + }, + async delete() { return true; }, + async deleteMany() { return 0; }, + async count() { return 0; }, + async bulkCreate() { return []; }, async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + rows.set('rec_1', { id: 'rec_1', title: 't0', status: 'open', settled_total: null }); + return { driver, calls, rows }; +} + +async function makeEngine() { + const engine = new ObjectQL({ logger: silentLogger }); + const { driver, calls } = makeRecordingDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject({ + name: 'task', + fields: { + title: { type: 'text' }, + status: { type: 'text' }, + settled_total: { type: 'number', readonly: true }, + closed_at: { type: 'datetime', readonlyWhen: "record.status == 'open'" }, + }, + } as any); + return { engine, calls }; +} + +/** Run a write with a listener attached; return the events and driver calls. */ +async function observe(data: unknown, options: Record = {}) { + const { engine, calls } = await makeEngine(); + const events: DroppedFieldsEvent[] = []; + await engine.update('task', data as any, { + ...options, + onFieldsDropped: (e: DroppedFieldsEvent) => events.push(e), + } as any); + return { events, calls }; +} + +/** Catch and return the rejection, so a case can assert on its shape. */ +async function rejection(p: Promise): Promise { + try { + await p; + } catch (err) { + return err; + } + throw new Error('expected the write to be refused, but it resolved'); +} + +/** Run a write under strict; return the error and whether the driver was hit. */ +async function refuse(data: unknown, options: Record = {}) { + const { engine, calls } = await makeEngine(); + const err = await rejection(engine.update('task', data as any, { + ...options, + strictReadonlyWrites: true, + } as any)); + return { err, calls }; +} + +// ── 1. the quiet half: onFieldsDropped now sees the primary-key strip ────── + +describe('#6437 — the primary-key strip is reported as reason `primary_key`', () => { + it('MULTI branch: an operator-object data.id is reported, and the write still succeeds', async () => { + const { events, calls } = await observe( + { id: { $in: ['a', 'b'] }, title: 'x' }, + { multi: true }, + ); + // The signal #6433 could not emit. + expect(events).toEqual([{ object: 'task', fields: ['id'], reason: 'primary_key' }]); + // ...and the strip's own behaviour is unchanged: still a bulk write, still + // no `id` in the SET clause, still carrying the column the caller meant. + expect(calls.map((c) => c.fn)).toEqual(['updateMany']); + expect(calls[0].data).toEqual({ title: 'x' }); + }); + + it('BY-ID branch: a ruled-non-id data.id beside a scalar where.id is reported too', async () => { + // #6435's block — the card named only PR #6433's multi block, but `main` + // carries two strips of the same class and both had the same deliberate + // "not reporting this one" comment. + const { events, calls } = await observe( + { id: { $in: ['a', 'b'] }, title: 'x' }, + { where: { id: 'rec_1' } }, + ); + expect(events).toEqual([{ object: 'task', fields: ['id'], reason: 'primary_key' }]); + expect(calls.map((c) => c.fn)).toEqual(['update']); + expect(calls[0].id).toBe('rec_1'); + expect(calls[0].data).toEqual({ title: 'x' }); + }); + + // ── the contrast pins: what does NOT report ────────────────────────────── + + it('a TRUTHY SCALAR data.id is the bound key — not stripped, so nothing is reported', async () => { + const { events, calls } = await observe({ id: 'rec_1', title: 'x' }, {}); + expect(events).toEqual([]); + expect(calls[0].data).toMatchObject({ title: 'x' }); + }); + + it('a write carrying no id at all reports nothing', async () => { + const { events } = await observe({ title: 'x' }, { multi: true, where: { status: 'open' } }); + expect(events).toEqual([]); + }); +}); + +// ── 2. the read-only seam is untouched ──────────────────────────────────── + +describe('#6437 — the two read-only reasons are unchanged', () => { + it('a static readonly strip still reports `readonly`, not the new value', async () => { + const { events } = await observe({ id: 'rec_1', settled_total: 99 }, {}); + expect(events).toEqual([{ object: 'task', fields: ['settled_total'], reason: 'readonly' }]); + }); + + it('a readonlyWhen strip still reports `readonly_when`', async () => { + const { events } = await observe({ id: 'rec_1', closed_at: '2026-01-01' }, {}); + expect(events).toEqual([{ object: 'task', fields: ['closed_at'], reason: 'readonly_when' }]); + }); + + it('a primary-key strip and a readonly strip in one payload are SEPARATE events', async () => { + // One event per strip pass, each truthfully labelled — the whole point of + // widening the vocabulary rather than merging the classes. + const { events } = await observe( + { id: { $in: ['a', 'b'] }, title: 'x', settled_total: 99 }, + { multi: true }, + ); + expect(events).toEqual([ + { object: 'task', fields: ['id'], reason: 'primary_key' }, + { object: 'task', fields: ['settled_total'], reason: 'readonly' }, + ]); + }); +}); + +// ── 3. the loud half: strictReadonlyWrites gains a refusal, deliberately ── + +describe('#6437 — strictReadonlyWrites refuses the primary-key strip too', () => { + it('MULTI branch: REFUSED with the envelope code, and no driver call happens', async () => { + const { err, calls } = await refuse({ id: { $in: ['a', 'b'] }, title: 'x' }, { multi: true }); + expect(err.code).toBe('ERR_READONLY_FIELD_REJECTED'); + expect(err.name).toBe('ReadonlyFieldRejectedError'); + expect(err.object).toBe('task'); + expect(err.fields).toEqual(['id']); + // Nothing written — not the stripped key, not `title`, which would have + // survived the strip on the default path. + expect(calls).toEqual([]); + }); + + it('BY-ID branch: REFUSED the same way', async () => { + const { err, calls } = await refuse( + { id: { $in: ['a', 'b'] }, title: 'x' }, + { where: { id: 'rec_1' } }, + ); + expect(err.code).toBe('ERR_READONLY_FIELD_REJECTED'); + expect(err.fields).toEqual(['id']); + expect(calls).toEqual([]); + }); + + it('`drops` carries the reason, so a caller separates the classes without parsing prose', async () => { + const { err } = await refuse({ id: { $in: ['a', 'b'] }, title: 'x' }, { multi: true }); + expect(err.drops).toEqual([{ object: 'task', fields: ['id'], reason: 'primary_key' }]); + }); + + it('a mixed payload accumulates BOTH classes into one refusal', async () => { + const { err } = await refuse( + { id: { $in: ['a', 'b'] }, title: 'x', settled_total: 99 }, + { multi: true }, + ); + expect([...err.fields].sort()).toEqual(['id', 'settled_total']); + expect([...err.drops].map((d: any) => d.reason)).toEqual(['primary_key', 'readonly']); + }); + + it('the listener does NOT also fire on a refused write — quiet-or-loud, one per call', async () => { + const { engine } = await makeEngine(); + const events: DroppedFieldsEvent[] = []; + await rejection(engine.update( + 'task', + { id: { $in: ['a', 'b'] }, title: 'x' } as any, + { multi: true, strictReadonlyWrites: true, onFieldsDropped: (e: DroppedFieldsEvent) => events.push(e) } as any, + )); + expect(events).toEqual([]); + }); + + it('a truthy scalar data.id is NOT refused — strict adds no second policy', async () => { + // The rule #5126 wrote down: strict refuses exactly what the strip takes. + // The strip leaves a bound scalar key alone, so strict must accept it. + const { engine, calls } = await makeEngine(); + await engine.update('task', { id: 'rec_1', title: 'x' } as any, { strictReadonlyWrites: true } as any); + expect(calls.map((c) => c.fn)).toEqual(['update']); + }); +}); + +// ── 4. the refusal message tells the truth per reason ───────────────────── + +describe('#6437 — the refusal message is composed from `drops`, not from the code name', () => { + it('a primary_key refusal does NOT claim the field is read-only', async () => { + const { err } = await refuse({ id: { $in: ['a', 'b'] }, title: 'x' }, { multi: true }); + // The lie this change exists to prevent — `id` is not read-only, and + // `isSystem` does not exempt it. + expect(err.message).not.toContain('are read-only and would have been stripped'); + expect(err.message).toContain('primary key'); + expect(err.message).toContain('was REFUSED'); + // ...and it points at the real remedy for THIS class. + expect(err.message).toContain('SCALAR id'); + }); + + it('the READ-ONLY-only message is byte-identical to the pre-#6437 text', async () => { + // The compatibility half. Every caller and pin written against #5126's / + // #5503's wording must be untouched by a change that only teaches the + // error about a class those payloads do not carry. + const { err } = await refuse({ id: 'rec_1', settled_total: 99 }, {}); + expect(err.message).toBe( + `Update on 'task' was REFUSED: 1 caller-supplied field(s) ` + + `(settled_total) are read-only and would have been stripped, and this write ` + + `passed options.strictReadonlyWrites — so NOTHING was written, including the fields ` + + `that would have survived. Remove the read-only field(s) from the payload; or, for ` + + `server-side code that legitimately writes read-only columns, pass ` + + `{ context: { isSystem: true } } (this exempts statically 'readonly' fields, but NOT ` + + `fields locked by a TRUE 'readonlyWhen' predicate — those stay locked for every ` + + `caller). To let the strip happen and merely observe it, drop ` + + `strictReadonlyWrites and pass options.onFieldsDropped instead (#3407).`, + ); + }); + + it('a mixed refusal names each reason against its own fields', async () => { + const { err } = await refuse( + { id: { $in: ['a', 'b'] }, title: 'x', settled_total: 99 }, + { multi: true }, + ); + expect(err.message).toContain('id — the primary key'); + expect(err.message).toContain('settled_total — read-only'); + expect(err.message).not.toContain('are read-only and would have been stripped'); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index e23ae3539b..2e8cf45753 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -6847,7 +6847,8 @@ export class ObjectQL implements IObjectQLEngine { }; // [#3407] Structured strip observability. The readonly/readonlyWhen strips - // below are LEGAL semantics (the write still succeeds without the locked + // below — and, since #6437, the primary-key strip at each branch's head — + // are LEGAL semantics (the write still succeeds without the dropped // fields), but until now the only trace was a server-side logger warn — a // caller that reports success per requested field (a flow's `update_record` // step) saw a clean success while the DB value never changed. When the @@ -6878,10 +6879,19 @@ export class ObjectQL implements IObjectQLEngine { // would otherwise report a partial success for a write that never // happened. Quiet-and-observable or loud — one per call. // - // Drops accumulate across BOTH passes and throw ONCE (below, after the + // Drops accumulate across EVERY pass and throw ONCE (below, after the // static strip) so the caller gets every offending field in one error // instead of a round-trip per field. The throw lands before any driver // call, so nothing is written. + // + // [#6437] Adding a reason therefore adds a REFUSAL, and that is the + // contract rather than a side effect: `strictReadonlyWrites` is documented + // as covering "every drop `onFieldsDropped` reports" — a set DERIVED from + // what this helper reports, never an enumeration frozen at #5126. So the + // `primary_key` strip is refused under strict for exactly the reason the + // 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. const onFieldsDropped = options?.onFieldsDropped; const strictReadonlyWrites = options?.strictReadonlyWrites === true; const strictDrops: DroppedFieldsEvent[] = []; @@ -7159,13 +7169,14 @@ export class ObjectQL implements IObjectQLEngine { // - Per-driver skip lists (route C) are the #5240 / #4434 shape // of five backends answering one question five ways. // - // Same choice as #6262 on the reporting seam: NOT routed through - // `reportDroppedFields`, because `DroppedFieldsEvent.reason` is a - // closed enum over the two READ-ONLY strips (`readonly` / - // `readonly_when`, #3407/#3042) and this drop is neither; - // widening that vocabulary is a `packages/spec` change with its - // own consumers (filed separately as #6437). The `warn` is the - // #4632 duty meanwhile — the caller is told the write succeeded. + // [#6437] REPORTED, on the same seam as the read-only strips. + // The vocabulary widened (`DroppedFieldsEvent.reason` gained + // `primary_key`), so the strip no longer has to choose between + // silence and a `reason` that lies — the choice #6262 / #6435 + // were right to refuse. The `warn` below STAYS: it carries the + // remedy prose, and `onFieldsDropped` is opt-in, so a caller + // that registered no listener would otherwise lose the #4632 + // signal entirely. const preIdById = hookContext.input.data as Record | null | undefined; if ( preIdById && @@ -7184,6 +7195,7 @@ export class ObjectQL implements IObjectQLEngine { `SELECT rows by an id set, put it in \`where\` ` + `(\`{ where: { id: { $in: [...] } }, multi: true }\`).`, ); + reportDroppedFields(preIdById, hookContext.input.data as Record, 'primary_key'); } await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); normalizeMultiValueFields(updateSchema, hookContext.input.data as Record); @@ -7349,14 +7361,16 @@ export class ObjectQL implements IObjectQLEngine { // `data.id` outranks both `where` and `multi` and never gets // here, and N rows cannot share one primary key anyway. // - // Deliberately NOT reported through `reportDroppedFields`: - // `DroppedFieldsEvent.reason` is a closed enum over the two - // READ-ONLY strips (`readonly` / `readonly_when`, #3407/#3042), - // and this drop is neither. Widening that vocabulary is a - // `packages/spec` change with its own consumers (batch + REST - // protocol responses), not a rider on an engine fix. The `warn` - // is the #4632 duty in the meantime: name the consequence and - // the remedy, since the caller is told the write succeeded. + // [#6437] REPORTED through `reportDroppedFields` under the + // `primary_key` reason. `DroppedFieldsEvent.reason` was a closed + // enum over the two READ-ONLY strips when this block landed, and + // this drop is neither — so PR #6433 emitted only the `warn` + // rather than force-fit an arm that would have lied. #6437 + // widened the vocabulary instead (spec, plus the batch/REST + // protocol responses that carry it transitively), which is what + // lets the seam report it truthfully now. The `warn` STAYS: it + // carries the remedy prose, and a caller that registered no + // listener still needs the #4632 signal. const preIdMulti = hookContext.input.data as Record | null | undefined; if (preIdMulti && typeof preIdMulti === 'object' && Object.prototype.hasOwnProperty.call(preIdMulti, 'id')) { const { id: notAnId, ...withoutId } = preIdMulti; @@ -7369,6 +7383,7 @@ export class ObjectQL implements IObjectQLEngine { `scalar id (\`update(object, { id, ...fields })\` or \`{ where: { id } }\`) instead of ` + `options.multi; to SELECT rows by an id set, put it in \`where\` (\`{ where: { id: { $in: [...] } }, multi: true }\`).`, ); + reportDroppedFields(preIdMulti, hookContext.input.data as Record, 'primary_key'); } await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); normalizeMultiValueFields(updateSchema, hookContext.input.data as Record); diff --git a/packages/objectql/src/readonly-strict-errors.ts b/packages/objectql/src/readonly-strict-errors.ts index d3f316086b..e3da5fcdc5 100644 --- a/packages/objectql/src/readonly-strict-errors.ts +++ b/packages/objectql/src/readonly-strict-errors.ts @@ -15,21 +15,134 @@ import type { DroppedFieldsEvent } from '@objectstack/spec/data'; * want a smaller version of it. * * `fields` is the union across every strip pass the operation runs — on UPDATE - * static `readonly` (#2948), a TRUE `readonlyWhen` predicate (#3042), and the - * implicitly-readonly runtime-owned types (#5503); on INSERT only the last of - * those, because a create is deliberately exempt from the author-declared - * strips (#3413). One error names everything wrong with the payload instead of - * forcing a round-trip per field. `drops` + * static `readonly` (#2948), a TRUE `readonlyWhen` predicate (#3042), the + * implicitly-readonly runtime-owned types (#5503), and the `primary_key` strip + * of a payload `id` the update dispatch ruled is not an identifier (#6437); on + * INSERT only the runtime-owned ones, because a create is deliberately exempt + * from the author-declared strips (#3413). One error names everything wrong + * with the payload instead of forcing a round-trip per field. `drops` * keeps the per-reason breakdown (the same `DroppedFieldsEvent` shape * `onFieldsDropped` would have received, had the write been allowed to * complete), so a caller can tell a schema-level lock from a state-dependent * one without parsing the message. * + * ## The message is composed from `drops`, not from the error's name (#6437) + * + * Until `primary_key` existed every reason was a read-only lock, so the message + * could say "are read-only" of the whole union and be true. It no longer can: + * an `id` refused by the primary-key strip is not read-only — a truthy scalar + * `id` writes fine — and `{ context: { isSystem: true } }`, the remedy the + * read-only wording offers, does not exempt it either. Asserting either would + * relocate the exact harm PR #6433 refused to commit (a `reason` that lies) + * from the event into the error. + * + * So the wording branches on the reasons actually present, and the + * READ-ONLY-ONLY message stays **byte-identical** to #5126's / #5503's — every + * caller and pin written against it is untouched, and only a payload carrying a + * genuinely new strip class sees new prose. The error `code` does NOT branch: + * `ERR_READONLY_FIELD_REJECTED` is what callers catch, and `drops` is what they + * read to tell the classes apart. + * * Identified by `code` rather than `instanceof` so it survives crossing package * boundaries — the convention `SummaryRecomputeError` / `DriverConnectError` * already follow here. The code is registered in the spec's `ERROR_CODE_LEDGER` * under `@objectstack/objectql`. */ +/** + * The reason arms whose shared wording ("are read-only", remedied by + * `isSystem`) is TRUE. A drop outside this set makes the read-only sentence a + * lie, so the message switches to the per-reason form below. + */ +const READONLY_CLASS_REASONS: ReadonlySet = new Set([ + 'readonly', + 'readonly_when', +]); + +/** How each reason is named to a human, in the per-reason message form. */ +const REASON_PHRASE: Record = { + readonly: "read-only (`readonly: true`)", + readonly_when: "read-only in the target record's current state (a TRUE `readonlyWhen` predicate)", + primary_key: + 'the primary key, carrying a value the engine has already ruled is not an identifier — ' + + "writing it would have overwritten the targeted row(s)' primary-key column", +}; + +/** + * The per-reason breakdown sentence, e.g. + * `id — the primary key, …; amount — read-only in the target record's …`. + * Fields keep the order they were dropped in, de-duplicated per reason. + */ +function describeDropsByReason(drops: readonly DroppedFieldsEvent[]): string { + const byReason = new Map(); + for (const d of drops) { + const seen = byReason.get(d.reason) ?? []; + for (const f of d.fields) if (!seen.includes(f)) seen.push(f); + byReason.set(d.reason, seen); + } + return [...byReason] + .filter(([, fields]) => fields.length > 0) + .map(([reason, fields]) => `${fields.join(', ')} — ${REASON_PHRASE[reason] ?? reason}`) + .join('; '); +} + +/** + * Compose the refusal message. + * + * The read-only-only branch is #5126's / #5503's text, byte for byte — an + * `expect(err.message)` written against it must not move because a *different* + * payload can now be refused for a different reason. + */ +function buildRefusalMessage( + object: string, + fields: string[], + drops: readonly DroppedFieldsEvent[], + operation: 'insert' | 'update', +): string { + const head = `${operation === 'insert' ? 'Insert' : 'Update'} on '${object}' was REFUSED: `; + const tail = + `To let the strip happen and merely observe it, drop ` + + `strictReadonlyWrites and pass options.onFieldsDropped instead (#3407).`; + + // Empty `drops` cannot happen on either throw site, but `every` on it is + // vacuously true, which lands on the historical wording — the safe default. + if (drops.every((d) => READONLY_CLASS_REASONS.has(d.reason))) { + return ( + head + + `${fields.length} caller-supplied field(s) ` + + `(${fields.join(', ')}) are read-only and would have been stripped, and this write ` + + `passed options.strictReadonlyWrites — so NOTHING was written, including the fields ` + + `that would have survived. Remove the read-only field(s) from the payload; or, for ` + + `server-side code that legitimately writes read-only columns, pass ` + + (operation === 'insert' + ? `{ context: { isSystem: true } } — or, for a data migration reinstating legacy ` + + `values for a runtime-owned field (a record number), the historical-import ` + + `context { context: { preserveAudit: true } } (#3493). ` + : `{ context: { isSystem: true } } (this exempts statically 'readonly' fields, but NOT ` + + `fields locked by a TRUE 'readonlyWhen' predicate — those stay locked for every ` + + `caller). `) + + tail + ); + } + + // At least one drop is NOT a read-only lock, so the union cannot be described + // as read-only and `isSystem` is not a blanket remedy. Name each reason. + return ( + head + + `${fields.length} caller-supplied field(s) ` + + `(${fields.join(', ')}) would have been stripped, and this write passed ` + + `options.strictReadonlyWrites — so NOTHING was written, including the fields that ` + + `would have survived. Per reason: ${describeDropsByReason(drops)}. The remedies differ ` + + `per reason: for a read-only lock, remove the field from the payload, or pass ` + + `{ context: { isSystem: true } } for server-side code that legitimately writes ` + + `statically 'readonly' columns (that exempts NEITHER a TRUE 'readonlyWhen' predicate ` + + `NOR the primary-key strip — both apply to every caller, isSystem included); for ` + + `'primary_key', pass a SCALAR id to update ONE row (\`update(object, { id, ...fields })\` ` + + `or \`{ where: { id } }\`), or put the id set in \`where\` to SELECT rows ` + + `(\`{ where: { id: { $in: [...] } }, multi: true }\`). ` + + tail + ); +} + export class ReadonlyFieldRejectedError extends Error { readonly code = 'ERR_READONLY_FIELD_REJECTED' as const; constructor( @@ -46,23 +159,7 @@ export class ReadonlyFieldRejectedError extends Error { */ public readonly operation: 'insert' | 'update' = 'update', ) { - super( - `${operation === 'insert' ? 'Insert' : 'Update'} on '${object}' was REFUSED: ` + - `${fields.length} caller-supplied field(s) ` + - `(${fields.join(', ')}) are read-only and would have been stripped, and this write ` + - `passed options.strictReadonlyWrites — so NOTHING was written, including the fields ` + - `that would have survived. Remove the read-only field(s) from the payload; or, for ` + - `server-side code that legitimately writes read-only columns, pass ` + - (operation === 'insert' - ? `{ context: { isSystem: true } } — or, for a data migration reinstating legacy ` + - `values for a runtime-owned field (a record number), the historical-import ` + - `context { context: { preserveAudit: true } } (#3493). ` - : `{ context: { isSystem: true } } (this exempts statically 'readonly' fields, but NOT ` + - `fields locked by a TRUE 'readonlyWhen' predicate — those stay locked for every ` + - `caller). `) + - `To let the strip happen and merely observe it, drop ` + - `strictReadonlyWrites and pass options.onFieldsDropped instead (#3407).`, - ); + super(buildRefusalMessage(object, fields, drops, operation)); this.name = 'ReadonlyFieldRejectedError'; } } diff --git a/packages/services/service-automation/src/builtin/crud-nodes.ts b/packages/services/service-automation/src/builtin/crud-nodes.ts index 7921fcc4ba..33908581fc 100644 --- a/packages/services/service-automation/src/builtin/crud-nodes.ts +++ b/packages/services/service-automation/src/builtin/crud-nodes.ts @@ -115,6 +115,11 @@ function resolveNodeFilter( const DROPPED_REASON_LABEL: Record = { readonly: 'the field is read-only (readonly: true)', readonly_when: 'the field is conditionally read-only (readonlyWhen; on multi-row updates: locked in ≥1 matched row)', + // [#6437] The map is `Record` on + // purpose: a reason added in `packages/spec` fails THIS file's typecheck + // until it is worded, which is how the flow author keeps getting a true + // sentence instead of a fall-through label. Keep it exhaustive. + primary_key: "the field is the object's primary key and the value sent is not an identifier — the row(s) are identified by the id argument or the filter, so writing it would have overwritten their primary key (pass a scalar id, or put an id set in the filter)", }; function droppedFieldsWarning(nodeType: string, e: DroppedFieldsEvent): string { diff --git a/packages/spec/src/api/error-code-ledger.zod.ts b/packages/spec/src/api/error-code-ledger.zod.ts index da81a2205d..281ce69056 100644 --- a/packages/spec/src/api/error-code-ledger.zod.ts +++ b/packages/spec/src/api/error-code-ledger.zod.ts @@ -246,7 +246,7 @@ export const ERROR_CODE_LEDGER = { 'ERR_BULK_RESULT_MISMATCH', 'ERR_DATASOURCE_UNAVAILABLE', 'ERR_DRIVER_CONNECT', - 'ERR_READONLY_FIELD_REJECTED', // strictReadonlyWrites: the write would strip read-only fields, so it was refused (#5126) + 'ERR_READONLY_FIELD_REJECTED', // strictReadonlyWrites: the write would strip caller-supplied fields, so it was refused (#5126; since #6437 that covers the primary_key strip too — one code, `drops` carries the per-reason breakdown) 'ERR_SUMMARY_RECOMPUTE', 'VALIDATION_FAILED', ], diff --git a/packages/spec/src/contracts/data-engine.ts b/packages/spec/src/contracts/data-engine.ts index d7203dc4ab..954aab32f4 100644 --- a/packages/spec/src/contracts/data-engine.ts +++ b/packages/spec/src/contracts/data-engine.ts @@ -17,12 +17,14 @@ import type { IDataDriver } from './data-driver.js'; * * `onFieldsDropped` is invoked by the engine when caller-supplied write fields * are LEGALLY stripped from the payload before the driver write — static - * `readonly` (#2948), a TRUE `readonlyWhen` predicate (#3042), or an + * `readonly` (#2948), a TRUE `readonlyWhen` predicate (#3042), an * implicitly-readonly runtime-owned type (#5503; `RUNTIME_OWNED_FIELD_TYPES`, - * today `autonumber` — the one strip that also runs on INSERT). The write - * still succeeds; the listener exists so callers that report per-field success - * (e.g. a flow's `update_record` step) can surface a warning instead of a - * silent success (#3356's masked stage write-backs). + * today `autonumber` — the one strip that also runs on INSERT), or the + * primary-key strip of a payload `id` the update dispatch has ruled is not an + * identifier (#6437). The write still succeeds; the listener exists so callers + * that report per-field success (e.g. a flow's `update_record` step) can + * surface a warning instead of a silent success (#3356's masked stage + * write-backs). * * Lives on the TS contract — NOT in the serializable Zod options schemas * (`EngineUpdateOptionsSchema` etc.): a function is unrepresentable in JSON @@ -47,16 +49,32 @@ export interface WriteObservabilityOptions { * would have survived. The strip passes still run — that is how the engine * learns WHICH fields would go — but their result is discarded. * - * It covers every drop `onFieldsDropped` reports, i.e. both + * It covers every drop `onFieldsDropped` reports — coverage DERIVED from the + * reported set, never an enumeration frozen at #5126. Today that is all three * `DroppedFieldsEvent['reason']` arms: static `readonly: true` (#2948, which - * only runs for non-system callers) and a TRUE `readonlyWhen` predicate - * (#3042, which runs for every caller, `isSystem` included) — plus, since - * #5503, the implicitly-readonly runtime-owned strip, which reports under - * the same `'readonly'` arm (see the INSERT section below). Covering only - * the static arm would leave a trusted caller — the very caller this option - * exists for, one that already passes `{ context: { isSystem: true } }` and - * is therefore exempt from the static strip — still losing `readonlyWhen` - * fields in silence, which is the bug this option exists to abolish. + * only runs for non-system callers), a TRUE `readonlyWhen` predicate (#3042, + * which runs for every caller, `isSystem` included), and the `primary_key` + * strip (#6437) — plus, since #5503, the implicitly-readonly runtime-owned + * strip, which reports under the same `'readonly'` arm rather than adding one + * (see the INSERT section below). Covering only the static arm would leave a + * trusted caller — the very caller this option exists for, one that already + * passes `{ context: { isSystem: true } }` and is therefore exempt from the + * static strip — still losing `readonlyWhen` fields in silence, which is the + * bug this option exists to abolish. + * + * ⚠️ **A new `reason` therefore adds a new REFUSAL, by construction** — the + * price of the derived coverage above, paid deliberately when `primary_key` + * landed (#6437). Since that change a `strictReadonlyWrites` caller that puts + * a ruled-non-key value in `data.id` is REFUSED, where it previously got a + * success whose `id` had been silently dropped. That is this option's whole + * promise ("don't half-apply my payload") reaching one more strip class, not + * a second policy: the strip already ran and already discarded the key. + * + * The flag's NAME is narrower than its coverage and stays that way on + * purpose — renaming a shipped in-process option is a separate acceptance + * decision, and the coverage sentence above, not the name, is the contract. + * The refusal error names what actually happened PER REASON, so a + * `primary_key` refusal never claims the field was read-only. * * `onFieldsDropped` does NOT fire on a write this option refuses. The two are * alternative outputs of one seam, not a sequence: `DroppedFieldsEvent` is @@ -68,9 +86,12 @@ export interface WriteObservabilityOptions { * * `ERR_READONLY_FIELD_REJECTED` (registered in `ERROR_CODE_LEDGER` under * `@objectstack/objectql`), carrying the FULL list of rejected fields - * accumulated across both strip passes — one error naming everything, so a - * caller fixes its payload once instead of one round-trip per field. Engines - * identify it by `code`, not `instanceof`, so it survives package boundaries. + * accumulated across every strip pass the operation runs — one error naming + * everything, so a caller fixes its payload once instead of one round-trip + * per field. Engines identify it by `code`, not `instanceof`, so it survives + * package boundaries. The code is stable across reasons deliberately: a + * caller catches ONE code and reads `drops` for the per-reason breakdown, + * which is why adding a reason does not add an error code (#6437). * * ## In-process only — what a REMOTE caller observes * diff --git a/packages/spec/src/data/data-engine.test.ts b/packages/spec/src/data/data-engine.test.ts index 3fc91c1c1d..4407bb5a48 100644 --- a/packages/spec/src/data/data-engine.test.ts +++ b/packages/spec/src/data/data-engine.test.ts @@ -23,6 +23,7 @@ import { DataEngineExecuteRequestSchema, DataEngineVectorFindRequestSchema, DataEngineRequestSchema, + DroppedFieldsEventSchema, } from './data-engine.zod'; describe('DataEngineFilterSchema', () => { @@ -1019,3 +1020,37 @@ describe('Integration Tests', () => { }); }); + +describe('DroppedFieldsEventSchema.reason (#3407, widened by #6437)', () => { + // The ACCEPTANCE surface: this enum is what validates on the protocol + // responses that carry `droppedFields` (`api/batch.zod.ts`, + // `api/protocol.zod.ts`), so the accepted set is the contract, not a label. + it('accepts all three reasons the write path can report', () => { + for (const reason of ['readonly', 'readonly_when', 'primary_key'] as const) { + const parsed = DroppedFieldsEventSchema.safeParse({ + object: 'task', fields: ['id'], reason, + }); + expect(parsed.success, `reason ${reason} should be accepted`).toBe(true); + } + }); + + it('primary_key is the value the engine reports for the ruled-non-id strip (#6262/#6433, #6435)', () => { + const parsed = DroppedFieldsEventSchema.parse({ + object: 'task', fields: ['id'], reason: 'primary_key', + }); + expect(parsed).toEqual({ object: 'task', fields: ['id'], reason: 'primary_key' }); + }); + + it('still REJECTS a reason outside the vocabulary — widening is deliberate, not open-ended', () => { + // The enum grows by decision (a new legal strip class), never by a + // producer inventing a string. Rejection is asserted on the issue's own + // `code` and `path`, not on truthiness of `success`. + const parsed = DroppedFieldsEventSchema.safeParse({ + object: 'task', fields: ['id'], reason: 'dispatch_ruled', + }); + expect(parsed.success).toBe(false); + const issue = parsed.success ? undefined : parsed.error.issues[0]; + expect(issue?.code).toBe('invalid_value'); + expect(issue?.path).toEqual(['reason']); + }); +}); diff --git a/packages/spec/src/data/data-engine.zod.ts b/packages/spec/src/data/data-engine.zod.ts index dec0e4f9d9..d4f4f9c181 100644 --- a/packages/spec/src/data/data-engine.zod.ts +++ b/packages/spec/src/data/data-engine.zod.ts @@ -200,11 +200,23 @@ export const EngineUpdateOptionsSchema = lazySchema(() => BaseEngineOptionsSchem /** * One strip event on a write path: the engine dropped caller-supplied field(s) - * from the payload for a LEGAL reason (static `readonly` (#2948) or a TRUE - * `readonlyWhen` predicate) and completed the write without them. The write - * itself still succeeds — stripping is legitimate semantics, not an error — - * but callers that report success per requested field (e.g. a flow's - * `update_record` step) need to know which fields never landed (#3407). + * from the payload for a LEGAL reason — a read-only lock (static `readonly` + * (#2948) or a TRUE `readonlyWhen` predicate (#3042)), or the primary-key strip + * that keeps a ruled-non-key payload value out of the id column (#6437) — and + * completed the write without them. The write itself still succeeds — + * stripping is legitimate semantics, not an error — but callers that report + * success per requested field (e.g. a flow's `update_record` step) need to know + * which fields never landed (#3407). + * + * `reason` is an OPEN vocabulary in the sense that matters to a consumer: it + * grows as the write path gains legal strips, and it is widened deliberately + * rather than force-fitted. Reusing an existing arm for a new strip class would + * make `reason` LIE, which is strictly worse than the silence it replaces — the + * judgement PR #6433 recorded in a code comment and #6437 discharged by adding + * `primary_key`. A consumer that branches on `reason` must therefore be + * exhaustive (a `Record` tsc re-checks), never + * a binary test whose `else` arm silently relabels every future value as + * read-only. * * Delivered in-process via the `onFieldsDropped` listener on the write options * (see `WriteObservabilityOptions` in `contracts/data-engine.ts`). The @@ -223,9 +235,26 @@ export const DroppedFieldsEventSchema = lazySchema(() => z.object({ * stripped for non-system contexts (#2948); * - `readonly_when` — a `readonlyWhen` predicate locked the field for the * target record's state; on a multi-row update this is "locked in ≥1 - * matched row" semantics (#3042). + * matched row" semantics (#3042); + * - `primary_key` — the field is the object's primary key and the engine had + * ALREADY RULED the submitted value is not one, so writing it would have + * overwritten the identity of the row(s) the call actually targets + * (#6262 / PR #6433 on the multi branch, #6435 on the by-id branch; #6437). + * The row is identified by the `id` argument or by the predicate, never by + * this payload key. NOT a read-only lock: a TRUTHY SCALAR `data.id` IS the + * bound key and is left in place, so this reason names the strip of a + * payload `id` the update-dispatch ruling (`resolveEngineUpdateDispatch`) + * has already classified as *not* an identifier — an authoring error the + * write survives without. + * + * `primary_key` names the FIELD's role, not the offending value's shape, on + * purpose: `not_a_primary_key` would describe the value and become false the + * day the strip widens to the same-value truthy-scalar no-op the engine + * currently leaves alone. `primary_key` stays true either way, and sits in + * the same register as the two read-only arms — each answers "what about this + * FIELD caused the strip?". */ - reason: z.enum(['readonly', 'readonly_when']).describe('Why the fields were dropped: static readonly (#2948) or a TRUE readonlyWhen predicate (#3042)'), + reason: z.enum(['readonly', 'readonly_when', 'primary_key']).describe('Why the fields were dropped: static readonly (#2948), a TRUE readonlyWhen predicate (#3042), or the primary-key strip of a payload id the engine ruled is not an identifier (#6437)'), }).describe('A write-path strip event: caller-supplied fields legally dropped from the payload (#3407)')); // --------------------------------------------------------------------------