migrate --stop-before an already-applied migration is now an error

The CLI validated --stop-before names against both pending and applied
migrations, but Migrations.apply() only looked for the stop name among
pending ones - naming an applied migration passed validation and then
silently applied every migration after it, the exact outcome the option
exists to prevent. apply() now raises ValueError before applying
anything if a stop_before name matches an applied migration in its set;
names not in the set are still ignored, since unqualified CLI values are
offered to every set. The migrate command reports it as a clean error.

Refs https://github.com/simonw/sqlite-utils/issues/769#issuecomment-4900034150

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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.
+- ``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.

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
 ==============

sqlite_utils/cli.py

@@ -3462,7 +3462,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

sqlite_utils/migrations.py

@@ -96,14 +96,34 @@ class Migrations:
         changes are rolled back, no record is written and the migration stays
         pending. Migrations registered with ``transactional=False`` run
         outside of a transaction.
+
+        :raises ValueError: if a ``stop_before`` name matches a migration in
+          this set that has already been applied - stopping before it is
+          impossible to honor, and no pending migrations are applied
         """
-        self.ensure_migrations_table(db)
         if stop_before is None:
             stop_before_names = set()
         elif isinstance(stop_before, str):
             stop_before_names = {stop_before}
         else:
             stop_before_names = set(stop_before)
+        # A stop_before naming an already-applied migration cannot be
+        # honored - error rather than applying everything after it. Names
+        # not in this set at all are ignored, because unqualified CLI
+        # values are offered to every migration set
+        already_applied = stop_before_names.intersection(
+            migration.name for migration in self.applied(db)
+        )
+        if already_applied:
+            raise ValueError(
+                "Cannot stop before migration{} {} in set '{}' - already "
+                "been applied".format(
+                    "s" if len(already_applied) > 1 else "",
+                    ", ".join(sorted(already_applied)),
+                    self.name,
+                )
+            )
+        self.ensure_migrations_table(db)
         for migration in self.pending(db):
             name = migration.name
             if name in stop_before_names:

tests/test_cli_migrate.py

@@ -463,3 +463,25 @@ def test_list_does_not_upgrade_legacy_migrations_table(two_migrations):
     db2 = sqlite_utils.Database(db_path)
     assert db2["_sqlite_migrations"].pks == ["migration_set", "name"]
     db2.close()
+
+
+def test_stop_before_applied_migration_errors(two_migrations):
+    path, _ = two_migrations
+    db_path = str(path / "test.db")
+    migrations_path = str(path / "foo" / "migrations.py")
+    # Apply everything first
+    first = CliRunner().invoke(
+        sqlite_utils.cli.cli,
+        ["migrate", db_path, migrations_path, "--stop-before", "bar"],
+    )
+    assert first.exit_code == 0
+    # foo is now applied - stopping before it is an error, and bar
+    # must not be applied as a side effect
+    result = CliRunner().invoke(
+        sqlite_utils.cli.cli,
+        ["migrate", db_path, migrations_path, "--stop-before", "foo"],
+    )
+    assert result.exit_code != 0
+    assert "already been applied" in result.output
+    db = sqlite_utils.Database(db_path)
+    assert not db["bar"].exists()

tests/test_migrations.py

@@ -214,3 +214,33 @@ def test_duplicate_migration_name_errors():
             pass
 
     assert "m001" in str(ex.value)
+
+
+def test_stop_before_applied_migration_errors(migrations):
+    # Stopping before a migration that has already been applied is
+    # impossible to honor - previously the stop name was only checked
+    # against pending migrations, so everything after it was applied
+    db = sqlite_utils.Database(memory=True)
+    migrations.apply(db, stop_before="m002")  # applies m001 only
+    with pytest.raises(ValueError) as ex:
+        migrations.apply(db, stop_before="m001")
+    assert "m001" in str(ex.value)
+    assert "already been applied" in str(ex.value)
+    # Nothing else was applied
+    assert not db["cats"].exists()
+
+
+def test_stop_before_applied_migration_errors_before_any_apply(migrations):
+    # The error fires before any pending migration runs, even those that
+    # come before the already-applied stop target in registration order
+    db = sqlite_utils.Database(memory=True)
+    only_second = Migrations("test")
+
+    @only_second()
+    def m002(db):
+        db["cats"].create({"name": str})
+
+    only_second.apply(db)  # m002 applied, m001 still pending
+    with pytest.raises(ValueError):
+        migrations.apply(db, stop_before="m002")
+    assert not db["dogs"].exists()