mirror of
https://github.com/simonw/sqlite-utils.git
synced 2026-07-23 01:14:31 +02:00
Improve db.query() error handling and transaction safety
db.query() rejected statements roll back, RETURNING commits immediately. Fixes two transaction bugs in db.query(): - A non-row-returning statement such as an UPDATE was executed and auto-committed by execute() before the ValueError was raised, so the write took effect despite being rejected. query() now runs the statement inside a savepoint and rolls it back before raising, so a rejected statement has no effect on the database - in every connection mode, including autocommit=True. - INSERT ... RETURNING only committed once the returned generator was fully exhausted, so calling query() without iterating - or partially iterating with next() - left the transaction open and the write could be rolled back on close. The write is now completed and committed at call time, as the documentation already promised. Plain SELECTs are still fetched lazily. Transaction control statements, VACUUM, ATTACH and DETACH never return rows, so query() now rejects them without executing them. PRAGMA statements skip the savepoint guard because some of them - such as pragma journal_mode=wal - refuse to run inside a transaction. Claude-Session: https://claude.ai/code/session_012U3iRfJoTZ5vd22cBSF2nJ
This commit is contained in:
Simon Willison
GitHub
parent
04f8971546
commit
0566a9f128
5 changed files with 171 additions and 26 deletions
|
|
@ -12,7 +12,7 @@ Unreleased
|
|||
Breaking changes:
|
||||
|
||||
- Write statements executed with ``db.execute()`` are now committed automatically, unless a transaction is already open in which case they join it. Previously they opened an implicit transaction that stayed open until something committed it - writes appeared to work when read on the same connection but were silently rolled back when the connection closed. Code that relied on rolling back uncommitted ``db.execute()`` writes should use the new ``db.begin()`` method to open an explicit transaction first. The transaction model is documented in full at :ref:`python_api_transactions`.
|
||||
- ``db.query()`` now executes its SQL as soon as it is called, rather than waiting until the returned generator is first iterated. Rows are still fetched lazily during iteration. SQL errors are now raised at the call site, statements such as ``INSERT ... RETURNING`` take effect without needing to iterate over their results, and passing a statement that returns no rows - previously a silent no-op - now raises a ``ValueError`` recommending ``db.execute()`` instead.
|
||||
- ``db.query()`` now executes its SQL as soon as it is called, rather than waiting until the returned generator is first iterated. Rows are still fetched lazily during iteration. SQL errors are now raised at the call site, statements such as ``INSERT ... RETURNING`` are executed and committed immediately without needing to iterate over their results, and passing a statement that returns no rows - previously a silent no-op - now raises a ``ValueError`` recommending ``db.execute()`` instead. A statement rejected this way is rolled back before the error is raised, so it has no effect on the database.
|
||||
- Python API validation errors now raise ``ValueError`` instead of ``AssertionError``. Previously invalid arguments - such as ``create_table()`` with no columns, ``transform()`` on a table that does not exist, or passing both ``ignore=True`` and ``replace=True`` - were rejected using bare ``assert`` statements, which are silently skipped when Python runs with the ``-O`` flag. Code that caught ``AssertionError`` for these cases should catch ``ValueError`` instead.
|
||||
- ``table.upsert()`` and ``table.upsert_all()`` now raise ``PrimaryKeyRequired`` if a record is missing a value for any primary key column, or has a value of ``None`` for one. Previously such records - which can never match an existing row - were quietly inserted as brand new rows, or triggered a confusing ``KeyError`` after the insert had already taken place.
|
||||
- ``db.enable_wal()`` and ``db.disable_wal()`` now raise a ``sqlite_utils.db.TransactionError`` if called while a transaction is open. Previously they would silently commit the open transaction as a side effect of changing the journal mode, breaking the rollback guarantee of ``db.atomic()`` and of user-managed transactions.
|
||||
|
|
|
|||
|
|
@ -229,9 +229,9 @@ The ``db.query(sql)`` function executes a SQL query and returns an iterator over
|
|||
# {'name': 'Cleo'}
|
||||
# {'name': 'Pancakes'}
|
||||
|
||||
The SQL query is executed as soon as ``db.query()`` is called. The resulting rows are fetched lazily as you iterate, so large result sets are not loaded into memory all at once. Because execution is immediate, an error in your SQL will raise an exception straight away, and a statement such as ``INSERT ... RETURNING`` will take effect even if you do not iterate over its results.
|
||||
The SQL query is executed as soon as ``db.query()`` is called. The resulting rows are fetched lazily as you iterate, so large result sets are not loaded into memory all at once. Because execution is immediate, an error in your SQL will raise an exception straight away, and a statement such as ``INSERT ... RETURNING`` will take effect - and be committed, unless a transaction is open - even if you do not iterate over its results.
|
||||
|
||||
``db.query()`` can only be used with SQL that returns rows. Passing a statement that returns no rows - an ``INSERT`` or ``UPDATE`` without a ``RETURNING`` clause, for example - will raise a ``ValueError``. Use :ref:`db.execute() <python_api_execute>` for those statements instead.
|
||||
``db.query()`` can only be used with SQL that returns rows. Passing a statement that returns no rows - an ``INSERT`` or ``UPDATE`` without a ``RETURNING`` clause, for example - will raise a ``ValueError``. The rejected statement is rolled back, so it has no effect on the database. Use :ref:`db.execute() <python_api_execute>` for those statements instead.
|
||||
|
||||
.. _python_api_execute:
|
||||
|
||||
|
|
@ -356,7 +356,7 @@ If a transaction is open - because the call happens inside a ``db.atomic()`` blo
|
|||
db.execute("insert into news (headline) values (?)", ["Cat unimpressed"])
|
||||
# Both rows committed together
|
||||
|
||||
One corner case: a row-returning write such as ``INSERT ... RETURNING`` executed through ``db.execute()`` cannot be auto-committed, because its rows have not been read yet - call ``db.commit()`` after fetching them, or use :ref:`db.query() <python_api_query>` for those statements, which commits automatically once you have iterated over the results.
|
||||
One corner case: a row-returning write such as ``INSERT ... RETURNING`` executed through ``db.execute()`` cannot be auto-committed, because its rows have not been read yet - call ``db.commit()`` after fetching them, or use :ref:`db.query() <python_api_query>` for those statements, which executes the write and commits it immediately.
|
||||
|
||||
.. _python_api_transactions_manual:
|
||||
|
||||
|
|
|
|||
|
|
@ -58,7 +58,7 @@ Python API changes
|
|||
**db.query() executes immediately.** ``db.query(sql)`` previously returned a generator that did not execute the SQL until you started iterating over it. The SQL now runs as soon as the method is called - rows are still fetched lazily. Two consequences:
|
||||
|
||||
- Errors in your SQL now raise at the ``db.query()`` call site rather than on first iteration.
|
||||
- Passing a statement that returns no rows - such as an ``INSERT`` or ``UPDATE`` without a ``RETURNING`` clause - previously did nothing at all, silently. It now raises a ``ValueError``. Use ``db.execute()`` for statements that do not return rows.
|
||||
- Passing a statement that returns no rows - such as an ``INSERT`` or ``UPDATE`` without a ``RETURNING`` clause - previously did nothing at all, silently. It now raises a ``ValueError``, and the statement is rolled back so it has no effect on the database. Use ``db.execute()`` for statements that do not return rows.
|
||||
|
||||
**Upserts use INSERT ... ON CONFLICT.** Upsert operations now use SQLite's ``INSERT ... ON CONFLICT SET`` syntax rather than the previous ``INSERT OR IGNORE`` followed by ``UPDATE``. If your code depends on the old behavior, pass ``use_old_upsert=True`` to the ``Database()`` constructor - see :ref:`python_api_old_upsert`.
|
||||
|
||||
|
|
|
|||
|
|
@ -652,33 +652,68 @@ class Database:
|
|||
Execute ``sql`` and return an iterable of dictionaries representing each row.
|
||||
|
||||
The SQL is executed as soon as this method is called - the resulting rows
|
||||
are then fetched lazily as the returned iterable is iterated over.
|
||||
are then fetched lazily as the returned iterable is iterated over. A
|
||||
row-returning write such as ``INSERT ... RETURNING`` takes effect
|
||||
immediately, even if the results are never iterated.
|
||||
|
||||
:param sql: SQL query to execute
|
||||
:param params: Parameters to use in that query - an iterable for ``where id = ?``
|
||||
parameters, or a dictionary for ``where id = :id``
|
||||
:raises ValueError: if the SQL statement does not return rows - use
|
||||
:meth:`execute` for those statements instead
|
||||
:meth:`execute` for those statements instead. The rejected statement
|
||||
is rolled back, so it has no effect on the database
|
||||
"""
|
||||
was_in_transaction = self.conn.in_transaction
|
||||
cursor = self.execute(sql, params or tuple())
|
||||
if cursor.description is None:
|
||||
raise ValueError(
|
||||
"query() can only be used with SQL that returns rows - "
|
||||
"use execute() for other statements"
|
||||
)
|
||||
keys = [d[0] for d in cursor.description]
|
||||
|
||||
def rows() -> Generator[dict, None, None]:
|
||||
for row in cursor:
|
||||
yield dict(zip(keys, row))
|
||||
if not was_in_transaction and self.conn.in_transaction:
|
||||
# A row-returning write such as INSERT ... RETURNING opened
|
||||
# an implicit transaction - commit it now that its rows have
|
||||
# been consumed
|
||||
self.conn.execute("COMMIT")
|
||||
|
||||
return rows()
|
||||
message = (
|
||||
"query() can only be used with SQL that returns rows - "
|
||||
"use execute() for other statements"
|
||||
)
|
||||
prefix = sql.lstrip().upper()
|
||||
if prefix.startswith(
|
||||
_TRANSACTION_CONTROL_PREFIXES + ("VACUUM", "ATTACH", "DETACH")
|
||||
):
|
||||
# None of these return rows - reject them without executing anything
|
||||
raise ValueError(message)
|
||||
if self._tracer:
|
||||
self._tracer(sql, params)
|
||||
args: tuple = (params,) if params is not None else ()
|
||||
if prefix.startswith("PRAGMA"):
|
||||
# Some PRAGMA statements refuse to run inside a transaction, so
|
||||
# execute these without the savepoint guard used below. PRAGMAs
|
||||
# never open an implicit transaction, so there is nothing to
|
||||
# undo if this one turns out not to return rows
|
||||
cursor = self.conn.execute(sql, *args)
|
||||
if cursor.description is None:
|
||||
raise ValueError(message)
|
||||
keys = [d[0] for d in cursor.description]
|
||||
return (dict(zip(keys, row)) for row in cursor)
|
||||
# Execute inside a savepoint, so a statement that turns out not to
|
||||
# return rows can be rolled back before the ValueError is raised
|
||||
self.conn.execute('SAVEPOINT "sqlite_utils_query"')
|
||||
released = False
|
||||
try:
|
||||
cursor = self.conn.execute(sql, *args)
|
||||
if cursor.description is None:
|
||||
raise ValueError(message)
|
||||
keys = [d[0] for d in cursor.description]
|
||||
try:
|
||||
self.conn.execute('RELEASE "sqlite_utils_query"')
|
||||
released = True
|
||||
except sqlite3.OperationalError:
|
||||
# The savepoint cannot be released while a write statement is
|
||||
# still executing - this is INSERT ... RETURNING or similar,
|
||||
# with unfetched rows. Fetch them so the write completes, then
|
||||
# release again - committing the write immediately, unless an
|
||||
# outer transaction is open
|
||||
fetched = cursor.fetchall()
|
||||
self.conn.execute('RELEASE "sqlite_utils_query"')
|
||||
released = True
|
||||
return (dict(zip(keys, row)) for row in fetched)
|
||||
return (dict(zip(keys, row)) for row in cursor)
|
||||
finally:
|
||||
if not released:
|
||||
# An error occurred - undo anything the statement changed
|
||||
self.conn.execute('ROLLBACK TO "sqlite_utils_query"')
|
||||
self.conn.execute('RELEASE "sqlite_utils_query"')
|
||||
|
||||
def execute(
|
||||
self, sql: str, parameters: Optional[Union[Sequence, Dict[str, Any]]] = None
|
||||
|
|
|
|||
|
|
@ -23,6 +23,56 @@ def test_query_rejects_statements_that_return_no_rows(fresh_db):
|
|||
with pytest.raises(ValueError) as ex:
|
||||
fresh_db.query("update dogs set name = 'Cleopaws'")
|
||||
assert "execute()" in str(ex.value)
|
||||
# The rejected update was rolled back, and no transaction is left open
|
||||
assert not fresh_db.conn.in_transaction
|
||||
assert [row["name"] for row in fresh_db["dogs"].rows] == ["Cleo"]
|
||||
|
||||
|
||||
def test_query_rejected_ddl_is_rolled_back(fresh_db):
|
||||
with pytest.raises(ValueError):
|
||||
fresh_db.query("create table dogs (id integer primary key)")
|
||||
assert not fresh_db.conn.in_transaction
|
||||
assert fresh_db.table_names() == []
|
||||
|
||||
|
||||
def test_query_rejected_write_inside_transaction_is_rolled_back(fresh_db):
|
||||
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("update dogs set name = 'Cleopaws'")
|
||||
# The transaction is still open and the earlier insert is intact
|
||||
assert fresh_db.conn.in_transaction
|
||||
fresh_db.commit()
|
||||
assert [row["name"] for row in fresh_db["dogs"].rows] == ["Cleo", "Pancakes"]
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"sql", ["begin", "commit", "rollback", "vacuum", "detach database foo"]
|
||||
)
|
||||
def test_query_rejects_transaction_control_and_vacuum(fresh_db, sql):
|
||||
with pytest.raises(ValueError) as ex:
|
||||
fresh_db.query(sql)
|
||||
assert "execute()" in str(ex.value)
|
||||
assert not fresh_db.conn.in_transaction
|
||||
|
||||
|
||||
def test_query_error_leaves_no_transaction_open(fresh_db):
|
||||
with pytest.raises(sqlite3.OperationalError):
|
||||
fresh_db.query("select * from missing_table")
|
||||
assert not fresh_db.conn.in_transaction
|
||||
|
||||
|
||||
def test_query_pragma(tmpdir):
|
||||
from sqlite_utils import Database
|
||||
|
||||
db = Database(str(tmpdir / "test.db"))
|
||||
# A row-returning PRAGMA works, including one that cannot run in a transaction
|
||||
assert list(db.query("pragma journal_mode = wal")) == [{"journal_mode": "wal"}]
|
||||
# A PRAGMA that returns no rows raises ValueError
|
||||
with pytest.raises(ValueError):
|
||||
db.query("pragma user_version = 5")
|
||||
db.close()
|
||||
|
||||
|
||||
@pytest.mark.skipif(
|
||||
|
|
@ -38,6 +88,66 @@ def test_query_insert_returning(fresh_db):
|
|||
assert fresh_db["dogs"].count == 2
|
||||
|
||||
|
||||
@pytest.mark.skipif(
|
||||
sqlite3.sqlite_version_info < (3, 35, 0),
|
||||
reason="RETURNING requires SQLite 3.35.0 or higher",
|
||||
)
|
||||
def test_query_insert_returning_commits_without_iteration(tmpdir):
|
||||
from sqlite_utils import Database
|
||||
|
||||
path = str(tmpdir / "test.db")
|
||||
db = Database(path)
|
||||
db["dogs"].insert({"name": "Cleo"})
|
||||
# Never iterate over the results
|
||||
db.query("insert into dogs (name) values ('Pancakes') returning name")
|
||||
assert not db.conn.in_transaction
|
||||
# A completely separate connection sees the new row straight away
|
||||
other = sqlite3.connect(path)
|
||||
assert other.execute("select count(*) from dogs").fetchone()[0] == 2
|
||||
other.close()
|
||||
db.close()
|
||||
|
||||
|
||||
@pytest.mark.skipif(
|
||||
sqlite3.sqlite_version_info < (3, 35, 0),
|
||||
reason="RETURNING requires SQLite 3.35.0 or higher",
|
||||
)
|
||||
def test_query_insert_returning_partial_iteration_still_commits(tmpdir):
|
||||
from sqlite_utils import Database
|
||||
|
||||
path = str(tmpdir / "test.db")
|
||||
db = Database(path)
|
||||
db["dogs"].insert({"name": "Cleo"})
|
||||
row = next(
|
||||
db.query(
|
||||
"insert into dogs (name) values ('Pancakes'), ('Marnie') returning name"
|
||||
)
|
||||
)
|
||||
assert row == {"name": "Pancakes"}
|
||||
assert not db.conn.in_transaction
|
||||
other = sqlite3.connect(path)
|
||||
assert other.execute("select count(*) from dogs").fetchone()[0] == 3
|
||||
other.close()
|
||||
db.close()
|
||||
|
||||
|
||||
@pytest.mark.skipif(
|
||||
sqlite3.sqlite_version_info < (3, 35, 0),
|
||||
reason="RETURNING requires SQLite 3.35.0 or higher",
|
||||
)
|
||||
def test_query_insert_returning_respects_explicit_transaction(fresh_db):
|
||||
fresh_db["dogs"].insert({"name": "Cleo"})
|
||||
fresh_db.begin()
|
||||
rows = list(
|
||||
fresh_db.query("insert into dogs (name) values ('Pancakes') returning name")
|
||||
)
|
||||
assert rows == [{"name": "Pancakes"}]
|
||||
# Still inside the explicit transaction - not committed
|
||||
assert fresh_db.conn.in_transaction
|
||||
fresh_db.rollback()
|
||||
assert [row["name"] for row in fresh_db["dogs"].rows] == ["Cleo"]
|
||||
|
||||
|
||||
def test_execute_returning_dicts(fresh_db):
|
||||
# Like db.query() but returns a list, included for backwards compatibility
|
||||
# see https://github.com/simonw/sqlite-utils/issues/290
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue