Close a compound SQL statement when its END is lowercase - #15046
Close a compound SQL statement when its END is lowercase#15046erwinzhang7 wants to merge 3 commits into
Conversation
splitSqlQuery matched the opening BEGIN/CASE of a compound statement case insensitively but matched the closing END case sensitively. SQLite accepts either case, so a trigger written with a lowercase end never closed and every following statement was folded into the trigger body instead of being split out. The existing tests already covered a lowercase begin, but always paired it with an uppercase END, so the asymmetry went unnoticed.
🦋 Changeset detectedLatest commit: d2dc2e8 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 |
alsuren
left a comment
There was a problem hiding this comment.
approve once changeset is fixed
|
Good catch, and correct on both counts. I had not read Rewritten in user-facing terms: it now says which commands are affected, what a user actually sees (statements after the block silently skipped), and keeps a short example. Dropped the reference to the internal helper and the paragraph about existing test coverage. I also confirmed the |
dario-piotrowicz
left a comment
There was a problem hiding this comment.
Looks good to me! 😄
Thanks for the fix @erwinzhang7 😄
| // SQLite accepts either case. Only the BEGIN side was matched case | ||
| // insensitively, so a lowercase `end` never closed the compound statement | ||
| // and every following statement was swallowed into the trigger. |
There was a problem hiding this comment.
Is this comment helpful/necessary? 🤔
| // SQLite accepts either case. Only the BEGIN side was matched case | |
| // insensitively, so a lowercase `end` never closed the compound statement | |
| // and every following statement was swallowed into the trigger. |
| "wrangler": patch | ||
| --- | ||
|
|
||
| Run every statement in a D1 SQL file when a trigger or CASE block ends with a lowercase `end` |
There was a problem hiding this comment.
very nit: the changelog title doesn't feel super clear to me 🤔
I'd go with something along the lines of:
| Run every statement in a D1 SQL file when a trigger or CASE block ends with a lowercase `end` | |
| Fixes D1 SQL statements not handling lowercase `end`s correclty |
maybe
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
@cloudflare/autoconfig
@cloudflare/build-output-utils
@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-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
Both taken, thank you. Dropped the comment: the test name already says what it checks, and the history of why it broke belongs in the changeset rather than the test body. Took the changelog title too, with the typo corrected: "Fixes D1 SQL statements not handling lowercase Also filled in the description checkboxes, which is what the The two Windows test failures look unrelated to this change. The suite passes on Linux and macOS, the failing jobs are |
|
The checks on this one are still pending workflow approval, could someone kick them off when you get a chance? |
Spotted while investigating #14991. Independent of that issue and of any fix for it.
The bug
splitSqlQuery()matches the two ends of a compound statement asymmetrically:SQLite accepts either case. With a lowercase
end, the compound statement never closes, socompoundStatementStackstays non-empty and every subsequent;is treated as being inside the trigger body. Everything after the trigger is folded into it instead of being split out and executed.Before, 2 statements, with the
CREATE TABLEswallowed:After, 3 statements, matching the uppercase behaviour exactly.
CASE ... endinside a trigger body is affected the same way, collapsing 3 statements into 1.Why it was not caught
should handle compound statements for BEGINsalready exercises a lowercasebegin, so case insensitivity was clearly intended. But every case in that test pairs it with an uppercaseEND, so only the start predicate was ever exercised in lowercase.Change
One character: add the
iflag toisCompoundStatementEnd, so both predicates agree.This does not widen what counts as an end marker beyond the existing behaviour.
\sENDstill requires whitespace immediately before, so an identifier such asweekend;does not match, exactly as it did not before.DISTINCT/BEGIN/ENDcase handling is not described anywhere in the public docs, and nothing about the supported SQL surface changes.Added
should handle a lowercase end closing a compound statement, placed next to the existing lowercase-begincoverage. Verified it is a real regression test rather than a passing assertion: with theiflag reverted the suite reports 1 failed of 16, and with the fix 16 of 16 pass.oxfmt --checkis clean on both changed files.