feat: add transaction support for nested and singular operations (#269)

Author: yxrxy

adapter.go

@@ -17,7 +17,6 @@ package gormadapter
 import (
 	"context"
 	"database/sql"
-	"errors"
 	"fmt"
 	"runtime"
 	"strings"
@@ -27,6 +26,7 @@ import (
 	"github.com/casbin/casbin/v2/model"
 	"github.com/casbin/casbin/v2/persist"
 	"github.com/glebarez/sqlite"
+	"github.com/pkg/errors"
 	"gorm.io/driver/mysql"
 	"gorm.io/driver/postgres"
 	"gorm.io/driver/sqlserver"
@@ -703,31 +703,61 @@ func (a *Adapter) Transaction(e casbin.IEnforcer, fc func(casbin.IEnforcer) erro
 			}
 		})
 	}
+
+	// check adapter type
+	adapter, ok := e.GetAdapter().(*Adapter)
+	if !ok {
+		return errors.New("expected adapter of type Adapter, but got incompatible type")
+	}
+
+	// check if we're already in a transaction by checking if the current adapter is a transaction adapter
+	if _, isTxAdapter := adapter.db.Statement.ConnPool.(*sql.Tx); isTxAdapter {
+		// we're already in a transaction, just execute the function directly
+		return fc(e)
+	}
+
 	// lock the transactionMu to ensure the transaction is thread-safe
 	a.transactionMu.Lock()
 	defer a.transactionMu.Unlock()
-	var err error
-	// reload policy from database to sync with the transaction
-	defer func() {
-		e.SetAdapter(a.Copy())
-		err = e.LoadPolicy()
+
+	// save original adapter
+	originalAdapter := adapter.Copy()
+
+	// use GORM transaction functionality
+	err := adapter.db.Transaction(func(tx *gorm.DB) error {
+		// create transaction adapter
+		txAdapter, err := NewAdapterByDB(tx)
+		if err != nil {
+			return errors.Wrap(err, "failed to initialize gorm adapter")
+		}
+
+		// temporarily set transaction adapter
+		e.SetAdapter(txAdapter)
+
+		// execute transaction function
+		err = fc(e)
 		if err != nil {
-			panic(err)
+			return errors.Wrap(err, "failed transactional policy operations")
 		}
-	}()
-	copyDB := *a.db
-	tx := copyDB.Begin(opts...)
-	b := a.Copy()
-	// copy enforcer to set the new adapter with transaction tx
-	copyEnforcer := e
-	copyEnforcer.SetAdapter(b)
-	err = fc(copyEnforcer)
+
+		return nil
+	}, opts...)
+
+	// restore original adapter
+	e.SetAdapter(originalAdapter)
+
 	if err != nil {
-		tx.Rollback()
-		return err
+		// LoadPolicy is called only when the transaction encounters an error and fails.
+		// While this operation is expensive, failures are rare due to validation at earlier layers.
+		// When a transaction fails, the in-memory model may be out of sync, so LoadPolicy is needed
+		// to restore consistency by reloading from the database.
+		if loadErr := e.LoadPolicy(); loadErr != nil {
+			return errors.Wrap(loadErr, "failed to load policy after transaction failure")
+		}
+		return errors.Wrap(err, "transaction execution failed")
 	}
-	err = tx.Commit().Error
-	return err
+
+	return nil
 }
 
 // RemovePolicies removes multiple policy rules from the storage.

adapter_test.go

