Skip to content

Commit 4d9dd7b

Browse files
os-zhuangclaude
andauthored
fix(rest): validate required fields in import dry-run to match the real insert (#2839)
Add a required-field pre-check to the shared import runner (CREATE rows only), mirroring the engine's insert-time validation, so a dry run predicts the same NOT NULL / required failures the real insert produces instead of green-lighting a row that then fails. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent f70eb2c commit 4d9dd7b

5 files changed

Lines changed: 196 additions & 2 deletions

File tree

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
'@objectstack/rest': patch
3+
---
4+
5+
fix(rest): validate required fields in import dry-run to match the real insert
6+
7+
The bulk-import dry run (`POST /data/:object/import`, `dryRun:true`) only ran cell
8+
coercion and reported every coercible CREATE row as ok — so a row missing a required
9+
NOT-NULL field with no default was green-lit, then died on the real insert with
10+
`NOT NULL constraint failed`. The ImportWizard shows the dry-run result, so it
11+
promised imports that then failed.
12+
13+
Add a required-field pre-check to the shared import runner (CREATE rows only),
14+
mirroring the engine's insert-time validation (`objectql/record-validator.ts` +
15+
`applyFieldDefaults`): a required field is unsatisfied only when it has no value AND
16+
no default; `system`/`readonly`/`autonumber` and the engine-owned lifecycle columns
17+
are exempt. `ExportFieldMeta` gains `required`/`system`/`readonly`/`hasDefault`
18+
(populated by `buildFieldMetaMap`). Applied to both dry-run and real paths so they
19+
stay identical and a real insert returns a readable `<field> is required` instead of
20+
a raw driver error; skipped when `runAutomations` is set (a beforeInsert hook may
21+
populate the field).

packages/rest/src/export-format.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,19 @@ export interface ExportFieldMeta {
2222
reference?: string;
2323
/** Field on the referenced record to show as its label. */
2424
displayField?: string;
25+
// The following four drive the import path's required-field pre-check
26+
// (import-runner.ts). They mirror the engine's insert-time validation
27+
// (objectql record-validator.ts) so a dry run can predict a NOT NULL /
28+
// required failure instead of green-lighting a row the real insert rejects.
29+
// Unused by the export path (formatting only reads type/options/reference).
30+
/** Field is required — a value (or default) must exist on insert. */
31+
required?: boolean;
32+
/** Engine-owned column the client never supplies (never required of import). */
33+
system?: boolean;
34+
/** Read-only column the client never supplies (never required of import). */
35+
readonly?: boolean;
36+
/** Field declares a `defaultValue` the engine applies on insert (satisfies required). */
37+
hasDefault?: boolean;
2538
}
2639

2740
/** Field types whose stored value points at another record. */
@@ -78,6 +91,13 @@ export function buildFieldMetaMap(schema: unknown): Map<string, ExportFieldMeta>
7891
options: Array.isArray(f.options) ? f.options : undefined,
7992
reference: typeof f.reference === 'string' ? f.reference : undefined,
8093
displayField: typeof f.displayField === 'string' ? f.displayField : undefined,
94+
required: f.required === true,
95+
system: f.system === true,
96+
readonly: f.readonly === true,
97+
// Mirror the engine's `applyFieldDefaults` gate (`f.defaultValue == null`
98+
// ⇒ no default): any non-null default — literal, expression object, or the
99+
// `current_user` token — counts as satisfying a required field.
100+
hasDefault: f.defaultValue != null,
81101
});
82102
}
83103
return map;

packages/rest/src/import-coerce.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,53 @@ export async function coerceFieldValue(
323323
return { value: trim && typeof raw === 'string' ? raw.trim() : raw };
324324
}
325325

