[wrangler] fix: throw clear error when D1 migration execution returns null#13882
[wrangler] fix: throw clear error when D1 migration execution returns null#13882matingathani wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 0de5eee The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
workers-devprod
requested review from
a team and
NuroDev
and removed request for
a team
|
Codeowners approval required for this PR:
Show detailed file reviewers |
There was a problem hiding this comment.
Pull request overview
This PR updates Wrangler’s D1 migrations apply flow to surface a user-facing error when executeSql() returns null (cancellation), instead of silently returning and leaving migration state unclear.
Changes:
- Replace the previous silent
returnonnullSQL execution response with aUserErrorindicating the migration was cancelled. - Add a Changeset entry to publish the fix as a patch release.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/wrangler/src/d1/migrations/apply.ts |
Throws a UserError when a migration execution returns null to avoid silent exits. |
.changeset/fix-d1-migrations-null-response.md |
Records the behavior change as a patch Changeset for wrangler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
NuroDev
left a comment
NuroDev
left a comment
There was a problem hiding this comment.
Can we also add some tests for this to make sure this improved error handling actually works as expected.
|
Addressed NuroDev's review — refactored the |
|
CI failure (Tests, Windows, packages-and-tools) is the pre-existing miniflare browser plugin flake ( |
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
… null The apply loop had a TODO: return error comment for the case where executeSql returns null. Replace the silent return with a UserError that names the migration and explains it was cancelled, so users are never left without feedback.
…ix telemetry string
- Throw UserError directly inside try block when executeSql returns null, then rethrow if instanceof UserError in catch — removes the `cancelled` flag approach suggested by NuroDev - Add regression test: mocks executeSql to return null and asserts the clear UserError message is surfaced to the user
petebacondarwin
force-pushed
the
fix/d1-migrations-null-response
branch
from
014a270 to
0de5eee
Compare
wrangler d1 migrations applysilently exited whenexecuteSqlreturnednull, leaving the migration table in an inconsistent state with no user feedback. The source file even had a// TODO: return errorcomment marking this as a known gap.Fix: Replace the silent
returnwith aUserErrorthat names the migration and explains it was cancelled, so users always know what happened.executeSqlreturningnullfrom acommand-based invocation) is currently unreachable through the automated test infrastructure —executeSqlonly returnsnullwhen given afileinput and the user declines an interactive prompt, which cannot be triggered fromapply.ts'scommand-only call site. The fix is defensive programming that resolves the existingTODOand ensures correct behaviour if the call site ever changes.