From 3de8507c6b7d28288d220d5627d4efe4f21b7abc Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 6 Jul 2026 21:47:59 -0700 Subject: [PATCH] insert(pk=, alter=True) can add the pk column from records again The InvalidColumns check added for #732 fired before alter=True had a chance to add the missing pk column - a regression from 3.x, where insert({"id": 5, "a": 2}, pk="id", alter=True) against a table without an id column worked. With alter=True the check is now deferred until the first batch of record keys is known: a pk column supplied by the records passes, one found in neither the table nor the records still raises InvalidColumns before anything is inserted. An empty record iterator with alter=True returns without error, matching the 3.x no-op. Refs https://github.com/simonw/sqlite-utils/issues/769#issuecomment-4900034150 Co-Authored-By: Claude Fable 5 --- docs/changelog.rst | 1 + sqlite_utils/db.py | 19 ++++++++++++++++++- tests/test_create.py | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 80bcfb7..df41884 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. +- 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``. - Fixed a bug where inserting CSV or TSV data into an existing table rewrote that table's column types to match the incoming file. Type detection is the default in 4.0, so ``sqlite-utils insert data.db places places.csv --csv`` against a table with a ``TEXT`` zip code column would convert the column to ``INTEGER`` and corrupt values with leading zeros - ``"01234"`` became ``1234``. Detected types are now only applied when the ``insert`` or ``upsert`` command creates the table. - Fixed ``pks_and_rows_where()`` raising ``AttributeError`` when called on a view, and no longer double-quotes the synthesized ``rowid`` column in its generated SQL - SQLite turns a double-quoted identifier that does not resolve into a string literal, which on a view produced a confusing ``KeyError`` instead of the ``OperationalError`` raised in 3.x. Compound primary keys returned by this method now follow ``PRIMARY KEY`` declaration order. - The ``foreign_keys=`` argument to ``create()`` and ``insert()`` accepts a mixed list of ``ForeignKey`` objects, tuples and column name strings again. In 4.0 pre-releases mixing ``ForeignKey`` objects with tuples raised a ``ValueError`` - a regression from 3.x, where ``ForeignKey`` was a ``namedtuple`` and passed the tuple checks. diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 4c7ef52..faf588f 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -4282,6 +4282,10 @@ class Table(Queryable): if hash_id: pk = hash_id + # pk columns missing from an existing table are an error - unless + # alter=True, where a pk column supplied by the records will be + # added, so validation waits until the record keys are known + deferred_invalid_pk_check = None if pk and not hash_id and self.exists(): pk_cols = [pk] if isinstance(pk, str) else list(pk) existing_columns = self.columns_dict @@ -4291,7 +4295,7 @@ class Table(Queryable): if resolve_casing(col, existing_columns) not in existing_columns ] if missing_pk_cols: - raise InvalidColumns( + invalid_pk_error = InvalidColumns( "Invalid primary key column{} {} for table {} with columns {}".format( "s" if len(missing_pk_cols) > 1 else "", missing_pk_cols, @@ -4299,6 +4303,9 @@ class Table(Queryable): list(existing_columns), ) ) + if not alter: + raise invalid_pk_error + deferred_invalid_pk_check = (missing_pk_cols, invalid_pk_error) if ignore and replace: raise ValueError("Use either ignore=True or replace=True, not both") @@ -4409,6 +4416,16 @@ class Table(Queryable): all_columns = list(sorted(all_columns_set)) if hash_id: all_columns.insert(0, hash_id) + if deferred_invalid_pk_check is not None: + # alter=True - pk columns the table lacks are valid if + # the records supply them, otherwise raise the error + missing_pk_cols, invalid_pk_error = deferred_invalid_pk_check + record_columns = {column: True for column in all_columns} + if any( + resolve_casing(col, record_columns) not in record_columns + for col in missing_pk_cols + ): + raise invalid_pk_error else: if not list_mode: for record in cast(List[Dict[str, Any]], chunk): diff --git a/tests/test_create.py b/tests/test_create.py index 2788920..42fa1c4 100644 --- a/tests/test_create.py +++ b/tests/test_create.py @@ -932,6 +932,23 @@ def test_insert_all_pk_not_in_records_raises(fresh_db, num_rows): fresh_db.conn.execute("CREATE TABLE t (a TEXT, b INT, PRIMARY KEY (a, b))") rows = [{"a": "x{}".format(i), "b": i} for i in range(num_rows)] + with pytest.raises(InvalidColumns) as ex: + fresh_db["t"].insert_all(rows, pk="not_a_column") + + assert ex.value.args == ( + "Invalid primary key column ['not_a_column'] for table t with columns ['a', 'b']", + ) + assert fresh_db["t"].count == 0 + + +@pytest.mark.parametrize("num_rows", (1, 2, 3, 10)) +def test_insert_all_pk_not_in_records_alter_raises(fresh_db, num_rows): + # With alter=True the check is deferred until the record keys are + # known - a pk column that is in neither the table nor the records + # still raises + fresh_db.conn.execute("CREATE TABLE t (a TEXT, b INT, PRIMARY KEY (a, b))") + rows = [{"a": "x{}".format(i), "b": i} for i in range(num_rows)] + with pytest.raises(InvalidColumns) as ex: fresh_db["t"].insert_all(rows, pk="not_a_column", alter=True) @@ -941,6 +958,26 @@ def test_insert_all_pk_not_in_records_raises(fresh_db, num_rows): assert fresh_db["t"].count == 0 +def test_insert_pk_in_records_with_alter_adds_column(fresh_db): + # 3.x allowed insert(pk=..., alter=True) to add the pk column from the + # records - the InvalidColumns check must not fire in that case + fresh_db["t"].insert({"a": 1}) + fresh_db["t"].insert({"id": 5, "a": 2}, pk="id", alter=True) + assert fresh_db["t"].columns_dict.keys() == {"a", "id"} + assert list(fresh_db.query("select * from t order by a")) == [ + {"a": 1, "id": None}, + {"a": 2, "id": 5}, + ] + + +def test_insert_all_invalid_pk_alter_empty_records_is_noop(fresh_db): + # With alter=True the pk check needs record keys, so an empty iterator + # returns without error - matching the 3.x no-op for empty inserts + fresh_db.conn.execute("CREATE TABLE t (a TEXT)") + fresh_db["t"].insert_all([], pk="not_a_column", alter=True) + assert fresh_db["t"].count == 0 + + def test_insert_ignore(fresh_db): fresh_db["test"].insert({"id": 1, "bar": 2}, pk="id") # Should raise an error if we try this again