From adc10df98102c76c86c77108615792ba238a0ae3 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 6 Jul 2026 21:22:29 -0700 Subject: [PATCH] Fix for db.query("; COMMIT") bypasses the first-token scanner Refs https://github.com/simonw/sqlite-utils/issues/769#issuecomment-4900034150 --- docs/changelog.rst | 1 + sqlite_utils/db.py | 11 ++++++----- tests/test_atomic.py | 13 +++++++++++++ tests/test_query.py | 27 +++++++++++++++++++++++++++ 4 files changed, 47 insertions(+), 5 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 3bdab36..30ad989 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -17,6 +17,7 @@ Unreleased - ``table.insert_all(..., pk=...)`` now raises ``InvalidColumns`` if ``pk=`` names columns that do not exist in an existing table. Previously this behaved inconsistently, with single-row inserts raising a ``KeyError`` while other row counts succeeded. (:issue:`732`) - Fixed an ``IndexError`` from ``table.insert(..., pk=..., ignore=True)`` when an ignored insert followed writes to another table on the same connection. ``last_pk`` is now populated from the explicit primary key value instead of looking up a stale ``lastrowid``. (:issue:`554`) - Fixed a bug where a failed write statement executed with ``db.execute()`` left the driver's implicit transaction open. Every subsequent write then joined that phantom transaction, which nothing committed, so their work was silently rolled back when the connection was closed. The implicit transaction opened by a failed statement is now rolled back before the exception is raised. A failed write inside a transaction opened with ``db.begin()`` or ``db.atomic()`` leaves that transaction open and untouched, as before. +- Fixed a bug where transaction-control statements prefixed with an empty statement - ``db.query("; COMMIT")`` - or a UTF-8 byte order mark slipped past the check that rejects them, committing the caller's open transaction before raising a confusing ``OperationalError``. The keyword scanner used by ``db.query()`` and ``db.execute()`` now skips leading ``;`` and byte order marks, matching what the ``sqlite3`` driver tolerates before the first token, so these statements are rejected with a ``ValueError`` without being executed. The same fix means ``db.execute("; BEGIN")`` no longer auto-commits the transaction it just opened. .. _v4_0rc3: diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 534f145..bd023c5 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -436,14 +436,15 @@ _QUERY_REJECTED_KEYWORDS = _TRANSACTION_CONTROL_KEYWORDS | { def _first_keyword(sql: str) -> str: """ - Return the first keyword of a SQL statement, uppercased, skipping any - leading whitespace and ``--`` or ``/* ... */`` comments - the only - things SQLite's tokenizer allows before the first token. Returns an - empty string if there is no leading keyword. + Return the first keyword of a SQL statement, uppercased, skipping + everything the sqlite3 driver tolerates before the first real token: + whitespace, ``--`` or ``/* ... */`` comments, empty statements + (bare ``;``) and a UTF-8 byte order mark. Returns an empty string if + there is no leading keyword. """ i, n = 0, len(sql) while i < n: - if sql[i].isspace(): + if sql[i].isspace() or sql[i] in (";", "\ufeff"): i += 1 elif sql.startswith("--", i): newline = sql.find("\n", i) diff --git a/tests/test_atomic.py b/tests/test_atomic.py index 1a4b4ae..0d25b84 100644 --- a/tests/test_atomic.py +++ b/tests/test_atomic.py @@ -258,6 +258,19 @@ def test_execute_comment_prefixed_begin_leaves_transaction_open(fresh_db): assert [r["id"] for r in fresh_db["t"].rows] == [1] +@pytest.mark.parametrize("begin_sql", ["; begin", "\ufeffbegin"]) +def test_execute_prefixed_begin_leaves_transaction_open(fresh_db, begin_sql): + # sqlite3 tolerates empty statements and a UTF-8 BOM before the first + # real token, so a BEGIN behind either must not be auto-committed + # out from under the caller + fresh_db["t"].insert({"id": 1}, pk="id") + fresh_db.execute(begin_sql) + assert fresh_db.conn.in_transaction + fresh_db.execute("insert into t (id) values (2)") + fresh_db.rollback() + assert [r["id"] for r in fresh_db["t"].rows] == [1] + + def test_execute_failed_write_rolls_back_implicit_transaction(tmpdir): # A failed write must not leave the driver's implicit transaction open - # that would silently disable auto-commit for every subsequent write diff --git a/tests/test_query.py b/tests/test_query.py index 0b9f2ae..c2f6731 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -59,6 +59,10 @@ def test_query_rejected_write_inside_transaction_is_rolled_back(fresh_db): "-- comment\nbegin", "/* multi\nline */ -- and another\n vacuum", "\t /* a */ /* b */ savepoint s1", + "; commit", + ";;\n ; rollback", + "; /* comment */ vacuum", + "\ufeffbegin", ], ) def test_query_rejects_transaction_control_and_vacuum(fresh_db, sql): @@ -83,6 +87,23 @@ def test_query_comment_prefixed_commit_does_not_commit_transaction(fresh_db): assert [row["name"] for row in fresh_db["dogs"].rows] == ["Cleo"] +@pytest.mark.parametrize("sql", ["; COMMIT", "\ufeffCOMMIT"]) +def test_query_prefixed_commit_does_not_commit_transaction(fresh_db, sql): + # sqlite3 tolerates empty statements and a UTF-8 BOM before the first + # real token, so the keyword scanner must skip them too - previously + # '; COMMIT' slipped past the check and committed the caller's open + # transaction before raising OperationalError + fresh_db["dogs"].insert({"name": "Cleo"}) + fresh_db.begin() + fresh_db.execute("insert into dogs (name) values ('Pancakes')") + with pytest.raises(ValueError): + fresh_db.query(sql) + # The explicit transaction is still open and can still be rolled back + assert fresh_db.conn.in_transaction + fresh_db.rollback() + assert [row["name"] for row in fresh_db["dogs"].rows] == ["Cleo"] + + def test_query_error_leaves_no_transaction_open(fresh_db): with pytest.raises(sqlite3.OperationalError): fresh_db.query("select * from missing_table") @@ -137,6 +158,12 @@ def test_query_comment_prefixed_pragma_inside_transaction(fresh_db): ("", ""), (" ", ""), ("123", ""), + ("; commit", "COMMIT"), + (";;\n ; rollback", "ROLLBACK"), + ("; -- comment\n begin", "BEGIN"), + ("\ufeffcommit", "COMMIT"), + ("\ufeff ; select 1", "SELECT"), + (";", ""), ], ) def test_first_keyword(sql, expected):