From 9c6691700b9af2d11ea6deca25f0dcde889d9bfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Wed, 29 Jul 2026 22:04:45 -0700 Subject: [PATCH] fix(rest): import dry run must bound-check values, not just coerce them (#3956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same request body, with only `dryRun` flipped, produced two different verdicts. A `number` field declaring `min: 0` received `-500`: the dry run answered `ok:1, errors:0, created:1` — the Console wizard renders that as 「全部 1 行均有效」 — and the real write then answered `ok:0, errors:1, penalty_amount must be ≥ 0`, dropping the row. A pre-check that cannot predict the write is worse than no pre-check: it turns "your file has a problem" into a false all-clear, and reviewers can't use it as evidence. The dry-run branch in `runImport` returned before any field-constraint check ran. Only two gates stood in front of it, and neither looks at a declared bound: - `coerceRow` — pure value conversion (is this cell a number at all, does this select option exist, does this lookup resolve). `-500` is a perfectly good number, so it sailed through. - `firstMissingRequiredField` — required-presence only. Everything the engine's `validateRecord` enforces (numeric range, string length, formats) lives past that branch, on the write path only. This closes the range/length half: - `ExportFieldMeta` now carries `min` / `max` / `minLength` / `maxLength`. The projection built by `buildFieldMetaMap` was dropping them, so the runner could not have checked a bound even if it wanted to. - `firstConstraintViolation` mirrors `validateRecord`'s numeric-range and string-length rules — same type applicability, same comparison, same `code` and `message` text — so both paths now report a violation identically. - The runner consults it on the DRY RUN ONLY. The write path already has the engine's own validation, which runs AFTER beforeInsert hooks; a pre-hook copy there could reject a row a hook would have made legal. The dry run has no such backstop. Deliberately not a full mirror. 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 (a `validateOnly` write path, which `BatchOptions.validateOnly` already declares but nothing implements) rather than growing this copy. The bounded-type lists are the engine's own, not the spec's wider `NUMERIC_VALUE_TYPES` / `STRING_VALUE_TYPES`: `progress` and `summary` are numeric per the spec but unchecked by the engine, and using the wider set would trade the false all-clear for a false alarm. Covers all three consumers of the shared runner: the synchronous import route, the async import-job worker, and plugin-auth's user import. Co-Authored-By: Claude Opus 5 --- packages/rest/src/export-format.ts | 16 ++++ packages/rest/src/import-coerce.test.ts | 72 ++++++++++++++++++ packages/rest/src/import-coerce.ts | 78 ++++++++++++++++++++ packages/rest/src/import-integration.test.ts | 49 ++++++++++++ packages/rest/src/import-runner.ts | 22 +++++- 5 files changed, 233 insertions(+), 4 deletions(-) diff --git a/packages/rest/src/export-format.ts b/packages/rest/src/export-format.ts index b60129d7ea..a2ef4545da 100644 --- a/packages/rest/src/export-format.ts +++ b/packages/rest/src/export-format.ts @@ -37,6 +37,18 @@ 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. */ + max?: number; + /** Minimum character count for string fields. */ + minLength?: number; + /** Maximum character count for string fields. */ + maxLength?: number; } /** @@ -137,6 +149,10 @@ export function buildFieldMetaMap(schema: unknown): Map // ⇒ no default): any non-null default — literal, expression object, or the // `current_user` token — counts as satisfying a required field. hasDefault: f.defaultValue != null, + min: typeof f.min === 'number' ? f.min : undefined, + max: typeof f.max === 'number' ? f.max : undefined, + minLength: typeof f.minLength === 'number' ? f.minLength : undefined, + maxLength: typeof f.maxLength === 'number' ? f.maxLength : undefined, }); } return map; diff --git a/packages/rest/src/import-coerce.test.ts b/packages/rest/src/import-coerce.test.ts index 740af7c283..1de9b2010d 100644 --- a/packages/rest/src/import-coerce.test.ts +++ b/packages/rest/src/import-coerce.test.ts @@ -14,6 +14,7 @@ import { matchOption, splitMulti, coerceRow, + firstConstraintViolation, } from './import-coerce'; import type { ExportFieldMeta } from './export-format'; @@ -272,3 +273,74 @@ describe('coerceRow', () => { expect(data).toEqual({ mystery: 'raw' }); }); }); + +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(); + }); +}); diff --git a/packages/rest/src/import-coerce.ts b/packages/rest/src/import-coerce.ts index f12204f4ba..cecc83c972 100644 --- a/packages/rest/src/import-coerce.ts +++ b/packages/rest/src/import-coerce.ts @@ -425,6 +425,84 @@ export function firstMissingRequiredField( 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, +): FieldCoerceError | null { + 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 { field: meta.name, code: 'min_value', message: `${meta.name} must be ≥ ${meta.min}` }; + } + if (meta.max !== undefined && n > meta.max) { + return { field: meta.name, code: 'max_value', message: `${meta.name} must be ≤ ${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 { field: meta.name, code: 'max_length', message: `${meta.name} must be ≤ ${meta.maxLength} characters (got ${s.length})` }; + } + if (meta.minLength !== undefined && s.length < meta.minLength) { + return { field: meta.name, code: 'min_length', message: `${meta.name} must be ≥ ${meta.minLength} characters (got ${s.length})` }; + } + } + } + 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 2281804a88..c1667b0144 100644 --- a/packages/rest/src/import-integration.test.ts +++ b/packages/rest/src/import-integration.test.ts @@ -125,6 +125,9 @@ const MEMBER = { name: 'tier', type: 'select' as const, label: 'Tier', required: true, defaultValue: 'standard', options: [{ label: 'Standard', value: 'standard' }, { label: 'Gold', value: 'gold' }], }, + // framework#3956 — bounded fields. The dry run used to ignore both. + penalty_amount: { name: 'penalty_amount', type: 'number' as const, label: '处罚金额', min: 0, max: 9999999.99 }, + nickname: { name: 'nickname', type: 'text' as const, label: 'Nickname', maxLength: 5 }, }, }; @@ -435,6 +438,52 @@ describe('import route — required-field dry-run fidelity', () => { for (const r of res._json.results) expect(r).toMatchObject({ field: 'member_name', code: 'required' }); }); + // framework#3956 — the dry run reported `ok:1, created:1` for a row the very + // same endpoint then rejected with `penalty_amount must be ≥ 0`, because the + // dry-run branch returned before any field-constraint check ran. Unlike the + // required pre-check above, this one is NOT gated on `runAutomations`: it runs + // on the dry run only, where the engine's own validation is never reached. + it('dry run fails a row that violates min/max — same verdict, same message as the real write', async () => { + const rows = [{ id: 'p1', member_name: 'Eve', status: 'active', penalty_amount: -500 }]; + + const dry = await imp({ format: 'json', dryRun: true, rows }); + expect(dry._json).toMatchObject({ dryRun: true, total: 1, ok: 0, errors: 1, created: 0 }); + expect(dry._json.results[0]).toMatchObject({ + row: 1, ok: false, action: 'failed', field: 'penalty_amount', + code: 'min_value', error: 'penalty_amount must be ≥ 0', + }); + + // The real write reaches the engine's validateRecord and says the same. + const real = await imp({ format: 'json', rows }); + expect(real._json).toMatchObject({ total: 1, ok: 0, errors: 1, created: 0 }); + expect(real._json.results[0].error).toContain('penalty_amount must be ≥ 0'); + expect(await engine.findOne('member', { where: { id: 'p1' } })).toBeNull(); + }); + + it('dry run fails an over-long string too (maxLength), and passes in-range rows', async () => { + const over = await imp({ format: 'json', dryRun: true, rows: [ + { id: 'p2', member_name: 'Fay', status: 'active', nickname: 'toolongname' }, + ] }); + expect(over._json).toMatchObject({ ok: 0, errors: 1 }); + expect(over._json.results[0]).toMatchObject({ + field: 'nickname', code: 'max_length', error: 'nickname must be ≤ 5 characters (got 11)', + }); + + // Boundary values are legal — the pre-check must not over-reject. + const ok = await imp({ format: 'json', dryRun: true, rows: [ + { id: 'p3', member_name: 'Gus', status: 'active', penalty_amount: 0, nickname: 'exact' }, + ] }); + expect(ok._json).toMatchObject({ ok: 1, errors: 0, created: 1 }); + }); + + it('bound-checks update rows as well as creates', async () => { + await engine.insert('member', { id: 'p4', member_name: 'Hana', status: 'active', penalty_amount: 10 }); + const res = await imp({ format: 'json', dryRun: true, writeMode: 'update', matchFields: ['id'], + rows: [{ id: 'p4', penalty_amount: -1 }] }); + expect(res._json).toMatchObject({ ok: 0, errors: 1, updated: 0 }); + expect(res._json.results[0]).toMatchObject({ field: 'penalty_amount', code: 'min_value' }); + }); + 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 diff --git a/packages/rest/src/import-runner.ts b/packages/rest/src/import-runner.ts index 9157fa73a3..4e105d3c23 100644 --- a/packages/rest/src/import-runner.ts +++ b/packages/rest/src/import-runner.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { randomUUID } from 'node:crypto'; -import { coerceRow, firstMissingRequiredField, type RefResolver, type RefMatch } from './import-coerce.js'; +import { coerceRow, firstMissingRequiredField, firstConstraintViolation, type RefResolver, type RefMatch } from './import-coerce.js'; import type { ExportFieldMeta } from './export-format.js'; import { bulkWrite, withTransientRetry, defaultIsTransientError, type BulkWriteRowResult } from '@objectstack/core'; @@ -601,9 +601,23 @@ export function runImport(opts: RunImportOptions): Promise { skipped++; results[i] = { row: rowNo, ok: true, action: 'skipped', code: 'NO_MATCH' }; } else if (dryRun) { - 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' }; } + // 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); + if (violation) { + errCount++; + results[i] = { row: rowNo, ok: false, action: 'failed', field: violation.field, code: violation.code, error: violation.message }; + } 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' }; } + } } else if (willUpdate) { const target = existing as Record; let res2: unknown;