Optimize calls for getting child items within the loops (#209)

* Optimize calls for getting child items within the loops * Apply fixes from StyleCI * Update CHANGELOG [skip ci] * Fix Psalm * Add tests --------- Co-authored-by: StyleCI Bot <bot@styleci.io>
yiisoft · Dec 6, 2023 · 713fdbf · 713fdbf
1 parent 674b23b
commit 713fdbf
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 52 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -44,6 +44,9 @@
 - Enh #203: Add `getGuestRoleName()` and `getGuestRole()` methods to `Manager` (@arogachev)
 - Chg #203: Throw `RuntimeException` in the case with implicit guest and non-existing guest role in 
 `Manager::userHasPermission()` (@arogachev)
+- Enh #206: Optimize calls for getting child items within the loops (@arogachev)
+- Chg #206: Rename `$name` argument to `$names` and allow array type for it in `getAllChildren()`, `getAllChildRoles()`,
+  `getAllChildPermissions()` methods of `ItemsStorageInterface` (@arogachev)
 
 ## 1.0.2 April 20, 2023
 

diff --git a/src/ItemsStorageInterface.php b/src/ItemsStorageInterface.php
@@ -190,32 +190,32 @@ public function getDirectChildren(string $name): array;
     /**
      * Returns all child permissions and/or roles.
      *
-     * @param string $name The parent name.
+     * @param array|string $names The parent name / names.
      *
      * @return array The child permissions and/or roles.
      * @psalm-return ItemsIndexedByName
      */
-    public function getAllChildren(string $name): array;
+    public function getAllChildren(string|array $names): array;
 
     /**
      * Returns all child roles.
      *
-     * @param string $name The parent name.
+     * @param array|string $names The parent name / names.
      *
      * @return Role[] The child roles.
      * @psalm-return array<string, Role>
      */
-    public function getAllChildRoles(string $name): array;
+    public function getAllChildRoles(string|array $names): array;
 
     /**
      * Returns all child permissions.
      *
-     * @param string $name The parent name.
+     * @param array|string $names The parent name / names.
      *
      * @return Permission[] The child permissions.
      * @psalm-return array<string, Permission>
      */
-    public function getAllChildPermissions(string $name): array;
+    public function getAllChildPermissions(string|array $names): array;
 
     /**
      * Returns whether named parent has children.

diff --git a/src/Manager.php b/src/Manager.php
@@ -182,32 +182,26 @@ public function getItemsByUserId(int|Stringable|string $userId): array
     {
         $userId = (string) $userId;
         $assignments = $this->assignmentsStorage->getByUserId($userId);
-        $items = array_merge(
+        $assignmentNames = array_keys($assignments);
+
+        return array_merge(
             $this->getDefaultRoles(),
-            $this->itemsStorage->getByNames(array_keys($assignments)),
+            $this->itemsStorage->getByNames($assignmentNames),
+            $this->itemsStorage->getAllChildren($assignmentNames),
         );
-
-        foreach ($assignments as $assignment) {
-            $items = array_merge($items, $this->itemsStorage->getAllChildren($assignment->getItemName()));
-        }
-
-        return $items;
     }
 
     public function getRolesByUserId(int|Stringable|string $userId): array
     {
         $userId = (string) $userId;
         $assignments = $this->assignmentsStorage->getByUserId($userId);
-        $roles = array_merge(
+        $assignmentNames = array_keys($assignments);
+
+        return array_merge(
             $this->getDefaultRoles(),
-            $this->itemsStorage->getRolesByNames(array_keys($assignments)),
+            $this->itemsStorage->getRolesByNames($assignmentNames),
+            $this->itemsStorage->getAllChildRoles($assignmentNames)
         );
-
-        foreach ($assignments as $assignment) {
-            $roles = array_merge($roles, $this->itemsStorage->getAllChildRoles($assignment->getItemName()));
-        }
-
-        return $roles;
     }
 
     public function getChildRoles(string $roleName): array
@@ -228,16 +222,12 @@ public function getPermissionsByUserId(int|Stringable|string $userId): array
     {
         $userId = (string) $userId;
         $assignments = $this->assignmentsStorage->getByUserId($userId);
-        $permissions = $this->itemsStorage->getPermissionsByNames(array_keys($assignments));
-
-        foreach ($assignments as $assignment) {
-            $permissions = array_merge(
-                $permissions,
-                $this->itemsStorage->getAllChildPermissions($assignment->getItemName()),
-            );
-        }
+        $assignmentNames = array_keys($assignments);
 
-        return $permissions;
+        return array_merge(
+            $this->itemsStorage->getPermissionsByNames($assignmentNames),
+            $this->itemsStorage->getAllChildPermissions($assignmentNames),
+        );
     }
 
     public function getUserIdsByRoleName(string $roleName): array

diff --git a/tests/Common/ItemsStorageTestTrait.php b/tests/Common/ItemsStorageTestTrait.php
@@ -190,16 +190,21 @@ public function dataGetAllChildren(): array
                 'posts.admin',
                 ['posts.redactor', 'posts.viewer', 'posts.view', 'posts.create', 'posts.update', 'posts.delete'],
             ],
+            [['Parent 1', 'Parent 2'], ['Child 1', 'Child 2', 'Child 3']],
+            [
+                ['posts.viewer', 'posts.redactor', 'posts.admin'],
+                ['posts.redactor', 'posts.viewer', 'posts.view', 'posts.create', 'posts.update', 'posts.delete'],
+            ],
             ['non-existing', []],
         ];
     }
 
     /**
      * @dataProvider dataGetAllChildren
      */
-    public function testGetAllChildren(string $parentName, array $expectedChildren): void
+    public function testGetAllChildren(string|array $parentNames, array $expectedChildren): void
     {
-        $children = $this->getItemsStorage()->getAllChildren($parentName);
+        $children = $this->getItemsStorage()->getAllChildren($parentNames);
         $this->assertChildren($children, $expectedChildren);
     }
 
@@ -215,16 +220,21 @@ public function dataGetAllChildPermissions(): array
             ['posts.viewer', ['posts.view']],
             ['posts.redactor', ['posts.view', 'posts.create', 'posts.update']],
             ['posts.admin', ['posts.view', 'posts.create', 'posts.update', 'posts.delete']],
+            [['Parent 1', 'Parent 5'], ['Child 1', 'Child 5']],
+            [
+                ['posts.viewer', 'posts.redactor', 'posts.admin'],
+                ['posts.view', 'posts.create', 'posts.update', 'posts.delete'],
+            ],
             ['non-existing', []],
         ];
     }
 
     /**
      * @dataProvider dataGetAllChildPermissions
      */
