Repository: gobuffalo/pop (go)

Issue:
pop.Connection.Transaction can leak connections
## Description

If a user's code panics within a [`pop.Connection.Transaction`](https://github.com/gobuffalo/pop/blob/main/connection.go#L150-L174) closure, `.Rollback` nor `.Commit` will not be called resulting in a leaked SQL connection.

### Steps to Reproduce the Problem

```go
func DoThing(conn *pop.Connection) {
    _ = conn.Transaction(func(tx *pop.Connection) error {
        panic("Oh no, this is probably due to a null pointer exception!")
    })
}
```

### Expected Behavior

`.Transaction` should `recover` panics and attempt to call `.Rollback` before re-panicking the error.

### Actual Behavior

A SQL connection is silently leaked. Depending on your connection pool setting, you might not even notice.

### Info

This issue is not specific to an OS and is observable on the latest `main` commit (4e72fd8f35449c084bd44fee9eedf64ebd2b62b4) as of writing

Pull request description:
catch panic within transaction to complete rollback (fixes #748)
`Connection.Transaction()` starts a transaction before executing a given inner function and then commits or rollback the transaction controlled by the return error value of the inner function. However, as reported in #748, it just exits without committing or rolling back the current transaction so the transaction (and lock status) could remain as open (it could depend on the behavior of the backing database engine).

The current behavior of the method is:

```
=== RUN   Test_Connection_Transaction
[POP] 2022/09/22 20:08:51 sql - --- BEGIN Transaction ---
--- FAIL: Test_Connection_Transaction (0.00s)
panic: handler panic [recovered]
	panic: handler panic
<...>
```

The fixed behavior by this PR is:

```
=== RUN   Test_Connection_Transaction
[POP] 2022/09/22 20:11:01 sql - --- BEGIN Transaction ---
[POP] 2022/09/22 20:11:01 sql - --- ROLLBACK Transaction (inner function panic) 
```

Additionally, this PR added `log(logging.SQL,...)` for begin, commit, and rollback actions so the transaction status could be easily observed along with SQL statements when the user enabled SQL logging.


fixes #748

Patch:
diff --git a/connection.go b/connection.go
index 932b477..f51fd36 100644
--- a/connection.go
+++ b/connection.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/gobuffalo/pop/v6/internal/defaults"
 	"github.com/gobuffalo/pop/v6/internal/randx"
+	"github.com/gobuffalo/pop/v6/logging"
 )
 
 // Connections contains all available connections
@@ -151,21 +152,37 @@ func (c *Connection) Close() error {
 // returns an error then the transaction will be rolled back, otherwise the transaction
 // will automatically commit at the end.
 func (c *Connection) Transaction(fn func(tx *Connection) error) error {
-	return c.Dialect.Lock(func() error {
+	return c.Dialect.Lock(func() (err error) {
 		var dberr error
+
+		log(logging.SQL, "--- BEGIN Transaction ---")
 		cn, err := c.NewTransaction()
 		if err != nil {
 			return err
 		}
+
+		defer func() {
+			if ex := recover(); ex != nil {
+				log(logging.SQL, "--- ROLLBACK Transaction (inner function panic) ---")
+				dberr = cn.TX.Rollback()
+				if dberr != nil {
+					err = fmt.Errorf("database error while inner panic rollback: %w", dberr)
+				}
+				err = fmt.Errorf("transaction was rolled back due to inner panic")
+			}
+		}()
+
 		err = fn(cn)
 		if err != nil {
+			log(logging.SQL, "--- ROLLBACK Transaction ---")
 			dberr = cn.TX.Rollback()
 		} else {
+			log(logging.SQL, "--- END Transaction ---")
 			dberr = cn.TX.Commit()
 		}
 
 		if dberr != nil {
-			return fmt.Errorf("error committing or rolling back transaction: %w", dberr)
+			return fmt.Errorf("database error on committing or rolling back transaction: %w", dberr)
 		}
 
 		return err


Test patch:
diff --git a/connection_test.go b/connection_test.go
index a923a28..55c801e 100644
--- a/connection_test.go
+++ b/connection_test.go
@@ -5,6 +5,7 @@ package pop
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -55,7 +56,7 @@ func Test_Connection_Open_BadDriver(t *testing.T) {
 	r.Error(err)
 }
 
-func Test_Connection_Transaction(t *testing.T) {
+func Test_Connection_NewTransaction(t *testing.T) {
 	r := require.New(t)
 	ctx := context.WithValue(context.Background(), "test", "test")
 
@@ -97,3 +98,35 @@ func Test_Connection_Transaction(t *testing.T) {
 		r.NoError(tx.TX.Rollback())
 	})
 }
+
+func Test_Connection_Transaction(t *testing.T) {
+	r := require.New(t)
+
+	c, err := NewConnection(&ConnectionDetails{
+		URL: "sqlite://file::memory:?_fk=true",
+	})
+	r.NoError(err)
+	r.NoError(c.Open())
+
+	t.Run("Success", func(t *testing.T) {
+		err = c.Transaction(func(c *Connection) error {
+			return nil
+		})
+		r.NoError(err)
+	})
+
+	t.Run("Failed", func(t *testing.T) {
+		err = c.Transaction(func(c *Connection) error {
+			return fmt.Errorf("failed")
+		})
+		r.ErrorContains(err, "failed")
+	})
+
+	t.Run("Panic", func(t *testing.T) {
+		err = c.Transaction(func(c *Connection) error {
+			panic("inner function panic")
+		})
+		r.ErrorContains(err, "panic")
+		r.ErrorContains(err, "rolled back")
+	})
+}