Repository: casbin/casbin (go)

Issue:
Fix:  adding policy to adapter before trying to add into model
Recently I'm having issue with the following code in `internal_api.go`

```go
func (e *Enforcer) addPolicy(sec string, ptype string, rule []string) (bool, error) {
	ruleAdded := e.model.AddPolicy(sec, ptype, rule)
	if !ruleAdded {
		return ruleAdded, nil
	}

	if e.adapter != nil && e.autoSave {
		if err := e.adapter.AddPolicy(sec, ptype, rule); err != nil {
			if err.Error() != notImplemented {
				return ruleAdded, err
			}
		}
	}

	if e.watcher !=nil && e.autoNotifyWatcher {
		err := e.watcher.Update()
		if err != nil {
			return ruleAdded, err
		}
	}

	return ruleAdded, nil
}
```

It seems logic, but it may cause bugs while having network or io issues. For example it fails to add policy into adapter for some reason but it succeeds to add policy into model. Then it return an error to say that we don't succeed to add policy but actually we already added policies into model and it will influence the enforce result.

To solve this, shall we add policy into adapter first? I mean when auto save has been enabled.

Pull request:
fix: Perform adapter persist actions before model changes
Should resolve #415.

> If the model changes are made first but the adapter fails (network, code error, anything really), the in-memory model and adapter store will be out of sync. The next time the adapter is instantiated it will call LoadPolicy(x) in the store and the changes made in-memory will obviously be lost.

This change moves the adapter action to be first. The mutation of in-memory state is done only if the store does not error (not implemented should be fine still).

Patch:
diff --git a/internal_api.go b/internal_api.go
index 455ebdf..12c453b 100644
--- a/internal_api.go
+++ b/internal_api.go
@@ -23,28 +23,33 @@ const (
 	notImplemented = "not implemented"
 )
 
