feat: add IPatternFuncs

[abichinger]

fix: #1027

Changelog

• add interface IPatternMatcher
• add function SetNamedRoleMatcher
• add function SetNamedDomainMatcher
• mark AddNamedMatchingFunc and AddNamedDomainMatchingFunc as deprecated (these function should be removed with the next major release)

@kizjig please review these changes.
roleManager.AddDomainMatchingFunc("KeyMatch", util.KeyMatch) needs to be replaced with:

domainMatcher := defaultrolemanager.NewPatternMatcher(util.IsKeyMatch4Pattern, util.KeyMatch4)
roleManager.SetDomainMatcher(domainMatcher)

Benchmarks

before:
BenchmarkBuildRoleLinksWithPatternLarge-12                             1        11420789697 ns/op       5610426576 B/op 63632133 allocs/op
BenchmarkBuildRoleLinksWithDomainPatternLarge-12                      70          16872961 ns/op         4251498 B/op      86735 allocs/op
BenchmarkBuildRoleLinksWithPatternAndDomainPatternLarge-12             1        11617835457 ns/op       5619315344 B/op 63660158 allocs/op

after:
BenchmarkBuildRoleLinksWithPatternLarge-12                           135           8714727 ns/op         2660236 B/op      62784 allocs/op
BenchmarkBuildRoleLinksWithDomainPatternLarge-12                     142           8885176 ns/op         2848862 B/op      62765 allocs/op
BenchmarkBuildRoleLinksWithPatternAndDomainPatternLarge-12           130           9349494 ns/op         2896310 B/op      65735 allocs/op

Signed-off-by: Andreas Bichinger [email protected]

[casbin-bot]

@tangyang9464 @closetool @sagilio please review

[hsluoyz]

@tangyang9464 @JalinWang @imp2002 plz review

[sagilio]

LGTM, the new matcher uses IsPattern to pre-check the tokens whether needed to call the Match function to avoid calling duplicate and useless match functions and storing useless match results.

[tangyang9464]

lgtm

[kizjig]

Thank you for these changes. Looks good to me too.

I have a related question, forgive me if I'm misunderstanding something here btw... If we create a role/domain manager with a max hierarchy of less than the default 10, for example 4. How is that value preserved when the enforcer has hardcoded calls to recreate the role manager with functions like this?

func (e *Enforcer) initRmMap() {
	for ptype := range e.model["g"] {
		if rm, ok := e.rmMap[ptype]; ok {
			_ = rm.Clear()
		} else {
			e.rmMap[ptype] = defaultrolemanager.NewRoleManager(10)
		}
	}
}```

[kizjig]

And as a follow up clarification question...

What's the best flow or best time to customise/recreate the role manager in terms of setting domain matching functions and max hierarchy level?

Should it be once we've created the enforcer with our model but before we've done the first initial load of the policy data?

[tangyang9464]

@kizjig In fact, you must call e.SetRoleManager (need to customize RoleManger, including setting max hierarchy level and domain matching functions) after NewEnforcer, then rebuild role links(e.BuildRoleLinks). Code like

e, _ := NewEnforcer("","")

roleManager := NewRoleManager(4)

domainMatcher := defaultrolemanager.NewPatternMatcher(util.IsKeyMatch4Pattern, util.KeyMatch4)
roleManager.SetDomainMatcher(domainMatcher)

e.SetRoleManager(roleManager)
e.BuildRoleLinks()

[kizjig]

@kizjig In fact, you must call e.SetRoleManager (need to customize RoleManger, including setting max hierarchy level and domain matching functions) after NewEnforcer, then rebuild role links(e.BuildRoleLinks). Code like

e, _ := NewEnforcer("","")

roleManager := NewRoleManager(4)

domainMatcher := defaultrolemanager.NewPatternMatcher(util.IsKeyMatch4Pattern, util.KeyMatch4)
roleManager.SetDomainMatcher(domainMatcher)

e.SetRoleManager(roleManager)
e.BuildRoleLinks()

Thanks. So if I take that approach in your code above, and then afterwards load policy data from disk/file, it will automatically build the role links for the incoming data right (assuming e.EnableAutoBuildRoleLinks(true) is already set)?

I'm trying to avoid loading/processing the data twice. So I see the flow working as something like...

1. create enforcer
2. customise new role manager and domain matching.
3. set new role manager on enforcer
4. Set e.EnableAutoBuildRoleLinks(true)
5. load policy data from disk into enforcer internal memory model.

Does that sound correct?

[tangyang9464]

@kizjig do like this

// 1. don't set adapter first to avoid calling LoadPolicy automatically.
e, _ := NewEnforcer(model)

// 2. customise new role manager and domain matching.
roleManager := NewRoleManager(4)
domainMatcher := defaultrolemanager.NewPatternMatcher(util.IsKeyMatch4Pattern, util.KeyMatch4)
roleManager.SetDomainMatcher(domainMatcher)

// 3. set new role manager and adapter on enforcer
e.SetRoleManager(roleManager)
e.SetAdapter(adapter)

// 4. load policy
e.LoadPolicy()

Unfortunately, autoBuildRoleLinks is set to true by default when the enforcer is created, so e.EnableAutoBuildRoleLinks(true) has no effect. Maybe we will add a NewEnforcerWithOutBuild API to set autoBuildRoleLinks when creating an enforcer. So for now, we can only do it like the above

[kizjig]

Yes. thanks. That's what I had in mind. I am glad I was on the same page.

[kizjig]

Can we get this change progressed and merged in soon please?

[hsluoyz]

@abichinger SetNamedRoleMatcher should be renamed to SetNamedRoleMatchingFunc. Matcher is a reserved keyword to specify the matcher (m = xxxx part) in PERM metamodel. It should not be pointed to anything else in Casbin. Actually in Casbin, anything called XXXer should be a complete and non-trivial component, like adapter, watcher, etc. A matching function is quite small and should not be named after XXXer.

[abichinger]

@hsluoyz renaming SetNamedRoleMatcher to SetNamedRoleMatchingFunc and removing the reserved word Matcher would probably lead to the following function definition: (first solution)

//drm is defaultrolemanager
func (e *Enforcer) SetNamedRoleMatchingFunc(ptype string, isMatch drm.MatchingFunc, isPattern drm.IsPatternFunc) bool

IPatternMatcher would be split into MatchingFunc and IsPatternFunc
This would lead to minor disadvantages over the current solution:

• we could remove redundant code if we move the cache from DomainManager and RoleManagerImpl to IPatternMatcher
• no default implementations of IPatternMatcher like PrefixMatcher

How about I do the following renames: (second solution)

• rename SetNamedRoleMatcher to SetPatternFuncsForRoles
• rename SetNamedDomainMatcher to SetPatternFuncsForDomains
• rename IPatternMatcher to IPatternFuncs
• rename PrefixMatcher to PatternFuncsWithPrefix

I would prefer the second solution.
Please let me know which solution you would like me to implement.

[timabilov]

why this one is not merged yet?