fix: improve policyManager

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

Author: nodece

api/manager.go

@@ -0,0 +1,22 @@
+// Copyright 2020 The casbin Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package api
+
+// PolicyManager is the policy manager for model and adapter
+type PolicyManager interface {
+	AddPolicies(sec string, ptype string, rules [][]string) ([][]string, error)
+	RemovePolicies(sec string, ptype string, rules [][]string) ([][]string, error)
+	RemoveFilteredPolicy(sec string, ptype string, fieldIndex int, fieldValues ...string) ([][]string, error)
+}

enforcer.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/Knetic/govaluate"
+	"github.com/casbin/casbin/v3/api"
 	"github.com/casbin/casbin/v3/effect"
 	"github.com/casbin/casbin/v3/internal"
 	"github.com/casbin/casbin/v3/log"
@@ -45,7 +46,7 @@ type Enforcer struct {
 	dispatcher persist.Dispatcher
 	rm         rbac.RoleManager
 
-	internal             internal.PolicyManager
+	policyManager        api.PolicyManager
 	enabled              bool
 	autoSave             bool
 	autoBuildRoleLinks   bool
@@ -159,7 +160,7 @@ func (e *Enforcer) InitWithModelAndAdapter(m model.Model, adapter persist.Adapte
 	e.model.PrintModel()
 	e.fm = model.LoadFunctionMap()
 	e.initialize()
-	e.internal = internal.NewPolicyManager(m, adapter, e.rm)
+	e.policyManager = internal.NewPolicyManager(m, adapter, e.rm, e.shouldPersist)
 	// Do not initialize the full policy when using a filtered adapter
 	fa, ok := e.adapter.(persist.FilteredAdapter)
 	if e.adapter != nil && (!ok || ok && !fa.IsFiltered()) {
@@ -209,7 +210,7 @@ func (e *Enforcer) GetModel() model.Model {
 func (e *Enforcer) SetModel(m model.Model) {
 	e.model = m
 	e.fm = model.LoadFunctionMap()
-	e.internal = internal.NewPolicyManager(m, e.adapter, e.rm)
+	e.policyManager = internal.NewPolicyManager(m, e.adapter, e.rm, e.shouldPersist)
 	e.initialize()
 }
 
@@ -221,12 +222,12 @@ func (e *Enforcer) GetAdapter() persist.Adapter {
 // SetAdapter sets the current adapter.
 func (e *Enforcer) SetAdapter(adapter persist.Adapter) {
 	e.adapter = adapter
-	e.internal = internal.NewPolicyManager(e.model, adapter, e.rm)
+	e.policyManager = internal.NewPolicyManager(e.model, e.adapter, e.rm, e.shouldPersist)
 }
 
 // GetPolicyManager gets the current policy manager.
-func (e *Enforcer) GetPolicyManager() internal.PolicyManager {
-	return e.internal
+func (e *Enforcer) GetPolicyManager() api.PolicyManager {
+	return e.policyManager
 }
 
 // SetWatcher sets the current watcher.
@@ -249,7 +250,7 @@ func (e *Enforcer) GetRoleManager() rbac.RoleManager {
 // SetRoleManager sets the current role manager.
 func (e *Enforcer) SetRoleManager(rm rbac.RoleManager) {
 	e.rm = rm
-	e.internal = internal.NewPolicyManager(e.model, e.adapter, rm)
+	e.policyManager = internal.NewPolicyManager(e.model, e.adapter, rm, e.shouldPersist)
 }
 
 // SetEffector sets the current effector.

internal/internal.go

@@ -15,77 +15,47 @@
 package internal
 
 import (
+	"github.com/casbin/casbin/v3/api"
 	"github.com/casbin/casbin/v3/errors"
 	"github.com/casbin/casbin/v3/model"
 	"github.com/casbin/casbin/v3/persist"
 	"github.com/casbin/casbin/v3/rbac"
 )
 
-// PolicyManager is the policy manager for model and adapter
-type PolicyManager interface {
-	AddPolicy(sec string, ptype string, rule []string, shouldPersist bool) (bool, error)
-	AddPolicies(sec string, ptype string, rules [][]string, shouldPersist bool) (bool, [][]string, error)
-	RemovePolicy(sec string, ptype string, rule []string, shouldPersist bool) (bool, error)
-	RemovePolicies(sec string, ptype string, rules [][]string, shouldPersist bool) (bool, [][]string, error)
-	RemoveFilteredPolicy(sec string, ptype string, shouldPersist bool, fieldIndex int, fieldValues ...string) (bool, [][]string, error)
-}
+var _ api.PolicyManager = &policyManager{}
 
 type policyManager struct {
-	model   model.Model
-	adapter persist.Adapter
-	rm      rbac.RoleManager
+	model         model.Model
+	adapter       persist.Adapter
+	rm            rbac.RoleManager
+	shouldPersist func() bool
 }
 
 const (
 	notImplemented = "not implemented"
 )
 
 // NewPolicyManager is the constructor for PolicyManager
-func NewPolicyManager(model model.Model, adapter persist.Adapter, rm rbac.RoleManager) PolicyManager {
+func NewPolicyManager(model model.Model, adapter persist.Adapter, rm rbac.RoleManager, shouldPersist func() bool) api.PolicyManager {
 	return &policyManager{
-		model:   model,
-		adapter: adapter,
-		rm:      rm,
-	}
-}
-
-// AddPolicy adds a rule to model and adapter.
-func (p *policyManager) AddPolicy(sec string, ptype string, rule []string, shouldPersist bool) (bool, error) {
-	if p.model.HasPolicy(sec, ptype, rule) {
-		return false, nil
-	}
-
-	if shouldPersist {
-		if err := p.adapter.AddPolicy(sec, ptype, rule); err != nil {
-			if err.Error() != notImplemented {
-				return false, err
-			}
-		}
-	}
-
-	p.model.AddPolicy(sec, ptype, rule)
-
-	if sec == "g" {
-		err := p.model.BuildIncrementalRoleLinks(p.rm, model.PolicyAdd, "g", ptype, [][]string{rule})
-		if err != nil {
-			return true, err
-		}
+		model:         model,
+		adapter:       adapter,
+		rm:            rm,
+		shouldPersist: shouldPersist,
 	}
-
-	return true, nil
 }
 
 // AddPolicies adds rules to model and adapter.
-func (p *policyManager) AddPolicies(sec string, ptype string, rules [][]string, shouldPersist bool) (bool, [][]string, error) {
-	rules = p.model.RemoveExistPolicy(sec, ptype, rules)
+func (p *policyManager) AddPolicies(sec string, ptype string, rules [][]string) ([][]string, error) {
+	rules = p.model.FilterNotExistPolicy(sec, ptype, rules)
 	if len(rules) == 0 {
-		return true, nil, nil
+		return nil, nil
 	}
 
-	if shouldPersist {
+	if p.shouldPersist() {
 		if err := p.adapter.(persist.BatchAdapter).AddPolicies(sec, ptype, rules); err != nil {
 			if err.Error() != notImplemented {
-				return false, nil, err
+				return nil, err
 			}
 		}
 	}
@@ -95,93 +65,67 @@ func (p *policyManager) AddPolicies(sec string, ptype string, rules [][]string,
 	if sec == "g" {
 		err := p.model.BuildIncrementalRoleLinks(p.rm, model.PolicyAdd, "g", ptype, rules)
 		if err != nil {
-			return true, rules, err
-		}
-	}
-
-	return true, rules, nil
-}
-
-// RemovePolicy removes a rule from model and adapter.
-func (p *policyManager) RemovePolicy(sec string, ptype string, rule []string, shouldPersist bool) (bool, error) {
-	if shouldPersist {
-		if err := p.adapter.RemovePolicy(sec, ptype, rule); err != nil {
-			if err.Error() != notImplemented {
-				return false, err
-			}
-		}
-	}
-
-	ruleRemoved := p.model.RemovePolicy(sec, ptype, rule)
-	if !ruleRemoved {
-		return ruleRemoved, nil
-	}
-
-	if sec == "g" {
-		err := p.model.BuildIncrementalRoleLinks(p.rm, model.PolicyRemove, "g", ptype, [][]string{rule})
-		if err != nil {
-			return ruleRemoved, err
+			return rules, err
 		}
 	}
 
-	return ruleRemoved, nil
+	return rules, nil
 }
 
 // RemovePolicies removes rules from model and adapter.
-func (p *policyManager) RemovePolicies(sec string, ptype string, rules [][]string, shouldPersist bool) (bool, [][]string, error) {
-	rules = p.model.RemoveNotExistPolicy(sec, ptype, rules)
+func (p *policyManager) RemovePolicies(sec string, ptype string, rules [][]string) ([][]string, error) {
 	if len(rules) == 0 {
-		return true, nil, nil
+		return nil, nil
 	}
 
-	if shouldPersist {
+	if p.shouldPersist() {
 		if err := p.adapter.(persist.BatchAdapter).RemovePolicies(sec, ptype, rules); err != nil {
 			if err.Error() != notImplemented {
-				return false, nil, err
+				return nil, err
 			}
 		}
 	}
 
 	rulesRemoved := p.model.RemovePolicies(sec, ptype, rules)
 	if !rulesRemoved {
-		return rulesRemoved, rules, nil
+		return rules, nil
 	}
 
 	if sec == "g" {
 		err := p.model.BuildIncrementalRoleLinks(p.rm, model.PolicyRemove, "g", ptype, rules)
 		if err != nil {
-			return rulesRemoved, rules, err
+			return rules, err
 		}
 	}
 
-	return rulesRemoved, rules, nil
+	return rules, nil
 }
 
 // RemoveFilteredPolicy removes rules based on field filters from model and adapter.
-func (p *policyManager) RemoveFilteredPolicy(sec string, ptype string, shouldPersist bool, fieldIndex int, fieldValues ...string) (bool, [][]string, error) {
+func (p *policyManager) RemoveFilteredPolicy(sec string, ptype string, fieldIndex int, fieldValues ...string) ([][]string, error) {
 	if len(fieldValues) == 0 {
-		return false, nil, errors.INVALID_FIELDVAULES_PARAMETER
+		return nil, errors.INVALID_FIELDVAULES_PARAMETER
 	}
 
-	if shouldPersist {
+	if p.shouldPersist() {
 		if err := p.adapter.RemoveFilteredPolicy(sec, ptype, fieldIndex, fieldValues...); err != nil {
 			if err.Error() != notImplemented {
-				return false, nil, err
+				return nil, err
 			}
 		}
 	}
 
 	ruleRemoved, effects := p.model.RemoveFilteredPolicy(sec, ptype, fieldIndex, fieldValues...)
 	if !ruleRemoved {
-		return ruleRemoved, effects, nil
+		return effects, nil
 	}
 
 	if sec == "g" {
 		err := p.model.BuildIncrementalRoleLinks(p.rm, model.PolicyRemove, "g", ptype, effects)
 		if err != nil {
-			return ruleRemoved, effects, err
+			return effects, err
 		}
 	}
 
-	return ruleRemoved, effects, nil
+	return effects, nil
 }

internal_api.go

@@ -30,8 +30,9 @@ func (e *Enforcer) addPolicy(sec string, ptype string, rule []string) (bool, err
 		return true, e.dispatcher.AddPolicies(sec, ptype, [][]string{rule})
 	}
 
-	if result, err := e.internal.AddPolicy(sec, ptype, rule, e.shouldPersist()); err != nil || !result {
-		return result, err
+	effects, err := e.policyManager.AddPolicies(sec, ptype, [][]string{rule})
+	if err != nil || len(effects) == 0 {
+		return false, err
 	}
 
 	if e.watcher != nil && e.autoNotifyWatcher {
@@ -57,9 +58,9 @@ func (e *Enforcer) addPolicies(sec string, ptype string, rules [][]string) (bool
 		return true, e.dispatcher.AddPolicies(sec, ptype, rules)
 	}
 
-	result, effects, err := e.internal.AddPolicies(sec, ptype, rules, e.shouldPersist())
-	if err != nil || !result {
-		return result, err
+	effects, err := e.policyManager.AddPolicies(sec, ptype, rules)
+	if err != nil || len(effects) == 0 {
+		return false, err
 	}
 
 	if e.watcher != nil && e.autoNotifyWatcher {
@@ -82,9 +83,9 @@ func (e *Enforcer) removePolicy(sec string, ptype string, rule []string) (bool,
 		return true, e.dispatcher.RemovePolicies(sec, ptype, [][]string{rule})
 	}
 
-	ruleRemoved, err := e.internal.RemovePolicy(sec, ptype, rule, e.shouldPersist())
-	if err != nil || !ruleRemoved {
-		return ruleRemoved, err
+	effects, err := e.policyManager.RemovePolicies(sec, ptype, [][]string{rule})
+	if err != nil || len(effects) == 0 {
+		return false, err
 	}
 
 	if e.watcher != nil && e.autoNotifyWatcher {
@@ -94,15 +95,15 @@ func (e *Enforcer) removePolicy(sec string, ptype string, rule []string) (bool,
 		} else {
 			err = e.watcher.Update()
 		}
-		return ruleRemoved, err
+		return true, err
 
 	}
 
 	if log.GetLogger().IsEnabled() {
 		log.LogPrintf("Policy Management, Type: RemovePolicy Assertion %s::%s\nrule: %s", sec, ptype, util.ArrayToString(rule))
 	}
 
-	return ruleRemoved, nil
+	return true, nil
 }
 
 // removePolicies removes rules from the current policy.
@@ -111,23 +112,23 @@ func (e *Enforcer) removePolicies(sec string, ptype string, rules [][]string) (b
 		return true, e.dispatcher.RemovePolicies(sec, ptype, rules)
 	}
 
-	rulesRemoved, effects, err := e.internal.RemovePolicies(sec, ptype, rules, e.shouldPersist())
-	if err != nil || !rulesRemoved {
-		return rulesRemoved, err
+	effects, err := e.policyManager.RemovePolicies(sec, ptype, rules)
+	if err != nil || len(effects) == 0 {
+		return false, err
 	}
 
 	if e.watcher != nil && e.autoNotifyWatcher {
 		err := e.watcher.Update()
 		if err != nil {
-			return rulesRemoved, err
+			return true, err
 		}
 	}
 
 	if log.GetLogger().IsEnabled() {
 		log.LogPrintf("Policy Management, Type: RemovePolicies Assertion %s::%s\nrules: %s", sec, ptype, util.Array2DToString(effects))
 	}
 
-	return rulesRemoved, nil
+	return true, nil
 }
 
 // removeFilteredPolicy removes rules based on field filters from the current policy.
@@ -136,9 +137,9 @@ func (e *Enforcer) removeFilteredPolicy(sec string, ptype string, fieldIndex int
 		return true, e.dispatcher.RemoveFilteredPolicy(sec, ptype, fieldIndex, fieldValues...)
 	}
 
-	ruleRemoved, effects, err := e.internal.RemoveFilteredPolicy(sec, ptype, e.shouldPersist(), fieldIndex, fieldValues...)
-	if err != nil || !ruleRemoved {
-		return ruleRemoved, err
+	effects, err := e.policyManager.RemoveFilteredPolicy(sec, ptype, fieldIndex, fieldValues...)
+	if err != nil || len(effects) == 0 {
+		return false, err
 	}
 
 	if e.watcher != nil && e.autoNotifyWatcher {
@@ -148,12 +149,12 @@ func (e *Enforcer) removeFilteredPolicy(sec string, ptype string, fieldIndex int
 		} else {
 			err = e.watcher.Update()
 		}
-		return ruleRemoved, err
+		return true, err
 	}
 
 	if log.GetLogger().IsEnabled() {
 		log.LogPrintf("Policy Management, Type: RemoveFilteredPolicy Assertion: %s::%s\nrules: %s", sec, ptype, util.Array2DToString(effects))
 	}
 
-	return ruleRemoved, nil
+	return true, nil
 }