diff --git a/.changeset/import-dryrun-asks-for-the-verdict.md b/.changeset/import-dryrun-asks-for-the-verdict.md new file mode 100644 index 0000000000..4c004be2d4 --- /dev/null +++ b/.changeset/import-dryrun-asks-for-the-verdict.md @@ -0,0 +1,50 @@ +--- +"@objectstack/rest": patch +"@objectstack/objectql": patch +--- + +fix(rest,objectql): the import dry run asks the engine for its verdict instead of predicting it (#4633 ruling D) + +`POST /api/v1/data/:object/import?dryRun=true` green-lit rows the very same +endpoint then rejected. Measured on 17.0.0-rc.1: a CSV cell aimed at a +structured `address` field reported `{ ok: 1, created: 1 }` on the dry run and +`{ errors: 1, code: 'VALIDATION_FAILED' }` on the real write. + +The dry run predicted the write's verdict with a hand-copied mirror of a slice +of the engine's rules (`import-coerce.ts`'s `firstMissingRequiredField` and +`firstConstraintViolation`). A copy cannot structurally keep up with the family +it mirrors: ADR-0104 value shapes (`address` / `location` / references / media), +`format` checks, object-level `validations` and the state machine had no +counterpart, and `coerceFieldValue` routes structured shapes through its +pass-through catch-all, so no verdict was formed at all. + +**The mirror is retired.** The dry run now calls `DataProtocol.validateData` +(#6037), which runs the same `validateRecord` / `evaluateValidationRules` that +`insert()` runs, under the deployment's own ADR-0104 posture — so a bad value +shape is an error on a self-certified deployment and an admitted warning on a +warn-first one, exactly as on the write. Agreement is by construction, not by a +copy kept in step by hand. + +Also in this change: + +- **`engine.validate()` now resolves `defaultValue`s and seeds owned roll-up + `summary` fields before validating, on `insert` mode**, because `insert()` + does. Without it a required-but-defaulted column left unmapped was previewed + `failed` and written `created` — a false alarm on the row a preview is meant + to reassure you about. `update` mode still does not default (#2706). +- **A row report failed by validation now names the offending column.** The + engine's `ValidationError` carries `fields[]`, so the row's `field` is set and + its `code` is the field-level code (`required`, `min_value`, `max_length`, + `invalid_type`, …) rather than the wrapper's `VALIDATION_FAILED`. This is the + same vocabulary the dry run and the per-cell coercion failures already spoke; + before, a `min: 0` violation was `min_value` on the dry run and + `VALIDATION_FAILED` on the write. +- **Dry-run rows may carry `warnings[]`** — findings this deployment admits + rather than rejects (ADR-0104 warn-first). The row is `ok`, and the complaint + is visible instead of living only in a server log line. + +A protocol that does not implement `validateData` (plugin-auth's identity +import, whose write is better-auth rather than the engine) is not handed a +substitute: its dry run reports coercion and create/update/skip resolution only. +An engine-derived preview of a non-engine write would report findings that write +never produces. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 7858da326e..cdb2722393 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -5427,9 +5427,31 @@ export class ObjectQL implements IObjectQLEngine { ): Promise { object = this.resolveObjectName(object); const mode = options?.mode ?? 'insert'; - const rows = Array.isArray(data) ? data : [data]; const schemaForValidation = this._registry.getObject(object); + // [#4633] `insert()` resolves `defaultValue`s and seeds owned roll-up + // `summary` fields BEFORE it validates, so a required field carrying a + // default is never missing by the time `validateRecord` runs. The preview + // has to walk the same two steps or it reports `required` on a row the + // write happily creates — a FALSE ALARM, and the one failure mode the + // ruling that created this operation set out to prevent. (Measured on the + // import dry run: `tier: { required: true, defaultValue: 'standard' }` + // unmapped ⇒ preview `failed`, write `created`.) + // + // Both helpers are pure and synchronous: they read the registry, copy the + // row, and touch neither driver nor hook — so running them here keeps the + // "nothing is written, nothing is executed" contract intact. `update()` + // deliberately does not default (#2706: a PATCH's explicit `null` means + // "clear it"), so neither does an `update`-mode preview. + const rawRows = Array.isArray(data) ? data : [data]; + const nowSnapshot = new Date(); + const rows: Record[] = mode === 'insert' + ? rawRows.map((row) => this.initializeSummaryFields( + object, + this.applyFieldDefaults(object, row, options?.context, nowSnapshot), + ) as Record) + : rawRows; + // Resolved once for the whole set, exactly as the write path resolves them // once per batch — this is the "same posture as the real write" guarantee. const mediaValueShapeStrict = await this.mediaValueShapeStrictFor(schemaForValidation); diff --git a/packages/objectql/src/validate-only.test.ts b/packages/objectql/src/validate-only.test.ts index 306c1973ff..4dd73cecf8 100644 --- a/packages/objectql/src/validate-only.test.ts +++ b/packages/objectql/src/validate-only.test.ts @@ -106,6 +106,51 @@ describe('engine.validate() — validate-only (#6037)', () => { expect(out.results![1].errors[0].field).toBe('email'); }); + // [#4633] The preview must walk the pre-validation steps `insert()` walks, + // or it reports findings on rows the write creates. Discovered by the import + // dry run this operation exists to serve: a required-but-defaulted column + // left unmapped was previewed `failed` and written `created`. + describe('insert-time preparation the write does before it validates', () => { + const DEFAULTED = { + ...LEAD, + fields: { + ...LEAD.fields, + tier: { type: 'text', required: true, defaultValue: 'standard' }, + }, + }; + + it('a required field with a `defaultValue` is NOT reported missing — the write defaults it first', async () => { + const { engine } = makeEngine([DEFAULTED]); + const out = await engine.validate('lead', { company: 'Acme' }); + expect(out.results![0].errors).toEqual([]); + expect(out.valid).toBe(true); + }); + + it('agrees with the write on that row — preview and insert, one engine', async () => { + const { engine } = makeEngine([DEFAULTED]); + const previewValid = (await engine.validate('lead', { company: 'Acme' })).valid; + let writeSucceeded = true; + try { await engine.insert('lead', { company: 'Acme' }); } catch { writeSucceeded = false; } + expect(previewValid).toBe(writeSucceeded); + expect(writeSucceeded).toBe(true); + }); + + it('does not default in `update` mode — a PATCH never re-applies defaults (#2706)', async () => { + const { engine } = makeEngine([DEFAULTED]); + // Supplying an explicit null on update means "clear it", so the preview + // must judge the null the caller sent, not a default it never gets. + const out = await engine.validate('lead', { tier: null }, { mode: 'update' }); + expect(out.results![0].errors.some((e) => e.field === 'tier')).toBe(true); + }); + + it('never mutates the caller’s row objects', async () => { + const { engine } = makeEngine([DEFAULTED]); + const row: Record = { company: 'Acme' }; + await engine.validate('lead', row); + expect(row).toEqual({ company: 'Acme' }); + }); + }); + it('judges only supplied keys in `update` mode, matching a PATCH', async () => { const { engine } = makeEngine(); // `company` is required but absent. An insert rejects that; a PATCH that diff --git a/packages/rest/src/export-format.ts b/packages/rest/src/export-format.ts index a2ef4545da..bcf170436f 100644 --- a/packages/rest/src/export-format.ts +++ b/packages/rest/src/export-format.ts @@ -24,11 +24,16 @@ export interface ExportFieldMeta { displayField?: string; /** Field holds multiple values (an array), e.g. a `multiple: true` lookup. */ multiple?: boolean; - // 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). + // ── constraint metadata, no longer read by the import path ────────── + // + // The eight keys below were added for the import dry run's hand-copied + // pre-check mirror (`firstMissingRequiredField` / `firstConstraintViolation`, + // framework#3956). That mirror is retired: the dry run now asks the engine + // for its verdict through `DataProtocol.validateData` (#4633 ruling D), which + // reads the object's own schema — so nothing in this repo consults these any + // more. Kept for now rather than removed in the same PR: `ExportFieldMeta` is + // exported from `@objectstack/rest`, and their retirement is a separable + // change with its own sweep. /** Field is required — a value (or default) must exist on insert. */ required?: boolean; /** Engine-owned column the client never supplies (never required of import). */ @@ -37,10 +42,6 @@ export interface ExportFieldMeta { readonly?: boolean; /** Field declares a `defaultValue` the engine applies on insert (satisfies required). */ hasDefault?: boolean; - // The bounds below drive the import path's field-constraint pre-check - // (import-coerce.ts `firstConstraintViolation`), mirroring the engine's - // `validateRecord` so a dry run predicts a range/length rejection instead of - // green-lighting a row the real write then fails (framework#3956). /** Lower bound for numeric fields. */ min?: number; /** Upper bound for numeric fields. */ diff --git a/packages/rest/src/import-coerce.test.ts b/packages/rest/src/import-coerce.test.ts index 29a0b9116b..da120e0710 100644 --- a/packages/rest/src/import-coerce.test.ts +++ b/packages/rest/src/import-coerce.test.ts @@ -14,7 +14,6 @@ import { matchOption, splitMulti, coerceRow, - firstConstraintViolation, } from './import-coerce'; import type { ExportFieldMeta } from './export-format'; @@ -274,76 +273,23 @@ describe('coerceRow', () => { }); }); -describe('firstConstraintViolation (framework#3956)', () => { - const meta = (defs: Record>): Map => { - const m = new Map(); - for (const [name, d] of Object.entries(defs)) m.set(name, { name, ...d }); - return m; - }; - - it('reports a numeric value below `min` with the engine\'s own message', () => { - // The issue's repro: penalty_amount { type: 'number', min: 0, max: 9999999.99 } - const metaMap = meta({ penalty_amount: { type: 'number', min: 0, max: 9999999.99 } }); - expect(firstConstraintViolation({ penalty_amount: -500 }, metaMap)).toEqual({ - field: 'penalty_amount', code: 'min_value', message: 'penalty_amount must be ≥ 0', - }); - }); - - it('reports a numeric value above `max`', () => { - const metaMap = meta({ pct: { type: 'percent', max: 100 } }); - expect(firstConstraintViolation({ pct: 101 }, metaMap)).toEqual({ - field: 'pct', code: 'max_value', message: 'pct must be ≤ 100', - }); - }); - - it('reports string length violations both ways', () => { - const metaMap = meta({ code: { type: 'text', minLength: 3, maxLength: 5 } }); - expect(firstConstraintViolation({ code: 'abcdef' }, metaMap)).toEqual({ - field: 'code', code: 'max_length', message: 'code must be ≤ 5 characters (got 6)', - }); - expect(firstConstraintViolation({ code: 'ab' }, metaMap)).toEqual({ - field: 'code', code: 'min_length', message: 'code must be ≥ 3 characters (got 2)', - }); - }); - - it('accepts values inside the declared bounds', () => { - const metaMap = meta({ - amount: { type: 'currency', min: 0, max: 100 }, - title: { type: 'text', maxLength: 10 }, - }); - expect(firstConstraintViolation({ amount: 0 }, metaMap)).toBeNull(); - expect(firstConstraintViolation({ amount: 100 }, metaMap)).toBeNull(); - expect(firstConstraintViolation({ title: 'ten chars!' }, metaMap)).toBeNull(); - }); - - it('skips absent values — a bound never fires on a field the row omits', () => { - const metaMap = meta({ amount: { type: 'number', min: 10 } }); - expect(firstConstraintViolation({}, metaMap)).toBeNull(); - expect(firstConstraintViolation({ amount: null }, metaMap)).toBeNull(); - expect(firstConstraintViolation({ amount: '' }, metaMap)).toBeNull(); - }); - - it('skips system / readonly columns the importer never supplies', () => { - const metaMap = meta({ - seq: { type: 'number', min: 100, system: true }, - score: { type: 'number', min: 100, readonly: true }, - }); - expect(firstConstraintViolation({ seq: 1, score: 1 }, metaMap)).toBeNull(); - }); - - it('leaves an unparseable number to coerceRow rather than double-reporting', () => { - const metaMap = meta({ amount: { type: 'number', min: 0 } }); - expect(firstConstraintViolation({ amount: 'abc' }, metaMap)).toBeNull(); - }); - - it('bound-checks only the types the engine bound-checks', () => { - // `progress` is numeric per the spec but the engine's validateOne leaves it - // unchecked — mirroring the wider spec set here would reject rows the real - // write accepts. - const metaMap = meta({ p: { type: 'progress', min: 0, max: 1 } }); - expect(firstConstraintViolation({ p: 42 }, metaMap)).toBeNull(); - }); -}); +// ── the retired constraint mirror (framework#3956) ──────────────────── +// +// `firstConstraintViolation` used to be pinned here with eight unit cases. It +// is gone: the import dry run asks the engine for its verdict through +// `DataProtocol.validateData` instead of re-deriving one (#4633 ruling D), so +// there is no longer a copy of the engine's numeric-range / string-length +// rules in this file to keep in step. +// +// Its VERDICTS did not retire with it — `import-dryrun-parity.test.ts` asserts +// every one of them (min_value, max_value, min_length, max_length, an omitted +// bounded field, boundary values) against a live engine, and asserts the dry +// run and the real write agree on each. Two of the eight cases have no +// successor by design: "skips system / readonly columns" and "bound-checks +// only the types the engine bound-checks" existed because a hand-maintained +// copy could disagree with the engine about WHICH fields and types are in +// scope. With no copy, there is no second opinion to police — that question is +// `record-validator.ts`'s alone, and pinned in objectql's own tests. /** * #3957 — the importer's row report is where a user meets these messages, and diff --git a/packages/rest/src/import-coerce.ts b/packages/rest/src/import-coerce.ts index 6743e4dc9c..df80641612 100644 --- a/packages/rest/src/import-coerce.ts +++ b/packages/rest/src/import-coerce.ts @@ -438,152 +438,28 @@ 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; -} - -// ── field-constraint pre-check ───────────────────────────────────── - -/** - * Number field types the engine bound-checks with `min` / `max`, and string - * field types it bound-checks with `minLength` / `maxLength`. - * - * These are the engine's OWN lists (objectql `record-validator.ts` - * `validateOne`), deliberately NOT the spec's `NUMERIC_VALUE_TYPES` / - * `STRING_VALUE_TYPES` — those are wider (`progress`, `summary`, `color`, - * `signature`, …) and the engine leaves their values unchecked. Using the - * wider sets here would make the dry run reject rows the real write accepts, - * trading a false all-clear for a false alarm. - */ -const BOUNDED_NUMBER_TYPES = new Set(['number', 'currency', 'percent', 'rating', 'slider']); -const BOUNDED_STRING_TYPES = new Set([ - 'text', 'textarea', 'email', 'url', 'phone', 'password', 'markdown', 'html', 'richtext', 'code', -]); - -/** - * The first declared bound a coerced row violates, or `null` when every - * supplied value is in range. - * - * Mirrors the numeric-range and string-length rules of the engine's - * `validateRecord` — same type applicability, same comparison, same `code` and - * `message` text — so the import's dry run predicts the verdict the real write - * produces (framework#3956). Before this, a dry run only reported *coercion* - * failures (a cell that isn't a number at all), so `-500` in a `min: 0` column - * was reported valid and then rejected by the write with - * `penalty_amount must be ≥ 0`. - * - * Applies to CREATE and UPDATE alike: the engine validates every supplied - * value on both, and a value the row doesn't carry is skipped here exactly as - * `validateOne` skips a missing one. - * - * NOT a complete mirror of `validateRecord`, and not meant to be — format - * checks (email/url/phone), object-level `validations` rules, uniqueness and - * the state machine still surface only on the real write. Closing those means - * validating through the engine itself rather than growing this copy. - */ -export function firstConstraintViolation( - data: Record, - metaMap: Map, - // Optional so existing callers keep working; supplied by the runner so a dry - // run's verdict reads in the same language as the real rejection (#3957). - ctx: Pick = {}, -): FieldCoerceError | null { - // Mirrors the engine's own rendering — same catalog, same label resolution — - // so a predicted violation and the real one are the SAME sentence. - const bound = ( - meta: ExportFieldMeta, - code: FieldErrorCode, - constraint: Record, - ): FieldCoerceError => ({ - field: meta.name, - code, - message: renderValidationMessage( - { - messageKey: code, - label: meta.label?.trim() || meta.name, - field: meta.name, - params: constraint, - }, - { locale: ctx.locale, translate: ctx.translate }, - ), - }); - for (const meta of metaMap.values()) { - if (meta.system || meta.readonly) continue; - if (REQUIRED_CHECK_SKIP.has(meta.name)) continue; - const value = data[meta.name]; - if (isBlankValue(value)) continue; // absent → nothing to bound-check - const t = meta.type ?? ''; - - if (BOUNDED_NUMBER_TYPES.has(t)) { - const n = typeof value === 'number' ? value : Number(value); - if (!Number.isFinite(n)) continue; // a non-number is coerceRow's verdict, not ours - if (meta.min !== undefined && n < meta.min) { - return bound(meta, 'min_value', { min: meta.min }); - } - if (meta.max !== undefined && n > meta.max) { - return bound(meta, 'max_value', { max: meta.max }); - } - continue; - } - - if (BOUNDED_STRING_TYPES.has(t)) { - // `String(value)` matches the engine, which stringifies a non-string - // (e.g. a `multiple` text cell joined by the export separator) before - // measuring it. - const s = typeof value === 'string' ? value : String(value); - if (meta.maxLength !== undefined && s.length > meta.maxLength) { - return bound(meta, 'max_length', { maxLength: meta.maxLength, actual: s.length }); - } - if (meta.minLength !== undefined && s.length < meta.minLength) { - return bound(meta, 'min_length', { minLength: meta.minLength, actual: s.length }); - } - } - } - return null; -} +// ── the retired pre-check mirror ─────────────────────────────────── +// +// `firstMissingRequiredField` and `firstConstraintViolation` used to live here +// (framework#3956): hand-copied re-implementations of the engine's required +// check and its numeric-range / string-length rules, kept in step with +// `record-validator.ts` by hand so the import's dry run could PREDICT the +// verdict the real write produces. +// +// A copy cannot structurally keep up with the family it mirrors, and the gap +// was measured (#4633): a CSV cell aimed at a `Field.address` passed the dry +// run — `coerceFieldValue` routes structured value shapes through its +// pass-through catch-all, so no verdict was formed at all — and the write then +// rejected it with `VALIDATION_FAILED`. The same hole covered `format` checks, +// object-level `validations`, and the state machine. +// +// Ruling D (maintainer, 2026-08-06) retired the mirror rather than growing it: +// the dry run now ASKS for the verdict through `DataProtocol.validateData` +// (#6037), which runs the same `validateRecord` / `evaluateValidationRules` +// `insert()` runs, under the deployment's own ADR-0104 posture. See +// `import-runner.ts`'s dry-run branch. Every verdict these two produced is +// re-asserted through that route in `import-dryrun-parity.test.ts` — retiring +// the mirror must not silently retire its coverage. /** * Coerce a whole raw row into a storage-ready record. Unknown columns (no diff --git a/packages/rest/src/import-dryrun-parity.test.ts b/packages/rest/src/import-dryrun-parity.test.ts new file mode 100644 index 0000000000..a1d834b057 --- /dev/null +++ b/packages/rest/src/import-dryrun-parity.test.ts @@ -0,0 +1,317 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Import dry run == real write, for the WHOLE validation family (#4633). + * + * The dry run's stated contract is that it reports the verdict the real write + * produces. It used to keep that promise with a hand-copied MIRROR of a slice + * of the engine's rules (`import-coerce.ts`'s `firstMissingRequiredField` / + * `firstConstraintViolation`), which structurally could not cover the rest of + * the family — and the gap was measured on a `Field.address`: a CSV cell aimed + * at a structured value shape passed the dry run and was rejected by the write + * with `VALIDATION_FAILED`. + * + * Ruling D (maintainer, 2026-08-06) replaced prediction with the verdict + * itself: the dry run asks `DataProtocol.validateData` (#6037 / PR #6474), + * which runs the same `validateRecord` / `evaluateValidationRules` `insert()` + * runs. So this file never pins the dry run's output ALONE — every case runs + * BOTH halves against one live engine and asserts they agree. A test that + * pinned only the dry run would stay green while the two drifted apart again, + * which is the entire defect. + * + * Both ADR-0104 postures are exercised, because the maintainer explicitly + * VETOED an unconditionally-strict pre-check (option B): on a warn-first + * deployment the write ACCEPTS a bad value shape after warning, so a dry run + * that reported `failed` there would be a mirror-induced phantom defect. The + * posture cases below are what keeps that veto enforced rather than minuted. + */ + +import { describe, it, expect, afterEach, beforeEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from './rest-server'; + +function makeSqliteDriver() { + return new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); +} + +const liveEngines: ObjectQL[] = []; +afterEach(async () => { + while (liveEngines.length) { + try { await liveEngines.pop()?.destroy(); } catch { /* noop */ } + } + delete process.env.OS_DATA_VALUE_SHAPE_STRICT_ENABLED; + delete process.env.OS_ALLOW_LAX_VALUE_SHAPES; +}); + +/** + * The issue's own object: HotCRM's account with a json-backed `Field.address` + * (`billing_address`), plus the fields the retiring mirror used to cover so + * their verdicts can be re-asserted through the new route. + */ +const CRM_ACCOUNT = { + name: 'crm_account', label: 'Account', systemFields: false, + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const, label: 'Name', required: true }, + // The issue's field. `coerceCell` routes `address` through its catch-all + // ("pass through"), so the cell reaches validation as a bare string. + billing_address: { name: 'billing_address', type: 'address' as const, label: 'Billing Address' }, + // Formerly covered by the mirror — kept so retiring it cannot silently + // retire its coverage. + tier: { + name: 'tier', type: 'select' as const, label: 'Tier', required: true, defaultValue: 'standard', + options: [{ label: 'Standard', value: 'standard' }, { label: 'Gold', value: 'gold' }], + }, + penalty_amount: { name: 'penalty_amount', type: 'number' as const, label: '处罚金额', min: 0, max: 9999999.99 }, + nickname: { name: 'nickname', type: 'text' as const, label: 'Nickname', minLength: 3, maxLength: 5 }, + }, +}; + +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; +} + +async function boot() { + const engine = new ObjectQL(); + liveEngines.push(engine); + engine.registerDriver(makeSqliteDriver(), true); + await engine.init(); + engine.registry.registerObject(CRM_ACCOUNT as any); + await engine.syncSchemas(); + + 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 route = rest.getRoutes().find( + (r: any) => r.method === 'POST' && r.path === '/api/v1/data/:object/import', + ); + return { engine, protocol, route }; +} + +describe('import dry run == real write (#4633)', () => { + let route: any; + let engine: any; + + const imp = (body: any) => { + const res = makeRes(); + return route.handler({ params: { object: 'crm_account' }, body } as any, res).then(() => res); + }; + + /** + * Run the SAME payload twice — dry run, then for real — and hand back both + * row reports. Every case in this file compares the two; the contract under + * test is agreement, so a case that read only one of them could not fail + * when they diverge. + */ + const bothWays = async (rows: any[], extra: Record = {}) => { + const dry = await imp({ format: 'json', dryRun: true, rows, ...extra }); + const real = await imp({ format: 'json', rows, ...extra }); + return { dry: dry._json, real: real._json }; + }; + + const verdictOf = (report: any, i = 0) => { + const r = report.results[i]; + return { ok: r.ok, action: r.action, field: r.field, code: r.code, error: r.error }; + }; + + describe('structured value shapes — self-certified (strict) deployment', () => { + beforeEach(async () => { + process.env.OS_DATA_VALUE_SHAPE_STRICT_ENABLED = '1'; + ({ route, engine } = await boot()); + }); + + it("the issue's repro: a string aimed at a `Field.address` is refused by BOTH halves, identically", async () => { + const rows = [{ id: 'a1', name: 'Acme', billing_address: '123 Main St, Springfield' }]; + const { dry, real } = await bothWays(rows); + + // The measured defect: dry run said `{ ok: 1, created: 1 }` here while + // the write said `{ errors: 1 }`. + expect(dry).toMatchObject({ dryRun: true, total: 1, ok: 0, errors: 1, created: 0 }); + expect(real).toMatchObject({ total: 1, ok: 0, errors: 1, created: 0 }); + + // Rejection-class case: assert the ENVELOPE (code + message), not merely + // that the row was not ok. `invalid_type` is the field-level code + // `record-validator.ts` fails a bad value shape with, and the message is + // the engine's own sentence — the same one the issue measured on write. + expect(verdictOf(dry)).toMatchObject({ + ok: false, action: 'failed', field: 'billing_address', code: 'invalid_type', + }); + expect(verdictOf(dry).error).toContain('Billing Address has an invalid address value'); + // Agreement is the contract: pin the two against each other so this case + // fails if EITHER side drifts. + expect(verdictOf(dry)).toEqual(verdictOf(real)); + + expect(await engine.findOne('crm_account', { where: { id: 'a1' } })).toBeNull(); + }); + + it('a well-formed address object is accepted by both halves', async () => { + const rows = [{ + id: 'a2', name: 'Globex', + billing_address: { street: '1 Infinite Loop', city: 'Cupertino', country: 'US' }, + }]; + const { dry, real } = await bothWays(rows); + expect(dry).toMatchObject({ ok: 1, errors: 0, created: 1 }); + expect(real).toMatchObject({ ok: 1, errors: 0, created: 1 }); + expect(await engine.findOne('crm_account', { where: { id: 'a2' } })).toBeTruthy(); + }); + }); + + describe('structured value shapes — warn-first (un-migrated) deployment', () => { + beforeEach(async () => { + // The posture option B was vetoed over: `record-validator.ts` ADMITS the + // row here after warning, so the write CREATES it. + process.env.OS_ALLOW_LAX_VALUE_SHAPES = '1'; + ({ route, engine } = await boot()); + }); + + it('ADMITS the same row the strict deployment refuses — and the dry run says `created`, not `failed`', async () => { + const rows = [{ id: 'a3', name: 'Initech', billing_address: '123 Main St, Springfield' }]; + const { dry, real } = await bothWays(rows); + + // An unconditionally-strict pre-check would report `errors: 1` here while + // the write reports `created: 1` — the phantom defect the ruling rejected. + expect(dry).toMatchObject({ dryRun: true, total: 1, ok: 1, errors: 0, created: 1 }); + expect(real).toMatchObject({ total: 1, ok: 1, errors: 0, created: 1 }); + expect(verdictOf(dry)).toEqual(verdictOf(real)); + + // The row IS written on this deployment — that is what makes the dry + // run's `created` truthful rather than lenient. + expect(await engine.findOne('crm_account', { where: { id: 'a3' } })).toBeTruthy(); + }); + + it('reports the admission as a per-row WARNING, so "accepted for now" is visible', async () => { + const rows = [{ id: 'a4', name: 'Umbrella', billing_address: 'not an address object' }]; + const dry = (await imp({ format: 'json', dryRun: true, rows }))._json; + expect(dry.results[0]).toMatchObject({ ok: true, action: 'created' }); + expect(dry.results[0].warnings).toEqual([ + expect.objectContaining({ field: 'billing_address', code: 'invalid_type' }), + ]); + }); + }); + + // ─────────────────────────────────────────────────────────────────────── + // The retiring mirror's coverage. `firstMissingRequiredField` and + // `firstConstraintViolation` are gone; every verdict they used to produce + // must still arrive — now from the engine, through `validateData`. + // ─────────────────────────────────────────────────────────────────────── + describe('the retired mirror’s verdicts still arrive, through the new route', () => { + beforeEach(async () => { ({ route, engine } = await boot()); }); + + it('required + no value: same verdict from both halves (was `firstMissingRequiredField`)', async () => { + const rows = [{ id: 'r1', billing_address: null }]; // `name` is required + const { dry, real } = await bothWays(rows, { runAutomations: false }); + expect(dry).toMatchObject({ ok: 0, errors: 1, created: 0 }); + expect(real).toMatchObject({ ok: 0, errors: 1, created: 0 }); + expect(verdictOf(dry)).toMatchObject({ field: 'name', code: 'required', error: 'Name is required' }); + expect(verdictOf(dry)).toEqual(verdictOf(real)); + expect(await engine.findOne('crm_account', { where: { id: 'r1' } })).toBeNull(); + }); + + it('required WITH a schema default is satisfied without being mapped — no false alarm', async () => { + // `tier` is required and defaulted. The write applies the default before + // validating, so the preview must too; reporting `required` here would be + // a false alarm on a row the write happily creates. + const rows = [{ id: 'r2', name: 'Stark' }]; + const { dry, real } = await bothWays(rows, { runAutomations: false }); + expect(dry).toMatchObject({ ok: 1, errors: 0, created: 1 }); + expect(real).toMatchObject({ ok: 1, errors: 0, created: 1 }); + expect((await engine.findOne('crm_account', { where: { id: 'r2' } }))?.tier).toBe('standard'); + }); + + it('numeric range: same verdict from both halves (was `firstConstraintViolation`, #3956)', async () => { + const rows = [{ id: 'r3', name: 'Wayne', penalty_amount: -500 }]; + const { dry, real } = await bothWays(rows); + expect(dry).toMatchObject({ ok: 0, errors: 1, created: 0 }); + expect(real).toMatchObject({ ok: 0, errors: 1, created: 0 }); + // #3957 — the message names the field by its declared LABEL. + expect(verdictOf(dry)).toMatchObject({ + field: 'penalty_amount', code: 'min_value', error: '处罚金额 must be ≥ 0', + }); + expect(verdictOf(dry)).toEqual(verdictOf(real)); + }); + + it('string length: same verdict from both halves (was `firstConstraintViolation`, #3956)', async () => { + const rows = [{ id: 'r4', name: 'Oscorp', nickname: 'toolongname' }]; + const { dry, real } = await bothWays(rows); + expect(dry).toMatchObject({ ok: 0, errors: 1, created: 0 }); + expect(real).toMatchObject({ ok: 0, errors: 1, created: 0 }); + expect(verdictOf(dry)).toMatchObject({ + field: 'nickname', code: 'max_length', error: 'Nickname must be ≤ 5 characters (got 11)', + }); + expect(verdictOf(dry)).toEqual(verdictOf(real)); + }); + + it('numeric range, the other bound: above `max`, same verdict from both halves', async () => { + const rows = [{ id: 'r3b', name: 'Wayne', penalty_amount: 1e9 }]; + const { dry, real } = await bothWays(rows); + expect(dry).toMatchObject({ ok: 0, errors: 1, created: 0 }); + expect(real).toMatchObject({ ok: 0, errors: 1, created: 0 }); + expect(verdictOf(dry)).toMatchObject({ field: 'penalty_amount', code: 'max_value' }); + expect(verdictOf(dry)).toEqual(verdictOf(real)); + }); + + it('string length, the other bound: below `minLength`, same verdict from both halves', async () => { + const rows = [{ id: 'r4b', name: 'Oscorp', nickname: 'ab' }]; + const { dry, real } = await bothWays(rows); + expect(dry).toMatchObject({ ok: 0, errors: 1, created: 0 }); + expect(real).toMatchObject({ ok: 0, errors: 1, created: 0 }); + expect(verdictOf(dry)).toMatchObject({ + field: 'nickname', code: 'min_length', error: 'Nickname must be ≥ 3 characters (got 2)', + }); + expect(verdictOf(dry)).toEqual(verdictOf(real)); + }); + + it('a bounded field the row omits is not a finding — a bound never fires on an absent value', async () => { + const rows = [{ id: 'r4c', name: 'Tyrell' }]; // no penalty_amount, no nickname + const { dry, real } = await bothWays(rows); + expect(dry).toMatchObject({ ok: 1, errors: 0, created: 1 }); + expect(real).toMatchObject({ ok: 1, errors: 0, created: 1 }); + }); + + it('boundary values stay legal — the new route must not over-reject either', async () => { + const rows = [{ id: 'r5', name: 'Cyberdyne', penalty_amount: 0, nickname: 'exact' }]; + const { dry, real } = await bothWays(rows); + expect(dry).toMatchObject({ ok: 1, errors: 0, created: 1 }); + expect(real).toMatchObject({ ok: 1, errors: 0, created: 1 }); + }); + + it('update mode judges only the supplied keys — an unmapped required field is not a finding', async () => { + await engine.insert('crm_account', { id: 'r6', name: 'Hooli', tier: 'gold' }); + const body = { writeMode: 'update', matchFields: ['id'] }; + const dry = (await imp({ format: 'json', dryRun: true, rows: [{ id: 'r6', nickname: 'okay' }], ...body }))._json; + const real = (await imp({ format: 'json', rows: [{ id: 'r6', nickname: 'okay' }], ...body }))._json; + expect(dry).toMatchObject({ ok: 1, errors: 0, updated: 1 }); + expect(real).toMatchObject({ ok: 1, errors: 0, updated: 1 }); + }); + + it('update mode still bound-checks a supplied value, both halves', async () => { + await engine.insert('crm_account', { id: 'r7', name: 'Aperture', penalty_amount: 10 }); + const body = { writeMode: 'update', matchFields: ['id'] }; + const rows = [{ id: 'r7', penalty_amount: -1 }]; + const dry = (await imp({ format: 'json', dryRun: true, rows, ...body }))._json; + const real = (await imp({ format: 'json', rows, ...body }))._json; + expect(dry).toMatchObject({ ok: 0, errors: 1, updated: 0 }); + expect(real).toMatchObject({ ok: 0, errors: 1, updated: 0 }); + expect(verdictOf(dry)).toMatchObject({ field: 'penalty_amount', code: 'min_value' }); + expect(verdictOf(dry)).toEqual(verdictOf(real)); + }); + }); +}); diff --git a/packages/rest/src/import-runner.ts b/packages/rest/src/import-runner.ts index 7225013c4a..5ab14a847a 100644 --- a/packages/rest/src/import-runner.ts +++ b/packages/rest/src/import-runner.ts @@ -1,9 +1,10 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { randomUUID } from 'node:crypto'; -import { coerceRow, firstMissingRequiredField, firstConstraintViolation, type RefResolver, type RefMatch } from './import-coerce.js'; +import { coerceRow, type RefResolver, type RefMatch } from './import-coerce.js'; import type { ExportFieldMeta } from './export-format.js'; -import { renderValidationMessage, type ValidationMessageTranslator } from '@objectstack/spec/system'; +import type { ValidationMessageTranslator } from '@objectstack/spec/system'; +import type { ValidateDataIssue, ValidateDataRequest, ValidateDataResponse } from '@objectstack/spec/api'; import { bulkWrite, withTransientRetry, defaultIsTransientError, type BulkWriteRowResult } from '@objectstack/core'; /** @@ -21,6 +22,20 @@ import { bulkWrite, withTransientRetry, defaultIsTransientError, type BulkWriteR * one `p.createData` call per row — see framework#2678. A protocol that * doesn't implement `createManyData` falls back to the original per-row * `createData` path unchanged. + * + * ## The dry run asks; it does not predict (#4633 ruling D) + * + * A dry run's contract is that it reports the verdict the real write produces. + * It used to keep that promise with a hand-copied mirror of a slice of the + * engine's rules (`import-coerce.ts`), which structurally could not cover the + * rest of the family — measured on a `Field.address`, where the dry run formed + * no verdict at all and the write answered `VALIDATION_FAILED`. + * + * So the dry-run branch below calls {@link ImportProtocolLike.validateData} + * (#6037) instead: the engine runs the same `validateRecord` / + * `evaluateValidationRules` `insert()` runs, under this deployment's own + * ADR-0104 posture, and persists nothing. Agreement is by construction rather + * than by a copy kept in step by hand. */ export type ImportAction = 'created' | 'updated' | 'skipped' | 'failed'; @@ -33,6 +48,14 @@ export interface ImportRowResult { field?: string; error?: string; code?: string; + /** + * Findings this deployment ADMITS rather than rejects — today, ADR-0104 + * value shapes under a warn-first posture (#4633). The row is `ok`: the + * write stores it and logs the same complaint. Present on dry-run rows, + * where they are the difference between "this row is fine" and "this row is + * fine HERE"; a real write has no equivalent channel to report them on. + */ + warnings?: ValidateDataIssue[]; } /** Running tallies handed to {@link RunImportOptions.onProgress}. */ @@ -86,6 +109,22 @@ export interface ImportProtocolLike { * rows. */ insertManyData?(args: { object: string; records: any[]; context?: any; environmentId?: string }): Promise<{ outcomes: Array<{ ok: boolean; record?: any; error?: unknown }> }>; + /** + * Validate-only (#6037 — #4633 ruling D). The write path's verdict on a + * candidate row, with nothing persisted. The dry run routes through THIS + * rather than re-deriving a verdict of its own. + * + * Optional for the same reason it is optional on `DataProtocol`: it is + * additive to a shipped contract. A protocol that omits it is not handed a + * substitute — the runner does not fabricate a verdict from a copy of the + * engine's rules, which is the defect this replaced. That is a capability + * gate, not leniency, and it is the honest answer for a protocol whose write + * is not the engine's write at all: plugin-auth's identity import creates + * users through better-auth, so an engine-derived preview would report + * findings ITS write never produces — a false alarm dressed as coverage. + * Such a dry run reports coercion + create/update/skip resolution only. + */ + validateData?(args: ValidateDataRequest & { context?: any; environmentId?: string }): Promise; } export interface RunImportOptions { @@ -138,10 +177,12 @@ export interface RunImportOptions { */ captureUndo?: boolean; /** - * `II18nService.t`-compatible lookup so this runner's own messages (the - * required-field pre-check, cell coercion) resolve a deployment's - * `validation.field.*` overrides — the same hook the engine gets (#3957). The - * locale itself rides `context.locale`. + * `II18nService.t`-compatible lookup so this runner's own messages (cell + * coercion) resolve a deployment's `validation.field.*` overrides — the same + * hook the engine gets (#3957). The locale itself rides `context.locale`. + * Validation verdicts are rendered by the engine, on both halves, so they + * need no hook here: the dry run's sentences come back already rendered + * from `validateData` and the write's from `ValidationError`. */ translate?: ValidationMessageTranslator; } @@ -203,10 +244,35 @@ export function sanitizeRowError(raw: unknown): string { return msg.slice(0, 300); } +/** + * A row report built from a write failure. + * + * When the failure is the engine's `ValidationError` it carries `fields[]` — + * the same `{ field, code, message }` triple `validateData` reports — and the + * row report is built from it rather than from the wrapper. Two reasons, and + * the second is the load-bearing one (#4633): + * + * 1. It names the offending COLUMN, which is what an import UI highlights. A + * bare `VALIDATION_FAILED` with no `field` made the caller re-read the + * sentence to find out which cell to fix. + * 2. It is the same shape the dry run now reports, so the two halves agree + * on `field` and `code`, not merely on "this row failed". Before, a + * `min: 0` violation was `min_value` on the dry run and `VALIDATION_FAILED` + * on the write — an agreement gap hidden inside a report that looked right. + * + * `code` therefore speaks one vocabulary across the whole row report: the + * field-level catalog (ADR-0114) that `coerceRow`'s cell failures already use. + */ function toFailedResult(rowNo: number, err: unknown): ImportRowResult { - const code = (err as any)?.code ?? 'IMPORT_ROW_FAILED'; - const message = sanitizeRowError((err as any)?.message); - return { row: rowNo, ok: false, action: 'failed', error: message, code }; + const e = err as { code?: unknown; message?: unknown; fields?: unknown } | null | undefined; + const fields = Array.isArray(e?.fields) ? (e.fields as Array<{ field?: unknown; code?: unknown }>) : []; + const first = fields[0]; + const code = first?.code ?? e?.code ?? 'IMPORT_ROW_FAILED'; + const message = sanitizeRowError(e?.message); + return { + row: rowNo, ok: false, action: 'failed', error: message, code: String(code), + ...(first?.field != null && first.field !== '' ? { field: String(first.field) } : {}), + }; } /** Upper bound on rows in one createManyData batch (framework#2678 suggests 100-500). */ @@ -341,6 +407,50 @@ export function runImport(opts: RunImportOptions): Promise { ...(treatAsHistorical ? { skipStateMachine: true, preserveAudit: true } : {}), }; + /** + * The dry run's verdict for one coerced row — asked of the engine, never + * derived here (#4633 ruling D). `null` means this protocol offers no + * validate-only operation, so no engine verdict exists to report; the caller + * falls back to the coercion + resolution verdict it already has rather than + * inventing one. + * + * Runs on EVERY dry run, whatever `runAutomations` says. The write's + * `beforeInsert` hooks fire before validation and could in principle derive + * a field this reports on — a boundary #6037 documents and deliberately does + * not close, because firing user-authored hooks (mail, outbound calls, + * writes to other objects) inside a preview is the retired `validateOnly` + * defect in a new spelling. Gating on `!runAutomations` instead would leave + * the DEFAULT dry run (`runAutomations` has defaulted to true since #2922) + * with no validation at all, which is the false all-clear this card exists + * to close. + */ + const previewVerdict = async ( + data: Record, + mode: 'insert' | 'update', + ): Promise[number] | null> => { + if (typeof p.validateData !== 'function') return null; + const res = await p.validateData({ + object: objectName, data, mode, + // The SAME context the write would carry, so a historical import's + // `skipStateMachine` and the caller's locale reach validation here + // exactly as they reach it on the write path. + context: writeCtx, + ...(environmentId ? { environmentId } : {}), + }); + return res?.results?.[0] ?? null; + }; + + /** + * Compose a row's `error` from the engine's findings the way + * `ValidationError` composes its own message — author-written rule text when + * there is one, `field (code)` otherwise, joined by `; `. Presentation only: + * the verdict and every sentence in it come from the engine. It is spelled + * out here because `validateData` returns structured findings rather than a + * rendered message, and the two halves must read identically. + */ + const composeValidationMessage = (issues: ValidateDataIssue[]): string => + issues.map((f) => (f.message?.trim() ? f.message : `${f.field} (${f.code})`)).join('; ') || 'Validation failed'; + // Sparse-indexed by row position `i` (not push-only): CREATE rows are // resolved immediately but their write is deferred to a later batch flush, // so their result would otherwise land out of order relative to @@ -592,54 +702,33 @@ export function runImport(opts: RunImportOptions): Promise { const willUpdate = existing && typeof existing === 'object'; const willCreate = !willUpdate && (writeMode === 'insert' || writeMode === 'upsert'); - // 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++; - // Same catalog the engine's own required-check renders from, so a - // pre-check verdict and a real rejection read identically (#3957). - results[i] = { - row: rowNo, ok: false, action: 'failed', field: requiredMiss, code: 'required', - error: renderValidationMessage( - { - messageKey: 'required', - label: metaMap.get(requiredMiss)?.label?.trim() || requiredMiss, - field: requiredMiss, - }, - { locale: context?.locale, translate: messageTranslator }, - ), - }; - } else if (!willUpdate && !willCreate) { + if (!willUpdate && !willCreate) { // update mode, no match → skip. skipped++; results[i] = { row: rowNo, ok: true, action: 'skipped', code: 'NO_MATCH' }; } else if (dryRun) { - // Field-constraint pre-check — DRY RUN ONLY (framework#3956). - // The write path is already covered: the engine's own - // `validateRecord` runs there (after beforeInsert hooks) and - // produces this exact message, so re-checking here would only add - // a pre-hook copy that could reject a row a hook would have made - // legal. The dry run has no such backstop — without this it - // reported `ok: true` for a row the very same endpoint then - // failed with `VALIDATION_FAILED`. - const violation = firstConstraintViolation(data, metaMap, { locale: context?.locale, translate: messageTranslator }); - if (violation) { + // Ask the engine for the verdict this row would get (#4633). + // The write path needs no counterpart: `validateRecord` runs + // there for real, after the hooks, and the row report is built + // from the very same findings by `toFailedResult`. + const verdict = await previewVerdict(data, willUpdate ? 'update' : 'insert'); + if (verdict && !verdict.valid) { errCount++; - results[i] = { row: rowNo, ok: false, action: 'failed', field: violation.field, code: violation.code, error: violation.message }; + const first = verdict.errors[0]; + results[i] = { + row: rowNo, ok: false, action: 'failed', + ...(first?.field ? { field: first.field } : {}), + code: first?.code ?? 'VALIDATION_FAILED', + error: composeValidationMessage(verdict.errors), + }; } else { okCount++; - if (willUpdate) { updated++; results[i] = { row: rowNo, ok: true, action: 'updated', id: String((existing as any).id ?? '') || undefined }; } - else { created++; results[i] = { row: rowNo, ok: true, action: 'created' }; } + // A warn-first deployment ADMITS some findings; the row is ok + // because the write would store it, and the complaint rides + // along so "accepted for now" is visible rather than silent. + const admitted = verdict?.warnings?.length ? { warnings: verdict.warnings } : {}; + if (willUpdate) { updated++; results[i] = { row: rowNo, ok: true, action: 'updated', id: String((existing as any).id ?? '') || undefined, ...admitted }; } + else { created++; results[i] = { row: rowNo, ok: true, action: 'created', ...admitted }; } } } else if (willUpdate) { const target = existing as Record;