Traversal strategies #5

danizord · 2014-01-06T17:18:08Z

Finally! Sorry for the delay :D

Traversal strategies

The Graph traversal is a very complex problem, there are several ways to do this, but none of them is the best way for all cases. There are many cases that need a specific algorithm. For instance, If you have a very dense role graph, you'll probably want to avoid redundancy when traversing the graph (checking the same role more than once). But if you have a flatter role graph, then you will not care about the redundancy, because the logic to avoid redundancy would be more costly than the redundancy itself.

So, if we make this algorithm pluggable, the RBAC component will become much more flexible, allowing the developers to easily implement their own strategies for specific cases.

I have many ideas for other strategies to implement (e.g a non-redundant generator strategy), but for now I'll suggest two:

Built-in traversal strategies:

RecursiveRoleIteratorStrategy - The default strategy, it recursively traverses the roles using a SPL RecursiveIteratorIterator that iterate over instances of Rbac\Traversal\RecursiveRoleIterator.
GeneratorStrategy - This strategy requires PHP >= 5.5, the fastest and lightweight strategy! It takes advantage of the new PHP 5.5 generators to recursively traverse the role graph.

Benchmarks

I did some performance tests of the built-in strategies to know which one is faster. You can run the benchmarks yourself, just follow the instructions in the danizord/rbac-benchmarks repository. See the results on my PC:

daniel@Danibook:~/projects/rbac-benchmarks$ php vendor/bin/athletic -p src/ -b vendor/autoload.php

Danizord\RbacBenchmarks\Traversal\GeneratorStrategyEvent
    Method Name                   Iterations    Average Time      Ops/second
    ---------------------------  ------------  --------------    -------------
    flatFirstMatch             : [1,000     ] [0.0000078084469] [128,066.44072]
    flatLastMatch              : [1,000     ] [0.0000445151329] [22,464.27079]
    hierarchicalFirstDepthMatch: [1,000     ] [0.0000128533840] [77,800.52308]
    hierarchicalLastDepthMatch : [1,000     ] [0.0000390281677] [25,622.51979]


Danizord\RbacBenchmarks\Traversal\RecursiveIteratorStrategyEvent
    Method Name                   Iterations    Average Time      Ops/second
    ---------------------------  ------------  --------------    -------------
    flatFirstMatch             : [1,000     ] [0.0000188462734] [53,060.88783]
    flatLastMatch              : [1,000     ] [0.0000955798626] [10,462.45488]
    hierarchicalFirstDepthMatch: [1,000     ] [0.0000269591808] [37,093.11519]
    hierarchicalLastDepthMatch : [1,000     ] [0.0000535430908] [18,676.54602]

Additional changes

HierarchicalRoleInterface no longer extends RecursiveIterator, this means that the roles are not aware of traversal nor recursion anymore.
The recursive iteration that was previously in HierarchicalRole, now is done in the new RecursiveRoleIterator.
Rbac::isGranted() now now accepts a collection/array of RoleInterfaces, but still accepting a single RoleInterface instance. If one of the given roles has the required permission, then it returns true.
Some tests was updated, the code coverage of this component is now 100%. 😃

Closes zf-fr#4 because it no longer needs to implement RecursiveIterator

bakura10 · 2014-01-06T17:41:12Z

src/Rbac/Rbac.php