+func (e *Enforcer) shouldPersist() bool {
+	return e.adapter != nil && e.autoSave
+}
+
 // addPolicy adds a rule to the current policy.
 func (e *Enforcer) addPolicy(sec string, ptype string, rule []string) (bool, error) {
-	ruleAdded := e.model.AddPolicy(sec, ptype, rule)
-	if !ruleAdded {
-		return ruleAdded, nil
-	}
-
-	if sec == "g" {
-		err := e.BuildIncrementalRoleLinks(model.PolicyAdd, ptype, [][]string{rule})
-		if err != nil {
-			return ruleAdded, err
-		}
+	if e.model.HasPolicy(sec, ptype, rule) {
+		return false, nil
 	}
 
-	if e.adapter != nil && e.autoSave {
+	if e.shouldPersist() {
 		if err := e.adapter.AddPolicy(sec, ptype, rule); err != nil {
 			if err.Error() != notImplemented {
-				return ruleAdded, err
+				return false, err
 			}
 		}
 	}
 
+	e.model.AddPolicy(sec, ptype, rule)
+
+	if sec == "g" {
+		err := e.BuildIncrementalRoleLinks(model.PolicyAdd, ptype, [][]string{rule})
+		if err != nil {
+			return true, err
+		}
+	}
+
 	if e.watcher != nil && e.autoNotifyWatcher {
 		var err error
 		if watcher, ok := e.watcher.(persist.WatcherEx); ok {
@@ -52,46 +57,55 @@ func (e *Enforcer) addPolicy(sec string, ptype string, rule []string) (bool, err
 		} else {
 			err = e.watcher.Update()
 		}
-		return ruleAdded, err
+		return true, err
 	}
 
-	return ruleAdded, nil
+	return true, nil
 }
 
 // addPolicies adds rules to the current policy.
 func (e *Enforcer) addPolicies(sec string, ptype string, rules [][]string) (bool, error) {
-	rulesAdded := e.model.AddPolicies(sec, ptype, rules)
-	if !rulesAdded {
-		return rulesAdded, nil
-	}
-
-	if sec == "g" {
-		err := e.BuildIncrementalRoleLinks(model.PolicyAdd, ptype, rules)
-		if err != nil {
-			return rulesAdded, err
-		}
+	if e.model.HasPolicies(sec, ptype, rules) {
+		return false, nil
 	}
 
-	if e.adapter != nil && e.autoSave {
+	if e.shouldPersist() {
 		if err := e.adapter.(persist.BatchAdapter).AddPolicies(sec, ptype, rules); err != nil {
 			if err.Error() != notImplemented {
-				return rulesAdded, err
+				return false, err
 			}
 		}
 	}
 
+	e.model.AddPolicies(sec, ptype, rules)
+
+	if sec == "g" {
+		err := e.BuildIncrementalRoleLinks(model.PolicyAdd, ptype, rules)
+		if err != nil {
+			return true, err
+		}
+	}
+
 	if e.watcher != nil && e.autoNotifyWatcher {
 		err := e.watcher.Update()
 		if err != nil {
-			return rulesAdded, err
+			return true, err
 		}
 	}
 
-	return rulesAdded, nil
+	return true, nil
 }
 
 // removePolicy removes a rule from the current policy.
 func (e *Enforcer) removePolicy(sec string, ptype string, rule []string) (bool, error) {
+	if e.shouldPersist() {
+		if err := e.adapter.(persist.BatchAdapter).RemovePolicy(sec, ptype, rule); err != nil {
+			if err.Error() != notImplemented {
+				return false, err
+			}
+		}
+	}
+
 	ruleRemoved := e.model.RemovePolicy(sec, ptype, rule)
 	if !ruleRemoved {
 		return ruleRemoved, nil
@@ -104,14 +118,6 @@ func (e *Enforcer) removePolicy(sec string, ptype string, rule []string) (bool,
 		}
 	}
 
-	if e.adapter != nil && e.autoSave {
-		if err := e.adapter.RemovePolicy(sec, ptype, rule); err != nil {
-			if err.Error() != notImplemented {
-				return ruleRemoved, err
-			}
-		}
-	}
-
 	if e.watcher != nil && e.autoNotifyWatcher {
 		var err error
 		if watcher, ok := e.watcher.(persist.WatcherEx); ok {
@@ -128,6 +134,18 @@ func (e *Enforcer) removePolicy(sec string, ptype string, rule []string) (bool,
 
 // removePolicies removes rules from the current policy.
 func (e *Enforcer) removePolicies(sec string, ptype string, rules [][]string) (bool, error) {
+	if !e.model.HasPolicies(sec, ptype, rules) {
+		return false, nil
+	}
+
+	if e.shouldPersist() {
+		if err := e.adapter.(persist.BatchAdapter).RemovePolicies(sec, ptype, rules); err != nil {
+			if err.Error() != notImplemented {
+				return false, err
+			}
+		}
+	}
+
 	rulesRemoved := e.model.RemovePolicies(sec, ptype, rules)
 	if !rulesRemoved {
 		return rulesRemoved, nil
@@ -140,14 +158,6 @@ func (e *Enforcer) removePolicies(sec string, ptype string, rules [][]string) (b
 		}
 	}
 
-	if e.adapter != nil && e.autoSave {
-		if err := e.adapter.(persist.BatchAdapter).RemovePolicies(sec, ptype, rules); err != nil {
-			if err.Error() != notImplemented {
-				return rulesRemoved, err
-			}
-		}
-	}
-
 	if e.watcher != nil && e.autoNotifyWatcher {
 		err := e.watcher.Update()
 		if err != nil {
@@ -160,6 +170,14 @@ func (e *Enforcer) removePolicies(sec string, ptype string, rules [][]string) (b
 
 // removeFilteredPolicy removes rules based on field filters from the current policy.
 func (e *Enforcer) removeFilteredPolicy(sec string, ptype string, fieldIndex int, fieldValues ...string) (bool, error) {
+	if e.shouldPersist() {
+		if err := e.adapter.RemoveFilteredPolicy(sec, ptype, fieldIndex, fieldValues...); err != nil {
+			if err.Error() != notImplemented {
+				return false, err
+			}
+		}
+	}
+
 	ruleRemoved, effects := e.model.RemoveFilteredPolicy(sec, ptype, fieldIndex, fieldValues...)
 	if !ruleRemoved {
 		return ruleRemoved, nil
@@ -172,14 +190,6 @@ func (e *Enforcer) removeFilteredPolicy(sec string, ptype string, fieldIndex int
 		}
 	}
 
-	if e.adapter != nil && e.autoSave {
-		if err := e.adapter.RemoveFilteredPolicy(sec, ptype, fieldIndex, fieldValues...); err != nil {
-			if err.Error() != notImplemented {
-				return ruleRemoved, err
-			}
-		}
-	}
-
 	if e.watcher != nil && e.autoNotifyWatcher {
 		var err error
 		if watcher, ok := e.watcher.(persist.WatcherEx); ok {
diff --git a/model/policy.go b/model/policy.go
index 09fb041..c5dd381 100644
--- a/model/policy.go
+++ b/model/policy.go
@@ -20,7 +20,9 @@ import (
 	"github.com/casbin/casbin/v2/util"
 )
 
-type PolicyOp int
+type (
+	PolicyOp int
+)
 
 const (
 	PolicyAdd PolicyOp = iota
@@ -107,28 +109,27 @@ func (model Model) HasPolicy(sec string, ptype string, rule []string) bool {
 	return false
 }
 
-// AddPolicy adds a policy rule to the model.
-func (model Model) AddPolicy(sec string, ptype string, rule []string) bool {
-	if !model.HasPolicy(sec, ptype, rule) {
-		model[sec][ptype].Policy = append(model[sec][ptype].Policy, rule)
-		return true
-	}
-	return false
-}
-
-// AddPolicies adds policy rules to the model.
-func (model Model) AddPolicies(sec string, ptype string, rules [][]string) bool {
+// HasPolicies determines whether a model has any of the specified policies. If one is found we return false.
+func (model Model) HasPolicies(sec string, ptype string, rules [][]string) bool {
 	for i := 0; i < len(rules); i++ {
 		if model.HasPolicy(sec, ptype, rules[i]) {
-			return false
+			return true
 		}
 	}
 
+	return false
+}
+
+// AddPolicy adds a policy rule to the model.
+func (model Model) AddPolicy(sec string, ptype string, rule []string) {
+	model[sec][ptype].Policy = append(model[sec][ptype].Policy, rule)
+}
+
+// AddPolicies adds policy rules to the model.
+func (model Model) AddPolicies(sec string, ptype string, rules [][]string) {
 	for i := 0; i < len(rules); i++ {
 		model[sec][ptype].Policy = append(model[sec][ptype].Policy, rules[i])
 	}
-
-	return true
 }
 
 // RemovePolicy removes a policy rule from the model.
@@ -145,16 +146,6 @@ func (model Model) RemovePolicy(sec string, ptype string, rule []string) bool {
 
 // RemovePolicies removes policy rules from the model.
 func (model Model) RemovePolicies(sec string, ptype string, rules [][]string) bool {
-OUTER:
-	for j := 0; j < len(rules); j++ {
-		for _, r := range model[sec][ptype].Policy {
-			if util.ArrayEquals(rules[j], r) {
-				continue OUTER
-			}
-		}
-		return false
-	}
-
 	for j := 0; j < len(rules); j++ {
 		for i, r := range model[sec][ptype].Policy {
 			if util.ArrayEquals(rules[j], r) {


Test patch:
diff --git a/error_test.go b/error_test.go
index 26e41cc..1e950d4 100644
--- a/error_test.go
+++ b/error_test.go
@@ -125,7 +125,10 @@ func TestMockAdapterErrors(t *testing.T) {
 
 	e, _ := NewEnforcer("examples/rbac_with_domains_model.conf", adapter)
 
-	_, err := e.AddPolicy("admin", "domain3", "data1", "read")
+	added, err := e.AddPolicy("admin", "domain3", "data1", "read")
+	if added {
+		t.Errorf("added should be false")
+	}
 
 	if err == nil {
 		t.Errorf("Should be an error here.")
@@ -134,10 +137,13 @@ func TestMockAdapterErrors(t *testing.T) {
 		t.Log(err.Error())
 	}
 
-	rules := [][]string {
-			{"admin", "domain4", "data1", "read"},
+	rules := [][]string{
+		{"admin", "domain4", "data1", "read"},
+	}
+	added, err = e.AddPolicies(rules)
+	if added {
+		t.Errorf("added should be false")
 	}
-	_, err = e.AddPolicies(rules)
 
 	if err == nil {
 		t.Errorf("Should be an error here.")
@@ -146,7 +152,10 @@ func TestMockAdapterErrors(t *testing.T) {
 		t.Log(err.Error())
 	}
 
-	_, err2 := e.RemoveFilteredPolicy(1, "domain1", "data1")
+	removed, err2 := e.RemoveFilteredPolicy(1, "domain1", "data1")
+	if removed {
+		t.Errorf("removed should be false")
+	}
 
 	if err2 == nil {
 		t.Errorf("Should be an error here.")
@@ -155,7 +164,10 @@ func TestMockAdapterErrors(t *testing.T) {
 		t.Log(err2.Error())
 	}
 
-	_, err3 := e.RemovePolicy("admin", "domain2", "data2", "read")
+	removed, err3 := e.RemovePolicy("admin", "domain2", "data2", "read")
+	if removed {
+		t.Errorf("removed should be false")
+	}
 
 	if err3 == nil {
 		t.Errorf("Should be an error here.")
@@ -164,11 +176,13 @@ func TestMockAdapterErrors(t *testing.T) {
 		t.Log(err3.Error())
 	}
 
-	rules = [][]string {
-		{"admin", "domain4", "data1", "read"},
+	rules = [][]string{
+		{"admin", "domain1", "data1", "read"},
+	}
+	removed, err = e.RemovePolicies(rules)
+	if removed {
+		t.Errorf("removed should be false")
 	}
-
-	_, err = e.RemovePolicies(rules)
 
 	if err == nil {
 		t.Errorf("Should be an error here.")
@@ -176,7 +190,11 @@ func TestMockAdapterErrors(t *testing.T) {
 		t.Log("Test on error: ")
 		t.Log(err.Error())
 	}
-	_, err4 := e.AddGroupingPolicy("bob", "admin2")
+
+	added, err4 := e.AddGroupingPolicy("bob", "admin2")
+	if added {
+		t.Errorf("added should be false")
+	}
 
 	if err4 == nil {
 		t.Errorf("Should be an error here.")
@@ -185,7 +203,10 @@ func TestMockAdapterErrors(t *testing.T) {
 		t.Log(err4.Error())
 	}
 
-	_, err5 := e.AddNamedGroupingPolicy("g", []string{"eve", "admin2", "domain1"})
+	added, err5 := e.AddNamedGroupingPolicy("g", []string{"eve", "admin2", "domain1"})
+	if added {
+		t.Errorf("added should be false")
+	}
 
 	if err5 == nil {
 		t.Errorf("Should be an error here.")
@@ -194,7 +215,10 @@ func TestMockAdapterErrors(t *testing.T) {
 		t.Log(err5.Error())
 	}
 
-	_, err6 := e.AddNamedPolicy("p", []string{"admin2", "domain2", "data2", "write"})
+	added, err6 := e.AddNamedPolicy("p", []string{"admin2", "domain2", "data2", "write"})
+	if added {
+		t.Errorf("added should be false")
+	}
 
 	if err6 == nil {
 		t.Errorf("Should be an error here.")
@@ -203,7 +227,10 @@ func TestMockAdapterErrors(t *testing.T) {
 		t.Log(err6.Error())
 	}
 
-	_, err7 := e.RemoveGroupingPolicy("bob", "admin2")
+	removed, err7 := e.RemoveGroupingPolicy("bob", "admin2")
+	if removed {
+		t.Errorf("removed should be false")
+	}
 
 	if err7 == nil {
 		t.Errorf("Should be an error here.")
@@ -212,7 +239,10 @@ func TestMockAdapterErrors(t *testing.T) {
 		t.Log(err7.Error())
 	}
 
-	_, err8 := e.RemoveFilteredGroupingPolicy(0, "bob")
+	removed, err8 := e.RemoveFilteredGroupingPolicy(0, "bob")
+	if removed {
+		t.Errorf("removed should be false")
+	}
 
 	if err8 == nil {
 		t.Errorf("Should be an error here.")
@@ -221,7 +251,10 @@ func TestMockAdapterErrors(t *testing.T) {
 		t.Log(err8.Error())
 	}
 
-	_, err9 := e.RemoveNamedGroupingPolicy("g", []string{"alice", "admin", "domain1"})
+	removed, err9 := e.RemoveNamedGroupingPolicy("g", []string{"alice", "admin", "domain1"})
+	if removed {
+		t.Errorf("removed should be false")
+	}
 
 	if err9 == nil {
 		t.Errorf("Should be an error here.")
@@ -230,7 +263,10 @@ func TestMockAdapterErrors(t *testing.T) {
 		t.Log(err9.Error())
 	}
 
-	_, err10 := e.RemoveFilteredNamedGroupingPolicy("g", 0, "eve")
+	removed, err10 := e.RemoveFilteredNamedGroupingPolicy("g", 0, "eve")
+	if removed {
+		t.Errorf("removed should be false")
+	}
 
 	if err10 == nil {
 		t.Errorf("Should be an error here.")