Skip to content

Commit 657b6b7

Browse files
feat(objectql,metadata-protocol): validate and insertMany answer which row lost which field (#21041)
Fixes #20922 Clause-②: yes (widening) ## What changes The dry run and the partial-success batch insert now say which row lost which field. - **Dry run.** `ObjectQL.validate` answers `droppedFields` on each accepted row of `results`: the fields the write would strip from that row, one `DroppedFieldsEvent` per reason. `validateData` relays the verdict, so its answer now fills `ValidateDataResponseSchema.results[].droppedFields`. - **Commit.** `ObjectQL.insertMany` answers `droppedFields` on each `ok` outcome: the fields stripped from that row. `insertManyData` passes the outcomes through, and its declared return type now names the key. - The key is absent when nothing was taken from the row. A preview row the verdict refuses, and an `ok: false` outcome, carry none: a drop means the write completed without the field (the `DroppedFieldsEventSchema` contract), and the write does not complete that row. ## How - **Row attribution is recorded at the strips.** `stripComputedWriteFields` now also returns `droppedPerRow` (the keys taken from each row, index-aligned). The caller-write strips (`stripRuntimeOwnedFields`, the static `readonly` strip, and `stripReadonlyFields` on an `update`-mode preview) record each taken key into the existing union and into the taking row's own list, in the same step. Nothing is reconstructed from the union afterwards. That matters because a `beforeInsert` hook can exempt a key on one row and not another (`hookWrittenKeys`), so "which rows supplied N" is not "which rows dropped N". - **One builder for both channels.** `droppedFieldEvents(object, computed, readonly)` turns a strip result into events: one per reason, `computed` before `readonly`, the order the strips run. It builds the batch-level union events (`validate`'s listener, `insert`'s `insertDrops`) and the per-row lists, so the two cannot disagree on a reason or its order. There is no second strip and no second reason vocabulary. - **The batch-level union is unchanged.** The `onFieldsDropped` events of `insert`, `insertMany` and `validate` are still one per reason, naming no row, built from the same union arrays in the same order. `insertManyData`'s top-level `droppedFields` and both union docblocks (`engine.ts` `insertMany`, `protocol.ts` `insertManyData`) are byte-identical. The per-row channel is documented on `InsertManyRowOutcome` and at the code points instead. - **Untouched:** `insert(object, rows[])` still returns the records with no per-row slot. `strictReadonlyWrites` still refuses the whole batch before any outcome is built. The engine's aggregate door and the packaged-base door in `protocol.ts` are not touched. ## Spec docblocks (two TSDoc sentences, no schema change) - `packages/spec/src/api/protocol.zod.ts`, the `ValidateDataResponseSchema` docblock: "the engine's `validate` already runs the write's strips ... no server sets the key until it does" now says what the producer does. `validate` runs the computed-field strip and the static `readonly` / runtime-owned strips, records what each takes per row, and reports it on an accepted row. `validateData` relays it. An `update`-mode preview does not run the `readonlyWhen` or primary-key strips, so it can report fewer drops than the update it predicts. - `packages/spec/src/api/export.zod.ts`, the `ImportRowResultSchema` docblock: "pinned at the wire" now names what is pinned. The synchronous route is pinned by `import-dryrun-parity.test.ts`, and no test reads `warnings` off the async job's results. - Both are TSDoc comments, not `.describe()`. `check:generated` reports all 15 artifacts up to date after a spec build. The diff touches `packages/spec/src/**`, so the landing owes an at-tier contract review. ## Premise checks (measured at `9b0de7de7` before the first edit) - **H1 holds.** `engine.validate` built `computedStrip.dropped` and `readonlyDropped` across all rows and emitted one event per reason. It already built per-row `results`, and the per-row channel now rides them. - **H2 holds.** `insertManyData` calls `engine.insertMany`, which is `insert(…, { __partialRowErrors: true })`. The outcomes are assembled in `insert`'s partial-mode branch, and that is where the per-outcome report is attached, from the strips' own record. `insertManyData` adds nothing. - **H3, insert mode: no fork.** An `insert`-mode preview runs the same three strips the write runs (computed, runtime-owned, static `readonly` with its re-default), in the same order and under the same `isSystem` gate. A row's preview drops therefore equal its outcome drops (pinned row for row). The one known difference is the documented "no hooks run" gap: a key a `beforeInsert` hook assigns is reported by the preview and kept by the write. - **H3, update mode: a fork, not settled here.** An `update`-mode preview runs no `readonlyWhen` or primary-key strip, while the update it predicts does. The import route previews an upsert's matched rows in `update` mode and commits them through `updateData`. On a row whose `readonlyWhen` is true (for example the showcase invoice's `tax_rate` once `status == 'paid'`), the dry run names nothing for that field and the update drops it under `readonly_when`. This PR keeps the existing named limit, and the spec docblock now states it. The options and their costs are in the report on the card. - **H4.** Both sentences are TSDoc, so no generated artifact moved. ## Tests The tests ran at `24ca898f8` / `99a7547af`. HEAD `886ad2c43` adds only a merge of `origin/main`, whose three commits touch none of `objectql`, `metadata-protocol`, `spec` or `rest`. The gate union ran at `886ad2c43`. - New `packages/objectql/src/engine-per-row-dropped-fields.test.ts` (14 cases, recording driver). It pins a formula column (`computed`) and a `readonly` column (`readonly`) through the dry run, the commit, and both relayed through the real protocol (`validateData`, `insertManyData`). A clean row carries none (the control), and the batch-level union is unchanged. A refused row and a failed outcome carry none. A hook-exempted row is not named. Under `isSystem`, only the computed strip reports. `update`-mode previews report per row too. - `engine-autonumber-runtime-owned.test.ts`: the case that pinned "names no row" against the real engine now pins the row that lost `account_number` on its own outcome, beside the unchanged union. - `protocol.dropped-fields.bulk.test.ts`: a new case checks that an engine-attributed outcome passes through as answered. The existing cases still pin that the seam derives nothing from the union. - `pnpm --filter @objectstack/objectql exec vitest run --project local --maxWorkers=2`: 349 files, 6835 tests passed. - `pnpm --filter @objectstack/metadata-protocol exec vitest run --maxWorkers=2`: 195 passed / 3 skipped files, 2897 passed / 19 skipped tests. - `pnpm --filter @objectstack/objectql --filter @objectstack/metadata-protocol run typecheck`: exit 0 (`check:test-typecheck` OK, ledger unchanged). - Downstream consumers, against rebuilt `dist/`: all 25 `packages/rest/src/import-*.test.ts` files (622 passed, 21 skipped), and four `plugin-security` preview/write tests (132 passed). - **Ablation A1** (via `scripts/ablation-replace.mjs`, which landed and restored the mutation on disk; the test reads `engine.ts` from `src`): the `insertMany` outcome was given the batch union instead of its row's list. Result: 4 failed / 10 passed. The tree was restored to the HEAD blob, and `git diff HEAD` was empty. - **Ablation A2**: the same on `validate`'s rows. Result: 5 failed / 9 passed, then restored. - **Reverse check of the cross-package type read**: a typo key on `insertManyData`'s outcome (read through `@objectstack/metadata-protocol`'s rebuilt `.d.ts`) turned `check:test-typecheck` red (1 type error in the new file). It was then restored. ## Gates - `node scripts/pm/dispatch-gates.mjs --commands` was derived at `886ad2c43` and named 89 families. All were run: 87 exited 0, and 2 exited 3. Reconciled with `--ran`: "89 derived famil(ies) accounted for, 87 run, 2 NOT-MEASURED (2 DERIVED from a recorded exit 3)". - NOT MEASURED: `check:dual-build-cjs-loads`, `check:type-check-debt`. Reason: PREREQUISITE NOT MET, because both read a full workspace build this worktree does not have. CI builds it. - NOT MEASURED locally: `packages/runtime` integration tier (`undeclared-field-write-driver-split.integration.test.ts` calls `validate`). Reason: 16 packages in runtime's closure are unbuilt here. It makes no deep-equality assertion on `results`. Declared to CI. - eslint, narrowed to the 7 touched `.ts` files with `--no-inline-config --format json`: 7 files, 0 errors, 0 warnings. Each file is inside the config's population (`--print-config` resolves it, and no "file ignored" warning). The narrowing excludes nothing: `eslint.config.mjs` enables no type-aware linting for any file (its own note near line 327), so this diff cannot move a verdict on an untouched file. ## Changeset `.changeset/20922-per-row-dropped-fields.md` grades `@objectstack/objectql: minor` and `@objectstack/metadata-protocol: minor` by the WHICH LEVEL rule. Each widens a published method's declared answer with a new optional key: `InsertManyRowOutcome` gains `droppedFields`, and so does each outcome of `insertManyData`'s return type. That is an additive widening of the public surface. Nothing is removed, renamed or refused. The spec edits are comments, so the spec carries no entry. ## Acceptance notes - The `ImportRowResultSchema` docblock's `droppedFields` bullet still reads "The engine has to report drops per row out of `validateData` and `insertMany`, and the REST import route has to copy them onto the row; until both do, no server sets the key". This stays true until the REST half copies the report, and the engine half is now done. The PR that lands #20701's REST item rewrites it. It is out of this card's two-sentence spec scope. - `packages/rest/src/import-runner.ts`'s `ImportProtocolLike.insertManyData` types its outcomes without `droppedFields`. Widening it is the REST half's first step, and it is not touched here. --- _Generated by [Claude Code](https://claude.ai/code/session_01Ujdtvqs7ree7WyQmEDwEnG)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent d1633f3 commit 657b6b7

8 files changed

Lines changed: 455 additions & 45 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@objectstack/objectql': minor
3+
'@objectstack/metadata-protocol': minor
4+
---
5+
6+
The dry run and the partial-success batch insert now say which row lost which field. `ObjectQL.validate` (and `validateData`, which relays it) answers `droppedFields` on each accepted row of `results`, and `ObjectQL.insertMany` (and `insertManyData`, which passes it through) answers `droppedFields` on each `ok` outcome: the caller-supplied fields the engine legally strips from that row, one `DroppedFieldsEvent` per reason, in the engine's own reason vocabulary (`computed` for a `formula` value, `readonly` for a static `readonly` or runtime-owned field). The key is absent when nothing was taken from the row.
7+
8+
- **Recorded at the strips, never inferred from the union.** Each strip records what it takes from each row as it runs. A `beforeInsert` hook that assigns a protected key on one row keeps it there, so that row is not named, while a sibling row that supplied the same key and lost it is.
9+
- **A row the write does not complete carries none.** A preview row the verdict refuses, and an `ok: false` outcome, carry no `droppedFields`: a drop means the write completed without the field.
10+
- **The dry run and the commit agree.** On `insert` mode the preview runs the same strips the write runs, so a row's preview drops and its outcome drops are the same list. One gap is unchanged: the preview runs no hooks, so a key a `beforeInsert` hook assigns is reported by the preview and kept by the write. An `update`-mode preview does not run the `readonlyWhen` or primary-key strips, which judge a prior record the preview does not read.
11+
- **Unchanged:** the `onFieldsDropped` listener on `insert`, `insertMany` and `validate` still reports the batch-level union, one event per reason, naming no row. So does `insertManyData`'s top-level `droppedFields`. `insert(object, rows[])` still returns the records, with no per-row slot. `strictReadonlyWrites` still refuses the whole batch before any outcome is built.
12+
13+
Graded `minor` in both packages: each widens a published method's declared answer with a new optional key (`InsertManyRowOutcome` gains `droppedFields`, and so does each outcome of `insertManyData`'s return type), which is an additive widening of the public surface. Nothing is removed, renamed or refused. The keys on the wire, `ValidateDataResponseSchema.results[].droppedFields` and `ImportRowResultSchema.droppedFields`, were already declared in `@objectstack/spec`. The REST import route does not copy the per-row report onto its row results yet.

