diff --git a/.changeset/metadata-protocol-seed-loader-driver-text.md b/.changeset/metadata-protocol-seed-loader-driver-text.md new file mode 100644 index 0000000000..6ef2f7d340 --- /dev/null +++ b/.changeset/metadata-protocol-seed-loader-driver-text.md @@ -0,0 +1,47 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +Stop putting raw driver text on the seed loader's `errors[].message` (#8442). + +#8333 closed the `error` **string** on `applySeedBodies`, but the same response +object carries a second channel the seed loader fills itself. Measured on +current `main`, a `sys_metadata` outage under a seed write still answered +`"Failed to write acct record #0 (name=acme): SQLITE_ERROR: no such table: +sys_metadata"` — and `seedApplied` rides a **200** publish response, so no HTTP +boundary's message withhold reaches it. + +`errors[].message` is free text, so #8441's catalog-membership rule (which +governs `code`, a closed union) does not apply: this is #8333's question — did +the producer AUTHOR this sentence for a caller? But #8333's **answer**, a +numeric 4xx `status`, is insufficient at this producer, because this sink +receives a population `protocol.ts`'s collectors never see: the data engine's +**validation layer**. An `@objectstack/objectql` `ValidationError` carries +`code: 'VALIDATION_FAILED'` and deliberately **no** `status` — deciding it means +400 is "the job of whichever boundary serves it", and for the seed channel this +loader is that boundary. So a caught sentence is quoted when the error declared +itself a client refusal by **either** shape: a 4xx `status`, or the +`VALIDATION_FAILED` shape that `@objectstack/types`' `validationFailureDetails` +already recognises (imported, not re-spelled). Everything else is replaced by a +stable line and goes to the log instead. + +That distinction is the whole fix rather than a nuance. On this producer the +structured keys do **not** carry the offending field: `field` is the literal +`'(write)'` and `targetField`/`attemptedValue` name the record's external key, +so "which key was rejected and why" exists only inside the validation sentence. +Applying the 4xx test alone would have blanked exactly the per-record authoring +feedback `errors[]` exists for — trading an authoring surface for a disclosure, +the trade #8441 refused. + +Nothing an author needs is lost. Every structured key is untouched (they are +built from the seed declaration and the record, never from the caught error), +the authored prefix is unchanged byte for byte, and a real malformed seed record +still reports which record and which key — pinned through the **real** ObjectQL +validator, not a hand-built error. The withheld driver line still reaches +`logger.error`, marked as withheld from the response, so the operator half of +the diagnostic is intact. + +Both payload producers are covered: the pass-1 record write and the pass-2 +deferred-reference back-fill. The loader's authored messages (unresolved +references, dropped references, dynamic-value failures) never quoted a driver +and are unchanged. diff --git a/packages/metadata-protocol/src/seed-loader-driver-text.test.ts b/packages/metadata-protocol/src/seed-loader-driver-text.test.ts new file mode 100644 index 0000000000..a684437fc9 --- /dev/null +++ b/packages/metadata-protocol/src/seed-loader-driver-text.test.ts @@ -0,0 +1,426 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8442 — the `errors[].message` limb of `seedApplied`, the third field in the + * family after #8333's `error` string and #8441's `code`. + * + * ## Which question this sink asks, and why it is neither sibling's answer + * + * | limb | question | predicate | + * |:--|:--|:--| + * | `error` (#8333) | did the producer AUTHOR this sentence for a caller? | 4xx `status` | + * | `code` (#8441) | is this value a MEMBER of the catalog? | `StandardErrorCode ∪ ERROR_CODE_LEDGER` | + * | `errors[].message` (this card) | did the producer AUTHOR this sentence? | 4xx `status` **OR** the `VALIDATION_FAILED` shape | + * + * A message is free text, so no catalog bounds it: #8441's membership rule does + * not apply and this is #8333's QUESTION. But #8333's ANSWER — a numeric 4xx + * `status` — is measurably insufficient at this producer, because this sink + * receives a population `protocol.ts`'s collectors never see: the data engine's + * VALIDATION layer. + * + * Measured on `main`, `@objectstack/objectql`'s `ValidationError` carries own + * properties `[stack, message, code, name, fields]` — `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". For the seed channel this loader IS + * that boundary, and `VALIDATION_FAILED_STATUS = 400` is the repo already + * stating that such a throw is a 4xx client refusal missing only the property. + * + * ## ⚠️ Why that distinction is the whole card + * + * On this producer the STRUCTURED keys do not carry the offending field. + * `buildWriteError` reports `field: '(write)'`, with `targetField` / + * `attemptedValue` naming the record's EXTERNAL key — i.e. WHICH ROW. "Which + * key was rejected and why" (`plan`, `max_length`) exists only inside the + * validation sentence. So answering with the 4xx test alone would blank exactly + * the per-record authoring feedback the issue names as the review bar, trading + * an authoring surface for a disclosure — the trade #8441 explicitly refused. + * Section 2 is that bound, and `seed-loader-authoring-feedback.test.ts` in + * `@objectstack/objectql` drives the same guarantee through the REAL validator. + * + * ## ⚠️ Anti-vacuity — the lesson #8441 recorded against #8333's pins + * + * #8333's fixture threw a bare `Error` with no `code`, so its scan could not + * see the `code` disclosure: the fixture never carried the field under test. + * The trap here is the mirror image — a fixture whose "validation failure" does + * not actually carry the shape the predicate reads would make section 2 pass + * for the wrong reason. Section 6 asserts the fixtures' own properties, and + * asserts the double is recognised by `validationFailureDetails` itself — the + * canonical recogniser, imported, not re-spelled. The double's shape was + * measured from the real class rather than guessed (objectql cannot be imported + * here: it depends on THIS package and would close a cycle). + * + * ## Reverse verification — both directions predicted BEFORE running + * + * **(a) `seed-loader.ts` reverted to pre-#8442.** Predicted **6 red / 4 green**: + * sections 1 (2), 3 (1), 4 (2) and 5 (1) go red because the driver text ships; + * section 2 (3) stays green because a declared refusal was quoted verbatim + * before this card too, and section 6 (1) is a pure fixture assertion + * independent of the loader. Section 5 is predicted RED deliberately — it + * asserts the PAYLOAD as well as the log, which is the miss both #8333 and + * #8441 recorded for their own operator-half case, so it is predicted rather + * than rediscovered. + * Measured: **6 red / 4 green** — every prediction held. + * + * **(b) The over-broad "just blank the tail" variant** — `WITHHELD_WRITE_REASON` + * unconditionally, the tempting wrong fix this card exists to refuse. Predicted + * **3 red / 7 green**: section 2's three cases go red (the authoring feedback + * vanishes) and everything else stays green, since nothing else asserts a + * quoted sentence. + * Measured: **3 red / 7 green** — every prediction held. + * + * Together the directions bound the fix on both sides: (a) proves it does + * something, (b) proves it does not do too much. + */ +import { describe, expect, it, vi } from 'vitest'; +// The canonical recogniser the fix reads. Imported in the TEST as well so +// section 6 can prove the fixture really satisfies it — a double the predicate +// would not recognise is how a guard passes for the wrong reason. +import { validationFailureDetails } from '@objectstack/types'; +import { SeedLoaderService } from './seed-loader.js'; +import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts'; + +// --------------------------------------------------------------------------- +// The physical conditions +// --------------------------------------------------------------------------- + +/** The sqlite phrasing of "`sys_metadata` is not there". */ +const DRIVER_TEXT = 'SQLITE_ERROR: no such table: sys_metadata'; + +/** Fragments that must never appear anywhere in a client-facing payload. */ +const LEAKED_FRAGMENTS = ['SQLITE_ERROR', 'no such table', 'sys_metadata']; + +/** The sentence a caller gets when nothing may be quoted. */ +const WITHHELD = 'the data engine rejected the write; the reason is in the server log'; + +/** A real better-sqlite3 failure: the dialect on a property, not only in the sentence. */ +const driverFault = () => + Object.assign(new Error(DRIVER_TEXT), { code: 'SQLITE_ERROR', errno: 1 }); + +/** + * Byte-faithful stand-in for `@objectstack/objectql`'s `ValidationError`. + * + * Measured from the real class rather than guessed — own properties + * `[stack, message, code, name, fields]`, `code = 'VALIDATION_FAILED'`, and NO + * `status` / `statusCode`. objectql cannot be imported here (it depends on this + * package), so section 6 pins the shape and the real-validator half of the + * guarantee lives in `@objectstack/objectql`'s + * `seed-loader-authoring-feedback.test.ts`. + */ +const validationFault = (message: string, fields: unknown[]) => () => + Object.assign(new Error(message), { + name: 'ValidationError', + code: 'VALIDATION_FAILED', + fields, + }); + +function expectNothingLeaked(payload: unknown): void { + const wire = JSON.stringify(payload) ?? ''; + expect(wire).not.toContain(DRIVER_TEXT); + for (const fragment of LEAKED_FRAGMENTS) expect(wire).not.toContain(fragment); +} + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +function createLogger() { + return { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; +} + +/** One business object, one text field beside the natural key. */ +function createMetadata(): IMetadataService { + const objects: Record = { + acct: { name: 'acct', fields: { name: { type: 'text' }, plan: { type: 'text' } } }, + }; + return { + getObject: vi.fn(async (name: string) => objects[name]), + listObjects: vi.fn(async () => Object.values(objects)), + register: vi.fn(async () => {}), + } as unknown as IMetadataService; +} + +/** An engine whose every write fails the way `thrown` says. */ +function failingEngine(thrown: () => unknown): IDataEngine { + return { + find: vi.fn(async () => []), + findOne: vi.fn(async () => null), + insert: vi.fn(async () => { throw thrown(); }), + update: vi.fn(async () => { throw thrown(); }), + delete: vi.fn(async () => ({ deleted: 1 })), + count: vi.fn(async () => 0), + aggregate: vi.fn(async () => []), + } as unknown as IDataEngine; +} + +const SEED = [{ + object: 'acct', + externalId: 'name', + mode: 'upsert', + env: ['prod', 'dev', 'test'], + records: [{ name: 'acme', plan: 'pro' }], +}]; + +const CONFIG = { + dryRun: false, haltOnError: false, multiPass: true, + defaultMode: 'upsert', batchSize: 1000, transaction: false, +}; + +/** Drive a pass-1 write failure and return the load result. */ +async function loadFailing(thrown: () => unknown, logger = createLogger()) { + const svc = new SeedLoaderService(failingEngine(thrown), createMetadata(), logger as never); + const result = await svc.load({ seeds: SEED, config: CONFIG } as never); + return { result, logger }; +} + +// ═══════════════════════════════════════════════════════════════════════════ +// 1. EVIDENCE — the driver's sentence never reaches `errors[].message` +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#8442] raw driver text is withheld from the seed `errors[].message`', () => { + it('the pass-1 record write answers the stable line, not `SQLITE_ERROR`', async () => { + const { result } = await loadFailing(driverFault); + + expect(result.success).toBe(false); + const error = result.errors[0]; + // The authored prefix is unchanged, byte for byte — two runtime pins + // read it, and it is the operation description an author needs. + expect(error.message).toContain('Failed to write acct record #0 (name=acme):'); + // ⛔ NOT blanked: the limb still answers, with the withheld-reason line. + expect(error.message).toContain(WITHHELD); + expectNothingLeaked(result); + }); + + it('the pass-2 deferred back-fill answers the stable line too', async () => { + // Two objects referencing each other force the multi-pass back-fill: + // `dept.head_id` is deferred to pass 2, whose `update` then fails. + const objects: Record = { + dept: { name: 'dept', fields: { name: { type: 'text' }, head_id: { type: 'lookup', reference: 'worker' } } }, + worker: { name: 'worker', fields: { name: { type: 'text' }, dept_id: { type: 'lookup', reference: 'dept' } } }, + }; + const metadata = { + getObject: vi.fn(async (n: string) => objects[n]), + listObjects: vi.fn(async () => Object.values(objects)), + register: vi.fn(async () => {}), + } as unknown as IMetadataService; + + const store: Record = {}; + let id = 0; + const engine = { + find: vi.fn(async (o: string, q?: any) => { + const rows = store[o] || []; + return q?.where + ? rows.filter((r) => Object.entries(q.where).every(([k, v]) => r[k] === v)) + : rows; + }), + findOne: vi.fn(async (o: string, q?: any) => { + const rows = await (engine.find as any)(o, q); + return rows[0] ?? null; + }), + insert: vi.fn(async (o: string, data: any) => { + if (!store[o]) store[o] = []; + const rec = { id: `gen-${++id}`, ...data }; + store[o].push(rec); + return rec; + }), + // The ONLY update in this load is pass-2's back-fill. + update: vi.fn(async () => { throw driverFault(); }), + delete: vi.fn(async () => ({ deleted: 1 })), + count: vi.fn(async () => 0), + aggregate: vi.fn(async () => []), + } as unknown as IDataEngine; + + const logger = createLogger(); + const result = await new SeedLoaderService(engine, metadata, logger as never).load({ + seeds: [ + { object: 'dept', externalId: 'name', mode: 'insert', env: ['prod', 'dev', 'test'], records: [{ name: 'Engineering', head_id: 'Alice' }] }, + { object: 'worker', externalId: 'name', mode: 'insert', env: ['prod', 'dev', 'test'], records: [{ name: 'Alice', dept_id: 'Engineering' }] }, + ], + config: CONFIG, + } as never); + + const backfill = result.errors.find((e) => e.message.includes('Failed to write deferred reference')); + expect(backfill, 'the pass-2 back-fill failure was not reported').toBeDefined(); + // The located structure survives whole — which field, which target. + expect(backfill!.message).toContain('dept.head_id'); + expect(backfill!.message).toContain('worker.name'); + expect(backfill!.message).toContain(WITHHELD); + expectNothingLeaked(result); + + // THE OPERATOR HALF OF *THIS* PASS — pinned here rather than left to + // section 5, which drives pass 1 only. Without this assertion a later + // edit could withhold the pass-2 log line too and every pin in the file + // would stay green while the deferred diagnostic disappeared: a payload + // pinned for both passes and an operator half pinned for one is exactly + // the "green pin narrower than its name" shape this family has already + // recorded once. + const logged = logger.error.mock.calls.map((c) => String(c[0])).join('\n'); + expect(logged).toContain(DRIVER_TEXT); + // …under the SAME marked vocabulary pass 1 uses, so an operator reading + // a pass-2 line learns the reporter never received this sentence. + expect(logged).toContain('Cause (withheld from the seed response)'); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// 2. [GUARD] The authoring surface — red under the "just blank the tail" fix +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#8442] [GUARD] a DECLARED refusal is quoted whole — the per-record authoring feedback', () => { + /** + * THE DISCRIMINATOR of this card. A validation failure declares itself by + * SHAPE and carries no `status`, so #8333's rule alone would withhold it — + * and with it the only statement of WHICH KEY was rejected, since the + * structured keys name only which ROW. Membership (#8441's rule) would not + * help either: `VALIDATION_FAILED` is not what bounds a free-text message. + */ + it('an objectql `ValidationError` (no `status`) keeps its per-field sentence', async () => { + const { result } = await loadFailing(validationFault( + 'Plan must be at most 4 characters.', + [{ field: 'plan', code: 'max_length', message: 'Plan must be at most 4 characters.' }], + )); + + const error = result.errors[0]; + // WHICH ROW — the structured half. + expect(error.recordIndex).toBe(0); + expect(error.attemptedValue).toBe('acme'); + // WHICH KEY AND WHY — the half that lives only in the sentence. + expect(error.message).toContain('Plan must be at most 4 characters.'); + }); + + it('a validation-RULE veto keeps its author-written sentence', async () => { + const { result } = await loadFailing(validationFault( + 'Cannot move a closed account back to draft.', + [{ field: '_record', code: 'rule_violation', message: 'Cannot move a closed account back to draft.' }], + )); + + expect(result.errors[0].message).toContain('Cannot move a closed account back to draft.'); + }); + + it('a declared 4xx refusal keeps its sentence (#8333’s rule, still in force)', async () => { + const { result } = await loadFailing(() => Object.assign( + new Error('[item_locked] Cannot overlay this item: the package is read-only.'), + { code: 'ITEM_LOCKED', status: 403 }, + )); + + expect(result.errors[0].message).toContain('[item_locked]'); + expect(result.errors[0].message).toContain('the package is read-only.'); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// 3. The structured per-record keys survive the withhold +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#8442] the located structure is untouched by the withhold', () => { + /** + * The issue's review bar: `errors[]` is per-record authoring feedback, so + * the fix must FILTER, not delete. Every key here is built from the seed + * declaration and the record — never from the caught error — so a withheld + * sentence costs none of them. + */ + it('every structured key is present and correct under a withheld driver fault', async () => { + const { result } = await loadFailing(driverFault); + + expect(result.errors[0]).toMatchObject({ + sourceObject: 'acct', + field: '(write)', + targetObject: 'acct', + targetField: 'name', + attemptedValue: 'acme', + recordIndex: 0, + }); + expectNothingLeaked(result); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// 4. The bounds — what a declaration is NOT +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#8442] an undeclared fault is withheld however it is dressed', () => { + /** + * The 5xx counterpart of #8441's discriminator, pointing the other way. A + * ledger-registered `code` and a declared `status` are both present, yet + * the status is 5xx — a SERVER fault, not a client refusal — so the + * sentence is withheld. That the code would survive #8441's rule on the + * sibling limb is exactly the point: the two limbs answer different + * questions about the same error. + */ + it('a declared 503 carrying a ledger code still has its sentence withheld', async () => { + const { result } = await loadFailing(() => Object.assign(new Error(DRIVER_TEXT), { + code: 'ERR_DATASOURCE_UNAVAILABLE', + status: 503, + })); + + expect(result.errors[0].message).toContain(WITHHELD); + expectNothingLeaked(result); + }); + + it('a bare `Error` declaring nothing is withheld', async () => { + const { result } = await loadFailing(() => new Error(DRIVER_TEXT)); + + expect(result.errors[0].message).toContain(WITHHELD); + expectNothingLeaked(result); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// 5. The operator half — withheld from the caller, intact in the log +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#8442] the withheld driver line still reaches the server log', () => { + /** + * Without this the fix would be indistinguishable from DELETING the + * diagnostic — the failure mode that makes a disclosure fix a net loss for + * whoever has to fix the database. Asserts the payload half too, which is + * why it is predicted RED in reverse direction (a). + */ + it('`logger.error` carries the driver sentence while the payload stays clean', async () => { + const { result, logger } = await loadFailing(driverFault); + + expectNothingLeaked(result); + const logged = logger.error.mock.calls.map((c) => String(c[0])).join('\n'); + expect(logged).toContain(DRIVER_TEXT); + // …and says plainly that the caller did not get it. + expect(logged).toContain('withheld from the seed response'); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// 6. ANTI-VACUITY — the fixtures really carry the fields under test +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#8442] the harness is non-vacuous', () => { + /** + * #8441's warning, applied to this card's own fixtures. #8333's pins could + * not see the `code` disclosure because its fake carried no `code`; the + * mirror trap here is a "validation failure" double that the predicate + * would not actually recognise, which would make section 2 green for the + * wrong reason. Both fixtures are asserted against the properties the fix + * reads — the validation double against the canonical recogniser itself. + */ + it('the validation double is recognised by `validationFailureDetails` and declares NO status', () => { + const err = validationFault('Plan must be at most 4 characters.', [ + { field: 'plan', code: 'max_length', message: 'Plan must be at most 4 characters.' }, + ])() as unknown as Record; + + // The exact own-property set measured from the real class. + expect(Object.getOwnPropertyNames(err).sort()) + .toEqual(['code', 'fields', 'message', 'name', 'stack']); + // It declares NO status — the whole reason #8333's rule is insufficient. + expect(err.status).toBeUndefined(); + expect((err as { statusCode?: unknown }).statusCode).toBeUndefined(); + // …and the canonical recogniser accepts it, so section 2 cannot be + // green because of a shape the production predicate would reject. + expect(validationFailureDetails(err)).toBeDefined(); + + // The driver fixture carries its dialect on a PROPERTY, not only in the + // sentence — and is NOT mistaken for a validation failure. + const driver = driverFault() as unknown as Record; + expect(driver.code).toBe('SQLITE_ERROR'); + expect(driver.errno).toBe(1); + expect(driver.status).toBeUndefined(); + expect(validationFailureDetails(driver)).toBeUndefined(); + }); +}); diff --git a/packages/metadata-protocol/src/seed-loader.ts b/packages/metadata-protocol/src/seed-loader.ts index 2f4a6620dc..ab907d251d 100644 --- a/packages/metadata-protocol/src/seed-loader.ts +++ b/packages/metadata-protocol/src/seed-loader.ts @@ -16,6 +16,11 @@ import type { import { SeedLoaderConfigSchema, isMultiValueField } from '@objectstack/spec/data'; import { resolveSeedRecord } from '@objectstack/formula'; import { bulkWrite, withTransientRetry, defaultIsTransientError, type BulkWriteRowResult } from '@objectstack/core'; +// [#8442] The repo's ONE recogniser for "this throw is a record-validation +// failure" — duck-typed on `code`/`name`, the same predicate `mapDataError` and +// both dispatcher error exits use. Imported rather than re-spelled so the seed +// channel and the HTTP boundaries cannot drift about what counts as one. +import { validationFailureDetails } from '@objectstack/types'; interface Logger { info(message: string, meta?: Record): void; @@ -27,6 +32,107 @@ interface Logger { /** Default field used for externalId matching on target objects */ const DEFAULT_EXTERNAL_ID_FIELD = 'name'; +/** + * [#8442] What a seed `errors[].message` says when the caught sentence may NOT + * be quoted. Names the operation and points at the log; quotes nothing. + */ +const WITHHELD_WRITE_REASON = + 'the data engine rejected the write; the reason is in the server log'; + +/** + * [#8442] Whether a caught error DECLARED itself a client-facing refusal, and + * may therefore have its sentence quoted back into `errors[].message`. + * + * ## Which question this sink asks + * + * `errors[].message` is free text — no catalog bounds it — so this is #8333's + * question ("did the producer AUTHOR this sentence for a caller?"), NOT #8441's + * membership question, which belongs to `code` because that field writes a + * closed union (ADR-0112 D4). Same question, one file over. + * + * ## …and why the ANSWER needs a second declaration shape + * + * `protocol.ts`'s {@link declaresClientRefusal} answers it with a numeric 4xx + * `status` alone, because every refusal reaching ITS collectors declares one + * (the repository's `ITEM_LOCKED` 403, `VERSION_NOT_FOUND` 404, …). This sink + * receives a population those collectors never see: the **data engine's + * validation layer**. Measured on `main`, an `@objectstack/objectql` + * `ValidationError` carries own properties `[stack, message, code, name, + * fields]` — `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". For the seed channel, + * THIS is that boundary. + * + * So the 4xx test alone would withhold exactly the sentence a seed author + * needs. That is not a hypothetical loss of nuance: on this producer the + * structured keys do NOT carry the offending field. `buildWriteError` reports + * `field: '(write)'` and `targetField`/`attemptedValue` = the record's + * EXTERNAL key ("which row"), so "which key was rejected and why" — `plan`, + * `max_length` — survives only inside the validation sentence. Blanking it + * would trade the authoring surface for the disclosure, the trade #8441 + * explicitly refused and this card's own warning names. + * + * `VALIDATION_FAILED_STATUS = 400` is the repo already stating that a + * validation failure IS a 4xx client refusal that merely omits the property, so + * admitting it here widens no boundary — it reads the declaration the error + * actually makes. A driver fault (`SQLITE_ERROR`, `errno`) matches neither + * shape and is withheld. + */ +function declaresSeedClientRefusal(err: unknown): boolean { + const status = (err as { status?: unknown } | null | undefined)?.status; + if (typeof status === 'number' && status >= 400 && status < 500) return true; + // A record-validation failure declares itself by SHAPE rather than status. + return validationFailureDetails(err) !== undefined; +} + +/** + * [#8442] The client-facing tail of a seed failure message — the caught + * sentence when {@link declaresSeedClientRefusal} admits it, otherwise + * `undefined` so the caller gets {@link WITHHELD_WRITE_REASON} instead. + */ +function quotableSeedFailureDetail(err: unknown): string | undefined { + if (!declaresSeedClientRefusal(err)) return undefined; + const declared = (err as { message?: unknown } | null | undefined)?.message; + return typeof declared === 'string' && declared.length > 0 ? declared : undefined; +} + +/** The caught sentence, whole — for the LOG, which never withholds. */ +function seedFailureCause(err: unknown): string { + const message = (err as { message?: unknown } | null | undefined)?.message; + return typeof message === 'string' && message.length > 0 ? message : String(err); +} + +/** + * [#8442] How a log line labels the cause it is about to print: marked when the + * payload half withheld that sentence, plain when the caller received it too. + * + * The marker is the half of the operator story that is not about the text. An + * operator reading `Cause: …` and an operator reading + * `Cause (withheld from the seed response): …` are looking at the same + * sentence and a DIFFERENT support situation — in the second the reporter never + * saw it, so "what did the response say?" has a different answer than the log + * suggests. Both passes share this one vocabulary so the two halves of the file + * cannot drift into answering that question differently. + */ +function seedCauseLabel(err: unknown): string { + return quotableSeedFailureDetail(err) === undefined + ? 'Cause (withheld from the seed response)' + : 'Cause'; +} + +/** + * [#8442] The operator half. The log line always carries the caught sentence, + * even when the payload may not quote it — without this, withholding the text + * would be indistinguishable from DELETING the diagnostic, which is what makes + * a disclosure fix a net loss for whoever has to fix the database. + */ +function seedFailureLogLine(payloadMessage: string, err: unknown): string { + const cause = seedFailureCause(err); + return payloadMessage.includes(cause) + ? `[SeedLoader] ${payloadMessage}` + : `[SeedLoader] ${payloadMessage} ${seedCauseLabel(err)}: ${cause}`; +} + /** The environments a seed dataset can be scoped to — mirrors `SeedSchema.env`. */ type SeedEnv = 'prod' | 'dev' | 'test'; @@ -521,9 +627,10 @@ export class SeedLoaderService implements ISeedLoaderService { // `error`, not `warn` (#4729 / #4632): this row is counted in // `allErrors` — the load already reports `success: false` — and the // consequence is that the record did NOT land. Count and log level - // must agree; the message names the row and the cause. + // must agree; the message names the row and the cause — [#8442] the + // cause EXPLICITLY, since the payload half may now withhold it. this.logger.error( - `[SeedLoader] ${error.message}`, + seedFailureLogLine(error.message, res.error), res.error instanceof Error ? res.error : undefined, { recordIndex }, ); @@ -887,8 +994,9 @@ export class SeedLoaderService implements ISeedLoaderService { // `error`, not `warn` (#4729 / #4632): counted in `allErrors`, and // the record did not land. `writeRecord` is in the durability // gate's vocabulary, so this catch cannot regress to `warn`. + // [#8442] carries the caught cause even when the payload withholds. this.logger.error( - `[SeedLoader] ${error.message}`, + seedFailureLogLine(error.message, err), err instanceof Error ? err : undefined, { recordIndex: i }, ); @@ -925,8 +1033,9 @@ export class SeedLoaderService implements ISeedLoaderService { // and the row's declared values did not land — an upsert that // fails here leaves the PREVIOUS row contents in place, which // looks like a seeded record and is not one. + // [#8442] carries the caught cause even when the payload withholds. this.logger.error( - `[SeedLoader] ${error.message}`, + seedFailureLogLine(error.message, err), err instanceof Error ? err : undefined, { recordIndex: i }, ); @@ -1164,7 +1273,10 @@ export class SeedLoaderService implements ISeedLoaderService { `${deferred.targetField} = '${this.formatAttempted(deferred.attemptedValue)}'. Nothing retries this — ` + `fix the write error below (a transient failure that outlasted the retry budget, or a validation rule ` + `vetoing the update) and re-run the seed to complete the link. ` + - `Cause: ${err?.message ?? String(err)}`, + // [#8442] Same cause vocabulary as the pass-1 write sites: the + // raw sentence always, MARKED when the payload half withheld it + // so an operator can see the reporter did not receive this line. + `${seedCauseLabel(err)}: ${seedFailureCause(err)}`, err instanceof Error ? err : undefined, { object: deferred.objectName, @@ -1173,8 +1285,18 @@ export class SeedLoaderService implements ISeedLoaderService { recordIndex: deferred.recordIndex, }, ); + // [#8442] The pass-2 counterpart of `buildWriteError`'s tail, and + // the same rule: the located structure (which object, which field, + // which target, which record) is authored here and untouched; only + // the caught sentence is gated. The `logger.error` above is this + // site's operator half — it runs on THIS path, in this same catch, + // and carries the raw cause under {@link seedCauseLabel}, the same + // marked vocabulary the pass-1 sites use. Both halves are pinned: + // an assertion on the pass-2 logger lives beside the payload one in + // `seed-loader-driver-text.test.ts`, so a later edit that withholds + // here too cannot stay green. this.recordDeferredError(deferred, allResults, allErrors, - `Failed to write deferred reference: ${deferred.objectName}.${deferred.field} = '${this.formatAttempted(deferred.attemptedValue)}' → ${deferred.targetObject}.${deferred.targetField}: ${err?.message ?? String(err)}`); + `Failed to write deferred reference: ${deferred.objectName}.${deferred.field} = '${this.formatAttempted(deferred.attemptedValue)}' → ${deferred.targetObject}.${deferred.targetField}: ${quotableSeedFailureDetail(err) ?? WITHHELD_WRITE_REASON}`); } } else { // THE TARGET RESOLVED BUT THE SOURCE ROW HAS NO ID (#5127). @@ -1649,7 +1771,21 @@ export class SeedLoaderService implements ISeedLoaderService { return String(a) === String(b); } - /** Builds the same `ReferenceResolutionError` shape a failed write has always reported. */ + /** + * Builds the same `ReferenceResolutionError` shape a failed write has always + * reported. + * + * [#8442] Every STRUCTURED key is unchanged — `sourceObject`, `field`, + * `targetObject`, `targetField`, `attemptedValue`, `recordIndex` are built + * from the seed declaration and the record, never from the caught error, so + * "which record, which key" is untouched by the withhold. The authored prefix + * is unchanged byte for byte too (two runtime pins read it). What changes is + * only what follows the colon: a DECLARED refusal — a 4xx, or the data + * engine's `VALIDATION_FAILED` shape, which is where "which field and why" + * lives — is quoted whole; a driver fault is replaced by + * {@link WITHHELD_WRITE_REASON} and goes to the log instead. See + * {@link declaresSeedClientRefusal}. + */ private buildWriteError( objectName: string, record: Record, @@ -1657,7 +1793,7 @@ export class SeedLoaderService implements ISeedLoaderService { recordIndex: number, err: unknown, ): ReferenceResolutionError { - const message = (err as { message?: unknown } | null)?.message ?? String(err); + const detail = quotableSeedFailureDetail(err) ?? WITHHELD_WRITE_REASON; const label = this.externalIdLabel(externalId); const keyValue = this.externalIdKey(record, externalId); return { @@ -1667,7 +1803,7 @@ export class SeedLoaderService implements ISeedLoaderService { targetField: label, attemptedValue: keyValue || null, recordIndex, - message: `Failed to write ${objectName} record #${recordIndex} (${label}=${keyValue}): ${message}`, + message: `Failed to write ${objectName} record #${recordIndex} (${label}=${keyValue}): ${detail}`, }; } diff --git a/packages/objectql/src/seed-loader-authoring-feedback.test.ts b/packages/objectql/src/seed-loader-authoring-feedback.test.ts new file mode 100644 index 0000000000..0fcb288ab2 --- /dev/null +++ b/packages/objectql/src/seed-loader-authoring-feedback.test.ts @@ -0,0 +1,183 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8442 — the MANDATORY positive control for the seed `errors[].message` + * withhold, driven through the REAL validator. + * + * The withhold itself is pinned in `@objectstack/metadata-protocol`'s + * `seed-loader-driver-text.test.ts`. That file must stand in a fixture double + * of `ValidationError`, because objectql depends on metadata-protocol and + * importing it there would close a cycle. This file is the other half, and it + * is the one that cannot be vacuous: a REAL `ObjectQL` engine, a REAL object + * declaring a REAL constraint, and a genuinely malformed seed record — no + * error is constructed by hand anywhere below. + * + * It is the analogue of #8333's broken-CEL approval flow, re-aimed at the field + * this card touches. What it guards is the issue's own review bar: `errors[]` + * is per-record authoring feedback, so the fix must FILTER, not delete. On this + * producer the structured keys name only WHICH ROW (`field` is the literal + * `'(write)'`, `targetField`/`attemptedValue` are the record's external key) — + * so "which key was rejected and why" survives only if the validator's own + * sentence is quoted. Blank the tail unconditionally and this test goes red, + * which is exactly the wrong fix it exists to refuse. + * + * It also closes the last vacuity gap in the pair: it proves a real + * `ValidationError` reaches the loader with its declaring shape intact, through + * the BUILT package, rather than only the hand-built double asserting so. + */ + +import { describe, it, expect } from 'vitest'; +import { SeedLoaderService } from '@objectstack/metadata-protocol'; +import { ObjectQL } from './engine.js'; + +/** + * `plan` carries a real `maxLength`, so a too-long value is rejected by + * `validateRecord` — the engine's own validator — rather than by anything this + * file arranges. + */ +const ACCT = { + name: 'sd_acct', + label: 'Account', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const, required: true }, + plan: { name: 'plan', label: 'Plan', type: 'text' as const, maxLength: 4 }, + }, +}; + +function makeMemoryDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + // `$and` / `$or` are conjoined WITH their sibling keys, the way a real + // driver ANDs them. The short-circuiting shape this stub used to carry + // (`if ($or) return $or.some(...)`) discarded every sibling equality key in + // the same object, so a query like + // `{ state:'draft', package_id, $or:[{organization_id:ORG},{organization_id:null}] }` + // was silently answered on the `$or` alone — a different query than the one + // written, with the suite still green. See #7620. + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k === '$and' && Array.isArray(v)) { + if (!v.every((w: any) => matchesWhere(row, w))) return false; + continue; + } + if (k === '$or' && Array.isArray(v)) { + if (!v.some((w: any) => matchesWhere(row, w))) return false; + continue; + } + if (k.startsWith('$')) continue; + const rowVal = row[k]; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + const a = rowVal === undefined ? null : rowVal; + const b = expected === undefined ? null : expected; + if (a !== b) return false; + } + return true; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { + return Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) throw new Error(`not found: ${object}/${id}`); + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return updated; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, stores }; +} + + +const CONFIG = { + dryRun: false, haltOnError: false, multiPass: true, + defaultMode: 'upsert', batchSize: 1000, transaction: false, +}; + +async function seedLoaderOverRealEngine() { + const engine = new ObjectQL(); + engine.registerDriver(makeMemoryDriver().driver, true); + await engine.init(); + engine.registry.registerObject(ACCT as never, 'com.objectstack.test.8442'); + const metadata = { getObject: async () => ACCT, listObjects: async () => [ACCT] }; + const logger = { info() {}, warn() {}, error() {}, debug() {} }; + return new SeedLoaderService(engine as never, metadata as never, logger as never); +} + +describe('[#8442] [GUARD] a malformed seed record still reports which record and which key', () => { + it('the real validator’s verdict survives the driver-text withhold', async () => { + const svc = await seedLoaderOverRealEngine(); + + const result = await svc.load({ + seeds: [{ + object: 'sd_acct', + externalId: 'name', + mode: 'upsert', + env: ['prod', 'dev', 'test'], + records: [ + { name: 'good_row', plan: 'pro' }, + // Genuinely malformed: 10 characters into a maxLength: 4 + // field. Nothing here throws on its own — the engine's + // validator rejects it. + { name: 'bad_row', plan: 'enterprise' }, + ], + }], + config: CONFIG, + } as never); + + // The load is reported as failed, and only the offending row failed. + expect(result.success).toBe(false); + expect(result.summary.totalInserted).toBe(1); + expect(result.summary.totalErrored).toBe(1); + + const error = result.errors[0]; + // WHICH RECORD — the structured half, untouched by this card. + expect(error.sourceObject).toBe('sd_acct'); + expect(error.recordIndex).toBe(1); + expect(error.attemptedValue).toBe('bad_row'); + // The authored prefix, unchanged. + expect(error.message).toContain('Failed to write sd_acct record #1 (name=bad_row):'); + // WHICH KEY AND WHY — the half that exists ONLY in the validator's + // sentence, and the half a blanket withhold would destroy. + expect(error.message).toMatch(/plan/i); + expect(error.message).toContain('4'); + // ⛔ The stable withheld line must NOT be what a real authoring + // rejection answers with — that substitution is the wrong fix. + expect(error.message).not.toContain('the reason is in the server log'); + }); +}); diff --git a/packages/runtime/src/seed-loader-driver-text-real-driver.integration.test.ts b/packages/runtime/src/seed-loader-driver-text-real-driver.integration.test.ts new file mode 100644 index 0000000000..6b2bd6cba1 --- /dev/null +++ b/packages/runtime/src/seed-loader-driver-text-real-driver.integration.test.ts @@ -0,0 +1,177 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8442 — the seed loader's `errors[].message` withhold, proven on a REAL + * driver rather than a fixture. + * + * ## The question this file was written to answer + * + * The fix quotes a caught sentence when the error declared itself a client + * refusal, and one of the two accepted declarations is the `VALIDATION_FAILED` + * shape (`@objectstack/types`' `validationFailureDetails`), because the seed + * channel's authoring feedback arrives that way and carries no `status`. + * Importing the canonical recogniser rather than re-spelling it is what stops + * the seed channel drifting from the HTTP boundaries — but it also means the + * limb inherits whatever that recogniser admits. + * + * So: can a DRIVER-originated constraint violation — unique / check / FK, the + * populations where the sentence's author and its shape could come apart — + * reach the loader's catch already wearing the validation shape? If it could, + * the new limb would be a disclosure path that no hand-built fixture in this + * repo would ever reveal, because every fixture constructs its error at the + * layer that authored the sentence. + * + * ## Measured answer: NO — and the disclosure withheld here is worse than the + * one the issue reported + * + * Driven end to end (real `SqlDriver` on better-sqlite3 on disk, real + * `ObjectQL`, real `SeedLoaderService`) with a duplicate on a `unique` column, + * the driver raises: + * + * ``` + * SqliteError name: 'SqliteError' code: 'SQLITE_CONSTRAINT_UNIQUE' + * own properties: [stack, message, code] status: undefined + * message: insert into `q2_acct` (…) select 'dup@example.com' as `email`, … - UNIQUE constraint failed: q2_acct.email + * ``` + * + * `validationFailureDetails` does NOT recognise it, so nothing converts it on + * the way up: between the driver and this catch there is only ObjectQL, whose + * own `ValidationError` throws are authored (`reference_not_found` from the + * message catalog, and a re-wrap of already-authored fields). The conversions + * that do exist — `mapDataError`, `resolveThrownHttpError` — live at HTTP + * boundaries that CONSUME this loader's output; they are downstream of this + * producer and can never wrap the engine's throw on its way into it. + * + * Note what that raw message contains: the full INSERT statement including the + * seeded VALUES. The issue's example leaked a table name; this path would leak + * row data and schema shape together. It is now withheld, and the assertion + * below is on the whole payload, not just the tail. + */ + +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 { SeedLoaderService } from '@objectstack/metadata-protocol'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { validationFailureDetails } from '@objectstack/types'; + +/** `email` declares UNIQUE, so the real driver — not a validator — rejects the duplicate. */ +const ACCT = { + name: 'dt_acct', + fields: { + name: { type: 'text' }, + email: { type: 'text', unique: true }, + }, +}; + +const SEED_CONFIG = { + dryRun: false, haltOnError: false, multiPass: true, + defaultMode: 'insert', batchSize: 1000, transaction: false, +} as any; + +function metadataFor(objects: any[]) { + const byName = new Map(objects.map((o) => [o.name, o])); + return { + getObject: async (name: string) => byName.get(name), + listObjects: async () => objects, + register: async () => {}, get: async (_t: string, n: string) => byName.get(n), + list: async () => [], unregister: async () => {}, exists: async () => false, listNames: async () => [], + } as any; +} + +describe('[#8442] a REAL driver constraint violation is withheld from the seed response', () => { + 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; } + }); + + it('a duplicate on a UNIQUE column leaks neither the SQL nor the seeded values', async () => { + dir = mkdtempSync(join(tmpdir(), 'os-8442-real-')); + const real = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'data.sqlite') }, + useNullAsDefault: true, + }); + await real.initObjects([ACCT]); + + // Capture the RAW driver error at the seam, then let it propagate + // untouched — so the test can assert on what the driver really threw + // rather than on an assumption about it. + let raw: any = null; + const driver = Object.create(real); + driver.create = async (o: string, d: any, opts: any) => { + try { return await real.create(o, d, opts); } catch (e) { raw ??= e; throw e; } + }; + driver.bulkCreate = async (o: string, rows: any[], opts: any) => { + try { return await real.bulkCreate(o, rows, opts); } catch (e) { raw ??= e; throw e; } + }; + + engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(ACCT as any, 'com.objectstack.test.8442'); + + const logger = { info() {}, warn() {}, error: (() => { + const calls: string[] = []; + const fn = (m: string) => { calls.push(String(m)); }; + (fn as any).calls = calls; + return fn; + })(), debug() {} }; + + const svc = new SeedLoaderService(engine as never, metadataFor([ACCT]), logger as never); + const result = await svc.load({ + seeds: [{ + object: 'dt_acct', + externalId: 'name', + mode: 'insert', + env: ['prod', 'dev', 'test'], + records: [ + { name: 'first', email: 'dup@example.com' }, + { name: 'second', email: 'dup@example.com' }, // duplicate on UNIQUE + ], + }], + config: SEED_CONFIG, + } as never); + + // ── The population really is what this file claims ────────────────────── + // Non-vacuity: if the driver stopped rejecting duplicates, or the error + // arrived wearing the validation shape, the assertions below would be + // measuring something else entirely. + expect(raw, 'the driver never rejected the duplicate').toBeTruthy(); + expect(String(raw.code)).toContain('SQLITE_CONSTRAINT'); + expect(String(raw.message)).toContain('UNIQUE constraint failed'); + // THE Q2 ANSWER: a driver-originated constraint violation does NOT arrive + // wearing the validation shape, so the quoting limb never opens for it. + expect(validationFailureDetails(raw)).toBeUndefined(); + expect(raw.status).toBeUndefined(); + + // ── The withhold ─────────────────────────────────────────────────────── + expect(result.success).toBe(false); + const failed = result.errors.find((e: any) => e.recordIndex === 1); + expect(failed, 'the duplicate row was not reported').toBeDefined(); + expect(failed!.message).toContain('the data engine rejected the write; the reason is in the server log'); + + // Nothing of the driver's sentence reaches the caller — asserted over the + // WHOLE payload, because this message carries the statement AND the values. + const wire = JSON.stringify(result); + expect(wire).not.toContain('UNIQUE constraint failed'); + expect(wire).not.toContain('SQLITE_CONSTRAINT'); + expect(wire).not.toContain('insert into'); + expect(wire).not.toContain('dup@example.com'); + + // ── …and the operator still gets it ──────────────────────────────────── + const logged = ((logger.error as any).calls as string[]).join('\n'); + expect(logged).toContain('UNIQUE constraint failed'); + expect(logged).toContain('Cause (withheld from the seed response)'); + + // The structured authoring feedback survives: which row failed. + expect(failed!.sourceObject).toBe('dt_acct'); + expect(failed!.attemptedValue).toBe('second'); + }); +});