326+
// ── required-field pre-check ───────────────────────────────────────
327+
328+
/**
329+
* Engine-owned lifecycle columns the client never supplies. Mirrors
330+
* `record-validator.ts`'s `SKIP_FIELDS`, so the import's required pre-check and
331+
* the engine's insert-time required check agree on which fields the caller is
332+
* responsible for.
333+
*/
334+
const REQUIRED_CHECK_SKIP = new Set<string>([
335+
'id', 'created_at', 'created_by', 'updated_at', 'updated_by',
336+
]);
337+
338+
function isBlankValue(v: unknown): boolean {
339+
return v === undefined || v === null || (typeof v === 'string' && v.trim() === '');
340+
}
341+
342+
/**
343+
* The first required field a would-be CREATE leaves unsatisfied, or `null` when
344+
* every required field has either a mapped value or a schema default.
345+
*
346+
* This mirrors the engine's insert-time required check (objectql
347+
* `record-validator.ts` + `applyFieldDefaults`) so the import's dry run predicts
348+
* the SAME verdict the real insert produces: a required (⇒ NOT NULL) field with
349+
* no default and no value fails both. Without it, dry run only reports coercion
350+
* errors and green-lights a row that then dies on `NOT NULL constraint failed`.
351+
*
352+
* Matches the engine's exemptions exactly — `system`/`readonly` columns, the
353+
* runtime-generated `autonumber`, a field carrying a `defaultValue`, and the
354+
* engine-owned lifecycle columns are never required of the importer. Applies to
355+
* CREATE only; an UPDATE touches just the supplied fields, so callers gate on
356+
* "will create" before calling.
357+
*/
358+
export function firstMissingRequiredField(
359+
data: Record<string, unknown>,
360+
metaMap: Map<string, ExportFieldMeta>,
361+
): string | null {
362+
for (const meta of metaMap.values()) {
363+
if (!meta.required) continue;
364+
if (meta.system || meta.readonly) continue;
365+
if (meta.hasDefault) continue;
366+
if (meta.type === 'autonumber') continue;
367+
if (REQUIRED_CHECK_SKIP.has(meta.name)) continue;
368+
if (isBlankValue(data[meta.name])) return meta.name;
369+
}
370+
return null;
371+
}
372+
326373
/**
327374
* Coerce a whole raw row into a storage-ready record. Unknown columns (no
328375
* matching field metadata) pass through untouched so ad-hoc / schemaless

packages/rest/src/import-integration.test.ts

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,31 @@ const TASK = {
9797
},
9898
};
9999

100+
// Mirrors an AI-built object with required fields and NO default (framework
101+
// import dry-run fidelity): `member_name` (required text) and `status` (required
102+
// select, no default) must be present on create; `tier` is required but carries
103+
// a default, so the engine fills it and the importer must NOT demand it.
104+
const MEMBER = {
105+
name: 'member', label: 'Member', systemFields: false,
106+
fields: {
107+
id: { name: 'id', type: 'text' as const, primaryKey: true },
108+
member_name: { name: 'member_name', type: 'text' as const, label: 'Name', required: true },
109+
status: {
110+
name: 'status', type: 'select' as const, label: 'Status', required: true,
111+
options: [
112+
{ label: 'Active', value: 'active' },
113+
{ label: 'Frozen', value: 'frozen' },
114+
{ label: 'Lost Contact', value: 'lost_contact' },
115+
{ label: 'Archived', value: 'archived' },
116+
],
117+
},
118+
tier: {
119+
name: 'tier', type: 'select' as const, label: 'Tier', required: true, defaultValue: 'standard',
120+
options: [{ label: 'Standard', value: 'standard' }, { label: 'Gold', value: 'gold' }],
121+
},
122+
},
123+
};
124+
100125
function createMockServer() {
101126
const noop = () => {};
102127
return { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} };
@@ -119,6 +144,7 @@ async function boot() {
119144
await engine.init();
120145
engine.registry.registerObject(USER as any);
121146
engine.registry.registerObject(TASK as any);
147+
engine.registry.registerObject(MEMBER as any);
122148
await engine.insert('user', { id: 'u1', name: '张三', email: 'zhang@x.com' });
123149
await engine.insert('user', { id: 'u2', name: '李四', email: 'li@x.com' });
124150

@@ -310,6 +336,71 @@ describe('import route — real engine + protocol integration', () => {
310336
});
311337
});
312338

339+
// ---------------------------------------------------------------------------
340+
// Required-field dry-run fidelity — the dry run must predict the real insert's
341+
// NOT NULL / required failures instead of green-lighting a row the insert
342+
// rejects. Mirrors the live mx1n_member case (required `status` select, no
343+
// default): dryRun said ok, the real insert died on a NOT NULL constraint.
344+
// ---------------------------------------------------------------------------
345+
describe('import route — required-field dry-run fidelity', () => {
346+
let route: any;
347+
let engine: any;
348+
beforeEach(async () => { ({ route, engine } = await boot()); });
349+
350+
const imp = (body: any) => {
351+
const res = makeRes();
352+
return route.handler({ params: { object: 'member' }, body } as any, res).then(() => res);
353+
};
354+
355+
it('dry run fails a create row missing a required no-default field — and the real insert agrees', async () => {
356+
const rows = [
357+
{ id: 'm1', member_name: 'Alice' }, // status missing → must fail
358+
{ id: 'm2', member_name: 'Bob', status: 'active' }, // complete → ok
359+
];
360+
// Dry run: no longer reports success for the row the insert will reject.
361+
const dry = await imp({ format: 'json', dryRun: true, rows });
362+
expect(dry._json).toMatchObject({ dryRun: true, total: 2, ok: 1, errors: 1 });
363+
expect(dry._json.results.find((r: any) => !r.ok)).toMatchObject({ row: 1, field: 'status', code: 'required' });
364+
expect(await engine.findOne('member', { where: { id: 'm2' } })).toBeNull(); // dry run never writes
365+
366+
// Real insert: SAME verdict (parity), and a readable `status is required`
367+
// instead of a raw `NOT NULL constraint failed: member.status`.
368+
const real = await imp({ format: 'json', rows });
369+
expect(real._json).toMatchObject({ total: 2, ok: 1, errors: 1, created: 1 });
370+
expect(real._json.results.find((r: any) => !r.ok)).toMatchObject({ field: 'status', code: 'required', error: 'status is required' });
371+
expect((await engine.findOne('member', { where: { id: 'm2' } }))?.status).toBe('active');
372+
expect(await engine.findOne('member', { where: { id: 'm1' } })).toBeNull();
373+
});
374+
375+
it('a required field with a schema default is satisfied without being mapped', async () => {
376+
// `tier` is required but defaulted — the importer must not demand it; the
377+
// engine fills 'standard'. Only member_name + status are supplied.
378+
const res = await imp({ format: 'json', rows: [{ id: 'm3', member_name: 'Cara', status: 'frozen' }] });
379+
expect(res._json).toMatchObject({ ok: 1, errors: 0, created: 1 });
380+
expect(await engine.findOne('member', { where: { id: 'm3' } }))
381+
.toMatchObject({ member_name: 'Cara', status: 'frozen', tier: 'standard' });
382+
});
383+
384+
it('flags a required text field too (not just selects); a blank cell counts as missing', async () => {
385+
const res = await imp({ format: 'json', dryRun: true, rows: [
386+
{ id: 'm4', status: 'active' }, // member_name missing
387+
{ id: 'm5', member_name: ' ', status: 'active' }, // member_name blank
388+
] });
389+
expect(res._json).toMatchObject({ ok: 0, errors: 2 });
390+
for (const r of res._json.results) expect(r).toMatchObject({ field: 'member_name', code: 'required' });
391+
});
392+
393+
it('required check does not apply to update-mode rows (only the touched fields matter)', async () => {
394+
await engine.insert('member', { id: 'm6', member_name: 'Dan', status: 'active', tier: 'gold' });
395+
// writeMode:update on an existing match, touching only member_name — status
396+
// is not supplied but the record already has it, so this must NOT fail.
397+
const res = await imp({ format: 'json', writeMode: 'update', matchFields: ['id'],
398+
rows: [{ id: 'm6', member_name: 'Daniel' }] });
399+
expect(res._json).toMatchObject({ ok: 1, errors: 0, updated: 1 });
400+
expect((await engine.findOne('member', { where: { id: 'm6' } }))?.member_name).toBe('Daniel');
401+
});
402+
});
403+
313404
// ---------------------------------------------------------------------------
314405
// Named mapping artifacts (#2611) — `mappingName` resolves a registered
315406
// `mapping` item and applies its fieldMapping pipeline before coercion.

packages/rest/src/import-runner.ts

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

3-
import { coerceRow, type RefResolver, type RefMatch } from './import-coerce.js';
3+
import { coerceRow, firstMissingRequiredField, type RefResolver, type RefMatch } from './import-coerce.js';
44
import type { ExportFieldMeta } from './export-format.js';
55
import { bulkWrite, withTransientRetry, type BulkWriteRowResult } from '@objectstack/core';
66

@@ -310,7 +310,22 @@ export function runImport(opts: RunImportOptions): Promise<ImportRunSummary> {
310310
const willUpdate = existing && typeof existing === 'object';
311311
const willCreate = !willUpdate && (writeMode === 'insert' || writeMode === 'upsert');
312312

313-
if (!willUpdate && !willCreate) {
313+
// Required-field pre-check (CREATE only). Give dry run the same
314+
// verdict the real insert produces — a required (⇒ NOT NULL) field
315+
// with no default and no value fails both — instead of reporting
316+
// success for a row that then dies on `NOT NULL constraint failed`.
317+
// Shared by both paths so they stay identical (and a real insert
318+
// gets a readable `<field> is required` instead of a raw driver
319+
// error). Skipped when automations run: a beforeInsert hook may
320+
// populate a required field, so we defer to the engine's own
321+
// validation rather than false-reject here.
322+
const requiredMiss =
323+
willCreate && !runAutomations ? firstMissingRequiredField(data, metaMap) : null;
324+
325+
if (requiredMiss) {
326+
errCount++;
327+
results[i] = { row: rowNo, ok: false, action: 'failed', field: requiredMiss, code: 'required', error: `${requiredMiss} is required` };
328+
} else if (!willUpdate && !willCreate) {
314329
// update mode, no match → skip.
315330
skipped++;
316331
results[i] = { row: rowNo, ok: true, action: 'skipped', code: 'NO_MATCH' };

0 commit comments

Comments
 (0)