diff --git a/.changeset/27676-import-job-every-row-failed.md b/.changeset/27676-import-job-every-row-failed.md new file mode 100644 index 00000000000..afe33a20b45 --- /dev/null +++ b/.changeset/27676-import-job-every-row-failed.md @@ -0,0 +1,19 @@ +--- +'@objectstack/rest': patch +--- + +fix(rest)!: an async import job whose every row failed ends `failed`, not `succeeded` (#27676) + +Clause-②: no (narrowing) — the async import job's `succeeded` status is no longer emitted for a job that imported nothing; it now reads `failed`. No value, key or shape is added. +Metadata-protocol: none + + + +**BREAKING** a published status value changes meaning at one REST door (`@objectstack/rest`), per maintainer ruling B (record 6509036166). + +- **Before.** A job of `POST /data/:object/import/jobs` that was not cancelled and did not throw ended `succeeded`, even when every row failed and nothing was written. The only sign was `errors` equal to `total`. +- **Now.** A job that was not cancelled, had at least one row, and created, updated or skipped none of them ends `failed`. Its `error` says every row failed and points at the job's `results`, which still carry each row's reason. A dry run follows the same rule: a dry run whose every row is invalid ends `failed`. +- **Unchanged, on purpose.** A job whose rows partly failed still ends `succeeded`. So does a job whose every row was skipped (for example `writeMode: 'update'` with no match), and a job with no rows. A cancel still wins: a cancelled job ends `cancelled` whatever its rows did. Counts and `results` are written as before. +- **Who sees it.** A client that polls an import job and branches on `status === 'succeeded'` is now told `failed` for a job that imported nothing. Undo is unaffected: such a job wrote nothing, so it was never undoable. + +**The remedy.** Read `failed` as "this job imported nothing"; read `errors` and `results` for why. A client that wants to treat such a job as finished can treat `succeeded` and `failed` alike as terminal, as before. diff --git a/packages/qa/dogfood/test/import-reference-exposure.dogfood.test.ts b/packages/qa/dogfood/test/import-reference-exposure.dogfood.test.ts index fdcba0f9a48..2c9e41fd67a 100644 --- a/packages/qa/dogfood/test/import-reference-exposure.dogfood.test.ts +++ b/packages/qa/dogfood/test/import-reference-exposure.dogfood.test.ts @@ -83,7 +83,12 @@ describe('[#22739] an import matches a lookup cell only against a target whose d const { jobId } = (await res.json()) as { jobId: string }; for (let i = 0; i < 200; i++) { const body: any = await (await stack.apiAs(who, 'GET', `/data/import/jobs/${jobId}/results`)).json(); - if (body.status === 'succeeded') return body.results; + // A job whose every row failed ends `failed`, one with any row ok + // ends `succeeded` (maintainer ruling B, record 6509036166). + if (body.status === 'succeeded' || body.status === 'failed') { + expect(body.status, JSON.stringify(body)).toBe((body.results as Row[]).some((r) => r.ok) ? 'succeeded' : 'failed'); + return body.results; + } expect(['pending', 'running'], JSON.stringify(body)).toContain(body.status); await new Promise((r) => setTimeout(r, 25)); } diff --git a/packages/rest/src/data-action-routes.ts b/packages/rest/src/data-action-routes.ts index 1afb44b8231..2e6727ca4e8 100644 --- a/packages/rest/src/data-action-routes.ts +++ b/packages/rest/src/data-action-routes.ts @@ -179,6 +179,14 @@ function describeNonFieldMap(value: unknown): string { return `a ${typeof value}`; } +/** + * The `error` of an async import job that ended `failed` because every row + * failed (maintainer ruling B, record 6509036166): the per-row reasons are the + * job's `results`, which this sentence points the reader at. + */ +const IMPORT_JOB_EVERY_ROW_FAILED = + 'Every row of this import failed and nothing was imported; each row\'s reason is in the job\'s results.'; + /** [#27500] The formats `GET /data/:object/export` serves, csv first as the default. */ const EXPORT_FORMATS = ['csv', 'json', 'xlsx'] as const; type ExportFormatServed = typeof EXPORT_FORMATS[number]; @@ -762,7 +770,7 @@ export function registerDataActionEndpoints(this: DataActionRouteHost, basePath: // the durable row 'cancelled', and a late 'succeeded' here // would silently overwrite it (framework#2824). Counts stay // truthful either way — they reflect what was written. - let finalStatus = summary.cancelled ? 'cancelled' : 'succeeded'; + let finalStatus: 'cancelled' | 'succeeded' | 'failed' = summary.cancelled ? 'cancelled' : 'succeeded'; if (finalStatus === 'succeeded' && this.cancelledImportJobs.has(jobId)) finalStatus = 'cancelled'; if (finalStatus === 'succeeded') { try { @@ -770,8 +778,20 @@ export function registerDataActionEndpoints(this: DataActionRouteHost, basePath: if (String(row?.status ?? '') === 'cancelled') finalStatus = 'cancelled'; } catch { /* keep succeeded */ } } + // A job that was not cancelled, had rows, and created, + // updated or skipped none of them accomplished nothing: it + // ends `failed`, not `succeeded` (maintainer ruling B, + // record 6509036166). Decided after every cancel re-check + // above, so a cancel still wins. `ok` is not the test: a + // skipped row is not counted in it. A zero-row job, an + // all-skipped job and a partly-failed job stay `succeeded` + // on purpose; a dry run follows the same rule. + const everyRowFailed = finalStatus === 'succeeded' && summary.total > 0 + && summary.created + summary.updated + summary.skipped === 0; + if (everyRowFailed) finalStatus = 'failed'; await patch({ status: finalStatus, + ...(everyRowFailed ? { error: IMPORT_JOB_EVERY_ROW_FAILED } : {}), processed_rows: summary.processed, created_count: summary.created, updated_count: summary.updated, diff --git a/packages/rest/src/import-job-integration.test.ts b/packages/rest/src/import-job-integration.test.ts index bec74b227eb..ee193b1354e 100644 --- a/packages/rest/src/import-job-integration.test.ts +++ b/packages/rest/src/import-job-integration.test.ts @@ -508,3 +508,123 @@ describe('async import job — an unregistered object is refused before a job ex expect(rows[0]).toMatchObject({ id: res._json.jobId, object_name: 'task' }); }); }); + +/** + * The terminal status of a job that wrote nothing (maintainer ruling B, record + * 6509036166). A job that was not cancelled, had at least one row, and ended + * with no row created, updated or skipped ends `failed`, with `error` naming + * the outcome and pointing at `results`. Kept `succeeded` on purpose: a job + * whose rows partly failed (the partial pin in the first describe above), a job + * whose every row was skipped, and a job with no rows. Cancellation still wins. + * A dry run follows the same rule. + */ +describe('async import job — a job whose every row failed ends failed', () => { + let ctx: Awaited>; + beforeEach(async () => { + ctx = await boot(); + expect(ctx.create && ctx.progress && ctx.results && ctx.undo).toBeTruthy(); + }); + + const badRows = (prefix: string) => [ + { id: `${prefix}1`, title: 'one', score: 'not-a-number' }, + { id: `${prefix}2`, title: 'two', score: 'nope' }, + { id: `${prefix}3`, title: 'three', score: 'x' }, + ]; + + it('every row fails: the job ends failed, errors === total, and error is set', async () => { + const res = await callCreate(ctx.create, { format: 'json', rows: badRows('af') }); + expect(res._status).toBe(201); + const done = await waitForTerminal(ctx.progress, res._json.jobId); + expect(done).toMatchObject({ status: 'failed', total: 3, processed: 3, created: 0, updated: 0, skipped: 0, errors: 3 }); + expect(done.errors).toBe(done.total); + expect(typeof done.error).toBe('string'); + expect(done.error).toMatch(/results/); + // The per-row report is still delivered beside the status. + const results = await callJob(ctx.results, res._json.jobId); + expect(results._json.results).toHaveLength(3); + expect(results._json.results.every((r: any) => r.ok === false)).toBe(true); + // Nothing landed, and the undo reader answers as it did for this job before. + expect(await ctx.engine.find('task', { where: {} })).toHaveLength(0); + expect(done.undoable).toBe(false); + const u = await callJob(ctx.undo, res._json.jobId); + expect(u._status).toBe(422); + expect(u._json.code).toBe('NOT_UNDOABLE'); + // The history filter sees it under its real status. + const failedOnly = await callJob(ctx.list, '', { status: 'failed' }); + expect(failedOnly._json.jobs.map((j: any) => j.jobId)).toEqual([res._json.jobId]); + }); + + it('control: every row skipped keeps succeeded', async () => { + const res = await callCreate(ctx.create, { + format: 'json', writeMode: 'update', matchFields: ['id'], + rows: [{ id: 'sk1', title: 'a' }, { id: 'sk2', title: 'b' }], + }); + expect(res._status).toBe(201); + const done = await waitForTerminal(ctx.progress, res._json.jobId); + expect(done).toMatchObject({ status: 'succeeded', total: 2, created: 0, updated: 0, skipped: 2, errors: 0 }); + expect(done.error).toBeUndefined(); + }); + + it('control: a job with no rows keeps succeeded', async () => { + const res = await callCreate(ctx.create, { format: 'csv', csv: 'id,title\n' }); + expect(res._status).toBe(201); + expect(res._json.total).toBe(0); + const done = await waitForTerminal(ctx.progress, res._json.jobId); + expect(done).toMatchObject({ status: 'succeeded', total: 0, errors: 0 }); + expect(done.error).toBeUndefined(); + }); + + it('every row of a dry run fails: the job ends failed', async () => { + const res = await callCreate(ctx.create, { format: 'json', dryRun: true, rows: badRows('dr') }); + expect(res._status).toBe(201); + const done = await waitForTerminal(ctx.progress, res._json.jobId); + expect(done).toMatchObject({ status: 'failed', dryRun: true, total: 3, created: 0, updated: 0, skipped: 0, errors: 3 }); + expect(typeof done.error).toBe('string'); + }); + + it('control: a valid dry run keeps succeeded', async () => { + const res = await callCreate(ctx.create, { format: 'json', dryRun: true, rows: [{ id: 'dv1', title: 'ok' }] }); + const done = await waitForTerminal(ctx.progress, res._json.jobId); + expect(done).toMatchObject({ status: 'succeeded', dryRun: true, created: 1, errors: 0 }); + }); + + it('control: a cancel that lands after the last row still wins over an all-failed outcome', async () => { + // Every row collides with a seeded primary key, so each fails at the + // driver; the slowed writes let the cancel land while the job runs, and + // the 3-row job has no mid-loop checkpoint, so the loop finishes first. + const slowCtx = await boot((driver) => { + const bulkCreate = driver.bulkCreate.bind(driver); + driver.bulkCreate = async (o: string, rows2: any[], ...rest: any[]) => { + if (o === 'task') await new Promise((r) => setImmediate(r)); + return bulkCreate(o, rows2, ...rest); + }; + const create = driver.create.bind(driver); + driver.create = async (o: string, data: any, ...rest: any[]) => { + if (o === 'task') await new Promise((r) => setImmediate(r)); + return create(o, data, ...rest); + }; + }); + for (const id of ['cf1', 'cf2', 'cf3']) { + await slowCtx.protocol.createData({ object: 'task', data: { id, title: 'seeded' } }); + } + const created = await callCreate(slowCtx.create, { + format: 'json', + rows: [{ id: 'cf1', title: 'a' }, { id: 'cf2', title: 'b' }, { id: 'cf3', title: 'c' }], + }); + const jobId = created._json.jobId; + const c = await callJob(slowCtx.cancel, jobId); + expect(c._json).toMatchObject({ success: true }); + + await waitForTerminal(slowCtx.progress, jobId); + // The cancel route writes `cancelled` first; wait until the worker's own + // final patch has landed (it is the only write of `results`), then read the row. + for (let i = 0; i < 400; i++) { + const r = await callJob(slowCtx.results, jobId); + if ((r._json?.results ?? []).length === 3) break; + await new Promise((rr) => setTimeout(rr, 5)); + } + const after = await callJob(slowCtx.progress, jobId); + expect(after._json.status).toBe('cancelled'); + expect(after._json).toMatchObject({ processed: 3, created: 0, updated: 0, skipped: 0, errors: 3 }); + }); +}); diff --git a/packages/rest/src/import-row-cell-errors.test.ts b/packages/rest/src/import-row-cell-errors.test.ts index a414ff40848..5c0735ff20e 100644 --- a/packages/rest/src/import-row-cell-errors.test.ts +++ b/packages/rest/src/import-row-cell-errors.test.ts @@ -106,8 +106,12 @@ async function boot() { 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) => { + /** + * The async job's per-row report, read off the results route once the job is terminal. + * A job whose every row failed ends `failed` (maintainer ruling B, record 6509036166); + * one with any row created, updated or skipped ends `succeeded`. + */ + const jobRows = async (rows: Array>, dryRun: boolean, terminal: 'succeeded' | 'failed' = 'succeeded') => { 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); @@ -117,7 +121,7 @@ async function boot() { 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'); + expect(progress?.body?.status, JSON.stringify(progress?.body)).toBe(terminal); 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); }; @@ -174,7 +178,7 @@ describe('[#25582] a row that failed for another reason carries no `errors`', () 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)]) { + for (const rows of [await b.syncRows([NO_NAME], true), await b.jobRows([NO_NAME], true, 'failed')]) { expect(rows[0], JSON.stringify(rows[0])).toMatchObject({ row: 1, ok: false, action: 'failed', field: 'name', code: 'required' }); expect(rows[0]).not.toHaveProperty('errors'); } @@ -182,7 +186,7 @@ describe('[#25582] a row that failed for another reason carries no `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)]) { + for (const rows of [await b.syncRows([NO_NAME], false), await b.jobRows([NO_NAME], false, 'failed')]) { expect(rows[0], JSON.stringify(rows[0])).toMatchObject({ row: 1, ok: false, action: 'failed', field: 'name', code: 'required' }); expect(rows[0]).not.toHaveProperty('errors'); }