Fixes for final review in issue #769

docs/changelog.rst

@@ -18,6 +18,20 @@ Unreleased
 - Fixed an ``IndexError`` from ``table.insert(..., pk=..., ignore=True)`` when an ignored insert followed writes to another table on the same connection. ``last_pk`` is now populated from the explicit primary key value instead of looking up a stale ``lastrowid``. (:issue:`554`)
 - 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 exception masking when a statement destroys the enclosing transaction. An error such as a ``RAISE(ROLLBACK)`` trigger or ``INSERT OR ROLLBACK`` conflict rolls back the whole transaction, destroying every savepoint - the cleanup in ``db.atomic()`` and ``db.query()`` then failed with ``OperationalError: no such savepoint`` (or ``cannot rollback - no transaction is active``), hiding the original ``IntegrityError`` from code that tried to catch it. Cleanup now checks whether a transaction is still open first, so the original exception propagates.
+- ``sqlite-utils migrate --list`` is now read-only even when the migrations file uses the legacy ``sqlite_migrate.Migrations`` class, whose listing methods create the ``_sqlite_migrations`` table as a side effect. The listing now runs inside a transaction that is rolled back.
+- ``sqlite-utils insert ... --pk <missing column>`` and ``sqlite-utils extract <missing column>`` now show a clean ``Error:`` message instead of a raw Python traceback. The ``extract`` command also shows a clean error when pointed at a view.
+- Fixed a bug where running ``table.extract()`` more than once against the same lookup table inserted duplicate rows for values containing ``null`` - SQLite unique indexes treat ``NULL`` values as distinct, so ``INSERT OR IGNORE`` alone could not dedupe them. Each repeat extract added another copy that nothing referenced. The insert now uses an ``IS``-based ``NOT EXISTS`` guard so ``null``-containing rows match existing lookup rows.
+- ``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``.
+- 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.
+- ``ForeignKey`` objects are hashable again. The 4.0 change from ``namedtuple`` to dataclass accidentally made them unhashable, breaking patterns like ``set(table.foreign_keys)`` that worked in 3.x. ``ForeignKey`` is now a frozen dataclass - immutable and hashable, like the namedtuple was.
+- Fixed a bug where compound primary key columns were returned in table column order instead of ``PRIMARY KEY`` declaration order. For a table declared as ``CREATE TABLE other (b TEXT, a TEXT, PRIMARY KEY (a, b))`` an implicit ``FOREIGN KEY (x, y) REFERENCES other`` was introspected as referencing ``(b, a)`` when SQLite resolves it as ``(a, b)`` - running ``transform()`` on such a table then rewrote the schema with the inverted column order, silently reversing the meaning of the constraint and causing foreign key errors on valid data. ``table.pks``, compound foreign key guessing and ``transform()`` now all use the primary key declaration order, and ``transform()`` no longer reorders a compound ``PRIMARY KEY (b, a)`` into table column order.
 
 .. _v4_0rc3:
 

docs/cli.rst

@@ -1326,6 +1326,8 @@ A progress bar is displayed when inserting data from a file. You can hide the pr
 
 By default, column types are automatically detected for CSV or TSV files - resulting in a mix of ``TEXT``, ``INTEGER`` and ``REAL`` columns. To disable type detection and treat all columns as ``TEXT``, use the ``--no-detect-types`` option.
 
+Detected types are only applied when the table is created by the command. Inserting CSV or TSV data into a table that already exists leaves the existing column types unchanged - values are inserted using the table's existing schema.
+
 For example, given a ``creatures.csv`` file containing this:
 
 .. code-block::

docs/migrations.rst

@@ -157,7 +157,7 @@ You can also target a specific migration set using ``migration_set:migration_nam
 
 The ``--stop-before`` option can be passed more than once.
 
-If a ``--stop-before`` value does not match any known migration the command exits with an error, rather than silently applying everything.
+If a ``--stop-before`` value does not match any known migration the command exits with an error, rather than silently applying everything. Naming a migration that has already been applied is also an error - stopping before it is impossible to honor - and no pending migrations are applied.
 
 Verbose output
 ==============

docs/python-api.rst

@@ -233,6 +233,8 @@ The SQL query is executed as soon as ``db.query()`` is called. The resulting row
 
 ``db.query()`` can only be used with SQL that returns rows. Passing a statement that returns no rows - an ``INSERT`` or ``UPDATE`` without a ``RETURNING`` clause, for example - will raise a ``ValueError``. The rejected statement is rolled back, so it has no effect on the database. Use :ref:`db.execute() <python_api_execute>` for those statements instead.
 