‎packages/metadata-protocol/src/protocol.dropped-fields.bulk.test.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,11 @@ describe('insertManyData — BATCH-LEVEL droppedFields that names no row (#3455)
141141
// so the cases are REPLACED rather than amended. The engine double below
142142
// models the exemption, which the old one had no concept of; that is why the
143143
// old case stayed green through exactly the shape it was written for.
144+
//
145+
// [#20922] Row precision now comes from the ENGINE, on each `ok` outcome's
146+
// own `droppedFields` (pinned against the real engine in objectql's
147+
// `engine-per-row-dropped-fields.test.ts`). The double below attributes no
148+
// row, so these cases pin what THIS seam adds: nothing derived from the union.
144149

145150
/**
146151
* `hookStamps` names the rows whose `beforeInsert` hook re-assigns
@@ -250,6 +255,29 @@ describe('insertManyData — BATCH-LEVEL droppedFields that names no row (#3455)
250255
});
251256
expect(res).not.toHaveProperty('droppedFields');
252257
});
258+
259+
it('[#20922] an outcome the ENGINE attributed passes through as answered, beside the unchanged union', async () => {
260+
// The per-row channel is the engine's (`InsertManyRowOutcome.droppedFields`,
261+
// recorded at its strips). This seam relays it and derives nothing: the
262+
// cases above, whose engine attributes no row, still get none.
263+
const rowDrop = { object: 'approval_case', fields: ['approval_status'], reason: 'readonly' as const };
264+
const insertMany = vi.fn(async (object: string, rows: any[], options?: any) => {
265+
options?.onFieldsDropped?.({ object, fields: ['approval_status'], reason: 'readonly' });
266+
return rows.map((r, i) => (i === 1
267+
? { ok: true, record: { id: 'rec-2', title: r.title, approval_status: 'draft' }, droppedFields: [rowDrop] }
268+
: { ok: true, record: { id: 'rec-1', title: r.title, approval_status: 'draft' } }));
269+
});
270+
const p = makeProtocol(insertMany as any);
271+
272+
const res: any = await p.insertManyData({
273+
object: 'approval_case',
274+
records: [{ title: 'A' }, { title: 'B', approval_status: 'approved' }],
275+
});
276+
277+
expect(res.outcomes[0]).not.toHaveProperty('droppedFields');
278+
expect(res.outcomes[1].droppedFields).toEqual([rowDrop]);
279+
expect(res.droppedFields).toEqual([rowDrop]);
280+
});
253281
});
254282

255283
describe('batchData — per-row droppedFields + context threading (#3455)', () => {

‎packages/metadata-protocol/src/protocol.ts‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11836,6 +11836,12 @@ export class ObjectStackProtocolImplementation implements
1183611836
* Same object-existence gate as every other data entry point (#3770), so
1183711837
* an unknown object fails the same way here as it would on the real write
1183811838
* — a preview that 404s differently from its write is a mirror again.
11839+
*
11840+
* [#20922] The per-row drops ride the same verdict: `engine.validate`
11841+
* answers each accepted row's `droppedFields` from its own strips, so this
11842+
* relays them as it relays `errors` / `warnings`. No listener is passed
11843+
* here: the engine's `onFieldsDropped` events are the batch-level union,
11844+
* and this response has no batch-level slot to put them in.
1183911845
*/
1184011846
async validateData(request: { object: string, data: any, mode?: 'insert' | 'update', context?: any }) {
1184111847
this.assertObjectRegistered(request.object);
@@ -13609,7 +13615,7 @@ export class ObjectStackProtocolImplementation implements
1360913615
* channel (an `onFieldsDropped` signature that carries the row), never a
1361013616
* reconstruction at this call site.
1361113617
*/
13612-
async insertManyData(request: { object: string, records: any[], context?: any }): Promise<{ object: string; outcomes: Array<{ ok: boolean; record?: any; error?: unknown }>; droppedFields?: DroppedFieldsEvent[] }> {
13618+
async insertManyData(request: { object: string, records: any[], context?: any }): Promise<{ object: string; outcomes: Array<{ ok: boolean; record?: any; error?: unknown; droppedFields?: DroppedFieldsEvent[] }>; droppedFields?: DroppedFieldsEvent[] }> {
1361313619
this.assertObjectRegistered(request.object); // [#3770]
1361413620
const engineInsertMany = (this.engine as any)?.insertMany;
1361513621
if (typeof engineInsertMany !== 'function') {
@@ -13623,7 +13629,12 @@ export class ObjectStackProtocolImplementation implements
1362313629
const dropped: DroppedFieldsEvent[] = [];
1362413630
const opts: any = { onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); } };
1362513631
if (request.context !== undefined) opts.context = request.context;
13626-
const outcomes: Array<{ ok: boolean; record?: any; error?: unknown }> = await engineInsertMany.call(
13632+
// [#20922] Each `ok` outcome carries the ENGINE's per-row report
13633+
// (`outcomes[i].droppedFields`), recorded at its strips — a separate
13634+
// channel from the batch-level union below, which still names no row.
13635+
// Passed through as the engine answers it; ⛔ nothing here derives a
13636+
// row's drops from the union.
13637+
const outcomes: Array<{ ok: boolean; record?: any; error?: unknown; droppedFields?: DroppedFieldsEvent[] }> = await engineInsertMany.call(
1362713638
this.engine,
1362813639
request.object,
1362913640
request.records,

‎packages/objectql/src/engine-autonumber-runtime-owned.test.ts‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -305,15 +305,15 @@ describe('#5503 — autonumber is runtime-owned: bulk-create surfaces', () => {
305305
expect((res.droppedFields ?? []).flatMap((e: DroppedFieldsEvent) => e.fields)).toContain('account_number');
306306
});
307307

308-
it('insertManyData reports the union at BATCH level and names no row', async () => {
308+
it('insertManyData reports the union at BATCH level, and the row that lost the value on its own outcome', async () => {
309309
// The import runner prefers this partial-success surface, so it is the one
310-
// that has to stay honest — and honest here means naming no row: the
311-
// engine's event carries no row index, and the two facts that would let a
312-
// caller resolve it are both unavailable at the protocol seam. The row
313-
// records below are the second one: BOTH come back carrying
314-
// `account_number`, because the strip is followed by `applyAutonumbers`.
315-
// So "is the key still on the row?" answers the same for the row that was
316-
// stripped and the row that was not.
310+
// that has to stay honest. The batch-level union names no row: the
311+
// engine's event carries no row index. Nor can the row records resolve it:
312+
// BOTH come back carrying `account_number`, because the strip is followed
313+
// by `applyAutonumbers`, so "is the key still on the row?" answers the
314+
// same for the row that was stripped and the row that was not.
315+
// [#20922] Row precision comes from the ENGINE instead: the strip records
316+
// what it took per row, and the row's `ok` outcome carries it.
317317
const rig = await makeEngine();
318318
const res: any = await rig.protocol.insertManyData({
319319
object: 'an_account',
@@ -323,7 +323,10 @@ describe('#5503 — autonumber is runtime-owned: bulk-create surfaces', () => {
323323
],
324324
});
325325
expect(res.outcomes.map((o: any) => o.record.account_number)).toEqual(['ACC-0001', 'ACC-0002']);
326-
for (const o of res.outcomes) expect(o).not.toHaveProperty('droppedFields');
326+
expect(res.outcomes[0]).not.toHaveProperty('droppedFields');
327+
expect(res.outcomes[1].droppedFields).toEqual([
328+
{ object: 'an_account', fields: ['account_number'], reason: 'readonly' },
329+
]);
327330
expect((res.droppedFields ?? []).flatMap((e: DroppedFieldsEvent) => e.fields)).toEqual(['account_number']);
328331
});
329332
});

0 commit comments

Comments
 (0)