Don't reverse steps on migration failure if backend supports transactional DDL

For fully transactional backends reversing migrations steps is unecessary:
everything gets rolled back at the end anyway.
If the reverse steps also fail the script output contains multiple tracebacks
which is unexpected and hard to interpret.

For MySQL (and any other backends without a transactional DDL),
we retain the existing behaviour of running the reverse steps to try to undo
any steps applied so far. This will hopefully leave the database in a
consistent state.

4 files changed, 88 insertions, 11 deletions

diff --git a/yoyo/backends.py b/yoyo/backends.py
index f5b00db..bce98f6 100644
--- a/yoyo/backends.py
+++ b/yoyo/backends.py
@@ -87,7 +87,13 @@ class SavepointTransactionManager(TransactionManager):
         self.backend.savepoint(self.id)
 
     def _do_commit(self):
-        self.backend.savepoint_release(self.id)
+        """
+        This does nothing.
+
+        Trying to the release savepoint here could cause an database error in
+        databases where DDL queries cause the transaction to be committed
+        and all savepoints released.
+        """
 
     def _do_rollback(self):
         self.backend.savepoint_rollback(self.id)
diff --git a/yoyo/migrations.py b/yoyo/migrations.py
index fd12c4d..09e60ad 100755
--- a/yoyo/migrations.py
+++ b/yoyo/migrations.py
@@ -100,14 +100,18 @@ class Migration(object):
                 getattr(step, direction)(backend, force)
                 executed_steps.append(step)
             except backend.DatabaseError:
-                backend.connection.rollback()
                 exc_info = sys.exc_info()
-                try:
-                    for step in reversed(executed_steps):
-                        getattr(step, reverse)(backend)
-                except backend.DatabaseError:
-                    logger.exception(
-                        'Database error when reversing %s of step', direction)
+
+                if not backend.has_transactional_ddl:
+                    # Any DDL statements that have been executed have been
+                    # committed. Go through the rollback steps to undo these
+                    # inasmuch is possible.
+                    try:
+                        for step in reversed(executed_steps):
+                            getattr(step, reverse)(backend)
+                    except backend.DatabaseError:
+                        logger.exception('Could not %s step %s',
+                                         direction, step.id)
                 reraise(exc_info[0], exc_info[1], exc_info[2])
 
 
diff --git a/yoyo/tests/test_backends.py b/yoyo/tests/test_backends.py
index 2509ecc..4b9850f 100644
--- a/yoyo/tests/test_backends.py
+++ b/yoyo/tests/test_backends.py
@@ -18,8 +18,11 @@ class TestTransactionHandling(object):
                 backend.execute("CREATE TABLE _yoyo_t "
                                 "(id CHAR(1) primary key)")
         yield backend
-        with backend.transaction():
-            backend.execute("DROP TABLE _yoyo_t")
+        backend.rollback()
+        for table in (backend.list_tables()):
+            if table.startswith('_yoyo'):
+                with backend.transaction():
+                    backend.execute("DROP TABLE {}".format(table))
 
     def test_it_commits(self, backend):
         with backend.transaction():
@@ -61,3 +64,30 @@ class TestTransactionHandling(object):
                     backends.MySQLBackend: False}
         if backend.__class__ in expected:
             assert backend.has_transactional_ddl is expected[backend.__class__]
+
+    def test_non_transactional_ddl_behaviour(self, backend):
+        """
+        DDL queries in MySQL commit the current transaction,
+        but it still seems to respect a subsequent rollback.
+
+        We don't rely on this behaviour, but it's weird and worth having
+        a test to document how it works and flag up in future should a new
+        backend do things differently
+        """
+        if backend.has_transactional_ddl:
+            return
+
+        with backend.transaction() as trans:
+            backend.execute("CREATE TABLE _yoyo_a (id INT)")  # implicit commit
+            backend.execute("INSERT INTO _yoyo_a VALUES (1)")
+            backend.execute("CREATE TABLE _yoyo_b (id INT)")  # implicit commit
+            backend.execute("INSERT INTO _yoyo_b VALUES (1)")
+            trans.rollback()
+
+        count_a = backend.execute("SELECT COUNT(1) FROM _yoyo_a")\
+                .fetchall()[0][0]
+        assert count_a == 1
+
+        count_b = backend.execute("SELECT COUNT(1) FROM _yoyo_b")\
+                .fetchall()[0][0]
+        assert count_b == 0
diff --git a/yoyo/tests/test_migrations.py b/yoyo/tests/test_migrations.py
index 4a2c303..c346764 100644
--- a/yoyo/tests/test_migrations.py
+++ b/yoyo/tests/test_migrations.py
@@ -20,7 +20,7 @@ from yoyo import read_migrations
 from yoyo import exceptions
 from yoyo import ancestors, descendants
 
-from yoyo.tests import with_migrations, dburi
+from yoyo.tests import with_migrations, migrations_dir, dburi
 from yoyo.migrations import topological_sort, MigrationList
 
 
@@ -121,6 +121,43 @@ def test_rollbackignores_errors(tmpdir):
     assert cursor.fetchall() == []
 
 
+def test_migration_is_committed(backend_fixture):
+    with migrations_dir('step("CREATE TABLE _yoyo_test (id INT)")') as tmpdir:
+        migrations = read_migrations(tmpdir)
+        backend_fixture.apply_migrations(migrations)
+
+    backend_fixture.rollback()
+    rows = backend_fixture.execute("SELECT * FROM _yoyo_test").fetchall()
+    assert list(rows) == []
+
+
+def test_rollback_happens_on_step_failure(backend_fixture):
+    with migrations_dir('''
+                        step("",
+                             "CREATE TABLE _yoyo_is_rolledback (i INT)"),
+                        step("CREATE TABLE _yoyo_test (s VARCHAR(100))",
+                             "DROP TABLE _yoyo_test")
+                        step("invalid sql!")''') as tmpdir:
+        migrations = read_migrations(tmpdir)
+        with pytest.raises(backend_fixture.DatabaseError):
+            backend_fixture.apply_migrations(migrations)
+
+    # The _yoyo_test table should have either been deleted (transactional ddl)
+    # or dropped (non-transactional-ddl)
+    with pytest.raises(backend_fixture.DatabaseError):
+        backend_fixture.execute("SELECT * FROM _yoyo_test")
+
+    # Transactional DDL: rollback steps not executed
+    if backend_fixture.has_transactional_ddl:
+        with pytest.raises(backend_fixture.DatabaseError):
+            backend_fixture.execute("SELECT * FROM _yoyo_is_rolledback")
+
+    # Non-transactional DDL: ensure the rollback steps were executed
+    else:
+        cursor = backend_fixture.execute("SELECT * FROM _yoyo_is_rolledback")
+        assert list(cursor.fetchall()) == []
+
+
 @with_migrations(
     '''
     step("CREATE TABLE _yoyo_test (id INT)")