[miniflare] fix: emit valid SQL literal for Infinity/-Infinity in D1 export - #15420
[miniflare] fix: emit valid SQL literal for Infinity/-Infinity in D1 export#15420manthaaaaan wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: 3d7b298 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
There was a problem hiding this comment.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
dario-piotrowicz
left a comment
There was a problem hiding this comment.
Thanks for the fix @manthaaaaan 😄
Looks good to me, beside some small suggestions 🙂
| const [dumpStatements] = result as [string[]]; | ||
| const dump = dumpStatements.join("\n"); | ||
|
|
||
| // The dump must not contain the raw JS token as a bare identifier |
There was a problem hiding this comment.
| // The dump must not contain the raw JS token as a bare identifier | |
| // The dump must not contain the raw JS Infinity/-Infinity tokens as bare identifiers |
| // exec(dump) must not throw — this is the core guarantee of the fix. | ||
| // (If the dump contained bare `Infinity`/`-Infinity` identifiers, SQLite | ||
| // would reject them at parse time and exec would throw.) |
There was a problem hiding this comment.
outside of the context of this PR the sentence "this is the code guarantee of the fix" doesn't have any meaning
I would simplify this comment:
| // exec(dump) must not throw — this is the core guarantee of the fix. | |
| // (If the dump contained bare `Infinity`/`-Infinity` identifiers, SQLite | |
| // would reject them at parse time and exec would throw.) | |
| // exec(dump) must not throw, If the dump contained the bare `Infinity`/`-Infinity` | |
| // identifiers, SQLite would reject them at parse time and exec would throw |
| // Assert the exported SQL actually contains the correctly-signed literal | ||
| // for each row — this is the real guarantee of the fix, checked before | ||
| // the D1 JS binding's Infinity->null normalisation can hide it. |
There was a problem hiding this comment.
as above, I would really avoid mentioning "the fix":
| // Assert the exported SQL actually contains the correctly-signed literal | |
| // for each row — this is the real guarantee of the fix, checked before | |
| // the D1 JS binding's Infinity->null normalisation can hide it. | |
| // Assert the exported SQL actually contains the correctly converted value for each row |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
Fixes #15378 (partially — the Infinity/-Infinity bare-identifier bug only. The int64 precision issue reported in the same issue is a separate, larger fix that needs its own discussion, since the value loses precision upstream before this code even runs.)
What this fixes
When a REAL column contained Infinity or -Infinity, dumpSql emitted the raw JavaScript string representation ("Infinity" / "-Infinity") directly into the generated SQL. SQLite doesn't recognize these as numeric literals — it parses them as bare identifiers — so re-importing D1's own export failed with: "no such column: Infinity at offset 45: SQLITE_ERROR"
The fix
In packages/miniflare/src/workers/d1/dumpSql.ts, non-finite numbers are now detected and emitted as 9e999 / -9e999 instead of their raw JS string form. This matches the approach SQLite's own .dump command uses — 9e999 is a valid numeric literal that overflows to Infinity on parse, so it round-trips correctly.
Testing
Added a new test, "dumpSql exports Infinity and -Infinity as valid, re-importable SQL literals", in packages/miniflare/test/plugins/d1/suite.ts. It:
Test passes locally (pnpm test suite.ts -t "Infinity"), and the full build passes with no new type errors.
A picture of a cute animal (not mandatory, but encouraged)
