From 564d57f786a216df0f2d8579a0bfde5ab62aab86 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 13:00:10 +0000 Subject: [PATCH 1/4] wip: insertManyData reports droppedFields at batch level Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RuoNSXUbBoWHkNS4AknTrM --- .../src/protocol.dropped-fields.bulk.test.ts | 123 +++++++++++++++--- .../src/protocol.readonly-insert.test.ts | 19 ++- packages/metadata-protocol/src/protocol.ts | 113 +++++++++++----- .../engine-autonumber-runtime-owned.test.ts | 14 +- packages/objectql/src/engine.ts | 16 ++- packages/spec/src/api/protocol.zod.ts | 6 +- 6 files changed, 219 insertions(+), 72 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.dropped-fields.bulk.test.ts b/packages/metadata-protocol/src/protocol.dropped-fields.bulk.test.ts index d52162a4ad..8d2f2d6d64 100644 --- a/packages/metadata-protocol/src/protocol.dropped-fields.bulk.test.ts +++ b/packages/metadata-protocol/src/protocol.dropped-fields.bulk.test.ts @@ -7,13 +7,16 @@ // single-write now reports, and (b) thread the caller's execution `context` to // the engine so RLS/FLS/`readonlyWhen` run under the caller — a gap the // pre-#3455 `updateManyData`/`batchData` loops had. Channels: -// - updateManyData / batchData → per-row `droppedFields` on each result row; -// - insertManyData → per-row `droppedFields` on each outcome; -// - createManyData → aggregated top-level `droppedFields` (its -// response has no per-row slot, so a union is the only view it can -// represent; read a name there as "at least one row dropped this field", -// never "every row dropped the same set" — ruling C (#14147) exempts keys a -// `beforeInsert` hook assigned, recorded per row, so rows CAN differ). +// - updateManyData / batchData → per-row `droppedFields` on each result row, +// earned mechanically: one engine call per row, so that call's events are +// that row's; +// - createManyData / insertManyData → aggregated top-level `droppedFields`. +// `createManyData`'s response has no per-row slot, so a union is the only +// view it can represent. `insertManyData` HAS one (`outcomes[i]`) and still +// reports at the top level, because ruling C (#14147) exempts keys a +// `beforeInsert` hook assigned — recorded per row — so rows CAN differ and +// the union cannot be resolved back to rows. Read a name in either as "at +// least one row dropped this field", never "this row dropped it". import { describe, it, expect, vi } from 'vitest'; import { assertEngineUpdateDispatch, assertEngineFindOnePredicate } from '@objectstack/metadata-core'; @@ -130,22 +133,48 @@ describe('createManyData — aggregated top-level droppedFields (#3455)', () => }); }); -describe('insertManyData — per-row droppedFields on outcomes (#3455)', () => { - it('attaches the create strip to the matching outcome row only', async () => { - // [#14147] The engine's listener carries no row index — it reports the - // batch UNION — so row precision here is recovered by asking which row - // SUPPLIED each dropped name. That recovery is what this case pins. - const insertMany = vi.fn(async (object: string, rows: any[], options?: any) => { - if (rows.some((r) => r && 'approval_status' in r)) { +describe('insertManyData — BATCH-LEVEL droppedFields that names no row (#3455)', () => { + // This block used to pin the opposite: a per-row `droppedFields` on each + // outcome, reconstructed from the batch union by asking which row SUPPLIED + // each dropped name. Ruling C (#14147) falsifies that reconstruction — the + // strip runs after `beforeInsert` and exempts keys a hook assigned, per row — + // so the cases are REPLACED rather than amended. The engine double below + // models the exemption, which the old one had no concept of; that is why the + // old case stayed green through exactly the shape it was written for. + + /** + * `hookStamps` names the rows whose `beforeInsert` hook re-assigns + * `approval_status`. Those rows KEEP it (ruling C); the others are stripped. + * Either way the engine reports ONE event, the union over the batch, with no + * row index — which is the real `engine.insert` contract this stands in for. + */ + function makeInsertMany(hookStamps: ReadonlySet = new Set(), dead: ReadonlySet = new Set()) { + return vi.fn(async (object: string, rows: any[], options?: any) => { + const strippedAny = rows.some((r, i) => r && 'approval_status' in r && !hookStamps.has(i) && !dead.has(i)); + if (strippedAny) { options?.onFieldsDropped?.({ object, fields: ['approval_status'], reason: 'readonly' }); } return rows.map((r, i) => { - const { approval_status: _forged, ...kept } = r ?? {}; - return { ok: true, record: { id: `rec-${i + 1}`, ...kept } }; + if (dead.has(i)) return { ok: false, error: { code: 'VALIDATION_FAILED' } }; + const { approval_status: forged, ...kept } = r ?? {}; + // A stripped `readonly` field falls back to its `defaultValue` (#3043), + // so the key is present on the row that DID lose the caller's value — + // the reason a post-hoc check against `outcomes[i].record` cannot + // recover row precision either. + const value = hookStamps.has(i) ? forged : SCHEMA.fields.approval_status.defaultValue; + return { ok: true, record: { id: `rec-${i + 1}`, ...kept, approval_status: value } }; }); }); + } + + function makeProtocol(insertMany: ReturnType) { const engine = { registry: { getObject: () => SCHEMA }, insertMany }; - const p = new ObjectStackProtocolImplementation(engine as any); + return new ObjectStackProtocolImplementation(engine as any); + } + + it('surfaces the batch union on the response and hangs nothing on any outcome', async () => { + const insertMany = makeInsertMany(); + const p = makeProtocol(insertMany); const res: any = await p.insertManyData({ object: 'approval_case', @@ -156,15 +185,71 @@ describe('insertManyData — per-row droppedFields on outcomes (#3455)', () => { context: { userId: 'u1' }, }); - expect(res.outcomes[0]).not.toHaveProperty('droppedFields'); - expect(res.outcomes[1].droppedFields).toEqual([ + expect(res.droppedFields).toEqual([ { object: 'approval_case', fields: ['approval_status'], reason: 'readonly' }, ]); + for (const o of res.outcomes) expect(o).not.toHaveProperty('droppedFields'); // [#14147] The ingress hands the caller's row over WHOLE — judging it is // the engine's job now, and this assertion is what would catch a // reintroduced second strip at this seam. expect(insertMany.mock.calls[0][1][1]).toHaveProperty('approval_status'); }); + + it('a hook exempts one row of the batch: the row that KEPT the value is not named', async () => { + // Row 0 and row 1 both forge `approval_status`. Row 1's `beforeInsert` hook + // re-assigns it, so ruling C keeps row 1's value and only row 0 is + // stripped. `f in supplied` — the reconstruction this response shape + // replaces — is true for BOTH, so it reported a dropped field on an outcome + // whose record carries the value that was written. + const p = makeProtocol(makeInsertMany(new Set([1]))); + + const res: any = await p.insertManyData({ + object: 'approval_case', + records: [ + { title: 'A', approval_status: 'approved' }, + { title: 'B', approval_status: 'approved' }, + ], + context: { userId: 'u1' }, + }); + + expect(res.outcomes[1].record.approval_status, 'the hook wrote it — ruling C keeps it').toBe('approved'); + expect(res.outcomes[1], 'a written value must never be reported as dropped').not.toHaveProperty('droppedFields'); + expect(res.outcomes[0], 'and the stripped row is not named either — the set is batch-level') + .not.toHaveProperty('droppedFields'); + expect(res.droppedFields).toEqual([ + { object: 'approval_case', fields: ['approval_status'], reason: 'readonly' }, + ]); + }); + + it('a row the batch culled supplied the name and is still not named', async () => { + // The engine's strip loop skips a row that already failed, so a dead row + // dropped nothing — but it supplied the key, which was enough for the + // reconstruction to name it on an outcome that carries no record at all. + const p = makeProtocol(makeInsertMany(new Set(), new Set([0]))); + + const res: any = await p.insertManyData({ + object: 'approval_case', + records: [ + { title: 'A', approval_status: 'approved' }, + { title: 'B', approval_status: 'approved' }, + ], + }); + + expect(res.outcomes[0].ok).toBe(false); + expect(res.outcomes[0]).not.toHaveProperty('droppedFields'); + expect(res.droppedFields).toEqual([ + { object: 'approval_case', fields: ['approval_status'], reason: 'readonly' }, + ]); + }); + + it('nothing dropped ⇒ the key is absent, keeping the omit-when-empty shape', async () => { + const p = makeProtocol(makeInsertMany()); + const res: any = await p.insertManyData({ + object: 'approval_case', + records: [{ title: 'A' }], + }); + expect(res).not.toHaveProperty('droppedFields'); + }); }); describe('batchData — per-row droppedFields + context threading (#3455)', () => { diff --git a/packages/metadata-protocol/src/protocol.readonly-insert.test.ts b/packages/metadata-protocol/src/protocol.readonly-insert.test.ts index f1d162c998..8c46ba50d7 100644 --- a/packages/metadata-protocol/src/protocol.readonly-insert.test.ts +++ b/packages/metadata-protocol/src/protocol.readonly-insert.test.ts @@ -216,27 +216,32 @@ describe('#14147 — the create ingress DELEGATES the readonly strip to engine.i ]); }); - it('insertManyData forwards every row whole and keeps ROW precision from the union', async () => { + it('insertManyData forwards every row whole and reports the union at BATCH level', async () => { const { p, inserts } = makeProtocol(); const res: any = await p.insertManyData({ object: 'approval_case', records: [{ title: 'A', approval_status: 'approved' }, { title: 'B' }], }); expect(inserts[0].data).toEqual([{ title: 'A', approval_status: 'approved' }, { title: 'B' }]); - // The engine's event is the batch UNION (its listener carries no row - // index); row precision is recovered by asking which row SUPPLIED the key. - expect(res.outcomes[0].droppedFields).toEqual([ + // The engine's event is the batch UNION and its listener carries no row + // index. It used to be resolved back to rows by asking which row SUPPLIED + // each name; ruling C (#14147) exempts keys a `beforeInsert` hook assigned, + // per row, so that is a different set. The union is reported where it is + // true — on the response — and no outcome is named. + expect(res.droppedFields).toEqual([ { object: 'approval_case', fields: ['approval_status'], reason: 'readonly' }, ]); - expect(res.outcomes[1].droppedFields, 'row B supplied none of the dropped names').toBeUndefined(); + for (const o of res.outcomes) { + expect(o, 'the union names no row').not.toHaveProperty('droppedFields'); + } }); }); describe('#14147 — engine listener wiring (the firing control for every assertion above)', () => { // The faces enumerated here are the ones whose RESPONSE carries // `droppedFields`: `CreateDataResponse`, `CloneDataResponse` (since #15703), - // `CreateManyDataResponse`, and the per-row results of `insertManyData` / - // `batchData`. That is every create face; the case after this one pins the + // `CreateManyDataResponse`, `insertManyData`'s batch-level key, and the + // per-row results of `batchData`. That is every create face; the case after this one pins the // clone by name so the enumeration cannot silently lose the face that was // the exclusion until its contract gained the member. it('every create face whose response carries droppedFields passes an onFieldsDropped listener to the engine', async () => { diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index f44ae04a2d..7b43df471e 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1994,12 +1994,14 @@ const CLONE_STRIP_FIELDS: readonly string[] = [ * [#3455] Collapse a batch's per-row `DroppedFieldsEvent`s into one event per * `(object, reason)` with the UNION of dropped field names. * - * Used by the bulk-create surface (`createManyData`), whose `{ object, records, - * count }` response has no per-row slot to hang a `droppedFields` on — a union - * is the only view that response can represent, which is the whole reason this - * collapse exists. (Since #14147 that strip is the ENGINE's, which reports one - * event per CALL for it, so the aggregation is over the runtime-owned per-row - * events.) + * Used by both bulk-create surfaces at BATCH level. `createManyData`'s + * `{ object, records, count }` response has no per-row slot to hang a + * `droppedFields` on — a union is the only view that response can represent, + * which is the whole reason this collapse exists. `insertManyData` has a + * per-row slot and reports here anyway: the slot is real, a per-row answer is + * not (its docblock states why), so it reports the union where the union is + * true. (Since #14147 that strip is the ENGINE's, which reports one event per + * CALL for it, so the aggregation is over the runtime-owned per-row events.) * * ⚠️ So read a name in a merged event as "AT LEAST ONE row dropped this field", * never "every row dropped the same set". Maintainer ruling C (#14147) put the @@ -2014,8 +2016,9 @@ const CLONE_STRIP_FIELDS: readonly string[] = [ * * Returns `[]` when nothing was dropped so callers can spread * `...(x.length ? { droppedFields: x } : {})` and keep the omit-when-empty shape. - * The per-row `insertMany`/`batch` paths carry their own per-row `droppedFields` - * instead — they have a per-row result to hang one on. + * The paths that DO keep row precision — `updateManyData` and `batchData` — + * earn it mechanically rather than by inference: each row is its own + * `engine.update` / `engine.insert` call, so that call's events are that row's. */ function mergeDroppedFieldEvents(events: DroppedFieldsEvent[]): DroppedFieldsEvent[] { if (events.length === 0) return []; @@ -12657,8 +12660,9 @@ export class ObjectStackProtocolImplementation implements // author-declared `readonly` — so ONE listener carries both, and this // seam no longer diffs payloads to recover a strip it performed itself. // AGGREGATED: the `{ records, count }` response has no per-row slot, so - // a union is the only representable view here. (`insertManyData`, which - // HAS a per-row slot, recovers row precision from the same union.) + // a union is the only representable view here. (`insertManyData` HAS a + // per-row slot and still reports at the top level — the union cannot be + // resolved to rows at either seam; see its own docblock.) const dropped: DroppedFieldsEvent[] = []; const opts: any = { onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); } }; if (request.context !== undefined) opts.context = request.context; @@ -12685,27 +12689,70 @@ export class ObjectStackProtocolImplementation implements * degradation re-run of the good rows' beforeInsert hooks. Requires an * engine with `insertMany` (ObjectQL has it); absent that, callers should * fall back to createManyData. + * + * ## `droppedFields` is BATCH-LEVEL here, and names no row + * + * This response HAS a per-row slot (`outcomes[i]`) and still reports the + * drop set at the top level, which is the one thing about this method worth + * writing down. The slot exists; what does not exist is a per-row ANSWER to + * put in it. + * + * Every create-side strip is the ENGINE's — runtime-owned `autonumber` + * (#5503) and static author-declared `readonly` (#14147) — and its + * `onFieldsDropped` event is the UNION over the batch, the listener + * signature carrying no row index. This seam used to reconstruct a row set + * from that union by asking which rows SUPPLIED each dropped name + * (`[...engineDropped].filter((f) => f in supplied)`). That is not the same + * question, and it named rows that are not at fault: + * + * - Maintainer ruling C (#14147) put the static-`readonly` strip INSIDE + * `engine.insert`, AFTER the `beforeInsert` hooks, where it exempts keys + * a hook itself assigned — recorded PER ROW + * (`hookWrittenKeys: rowHookWrittenKeys[i]`, + * `packages/objectql/src/engine.ts`). A hook that stamps a protected key + * on some rows and not others makes those rows drop DIFFERENT sets, so a + * row that supplied the name and had it KEPT was reported as having lost + * it — a dropped-field warning on an outcome whose `record` carries the + * value that was written. + * - A row the batch culled before the strip (a validation failure — + * `ok: false`) dropped nothing at all, because the strip loop skips it; + * supplying the name was still enough to have it named. + * + * ⛔ And the outcome's own `record` cannot repair the inference either, so + * a post-hoc "is the key still there?" check is not the cheaper route: a + * stripped `readonly` field is RE-DEFAULTED over exactly the keys the strip + * took (#3043's contract — a forged `approval_status` comes back `draft`), + * and a stripped `autonumber` is refilled by `applyAutonumbers` afterwards. + * On both the key is PRESENT on the row that really did drop it, so that + * check would delete TRUE attributions while leaving the hook-exempt false + * one standing. Comparing values fails for the case `hookWrittenKeys` was + * built for in the first place — the hook assigning the value the caller + * also sent. + * + * So the honest set — `{rows whose payload carried N}` minus `{rows whose + * beforeInsert hook assigned N}` — is computed per row upstream and is not + * reachable through this seam. Rather than name rows on a guess, the union + * is reported where it is true: on the response. ⚠️ Read a name here as + * "AT LEAST ONE row dropped this field", never "this row dropped it". + * Restoring row precision means giving the engine's drop report a per-row + * channel (an `onFieldsDropped` signature that carries the row), never a + * reconstruction at this call site. */ - async insertManyData(request: { object: string, records: any[], context?: any }): Promise<{ object: string; outcomes: Array<{ ok: boolean; record?: any; error?: unknown; droppedFields?: DroppedFieldsEvent[] }> }> { + async insertManyData(request: { object: string, records: any[], context?: any }): Promise<{ object: string; outcomes: Array<{ ok: boolean; record?: any; error?: unknown }>; droppedFields?: DroppedFieldsEvent[] }> { this.assertObjectRegistered(request.object); // [#3770] const engineInsertMany = (this.engine as any)?.insertMany; if (typeof engineInsertMany !== 'function') { throw new Error('insertManyData requires an engine with insertMany (framework#3172)'); } - // [#5503/#14147] Every create-side strip is the ENGINE's — runtime-owned - // `autonumber` (#5503) and static author-declared `readonly` (#14147) — - // and its `onFieldsDropped` event is the UNION over the batch, the - // listener signature carrying no row index. This partial-success path HAS - // a per-row slot (`outcomes[i]`), and row precision is recoverable - // without an index: the strip only removes keys the ROW ITSELF supplied, - // so a dropped name belongs to exactly the rows whose supplied payload - // carried it. Without this the import surface (which prefers this path - // over createManyData) would drop columns with nothing but a server log - // to show for it. - const engineDropped = new Set(); - const opts: any = { onFieldsDropped: (e: DroppedFieldsEvent) => { for (const f of e.fields) engineDropped.add(f); } }; + // [#5503/#14147] The engine's events are collected WHOLE and merged — + // the same handling `createManyData` gives them, and for the reason + // spelled out on this method: the union is reportable, a row set is not. + // Keeping each event also keeps its own `reason`, which the old + // flatten-into-a-Set-and-relabel could not. + const dropped: DroppedFieldsEvent[] = []; + const opts: any = { onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); } }; if (request.context !== undefined) opts.context = request.context; - const outcomes: Array<{ ok: boolean; record?: any; error?: unknown; droppedFields?: DroppedFieldsEvent[] }> = await engineInsertMany.call( + const outcomes: Array<{ ok: boolean; record?: any; error?: unknown }> = await engineInsertMany.call( this.engine, request.object, request.records, @@ -12720,18 +12767,12 @@ export class ObjectStackProtocolImplementation implements if (o?.record) omitInternalFieldsFromWriteResponse(outcomeSchema, o.record); } } - if (Array.isArray(outcomes)) { - for (let i = 0; i < outcomes.length; i++) { - if (!outcomes[i]) continue; - const supplied = (request.records?.[i] ?? {}) as Record; - const mine = [...engineDropped].filter((f) => f in supplied); - const events: DroppedFieldsEvent[] = []; - if (mine.length > 0) events.push({ object: request.object, fields: mine, reason: 'readonly' }); - const merged = mergeDroppedFieldEvents(events); - if (merged.length > 0) outcomes[i].droppedFields = merged; - } - } - return { object: request.object, outcomes }; + const merged = mergeDroppedFieldEvents(dropped); + return { + object: request.object, + outcomes, + ...(merged.length > 0 ? { droppedFields: merged } : {}), + }; } async updateManyData(request: UpdateManyDataRequest & { context?: any }): Promise { diff --git a/packages/objectql/src/engine-autonumber-runtime-owned.test.ts b/packages/objectql/src/engine-autonumber-runtime-owned.test.ts index b93e8b6633..c80fdcf463 100644 --- a/packages/objectql/src/engine-autonumber-runtime-owned.test.ts +++ b/packages/objectql/src/engine-autonumber-runtime-owned.test.ts @@ -305,9 +305,15 @@ describe('#5503 — autonumber is runtime-owned: bulk-create surfaces', () => { expect((res.droppedFields ?? []).flatMap((e: DroppedFieldsEvent) => e.fields)).toContain('account_number'); }); - it('insertManyData keeps ROW precision — only the forging row is reported', async () => { + it('insertManyData reports the union at BATCH level and names no row', async () => { // The import runner prefers this partial-success surface, so it is the one - // that has to stay honest about which row lost its record number. + // that has to stay honest — and honest here means naming no row: the + // engine's event carries no row index, and the two facts that would let a + // caller resolve it are both unavailable at the protocol seam. The row + // records below are the second one: BOTH come back carrying + // `account_number`, because the strip is followed by `applyAutonumbers`. + // So "is the key still on the row?" answers the same for the row that was + // stripped and the row that was not. const rig = await makeEngine(); const res: any = await rig.protocol.insertManyData({ object: 'an_account', @@ -317,8 +323,8 @@ describe('#5503 — autonumber is runtime-owned: bulk-create surfaces', () => { ], }); expect(res.outcomes.map((o: any) => o.record.account_number)).toEqual(['ACC-0001', 'ACC-0002']); - expect(res.outcomes[0].droppedFields).toBeUndefined(); - expect(res.outcomes[1].droppedFields.flatMap((e: DroppedFieldsEvent) => e.fields)).toEqual(['account_number']); + for (const o of res.outcomes) expect(o).not.toHaveProperty('droppedFields'); + expect((res.droppedFields ?? []).flatMap((e: DroppedFieldsEvent) => e.fields)).toEqual(['account_number']); }); }); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index d14c056148..bcc7d83b4d 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -11091,10 +11091,18 @@ export class ObjectQL implements IObjectQLEngine { * outcome array — the records ARE written. * * `onFieldsDropped` (#3407) is forwarded to `insert`, so the runtime-owned - * strip (#5503) reports here too. The event carries no row index — it is the - * UNION over the batch — but the strip only ever removes keys the row itself - * supplied, so a caller holding the input rows can attribute each name back to - * the rows that carried it (`insertManyData` does exactly that). + * strip (#5503) reports here too. ⚠️ The event carries no row index — it is + * the UNION over the batch — and a caller holding the input rows CANNOT + * resolve it back to rows. "Which rows supplied N" is a different set from + * "which rows dropped N": since ruling C (#14147) the static-`readonly` strip + * runs after `beforeInsert` and exempts keys a hook assigned, per row + * (`hookWrittenKeys: rowHookWrittenKeys[i]`, above), so two rows that both + * supplied N can differ on whether N survived — and a row this method culled + * before the strip dropped nothing at all. Nor does the returned row answer + * it: a stripped `readonly` field is re-defaulted and a stripped `autonumber` + * is refilled, so the key is present on the row that did drop it. Read a + * reported name as "at least one row dropped this field"; `insertManyData` + * surfaces it at batch level for exactly this reason. */ async insertMany(object: string, rows: any[], options?: DataEngineInsertOptions & WriteObservabilityOptions): Promise { if (!Array.isArray(rows)) throw new Error('insertMany expects an array of rows'); diff --git a/packages/spec/src/api/protocol.zod.ts b/packages/spec/src/api/protocol.zod.ts index 34019bf779..5265288c5b 100644 --- a/packages/spec/src/api/protocol.zod.ts +++ b/packages/spec/src/api/protocol.zod.ts @@ -2369,8 +2369,10 @@ export const CreateManyDataResponseSchema = lazySchema(() => z.object({ 'tracked per row: rows where a hook stamped a protected key drop a different set from ' + 'rows where it did not. 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.)' + 'backward-compatible. (The same reading applies to the partial-success bulk create, whose ' + + 'batch-level `droppedFields` names no row for the same reason. The paths that DO carry ' + + 'per-row `droppedFields` on each result are the bulk UPDATE and the mixed batch, where each ' + + 'row is its own engine call — see BatchOperationResultSchema.)' ), })); From c6301aa2254e45d99039b6e9007544882bdd3d1f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 13:01:32 +0000 Subject: [PATCH 2/4] wip: changeset --- ...0-insertmany-dropped-fields-name-no-row.md | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 .changeset/17290-insertmany-dropped-fields-name-no-row.md diff --git a/.changeset/17290-insertmany-dropped-fields-name-no-row.md b/.changeset/17290-insertmany-dropped-fields-name-no-row.md new file mode 100644 index 0000000000..fecd300a32 --- /dev/null +++ b/.changeset/17290-insertmany-dropped-fields-name-no-row.md @@ -0,0 +1,71 @@ +--- +'@objectstack/metadata-protocol': minor +'@objectstack/objectql': patch +'@objectstack/spec': patch +--- + +fix(metadata-protocol): `insertManyData` reports the dropped-field union at BATCH level instead of naming rows it cannot identify (#17290) + +**BREAKING** — `insertManyData`'s response moves `droppedFields` from each +outcome to the response itself: + +``` +FROM { object, outcomes: [{ ok, record, droppedFields? }, …] } +TO { object, outcomes: [{ ok, record }, …], droppedFields? } +``` + +The set reported is the same set. What is gone is a per-row attribution that +could not be computed here and was wrong whenever it mattered. + +**What it got wrong.** Every create-side strip is the engine's, and its +`onFieldsDropped` event is the UNION over the batch — the listener signature +carries no row index. This seam reconstructed a row set from that union by +asking which rows SUPPLIED each dropped name +(`[...engineDropped].filter((f) => f in supplied)`), on the stated premise that +"the strip only removes keys the ROW ITSELF supplied, so a dropped name belongs +to exactly the rows whose supplied payload carried it". Maintainer ruling C +falsifies the premise: the static-`readonly` strip runs INSIDE `engine.insert`, +AFTER the `beforeInsert` hooks, and exempts keys a hook itself assigned — +recorded per row (`hookWrittenKeys: rowHookWrittenKeys[i]`). So in a batch where +a hook stamps a protected key on some rows and not others: + +- row A supplied `approval_status`, no hook write ⇒ stripped, enters the union; +- row B supplied `approval_status`, its hook re-assigned it ⇒ **kept and + written**; +- and row B's outcome carried `droppedFields: [{ fields: ['approval_status'] }]` + on a record that still held `approval_status`. + +A row the batch culled before the strip ran (a per-row validation failure) was +named on the same test, having dropped nothing at all. + +⇒ A wrong attribution costs the reader a wrong investigation, and the import +surface — which prefers this path over `createManyData` — is the consumer most +likely to act on it while reconciling what landed. + +**Why not attribute per row instead.** The honest set is `{rows whose payload +carried N}` minus `{rows whose beforeInsert hook assigned N}`, and the second +half is computed per row upstream but does not cross this seam. The outcome's +own `record` cannot stand in for it: a stripped `readonly` field is RE-DEFAULTED +over exactly the keys the strip took, and a stripped `autonumber` is refilled by +`applyAutonumbers` — so on both, the key is PRESENT on the row that really did +drop it, and a post-hoc "is the key still there?" check would delete true +attributions while leaving the hook-exempt false one standing. Comparing values +fails on the very case `hookWrittenKeys` exists for: the hook assigning the +value the caller also sent. Restoring row precision means giving the engine's +drop report a per-row channel, not a reconstruction at the call site. + +**Prose corrected with it**, by CLAIM rather than by spelling — the docblock +that authorised the inference is the thing that re-authorises the next author: +`insertManyData`'s own docblock and `createManyData`'s parenthetical +(`@objectstack/metadata-protocol`), `mergeDroppedFieldEvents`'s closing +sentence, `engine.insertMany`'s docblock claim that "a caller holding the input +rows can attribute each name back to the rows that carried it" +(`@objectstack/objectql`, TSDoc emitted into its published `.d.ts`), and +`CreateManyDataResponseSchema.droppedFields`'s `.describe()` parenthetical +(`@objectstack/spec`, a string printed AT the customer). + +**Unchanged.** `updateManyData` and `batchData` keep per-row `droppedFields`, +and they always could: each row is its own `engine.update` / `engine.insert` +call, so that call's events are that row's — earned mechanically, not inferred. +`createManyData`'s aggregated shape is untouched. No strip changes, no row +changes, and the same field names are reported. From 96ee7580ed74f6ff804161ecc5df083bf0063e97 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 13:25:04 +0000 Subject: [PATCH 3/4] wip: adr-0087 disposition --- ...7290-insertmany-dropped-fields-name-no-row.md | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/.changeset/17290-insertmany-dropped-fields-name-no-row.md b/.changeset/17290-insertmany-dropped-fields-name-no-row.md index fecd300a32..f85ab720f1 100644 --- a/.changeset/17290-insertmany-dropped-fields-name-no-row.md +++ b/.changeset/17290-insertmany-dropped-fields-name-no-row.md @@ -6,16 +6,14 @@ fix(metadata-protocol): `insertManyData` reports the dropped-field union at BATCH level instead of naming rows it cannot identify (#17290) -**BREAKING** — `insertManyData`'s response moves `droppedFields` from each -outcome to the response itself: + -``` -FROM { object, outcomes: [{ ok, record, droppedFields? }, …] } -TO { object, outcomes: [{ ok, record }, …], droppedFields? } -``` - -The set reported is the same set. What is gone is a per-row attribution that -could not be computed here and was wrong whenever it mattered. +**BREAKING** — `@objectstack/metadata-protocol`'s `insertManyData` no longer hangs +`droppedFields` on each entry of `outcomes`; the response itself carries it, beside +`outcomes`, exactly as `createManyData` already does. A TypeScript consumer that read +the per-row member stops compiling, and the compiler names the site. The set reported +is the same set — what is gone is a per-row attribution that could not be computed +here and was wrong whenever it mattered. Nothing authored or stored changes shape. **What it got wrong.** Every create-side strip is the engine's, and its `onFieldsDropped` event is the UNION over the batch — the listener signature From 817db84a881548f857f75928a3b3777acea82977 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 13:32:48 +0000 Subject: [PATCH 4/4] wip: regenerate protocol reference docs --- content/docs/references/api/protocol.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content/docs/references/api/protocol.mdx b/content/docs/references/api/protocol.mdx index ce47202ca4..e75a041769 100644 --- a/content/docs/references/api/protocol.mdx +++ b/content/docs/references/api/protocol.mdx @@ -570,7 +570,7 @@ A write-path strip event: caller-supplied fields legally dropped from the payloa | **object** | `string` | ✅ | Object name | | **records** | `Record[]` | ✅ | Created records | | **count** | `number` | ✅ | Number of records created | -| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when' \| 'primary_key'> }[]` | optional | Write-observability: caller-supplied `readonly` fields the in-engine create-side strip (`engine.insert`, `isSystem`-gated) 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 this response is `{ object, records, count }` and has no per-row slot to hang a drop set on — a union is the only view it can represent. So read a name here as "at least one row dropped this field", NOT "every row dropped the same set": the strip runs INSIDE `engine.insert` after the `beforeInsert` hooks and exempts keys a hook itself wrote, tracked per row: rows where a hook stamped a protected key drop a different set from rows where it did not. 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: caller-supplied `readonly` fields the in-engine create-side strip (`engine.insert`, `isSystem`-gated) 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 this response is `{ object, records, count }` and has no per-row slot to hang a drop set on — a union is the only view it can represent. So read a name here as "at least one row dropped this field", NOT "every row dropped the same set": the strip runs INSIDE `engine.insert` after the `beforeInsert` hooks and exempts keys a hook itself wrote, tracked per row: rows where a hook stamped a protected key drop a different set from rows where it did not. 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 same reading applies to the partial-success bulk create, whose batch-level `droppedFields` names no row for the same reason. The paths that DO carry per-row `droppedFields` on each result are the bulk UPDATE and the mixed batch, where each row is its own engine call — see BatchOperationResultSchema.) | ### Nested Shape: `CreateManyDataResponse.droppedFields[number]`