Skip to content
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

🐛 BUG: Invalid BEGIN TRANSACTION detection #5085

Open
schkovich opened this issue Feb 24, 2024 · 2 comments · May be fixed by #8412
Open

🐛 BUG: Invalid BEGIN TRANSACTION detection #5085

schkovich opened this issue Feb 24, 2024 · 2 comments · May be fixed by #8412
Labels
bug Something that isn't working d1 Relating to D1

Comments

@schkovich
Copy link

schkovich commented Feb 24, 2024

Which Cloudflare product(s) does this pertain to?

D1, Wrangler core

What version(s) of the tool(s) are you using?

3.29.0 [wrangler]

What version of Node are you using?

v20.11.1

What operating system and version are you using?

Ubuntu 22.04.3 LTS

Describe the Bug

Observed behaviour

I tried to update a row in D1 by passing a string that contains the words BEGIN TRANSACTION utilising the Wrangler D1 command execute and got the error below.

✘ [ERROR] Wrangler could not process the provided SQL file, as it contains several transactions.

Expected behaviour

The row will be updated.

Steps to reproduce

  • No worker involved. I am running a Wrangler command against D1
  • wrangler.toml is not relevant, since I am running a Wrangler command against D1, but here it is:
name = "ai"
main = "src/index.js"
compatibility_date = "2023-12-06"
  • I am not running a local dev server
  • To reproduce run from terminal the two bash commands below:
TEXT="You can''t refer to a table that doesn''t yet exist.\n* If you get \"cannot start a transaction within a transaction\", make sure you have removed \`BEGIN TRANSACTION\` and \`COMMIT\` from your dumped SQL statements."
npx wrangler d1 execute <any-valid-db-name> --local --command "UPDATE Nodes SET text=\"${TEXT}\" WHERE id=537;" 

Please provide a link to a minimal reproduction

No response

Please provide any relevant error logs

No response

@schkovich schkovich added the bug Something that isn't working label Feb 24, 2024
@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Feb 24, 2024
@schkovich
Copy link
Author

schkovich commented Feb 24, 2024

A workaround is to add a space between BEGIN and TRANSACTION, eg BEGIN TRANSACTION. Since having two spaces is a valid SQL command, it makes me think that it might be possible to bypass this restriction by adding an extra space to the BEGIN TRANSACTION command. For example, create a file elon.sql:

BEGIN  TRANSACTION;
INSERT INTO "X" ("id", "text") VALUES (314, "Formerly know as Twitter");
# 101k insert statements
COMMIT;

and run

npx wrangler d1 execute musk-db --file=elon.sql

@penalosa penalosa added the d1 Relating to D1 label Jun 10, 2024
@penalosa penalosa removed this from workers-sdk Jul 8, 2024
@FlareLine FlareLine linked a pull request Mar 10, 2025 that will close this issue
9 tasks
@FlareLine
Copy link

FlareLine commented Mar 10, 2025

NB: Transactions aren't currently supported yet, there is #2733 is implement this, however it is still open and in active discussion.

This, on an initial glance, seems to be caused by

export function trimSqlQuery(sql: string): string {
if (!mayContainTransaction(sql)) {
return sql;
}
//note that we are intentionally not using greedy replace here, as we're targeting sqlite's dump command
const trimmedSql = sql
.replace("BEGIN TRANSACTION;", "")
.replace("COMMIT;", "");
//if the trimmed output STILL contains transactions, we should just tell them to remove them and try again.
if (mayContainTransaction(trimmedSql)) {
throw new UserError(
"Wrangler could not process the provided SQL file, as it contains several transactions.\nD1 runs your SQL in a transaction for you.\nPlease export an SQL file from your SQLite database and try again."
);
}
return trimmedSql;
}
// sqlite may start an sql dump file with pragmas,
// so we can't just use sql.startsWith here.
export function mayContainTransaction(sql: string): boolean {
return sql.includes("BEGIN TRANSACTION");
}

The mayContainTransaction function checks for BEGIN TRANSACTION, but the trimSqlQuery attempts to remove statements BEGIN TRANSACTION; (with a semicolon) and COMMIT; (with a semicolon). As in your example, this will fail (and result in the multiple transaction error) when the string BEGIN TRANSACTION is found anywhere in a SQL statement, even when the string is representing a string value.

I have raised #8412 to identify a potential fix - I most likely won't have free capcity to write tests to test this out in the near future, so if anyone's got any spare time feel free to take this draft PR as inspiration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working d1 Relating to D1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants