mirror of
https://github.com/simonw/sqlite-utils.git
synced 2026-07-23 09:24:31 +02:00
Transform now refuses to run inside a transaction if destructive foreign keys exist
PRAGMA foreign_keys cannot be changed inside a transaction, and the defer_foreign_keys fallback only defers violation checks, not ON DELETE actions - so calling table.transform() inside db.atomic() with PRAGMA foreign_keys enabled would silently fire ON DELETE CASCADE / SET NULL / SET DEFAULT actions on referencing tables when the old table is dropped. transform() now raises TransactionError in that situation, naming the offending foreign keys and suggesting either running it outside the transaction or turning the pragma off before opening it. Tables with only non-destructive inbound foreign keys (NO ACTION, RESTRICT) can still be transformed inside a transaction via defer_foreign_keys. Closes #794 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014StVTWQJpFhfZJK2CYVBwv
This commit is contained in:
Claude
parent
d714200659
commit
44dd92a12c
3 changed files with 190 additions and 2 deletions
|
|
@ -434,9 +434,10 @@ The library will never commit a transaction you opened. If you call write method
|
|||
|
||||
Prefer ``db.atomic()`` or ``db.begin()``, ``db.commit()`` and ``db.rollback()`` over mixing sqlite-utils transaction methods with calls to ``db.conn.commit()``, ``db.conn.rollback()`` or raw transaction-control SQL. Mixing the two layers makes it much harder to tell which layer owns the current transaction.
|
||||
|
||||
Two related safeguards to be aware of:
|
||||
Some related safeguards to be aware of:
|
||||
|
||||
- ``db.enable_wal()`` and ``db.disable_wal()`` raise a ``sqlite_utils.db.TransactionError`` if called while a transaction is open, because changing the journal mode would commit it as a side effect.
|
||||
- ``table.transform()`` raises a ``sqlite_utils.db.TransactionError`` if called while a transaction is open with ``PRAGMA foreign_keys`` enabled and the table is referenced by foreign keys with destructive ``ON DELETE`` actions, because the pragma cannot be turned off mid-transaction to protect those referencing rows - see :ref:`python_api_transform_foreign_keys_transactions`.
|
||||
- Closing the database - explicitly with ``db.close()``, or by exiting a ``with Database(...) as db:`` block - rolls back any transaction that is still open, see :ref:`python_api_close`.
|
||||
|
||||
.. _python_api_transactions_modes:
|
||||
|
|
@ -1799,6 +1800,36 @@ This method raises a ``sqlite_utils.db.TransformError`` exception if the table c
|
|||
.. note::
|
||||
In the CLI: :ref:`sqlite-utils transform <cli_transform_table>`
|
||||
|
||||
.. _python_api_transform_foreign_keys_transactions:
|
||||
|
||||
Foreign keys and transactions
|
||||
-----------------------------
|
||||
|
||||
Because ``.transform()`` drops the old table, running it with ``PRAGMA foreign_keys`` enabled could fire ``ON DELETE`` actions on any tables that reference it - an inbound ``ON DELETE CASCADE`` foreign key would silently delete those referencing rows. To prevent this, ``.transform()`` turns ``PRAGMA foreign_keys`` off for the duration of the operation and restores it afterwards, running ``PRAGMA foreign_key_check`` before committing.
|
||||
|
||||
``PRAGMA foreign_keys`` cannot be changed inside a transaction, so this protection is impossible if you call ``.transform()`` while a transaction is already open - for example inside a ``with db.atomic():`` block or after ``db.begin()``. If ``PRAGMA foreign_keys`` is on and another table references the table being transformed with a destructive ``ON DELETE`` action - ``CASCADE``, ``SET NULL`` or ``SET DEFAULT`` - the method will refuse to run and raise a ``sqlite_utils.db.TransactionError``:
|
||||
|
||||
.. code-block:: python
|
||||
|
||||
from sqlite_utils.db import TransactionError
|
||||
|
||||
try:
|
||||
with db.atomic():
|
||||
db["authors"].transform(types={"id": str})
|
||||
except TransactionError as ex:
|
||||
print("Could not transform in transaction:", ex)
|
||||
|
||||
To transform such a table either call ``.transform()`` outside of the transaction, or execute ``PRAGMA foreign_keys = off`` before opening it:
|
||||
|
||||
.. code-block:: python
|
||||
|
||||
db.execute("PRAGMA foreign_keys = off")
|
||||
with db.atomic():
|
||||
db["authors"].transform(types={"id": str})
|
||||
db.execute("PRAGMA foreign_keys = on")
|
||||
|
||||
Tables referenced by foreign keys without a destructive action (the default ``NO ACTION``, or ``RESTRICT``) can still be transformed inside a transaction - sqlite-utils uses ``PRAGMA defer_foreign_keys`` to postpone the foreign key checks until the transaction commits.
|
||||
|
||||
.. _python_api_transform_alter_column_types:
|
||||
|
||||
Altering column types
|
||||
|
|
|
|||
|
|
@ -2522,6 +2522,11 @@ class Table(Queryable):
|
|||
|
||||
See :ref:`python_api_transform` for full details.
|
||||
|
||||
Raises :py:class:`sqlite_utils.db.TransactionError` if called while a
|
||||
transaction is open with ``PRAGMA foreign_keys`` enabled and the table
|
||||
is referenced by foreign keys with destructive ``ON DELETE`` actions -
|
||||
see :ref:`python_api_transform_foreign_keys_transactions`.
|
||||
|
||||
:param types: Columns that should have their type changed, for example ``{"weight": float}``
|
||||
:param rename: Columns to rename, for example ``{"headline": "title"}``
|
||||
:param drop: Columns to drop
|
||||
|
|
@ -2566,6 +2571,36 @@ class Table(Queryable):
|
|||
should_defer_foreign_keys = (
|
||||
pragma_foreign_keys_was_on and already_in_transaction
|
||||
)
|
||||
if should_defer_foreign_keys:
|
||||
# PRAGMA foreign_keys is a no-op inside a transaction, and
|
||||
# defer_foreign_keys only defers violation checks, not ON DELETE
|
||||
# actions - so dropping the old table would still fire destructive
|
||||
# actions on any tables that reference it. Refuse rather than
|
||||
# silently modify or delete those rows.
|
||||
destructive_fks = [
|
||||
(table.name, fk)
|
||||
for table in self.db.tables
|
||||
for fk in table.foreign_keys
|
||||
if fk.other_table == self.name
|
||||
and fk.on_delete in ("CASCADE", "SET NULL", "SET DEFAULT")
|
||||
]
|
||||
if destructive_fks:
|
||||
raise TransactionError(
|
||||
"Cannot transform table {table} while a transaction is open: "
|
||||
"PRAGMA foreign_keys cannot be changed inside a transaction, "
|
||||
"and the table is referenced by foreign keys with ON DELETE "
|
||||
"actions that would fire when the old table is dropped: "
|
||||
"{fks}. Call transform() outside of the transaction, or "
|
||||
'execute "PRAGMA foreign_keys = off" before opening it.'.format(
|
||||
table=self.name,
|
||||
fks=", ".join(
|
||||
"{}.{} (ON DELETE {})".format(
|
||||
table_name, ", ".join(fk.columns), fk.on_delete
|
||||
)
|
||||
for table_name, fk in destructive_fks
|
||||
),
|
||||
)
|
||||
)
|
||||
defer_foreign_keys_was_on = False
|
||||
try:
|
||||
if should_disable_foreign_keys:
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import sqlite3
|
||||
|
||||
from sqlite_utils.db import ForeignKey, TransformError
|
||||
from sqlite_utils.db import ForeignKey, TransactionError, TransformError
|
||||
from sqlite_utils.utils import OperationalError
|
||||
import pytest
|
||||
|
||||
|
|
@ -469,6 +469,128 @@ def test_transform_on_delete_cascade_does_not_delete_records(
|
|||
assert fresh_db.conn.execute("PRAGMA foreign_keys").fetchone()[0]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("on_delete", ["CASCADE", "SET NULL", "SET DEFAULT", "cascade"])
|
||||
def test_transform_in_transaction_refuses_destructive_on_delete(fresh_db, on_delete):
|
||||
# PRAGMA foreign_keys is a no-op inside a transaction, so transforming a
|
||||
# table referenced by ON DELETE CASCADE / SET NULL / SET DEFAULT foreign
|
||||
# keys inside an open transaction would fire those actions when the old
|
||||
# table is dropped - transform() should refuse instead
|
||||
fresh_db.conn.execute("PRAGMA foreign_keys=ON")
|
||||
fresh_db.executescript("""
|
||||
CREATE TABLE authors (id INTEGER PRIMARY KEY, name TEXT);
|
||||
CREATE TABLE books (
|
||||
id INTEGER PRIMARY KEY,
|
||||
title TEXT,
|
||||
author_id INTEGER REFERENCES authors(id) ON DELETE {}
|
||||
);
|
||||
""".format(on_delete))
|
||||
fresh_db["authors"].insert({"id": 1, "name": "Ursula K. Le Guin"})
|
||||
fresh_db["books"].insert({"id": 1, "title": "The Dispossessed", "author_id": 1})
|
||||
previous_schema = fresh_db["authors"].schema
|
||||
with fresh_db.atomic():
|
||||
with pytest.raises(TransactionError) as excinfo:
|
||||
fresh_db["authors"].transform(rename={"name": "author_name"})
|
||||
message = str(excinfo.value)
|
||||
assert "books" in message
|
||||
assert "ON DELETE {}".format(on_delete.upper()) in message
|
||||
# Nothing should have changed
|
||||
assert fresh_db["authors"].schema == previous_schema
|
||||
assert list(fresh_db["books"].rows) == [
|
||||
{"id": 1, "title": "The Dispossessed", "author_id": 1}
|
||||
]
|
||||
assert fresh_db.conn.execute("PRAGMA foreign_keys").fetchone()[0]
|
||||
|
||||
|
||||
def test_transform_in_transaction_refuses_self_referential_cascade(fresh_db):
|
||||
# The copied table carries a foreign key referencing the original table
|
||||
# name, so a self-referential cascade would wipe the copy too
|
||||
fresh_db.conn.execute("PRAGMA foreign_keys=ON")
|
||||
fresh_db.executescript("""
|
||||
CREATE TABLE categories (
|
||||
id INTEGER PRIMARY KEY,
|
||||
name TEXT,
|
||||
parent_id INTEGER REFERENCES categories(id) ON DELETE CASCADE
|
||||
);
|
||||
""")
|
||||
fresh_db["categories"].insert_all(
|
||||
[
|
||||
{"id": 1, "name": "Fiction", "parent_id": None},
|
||||
{"id": 2, "name": "Science Fiction", "parent_id": 1},
|
||||
]
|
||||
)
|
||||
with fresh_db.atomic():
|
||||
with pytest.raises(TransactionError) as excinfo:
|
||||
fresh_db["categories"].transform(rename={"name": "title"})
|
||||
assert "categories" in str(excinfo.value)
|
||||
assert fresh_db["categories"].count == 2
|
||||
|
||||
|
||||
def test_transform_in_transaction_allowed_with_no_action_foreign_key(fresh_db):
|
||||
# An inbound foreign key without a destructive ON DELETE action is safe
|
||||
# inside a transaction thanks to PRAGMA defer_foreign_keys
|
||||
fresh_db.conn.execute("PRAGMA foreign_keys=ON")
|
||||
fresh_db.executescript("""
|
||||
CREATE TABLE authors (id INTEGER PRIMARY KEY, name TEXT);
|
||||
CREATE TABLE books (
|
||||
id INTEGER PRIMARY KEY,
|
||||
title TEXT,
|
||||
author_id INTEGER REFERENCES authors(id)
|
||||
);
|
||||
""")
|
||||
fresh_db["authors"].insert({"id": 1, "name": "Ursula K. Le Guin"})
|
||||
fresh_db["books"].insert({"id": 1, "title": "The Dispossessed", "author_id": 1})
|
||||
with fresh_db.atomic():
|
||||
fresh_db["authors"].transform(rename={"name": "author_name"})
|
||||
assert list(fresh_db["authors"].rows) == [
|
||||
{"id": 1, "author_name": "Ursula K. Le Guin"}
|
||||
]
|
||||
assert list(fresh_db["books"].rows) == [
|
||||
{"id": 1, "title": "The Dispossessed", "author_id": 1}
|
||||
]
|
||||
assert fresh_db.conn.execute("PRAGMA foreign_keys").fetchone()[0]
|
||||
|
||||
|
||||
def test_transform_in_transaction_allowed_for_child_table(fresh_db):
|
||||
# The table being transformed only has an outbound foreign key - dropping
|
||||
# it fires no ON DELETE actions, so this is allowed inside a transaction
|
||||
fresh_db.conn.execute("PRAGMA foreign_keys=ON")
|
||||
fresh_db.executescript("""
|
||||
CREATE TABLE authors (id INTEGER PRIMARY KEY, name TEXT);
|
||||
CREATE TABLE books (
|
||||
id INTEGER PRIMARY KEY,
|
||||
title TEXT,
|
||||
author_id INTEGER REFERENCES authors(id) ON DELETE CASCADE
|
||||
);
|
||||
""")
|
||||
fresh_db["authors"].insert({"id": 1, "name": "Ursula K. Le Guin"})
|
||||
fresh_db["books"].insert({"id": 1, "title": "The Dispossessed", "author_id": 1})
|
||||
with fresh_db.atomic():
|
||||
fresh_db["books"].transform(rename={"title": "book_title"})
|
||||
assert list(fresh_db["books"].rows) == [
|
||||
{"id": 1, "book_title": "The Dispossessed", "author_id": 1}
|
||||
]
|
||||
|
||||
|
||||
def test_transform_in_transaction_allowed_with_foreign_keys_off(fresh_db):
|
||||
# With PRAGMA foreign_keys off (the default) no cascades can fire, so
|
||||
# transform inside a transaction is safe even with a CASCADE schema
|
||||
fresh_db.executescript("""
|
||||
CREATE TABLE authors (id INTEGER PRIMARY KEY, name TEXT);
|
||||
CREATE TABLE books (
|
||||
id INTEGER PRIMARY KEY,
|
||||
title TEXT,
|
||||
author_id INTEGER REFERENCES authors(id) ON DELETE CASCADE
|
||||
);
|
||||
""")
|
||||
fresh_db["authors"].insert({"id": 1, "name": "Ursula K. Le Guin"})
|
||||
fresh_db["books"].insert({"id": 1, "title": "The Dispossessed", "author_id": 1})
|
||||
with fresh_db.atomic():
|
||||
fresh_db["authors"].transform(rename={"name": "author_name"})
|
||||
assert list(fresh_db["books"].rows) == [
|
||||
{"id": 1, "title": "The Dispossessed", "author_id": 1}
|
||||
]
|
||||
|
||||
|
||||
def test_transform_add_foreign_keys_from_scratch(fresh_db):
|
||||
_add_country_city_continent(fresh_db)
|
||||
fresh_db["places"].insert(_CAVEAU)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue