fix(rest,objectql): the import dry run asks the engine for its verdict instead of predicting it (#4633)

.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.

packages/objectql/src/engine.ts

@@ -5427,9 +5427,31 @@ export class ObjectQL implements IObjectQLEngine {
   ): Promise<ValidateDataResponse> {
     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<string, unknown>[] = mode === 'insert'
+      ? rawRows.map((row) => this.initializeSummaryFields(
+          object,
+          this.applyFieldDefaults(object, row, options?.context, nowSnapshot),
+        ) as Record<string, unknown>)
+      : 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);

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<string, unknown> = { 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

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. */

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<string, Partial<ExportFieldMeta>>): Map<string, ExportFieldMeta> => {
-    const m = new Map<string, ExportFieldMeta>();
-    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