From 6069fe9a298a3c886d90d79d092ae8c1e620777d Mon Sep 17 00:00:00 2001
From: Genadi Samokovarov <gsamokovarov@gmail.com>
Date: Thu, 20 Sep 2018 21:15:59 +0300
Subject: [PATCH] Fix the no-error reporting on failed migration applications

We need to return the initial error and the error about whether the
reversal of the transaction was successful.
---
 executor.go                                        | 10 ++--------
 executor_test.go                                   | 14 ++++++++++++++
 gloat_test.go                                      | 13 ++++++++-----
 source_test.go                                     |  5 ++++-
 .../down.sql                                       |  1 +
 .../20180920181906_migration_with_an_error/up.sql  |  4 ++++
 6 files changed, 33 insertions(+), 14 deletions(-)
 create mode 100644 testdata/migrations/20180920181906_migration_with_an_error/down.sql
 create mode 100644 testdata/migrations/20180920181906_migration_with_an_error/up.sql

diff --git a/executor.go b/executor.go
index 24f1e43..cb86f23 100644
--- a/executor.go
+++ b/executor.go
@@ -28,11 +28,6 @@ type SQLExecutor struct {
 
 // Up applies a migration.
 func (e *SQLExecutor) Up(migration *Migration, store Store) error {
-	tx, err := e.db.Begin()
-	if err != nil {
-		return err
-	}
-
 	return e.exec(migration.Options.Transaction, func(tx SQLExecer) error {
 		if _, err := tx.Exec(string(migration.UpSQL)); err != nil {
 			return err
@@ -40,8 +35,6 @@ func (e *SQLExecutor) Up(migration *Migration, store Store) error {
 
 		return store.Insert(migration, tx)
 	})
-
-	return tx.Commit()
 }
 
 // Down reverses a migrations.
@@ -70,7 +63,8 @@ func (e *SQLExecutor) exec(transaction bool, action func(SQLExecer) error) error
 	}
 
 	if err := action(tx); err != nil {
-		return tx.Rollback()
+		defer tx.Rollback()
+		return err
 	}
 
 	return tx.Commit()
diff --git a/executor_test.go b/executor_test.go
index 44ca2c1..a4a4ba8 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -25,6 +25,20 @@ func TestSQLExecutor_Up(t *testing.T) {
 	})
 }
 
+func TestSQLExecutor_Up_Broken(t *testing.T) {
+	td := filepath.Join(dbSrc, "20180920181906_migration_with_an_error")
+
+	exe := NewSQLExecutor(db)
+
+	migration, err := MigrationFromBytes(td, ioutil.ReadFile)
+	assert.Nil(t, err)
+
+	cleanState(func() {
+		err := exe.Up(migration, new(testingStore))
+		assert.Error(t, err)
+	})
+}
+
 func TestSQLExecutor_Down(t *testing.T) {
 	td := filepath.Join(dbSrc, "20170329154959_introduce_domain_model")
 
diff --git a/gloat_test.go b/gloat_test.go
index dc6dec8..a8c2bf2 100644
--- a/gloat_test.go
+++ b/gloat_test.go
@@ -18,10 +18,11 @@ import (
 var (
 	gl Gloat
 
-	db       *sql.DB
-	dbURL    string
-	dbSrc    string
-	dbDriver string
+	db          *sql.DB
+	dbURL       string
+	dbSrc       string
+	dbSrcBroken string
+	dbDriver    string
 )
 
 type testingStore struct{ applied Migrations }
@@ -89,7 +90,7 @@ func TestUnapplied(t *testing.T) {
 
 	migrations, err := gl.Unapplied()
 	assert.Nil(t, err)
-	assert.Len(t, 3, migrations)
+	assert.Len(t, 4, migrations)
 
 	assert.Equal(t, 20170329154959, migrations[0].Version)
 }
@@ -100,6 +101,7 @@ func TestUnapplied_Empty(t *testing.T) {
 			&Migration{Version: 20170329154959},
 			&Migration{Version: 20170511172647},
 			&Migration{Version: 20180905150724},
+			&Migration{Version: 20180920181906},
 		},
 	}
 
@@ -115,6 +117,7 @@ func TestUnapplied_MissingInSource(t *testing.T) {
 			&Migration{Version: 20170329154959},
 			&Migration{Version: 20170511172647},
 			&Migration{Version: 20180905150724},
+			&Migration{Version: 20180920181906},
 		},
 	}
 
diff --git a/source_test.go b/source_test.go
index 0c992ac..b5878df 100644
--- a/source_test.go
+++ b/source_test.go
@@ -24,7 +24,10 @@ func TestFileSystemSourceCollect(t *testing.T) {
 	m3, err := MigrationFromBytes(filepath.Join(td, "20180905150724_concurrent_migration"), ioutil.ReadFile)
 	assert.Nil(t, err)
 
-	expectedMigrations := Migrations{m1, m2, m3}
+	m4, err := MigrationFromBytes(filepath.Join(td, "20180920181906_migration_with_an_error"), ioutil.ReadFile)
+	assert.Nil(t, err)
+
+	expectedMigrations := Migrations{m1, m2, m3, m4}
 	assert.Equal(t, migrations, expectedMigrations)
 }
 
diff --git a/testdata/migrations/20180920181906_migration_with_an_error/down.sql b/testdata/migrations/20180920181906_migration_with_an_error/down.sql
new file mode 100644
index 0000000..55a9f9f
--- /dev/null
+++ b/testdata/migrations/20180920181906_migration_with_an_error/down.sql
@@ -0,0 +1 @@
+DROP TABL users;
diff --git a/testdata/migrations/20180920181906_migration_with_an_error/up.sql b/testdata/migrations/20180920181906_migration_with_an_error/up.sql
new file mode 100644
index 0000000..ef0bb38
--- /dev/null
+++ b/testdata/migrations/20180920181906_migration_with_an_error/up.sql
@@ -0,0 +1,4 @@
+CREATE TABL users (
+    id bigserial PRIMARY KEY NOT NULL
+);
+