From 99eeb019849989e2ee82757491d8f96cd73297d2 Mon Sep 17 00:00:00 2001
From: Daniel McCarney <daniel@binaryparadox.net>
Date: Thu, 31 Aug 2017 14:44:25 -0400
Subject: [PATCH] Only wrap error given to `Rollback` when `tx.Rollback()`
 fails. (#3025)

Prior to this commit the `Rollback` function always wrapped the provided
error in a `sa.RollbackError`. This makes it difficult for callers to
test the type of the original error. This commit updates the `Rollback` function to only
return a `sa.RollbackError` when the call to `tx.Rollback()` produces an error.
---
 sa/rollback.go      | 18 +++++++++---------
 sa/rollback_test.go | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 9 deletions(-)
 create mode 100644 sa/rollback_test.go

diff --git a/sa/rollback.go b/sa/rollback.go
index d62105708..f19ced04d 100644
--- a/sa/rollback.go
+++ b/sa/rollback.go
@@ -26,15 +26,15 @@ func (re *RollbackError) Error() string {
 	return fmt.Sprintf("%s (also, while rolling back: %s)", re.Err, re.RollbackErr)
 }
 
-// Rollback rolls back the provided transaction (if err is non-nil) and wraps
-// the error, if any, of the rollback into a RollbackError.
-//
-// The err parameter must be non-nil.
-//
-//   err = sa.Rollback(tx, err)
+// Rollback rolls back the provided transaction. If the rollback fails for any
+// reason a `RollbackError` error is returned wrapping the original error. If no
+// rollback error occurs then the original error is returned.
 func Rollback(tx *gorp.Transaction, err error) error {
-	return &RollbackError{
-		Err:         err,
-		RollbackErr: tx.Rollback(),
+	if txErr := tx.Rollback(); txErr != nil {
+		return &RollbackError{
+			Err:         err,
+			RollbackErr: txErr,
+		}
 	}
+	return err
 }
diff --git a/sa/rollback_test.go b/sa/rollback_test.go
new file mode 100644
index 000000000..9ffc7a666
--- /dev/null
+++ b/sa/rollback_test.go
@@ -0,0 +1,36 @@
+package sa
+
+import (
+	"testing"
+
+	berrors "github.com/letsencrypt/boulder/errors"
+	"github.com/letsencrypt/boulder/test"
+)
+
+func TestRollback(t *testing.T) {
+	sa, _, cleanUp := initSA(t)
+	defer cleanUp()
+
+	tx, _ := sa.dbMap.Begin()
+	// Commit the transaction so that a subsequent Rollback will always fail.
+	_ = tx.Commit()
+
+	innerErr := berrors.NotFoundError("Gone, gone, gone")
+	result := Rollback(tx, innerErr)
+
+	// Since the tx.Rollback will fail we expect the result to be a wrapped error
+	test.AssertNotEquals(t, result, innerErr)
+	if rbErr, ok := result.(*RollbackError); !ok {
+		t.Fatal("Result was not a RollbackError")
+		test.AssertEquals(t, rbErr.Err, innerErr)
+		test.AssertNotNil(t, rbErr.RollbackErr, "RollbackErr was nil")
+	}
+
+	// Create a new transaction and don't commit it this time. The rollback should
+	// succeed.
+	tx, _ = sa.dbMap.Begin()
+	result = Rollback(tx, innerErr)
+
+	// We expect that the err is returned unwrapped.
+	test.AssertEquals(t, result, innerErr)
+}