diff --git a/docs/changelog.rst b/docs/changelog.rst index 87a1ab7..ff78738 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -19,6 +19,7 @@ Unreleased - 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. - Documented a limitation of ``db.query()``: a ``PRAGMA`` statement that returns no rows raises a ``ValueError`` but still takes effect, because PRAGMA statements run outside the savepoint guard used to roll back other rejected statements. Use ``db.execute()`` for row-less PRAGMA statements. +- ``db.add_foreign_keys()`` no longer silently ignores requested ``ON DELETE``/``ON UPDATE`` actions when a foreign key with the same columns already exists - it raises ``AlterError`` suggesting ``table.transform()``, since the actions of an existing foreign key cannot be changed in place. Exact duplicates, including actions, are still skipped so repeated calls stay idempotent. The method also now validates that compound foreign keys have the same number of columns on both sides, instead of silently discarding the extra columns. - ``db.ensure_autocommit_on()`` now raises ``TransactionError`` if called while a transaction is open. Assigning ``isolation_level`` commits any pending transaction as a side effect, so entering the block silently committed the caller's open transaction and made a later ``rollback()`` a no-op. - ``sqlite-utils migrate --stop-before`` now exits with an error if the named migration has already been applied. Previously the name passed validation but was only checked against pending migrations, so every migration after it was silently applied - the exact outcome ``--stop-before`` exists to prevent. ``Migrations.apply(db, stop_before=...)`` raises ``ValueError`` in the same situation, before applying anything. - Fixed a regression where ``table.insert(..., pk=..., alter=True)`` raised ``InvalidColumns`` if the primary key column did not exist in the table yet. With ``alter=True`` the check now waits until the record keys are known, so a pk column supplied by the records is added by the alter as it was in 3.x. A pk column found in neither the table nor the records still raises ``InvalidColumns``. diff --git a/docs/python-api.rst b/docs/python-api.rst index 516e2aa..11af1f4 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -1636,6 +1636,8 @@ Here's an example adding two foreign keys at once: This method runs the same checks as ``.add_foreign_keys()`` and will raise ``sqlite_utils.db.AlterError`` if those checks fail. +Foreign keys that already exist are silently skipped, so repeated calls are idempotent - but only if they match exactly. Requesting a foreign key that exists with different ``ON DELETE``/``ON UPDATE`` actions raises ``AlterError``: use ``table.transform()`` to change the actions of an existing foreign key. + .. _python_api_index_foreign_keys: Adding indexes for all foreign keys diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 90e7a83..eb59c50 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -1786,6 +1786,11 @@ class Database: if isinstance(other_column_or_columns, str) else tuple(other_column_or_columns) ) + if len(columns) != len(other_columns): + raise ValueError( + "Compound foreign key must have the same number of " + "columns on both sides" + ) if len(columns) == 1: fk_object = ForeignKey( table, columns[0], other_table, other_columns[0] @@ -1825,10 +1830,11 @@ class Database: other_column, other_table ) ) - # We will silently skip foreign keys that exist already + # Silently skip foreign keys that exist already - but only if + # they match exactly, including ON DELETE/ON UPDATE actions columns_folded = tuple(fold_identifier_case(c) for c in columns) other_columns_folded = tuple(fold_identifier_case(c) for c in other_columns) - if not any( + existing = [ fk for fk in table_obj.foreign_keys if tuple(fold_identifier_case(c) for c in fk.columns) == columns_folded @@ -1836,8 +1842,21 @@ class Database: == fold_identifier_case(other_table) and tuple(fold_identifier_case(c) for c in fk.other_columns) == other_columns_folded - ): + ] + if not existing: foreign_keys_to_create.append(fk_object) + elif any( + fk.on_delete != fk_object.on_delete + or fk.on_update != fk_object.on_update + for fk in existing + ): + raise AlterError( + "Foreign key already exists for {} => {}.{} but with " + "different ON DELETE/ON UPDATE actions - use " + "table.transform() to change them".format( + ", ".join(columns), other_table, ", ".join(other_columns) + ) + ) # Group them by table by_table: Dict[str, List[ForeignKey]] = {} diff --git a/tests/test_foreign_keys.py b/tests/test_foreign_keys.py index d7c20f6..b37d374 100644 --- a/tests/test_foreign_keys.py +++ b/tests/test_foreign_keys.py @@ -643,3 +643,49 @@ def test_create_table_mixed_foreign_keys_with_string(fresh_db): ) fks = {fk.column: fk.other_table for fk in fresh_db["books"].foreign_keys} assert fks == {"author_id": "authors", "publisher_id": "publishers"} + + +def test_add_foreign_keys_existing_with_different_actions_errors(fresh_db): + # Requesting an existing foreign key with different ON DELETE/ON UPDATE + # actions was silently skipped, dropping the requested change + fresh_db["authors"].insert({"id": 1}, pk="id") + fresh_db["books"].insert( + {"id": 1, "author_id": 1}, + pk="id", + foreign_keys=[("author_id", "authors", "id")], + ) + with pytest.raises(AlterError) as ex: + fresh_db.add_foreign_keys( + [ForeignKey("books", "author_id", "authors", "id", on_delete="CASCADE")] + ) + assert "ON DELETE" in str(ex.value) + assert fresh_db["books"].foreign_keys[0].on_delete == "NO ACTION" + + +def test_add_foreign_keys_identical_existing_is_noop(fresh_db): + # An exact match, including actions, is silently skipped so repeated + # calls stay idempotent + fresh_db["authors"].insert({"id": 1}, pk="id") + fresh_db["books"].insert({"id": 1, "author_id": 1}, pk="id") + fresh_db["books"].add_foreign_key("author_id", "authors", "id", on_delete="CASCADE") + fresh_db.add_foreign_keys( + [ForeignKey("books", "author_id", "authors", "id", on_delete="CASCADE")] + ) + fks = fresh_db["books"].foreign_keys + assert len(fks) == 1 + assert fks[0].on_delete == "CASCADE" + + +def test_add_foreign_keys_compound_column_count_mismatch_errors(fresh_db): + # Previously the extra other-column was silently discarded, creating + # a single-column foreign key to just ("id") + fresh_db["departments"].insert( + {"campus": "north", "code": "cs"}, pk=("campus", "code") + ) + fresh_db["courses"].insert({"id": 1, "campus": "north"}, pk="id") + with pytest.raises(ValueError) as ex: + fresh_db.add_foreign_keys( + [("courses", ("campus",), "departments", ("campus", "code"))] + ) + assert "same number of columns" in str(ex.value) + assert fresh_db["courses"].foreign_keys == []