-    public function testGetAllChildPermissions(string $parentName, array $expectedChildren): void
+    public function testGetAllChildPermissions(string|array $parentNames, array $expectedChildren): void
     {
-        $children = $this->getItemsStorage()->getAllChildPermissions($parentName);
+        $children = $this->getItemsStorage()->getAllChildPermissions($parentNames);
         $this->assertChildren($children, $expectedChildren);
     }
 
@@ -240,16 +250,18 @@ public function dataGetAllChildRoles(): array
             ['posts.viewer', []],
             ['posts.redactor', ['posts.viewer']],
             ['posts.admin', ['posts.redactor', 'posts.viewer']],
+            [['Parent 2', 'Parent 4'], ['Child 2', 'Child 3', 'Child 4']],
+            [['posts.viewer', 'posts.redactor', 'posts.admin'], ['posts.redactor', 'posts.viewer']],
             ['non-existing', []],
         ];
     }
 
     /**
      * @dataProvider dataGetAllChildRoles
      */
-    public function testGetAllChildRoles(string $parentName, array $expectedChildren): void
+    public function testGetAllChildRoles(string|array $parentNames, array $expectedChildren): void
     {
-        $children = $this->getItemsStorage()->getAllChildRoles($parentName);
+        $children = $this->getItemsStorage()->getAllChildRoles($parentNames);
         $this->assertChildren($children, $expectedChildren);
     }
 

diff --git a/tests/Support/FakeItemsStorage.php b/tests/Support/FakeItemsStorage.php
@@ -102,26 +102,26 @@ public function getDirectChildren(string $name): array
         return $this->children[$name] ?? [];
     }
 
-    public function getAllChildren(string $name): array
+    public function getAllChildren(string|array $names): array
     {
         $result = [];
-        $this->fillChildrenRecursive($name, $result);
+        $this->getAllChildrenInternal($names, $result);
 
         return $result;
     }
 
-    public function getAllChildRoles(string $name): array
+    public function getAllChildRoles(string|array $names): array
     {
         $result = [];
-        $this->fillChildrenRecursive($name, $result);
+        $this->getAllChildrenInternal($names, $result);
 
         return $this->filterRoles($result);
     }
 
-    public function getAllChildPermissions(string $name): array
+    public function getAllChildPermissions(string|array $names): array
     {
         $result = [];
-        $this->fillChildrenRecursive($name, $result);
+        $this->getAllChildrenInternal($names, $result);
 
         return $this->filterPermissions($result);
     }
@@ -290,6 +290,14 @@ private function fillAccessTreeRecursive(string $name, array &$result, array $ad
         }
     }
 
+    private function getAllChildrenInternal(string|array $names, array &$result): void
+    {
+        $names = (array) $names;
+        foreach ($names as $name) {
+            $this->fillChildrenRecursive($name, $result);
+        }
+    }
+
     private function fillChildrenRecursive(string $name, array &$result): void
     {
         $children = $this->children[$name] ?? [];
@@ -327,14 +335,9 @@ private function filterRoles(array $array): array
      */
     private function filterPermissions(array $items): array
     {
-        $permissions = [];
-        foreach (array_keys($items) as $permissionName) {
-            $permission = $this->getPermission($permissionName);
-            if ($permission !== null) {
-                $permissions[$permissionName] = $permission;
-            }
-        }
-
-        return $permissions;
+        return array_filter(
+            $this->getPermissions(),
+            static fn (Permission $permissionItem): bool => array_key_exists($permissionItem->getName(), $items),
+        );
     }
 }