+     */
+    public function __construct(TraversalStrategyInterface $strategy = null)
+    {
+        $this->traversalStrategy = ($strategy) ? $strategy : new RecursiveRoleIteratorStrategy;


can be $this->traversalStrategy = $strategy ?: new RecursiveRoleIteratorStrategy();

If RecursiveRoleIteratorStrategy and GeneratorStrategy do exactly the same, you should add a check based on the PHP version, and use the most efficient one.

@bakura10 First, the $this->traversalStrategy must not be null. Second, I want to allow developers to use custom strategies, this is why I dont choose the strategy based on the PHP version.

The constructt $strategy ?: new Recursive() is equivalent to : if $straetgy is not null, use strategy, otherwise new Recursive.

It's a shortcut.

Yes I understood abut custom strategy, but you could do:

if (PHP_VER < 5.5) { // Not the right syntax $this->traversalStrategy = $strategy ?: new RecursiveRoleIteratorStrategy; } else { $this->traversalStrategy = $strategy ?: new GeneratorStrategy() }

Hmm, nice! I'll make this changes tonight.

bakura10 · 2014-01-06T18:04:34Z

Please note that tests are not passing on PHP 5.4 too. I think you'll have to skip some tests if not on PHP 5.5

danizord · 2014-01-06T18:48:13Z

I see, there is a standard way to do it?

bakura10 · 2014-01-06T20:09:11Z

@danizord , I don't remember the exact syntax. Please check the ZF2 tests, do a search on the source code, I don't remember the exact syntax.

bakura10 · 2014-01-06T20:14:57Z

I like the idea in overall.

The only thing I'm still not sure is really the Generator thing. This is just my opinion, but I still find the implementation a bit unusual (generators were clearly not thought for recursive it seems). Furthermore, we now have two implementations that do the same thing.

The performance question is here a bit tendentious: most of the time you have very few roles (even for a complex system I don't think you will have more than 10 roles), so the advantage is pretty negligeable. While I think it makes sense to have a polyfill for very critical components like the router (in this case you could have a loot of routes so the performance bump makes sense).

In this specific case, it seems to me like it is using generators for the sake of using generators. I really find the RecursiveIterator approach much more elegant for solving this issue.

Havign said that, I won't be against including it, but I'm not fond of it.

aeneasr · 2014-01-07T11:05:29Z

src/Rbac/Rbac.php

+     */
+    public function __construct(TraversalStrategyInterface $strategy = null)
+    {
+        $this->traversalStrategy = $strategy ?: new RecursiveRoleIteratorStrategy;


Why aren't you using the GeneratorStrategy by default? It's faster and doesn't have problems with doctrine collections

Because generators are not supported on PHP 5.4

Envoyé de mon iPhone

Le 7 janv. 2014 à 12:05, arekkas notifications@github.com a écrit :

In src/Rbac/Rbac.php:

/
class Rbac
{
/*

\* Determines if access is granted by checking the role and child roles for permission.

\* @var TraversalStrategyInterface

*/

protected $traversalStrategy;

/**

\* @param null|TraversalStrategyInterface $strategy

*/

public function __construct(TraversalStrategyInterface $strategy = null)

{

$this->traversalStrategy = $strategy ?: new RecursiveRoleIteratorStrategy;
Why aren't you using the GeneratorStrategy by default? It's faster and doesn't have problems with doctrine collections

—
Reply to this email directly or view it on GitHub.

Adding a check for the PHP version here would help.

I'm waiting for the result of the discussion: "GeneratorStrategy is good or bad" to make sure if I should set it by default.

Please: $this->traversalStrategy = $strategy ?: new RecursiveRoleIteratorStrategy();

Note the () at the end. I know it makes no difference but... it's on my namespace :muhaha: .D

@bakura10 done.

aeneasr · 2014-01-07T11:06:55Z

I like the idea to use the Strategy pattern for the graph traversal. What seems odd to me is that generators are faster than iterators, but then again, this is PHP so everything is possible ;)

In conclusion, I'm +1 with this!

Ocramius · 2014-01-07T13:32:45Z

src/Rbac/Rbac.php

-            /** @var RoleInterface $child */
-            if ($child->hasPermission($permission)) {
+        foreach ($iterator as $role) {
+            /* @var RoleInterface $role */


Instead, just consider $iterator a RoleInterface[]

That works even for return values in docblocks.

This is correct? $iterator is actually a Traversable and Traversable !== RoleInterface[]

@danizord [] at the end of a type means "a traversable of "

@Ocramius No, it means "an array of" AFAIK. So I'd suggest Traversable|RoleInterface[]. But I dont know if it will work as expected in PHPStorm. I'm using Sublime (:

danizord · 2014-01-07T13:53:33Z

@bakura10 This is not just for the sake of using generators. See, the RecursiveRoleIteratorStrategy needs about 3 classes with a lot of code (even extending ArrayIterator to reuse some code) to do the work, and the workflow is harder to undestard. While the GeneratorStrategy has just some lines of (readable) code in a single method class. So, I really dont see why it is not a good use case for generators. Consider:

Less classes,
Less lines of code,
More readable,
Faster.

generators were clearly not thought for recursive it seems

Why? https://github.com/nikic/iter/blob/master/src/iter.php#L490-500 😃

It almost solves the Doctrine collections incompatibility

bakura10 · 2014-01-07T21:15:31Z

src/Rbac/Traversal/RecursiveRoleIterator.php

+            return false;
+        }
+
+        if (empty($current->getChildren())) {


Please note that this only works on PHP 5.5, please do this as we need to support PHP 5.4:

$children = $current->getChildren(); if (empty($children)) { return false; }

return ! empty($children);

You think I can remove it?
Or pass this responsibility to the role, e.g HierarchicalRoleInterface::hasChildren()?

Well, getChildren will return $this->children, so yeah you can do return !empty($this->children)

bakura10 · 2014-01-07T21:17:51Z

It looks pretty good to me, excepting the few namings we should fix. @Ocramius do you think this is good?

However I'll want you to also update ZfcRbac to use this code so that I can push both PR at the same time (I suppose this can break ZfcRbac no?)

danizord · 2014-01-09T16:11:53Z

The issue with Doctrine collections was solved! Updating PR description.

bakura10 · 2014-01-09T21:31:24Z

Looks good to me.

Please could you update ZfcRbac so ZfcRbac does not stay in a zombie state after merging this?

bakura10 · 2014-01-09T21:34:41Z

Please also update the various .dist file, as well as the tests where I have simple entities based on previous Rbac.

danizord · 2014-01-13T04:06:36Z

src/Rbac/Rbac.php

+    {
+        if (null !== $strategy) {
+            $this->traversalStrategy = $strategy;
+        } elseif (version_compare(PHP_VERSION, '5.5.0', '>=')) {


It looks ugly =/ @bakura10 Don't you think a factory is the best place for this logic?

Also, use PHP_VERSION_ID instead.

I am with @danizord on this - the constructor argument should not be nullable (may be misunderstood) and injection should happen in a factory.

bakura10 · 2014-01-13T08:39:18Z

Ok, create a factory then

Traversal strategies

danizord added 4 commits January 6, 2014 02:01

Remove RecursiveIterator interface from hierarchical roles

a7f9f85

Closes zf-fr#4 because it no longer needs to implement RecursiveIterator

Add RecursiveRoleIterator

f092adc

Pluggable traversal algorithm with strategy pattern

0650437

Add GeneratorStrategy

5989bd7

bakura10 reviewed Jan 6, 2014
View reviewed changes

danizord added 2 commits January 6, 2014 21:19

Require PHP 5.5.0 in GeneratorStrategyTest

728b45e

Use ternary operator shortcut

6ebb713

aeneasr reviewed Jan 7, 2014
View reviewed changes

Ocramius reviewed Jan 7, 2014
View reviewed changes

Accept Traversable on RecursiveRoleIterator

4304f12

It almost solves the Doctrine collections incompatibility

bakura10 reviewed Jan 7, 2014
View reviewed changes

danizord added 3 commits January 9, 2014 10:38

Move empty() check to the role itself

27588ec

Rename traverseRoles -> getRolesIterator

25bde3e

Disallow duplicates

9023d07

Set default strategy based on PHP Version

fc38b5a

danizord mentioned this pull request Jan 13, 2014

Move traversal to Rbac component ZF-Commons/zfc-rbac#162

Merged

danizord reviewed Jan 13, 2014
View reviewed changes

danizord mentioned this pull request Jan 13, 2014

Remove RecursiveIterator implementation from HierarchicalRole ZF-Commons/zfc-rbac#163

Merged

bakura10 added a commit that referenced this pull request Jan 13, 2014

Merge pull request #5 from danizord/feature/traversal-strategies

1ee80ec

Traversal strategies

bakura10 merged commit 1ee80ec into zf-fr:master Jan 13, 2014

danizord mentioned this pull request Jul 16, 2015

Refactor zendframework/zend-permissions-rbac#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Traversal strategies #5

Traversal strategies #5

danizord commented Jan 6, 2014

bakura10 Jan 6, 2014

bakura10 Jan 6, 2014

danizord Jan 6, 2014

bakura10 Jan 6, 2014

danizord Jan 6, 2014

bakura10 commented Jan 6, 2014

danizord commented Jan 6, 2014

bakura10 commented Jan 6, 2014

bakura10 commented Jan 6, 2014

aeneasr Jan 7, 2014

bakura10 Jan 7, 2014

Ocramius Jan 7, 2014

danizord Jan 7, 2014

bakura10 Jan 7, 2014

danizord Jan 9, 2014

aeneasr commented Jan 7, 2014

Ocramius Jan 7, 2014

danizord Jan 7, 2014

Ocramius Jan 7, 2014

danizord Jan 8, 2014

danizord commented Jan 7, 2014

bakura10 Jan 7, 2014

Ocramius Jan 7, 2014

danizord Jan 8, 2014

bakura10 Jan 8, 2014

bakura10 commented Jan 7, 2014

danizord commented Jan 9, 2014

bakura10 commented Jan 9, 2014

bakura10 commented Jan 9, 2014

danizord Jan 13, 2014

Ocramius Jan 13, 2014

bakura10 commented Jan 13, 2014

Traversal strategies #5

Traversal strategies #5

Conversation

danizord commented Jan 6, 2014

Traversal strategies

Built-in traversal strategies:

Benchmarks

Additional changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakura10 commented Jan 6, 2014

danizord commented Jan 6, 2014

bakura10 commented Jan 6, 2014

bakura10 commented Jan 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Jan 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danizord commented Jan 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakura10 commented Jan 7, 2014

danizord commented Jan 9, 2014

bakura10 commented Jan 9, 2014

bakura10 commented Jan 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakura10 commented Jan 13, 2014