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

@@ -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
+// Rollback rolls back the provided transaction. If the rollback fails for any
-// the error, if any, of the rollback into a RollbackError.
+// reason a `RollbackError` error is returned wrapping the original error. If no
-//
+// rollback error occurs then the original error is returned.
-// The err parameter must be non-nil.
-//
-//   err = sa.Rollback(tx, err)
 func Rollback(tx *gorp.Transaction, err error) error {
-	return &RollbackError{
+	if txErr := tx.Rollback(); txErr != nil {
-		Err:         err,
+		return &RollbackError{
-		RollbackErr: tx.Rollback(),
+			Err:         err,
+			RollbackErr: txErr,
+		}
 	}
+	return err
 }

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)
+}