Repository: casbin/casbin
Pull request #1085 (2022-08-23 12:38:38)

feat: return error in LoadPolicyArray and LoadPolicyLine
fix https://github.com/casbin/casbin/issues/1083#issuecomment-1221529782

casbin-bot:  @tangyang9464 please review
tangyang9464: @JalinWang plz fix CI error
![image](https://user-images.githubusercontent.com/45587763/188305188-76f5015d-3b30-4eb3-8141-7e0306717603.png)

hsluoyz: @tangyang9464 

Patch:
diff --git a/internal_api.go b/internal_api.go
index 2af0dbe..8c3f97f 100644
--- a/internal_api.go
+++ b/internal_api.go
@@ -369,8 +369,8 @@ func (e *Enforcer) updatePolicy(sec string, ptype string, oldRule []string, newR
 
 	if e.shouldNotify() {
 		var err error
-		if watcher, ok := e.watcher.(persist.WatcherUpdatable); ok {
-			err = watcher.UpdateForUpdatePolicy(oldRule, newRule)
+		if watcher, ok := e.watcher.(persist.UpdatableWatcher); ok {
+			err = watcher.UpdateForUpdatePolicy(sec, ptype, oldRule, newRule)
 		} else {
 			err = e.watcher.Update()
 		}
@@ -388,8 +388,8 @@ func (e *Enforcer) updatePolicies(sec string, ptype string, oldRules [][]string,
 
 	if e.shouldNotify() {
 		var err error
-		if watcher, ok := e.watcher.(persist.WatcherUpdatable); ok {
-			err = watcher.UpdateForUpdatePolicies(oldRules, newRules)
+		if watcher, ok := e.watcher.(persist.UpdatableWatcher); ok {
+			err = watcher.UpdateForUpdatePolicies(sec, ptype, oldRules, newRules)
 		} else {
 			err = e.watcher.Update()
 		}
@@ -448,8 +448,8 @@ func (e *Enforcer) updateFilteredPolicies(sec string, ptype string, newRules [][
 
 	if e.shouldNotify() {
 		var err error
-		if watcher, ok := e.watcher.(persist.WatcherUpdatable); ok {
-			err = watcher.UpdateForUpdatePolicies(oldRules, newRules)
+		if watcher, ok := e.watcher.(persist.UpdatableWatcher); ok {
+			err = watcher.UpdateForUpdatePolicies(sec, ptype, oldRules, newRules)
 		} else {
 			err = e.watcher.Update()
 		}
diff --git a/persist/adapter.go b/persist/adapter.go
index f76389a..0525657 100644
--- a/persist/adapter.go
+++ b/persist/adapter.go
@@ -22,9 +22,9 @@ import (
 )
 
 // LoadPolicyLine loads a text line as a policy rule to model.
-func LoadPolicyLine(line string, m model.Model) {
+func LoadPolicyLine(line string, m model.Model) error {
 	if line == "" || strings.HasPrefix(line, "#") {
-		return
+		return nil
 	}
 
 	r := csv.NewReader(strings.NewReader(line))
@@ -34,24 +34,25 @@ func LoadPolicyLine(line string, m model.Model) {
 
 	tokens, err := r.Read()
 	if err != nil {
-		return
+		return err
 	}
 
-	LoadPolicyArray(tokens, m)
+	return LoadPolicyArray(tokens, m)
 }
 
 // LoadPolicyArray loads a policy rule to model.
-func LoadPolicyArray(rule []string, m model.Model) {
+func LoadPolicyArray(rule []string, m model.Model) error {
 	key := rule[0]
 	sec := key[:1]
 	ok, err := m.HasPolicyEx(sec, key, rule[1:])
 	if err != nil {
-		return
+		return err
 	}
 	if ok {
-		return
+		return nil // skip duplicated policy
 	}
 	m.AddPolicy(sec, key, rule[1:])
+	return nil
 }
 
 // Adapter is the interface for Casbin adapters.
diff --git a/persist/file-adapter/adapter.go b/persist/file-adapter/adapter.go
index 3c4f1b1..c68f0ea 100644
--- a/persist/file-adapter/adapter.go
+++ b/persist/file-adapter/adapter.go
@@ -32,7 +32,7 @@ type Adapter struct {
 	filePath string
 }
 
-func (a *Adapter) UpdatePolicy(sec string, ptype string, oldRule, newPolicy []string) error {
+func (a *Adapter) UpdatePolicy(sec string, ptype string, oldRule, newRule []string) error {
 	return errors.New("not implemented")
 }
 
@@ -40,7 +40,7 @@ func (a *Adapter) UpdatePolicies(sec string, ptype string, oldRules, newRules []
 	return errors.New("not implemented")
 }
 
-func (a *Adapter) UpdateFilteredPolicies(sec string, ptype string, newPolicies [][]string, fieldIndex int, fieldValues ...string) ([][]string, error) {
+func (a *Adapter) UpdateFilteredPolicies(sec string, ptype string, newRules [][]string, fieldIndex int, fieldValues ...string) ([][]string, error) {
 	return nil, errors.New("not implemented")
 }
 
@@ -85,7 +85,7 @@ func (a *Adapter) SavePolicy(model model.Model) error {
 	return a.savePolicyFile(strings.TrimRight(tmp.String(), "\n"))
 }
 
-func (a *Adapter) loadPolicyFile(model model.Model, handler func(string, model.Model)) error {
+func (a *Adapter) loadPolicyFile(model model.Model, handler func(string, model.Model) error) error {
 	f, err := os.Open(a.filePath)
 	if err != nil {
 		return err
@@ -95,7 +95,10 @@ func (a *Adapter) loadPolicyFile(model model.Model, handler func(string, model.M
 	scanner := bufio.NewScanner(f)
 	for scanner.Scan() {
 		line := strings.TrimSpace(scanner.Text())
-		handler(line, model)
+		err = handler(line, model)
+		if err != nil {
+			return err
+		}
 	}
 	return scanner.Err()
 }
diff --git a/persist/file-adapter/adapter_filtered.go b/persist/file-adapter/adapter_filtered.go
index e3c6c0c..1a074c9 100644
--- a/persist/file-adapter/adapter_filtered.go
+++ b/persist/file-adapter/adapter_filtered.go
@@ -77,7 +77,7 @@ func (a *FilteredAdapter) LoadFilteredPolicy(model model.Model, filter interface
 	return err
 }
 
-func (a *FilteredAdapter) loadFilteredPolicyFile(model model.Model, filter *Filter, handler func(string, model.Model)) error {
+func (a *FilteredAdapter) loadFilteredPolicyFile(model model.Model, filter *Filter, handler func(string, model.Model) error) error {
 	f, err := os.Open(a.filePath)
 	if err != nil {
 		return err
@@ -92,7 +92,10 @@ func (a *FilteredAdapter) loadFilteredPolicyFile(model model.Model, filter *Filt
 			continue
 		}
 
-		handler(line, model)
+		err = handler(line, model)
+		if err != nil {
+			return err
+		}
 	}
 	return scanner.Err()
 }
diff --git a/persist/file-adapter/adapter_mock.go b/persist/file-adapter/adapter_mock.go
index 55dd131..8f8632b 100644
--- a/persist/file-adapter/adapter_mock.go
+++ b/persist/file-adapter/adapter_mock.go
@@ -50,7 +50,7 @@ func (a *AdapterMock) SavePolicy(model model.Model) error {
 	return nil
 }
 
-func (a *AdapterMock) loadPolicyFile(model model.Model, handler func(string, model.Model)) error {
+func (a *AdapterMock) loadPolicyFile(model model.Model, handler func(string, model.Model) error) error {
 	f, err := os.Open(a.filePath)
 	if err != nil {
 		return err
@@ -61,11 +61,14 @@ func (a *AdapterMock) loadPolicyFile(model model.Model, handler func(string, mod
 	for {
 		line, err := buf.ReadString('\n')
 		line = strings.TrimSpace(line)
-		handler(line, model)
+		if err2 := handler(line, model); err2 != nil {
+			return err2
+		}
 		if err != nil {
 			if err == io.EOF {
 				return nil
 			}
+			return err
 		}
 	}
 }
diff --git a/persist/update_adapter.go b/persist/update_adapter.go
index 3dd59f7..fe9204a 100644
--- a/persist/update_adapter.go
+++ b/persist/update_adapter.go
@@ -19,9 +19,9 @@ type UpdatableAdapter interface {
 	Adapter
 	// UpdatePolicy updates a policy rule from storage.
 	// This is part of the Auto-Save feature.
-	UpdatePolicy(sec string, ptype string, oldRule, newPolicy []string) error
+	UpdatePolicy(sec string, ptype string, oldRule, newRule []string) error
 	// UpdatePolicies updates some policy rules to storage, like db, redis.
 	UpdatePolicies(sec string, ptype string, oldRules, newRules [][]string) error
 	// UpdateFilteredPolicies deletes old rules and adds new rules.
-	UpdateFilteredPolicies(sec string, ptype string, newPolicies [][]string, fieldIndex int, fieldValues ...string) ([][]string, error)
+	UpdateFilteredPolicies(sec string, ptype string, newRules [][]string, fieldIndex int, fieldValues ...string) ([][]string, error)
 }
diff --git a/persist/watcher_update.go b/persist/watcher_update.go
index 6743316..694123c 100644
--- a/persist/watcher_update.go
+++ b/persist/watcher_update.go
@@ -14,13 +14,13 @@
 
 package persist
 
-// WatcherUpdatable is the strengthen for Casbin watchers.
-type WatcherUpdatable interface {
+// UpdatableWatcher is strengthened for Casbin watchers.
+type UpdatableWatcher interface {
 	Watcher
 	// UpdateForUpdatePolicy calls the update callback of other instances to synchronize their policy.
 	// It is called after Enforcer.UpdatePolicy()
-	UpdateForUpdatePolicy(oldRule, newRule []string) error
+	UpdateForUpdatePolicy(sec string, ptype string, oldRule, newRule []string) error
 	// UpdateForUpdatePolicies calls the update callback of other instances to synchronize their policy.
 	// It is called after Enforcer.UpdatePolicies()
-	UpdateForUpdatePolicies(oldRules, newRules [][]string) error
+	UpdateForUpdatePolicies(sec string, ptype string, oldRules, newRules [][]string) error
 }


Test patch:
diff --git a/persist/persist_test.go b/persist/persist_test.go
index d38bd7b..e37bfcb 100644
--- a/persist/persist_test.go
+++ b/persist/persist_test.go
@@ -46,8 +46,8 @@ func TestDuplicateRuleInAdapter(t *testing.T) {
 	e.ClearPolicy()
 
 	//simulate adapter.LoadPolicy with duplicate rules
-	persist.LoadPolicyArray([]string{"p", "alice", "data1", "read"}, e.GetModel())
-	persist.LoadPolicyArray([]string{"p", "alice", "data1", "read"}, e.GetModel())
+	_ = persist.LoadPolicyArray([]string{"p", "alice", "data1", "read"}, e.GetModel())
+	_ = persist.LoadPolicyArray([]string{"p", "alice", "data1", "read"}, e.GetModel())
 
 	testRuleCount(t, e.GetModel(), 1, "p", "p", "LoadPolicyArray")
 }