diff --git a/.changeset/batch-row-driver-text-withhold.md b/.changeset/batch-row-driver-text-withhold.md new file mode 100644 index 0000000000..0e8ae3efcc --- /dev/null +++ b/.changeset/batch-row-driver-text-withhold.md @@ -0,0 +1,34 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +Withhold undeclared driver text from a bulk write's per-row `errors[].message` (#8502) + +`toRowApiError` interpolated whatever it caught into a batch row's message, so a +driver fault under `deleteManyData` answered +`{ code: "INTERNAL_ERROR", message: "SQLITE_ERROR: no such table: leave_request" }` +on response DATA riding a 200 — where no HTTP boundary's 5xx withhold can reach +it. Driven against a real driver the leaked text is worse than the tidy example: +a delete's raw message carries the failing statement's `WHERE` clause and its +bound record id, and a create's carries the whole `INSERT` with its values. The +causal row's message is also copied onto every `NOT_ATTEMPTED` / `ROLLED_BACK` +sibling, so one leaked sentence was repeated across the batch. + +A caught sentence now reaches a caller only when its producer declared a +client-facing refusal, asked through `resolveThrownHttpError` — the same +resolver the HTTP doors answer with — so all three declarations this sink +actually receives are honoured: a 4xx `status`, a 4xx `statusCode`, and the +`VALIDATION_FAILED` shape that carries neither. Per-field authoring feedback +from the engine's validator, `RECORD_NOT_FOUND`, `VALIDATION_FAILED` and +`plugin-approvals`' `RECORD_LOCKED` are unchanged, byte for byte. Anything +undeclared — a driver fault, or a hook that throws a bare `Error` — gets a +stable sentence naming the operation, and the original goes to the server log. + +The `code` limb is untouched (#8441 already gates it on catalog membership), and +no `httpStatus` is minted where the wire did not carry one. + +**Behaviour change for hook authors**: a hook that refuses by throwing an +undeclared `Error` no longer has its sentence echoed on the row. Declare the +refusal — a 4xx `status` or `statusCode`, or `validationFailure(message, fields)` +from `@objectstack/types` — and the message is served verbatim, as it now is on +the single-record path. diff --git a/packages/metadata-protocol/src/protocol.batch-atomic.test.ts b/packages/metadata-protocol/src/protocol.batch-atomic.test.ts index 8a2b381d10..4a1eed6b28 100644 --- a/packages/metadata-protocol/src/protocol.batch-atomic.test.ts +++ b/packages/metadata-protocol/src/protocol.batch-atomic.test.ts @@ -104,8 +104,15 @@ describe('batchData atomic — rollback is real and the response admits it (ADR- // "Attempted, undone" vs "never ran" is a CODE, not a message-prefix // regex (#4793) — the message keeps the human-readable cause. expect(res.results[0].errors?.[0]?.code).toBe('ROLLED_BACK'); - expect(res.results[0].errors?.[0]?.message).toContain('insert exploded'); // carries the cause - expect(res.results[1].errors?.[0]?.message).toBe('insert exploded'); // the causal row, verbatim + // [#8502] `insert exploded` is a BARE `Error` — it declares no client + // refusal, so its sentence is withheld and the row says the stable + // operation-named line instead. The claim under test is unchanged and + // is about PROPAGATION: whatever the causal row says, the rolled-back + // row quotes it, so a caller reading row 0 learns why row 1 stopped + // the batch. Asserted against the causal row's own message rather than + // a literal, so the two cannot drift apart. + expect(res.results[1].errors?.[0]?.message).toBe('The create of this record failed. The reason is in the server log.'); + expect(res.results[0].errors?.[0]?.message).toContain(res.results[1].errors?.[0]?.message); // carries the cause expect(res.results[2].errors?.[0]?.code).toBe('NOT_ATTEMPTED'); // Rows correlate to the request array by `index` (#4793). expect(res.results.map((r: any) => r.index)).toEqual([0, 1, 2]); @@ -162,7 +169,8 @@ describe('batchData atomic — rollback is real and the response admits it (ADR- expect(res.succeeded).toBe(0); expect(res.results[0].errors?.[0]?.code).toBe('ROLLED_BACK'); expect(res.results[0].id).toBe('rec-1'); // ids survive so a caller can reconcile - expect(res.results[1].errors?.[0]?.message).toBe('update exploded'); + // [#8502] withheld: a bare `Error` declares no client refusal. + expect(res.results[1].errors?.[0]?.message).toBe('The update of this record failed. The reason is in the server log.'); }); }); @@ -236,7 +244,13 @@ describe('batchData atomic — precedence and opt-in (ADR-0119 D4)', () => { expect(t.rollbacks).toHaveLength(1); expect(t.insert).not.toHaveBeenCalled(); // no blind fallback - expect(res.results[0].errors?.[0]?.message).toBe('update exploded'); // the real cause survives + // [#8502] The cause is withheld from the RESPONSE (bare `Error`), so + // "the real cause survives" is now carried by the two structural + // assertions above — the update was attempted and no fallback insert + // ran — plus the row naming the UPSERT it was doing. What must never + // appear is the fallback insert's duplicate-key text. + expect(res.results[0].errors?.[0]?.message).toBe('The upsert of this record failed. The reason is in the server log.'); + expect(res.results[0].errors?.[0]?.message).not.toContain('duplicate key'); }); }); diff --git a/packages/metadata-protocol/src/protocol.batch-row-conformance.test.ts b/packages/metadata-protocol/src/protocol.batch-row-conformance.test.ts index aa08345895..7a115b266a 100644 --- a/packages/metadata-protocol/src/protocol.batch-row-conformance.test.ts +++ b/packages/metadata-protocol/src/protocol.batch-row-conformance.test.ts @@ -129,7 +129,11 @@ describe('batchData rows conform to BatchOperationResultSchema (#4793)', () => { expectConformantResponse(res, 3); expect(res.results[0].data).toMatchObject({ title: 'A' }); // An unclassified engine throw is a 500 in row form. - expect(res.results[1].errors[0]).toMatchObject({ code: 'INTERNAL_ERROR', message: 'insert exploded' }); + // [#8502] `code` is unchanged; the message is the withheld stable line. + expect(res.results[1].errors[0]).toMatchObject({ + code: 'INTERNAL_ERROR', + message: 'The create of this record failed. The reason is in the server log.', + }); expect(res.results[1].data).toBeUndefined(); }); @@ -185,8 +189,9 @@ describe('batchData rows conform to BatchOperationResultSchema (#4793)', () => { expectConformantResponse(res, 3); expect(res.results[0].errors[0].code).toBe('ROLLED_BACK'); - expect(res.results[0].errors[0].message).toContain('insert exploded'); // human-readable cause - expect(res.results[1].errors[0].message).toBe('insert exploded'); // causal row keeps its own error + // [#8502] Same propagation claim, against the causal row's own text. + expect(res.results[1].errors[0].message).toBe('The create of this record failed. The reason is in the server log.'); + expect(res.results[0].errors[0].message).toContain(res.results[1].errors[0].message); // human-readable cause expect(res.results[2].errors[0].code).toBe('NOT_ATTEMPTED'); // No reverted write may carry a record payload. for (const row of res.results) expect(row.data).toBeUndefined(); @@ -210,7 +215,7 @@ describe('updateManyData rows conform to BatchOperationResultSchema (#4793)', () expectConformantResponse(res, 3); expect(res.results[0].data).toMatchObject({ id: 'a', title: 'a-new' }); - expect(res.results[1].errors[0].message).toBe('update exploded'); + expect(res.results[1].errors[0].message).toBe('The update of this record failed. The reason is in the server log.'); // [#8502] }); it('atomic rollback — all three row classes, as codes', async () => { diff --git a/packages/metadata-protocol/src/protocol.batch-row-driver-text.test.ts b/packages/metadata-protocol/src/protocol.batch-row-driver-text.test.ts new file mode 100644 index 0000000000..0206524668 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.batch-row-driver-text.test.ts @@ -0,0 +1,445 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8502] A batch row's `errors[].message` quotes a caught sentence only when + * its producer declared a client-facing refusal. + * + * ## The premise, reproduced + * + * `toRowApiError` interpolated `err.message` unconditionally, so one row of a + * `deleteManyData` against a failing store answered + * + * ```json + * { "code": "INTERNAL_ERROR", "message": "SQLITE_ERROR: no such table: leave_request" } + * ``` + * + * riding a **200** as response DATA. The `code` half was already right (#8441 + * gates it on catalog membership, which is why `SQLITE_ERROR` had become + * `INTERNAL_ERROR`); the message half had no gate at all. Fourth sink in the + * family, after #8136's overlay delete, #8333's `failed[].error` and #8442's + * seed `errors[].message`. + * + * ## Why this sink needed its OWN measurement, and got a different answer + * + * The question is #8136's — *did a producer author this sentence for a + * caller?* — but the population that answers it is this sink's own. All three + * catches sit under `engine.insert` / `update` / `delete`, so they receive + * every refusal the DATA path raises, and driven on the real stack (a real + * `ObjectQL` over a real `SqlDriver` on better-sqlite3, through all three + * loops) that population declares itself in **three** spellings: + * + * | producer | code | `status` | `statusCode` | validation shape | + * |---|---|---|---|---| + * | `rowRequiredIdError` | VALIDATION_FAILED | **400** | — | no | + * | `recordNotFoundError` | RECORD_NOT_FOUND | **404** | — | no | + * | objectql `ValidationError` | VALIDATION_FAILED | — | — | **yes** | + * | plugin-approvals' record lock | RECORD_LOCKED | — | **409** | no | + * | an app hook throwing a bare `Error` | — | — | — | no | + * | driver fault (`SqliteError`) | SQLITE_* | — | — | no | + * + * A `status`-only test (#8333's answer) admits the first two and blanks the + * next two. #8442's answer — a 4xx `status` OR the `VALIDATION_FAILED` shape — + * reaches the third and still blanks the fourth, because `plugin-approvals` + * binds a GLOBAL `beforeUpdate` hook whose `lockedError` spells its refusal + * `statusCode`. So neither sibling's answer transfers whole, and the rule here + * asks the one question that covers all three: **would the boundary serving + * this throw call it a client refusal?** — `resolveThrownHttpError`, + * IMPORTED from `@objectstack/types` rather than re-spelled, which is the same + * resolver `/api/v1/data` answers with. Reading only one status spelling is + * how that door answered 500 to a deliberate `409 RECORD_LOCKED` until #7525; + * a fourth local spelling here would re-create exactly that divergence one + * layer down. + * + * ## What the doubles below are, and why they are trustworthy + * + * Every error shape here was MEASURED from its real producer on the real stack + * (see the `MEASURED` note on each), never invented: metadata-protocol cannot + * import `@objectstack/objectql` or `@objectstack/driver-sql` — objectql + * depends on THIS package, so the import would close a cycle. Section 6 pins + * each double's exact own-property set and runs the PRODUCTION recogniser over + * it, so a double that drifted from the class it stands for turns this file + * red rather than passing for the wrong reason. The populations that CAN be + * driven for real are, in the two packages that may import both sides: + * `packages/objectql/src/batch-row-authoring-feedback.test.ts` (the real + * validator) and `packages/runtime/src/batch-row-driver-text-real-driver. + * integration.test.ts` (the real driver). + */ + +import { describe, it, expect, vi } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/metadata-core'; +import { resolveThrownHttpError, validationFailureDetails } from '@objectstack/types'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { + name: 'leave_request', + fields: { + title: { name: 'title', type: 'text' }, + progress: { name: 'progress', type: 'number' }, + }, +}; + +/** The stable sentences the withhold produces — one per operation verb. */ +const WITHHELD = { + create: 'The create of this record failed. The reason is in the server log.', + update: 'The update of this record failed. The reason is in the server log.', + upsert: 'The upsert of this record failed. The reason is in the server log.', + delete: 'The delete of this record failed. The reason is in the server log.', +}; + +// ─── The measured populations ──────────────────────────────────────────────── + +/** + * MEASURED — a real `SqliteError` from better-sqlite3 through the real + * `SqlDriver`, reaching `runDeleteManyLoop`'s catch: own properties + * `[stack, message, code]`, `code: 'SQLITE_ERROR'`, `status` undefined. + * + * The message is the card's own example. The real one is worse and is pinned + * in the runtime integration file: a delete's raw text carries the statement's + * WHERE clause **and its bound id**. + */ +function driverFault(message = 'SQLITE_ERROR: no such table: leave_request'): Error { + return Object.assign(new Error(message), { code: 'SQLITE_ERROR' }); +} + +/** + * MEASURED — `@objectstack/objectql`'s `ValidationError` as it arrives at + * these catches from `engine.insert` / `engine.update`: own properties + * `[stack, message, code, name, fields]`, `code: 'VALIDATION_FAILED'`, + * `name: 'ValidationError'`, and deliberately **no `status`** — per + * `@objectstack/types`' `validation-failure.ts`, deciding it means 400 is + * "the job of whichever boundary serves it". + */ +function engineValidationError(message: string, fields: unknown[]): Error { + const err = new Error(message) as Error & { code: string; fields: unknown[] }; + // Assignment ORDER matches the real class, whose `readonly code` field + // initialiser runs before the constructor body sets `name` then `fields`. + // Section 5 asserts the own-key list in order, and it caught this exact + // difference when the double was first written the other way round. + err.code = 'VALIDATION_FAILED'; + err.name = 'ValidationError'; + err.fields = fields; + return err; +} + +/** + * MEASURED — `plugin-approvals`' `lockedError`, raised inside the GLOBAL + * `beforeUpdate` hook it binds (`lifecycle-hooks.ts`), driven through the real + * hook against a real pending `sys_approval_request` row: own properties + * `[stack, message, code, statusCode]`, `code: 'RECORD_LOCKED'`, + * `statusCode: 409`, and `status` **undefined**. + * + * The spelling is the whole point of this entry — see the file header. + */ +function approvalsRecordLock(recordId: string): Error { + const err = new Error( + `RECORD_LOCKED: record '${recordId}' of 'leave_request' is locked while an approval is in progress`, + ) as Error & { code: string; statusCode: number }; + err.code = 'RECORD_LOCKED'; + err.statusCode = 409; + return err; +} + +/** An app-authored hook that refuses without declaring anything. */ +function undeclaredHookRefusal(): Error { + return new Error('Approval is required before this task may be updated. Ask your manager first.'); +} + +// ─── Harness ───────────────────────────────────────────────────────────────── + +/** + * The engine double every case drives. `throwOn` decides which row fails and + * with what, so one harness serves all three loops and the response rows are + * produced by the ACTUAL loops, builders and rollback classifier. + */ +function makeEngine(throwOn: (verb: string, id: unknown) => unknown | undefined) { + const rows = new Map([ + ['r1', { id: 'r1', title: 'one', progress: 0 }], + ['r2', { id: 'r2', title: 'two', progress: 0 }], + ['r3', { id: 'r3', title: 'three', progress: 0 }], + ]); + const handle = { id: 'trx-1' }; + + const engine: any = { + registry: { getObject: (n: string) => (n === 'leave_request' ? SCHEMA : undefined) }, + findOne: vi.fn(async (_o: string, opts?: any) => rows.get(opts?.where?.id) ?? null), + insert: vi.fn(async (_o: string, data: any) => { + const boom = throwOn('insert', data?.id); + if (boom) throw boom; + const rec = { id: data.id ?? `new-${rows.size + 1}`, ...data }; + rows.set(rec.id, rec); + return rec; + }), + update: vi.fn(async (_o: string, data: any, opts?: any) => { + const id = opts?.where?.id; + const boom = throwOn('update', id); + if (boom) throw boom; + const next = { ...rows.get(id), ...data }; + rows.set(id, next); + return next; + }), + delete: vi.fn(async (_o: string, opts?: any) => { + assertEngineDeleteDispatch(opts); + const id = opts?.where?.id; + const boom = throwOn('delete', id); + if (boom) throw boom; + if (!rows.has(id)) return false; + rows.delete(id); + return { deleted: 1 }; + }), + getDefaultDriverName: () => 'default', + getDriverByName: () => ({ beginTransaction: async () => handle }), + transaction: vi.fn(async (cb: (ctx: any) => Promise, base?: any) => { + const snapshot = new Map(rows); + try { + return await cb({ ...(base ?? {}), transaction: handle }); + } catch (err) { + rows.clear(); + for (const [k, v] of snapshot) rows.set(k, v); + throw err; + } + }), + }; + return { engine, protocol: new ObjectStackProtocolImplementation(engine) as any, rows }; +} + +/** Silence — and capture — the operator-half warn the withhold writes. */ +function captureWarn() { + return vi.spyOn(console, 'warn').mockImplementation(() => {}); +} + +describe('[#8502] section 1 — the premise: an undeclared driver sentence never reaches a row', () => { + it('deleteManyData answers the stable sentence, not the driver line', async () => { + const warn = captureWarn(); + const { protocol } = makeEngine((verb) => (verb === 'delete' ? driverFault() : undefined)); + + const res: any = await protocol.deleteManyData({ object: 'leave_request', ids: ['r1'] }); + + expect(res.results[0]).toEqual({ + id: 'r1', success: false, index: 0, + // `code` unchanged — #8441's limb, deliberately untouched here. + errors: [{ code: 'INTERNAL_ERROR', message: WITHHELD.delete }], + }); + // The whole payload, not just the tail: the leak the card measured was + // one field, but `reconcileStoppedBatch` copies a row's message onto + // its siblings, so a scan of the row alone can miss a live path. + expect(JSON.stringify(res)).not.toContain('SQLITE_ERROR'); + expect(JSON.stringify(res)).not.toContain('leave_request. '); + expect(JSON.stringify(res)).not.toContain('no such table'); + warn.mockRestore(); + }); + + it('the bulk batchData loop and updateManyData answer the same way, each naming ITS verb', async () => { + const warn = captureWarn(); + const a = makeEngine((verb) => (verb === 'insert' ? driverFault() : undefined)); + const createRes: any = await a.protocol.batchData({ + object: 'leave_request', + request: { operation: 'create', records: [{ data: { title: 'x' } }] }, + }); + expect(createRes.results[0].errors[0].message).toBe(WITHHELD.create); + + const b = makeEngine((verb) => (verb === 'update' ? driverFault() : undefined)); + const updateRes: any = await b.protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { progress: 1 } }], + }); + expect(updateRes.results[0].errors[0].message).toBe(WITHHELD.update); + + const c = makeEngine((verb) => (verb === 'update' ? driverFault() : undefined)); + const upsertRes: any = await c.protocol.batchData({ + object: 'leave_request', + request: { operation: 'upsert', records: [{ id: 'r1', data: { progress: 1 } }] }, + }); + expect(upsertRes.results[0].errors[0].message).toBe(WITHHELD.upsert); + + for (const res of [createRes, updateRes, upsertRes]) { + expect(JSON.stringify(res)).not.toContain('SQLITE_ERROR'); + } + warn.mockRestore(); + }); + + it('an EMPTY message no longer falls back to String(err) — the second leak path', async () => { + // The old fallback was `String(err)`, which renders `Error: SQLITE…`. + // Same shape as the second leak #8333 found at P13; closed by the same + // change, since the withhold branch now owns the no-message case too. + const warn = captureWarn(); + const bare = Object.assign(new Error(''), { code: 'SQLITE_ERROR' }); + Object.defineProperty(bare, 'toString', { value: () => 'Error: SQLITE_ERROR: no such table: leave_request' }); + const { protocol } = makeEngine((verb) => (verb === 'delete' ? bare : undefined)); + + const res: any = await protocol.deleteManyData({ object: 'leave_request', ids: ['r1'] }); + + expect(res.results[0].errors[0].message).toBe(WITHHELD.delete); + expect(JSON.stringify(res)).not.toContain('SQLITE_ERROR'); + warn.mockRestore(); + }); +}); + +describe('[#8502] section 2 — the authored population survives, in all THREE declarations', () => { + it('a 4xx `status` is quoted verbatim (rowRequiredIdError, the REAL producer)', async () => { + const { protocol } = makeEngine(() => undefined); + const res: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ data: { progress: 1 } }], + }); + expect(res.results[0].errors[0]).toEqual({ + code: 'VALIDATION_FAILED', message: 'Record id is required for update', httpStatus: 400, + }); + }); + + it('a 4xx `status` is quoted verbatim (recordNotFoundError, the REAL producer)', async () => { + const { protocol } = makeEngine(() => undefined); + const res: any = await protocol.deleteManyData({ object: 'leave_request', ids: ['ghost'] }); + expect(res.results[0].errors[0]).toEqual({ + code: 'RECORD_NOT_FOUND', message: 'Record ghost not found in leave_request', httpStatus: 404, + }); + }); + + it('the VALIDATION_FAILED shape is quoted even though it carries NO status', async () => { + // #8442's population, and the reason a status-only test would blank + // exactly the per-field authoring feedback `errors[]` exists for. + const boom = engineValidationError( + 'title must be ≤ 4 characters (got 15)', + [{ field: 'title', code: 'too_long', message: 'title must be ≤ 4 characters (got 15)' }], + ); + const { protocol } = makeEngine((verb) => (verb === 'update' ? boom : undefined)); + + const res: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { title: 'far too long' } }], + }); + + expect(res.results[0].errors[0].message).toBe('title must be ≤ 4 characters (got 15)'); + expect(res.results[0].errors[0].code).toBe('VALIDATION_FAILED'); + // NO `httpStatus`: the producer declared none, and #8502 does not mint + // one — that would be an ADDITION to the wire, a separate decision. + expect(res.results[0].errors[0].httpStatus).toBeUndefined(); + }); + + it('a 4xx `statusCode` is quoted — THIS sink’s own population, met by neither sibling', async () => { + const boom = approvalsRecordLock('r1'); + const { protocol } = makeEngine((verb) => (verb === 'update' ? boom : undefined)); + + const res: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { progress: 1 } }], + }); + + expect(res.results[0].errors[0].message).toBe( + "RECORD_LOCKED: record 'r1' of 'leave_request' is locked while an approval is in progress", + ); + expect(res.results[0].errors[0].code).toBe('RECORD_LOCKED'); + }); + + it('an UNDECLARED hook refusal is withheld — the measured cost of a positive list', async () => { + // Deliberate and pinned rather than regretted: at this sink an + // undeclared hook throw is indistinguishable from an undeclared driver + // throw, which is the whole hole. The remedy is at the producer and has + // three accepted spellings (the three cases above). + const warn = captureWarn(); + const { protocol } = makeEngine((verb) => (verb === 'update' ? undeclaredHookRefusal() : undefined)); + + const res: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { progress: 1 } }], + }); + + expect(res.results[0].errors[0].message).toBe(WITHHELD.update); + expect(JSON.stringify(res)).not.toContain('Approval is required'); + warn.mockRestore(); + }); +}); + +describe('[#8502] section 3 — the leak does not survive in the collateral rows either', () => { + it('NOT_ATTEMPTED quotes the causal row, so it quotes the WITHHELD sentence', async () => { + const warn = captureWarn(); + const { protocol } = makeEngine((verb, id) => (verb === 'delete' && id === 'r2' ? driverFault() : undefined)); + + const res: any = await protocol.deleteManyData({ object: 'leave_request', ids: ['r1', 'r2', 'r3'] }); + + expect(res.results[1].errors[0].message).toBe(WITHHELD.delete); + expect(res.results[2].errors[0].code).toBe('NOT_ATTEMPTED'); + expect(res.results[2].errors[0].message).toContain(WITHHELD.delete); + expect(JSON.stringify(res)).not.toContain('SQLITE_ERROR'); + warn.mockRestore(); + }); + + it('ROLLED_BACK quotes the causal row, so it quotes the WITHHELD sentence', async () => { + const warn = captureWarn(); + const { protocol } = makeEngine((verb, id) => (verb === 'update' && id === 'r2' ? driverFault() : undefined)); + + const res: any = await protocol.updateManyData({ + object: 'leave_request', + records: [{ id: 'r1', data: { progress: 1 } }, { id: 'r2', data: { progress: 2 } }], + options: { atomic: true }, + }); + + expect(res.results[0].errors[0].code).toBe('ROLLED_BACK'); + expect(res.results[0].errors[0].message).toContain(WITHHELD.update); + expect(JSON.stringify(res)).not.toContain('SQLITE_ERROR'); + warn.mockRestore(); + }); +}); + +describe('[#8502] section 4 — the operator half: withheld, not discarded', () => { + it('the withheld sentence reaches console.warn, marked as withheld, with the original error', async () => { + const warn = captureWarn(); + const boom = driverFault(); + const { protocol } = makeEngine((verb) => (verb === 'delete' ? boom : undefined)); + + await protocol.deleteManyData({ object: 'leave_request', ids: ['r1'] }); + + expect(warn).toHaveBeenCalledTimes(1); + const [line, cause] = warn.mock.calls[0]; + expect(line).toContain('#8502'); + expect(line).toContain('withheld from the response'); + // The ORIGINAL error object, not a re-spelling of it, so a log reader + // gets the stack too. + expect(cause).toBe(boom); + warn.mockRestore(); + }); + + it('a QUOTED row writes no withhold warning at all', async () => { + const warn = captureWarn(); + const { protocol } = makeEngine(() => undefined); + await protocol.deleteManyData({ object: 'leave_request', ids: ['ghost'] }); + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); +}); + +describe('[#8502] section 5 — anti-vacuity: the doubles are the shapes they claim to be', () => { + it('each double carries EXACTLY the own properties measured on its real producer', () => { + expect(Object.getOwnPropertyNames(driverFault())).toEqual(['stack', 'message', 'code']); + expect(Object.getOwnPropertyNames(engineValidationError('m', []))).toEqual( + ['stack', 'message', 'code', 'name', 'fields'], + ); + expect(Object.getOwnPropertyNames(approvalsRecordLock('r1'))).toEqual( + ['stack', 'message', 'code', 'statusCode'], + ); + expect(Object.getOwnPropertyNames(undeclaredHookRefusal())).toEqual(['stack', 'message']); + }); + + it('none of the withheld population declares a `status`, and the driver fault is not validation-shaped', () => { + for (const e of [driverFault(), engineValidationError('m', []), approvalsRecordLock('r1'), undeclaredHookRefusal()]) { + expect((e as any).status).toBeUndefined(); + } + expect(validationFailureDetails(driverFault())).toBeUndefined(); + expect(validationFailureDetails(approvalsRecordLock('r1'))).toBeUndefined(); + expect(validationFailureDetails(engineValidationError('m', []))).toBeDefined(); + }); + + it('the PRODUCTION recogniser classifies each double the way the real stack measured it', () => { + // Not the test's own predicate — the very function `toRowApiError` now + // calls, so a double that drifted from its class cannot pass here. + expect(resolveThrownHttpError(driverFault(), 500).status).toBe(500); + expect(resolveThrownHttpError(undeclaredHookRefusal(), 500).status).toBe(500); + expect(resolveThrownHttpError(engineValidationError('m', []), 500).status).toBe(400); + expect(resolveThrownHttpError(approvalsRecordLock('r1'), 500).status).toBe(409); + }); + + it('the withheld sentence interpolates NOTHING — it cannot carry a leak by construction', () => { + const secret = 'no such table: leave_request'; + for (const message of Object.values(WITHHELD)) { + expect(message).not.toContain(secret); + expect(message).not.toContain('${'); + } + // Four verbs, four distinct sentences: a row says which operation it + // was doing, which is the whole information budget a withheld row has. + expect(new Set(Object.values(WITHHELD)).size).toBe(4); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.delete-many.test.ts b/packages/metadata-protocol/src/protocol.delete-many.test.ts index 01beeb6a98..8f5b08c604 100644 --- a/packages/metadata-protocol/src/protocol.delete-many.test.ts +++ b/packages/metadata-protocol/src/protocol.delete-many.test.ts @@ -139,8 +139,9 @@ describe('deleteManyData — partial-failure semantics (#3897)', () => { expect(res.results[1]).toEqual({ id: 'b', success: false, index: 1, // A thrown error with no code of its own maps to the unclassified-500 - // row form; the message survives verbatim (#4793). - errors: [{ code: 'INTERNAL_ERROR', message: 'RLS: not visible' }], + // row form (#4793) — and since it declares no client refusal, #8502 + // withholds its sentence and the row names the operation instead. + errors: [{ code: 'INTERNAL_ERROR', message: 'The delete of this record failed. The reason is in the server log.' }], }); expect(res.results[2]).toMatchObject({ id: 'c', success: false, index: 2 }); expect(res.results[2].errors[0].code).toBe('NOT_ATTEMPTED'); diff --git a/packages/metadata-protocol/src/protocol.many-data-atomic.test.ts b/packages/metadata-protocol/src/protocol.many-data-atomic.test.ts index d68cd65645..faa8bf7ad4 100644 --- a/packages/metadata-protocol/src/protocol.many-data-atomic.test.ts +++ b/packages/metadata-protocol/src/protocol.many-data-atomic.test.ts @@ -203,8 +203,10 @@ describe('updateManyData atomic — the option is finally read (#4620)', () => { expect(res.failed).toBe(3); expect(res.results[0].id).toBe('a'); expect(res.results[0].errors?.[0]?.code).toBe('ROLLED_BACK'); - expect(res.results[0].errors?.[0]?.message).toContain('update exploded'); // carries the cause - expect(res.results[1].errors?.[0]?.message).toBe('update exploded'); // the causal row, verbatim + // [#8502] withheld sentence; the propagation claim is asserted against + // the causal row's own message so the two cannot drift. + expect(res.results[1].errors?.[0]?.message).toBe('The update of this record failed. The reason is in the server log.'); + expect(res.results[0].errors?.[0]?.message).toContain(res.results[1].errors?.[0]?.message); // carries the cause expect(res.results[2].errors?.[0]?.code).toBe('NOT_ATTEMPTED'); // Nothing persisted, so no reverted write may be reported as a success // or carry a record payload. @@ -311,7 +313,10 @@ describe('many-data non-atomic — unchanged (#4620 regression net)', () => { expect(res.results).toHaveLength(3); expect(res.succeeded + res.failed).toBe(res.total); expect(res.results[0]).toMatchObject({ id: 'a', success: true, index: 0 }); - expect(res.results[1]).toMatchObject({ id: 'b', success: false, index: 1, errors: [{ message: 'update exploded' }] }); + expect(res.results[1]).toMatchObject({ + id: 'b', success: false, index: 1, + errors: [{ message: 'The update of this record failed. The reason is in the server log.' }], // [#8502] + }); expect(res.results[2]).toMatchObject({ id: 'c', success: false, index: 2 }); expect(res.results[2].errors[0].code).toBe('NOT_ATTEMPTED'); expect(t.rows.get('c')).toEqual({ id: 'c', title: 'c-old' }); // stops without continueOnError diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 796536b2df..8b84b9734e 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -4,7 +4,7 @@ import type { DataProtocol, MetadataProtocol, PackageProtocol, } from '@objectstack/spec/api'; import { IDataEngine, engineCanRollBack, recordNotFoundError } from '@objectstack/core'; -import { readEnvWithDeprecation, resolveTenancyPosture } from '@objectstack/types'; +import { readEnvWithDeprecation, resolveTenancyPosture, resolveThrownHttpError } from '@objectstack/types'; // [#6285] ADR-0105 D1's authority on "does this deployment wall organizations?". // `resolveMultiOrgEnabled()` is DEMOTED and its own doc comment says answering // this question with it is a bug (cloud#1020, #5233) — so the posture, and only @@ -1480,19 +1480,42 @@ type BatchDataRowResult = BatchOperationResult; * conformance pin exists to catch loudly rather than ship). Otherwise it * derives from the HTTP status when the error carries one, falling back to * INTERNAL_ERROR — an unclassified engine throw is a 500 in row form. + * + * `message` is {@link clientFacingRowFailureText}'s decision (#8502): a caught + * sentence reaches a caller only when its producer declared a client-facing + * refusal. `fallback` is what the row says otherwise — it names the operation + * and quotes nothing. + * + * ⛔ `fallback` is REQUIRED, not defaulted. All three catches that build a row + * know their operation, and a default would let a fourth one be added that + * silently reports the wrong verb. */ -function toRowApiError(err: any): ApiError { +function toRowApiError(err: any, fallback: string): ApiError { const thrown = typeof err?.code === 'string' && ErrorCode.safeParse(err.code).success ? (err.code as ApiError['code']) : undefined; const status = typeof err?.status === 'number' ? err.status : undefined; return { code: thrown ?? (status !== undefined ? standardErrorCodeForHttpStatus(status) : 'INTERNAL_ERROR'), - message: typeof err?.message === 'string' && err.message.length > 0 ? err.message : String(err), + message: clientFacingRowFailureText(err, fallback), ...(status !== undefined ? { httpStatus: status } : {}), }; } +/** + * [#8502] The stable sentence a failed batch row says when nothing may be + * quoted — the operation, named, and no interpolation of any kind. + * + * One vocabulary for all three loops, so a caller reconciling a mixed batch is + * not reading three phrasings of one condition. Deliberately says where the + * reason IS (the server log), because the row itself is the caller's only + * channel here: a batch row rides response DATA on a 200, so there is no `cause` + * for a boundary to print and no 5xx withhold anywhere above it. + */ +function rowOperationFailureFallback(operation: string): string { + return `The ${operation} of this record failed. The reason is in the server log.`; +} + /** * [#7426] Carry a wrapped error's `code` onto its re-wrap — but only when that * code is part of the DECLARED vocabulary. @@ -1661,6 +1684,100 @@ export function clientFacingFailureText(err: unknown, fallback: string): string return fallback; } +/** + * [#8502] The per-row `errors[].message` a bulk data write puts on its + * response — the producer's own refusal when it declared one, otherwise a + * stable sentence naming the operation. + * + * The third sink in this family, after #8333's `failed[].error` and #8442's + * seed `errors[].message`, and the same defect: `toRowApiError` interpolated + * `err.message` unconditionally, so a driver fault under `deleteManyData` + * answered `{ code: 'INTERNAL_ERROR', message: 'SQLITE_ERROR: no such table: + * leave_request' }` on a per-row result riding a **200**. No HTTP boundary's + * 5xx withhold reaches it, because it is not the response's message — it is + * response DATA. It also multiplies: `reconcileStoppedBatch` and + * `buildRolledBackBatchResponse` interpolate the causal row's message into + * every `NOT_ATTEMPTED` / `ROLLED_BACK` sibling, so one leaked sentence is + * repeated across the batch. + * + * ## Why this asks the BOUNDARY RESOLVER rather than {@link declaresClientRefusal} + * + * The question is #8136's, unchanged: **did a producer author this sentence + * for a caller?** What differs is the population that answers it. This sink + * sits under `engine.insert` / `update` / `delete`, so it receives every + * refusal the data path can raise — and MEASURED on the real stack (a real + * `ObjectQL` over a real `SqlDriver`, driven through all three loops), that + * population declares itself in THREE spellings, only one of which + * `declaresClientRefusal` reads: + * + * | producer | code | `status` | `statusCode` | validation shape | + * |---|---|---|---|---| + * | {@link rowRequiredIdError} | VALIDATION_FAILED | **400** | — | no | + * | `recordNotFoundError` (`@objectstack/core`) | RECORD_NOT_FOUND | **404** | — | no | + * | objectql `ValidationError` | VALIDATION_FAILED | — | — | **yes** | + * | plugin-approvals' record lock | RECORD_LOCKED | — | **409** | no | + * | an app hook throwing a bare `Error` | — | — | — | no | + * | driver fault (`SqliteError`, …) | SQLITE_* | — | — | no | + * + * The first two are the only ones a `status`-only test admits. The third is + * the population #8442 met on the seed channel: an objectql `ValidationError` + * carries `code: 'VALIDATION_FAILED'` and deliberately NO `status`, because + * per `@objectstack/types`' `validation-failure.ts` deciding it means 400 is + * "the job of whichever boundary serves it". The fourth is this sink's own, + * and neither sibling card met it: `plugin-approvals` binds a GLOBAL + * `beforeUpdate` hook whose `lockedError` throws `statusCode` — the spelling + * `resolveThrownHttpError` reads "because both are produced in this repo", + * after reading only one of them made `/api/v1/data` answer 500 to a + * deliberate `409 RECORD_LOCKED` (#7525). + * + * So the test is not "does it carry `status` 4xx" but **"would the boundary + * that serves this throw call it a client refusal?"** — and that question + * already has exactly one implementation, {@link resolveThrownHttpError}, + * which folds all three declarations into one status. It is IMPORTED, never + * re-spelled: a fourth local spelling of "this is a 4xx" is how a batch row + * and the single-record `PATCH` of the same object would start disagreeing + * about the same error. ⛔ Do not replace this with a hand-written + * `status ?? statusCode ?? validation` chain for being cheaper to read. + * + * ## ⛔ Still a POSITIVE list — the undeclared case is withheld + * + * A hook that throws a bare `Error` has its sentence withheld, and that is the + * measured cost of the rule, not an oversight: at this sink an undeclared hook + * throw is indistinguishable from an undeclared driver throw, which is the + * whole hole. The remedy is at the producer and now has three accepted + * spellings — a 4xx `status`, a 4xx `statusCode`, or the `VALIDATION_FAILED` + * shape `validationFailure()` builds — so declaring is cheaper than the + * workaround, which is the direction that makes authored code hard to get + * wrong. The same reasoning #8333 applied when it fixed P9's undeclared + * `ZodError` at the producer rather than loosening the collector. + * + * ## The operator half + * + * The withheld sentence never leaves the server: it is logged here, at the one + * point where the decision is made, so no call site can withhold without + * logging. `console.warn` and not `error` deliberately — nothing claimed to be + * persisted was silently dropped (the row reports `success: false` and the + * counters reconcile), which is the AGENTS.md judgment question the durability + * levels turn on. + */ +function clientFacingRowFailureText(err: unknown, fallback: string): string { + // 500 as the fallback status: an error that declared nothing is a server + // fault in row form, exactly what `toRowApiError`'s own code limb assumes. + const { status } = resolveThrownHttpError(err, 500); + if (status >= 400 && status < 500) { + const declared = (err as { message?: unknown } | null | undefined)?.message; + if (typeof declared === 'string' && declared.length > 0) return declared; + } + console.warn( + '[Protocol] Withheld a caught error\'s text from a batch row (#8502): the producer declared no ' + + 'client-facing refusal (no 4xx status or statusCode, and not the VALIDATION_FAILED shape), so its ' + + 'sentence must not be quoted back on response data. The row says: ' + + `"${fallback}" — cause (withheld from the response):`, + err, + ); + return fallback; +} + /** * [#8441] The client-facing `code` for a failed row on a batch verb's * `failed[]` — the caught error's own code when the CATALOG declares it, and @@ -8705,7 +8822,9 @@ export class ObjectStackProtocolImplementation implements failed++; } } catch (err: any) { - results.push({ id: record.id, success: false, index, errors: [toRowApiError(err)] }); + // [#8502] `operation` is the request's own verb, so a mixed + // batch's withheld rows each name what THEY were doing. + results.push({ id: record.id, success: false, index, errors: [toRowApiError(err, rowOperationFailureFallback(operation))] }); failed++; if (atomic) { // Abort on the first failure; the caller rolls back. Atomic @@ -9072,7 +9191,7 @@ export class ObjectStackProtocolImplementation implements results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) }); succeeded++; } catch (err: any) { - results.push({ id: record.id, success: false, index, errors: [toRowApiError(err)] }); + results.push({ id: record.id, success: false, index, errors: [toRowApiError(err, rowOperationFailureFallback('update'))] }); failed++; if (atomic) { // Abort on the first failure; the caller rolls back. @@ -9245,7 +9364,7 @@ export class ObjectStackProtocolImplementation implements results.push({ id: String(id), success: true, index }); succeeded++; } catch (err: any) { - results.push({ id: String(id), success: false, index, errors: [toRowApiError(err)] }); + results.push({ id: String(id), success: false, index, errors: [toRowApiError(err, rowOperationFailureFallback('delete'))] }); failed++; // Same stop semantics as `batchData`: `atomic` aborts the rest on // the first failure (the caller rolls back), and without diff --git a/packages/metadata-protocol/src/protocol.upsert-existence.test.ts b/packages/metadata-protocol/src/protocol.upsert-existence.test.ts index 70be250204..b07c27759c 100644 --- a/packages/metadata-protocol/src/protocol.upsert-existence.test.ts +++ b/packages/metadata-protocol/src/protocol.upsert-existence.test.ts @@ -155,16 +155,25 @@ describe('[#5099] batchData upsert — the fork asks EXISTENCE, not visibility', it('a real update failure surfaces ITSELF — no fallback insert to mask it (non-atomic)', async () => { const t = makeRlsEngine(); - t.engine.update = vi.fn(async () => { throw new Error('update exploded'); }); + const explodingUpdate = vi.fn(async () => { throw new Error('update exploded'); }); + t.engine.update = explodingUpdate; const p = new ObjectStackProtocolImplementation(t.engine); const res: any = await upsert(p, [{ id: 'mine_1', data: { progress: 1 } }], { continueOnError: true }); // The old non-atomic fallback inserted here, buried 'update exploded' // under a duplicate-key error, and reported THAT to the caller. + // [#8502] The message no longer discriminates: `update exploded` is a + // bare `Error`, so its sentence is withheld, and the fallback insert's + // duplicate-key text would be withheld too. `.not.toContain('duplicate + // key')` alone would therefore pass for the WRONG reason — it would + // pass even if the fallback had run. So the claim is carried by the + // structural pair, which is strictly stronger than the string ever + // was: the update WAS attempted, and no insert followed it. + expect(explodingUpdate).toHaveBeenCalledTimes(1); expect(t.insert).not.toHaveBeenCalled(); expect(res.results[0].success).toBe(false); - expect(res.results[0].errors?.[0]?.message).toContain('update exploded'); + expect(res.results[0].errors?.[0]?.message).toBe('The upsert of this record failed. The reason is in the server log.'); expect(res.results[0].errors?.[0]?.message).not.toContain('duplicate key'); }); diff --git a/packages/objectql/src/batch-row-authoring-feedback.test.ts b/packages/objectql/src/batch-row-authoring-feedback.test.ts new file mode 100644 index 0000000000..0810e34667 --- /dev/null +++ b/packages/objectql/src/batch-row-authoring-feedback.test.ts @@ -0,0 +1,161 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8502] The POSITIVE CONTROL for the batch-row message withhold: a real + * `ObjectQL` validator, over a genuinely malformed record, through the real + * bulk-write loops. + * + * ## Why it lives here and not beside the fix + * + * `@objectstack/metadata-protocol` cannot import `@objectstack/objectql` — + * objectql depends on IT, so the import would close a cycle. The withhold's + * pins therefore stand in for the validation population with a double whose + * shape was measured (`protocol.batch-row-driver-text.test.ts`, section 5). + * This file is the half that needs no double at all: objectql may import + * metadata-protocol, so the sentence under test is produced by the actual + * `validateRecord` and read off the actual `BatchOperationResult`. + * + * ## What would be wrong without it + * + * The withhold is a POSITIVE list: a caught sentence reaches the caller only + * when its producer declared a client refusal. An objectql `ValidationError` + * declares `code: 'VALIDATION_FAILED'` and, deliberately, **no `status`** — + * so a rule that tested `status` alone would blank exactly the per-field + * authoring feedback these rows exist to carry, trading a real usability + * surface for no disclosure gain. That is the trade the card warned about in + * capitals, and this file is what makes the answer measured rather than + * argued: if the predicate ever narrows back to `status`, the assertions below + * go red with the author's own sentence replaced by the stable line. + * + * ⛔ Nothing here builds an error by hand. The record is malformed against a + * real schema and the engine rejects it on its own. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { ObjectQL } from './index.js'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; + +const LEAVE_REQUEST = { + name: 'bf_leave_request', + label: 'Leave Request', + fields: { + // `maxLength` is what the real record validator rejects against, and + // its message is the sentence an author has to see to fix the row. + reason: { type: 'text', maxLength: 8, label: 'Reason' }, + days: { type: 'number' }, + }, +}; + +/** In-memory driver: the validator, not the store, is what must reject here. */ +function makeDriver() { + const rows = new Map(); + return { + name: 'com.objectstack.driver.memory.bf', + version: '1.0.0', + async connect() { /* noop */ }, + async disconnect() { /* noop */ }, + async initObjects() { /* noop */ }, + async find(_o: string, options?: any) { + const id = options?.where?.id; + if (id !== undefined) { const r = rows.get(String(id)); return r ? [r] : []; } + return [...rows.values()]; + }, + async findOne(_o: string, options?: any) { return rows.get(String(options?.where?.id)) ?? null; }, + async count() { return rows.size; }, + async create(_o: string, data: any) { rows.set(String(data.id), data); return data; }, + async update(_o: string, data: any, options?: any) { + const id = String(options?.where?.id); + const next = { ...(rows.get(id) ?? { id }), ...data }; + rows.set(id, next); + return next; + }, + async delete(_o: string, options?: any) { + const id = String(options?.where?.id); + if (!rows.has(id)) return false; + rows.delete(id); + return { deleted: 1 }; + }, + rows, + } as any; +} + +describe('[#8502] a REAL validation refusal keeps its sentence on a batch row', () => { + let engine: ObjectQL; + let protocol: any; + let driver: any; + + beforeEach(async () => { + driver = makeDriver(); + engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(LEAVE_REQUEST as any, 'com.objectstack.test.8502'); + protocol = new ObjectStackProtocolImplementation(engine as any); + }); + + afterEach(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + it('batchData create — the author reads WHICH field and WHY, from the real validator', async () => { + const res: any = await protocol.batchData({ + object: 'bf_leave_request', + request: { + operation: 'create', + records: [ + { data: { reason: 'ok', days: 1 } }, + { data: { reason: 'far too long to be accepted', days: 2 } }, + ], + options: { continueOnError: true }, + }, + }); + + expect(res.results[0].success).toBe(true); + const bad = res.results[1]; + expect(bad.success).toBe(false); + expect(bad.errors[0].code).toBe('VALIDATION_FAILED'); + // The engine's own sentence, verbatim — it names the field and the + // bound that was exceeded. Asserted by CONTENT, not by equality with a + // literal copied from the validator, so a wording change in objectql + // does not make this file lie about what it proved. + expect(bad.errors[0].message).toContain('Reason'); + expect(bad.errors[0].message.toLowerCase()).toContain('8'); + // NON-VACUITY: the withhold's stable sentence must NOT be what came + // back — that is the exact failure this control exists to catch. + expect(bad.errors[0].message).not.toContain('The reason is in the server log'); + // And the clean row really landed, so the batch ran for real. + expect(driver.rows.size).toBe(1); + }); + + it('updateManyData — the same sentence survives on the other loop', async () => { + await engine.insert('bf_leave_request', { id: 'lr1', reason: 'ok', days: 1 }); + + const res: any = await protocol.updateManyData({ + object: 'bf_leave_request', + records: [{ id: 'lr1', data: { reason: 'far too long to be accepted' } }], + }); + + expect(res.results[0].success).toBe(false); + expect(res.results[0].errors[0].code).toBe('VALIDATION_FAILED'); + expect(res.results[0].errors[0].message).toContain('Reason'); + expect(res.results[0].errors[0].message).not.toContain('The reason is in the server log'); + // The stored row is untouched: the refusal happened before the write. + expect((await engine.findOne('bf_leave_request', { where: { id: 'lr1' } })).reason).toBe('ok'); + }); + + it('the refusal carries no `status`, so a status-only rule WOULD have blanked it', async () => { + // The measurement that makes the two cases above evidence rather than + // coincidence: it is not that the error happens to be quotable, it is + // that it is quotable ONLY because the rule reads more than `status`. + let caught: any = null; + try { + await engine.insert('bf_leave_request', { id: 'lr2', reason: 'far too long to be accepted' }); + } catch (e) { caught = e; } + + expect(caught).not.toBeNull(); + expect(caught.name).toBe('ValidationError'); + expect(caught.code).toBe('VALIDATION_FAILED'); + expect(caught.status).toBeUndefined(); + expect(caught.statusCode).toBeUndefined(); + }); +}); diff --git a/packages/objectql/src/protocol-batch-atomic.test.ts b/packages/objectql/src/protocol-batch-atomic.test.ts index bdb99d4c0a..8ceefb33af 100644 --- a/packages/objectql/src/protocol-batch-atomic.test.ts +++ b/packages/objectql/src/protocol-batch-atomic.test.ts @@ -178,7 +178,16 @@ describe('atomic batchData over the real engine (ADR-0119 D4 / ADR-0034)', () => expect(res.succeeded).toBe(0); expect(res.results.every((r: any) => r.success === false)).toBe(true); expect(res.results[0].errors?.[0]?.code).toBe('ROLLED_BACK'); - expect(res.results[2].errors?.[0]?.message).toContain('constraint violated'); + // [#8502] `constraint violated` is a bare driver `Error` — it declares + // no client refusal, so its sentence is withheld and the causal row + // says the stable operation-named line. The claim here is unchanged: + // row 2 is the CAUSAL row and rows 0/1 are its collateral, which is + // what `ROLLED_BACK` above and this row's own message together say. + expect(res.results[2].errors?.[0]?.message).toBe( + 'The create of this record failed. The reason is in the server log.', + ); + // …and the driver's own text is nowhere in the response. + expect(JSON.stringify(res)).not.toContain('constraint violated'); }); it('leaves rows written BEFORE the batch untouched when it rolls back', async () => { diff --git a/packages/runtime/src/batch-row-driver-text-real-driver.integration.test.ts b/packages/runtime/src/batch-row-driver-text-real-driver.integration.test.ts new file mode 100644 index 0000000000..10ace48e5a --- /dev/null +++ b/packages/runtime/src/batch-row-driver-text-real-driver.integration.test.ts @@ -0,0 +1,187 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8502] The batch-row withhold, proven on a REAL driver — and the + * measurement that says what this card was actually disclosing. + * + * ## The card understated the leak, exactly as #8442's did + * + * The issue quotes a tidy `SQLITE_ERROR: no such table: leave_request`, which + * reads like a schema-shape disclosure. Driven for real — `SqlDriver` on + * better-sqlite3 on disk, a real `ObjectQL`, the real + * `ObjectStackProtocolImplementation` — a delete's raw driver text is the + * whole failing statement: + * + * ``` + * SqliteError code: 'SQLITE_CONSTRAINT_FOREIGNKEY' status: undefined + * message: delete from `bd_parent` where `id` = 'p1' - FOREIGN KEY constraint failed + * ``` + * + * So the leaked text carries the **WHERE clause and its bound value** — which + * row, by id, in which table. On the insert side of the same batch surface it + * is worse still and matches what #8442 measured on the seed path: the full + * INSERT with every seeded VALUE. This is row data, not just schema shape, and + * it rides `errors[].message` on a **200** where no boundary withhold reaches + * it. + * + * ## What is asserted, and why over the whole payload + * + * `reconcileStoppedBatch` and `buildRolledBackBatchResponse` copy the causal + * row's message onto its `NOT_ATTEMPTED` / `ROLLED_BACK` siblings, so one + * leaked sentence is repeated across the batch. A scan of the failing row + * alone can therefore be green while the payload still carries the text — + * every assertion below is taken over `JSON.stringify(res)`. + * + * Non-vacuity is asserted alongside: that the driver really rejected the + * operation (the row is a failure, and the store is unchanged), and that the + * error really is NOT validation-shaped — otherwise the withhold could be + * green because the quoting limb was never reachable for this population. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { ObjectQL } from '@objectstack/objectql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { validationFailureDetails, resolveThrownHttpError } from '@objectstack/types'; + +const PARENT = { name: 'bd_parent', fields: { name: { type: 'text' } } }; +const CHILD = { + name: 'bd_child', + fields: { + name: { type: 'text' }, + parent: { type: 'lookup', reference_to: 'bd_parent' }, + }, +}; +const NOTE = { + name: 'bd_note', + fields: { + body: { type: 'text' }, + email: { type: 'text', unique: true }, + }, +}; + +describe('[#8502] a REAL driver fault is withheld from every batch row', () => { + let dir: string | null = null; + let engine: ObjectQL | null = null; + + afterEach(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + engine = null; + if (dir) { rmSync(dir, { recursive: true, force: true }); dir = null; } + }); + + async function rig() { + dir = mkdtempSync(join(tmpdir(), 'os-8502-real-')); + const real = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'data.sqlite') }, + useNullAsDefault: true, + }); + await real.initObjects([PARENT, CHILD, NOTE]); + + // Capture the RAW driver error at the seam and let it propagate + // untouched, so the test asserts on what the driver really threw + // rather than on an assumption about it. + let raw: any = null; + const driver: any = Object.create(real); + for (const m of ['create', 'update', 'delete', 'bulkCreate'] as const) { + driver[m] = async (...args: any[]) => { + try { return await (real as any)[m](...args); } catch (e) { raw ??= e; throw e; } + }; + } + + engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + for (const o of [PARENT, CHILD, NOTE]) { + engine.registry.registerObject(o as any, 'com.objectstack.test.8502'); + } + const protocol: any = new ObjectStackProtocolImplementation(engine as any); + return { protocol, real, rawOf: () => raw }; + } + + it('deleteManyData leaks neither the DELETE statement nor the bound id it names', async () => { + const { protocol, real, rawOf } = await rig(); + await engine!.insert('bd_parent', { id: 'p1', name: 'kept' }); + await engine!.insert('bd_child', { id: 'c1', name: 'dependent', parent: 'p1' }); + + const res: any = await protocol.deleteManyData({ object: 'bd_parent', ids: ['p1'] }); + + // The driver really refused, and it refused as a DRIVER: not + // validation-shaped, no declared status. Without this the withhold + // could be green because the quoting limb was never reachable. + const raw = rawOf(); + expect(raw).not.toBeNull(); + expect(raw.code).toBe('SQLITE_CONSTRAINT_FOREIGNKEY'); + expect(raw.status).toBeUndefined(); + expect(raw.statusCode).toBeUndefined(); + expect(validationFailureDetails(raw)).toBeUndefined(); + expect(resolveThrownHttpError(raw, 500).status).toBe(500); + // What the raw text actually contains — the measurement this file + // exists for. Asserted so a driver upgrade that stops interpolating + // the statement makes this claim fail loudly instead of silently. + expect(raw.message).toContain('delete from'); + expect(raw.message).toContain("'p1'"); + + // …and none of it reaches the caller. + const payload = JSON.stringify(res); + expect(res.results[0].success).toBe(false); + expect(res.results[0].errors[0].message).toBe('The delete of this record failed. The reason is in the server log.'); + expect(payload).not.toContain('delete from'); + expect(payload).not.toContain('FOREIGN KEY'); + expect(payload).not.toContain('SQLITE'); + expect(payload).not.toContain('bd_child'); + + // Non-vacuity on the other side: the row is still there, so the + // failure was real rather than a swallowed success. + expect(await engine!.findOne('bd_parent', { where: { id: 'p1' } })).toBeTruthy(); + // The response's own accounting agrees the row failed. + expect(res).toMatchObject({ success: false, total: 1, succeeded: 0, failed: 1 }); + void real; + }); + + it('batchData create leaks neither the INSERT statement nor the values it carries', async () => { + const { protocol, rawOf } = await rig(); + await engine!.insert('bd_note', { id: 'n1', body: 'first', email: 'dup@example.com' }); + + const res: any = await protocol.batchData({ + object: 'bd_note', + request: { + operation: 'create', + records: [{ data: { body: 'second', email: 'dup@example.com' } }], + }, + }); + + const raw = rawOf(); + expect(raw.code).toBe('SQLITE_CONSTRAINT_UNIQUE'); + expect(validationFailureDetails(raw)).toBeUndefined(); + // The raw text carries the statement AND the submitted values. + expect(raw.message).toContain('insert into'); + expect(raw.message).toContain('dup@example.com'); + + const payload = JSON.stringify(res); + expect(res.results[0].errors[0].message).toBe('The create of this record failed. The reason is in the server log.'); + expect(payload).not.toContain('insert into'); + expect(payload).not.toContain('dup@example.com'); + expect(payload).not.toContain('UNIQUE constraint failed'); + }); + + it('a stopped batch does not re-publish the withheld text through its NOT_ATTEMPTED rows', async () => { + const { protocol } = await rig(); + await engine!.insert('bd_parent', { id: 'p1', name: 'kept' }); + await engine!.insert('bd_parent', { id: 'p2', name: 'also kept' }); + await engine!.insert('bd_child', { id: 'c1', name: 'dependent', parent: 'p1' }); + + const res: any = await protocol.deleteManyData({ object: 'bd_parent', ids: ['p1', 'p2'] }); + + // Row 0 failed (FK), row 1 was never attempted and quotes row 0. + expect(res.results[0].success).toBe(false); + expect(res.results[1].errors[0].code).toBe('NOT_ATTEMPTED'); + expect(res.results[1].errors[0].message).toContain('The delete of this record failed'); + expect(JSON.stringify(res)).not.toContain('FOREIGN KEY'); + expect(JSON.stringify(res)).not.toContain('delete from'); + }); +});