diff --git a/.changeset/import-dryrun-required-validation.md b/.changeset/import-dryrun-required-validation.md new file mode 100644 index 0000000000..44bf661297 --- /dev/null +++ b/.changeset/import-dryrun-required-validation.md @@ -0,0 +1,21 @@ +--- +'@objectstack/rest': patch +--- + +fix(rest): validate required fields in import dry-run to match the real insert + +The bulk-import dry run (`POST /data/:object/import`, `dryRun:true`) only ran cell +coercion and reported every coercible CREATE row as ok — so a row missing a required +NOT-NULL field with no default was green-lit, then died on the real insert with +`NOT NULL constraint failed`. The ImportWizard shows the dry-run result, so it +promised imports that then failed. + +Add a required-field pre-check to the shared import runner (CREATE rows only), +mirroring the engine's insert-time validation (`objectql/record-validator.ts` + +`applyFieldDefaults`): a required field is unsatisfied only when it has no value AND +no default; `system`/`readonly`/`autonumber` and the engine-owned lifecycle columns +are exempt. `ExportFieldMeta` gains `required`/`system`/`readonly`/`hasDefault` +(populated by `buildFieldMetaMap`). Applied to both dry-run and real paths so they +stay identical and a real insert returns a readable ` is required` instead of +a raw driver error; skipped when `runAutomations` is set (a beforeInsert hook may +populate the field). diff --git a/packages/rest/src/export-format.ts b/packages/rest/src/export-format.ts index 2386670477..dccdf4732e 100644 --- a/packages/rest/src/export-format.ts +++ b/packages/rest/src/export-format.ts @@ -22,6 +22,19 @@ export interface ExportFieldMeta { reference?: string; /** Field on the referenced record to show as its label. */ displayField?: string; + // The following four drive the import path's required-field pre-check + // (import-runner.ts). They mirror the engine's insert-time validation + // (objectql record-validator.ts) so a dry run can predict a NOT NULL / + // required failure instead of green-lighting a row the real insert rejects. + // Unused by the export path (formatting only reads type/options/reference). + /** Field is required — a value (or default) must exist on insert. */ + required?: boolean; + /** Engine-owned column the client never supplies (never required of import). */ + system?: boolean; + /** Read-only column the client never supplies (never required of import). */ + readonly?: boolean; + /** Field declares a `defaultValue` the engine applies on insert (satisfies required). */ + hasDefault?: boolean; } /** Field types whose stored value points at another record. */ @@ -78,6 +91,13 @@ export function buildFieldMetaMap(schema: unknown): Map options: Array.isArray(f.options) ? f.options : undefined, reference: typeof f.reference === 'string' ? f.reference : undefined, displayField: typeof f.displayField === 'string' ? f.displayField : undefined, + required: f.required === true, + system: f.system === true, + readonly: f.readonly === true, + // Mirror the engine's `applyFieldDefaults` gate (`f.defaultValue == null` + // ⇒ no default): any non-null default — literal, expression object, or the + // `current_user` token — counts as satisfying a required field. + hasDefault: f.defaultValue != null, }); } return map; diff --git a/packages/rest/src/import-coerce.ts b/packages/rest/src/import-coerce.ts index 8a907efc41..9682b7df38 100644 --- a/packages/rest/src/import-coerce.ts +++ b/packages/rest/src/import-coerce.ts @@ -323,6 +323,53 @@ export async function coerceFieldValue( return { value: trim && typeof raw === 'string' ? raw.trim() : raw }; } +// ── required-field pre-check ─────────────────────────────────────── + +/** + * Engine-owned lifecycle columns the client never supplies. Mirrors + * `record-validator.ts`'s `SKIP_FIELDS`, so the import's required pre-check and + * the engine's insert-time required check agree on which fields the caller is + * responsible for. + */ +const REQUIRED_CHECK_SKIP = new Set([ + 'id', 'created_at', 'created_by', 'updated_at', 'updated_by', +]); + +function isBlankValue(v: unknown): boolean { + return v === undefined || v === null || (typeof v === 'string' && v.trim() === ''); +} + +/** + * The first required field a would-be CREATE leaves unsatisfied, or `null` when + * every required field has either a mapped value or a schema default. + * + * This mirrors the engine's insert-time required check (objectql + * `record-validator.ts` + `applyFieldDefaults`) so the import's dry run predicts + * the SAME verdict the real insert produces: a required (⇒ NOT NULL) field with + * no default and no value fails both. Without it, dry run only reports coercion + * errors and green-lights a row that then dies on `NOT NULL constraint failed`. + * + * Matches the engine's exemptions exactly — `system`/`readonly` columns, the + * runtime-generated `autonumber`, a field carrying a `defaultValue`, and the + * engine-owned lifecycle columns are never required of the importer. Applies to + * CREATE only; an UPDATE touches just the supplied fields, so callers gate on + * "will create" before calling. + */ +export function firstMissingRequiredField( + data: Record, + metaMap: Map, +): string | null { + for (const meta of metaMap.values()) { + if (!meta.required) continue; + if (meta.system || meta.readonly) continue; + if (meta.hasDefault) continue; + if (meta.type === 'autonumber') continue; + if (REQUIRED_CHECK_SKIP.has(meta.name)) continue; + if (isBlankValue(data[meta.name])) return meta.name; + } + return null; +} + /** * Coerce a whole raw row into a storage-ready record. Unknown columns (no * matching field metadata) pass through untouched so ad-hoc / schemaless diff --git a/packages/rest/src/import-integration.test.ts b/packages/rest/src/import-integration.test.ts index 54426b3c7d..51b30ba060 100644 --- a/packages/rest/src/import-integration.test.ts +++ b/packages/rest/src/import-integration.test.ts @@ -97,6 +97,31 @@ const TASK = { }, }; +// Mirrors an AI-built object with required fields and NO default (framework +// import dry-run fidelity): `member_name` (required text) and `status` (required +// select, no default) must be present on create; `tier` is required but carries +// a default, so the engine fills it and the importer must NOT demand it. +const MEMBER = { + name: 'member', label: 'Member', systemFields: false, + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + member_name: { name: 'member_name', type: 'text' as const, label: 'Name', required: true }, + status: { + name: 'status', type: 'select' as const, label: 'Status', required: true, + options: [ + { label: 'Active', value: 'active' }, + { label: 'Frozen', value: 'frozen' }, + { label: 'Lost Contact', value: 'lost_contact' }, + { label: 'Archived', value: 'archived' }, + ], + }, + tier: { + name: 'tier', type: 'select' as const, label: 'Tier', required: true, defaultValue: 'standard', + options: [{ label: 'Standard', value: 'standard' }, { label: 'Gold', value: 'gold' }], + }, + }, +}; + function createMockServer() { const noop = () => {}; return { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} }; @@ -119,6 +144,7 @@ async function boot() { await engine.init(); engine.registry.registerObject(USER as any); engine.registry.registerObject(TASK as any); + engine.registry.registerObject(MEMBER as any); await engine.insert('user', { id: 'u1', name: '张三', email: 'zhang@x.com' }); await engine.insert('user', { id: 'u2', name: '李四', email: 'li@x.com' }); @@ -310,6 +336,71 @@ describe('import route — real engine + protocol integration', () => { }); }); +// --------------------------------------------------------------------------- +// Required-field dry-run fidelity — the dry run must predict the real insert's +// NOT NULL / required failures instead of green-lighting a row the insert +// rejects. Mirrors the live mx1n_member case (required `status` select, no +// default): dryRun said ok, the real insert died on a NOT NULL constraint. +// --------------------------------------------------------------------------- +describe('import route — required-field dry-run fidelity', () => { + let route: any; + let engine: any; + beforeEach(async () => { ({ route, engine } = await boot()); }); + + const imp = (body: any) => { + const res = makeRes(); + return route.handler({ params: { object: 'member' }, body } as any, res).then(() => res); + }; + + it('dry run fails a create row missing a required no-default field — and the real insert agrees', async () => { + const rows = [ + { id: 'm1', member_name: 'Alice' }, // status missing → must fail + { id: 'm2', member_name: 'Bob', status: 'active' }, // complete → ok + ]; + // Dry run: no longer reports success for the row the insert will reject. + const dry = await imp({ format: 'json', dryRun: true, rows }); + expect(dry._json).toMatchObject({ dryRun: true, total: 2, ok: 1, errors: 1 }); + expect(dry._json.results.find((r: any) => !r.ok)).toMatchObject({ row: 1, field: 'status', code: 'required' }); + expect(await engine.findOne('member', { where: { id: 'm2' } })).toBeNull(); // dry run never writes + + // Real insert: SAME verdict (parity), and a readable `status is required` + // instead of a raw `NOT NULL constraint failed: member.status`. + const real = await imp({ format: 'json', rows }); + expect(real._json).toMatchObject({ total: 2, ok: 1, errors: 1, created: 1 }); + expect(real._json.results.find((r: any) => !r.ok)).toMatchObject({ field: 'status', code: 'required', error: 'status is required' }); + expect((await engine.findOne('member', { where: { id: 'm2' } }))?.status).toBe('active'); + expect(await engine.findOne('member', { where: { id: 'm1' } })).toBeNull(); + }); + + it('a required field with a schema default is satisfied without being mapped', async () => { + // `tier` is required but defaulted — the importer must not demand it; the + // engine fills 'standard'. Only member_name + status are supplied. + const res = await imp({ format: 'json', rows: [{ id: 'm3', member_name: 'Cara', status: 'frozen' }] }); + expect(res._json).toMatchObject({ ok: 1, errors: 0, created: 1 }); + expect(await engine.findOne('member', { where: { id: 'm3' } })) + .toMatchObject({ member_name: 'Cara', status: 'frozen', tier: 'standard' }); + }); + + it('flags a required text field too (not just selects); a blank cell counts as missing', async () => { + const res = await imp({ format: 'json', dryRun: true, rows: [ + { id: 'm4', status: 'active' }, // member_name missing + { id: 'm5', member_name: ' ', status: 'active' }, // member_name blank + ] }); + expect(res._json).toMatchObject({ ok: 0, errors: 2 }); + for (const r of res._json.results) expect(r).toMatchObject({ field: 'member_name', code: 'required' }); + }); + + it('required check does not apply to update-mode rows (only the touched fields matter)', async () => { + await engine.insert('member', { id: 'm6', member_name: 'Dan', status: 'active', tier: 'gold' }); + // writeMode:update on an existing match, touching only member_name — status + // is not supplied but the record already has it, so this must NOT fail. + const res = await imp({ format: 'json', writeMode: 'update', matchFields: ['id'], + rows: [{ id: 'm6', member_name: 'Daniel' }] }); + expect(res._json).toMatchObject({ ok: 1, errors: 0, updated: 1 }); + expect((await engine.findOne('member', { where: { id: 'm6' } }))?.member_name).toBe('Daniel'); + }); +}); + // --------------------------------------------------------------------------- // Named mapping artifacts (#2611) — `mappingName` resolves a registered // `mapping` item and applies its fieldMapping pipeline before coercion. diff --git a/packages/rest/src/import-runner.ts b/packages/rest/src/import-runner.ts index fa2f5e8e70..59bdd8a821 100644 --- a/packages/rest/src/import-runner.ts +++ b/packages/rest/src/import-runner.ts @@ -1,6 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import { coerceRow, type RefResolver, type RefMatch } from './import-coerce.js'; +import { coerceRow, firstMissingRequiredField, type RefResolver, type RefMatch } from './import-coerce.js'; import type { ExportFieldMeta } from './export-format.js'; import { bulkWrite, withTransientRetry, type BulkWriteRowResult } from '@objectstack/core'; @@ -310,7 +310,22 @@ export function runImport(opts: RunImportOptions): Promise { const willUpdate = existing && typeof existing === 'object'; const willCreate = !willUpdate && (writeMode === 'insert' || writeMode === 'upsert'); - if (!willUpdate && !willCreate) { + // Required-field pre-check (CREATE only). Give dry run the same + // verdict the real insert produces — a required (⇒ NOT NULL) field + // with no default and no value fails both — instead of reporting + // success for a row that then dies on `NOT NULL constraint failed`. + // Shared by both paths so they stay identical (and a real insert + // gets a readable ` is required` instead of a raw driver + // error). Skipped when automations run: a beforeInsert hook may + // populate a required field, so we defer to the engine's own + // validation rather than false-reject here. + const requiredMiss = + willCreate && !runAutomations ? firstMissingRequiredField(data, metaMap) : null; + + if (requiredMiss) { + errCount++; + results[i] = { row: rowNo, ok: false, action: 'failed', field: requiredMiss, code: 'required', error: `${requiredMiss} is required` }; + } else if (!willUpdate && !willCreate) { // update mode, no match → skip. skipped++; results[i] = { row: rowNo, ok: true, action: 'skipped', code: 'NO_MATCH' };