diff --git a/.changeset/25582-import-row-every-cell-error.md b/.changeset/25582-import-row-every-cell-error.md new file mode 100644 index 00000000000..630b5ada258 --- /dev/null +++ b/.changeset/25582-import-row-every-cell-error.md @@ -0,0 +1,15 @@ +--- +'@objectstack/core': patch +--- + +fix(core): an import row with several bad cells reports every one of them in `errors` (#25582) + +Clause-②: yes (widening: a failed import row whose cells did not coerce gains the optional `errors` list, `ImportRowResultSchema.errors`; every existing key keeps its value) +Metadata-protocol: none +Ruling-ref: #25582 ruling `6377844879` letter A, proposal 4; the spec seat's fill rule `6434667735` + +A row with several bad cells used to report only the first one, so an operator fixed one cell, imported again, and only then saw the next. The row now carries `errors`, one `{ field, code, message }` element per failed cell, in column order. The first-error keys `field`, `code` and `error` are unchanged and name element 0, so a reader of those keys sees the same answer as before. + +- It reaches every path that reports rows: the dry run and the commit, on the synchronous `POST /api/v1/data/:object/import` and on the async import job's results. +- It is filled only from the import's cell-coercion errors (`invalid_date`, `invalid_number`, `invalid_option` and the rest). A row that failed for another reason omits it: the dry run's validate verdict, a refused write, a duplicate or ambiguous match key. +- `ImportRowResult` (`@objectstack/core`) gains the optional `errors?: ValidateDataIssue[]`. diff --git a/packages/core/src/utils/import-runner-cell-errors.test.ts b/packages/core/src/utils/import-runner-cell-errors.test.ts new file mode 100644 index 00000000000..bb06761b24c --- /dev/null +++ b/packages/core/src/utils/import-runner-cell-errors.test.ts @@ -0,0 +1,131 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#25582, maintainer ruling 6377844879 letter A, proposal 4] A row with + * several bad cells reports every one of them in `errors`, one element per + * failed cell in column order, beside the first-error keys `field` / `code` / + * `error`, which keep naming element 0 (`ImportRowResultSchema.errors`). + * + * The list is filled from `coerceRow`'s cell errors only. A row the validate + * verdict refuses (the dry run) and a row whose write is refused (the commit, + * through `toFailedResult`) carry no `errors`: on those paths `error` is not + * element 0's message, and the spec's describe names cell-coercion errors as + * the list's only source. + * + * Driven against an `ImportProtocolLike` double; the route, the async job and + * a real engine are pinned in `packages/rest/src/import-row-cell-errors.test.ts`. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { runImport, type ImportProtocolLike } from './import-runner'; +import type { ExportFieldMeta } from './import-field-meta.js'; + +type ValidateArgs = Parameters>[0]; + +const OBJECT = 'supplier'; + +const metaMap = new Map([ + ['name', { name: 'name', type: 'text', label: 'Name' }], + ['opened', { name: 'opened', type: 'date', label: 'Opened' }], + ['qty', { name: 'qty', type: 'number', label: 'Qty' }], + ['country', { name: 'country', type: 'select', label: 'Country', options: [{ label: 'China', value: 'cn' }] }], +]); + +const baseOpts = { + objectName: OBJECT, + metaMap, + writeMode: 'insert' as const, + matchFields: [] as string[], + runAutomations: false, + trimWhitespace: true, + createMissingOptions: false, + skipBlankMatchKey: false, +}; + +/** Three bad cells, in this column order, around a good one. */ +const BAD_ROW = { opened: 'not a day', name: 'Acme', qty: 'many', country: 'DE' }; + +function protocol(over: Partial = {}): ImportProtocolLike { + return { + findData: vi.fn(async () => []), + createData: vi.fn(async (args: any) => ({ record: { id: 'r1', ...args.data } })), + updateData: vi.fn(), + validateData: vi.fn(async (args: ValidateArgs) => ({ + object: OBJECT, mode: args.mode ?? 'insert', valid: true, + results: [{ valid: true, errors: [], warnings: [] }], + posture: { valueShapeStrict: false, mediaValueShapeStrict: false }, + })), + ...over, + }; +} + +function expectEveryCellError(row: any) { + expect(row, JSON.stringify(row)).toMatchObject({ row: 1, ok: false, action: 'failed' }); + expect(row.errors.map((e: any) => [e.field, e.code])).toEqual([ + ['opened', 'invalid_date'], + ['qty', 'invalid_number'], + ['country', 'invalid_option'], + ]); + // Element 0 is the first-error keys, and every element is `{ field, code, message }`. + expect({ field: row.field, code: row.code, error: row.error }) + .toEqual({ field: row.errors[0].field, code: row.errors[0].code, error: row.errors[0].message }); + for (const e of row.errors) expect(Object.keys(e).sort()).toEqual(['code', 'field', 'message']); +} + +describe('[#25582] a row with several bad cells reports every cell error', () => { + it('the dry run: every cell error, in column order, element 0 = the first-error keys', async () => { + const p = protocol(); + const s = await runImport({ ...baseOpts, p, dryRun: true, rows: [BAD_ROW, { name: 'Good' }] }); + expect(s).toMatchObject({ ok: 1, errors: 1 }); + expectEveryCellError(s.results[0]); + // The good row has no list. + expect(s.results[1]).not.toHaveProperty('errors'); + }); + + it('the commit: the same report, and the row is never written', async () => { + const p = protocol(); + const s = await runImport({ ...baseOpts, p, dryRun: false, rows: [BAD_ROW] }); + expect(s).toMatchObject({ ok: 0, errors: 1, created: 0 }); + expectEveryCellError(s.results[0]); + expect(p.createData).not.toHaveBeenCalled(); + }); + + it('one bad cell: a one-element list naming it', async () => { + const s = await runImport({ ...baseOpts, p: protocol(), dryRun: true, rows: [{ name: 'Acme', qty: 'many' }] }); + expect(s.results[0]).toMatchObject({ field: 'qty', code: 'invalid_number' }); + expect(s.results[0].errors).toEqual([{ field: 'qty', code: 'invalid_number', message: s.results[0].error }]); + }); +}); + +describe('[#25582] a row that failed for another reason carries no `errors`', () => { + it('a validate-verdict refusal (dry run)', async () => { + const p = protocol({ + validateData: vi.fn(async (args: ValidateArgs) => ({ + object: OBJECT, mode: args.mode ?? 'insert', valid: false, + results: [{ + valid: false, + errors: [ + { field: 'name', code: 'required', message: 'Name is required' }, + { field: 'qty', code: 'min', message: 'Qty must be at least 1' }, + ], + warnings: [], + }], + posture: { valueShapeStrict: false, mediaValueShapeStrict: false }, + })), + }); + const s = await runImport({ ...baseOpts, p, dryRun: true, rows: [{ qty: '0' }] }); + expect(s.results[0]).toMatchObject({ row: 1, ok: false, action: 'failed', field: 'name', code: 'required' }); + expect(s.results[0]).not.toHaveProperty('errors'); + }); + + it('a refused write (commit, through the create door\'s verdict)', async () => { + const refusal = Object.assign(new Error('Name is required'), { + statusCode: 400, code: 'VALIDATION_FAILED', + fields: [{ field: 'name', code: 'required', message: 'Name is required' }], + }); + const p = protocol({ createData: vi.fn(async () => { throw refusal; }) }); + const s = await runImport({ ...baseOpts, p, dryRun: false, rows: [{ qty: '3' }] }); + expect(s.results[0]).toMatchObject({ row: 1, ok: false, action: 'failed' }); + expect(s.results[0]).not.toHaveProperty('errors'); + }); +}); diff --git a/packages/core/src/utils/import-runner.ts b/packages/core/src/utils/import-runner.ts index 07a4f8d8c6c..4983d0e39a2 100644 --- a/packages/core/src/utils/import-runner.ts +++ b/packages/core/src/utils/import-runner.ts @@ -84,6 +84,16 @@ export interface ImportRowResult { field?: string; error?: string; code?: string; + /** + * [#25582] Every cell error of a failed row, one element per failed cell in + * column order, each `{ field, code, message }` (`ImportRowResultSchema.errors`), + * so one import names every bad cell instead of one per re-import. Filled + * only from `coerceRow`'s cell errors, where `field` / `code` / `error` above + * are element 0. ⛔ Never on a validate-verdict row or a refused write: their + * `error` is not element 0's message, and the spec names cell errors as the + * list's only source. + */ + errors?: ValidateDataIssue[]; /** * Findings this deployment ADMITS rather than rejects — today, ADR-0104 * value shapes under a warn-first posture (#4633). The row is `ok`: the @@ -1220,7 +1230,10 @@ export function runImport(opts: RunImportOptions): Promise { if (errors.length > 0) { const first = errors[0]; errCount++; - results[i] = { row: rowNo, ok: false, action: 'failed', field: first.field, code: first.code, error: first.message }; + results[i] = { + row: rowNo, ok: false, action: 'failed', field: first.field, code: first.code, error: first.message, + errors: errors.map(({ field, code, message }) => ({ field, code, message })), + }; } else if (shared) { // [#26133] Another row of this import carries the same key. errCount++; diff --git a/packages/rest/src/import-row-cell-errors.test.ts b/packages/rest/src/import-row-cell-errors.test.ts new file mode 100644 index 00000000000..a414ff40848 --- /dev/null +++ b/packages/rest/src/import-row-cell-errors.test.ts @@ -0,0 +1,191 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#25582, maintainer ruling 6377844879 letter A, proposal 4] An import row + * with several bad cells reports every one of them, so one import tells the + * operator about every bad cell instead of one per re-import. Through the real + * routes over a real {@link ObjectQL} + {@link ObjectStackProtocolImplementation} + * + SQLite `:memory:`, with the platform's own `sys_import_job`: the + * synchronous `POST /data/:object/import` and the async job's + * `GET /data/import/jobs/:jobId/results`, each on the dry run and the commit. + * + * The row carries `errors` (`ImportRowResultSchema.errors`): one element per + * failed cell, in column order, each `{ field, code, message }`. The + * first-error keys `field` / `code` / `error` stay and name element 0. Measured + * at the base (`45de8c6cba`): the same row reported its first cell only, with + * no `errors` key, on all four paths. + * + * A row that failed for another reason omits the key: the dry run's validate + * verdict (`required` here) and the commit's refused write (the create door's + * `required` verdict) both name a field, but not as a cell error. + * + * The runner's own cases are `packages/core/src/utils/import-runner-cell-errors.test.ts`. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { SysImportJob } from '@objectstack/platform-objects/audit'; +import { SysMetadataAuditObject, SysMetadataCommitObject, SysMetadataHistoryObject, SysMetadataObject } from '@objectstack/metadata-core'; +import { RestServer } from './rest-server'; +import { installDefaultOrganization } from './default-organization.testkit.js'; + +const OBJECT = 'supplier_cells_25582'; + +const SUPPLIER = { + name: OBJECT, label: 'Supplier cells 25582', systemFields: false, + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true, label: 'ID' }, + name: { name: 'name', type: 'text' as const, label: 'Name', required: true }, + opened: { name: 'opened', type: 'date' as const, label: 'Opened' }, + qty: { name: 'qty', type: 'number' as const, label: 'Qty' }, + country: { + name: 'country', type: 'select' as const, label: 'Country', + options: [{ label: '中国', value: 'cn' }, { label: '美国', value: 'us' }], + }, + }, +}; + +function createMockServer() { + const noop = () => {}; + return { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} }; +} + +function makeRes() { + const res: any = { + write: () => true, end: () => {}, + header: () => res, + status: (code: number) => { res._status = code; return res; }, + json: (body: any) => { res._json = body; return res; }, + }; + return res; +} + +const liveEngines: ObjectQL[] = []; +afterEach(async () => { + while (liveEngines.length) { + try { await liveEngines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +async function boot() { + const engine = new ObjectQL(); + liveEngines.push(engine); + engine.registerDriver(new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }), true); + await engine.init(); + engine.registry.registerObject(SUPPLIER as any); + engine.registry.registerObject(SysImportJob as any); + await engine.syncSchemas(); + // A real `single` install's Default Organization: the job row's system + // writes derive it (ADR-0131 D9). + await installDefaultOrganization(engine); + // The route reads the object's field metadata through the protocol, which + // reads the stored-metadata family: without it every cell passes through + // uncoerced and the engine's validator answers instead. + for (const o of [SysMetadataObject, SysMetadataHistoryObject, SysMetadataAuditObject, SysMetadataCommitObject]) { + if (!engine.registry.getObject(o.name)) engine.registry.registerObject(o as any); + } + + const protocol = new ObjectStackProtocolImplementation(engine as any); + const rest = new RestServer(createMockServer() as any, protocol as any, { api: { requireAuth: false } } as any); + (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); + rest.registerRoutes(); + const call = async (method: string, path: string, body: unknown, params: Record = { object: OBJECT }): Promise<{ status: number; body: any }> => { + const route = rest.getRoutes().find((r: any) => r.method === method && r.path === path); + expect(route, `${method} ${path}`).toBeDefined(); + const res = makeRes(); + await route!.handler({ params, body, query: {}, headers: {} } as any, res); + return { status: res._status ?? 200, body: res._json }; + }; + + /** The synchronous route's per-row report. */ + const syncRows = async (rows: Array>, dryRun: boolean) => { + const r = await call('POST', '/api/v1/data/:object/import', { format: 'json', rows, dryRun }); + expect(r.status, JSON.stringify(r.body)).toBe(200); + return r.body.results as any[]; + }; + + /** The async job's per-row report, read off the results route once the job is terminal. */ + const jobRows = async (rows: Array>, dryRun: boolean) => { + const created = await call('POST', '/api/v1/data/:object/import/jobs', { format: 'json', rows, dryRun }); + expect(created.status, JSON.stringify(created.body)).toBe(201); + const jobId = String(created.body.jobId); + let progress: { status: number; body: any } | undefined; + for (let i = 0; i < 200; i++) { + progress = await call('GET', '/api/v1/data/import/jobs/:jobId', undefined, { jobId }); + if (['succeeded', 'failed', 'cancelled'].includes(progress.body?.status)) break; + await new Promise((r) => setTimeout(r, 5)); + } + expect(progress?.body?.status, JSON.stringify(progress?.body)).toBe('succeeded'); + const results = await call('GET', '/api/v1/data/import/jobs/:jobId/results', undefined, { jobId }); + return [...(results.body.results as any[])].sort((a, b) => a.row - b.row); + }; + + return { engine, syncRows, jobRows }; +} + +/** Three bad cells around good ones, in this column order. */ +const BAD_ROW = { id: 'b1', name: 'Acme', opened: '2011-13-45', qty: 'many', country: 'DE' }; +const GOOD_ROW = { id: 'g1', name: 'Good', opened: '2011-07-01', qty: '3', country: 'cn' }; + +function expectEveryCellError(row: any) { + expect(row, JSON.stringify(row)).toMatchObject({ row: 1, ok: false, action: 'failed' }); + expect(row.errors?.map((e: any) => [e.field, e.code]), JSON.stringify(row)).toEqual([ + ['opened', 'invalid_date'], + ['qty', 'invalid_number'], + ['country', 'invalid_option'], + ]); + for (const e of row.errors) { + expect(Object.keys(e).sort()).toEqual(['code', 'field', 'message']); + expect(typeof e.message).toBe('string'); + } + // The first-error keys are element 0, as they were before the list. + expect({ field: row.field, code: row.code, error: row.error }) + .toEqual({ field: row.errors[0].field, code: row.errors[0].code, error: row.errors[0].message }); +} + +describe('[#25582] every cell error of a row reaches the report', () => { + for (const dryRun of [true, false]) { + const leg = dryRun ? 'dry run' : 'commit'; + + it(`sync route, ${leg}`, async () => { + const b = await boot(); + const rows = await b.syncRows([BAD_ROW, GOOD_ROW], dryRun); + expectEveryCellError(rows[0]); + expect(rows[1]).toMatchObject({ row: 2, ok: true, action: 'created' }); + expect(rows[1]).not.toHaveProperty('errors'); + expect(await b.engine.findOne(OBJECT, { where: { id: 'b1' } })).toBeNull(); + }); + + it(`async job, ${leg}`, async () => { + const b = await boot(); + const rows = await b.jobRows([BAD_ROW, GOOD_ROW], dryRun); + expectEveryCellError(rows[0]); + expect(rows[1]).toMatchObject({ row: 2, ok: true, action: 'created' }); + expect(rows[1]).not.toHaveProperty('errors'); + }); + } +}); + +describe('[#25582] a row that failed for another reason carries no `errors`', () => { + /** Every cell coerces; `name` is required and missing. */ + const NO_NAME = { id: 'n1', qty: '3' }; + + it('the dry run\'s validate verdict (sync and async)', async () => { + const b = await boot(); + for (const rows of [await b.syncRows([NO_NAME], true), await b.jobRows([NO_NAME], true)]) { + expect(rows[0], JSON.stringify(rows[0])).toMatchObject({ row: 1, ok: false, action: 'failed', field: 'name', code: 'required' }); + expect(rows[0]).not.toHaveProperty('errors'); + } + }); + + it('the commit\'s refused write (sync and async)', async () => { + const b = await boot(); + for (const rows of [await b.syncRows([NO_NAME], false), await b.jobRows([NO_NAME], false)]) { + expect(rows[0], JSON.stringify(rows[0])).toMatchObject({ row: 1, ok: false, action: 'failed', field: 'name', code: 'required' }); + expect(rows[0]).not.toHaveProperty('errors'); + } + expect(await b.engine.findOne(OBJECT, { where: { id: 'n1' } })).toBeNull(); + }); +});