[wrangler] Fix D1 SQL splitter under-splitting migrations with a CASE...END followed by a comma - #15234
[wrangler] Fix D1 SQL splitter under-splitting migrations with a CASE...END followed by a comma#15234mittalpk wants to merge 7 commits into
Conversation
…...END followed by a comma isCompoundStatementEnd() only recognized END as closing a compound statement when the next character was ';' or whitespace. A CASE used as a value expression (SET x = CASE ... END, y = 1) is legitimately followed directly by a comma, so that END went undetected, leaving the splitter's nesting tracker permanently one level too deep for the rest of the file -- every remaining statement got silently merged into one oversized blob. Broadened the check to any non-identifier character, matching what already worked for ';' and whitespace. Fixes cloudflare#15162.
🦋 Changeset detectedLatest commit: 8b91b2c 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
@cloudflare/containers-shared
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: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ed CASE isCompoundStatementEnd() was broadened to accept any non-identifier character after END (to fix cloudflare#15162's comma case), but isCompoundStatementStart() still required whitespace immediately before CASE/BEGIN. A parenthesised CASE used as a value expression, e.g. (CASE ... END), is preceded by '(' with no space -- its start was never detected, but its END now was. That let an inner CASE's END) prematurely close an unrelated, already-open compound statement (e.g. an enclosing trigger's BEGIN ... END), corrupting the split. Broadened isCompoundStatementStart() the same way, so detection is symmetric on both sides. Thanks to Devin Review for catching this.
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
NuroDev
left a comment
NuroDev
left a comment
There was a problem hiding this comment.
Just one issue that needs fixing then good to merge this.
…e-end-comma # Conflicts: # packages/wrangler/src/__tests__/d1/splitter.test.ts
isCompoundStatementStart/End used [^A-Za-z0-9_] as the identifier boundary, but SQLite's own unquoted-identifier syntax allows $ and any code point at or above U+0080. An identifier like foo$CASE or ENDα was misread as the BEGIN/CASE/END keyword, corrupting the compound-statement nesting stack for the rest of the file. Extract the boundary into a single SQL_IDENTIFIER_CHAR class shared by both functions so they can't drift out of sync again -- that exact drift (one regex broadened, the other left narrow) is what caused the CASE/END regression this PR already fixes. Precompile both regexes at module scope: isCompoundStatementStart runs on every character of the input in the main split loop, so constructing a new RegExp inside the function would recompile it per character. Adds 2 regression tests. Also resolves the merge conflict with cloudflare#15557's consumeWhile windowing, which touched the same test file.
| const COMPOUND_STATEMENT_START_RE = new RegExp( | ||
| `[^${SQL_IDENTIFIER_CHAR}](BEGIN|CASE)\\s$`, | ||
| "i" | ||
| ); | ||
| const COMPOUND_STATEMENT_END_RE = new RegExp( | ||
| `\\sEND[^${SQL_IDENTIFIER_CHAR}]$`, | ||
| "i" | ||
| ); |
There was a problem hiding this comment.
🔍 Module constants conflict with review guidance
COMPOUND_STATEMENT_START_RE and COMPOUND_STATEMENT_END_RE are module-level state. REVIEW.md rejects global variables, so maintainers need to confirm whether immutable regex constants are exempt.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
These are module-private immutable constants, not global mutable state. Wrangler already uses module-level regex constants extensively, and keeping them here avoids recreating the regexes in the per-character path, so I think this is fine to ignore.
| const COMPOUND_STATEMENT_START_RE = new RegExp( | ||
| `[^${SQL_IDENTIFIER_CHAR}](BEGIN|CASE)\\s$`, | ||
| "i" | ||
| ); | ||
| const COMPOUND_STATEMENT_END_RE = new RegExp( | ||
| `\\sEND[^${SQL_IDENTIFIER_CHAR}]$`, | ||
| "i" | ||
| ); |
There was a problem hiding this comment.
These are module-private immutable constants, not global mutable state. Wrangler already uses module-level regex constants extensively, and keeping them here avoids recreating the regexes in the per-character path, so I think this is fine to ignore.
…EGIN/CASE/END detection symmetrically Three related gaps in the boundary logic, all found by review (Devin + NuroDev) and independently verified before fixing: 1. `new.begin` (a column reference qualified by `.`) was misread as the BEGIN keyword, since `.` counted as a valid boundary. SQLite's `.` qualifier always introduces an identifier on its right, never a real keyword. Excludes `.` from the boundary class via a new SQL_STATEMENT_BOUNDARY, used only on the leading side. 2. `CASE(expr)` (no space after CASE) was never recognized as a start, since the trailing side still required literal whitespace. Broadened to any non-identifier character, mirroring the trailing side of END. 3. A compact `THEN 1 ELSE(0)END` (no space before END) was never recognized as an end. This isn't just "unsplit" -- verified live that it silently pops the *wrong* stack frame (a later, unrelated END instead), merging multiple statements including a trailing CREATE INDEX into one. Adds 3 regression tests, each confirmed failing against unpatched code via git stash before the fix. Full D1 suite (209 tests) passes, oxlint and tsc clean.
| const COMPOUND_STATEMENT_START_RE = new RegExp( | ||
| `${SQL_STATEMENT_BOUNDARY}(BEGIN|CASE)[^${SQL_IDENTIFIER_CHAR}]$`, | ||
| "i" | ||
| ); | ||
| const COMPOUND_STATEMENT_END_RE = new RegExp( | ||
| `${SQL_STATEMENT_BOUNDARY}END[^${SQL_IDENTIFIER_CHAR}]$`, | ||
| "i" |
There was a problem hiding this comment.
🟡 Qualified keyword names corrupt nesting
A qualified name like case.value makes COMPOUND_STATEMENT_START_RE open a false frame. A name like end.value closes the trigger's frame, splitting its body at the next semicolon.
Learn more
SQLite permits keywords as identifiers where the grammar expects a name. A qualifier dot therefore can follow an identifier named begin, case, or end, just as it can precede one. The trailing boundary currently includes dots, so the stack changes while scanning the qualified name rather than a compound marker.
Example: Inside CREATE TRIGGER t ... BEGIN SELECT end.value; UPDATE items SET x = 1; END;, scanning end. removes the trigger's BEGIN frame. The semicolon after value then splits the trigger into invalid statements.
Recommended fix: Use SQL_STATEMENT_BOUNDARY for both sides of each keyword. This preserves punctuation forms such as CASE( and END, while excluding qualifier dots symmetrically.
| const COMPOUND_STATEMENT_START_RE = new RegExp( | |
| `${SQL_STATEMENT_BOUNDARY}(BEGIN|CASE)[^${SQL_IDENTIFIER_CHAR}]$`, | |
| "i" | |
| ); | |
| const COMPOUND_STATEMENT_END_RE = new RegExp( | |
| `${SQL_STATEMENT_BOUNDARY}END[^${SQL_IDENTIFIER_CHAR}]$`, | |
| "i" | |
| const COMPOUND_STATEMENT_START_RE = new RegExp( | |
| `${SQL_STATEMENT_BOUNDARY}(BEGIN|CASE)${SQL_STATEMENT_BOUNDARY}$`, | |
| "i" | |
| ); | |
| const COMPOUND_STATEMENT_END_RE = new RegExp( | |
| `${SQL_STATEMENT_BOUNDARY}END${SQL_STATEMENT_BOUNDARY}$`, | |
| "i" | |
| ); |
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
Fixes #15162
splitSqlIntoStatements()tracks nestedBEGIN/CASEblocks so it doesn't split a semicolon that's inside a trigger body or aCASEexpression. It detected the end of a block by checking whetherENDwas followed by;or whitespace — but aCASEused as a value expression (e.g.SET x = CASE ... END, y = 1, which is exactly what the reporter's real migration file does) is legitimately followed directly by a comma. When that happened, the block'sENDwent undetected, leaving the splitter's internal nesting tracker permanently one level too deep for the rest of the file — every remaining statement got silently merged into one giant blob instead of being split correctly.Reproduced with the reporter's linked repro repo: the 5th migration file (667 lines, ~193 real statements) was being collapsed into 2. Passing that single oversized "statement" through D1's local
batch()execution is what produced the reporteddatabase table is locked: SQLITE_LOCKEDerror — confirmed by rebuilding wrangler both with and without the fix and running the exact repro both ways, live.Fix: broaden the single trailing-character check from
[;\s]to any non-identifier character, soEND,andEND)are recognized the same wayEND;andENDalready were.