Repository: casbin/casbin
Pull request #1031 (2022-06-13 16:24:36)

feat: caching the govaluate expressions
Trying to provide a solution for issue https://github.com/casbin/casbin/issues/1028 

All tests pass, but I'm not quite sure if every edge case is covered

Fix: https://github.com/casbin/casbin/issues/1028

casbin-bot:  @tangyang9464 @closetool @sagilio please review
abichinger: @timconinx Please make sure, that the PR title and commit messages follow the [Conventional Commits spec](https://www.conventionalcommits.org/en/v1.0.0/#summary)

Otherwise it looks good to me.

Patch:
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index a593d26..c5f0ddb 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -6,10 +6,9 @@ This project adheres to the [Contributor Covenant 1.2.](https://www.contributor-
 
 ## Questions
 
-* We do our best to have an [up-to-date documentation](https://casbin.org/docs/en/overview)
-* [Stack Overflow](https://stackoverflow.com) is the best place to start if you have a question. Please use the [casbin tag](https://stackoverflow.com/tags/casbin/info) we are actively monitoring. We encourage you to use Stack Overflow specially for Modeling Access Control Problems, in order to build a shared knowledge base.
-* You can also join our [Gitter community](https://gitter.im/casbin/Lobby).
-
+- We do our best to have an [up-to-date documentation](https://casbin.org/docs/overview)
+- [Stack Overflow](https://stackoverflow.com) is the best place to start if you have a question. Please use the [casbin tag](https://stackoverflow.com/tags/casbin/info) we are actively monitoring. We encourage you to use Stack Overflow specially for Modeling Access Control Problems, in order to build a shared knowledge base.
+- You can also join our [Gitter community](https://gitter.im/casbin/Lobby).
 
 ## Reporting issues
 
@@ -27,7 +26,6 @@ In what environment did you encounter the issue?
 
 Feature requests can also be submitted as issues.
 
-
 ## Pull requests
 
 Good pull requests (e.g. patches, improvements, new features) are a fantastic help. They should remain focused in scope and avoid unrelated commits.
diff --git a/README.md b/README.md
index 96cd283..98723bf 100644
--- a/README.md
+++ b/README.md
@@ -145,7 +145,7 @@ go get github.com/casbin/casbin/v2
 
 ## Documentation
 
-https://casbin.org/docs/en/overview
+https://casbin.org/docs/overview
 
 ## Online editor
 
@@ -153,7 +153,7 @@ You can also use the online editor (https://casbin.org/editor/) to write your Ca
 
 ## Tutorials
 
-https://casbin.org/docs/en/tutorials
+https://casbin.org/docs/tutorials
 
 ## Get started
 
@@ -191,10 +191,10 @@ See [Policy management APIs](#policy-management) for more usage.
 
 Casbin provides two sets of APIs to manage permissions:
 
-- [Management API](https://casbin.org/docs/en/management-api): the primitive API that provides full support for Casbin policy management.
-- [RBAC API](https://casbin.org/docs/en/rbac-api): a more friendly API for RBAC. This API is a subset of Management API. The RBAC users could use this API to simplify the code.
+- [Management API](https://casbin.org/docs/management-api): the primitive API that provides full support for Casbin policy management.
+- [RBAC API](https://casbin.org/docs/rbac-api): a more friendly API for RBAC. This API is a subset of Management API. The RBAC users could use this API to simplify the code.
 
-We also provide a [web-based UI](https://casbin.org/docs/en/admin-portal) for model management and policy management:
+We also provide a [web-based UI](https://casbin.org/docs/admin-portal) for model management and policy management:
 
 ![model editor](https://hsluoyz.github.io/casbin/ui_model_editor.png)
 
@@ -202,19 +202,19 @@ We also provide a [web-based UI](https://casbin.org/docs/en/admin-portal) for mo
 
 ## Policy persistence
 
-https://casbin.org/docs/en/adapters
+https://casbin.org/docs/adapters
 
 ## Policy consistence between multiple nodes
 
-https://casbin.org/docs/en/watchers
+https://casbin.org/docs/watchers
 
 ## Role manager
 
-https://casbin.org/docs/en/role-managers
+https://casbin.org/docs/role-managers
 
 ## Benchmarks
 
-https://casbin.org/docs/en/benchmark
+https://casbin.org/docs/benchmark
 
 ## Examples
 
@@ -234,11 +234,11 @@ Priority | [priority_model.conf](https://github.com/casbin/casbin/blob/master/ex
 
 ## Middlewares
 
-Authz middlewares for web frameworks: https://casbin.org/docs/en/middlewares
+Authz middlewares for web frameworks: https://casbin.org/docs/middlewares
 
 ## Our adopters
 
-https://casbin.org/docs/en/adopters
+https://casbin.org/docs/adopters
 
 ## How to Contribute
 
diff --git a/enforcer.go b/enforcer.go
index de25375..d3c0c72 100644
--- a/enforcer.go
+++ b/enforcer.go
@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"runtime/debug"
 	"strings"
+	"sync"
 
 	"github.com/Knetic/govaluate"
 	"github.com/casbin/casbin/v2/effector"
@@ -42,6 +43,7 @@ type Enforcer struct {
 	watcher    persist.Watcher
 	dispatcher persist.Dispatcher
 	rmMap      map[string]rbac.RoleManager
+	matcherMap sync.Map
 
 	enabled              bool
 	autoSave             bool
@@ -198,6 +200,7 @@ func (e *Enforcer) initialize() {
 	e.rmMap = map[string]rbac.RoleManager{}
 	e.eft = effector.NewDefaultEffector()
 	e.watcher = nil
+	e.matcherMap = sync.Map{}
 
 	e.enabled = true
 	e.autoSave = true
@@ -472,6 +475,7 @@ func (e *Enforcer) BuildRoleLinks() error {
 
 // BuildIncrementalRoleLinks provides incremental build the role inheritance relations.
 func (e *Enforcer) BuildIncrementalRoleLinks(op model.PolicyOp, ptype string, rules [][]string) error {
+	e.invalidateMatcherMap()
 	return e.model.BuildIncrementalRoleLinks(e.rmMap, op, "g", ptype, rules)
 }
 
@@ -485,6 +489,10 @@ func NewEnforceContext(suffix string) EnforceContext {
 	}
 }
 
+func (e *Enforcer) invalidateMatcherMap() {
+	e.matcherMap = sync.Map{}
+}
+
 // enforce use a custom matcher to decides whether a "subject" can access a "object" with the operation "action", input parameters are usually: (matcher, sub, obj, act), use model matcher by default when matcher is "".
 func (e *Enforcer) enforce(matcher string, explains *[]string, rvals ...interface{}) (ok bool, err error) {
 	defer func() {
@@ -548,14 +556,12 @@ func (e *Enforcer) enforce(matcher string, explains *[]string, rvals ...interfac
 		pTokens: pTokens,
 	}
 
-	var expression *govaluate.EvaluableExpression
 	hasEval := util.HasEval(expString)
-
 	if hasEval {
 		functions["eval"] = generateEvalFunction(functions, &parameters)
 	}
-
-	expression, err = govaluate.NewEvaluableExpressionWithFunctions(expString, functions)
+	var expression *govaluate.EvaluableExpression
+	expression, err = e.getAndStoreMatcherExpression(hasEval, expString, functions)
 	if err != nil {
 		return false, err
 	}
@@ -690,6 +696,23 @@ func (e *Enforcer) enforce(matcher string, explains *[]string, rvals ...interfac
 	return result, nil
 }
 
+func (e *Enforcer) getAndStoreMatcherExpression(hasEval bool, expString string, functions map[string]govaluate.ExpressionFunction) (*govaluate.EvaluableExpression, error) {
+	var expression *govaluate.EvaluableExpression
+	var err error
+	var cachedExpression, isPresent = e.matcherMap.Load(expString)
+
+	if !hasEval && isPresent {
+		expression = cachedExpression.(*govaluate.EvaluableExpression)
+	} else {
+		expression, err = govaluate.NewEvaluableExpressionWithFunctions(expString, functions)
+		if err != nil {
+			return nil, err
+		}
+		e.matcherMap.Store(expString, expression)
+	}
+	return expression, nil
+}
+
 // Enforce decides whether a "subject" can access a "object" with the operation "action", input parameters are usually: (sub, obj, act).
 func (e *Enforcer) Enforce(rvals ...interface{}) (bool, error) {
 	return e.enforce("", nil, rvals...)
@@ -741,18 +764,18 @@ func (e *Enforcer) BatchEnforceWithMatcher(matcher string, requests [][]interfac
 }
 
 // AddNamedMatchingFunc add MatchingFunc by ptype RoleManager
-func (e *Enforcer) AddNamedMatchingFunc(ptype, name string, fn defaultrolemanager.MatchingFunc) bool {
+func (e *Enforcer) AddNamedMatchingFunc(ptype, name string, fn rbac.MatchingFunc) bool {
 	if rm, ok := e.rmMap[ptype]; ok {
-		rm.(*defaultrolemanager.RoleManager).AddMatchingFunc(name, fn)
+		rm.AddMatchingFunc(name, fn)
 		return true
 	}
 	return false
 }
 
 // AddNamedDomainMatchingFunc add MatchingFunc by ptype to RoleManager
-func (e *Enforcer) AddNamedDomainMatchingFunc(ptype, name string, fn defaultrolemanager.MatchingFunc) bool {
+func (e *Enforcer) AddNamedDomainMatchingFunc(ptype, name string, fn rbac.MatchingFunc) bool {
 	if rm, ok := e.rmMap[ptype]; ok {
-		rm.(*defaultrolemanager.RoleManager).AddDomainMatchingFunc(name, fn)
+		rm.AddDomainMatchingFunc(name, fn)
 		return true
 	}
 	return false
diff --git a/enforcer_distributed.go b/enforcer_distributed.go
index b13b928..c8743c4 100644
--- a/enforcer_distributed.go
+++ b/enforcer_distributed.go
@@ -29,6 +29,8 @@ func (e *DistributedEnforcer) SetDispatcher(dispatcher persist.Dispatcher) {
 // AddPoliciesSelf provides a method for dispatcher to add authorization rules to the current policy.
 // The function returns the rules affected and error.
 func (d *DistributedEnforcer) AddPoliciesSelf(shouldPersist func() bool, sec string, ptype string, rules [][]string) (affected [][]string, err error) {
+	d.m.Lock()
+	defer d.m.Unlock()
 	if shouldPersist != nil && shouldPersist() {
 		var noExistsPolicy [][]string
 		for _, rule := range rules {
@@ -59,6 +61,8 @@ func (d *DistributedEnforcer) AddPoliciesSelf(shouldPersist func() bool, sec str
 // RemovePoliciesSelf provides a method for dispatcher to remove a set of rules from current policy.
 // The function returns the rules affected and error.
 func (d *DistributedEnforcer) RemovePoliciesSelf(shouldPersist func() bool, sec string, ptype string, rules [][]string) (affected [][]string, err error) {
+	d.m.Lock()
+	defer d.m.Unlock()
 	if shouldPersist != nil && shouldPersist() {
 		if err := d.adapter.(persist.BatchAdapter).RemovePolicies(sec, ptype, rules); err != nil {
 			if err.Error() != notImplemented {
@@ -82,6 +86,8 @@ func (d *DistributedEnforcer) RemovePoliciesSelf(shouldPersist func() bool, sec
 // RemoveFilteredPolicySelf provides a method for dispatcher to remove an authorization rule from the current policy, field filters can be specified.
 // The function returns the rules affected and error.
 func (d *DistributedEnforcer) RemoveFilteredPolicySelf(shouldPersist func() bool, sec string, ptype string, fieldIndex int, fieldValues ...string) (affected [][]string, err error) {
+	d.m.Lock()
+	defer d.m.Unlock()
 	if shouldPersist != nil && shouldPersist() {
 		if err := d.adapter.RemoveFilteredPolicy(sec, ptype, fieldIndex, fieldValues...); err != nil {
 			if err.Error() != notImplemented {
@@ -104,6 +110,8 @@ func (d *DistributedEnforcer) RemoveFilteredPolicySelf(shouldPersist func() bool
 
 // ClearPolicySelf provides a method for dispatcher to clear all rules from the current policy.
 func (d *DistributedEnforcer) ClearPolicySelf(shouldPersist func() bool) error {
+	d.m.Lock()
+	defer d.m.Unlock()
 	if shouldPersist != nil && shouldPersist() {
 		err := d.adapter.SavePolicy(nil)
 		if err != nil {
@@ -118,6 +126,8 @@ func (d *DistributedEnforcer) ClearPolicySelf(shouldPersist func() bool) error {
 
 // UpdatePolicySelf provides a method for dispatcher to update an authorization rule from the current policy.
 func (d *DistributedEnforcer) UpdatePolicySelf(shouldPersist func() bool, sec string, ptype string, oldRule, newRule []string) (affected bool, err error) {
+	d.m.Lock()
+	defer d.m.Unlock()
 	if shouldPersist != nil && shouldPersist() {
 		err := d.adapter.(persist.UpdatableAdapter).UpdatePolicy(sec, ptype, oldRule, newRule)
 		if err != nil {
@@ -146,6 +156,8 @@ func (d *DistributedEnforcer) UpdatePolicySelf(shouldPersist func() bool, sec st
 
 // UpdatePoliciesSelf provides a method for dispatcher to update a set of authorization rules from the current policy.
 func (d *DistributedEnforcer) UpdatePoliciesSelf(shouldPersist func() bool, sec string, ptype string, oldRules, newRules [][]string) (affected bool, err error) {
+	d.m.Lock()
+	defer d.m.Unlock()
 	if shouldPersist != nil && shouldPersist() {
 		err := d.adapter.(persist.UpdatableAdapter).UpdatePolicies(sec, ptype, oldRules, newRules)
 		if err != nil {
@@ -174,6 +186,8 @@ func (d *DistributedEnforcer) UpdatePoliciesSelf(shouldPersist func() bool, sec
 
 // UpdateFilteredPoliciesSelf provides a method for dispatcher to update a set of authorization rules from the current policy.
 func (d *DistributedEnforcer) UpdateFilteredPoliciesSelf(shouldPersist func() bool, sec string, ptype string, newRules [][]string, fieldIndex int, fieldValues ...string) (bool, error) {
+	d.m.Lock()
+	defer d.m.Unlock()
 	var (
 		oldRules [][]string
 		err      error
diff --git a/rbac/default-role-manager/role_manager.go b/rbac/default-role-manager/role_manager.go
index 6e69cf8..8939a48 100644
--- a/rbac/default-role-manager/role_manager.go
+++ b/rbac/default-role-manager/role_manager.go
@@ -21,13 +21,12 @@ import (
 
 	"github.com/casbin/casbin/v2/errors"
 	"github.com/casbin/casbin/v2/log"
+	"github.com/casbin/casbin/v2/rbac"
 	"github.com/casbin/casbin/v2/util"
 )
 
 const defaultDomain string = ""
 
-type MatchingFunc func(arg1 string, arg2 string) bool
-
 // Role represents the data structure for a role in RBAC.
 type Role struct {
 	name      string
@@ -168,8 +167,8 @@ func (r *Role) getUsers() []string {
 type RoleManagerImpl struct {
 	allRoles           *sync.Map
 	maxHierarchyLevel  int
-	matchingFunc       MatchingFunc
-	domainMatchingFunc MatchingFunc
+	matchingFunc       rbac.MatchingFunc
+	domainMatchingFunc rbac.MatchingFunc
 	logger             log.Logger
 	matchingFuncCache  *util.SyncLRUCache
 }
@@ -185,7 +184,7 @@ func NewRoleManagerImpl(maxHierarchyLevel int) *RoleManagerImpl {
 }
 
 // use this constructor to avoid rebuild of AddMatchingFunc
-func newRoleManagerWithMatchingFunc(maxHierarchyLevel int, fn MatchingFunc) *RoleManagerImpl {
+func newRoleManagerWithMatchingFunc(maxHierarchyLevel int, fn rbac.MatchingFunc) *RoleManagerImpl {
 	rm := NewRoleManagerImpl(maxHierarchyLevel)
 	rm.matchingFunc = fn
 	return rm
@@ -276,13 +275,13 @@ func (rm *RoleManagerImpl) removeRole(name string) {
 }
 
 // AddMatchingFunc support use pattern in g
-func (rm *RoleManagerImpl) AddMatchingFunc(name string, fn MatchingFunc) {
+func (rm *RoleManagerImpl) AddMatchingFunc(name string, fn rbac.MatchingFunc) {
 	rm.matchingFunc = fn
 	rm.rebuild()
 }
 
 // AddDomainMatchingFunc support use domain pattern in g
-func (rm *RoleManagerImpl) AddDomainMatchingFunc(name string, fn MatchingFunc) {
+func (rm *RoleManagerImpl) AddDomainMatchingFunc(name string, fn rbac.MatchingFunc) {
 	rm.domainMatchingFunc = fn
 }
 
@@ -439,8 +438,8 @@ func (rm *RoleManagerImpl) BuildRelationship(name1 string, name2 string, domain
 type DomainManager struct {
 	rmMap              *sync.Map
 	maxHierarchyLevel  int
-	matchingFunc       MatchingFunc
-	domainMatchingFunc MatchingFunc
+	matchingFunc       rbac.MatchingFunc
+	domainMatchingFunc rbac.MatchingFunc
 	logger             log.Logger
 	matchingFuncCache  *util.SyncLRUCache
 }
@@ -460,7 +459,7 @@ func (dm *DomainManager) SetLogger(logger log.Logger) {
 }
 
 // AddMatchingFunc support use pattern in g
-func (dm *DomainManager) AddMatchingFunc(name string, fn MatchingFunc) {
+func (dm *DomainManager) AddMatchingFunc(name string, fn rbac.MatchingFunc) {
 	dm.matchingFunc = fn
 	dm.rmMap.Range(func(key, value interface{}) bool {
 		value.(*RoleManagerImpl).AddMatchingFunc(name, fn)
@@ -469,7 +468,7 @@ func (dm *DomainManager) AddMatchingFunc(name string, fn MatchingFunc) {
 }
 
 // AddDomainMatchingFunc support use domain pattern in g
-func (dm *DomainManager) AddDomainMatchingFunc(name string, fn MatchingFunc) {
+func (dm *DomainManager) AddDomainMatchingFunc(name string, fn rbac.MatchingFunc) {
 	dm.domainMatchingFunc = fn
 	dm.rmMap.Range(func(key, value interface{}) bool {
 		value.(*RoleManagerImpl).AddDomainMatchingFunc(name, fn)
diff --git a/rbac/role_manager.go b/rbac/role_manager.go
index ac895df..18b4ca8 100644
--- a/rbac/role_manager.go
+++ b/rbac/role_manager.go
@@ -20,6 +20,8 @@ import (
 	"github.com/casbin/casbin/v2/log"
 )
 
+type MatchingFunc func(arg1 string, arg2 string) bool
+
 // RoleManager provides interface to define the operations for managing roles.
 type RoleManager interface {
 	// Clear clears all stored data and resets the role manager to the initial state.
@@ -49,6 +51,12 @@ type RoleManager interface {
 	PrintRoles() error
 	// SetLogger sets role manager's logger.
 	SetLogger(logger log.Logger)
+	// Match matches the domain with the pattern
+	Match(str string, pattern string) bool
+	// AddMatchingFunc adds the matching function
+	AddMatchingFunc(name string, fn MatchingFunc)
+	// AddDomainMatchingFunc adds the domain matching function
+	AddDomainMatchingFunc(name string, fn MatchingFunc)
 }
 
 // RoleManagerWithContext provides a context-aware interface to define the operations for managing roles.
diff --git a/rbac_api.go b/rbac_api.go
index 9c5b72e..7a44311 100644
--- a/rbac_api.go
+++ b/rbac_api.go
@@ -17,7 +17,6 @@ package casbin
 import (
 	"github.com/casbin/casbin/v2/constant"
 	"github.com/casbin/casbin/v2/errors"
-	"github.com/casbin/casbin/v2/rbac/default-role-manager"
 	"github.com/casbin/casbin/v2/util"
 )
 
@@ -308,7 +307,7 @@ func (e *Enforcer) GetNamedImplicitPermissionsForUser(ptype string, user string,
 			return nil, errors.ERR_DOMAIN_PARAMETER
 		} else {
 			d := domain[0]
-			matched := rm.(*defaultrolemanager.RoleManager).Match(d, rule[domainIndex])
+			matched := rm.Match(d, rule[domainIndex])
 			if !matched {
 				continue
 			}
diff --git a/util/builtin_operators.go b/util/builtin_operators.go
old mode 100755
new mode 100644
index fd24c64..ee091ce
--- a/util/builtin_operators.go
+++ b/util/builtin_operators.go
@@ -21,6 +21,7 @@ import (
 	"path"
 	"regexp"
 	"strings"
+	"sync"
 
 	"github.com/Knetic/govaluate"
 	"github.com/casbin/casbin/v2/rbac"
@@ -367,7 +368,7 @@ func GlobMatchFunc(args ...interface{}) (interface{}, error) {
 
 // GenerateGFunction is the factory method of the g(_, _[, _]) function.
 func GenerateGFunction(rm rbac.RoleManager) govaluate.ExpressionFunction {
-	memorized := map[string]bool{}
+	memorized := sync.Map{}
 	return func(args ...interface{}) (interface{}, error) {
 		// Like all our other govaluate functions, all args are strings.
 
@@ -386,7 +387,7 @@ func GenerateGFunction(rm rbac.RoleManager) govaluate.ExpressionFunction {
 		key := builder.String()
 
 		// ...and see if we've already calculated this.
-		v, found := memorized[key]
+		v, found := memorized.Load(key)
 		if found {
 			return v, nil
 		}
@@ -403,7 +404,7 @@ func GenerateGFunction(rm rbac.RoleManager) govaluate.ExpressionFunction {
 			v, _ = rm.HasLink(name1, name2, domain)
 		}
 
-		memorized[key] = v
+		memorized.Store(key, v)
 		return v, nil
 	}
 }


Test patch:
diff --git a/model_test.go b/model_test.go
index 072e894..f2c3437 100644
--- a/model_test.go
+++ b/model_test.go
@@ -366,6 +366,10 @@ func (rm *testCustomRoleManager) GetAllDomains() ([]string, error) {
 func (rm *testCustomRoleManager) PrintRoles() error           { return nil }
 func (rm *testCustomRoleManager) SetLogger(logger log.Logger) {}
 
+func (rm *testCustomRoleManager) Match(str string, pattern string) bool                   { return true }
+func (rm *testCustomRoleManager) AddMatchingFunc(name string, fn rbac.MatchingFunc)       {}
+func (rm *testCustomRoleManager) AddDomainMatchingFunc(name string, fn rbac.MatchingFunc) {}
+
 func TestRBACModelWithCustomRoleManager(t *testing.T) {
 	e, _ := NewEnforcer("examples/rbac_model.conf", "examples/rbac_policy.csv")
 	e.SetRoleManager(NewRoleManager())
diff --git a/rbac_api_test.go b/rbac_api_test.go
index 236dba4..e604572 100644
--- a/rbac_api_test.go
+++ b/rbac_api_test.go
@@ -20,7 +20,6 @@ import (
 	"testing"
 
 	"github.com/casbin/casbin/v2/errors"
-	defaultrolemanager "github.com/casbin/casbin/v2/rbac/default-role-manager"
 	"github.com/casbin/casbin/v2/util"
 )
 
@@ -307,7 +306,7 @@ func TestImplicitRoleAPI(t *testing.T) {
 
 	e, _ = NewEnforcer("examples/rbac_with_pattern_model.conf", "examples/rbac_with_pattern_policy.csv")
 
-	e.GetRoleManager().(*defaultrolemanager.RoleManager).AddMatchingFunc("matcher", util.KeyMatch)
+	e.GetRoleManager().AddMatchingFunc("matcher", util.KeyMatch)
 	e.AddNamedMatchingFunc("g2", "matcher", util.KeyMatch)
 
 	//testGetImplicitRoles(t, e, "cathy", []string{"/book/1/2/3/4/5", "pen_admin", "/book/*", "book_group"})