feat: support isolated nested transactions#271
feat: support isolated nested transactions#271hsluoyz merged 2 commits intoapache:masterfrom yxrxy:master
Conversation
| savepointName := fmt.Sprintf("casbin_nested_%d", time.Now().UnixNano()) | ||
|
|
||
| // create savepoint | ||
| if err := adapter.db.SavePoint(savepointName).Error; err != nil { |
There was a problem hiding this comment.
suggestion: The gorm's Transaction function already does this. Consider just removing the isTxAdapter check.
Also some bulk methods like RemovePoliciesCtx are using the gorm's Transaction directly. This can lead to some operations happening in a separate transaction loosing the ability to rollback in case of further errors. Consider updating those to use this Transaction method as well.
There was a problem hiding this comment.
Maybe Need Help: Bulk methods such as RemovePoliciesCtx and UpdatePoliciesCtx require the enforcer in order to use Adapter.Transaction. But existing code does not provide it in the context. How can I switch those bulk methods to use Adapter.Transaction?
There was a problem hiding this comment.
suggestion: The gorm's Transaction function already does this. Consider just removing the isTxAdapter check.
Also some bulk methods like RemovePoliciesCtx are using the gorm's Transaction directly. This can lead to some operations happening in a separate transaction loosing the ability to rollback in case of further errors. Consider updating those to use this Transaction method as well.
If we remove the isTxAdapter check and only use GORM’s Transaction, it can cause deadlocks in concurrent scenarios(some tests will fail). The lock is necessary to prevent race conditions on the adapter’s state. i think this logic is needed for thread safety.
|
@tangyang9464 plz review |
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for isolated nested transactions in the gorm-adapter, addressing an issue identified in PR #269. The implementation uses database savepoints to handle nested transaction scenarios with proper rollback capabilities.
- Adds savepoint-based nested transaction support to allow inner transactions to fail without affecting outer transactions
- Enhances test coverage with comprehensive nested transaction scenarios including success/failure combinations and deeply nested cases
- Standardizes test database setup by switching from in-memory SQLite to MySQL using the common
initAdapterpattern
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| adapter.go | Implements savepoint-based nested transaction logic with model state preservation |
| adapter_test.go | Adds comprehensive nested transaction test cases and standardizes database setup |
Comments suppressed due to low confidence (1)
adapter_test.go:847
- The test expects the outer transaction to continue after inner transaction failure, but the code returns the error immediately, preventing the outer transaction from continuing. This doesn't match the test's intent or the comment on line 850.
return err
| } | ||
| // restore model state to undo inner transaction changes | ||
| e.SetModel(originalModel) | ||
| fmt.Printf("Warning: Inner transaction failed and was rolled back: %v\n", err) |
There was a problem hiding this comment.
Using fmt.Printf for error logging is not ideal for a library. Consider using a proper logging framework or removing this debug output for production code.
| fmt.Printf("Warning: Inner transaction failed and was rolled back: %v\n", err) | |
| adapter.db.Logger.Warn(context.Background(), "Inner transaction failed and was rolled back: %v", err) |
| // Don't return error to allow outer transaction to continue | ||
| return nil |
There was a problem hiding this comment.
Silently swallowing the inner transaction error by returning nil changes the expected behavior. Consider returning the error or providing a way for callers to handle nested transaction failures explicitly.
| // Don't return error to allow outer transaction to continue | |
| return nil | |
| // Return the error to allow callers to handle the inner transaction failure explicitly | |
| return errors.Wrap(err, "inner transaction failed and was rolled back") |
| _, err := e.AddPolicy("frank", "data13", "read") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // level 2 | ||
| err = adapter.Transaction(e, func(e2 casbin.IEnforcer) error { | ||
| _, err := e2.AddPolicy("frank", "data14", "write") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // level 3 | ||
| return adapter.Transaction(e2, func(e3 casbin.IEnforcer) error { | ||
| _, err := e3.AddPolicy("frank", "data15", "delete") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| _, err = e3.AddPolicy("frank", "data16", "execute") |
There was a problem hiding this comment.
The policy data 'data13' conflicts with the policy created in test 5 (line 896). This could cause test interdependencies if tests run in different orders.
| _, err := e.AddPolicy("frank", "data13", "read") | |
| if err != nil { | |
| return err | |
| } | |
| // level 2 | |
| err = adapter.Transaction(e, func(e2 casbin.IEnforcer) error { | |
| _, err := e2.AddPolicy("frank", "data14", "write") | |
| if err != nil { | |
| return err | |
| } | |
| // level 3 | |
| return adapter.Transaction(e2, func(e3 casbin.IEnforcer) error { | |
| _, err := e3.AddPolicy("frank", "data15", "delete") | |
| if err != nil { | |
| return err | |
| } | |
| _, err = e3.AddPolicy("frank", "data16", "execute") | |
| _, err := e.AddPolicy("frank", "data17", "read") | |
| if err != nil { | |
| return err | |
| } | |
| // level 2 | |
| err = adapter.Transaction(e, func(e2 casbin.IEnforcer) error { | |
| _, err := e2.AddPolicy("frank", "data18", "write") | |
| if err != nil { | |
| return err | |
| } | |
| // level 3 | |
| return adapter.Transaction(e2, func(e3 casbin.IEnforcer) error { | |
| _, err := e3.AddPolicy("frank", "data19", "delete") | |
| if err != nil { | |
| return err | |
| } | |
| _, err = e3.AddPolicy("frank", "data20", "execute") |
|
🎉 This PR is included in version 3.35.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Fix issue in PR: #269