Transform now refuses to run inside a transaction if destructive foreign keys exist

docs/python-api.rst

@@ -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:
@@ -1996,6 +1997,36 @@ If you want to do something more advanced, you can call the ``table.transform_sq
 
 This method will return a list of SQL statements that should be executed to implement the change. You can then make modifications to that SQL - or add additional SQL statements - before executing it yourself.
 
+.. _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_extract:
 
 Extracting columns into a separate table

sqlite_utils/db.py

@@ -2522,6 +2522,11 @@ def transform(
 
         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 @@ def transform(
         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:

tests/test_transform.py

@@ -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)