Get `add_foreign_keys()` to work without modifying `sqlite_master`

[simonw]

https://github.com/simonw/sqlite-utils/blob/13ebcc575d2547c45e8d31288b71a3242c16b886/sqlite_utils/db.py#L1165-L1174

This is the only place in the code that attempts to modify sqlite_master directly, which fails on some Python installations.

Could this use the .transform() trick instead?

Or automatically switch to that trick if it hits an error?

[simonw]

I'm certain this could work.

It turns out the .transform() method already has code that creates the new table with a copy of foreign keys from the old one - dropping any foreign keys that were specified in the drop_foreign_keys= parameter:

https://github.com/simonw/sqlite-utils/blob/1dc6b5aa644a92d3654f7068110ed7930989ce71/sqlite_utils/db.py#L1850-L1872

Improving this code to support adding foreign keys as well would be pretty simple.

And then the .add_foreign_keys() and .add_foreign_key() methods could be updated to use .transform(...) under the hood instead.

[simonw]

This would help address these issues, among potentially many others:

• https://github.com/simonw/llm/issues/60
• https://github.com/simonw/llm/issues/116
• https://github.com/simonw/llm/issues/123

[simonw]

Looking at the whole of the .add_foreign_keys() method, the first section of it can remain unchanged - it's just a bunch of validation:

https://github.com/simonw/sqlite-utils/blob/13ebcc575d2547c45e8d31288b71a3242c16b886/sqlite_utils/db.py#L1106-L1149

At that point we have foreign_keys_to_create as the ones that are new, but we should instead try to build up a foreign_keys which is both new and old, ready to be passed to .transform().

Here's the rest of that function, which will be replaced by a called to .transform(foreign_keys=foreign_keys):

https://github.com/simonw/sqlite-utils/blob/13ebcc575d2547c45e8d31288b71a3242c16b886/sqlite_utils/db.py#L1151-L1177

[simonw]

Actually I think table.transform() might get the following optional arguments:

    def transform(
        self,
        *,
        # ...
        # This one exists already:
        drop_foreign_keys: Optional[Iterable] = None,
        # These two are new. This one specifies keys to add:
        add_foreign_keys: Optional[ForeignKeysType] = None,
        # Or this one causes them all to be replaced with the new definitions:
        foreign_keys: Optional[ForeignKeysType] = None,

There should be validation that forbids you from using foreign_keys= at the same time as either drop_foreign_keys= or add_foreign_keys= because the point of foreign_keys= is to define the keys for the new table all in one go.

[simonw]

Maybe this:

        drop_foreign_keys: Optional[Iterable] = None,

Should be this:

        drop_foreign_keys: Optional[Iterable[str]] = None,

Because it takes a list of column names that should have their foreign keys dropped.

[simonw]

As a reminder: https://github.com/simonw/sqlite-utils/blob/1dc6b5aa644a92d3654f7068110ed7930989ce71/sqlite_utils/db.py#L159-L165

[simonw]

I'm inclined to just go with the .transform() method and not attempt to keep around the method that involves updating sqlite_master and then add code to detect if that's possible (or catch if it fails) and fall back on the other mechanism.

It would be nice to drop some code complexity, plus I don't yet have a way of running automated tests against Python + SQLite versions that exhibit the problem.

[simonw]

An interesting side-effect of this change is that it does result in a slightly different schema - e.g. this test: https://github.com/simonw/sqlite-utils/blob/1dc6b5aa644a92d3654f7068110ed7930989ce71/tests/test_extract.py#L118-L133

Needs updating like so:

diff --git a/tests/test_extract.py b/tests/test_extract.py
index 70ad0cf..fd52534 100644
--- a/tests/test_extract.py
+++ b/tests/test_extract.py
@@ -127,8 +127,7 @@ def test_extract_rowid_table(fresh_db):
     assert fresh_db["tree"].schema == (
         'CREATE TABLE "tree" (\n'
         "   [name] TEXT,\n"
-        "   [common_name_latin_name_id] INTEGER,\n"
-        "   FOREIGN KEY([common_name_latin_name_id]) REFERENCES [common_name_latin_name]([id])\n"
+        "   [common_name_latin_name_id] INTEGER REFERENCES [common_name_latin_name]([id])\n"
         ")"
     )
     assert (

Unfortunately this means it may break other test suites that depend on sqlite-utils that have schema tests like this baked in.

I don't think this should count as a breaking change release though, but it's still worth noting.

[simonw]

Here's a new plugin that brings back the sqlite_master modifying version, for those that can use it:

https://github.com/simonw/sqlite-utils-fast-fks