[wrangler] Recognise compound statement markers without surrounding whitespace - #15226
[wrangler] Recognise compound statement markers without surrounding whitespace#15226MatheusMartinho wants to merge 5 commits into
Conversation
…hitespace The D1 SQL splitter only treated BEGIN, CASE and END as compound statement markers when they were surrounded by whitespace. SQLite also accepts them delimited by punctuation, so a trigger body ending in `INSERT ...;END;` never terminated and every following statement was swallowed into it and sent as one statement, and a trigger declared with `WHEN (1=1)BEGIN` was split in the middle of its body. Match the markers when they are delimited by any non-identifier character, keeping identifiers that merely end in a keyword, such as a `weekend` table, from matching.
🦋 Changeset detectedLatest commit: 0fde4b0 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
|
![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.
@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: |
…racter Relaxing only the character before the marker made a parenthesised CASE expression, such as SUM(CASE WHEN a THEN 1 ELSE 0 END), open a compound statement that END) never closed, so the rest of the file was sent as a single statement. Accept any non-identifier character after END too, and cover the case in the tests.
| const COMPOUND_STATEMENT_START = /(?<![A-Za-z0-9_$])(BEGIN|CASE)\s$/i; | ||
| const COMPOUND_STATEMENT_END = /(?<![A-Za-z0-9_$])END[^A-Za-z0-9_$]$/i; |
There was a problem hiding this comment.
🟡 Trigger bodies are split apart when they mention a bracket-quoted or accented name ending in "end"
A trigger body is treated as finished (COMPOUND_STATEMENT_END at packages/wrangler/src/d1/splitter.ts:257) as soon as it contains a name such as [end] or néend, so the trigger is chopped into several broken statements and sent to D1 that way.
Impact: SQL files whose triggers reference such column/table names are executed as invalid fragments instead of one trigger, causing errors or a partially applied migration.
Why the new delimiter class matches inside identifiers
The new end marker regex is /(?<![A-Za-z0-9_$])END[^A-Za-z0-9_$]$/i. The character class is ASCII-only and does not account for identifier quoting:
néend— the character beforeendisé, which is not in[A-Za-z0-9_$], so the lookbehind passes and the following;/space satisfies the trailing class. NoteisDollarQuoteIdentifier(packages/wrangler/src/d1/splitter.ts:239-248) deliberately treats accented letters as identifier characters, so the two helpers disagree.[end]—[and]are both outside the class, so the marker matches. Bracket quoting is recognised elsewhere in this module (packages/wrangler/src/d1/splitter.ts:114-118), butsplitSqlIntoStatements(packages/wrangler/src/d1/splitter.ts:146-194) does not consume[...]in bulk the way it does',"and`.
Verified against both regex versions: with the old \sEND[;\s]$ the trigger below stays intact; with the new regex it splits into CREATE TRIGGER ... néend, UPDATE z SET c = 2, END, SELECT 1:
CREATE TRIGGER t AFTER INSERT ON x
BEGIN
UPDATE y SET a = 1 WHERE b = néend;
UPDATE z SET c = 2;
END;
SELECT 1;
Prompt for agents
The new compound-statement marker regexes in packages/wrangler/src/d1/splitter.ts use an ASCII-only identifier class ([A-Za-z0-9_$]) for the negative lookbehind and the trailing delimiter. Two false positives result: (1) identifiers containing non-ASCII letters immediately before the keyword, e.g. `néend`, match because `é` is not in the class — note isDollarQuoteIdentifier in the same file intentionally treats diacritical letters as identifier characters, so the two helpers are inconsistent; (2) bracket-quoted identifiers such as `[end]` match because `[` and `]` are outside the class, and unlike '"` quoting, splitSqlIntoStatements does not consume `[...]` in bulk (normalizeSqlLineEndings in the same file does treat `[` as a quote opener). Consider using Unicode-aware classes (\p{L}\p{N}_$ with the u flag) for both regexes, and handling `[` ... `]` in the switch in splitSqlIntoStatements the same way as the other quote characters so bracket-quoted identifiers are skipped.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This one reproduces, but it predates this PR — the behaviour is identical before and after my change. For
CREATE TRIGGER t AFTER INSERT ON items
BEGIN
UPDATE x SET [end] = 1;
UPDATE y SET z = 2;
END;
SELECT 1;both main and this branch return the same four fragments (...SET [end] = 1, UPDATE y SET z = 2, END, SELECT 1).
The root cause is separate from the delimiter matching: splitSqlIntoStatements() treats ', " and backticks as quotes but not SQLite's bracket-quoted identifiers, so [end] is scanned as bare SQL. Interestingly normalizeSqlLineEndings() in the same file does handle [ … ], so the two functions disagree. Happy to open a separate issue for that (and to take it in a follow-up PR) — it needs its own fix in the scanner rather than in these regexes, so I'd rather not widen this PR to cover it.
There was a problem hiding this comment.
Correcting my reply above — I got this wrong, and both halves of this finding were valid regressions introduced by this branch.
The four-fragment split I attributed to main was actually measured on this branch. On main, the whitespace-delimited \sEND[;\s]$ never fires inside [end] (no whitespace on either side of the keyword), so that example splits correctly into two statements — re-verified end-to-end on released wrangler 4.123.0, where the trigger executes fine via d1 execute --local --file. The auto-triage bot on #15228 flagged exactly this discrepancy, and it was right.
Fixed in two commits:
- de80155 —
splitSqlIntoStatements()now consumes[…]like the other quote styles, so bracket-quoted identifiers are opaque to both the;splitting and the marker regexes. This also fixes the genuinely pre-existing half of [wrangler] D1 SQL splitter does not treat bracket-quoted identifiers as quoted #15228: a;inside a bracket-quoted identifier (CREATE TABLE metrics ([value;unit] TEXT);) mis-split onmaintoo. - d543f3a — the marker regexes now use Unicode identifier classes (
\p{L}\p{N}_$with theuflag), sonéendno longer matches, consistent with howisDollarQuoteIdentifier()treats letters with diacritics.
Regression tests added for the trigger-[end], [value;unit] and néend cases — all three fail against the previous state of this branch and pass now.
| * `INSERT ...;END;`. The lookbehind keeps identifiers that merely end in the | ||
| * keyword, such as a `weekend` column, from matching. | ||
| */ | ||
| const COMPOUND_STATEMENT_START = /(?<![A-Za-z0-9_$])(BEGIN|CASE)\s$/i; |
There was a problem hiding this comment.
🟡 A SQL file that opens with a transaction keyword is now sent to D1 as a single unsplit blob
A file whose very first word opens a transaction is now treated as the start of a trigger body (COMPOUND_STATEMENT_START at packages/wrangler/src/d1/splitter.ts:256), so every statement in the file is merged into one and sent to D1 unsplit.
Impact: Running such a SQL file with wrangler d1 execute --file or d1 migrations apply fails or behaves incorrectly instead of executing each statement.
Why removing the leading \s changes first-statement behaviour
The old regex /\s(BEGIN|CASE)\s$/i required whitespace before the keyword, so a BEGIN at the very beginning of the accumulated buffer (i.e. at the start of the file, before any ; has reset str) never started a compound statement. The new lookbehind also passes on an empty prefix, so begin transaction; as the first line pushes a frame onto compoundStatementStack and no END ever pops it — all subsequent semicolons are swallowed (packages/wrangler/src/d1/splitter.ts:183-190).
trimSqlQuery only strips the exact uppercase literal BEGIN TRANSACTION; (packages/wrangler/src/d1/trimmer.ts:17-19), so lowercase begin transaction;, BEGIN TRANSACTION ;, or BEGIN IMMEDIATE; are not removed.
Verified: with the old regexes, begin transaction;\nINSERT INTO a VALUES (1);\nINSERT INTO b VALUES (2);\ncommit; splits into four statements; with the new one it returns the whole file as a single statement.
Prompt for agents
COMPOUND_STATEMENT_START in packages/wrangler/src/d1/splitter.ts previously required whitespace before BEGIN/CASE, which incidentally prevented a BEGIN at the very start of the input (before any semicolon resets the accumulator) from opening a compound statement. With the negative lookbehind, an empty prefix now matches, so a file that begins with a transaction opener that trimSqlQuery did not strip (it only removes the exact literal 'BEGIN TRANSACTION;', see packages/wrangler/src/d1/trimmer.ts) — e.g. lowercase 'begin transaction;' or 'BEGIN IMMEDIATE;' — pushes a compound frame that is never popped, so the whole file collapses into one statement. Consider either making transaction openers recognisable (e.g. broaden trimSqlQuery/mayContainTransaction to be case-insensitive and tolerate the DEFERRED/IMMEDIATE/EXCLUSIVE forms) or making the splitter not treat a BEGIN that starts a transaction as a compound-statement start. Add a regression test covering a file starting with a lowercase transaction opener.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I don't think this one holds. splitSqlQuery() calls trimSqlQuery() first, which strips a leading BEGIN TRANSACTION; and trailing COMMIT; by design, since D1 already wraps the file in a transaction (src/d1/trimmer.ts). So for
BEGIN TRANSACTION;
INSERT INTO t VALUES (1);
COMMIT;the splitter never sees the transaction keywords, and the output is the single remaining INSERT — which is the intended behaviour, not an unsplit blob. I checked the output on this branch and on main and it is byte-for-byte identical: ["\\nINSERT INTO t VALUES (1);\\n"]. The existing should trim a regular old sqlite dump test covers this path and still passes.
|
The three red
This diff only changes two regexes in the D1 SQL splitter, and the tests that cover it pass on all three platforms — on the Windows run, Worth noting when comparing against other PRs that appear fully green: they do not run the same set. Because this PR touches Happy to rebase or push an empty commit for a re-run if that helps. |
The statement scanner handled ', " and backtick quoting but not SQLite's bracket-quoted identifiers, while normalizeSqlLineEndings() in the same file already did. A ; inside such an identifier, as in CREATE TABLE metrics ([value;unit] TEXT), was treated as a statement boundary and broken fragments were sent to D1 (pre-existing, cloudflare#15228). With the punctuation-delimited markers from this branch the gap also made a [end] column inside a trigger body pop the compound statement early and chop the trigger apart. Consume [ ... ] like the other quote styles, and cover both cases in the tests.
The ASCII-only class treated a letter with a diacritical mark as a
delimiter, so a column such as néend matched the END marker and closed
a trigger body early. Match identifier characters with \p{L}\p{N}
instead, consistent with how isDollarQuoteIdentifier() in the same file
treats letters with diacritics.
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
2 participants
Fixes #15228. Related to #15093 (see note below).
splitSqlQuery()splits a SQL file into statements forwrangler d1 execute --fileandwrangler d1 migrations apply. It tracks compound statements so that semicolons inside a trigger body do not split it, but it only recognised theBEGIN/CASE/ENDmarkers when they were surrounded by whitespace:SQLite also accepts those markers delimited by punctuation, and then the split goes wrong in two ways:
INSERT INTO audit VALUES (new.id);END;— theENDis preceded by;, so the compound statement never terminates and every following statement is swallowed into the trigger and sent to D1 as one statement. The visible symptom is silently missing result sets, not an error.CREATE TRIGGER ... WHEN (1=1)BEGIN— theBEGINis preceded by), so the compound statement never starts and the trigger body is split at its first internal semicolon.Both markers are now matched when delimited by any non-identifier character, using a negative lookbehind so identifiers that merely end in a keyword — a
weekendtable, a value of'weekend'— still do not match.Review follow-up: bracket-quoted and non-ASCII identifiers
The Devin review correctly caught that widening the delimiter class introduced two false positives, which are now fixed:
[and]are outside the identifier class, so a trigger body containing a name like[end]popped the compound statement early and the trigger was chopped apart.splitSqlIntoStatements()now consumes[…]like the other quote styles. This also fixes [wrangler] D1 SQL splitter does not treat bracket-quoted identifiers as quoted #15228, which is pre-existing: a;inside a bracket-quoted identifier (CREATE TABLE metrics ([value;unit] TEXT);) splits the statement even onmain—normalizeSqlLineEndings()in the same file already treats[…]as a quote pair, so the two scanners disagreed about the same syntax. (My earlier review reply claiming the[end]case reproduced identically onmainwas wrong — onmainthe whitespace-only markers coincidentally never fire inside[end]; I've corrected that in the thread.)éas a delimiter, so anéendcolumn matched theENDmarker. Both regexes now use Unicode identifier classes (\p{L}\p{N}_$with theuflag), consistent with howisDollarQuoteIdentifier()in the same file treats letters with diacritical marks.On #15093
That issue reports the same class of bug for a lowercase
end;. Case handling is already fixed onmain(both regexes carry/i), and I confirmed lowercase input now splits identically to uppercase, so the originally reported reproduction passes as-is. The variants above still reproduced, which is what this PR fixes.Testing
Seven regression tests in
splitter.test.ts, red/green verified:;END;,(1=1)BEGINandSUM(CASE ... END)fail againstmainand pass with this change; the false-positive guard (weekend) passes both before and after.[value;unit]fails onmainand on this branch before the scanner change (it is the [wrangler] D1 SQL splitter does not treat bracket-quoted identifiers as quoted #15228 bug), and passes now.[end]andnéendcases pass onmain, failed on this branch before the follow-up commits (the regressions Devin found), and pass again now.Full file passing;
check:typeandcheck:formatare clean. I also verified end-to-end withwrangler d1 execute --local --filethat the[value;unit]schema fails on released wrangler 4.123.0 and that the trigger examples execute correctly with this branch.Note
This contribution was written with an AI agent: Claude Code (Claude Fable 5), directed and reviewed by @MatheusMartinho, who takes responsibility for the change.