-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[wrangler] Recognise compound statement markers without surrounding whitespace #15226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
50bc2ee
898dc6e
de80155
d543f3a
0fde4b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| "wrangler": patch | ||
| --- | ||
|
|
||
| Recognise compound statement markers that are not padded with whitespace | ||
|
|
||
| `wrangler d1 execute --file` and `wrangler d1 migrations apply` split a SQL file into statements before sending them to D1. The splitter only recognised `BEGIN`, `CASE` and `END` when they were surrounded by whitespace, so SQL that SQLite accepts, such as a trigger body ending in `INSERT ...;END;` or a trigger declared with `WHEN (1=1)BEGIN`, was split incorrectly: statements after the trigger were swallowed into it and silently sent as a single statement. | ||
|
|
||
| Markers are now matched when delimited by punctuation as well, while identifiers that merely end in a keyword, such as a `weekend` table, are still left alone. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -247,16 +247,25 @@ function isDollarQuoteIdentifier(str: string) { | |
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Compound statement markers only need to be delimited from surrounding | ||
| * identifiers, not padded with whitespace: SQLite accepts `WHEN (1=1)BEGIN` and | ||
| * `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; | ||
| const COMPOUND_STATEMENT_END = /(?<![A-Za-z0-9_$])END[^A-Za-z0-9_$]$/i; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Trigger bodies are split apart when they mention a bracket-quoted or accented name ending in "end" A trigger body is treated as finished ( 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 identifiersThe new end marker regex is
Verified against both regex versions: with the old Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 The root cause is separate from the delimiter matching:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Fixed in two commits:
Regression tests added for the trigger- |
||
|
|
||
| /** | ||
| * Returns true if the `str` ends with a compound statement `BEGIN` or `CASE` marker. | ||
| */ | ||
| function isCompoundStatementStart(str: string) { | ||
| return /\s(BEGIN|CASE)\s$/i.test(str); | ||
| return COMPOUND_STATEMENT_START.test(str); | ||
| } | ||
|
|
||
| /** | ||
| * Returns true if the `str` ends with a compound statement `END` marker. | ||
| */ | ||
| function isCompoundStatementEnd(str: string) { | ||
| return /\sEND[;\s]$/i.test(str); | ||
| return COMPOUND_STATEMENT_END.test(str); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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_STARTatpackages/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 --fileord1 migrations applyfails or behaves incorrectly instead of executing each statement.Why removing the leading \s changes first-statement behaviour
The old regex
/\s(BEGIN|CASE)\s$/irequired whitespace before the keyword, so aBEGINat the very beginning of the accumulated buffer (i.e. at the start of the file, before any;has resetstr) never started a compound statement. The new lookbehind also passes on an empty prefix, sobegin transaction;as the first line pushes a frame ontocompoundStatementStackand noENDever pops it — all subsequent semicolons are swallowed (packages/wrangler/src/d1/splitter.ts:183-190).trimSqlQueryonly strips the exact uppercase literalBEGIN TRANSACTION;(packages/wrangler/src/d1/trimmer.ts:17-19), so lowercasebegin transaction;,BEGIN TRANSACTION ;, orBEGIN 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
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this one holds.
splitSqlQuery()callstrimSqlQuery()first, which strips a leadingBEGIN TRANSACTION;and trailingCOMMIT;by design, since D1 already wraps the file in a transaction (src/d1/trimmer.ts). So forthe 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 onmainand it is byte-for-byte identical:["\\nINSERT INTO t VALUES (1);\\n"]. The existingshould trim a regular old sqlite dumptest covers this path and still passes.