@@ -730,36 +730,125 @@ func TestAddPolicy(t *testing.T) {
 }
 
 func TestTransaction(t *testing.T) {
-	a := initAdapter(t, "mysql", "root:@tcp(127.0.0.1:3306)/", "casbin", "casbin_rule")
-	e, _ := casbin.NewEnforcer("examples/rbac_model.conf", a)
-	err := e.GetAdapter().(*Adapter).Transaction(e, func(e casbin.IEnforcer) error {
-		_, err := e.AddPolicy("jack", "data1", "write")
-		if err != nil {
-			return err
-		}
-		_, err = e.AddPolicy("jack", "data2", "write")
-		//err = errors.New("some error")
-		if err != nil {
+	// create in-memory database
+	db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{})
+	assert.NoError(t, err)
+
+	// create adapter
+	adapter, err := NewAdapterByDB(db)
+	assert.NoError(t, err)
+
+	// create enforcer
+	enforcer, err := casbin.NewEnforcer("examples/rbac_model.conf", adapter)
+	assert.NoError(t, err)
+
+	// load policy
+	err = enforcer.LoadPolicy()
+	assert.NoError(t, err)
+
+	// test 1: basic transaction operation
+	t.Run("Basic Transaction", func(t *testing.T) {
+		err := adapter.Transaction(enforcer, func(e casbin.IEnforcer) error {
+			_, err := e.AddPolicy("alice", "data1", "read")
+			if err != nil {
+				return err
+			}
+			_, err = e.AddPolicy("alice", "data2", "write")
 			return err
-		}
-		return nil
+		})
+		assert.NoError(t, err)
+
+		// verify policies were added successfully
+		ok, _ := enforcer.Enforce("alice", "data1", "read")
+		assert.True(t, ok)
+		ok, _ = enforcer.Enforce("alice", "data2", "write")
+		assert.True(t, ok)
+	})
+
+	// test 2: transaction rollback
+	t.Run("Transaction Rollback", func(t *testing.T) {
+		err := adapter.Transaction(enforcer, func(e casbin.IEnforcer) error {
+			_, err := e.AddPolicy("bob", "data3", "read")
+			if err != nil {
+				return err
+			}
+			// intentionally return error to trigger rollback
+			return assert.AnError
+		})
+		assert.Error(t, err)
+
+		// verify policy was rolled back
+		ok, _ := enforcer.Enforce("bob", "data3", "read")
+		assert.False(t, ok)
+	})
+
+	// test 3: nested transaction
+	t.Run("Nested Transaction", func(t *testing.T) {
+		err := adapter.Transaction(enforcer, func(e casbin.IEnforcer) error {
+			// outer transaction
+			_, err := e.AddPolicy("charlie", "data4", "read")
+			if err != nil {
+				return err
+			}
+
+			// nested transaction
+			return adapter.Transaction(e, func(innerE casbin.IEnforcer) error {
+				_, err := innerE.AddPolicy("charlie", "data5", "write")
+				if err != nil {
+					return err
+				}
+				_, err = innerE.AddPolicy("charlie", "data6", "delete")
+				return err
+			})
+		})
+		assert.NoError(t, err)
+
+		// verify all policies
+		ok, _ := enforcer.Enforce("charlie", "data4", "read")
+		assert.True(t, ok)
+		ok, _ = enforcer.Enforce("charlie", "data5", "write")
+		assert.True(t, ok)
+		ok, _ = enforcer.Enforce("charlie", "data6", "delete")
+		assert.True(t, ok)
+	})
+
+	// test 4: adapter type check
+	t.Run("Adapter Type Check", func(t *testing.T) {
+		// create an incompatible adapter
+		mockEnforcer, _ := casbin.NewEnforcer("examples/rbac_model.conf")
+
+		err := adapter.Transaction(mockEnforcer, func(e casbin.IEnforcer) error {
+			return nil
+		})
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "expected adapter of type Adapter")
 	})
-	if err != nil {
-		return
-	}
 }
 
 func TestTransactionRace(t *testing.T) {
-	a := initAdapter(t, "mysql", "root:@tcp(127.0.0.1:3306)/", "casbin", "casbin_rule")
-	e, _ := casbin.NewEnforcer("examples/rbac_model.conf", a)
+	// create in-memory database for testing
+	db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{})
+	require.NoError(t, err)
+
+	// create adapter
+	a, err := NewAdapterByDB(db)
+	require.NoError(t, err)
+
+	// create enforcer
+	e, err := casbin.NewEnforcer("examples/rbac_model.conf", a)
+	require.NoError(t, err)
+
+	// load policy
+	err = e.LoadPolicy()
+	require.NoError(t, err)
 
 	concurrency := 100
 
 	var g errgroup.Group
 	for i := 0; i < concurrency; i++ {
 		i := i
 		g.Go(func() error {
-			return e.GetAdapter().(*Adapter).Transaction(e, func(e casbin.IEnforcer) error {
+			return a.Transaction(e, func(e casbin.IEnforcer) error {
 				_, err := e.AddPolicy("jack", fmt.Sprintf("data%d", i), "write")
 				if err != nil {
 					return err
@@ -781,16 +870,23 @@ func TestTransactionRace(t *testing.T) {
 }
 
 func TestTransactionWithSavePolicy(t *testing.T) {
-	a := initAdapter(t, "mysql", "root:@tcp(127.0.0.1:3306)/", "casbin", "casbin_rule")
-	e, _ := casbin.NewEnforcer("examples/rbac_model.conf", a)
-	defer func() {
-		e.ClearPolicy()
-		err := e.SavePolicy()
-		if err != nil {
-			t.Fatalf("save policy err %v", err)
-		}
-	}()
-	err := e.GetAdapter().(*Adapter).Transaction(e, func(e casbin.IEnforcer) error {
+	// create in-memory database for testing
+	db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{})
+	require.NoError(t, err)
+
+	// create adapter
+	a, err := NewAdapterByDB(db)
+	require.NoError(t, err)
+
+	// create enforcer
+	e, err := casbin.NewEnforcer("examples/rbac_model.conf", a)
+	require.NoError(t, err)
+
+	// load policy
+	err = e.LoadPolicy()
+	require.NoError(t, err)
+
+	err = a.Transaction(e, func(e casbin.IEnforcer) error {
 		_, err := e.AddPolicy("jack", "data1", "write")
 		if err != nil {
 			return err
@@ -799,13 +895,13 @@ func TestTransactionWithSavePolicy(t *testing.T) {
 		if err != nil {
 			return err
 		}
-		err = e.SavePolicy()
-		if err != nil {
-			return err
-		}
 		return nil
 	})
-	if err != nil {
-		return
-	}
+	require.NoError(t, err)
+
+	// verify policies were added successfully
+	ok, _ := e.Enforce("jack", "data1", "write")
+	require.True(t, ok)
+	ok, _ = e.Enforce("jack", "data2", "write")
+	require.True(t, ok)
 }

go.mod

@@ -34,6 +34,7 @@ require (
 	github.com/kr/text v0.1.0 // indirect
 	github.com/mattn/go-isatty v0.0.17 // indirect
 	github.com/microsoft/go-mssqldb v1.6.0 // indirect
+	github.com/pkg/errors v0.9.1 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/remyoudompheng/bigfft v0.0.0-20230126093431-47fa9a501578 // indirect
 	github.com/rogpeppe/go-internal v1.12.0 // indirect

go.sum

@@ -86,6 +86,8 @@ github.com/modocache/gover v0.0.0-20171022184752-b58185e213c5/go.mod h1:caMODM3P
 github.com/montanaflynn/stats v0.7.0/go.mod h1:etXPPgVO6n31NxCd9KQUMvCM+ve0ruNzt6R8Bnaayow=
 github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 h1:KoWmjvw+nsYOo29YJK9vDA65RGE3NrOnUtO7a+RF9HU=
 github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8/go.mod h1:HKlIX3XHQyzLZPlr7++PzdhaXEj94dEiJgZDTsxEqUI=
+github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
+github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/remyoudompheng/bigfft v0.0.0-20200410134404-eec4a21b6bb0/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo=