Skip to content

fix(rest): import dry run must bound-check values, not just coerce them (#3956) - #3996

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-3956-review-261f60
Jul 30, 2026
Merged

fix(rest): import dry run must bound-check values, not just coerce them (#3956)#3996
os-zhuang merged 1 commit into
mainfrom
claude/issue-3956-review-261f60

Conversation

@baozhoutao

Copy link
Copy Markdown
Contributor

Closes#3956.

The problem

The same request body, with only dryRun flipped, produced two different verdicts. A number field declaring min: 0 received -500:

// dryRun: true
{"ok":1,"errors":0,"created":1,"results":[{"row":1,"ok":true,"action":"created"}]}
// → Console wizard renders 「全部 1 行均有效」// dryRun: false — same payload
{"ok":0,"errors":1,"created":0,
"results":[{"row":1,"ok":false,"error":"penalty_amount must be ≥ 0","code":"VALIDATION_FAILED"}]}

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

Root cause

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.
  • firstMissingRequiredField — required-presence only.

Everything the engine's validateRecord enforces lives past that branch, on the write path only.

The fix

  • ExportFieldMeta now carries min / max / minLength / maxLength. 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 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 afterbeforeInsert hooks; a pre-hook copy there could reject a row a hook would have made legal. The dry run has no such backstop.

Covers all three consumers of the shared runner: the synchronous import route, the async import-job worker, and plugin-auth's user import.

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 in the spec 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.

Tests

  • 8 unit tests on firstConstraintViolation (bounds both ways, boundary values, absent values, system/readonly skip, unparseable numbers left to coerceRow, engine-list type applicability).
  • 3 real-engine integration tests on the actual POST /data/:object/import route, using the issue's exact repro (penalty_amount min: 0, value -500): dry run and real write now agree, field/code/message included; maxLength behaves the same; update-mode rows are bound-checked too.
  • Verified the tests catch the regression: with the fix stubbed out, all 3 integration tests fail; restored, they pass.
  • packages/rest full suite: 447 passed. The 4 failures in rest.test.ts / rest-env-resolution.test.ts are pre-existing in this worktree (missing looksLikeInternalErrorLeak / ISecurityService exports from stale sibling builds) and reproduce identically with these changes stashed.
  • plugin-auth user-import suite (shares the runner): 26 passed.

Pure bug fix — no changeset per AGENTS.md.

🤖 Generated with Claude Code

@vercel

vercelBot commented Jul 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredJul 30, 2026 2:06am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/rest.

7 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/api/error-handling-server.mdx(via @objectstack/rest)
  • content/docs/api/index.mdx(via @objectstack/rest)
  • content/docs/plugins/index.mdx(via @objectstack/rest)
  • content/docs/plugins/packages.mdx(via @objectstack/rest)
  • content/docs/protocol/kernel/i18n-standard.mdx(via packages/rest)
  • content/docs/releases/implementation-status.mdx(via @objectstack/rest)
  • content/docs/releases/v12.mdx(via @objectstack/rest)

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@baozhoutaobaozhoutao added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Jul 30, 2026
@os-zhuang
os-zhuang merged commit fb7de07 into mainJul 30, 2026
19 of 20 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-3956-review-261f60 branch July 30, 2026 04:11
…em (#3956)
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 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Import dry-run skips field-level validation: dryRun:true reports ok:1 for a row the same endpoint then rejects

2 participants

@baozhoutao@os-zhuang