+There is one exception to the rolled-back guarantee: a ``PRAGMA`` statement that returns no rows, such as ``PRAGMA user_version = 5``, still raises a ``ValueError`` but will already have taken effect. Some PRAGMA statements refuse to run inside a transaction, so PRAGMAs are executed outside the savepoint that is used to roll back other rejected statements. Use ``db.execute()`` for PRAGMA statements that do not return rows.
+
 If a query returns more than one column with the same name - a join between two tables that share column names, for example - later occurrences are renamed with a numeric suffix, so every value is included in the dictionary:
 
 .. code-block:: python
@@ -1634,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

docs/upgrading.rst

@@ -95,7 +95,7 @@ Python API changes
     for fk in db["courses"].foreign_keys:
         fk.table, fk.column, fk.other_table, fk.other_column
 
-Attempting the old unpacking or ``fk[0]`` indexing now raises ``TypeError``, so any code using those patterns will fail loudly rather than silently misbehave.
+Attempting the old unpacking or ``fk[0]`` indexing now raises ``TypeError``, so any code using those patterns will fail loudly rather than silently misbehave. Like the old namedtuple, ``ForeignKey`` instances are immutable and hashable - they can be collected into sets and used as dictionary keys. Note that equality now includes the ``on_delete`` and ``on_update`` actions: a ``ForeignKey`` with ``ON DELETE CASCADE`` is not equal to one without.
 
 Compound foreign keys - previously returned as one ``ForeignKey`` per column, misleadingly suggesting several independent single-column keys - are now returned as a single ``ForeignKey`` with ``is_compound=True``. For these the scalar ``column`` and ``other_column`` fields are ``None``; use the ``columns`` and ``other_columns`` tuples instead. Single-column foreign keys are unaffected apart from the class change: ``column``/``other_column`` behave as before and ``columns``/``other_columns`` are one-item tuples.
 

sqlite_utils/cli.py

@@ -13,6 +13,7 @@
     BadMultiValues,
     DEFAULT,
     DescIndex,
+    InvalidColumns,
     NoTable,
     NoView,
     quote_identifier,
@@ -1165,11 +1166,14 @@ def insert_upsert_implementation(
                     db.conn.cursor().executemany(bulk_sql, doc_chunk)
             return
 
+        # table_names() rather than db.table(), which raises NoTable for
+        # views before the error handling below can deal with them
+        table_existed_before_insert = table in db.table_names()
         try:
             db.table(table).insert_all(
                 docs, pk=pk, batch_size=batch_size, alter=alter, **extra_kwargs
             )
-        except NoTable as e:
+        except (NoTable, InvalidColumns) as e:
             raise click.ClickException(str(e))
         except Exception as e:
             if (
@@ -1194,7 +1198,14 @@ def insert_upsert_implementation(
                 )
             else:
                 raise
-        if tracker is not None and db.table(table).exists():
+        # Apply detected types only to a table this command created -
+        # transforming a pre-existing table would rewrite its column types
+        # and corrupt values such as TEXT zip codes with leading zeros
+        if (
+            tracker is not None
+            and not table_existed_before_insert
+            and db.table(table).exists()
+        ):
             db.table(table).transform(types=tracker.types)
 
         # Clean up open file-like objects
@@ -2724,7 +2735,10 @@ def extract(
         fk_column=fk_column,
         rename=dict(rename),
     )
-    db.table(table).extract(**kwargs)
+    try:
+        db.table(table).extract(**kwargs)
+    except (NoTable, InvalidColumns) as e:
+        raise click.ClickException(str(e))
 
 
 @cli.command(name="insert-files")
@@ -3420,7 +3434,14 @@ def migrate(db_path, migrations, stop_before, list_, verbose):
             # Listing is read-only - don't create the database file
             db = sqlite_utils.Database(memory=True)
         _register_db_for_cleanup(db)
-        _display_migration_list(db, migration_sets)
+        # Legacy sqlite-migrate classes create the migrations table from
+        # their pending()/applied() methods - run the listing inside a
+        # transaction and roll it back so --list stays read-only
+        db.begin()
+        try:
+            _display_migration_list(db, migration_sets)
+        finally:
+            db.rollback()
         return
 
     db = sqlite_utils.Database(db_path)
@@ -3452,7 +3473,10 @@ def migrate(db_path, migrations, stop_before, list_, verbose):
     for migration_set in migration_sets:
         matches = _stop_before_for_migration_set(stop_before, migration_set.name)
         if isinstance(migration_set, sqlite_utils.Migrations):
-            migration_set.apply(db, stop_before=matches)
+            try:
+                migration_set.apply(db, stop_before=matches)
+            except ValueError as e:
+                raise click.ClickException(str(e))
         else:
             # Legacy sqlite-migrate Migrations objects take a single string
             # for stop_before, not a list