Merge pull request #1835 from erikn69/upgrade

[V5] Merge pull request #1834 from danilopinotti/avoid-overhydratation

Author: drbyte

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 All notable changes to `laravel-permission` will be documented in this file
 
+## 5.1.1 - 2021-09-01
+- Avoid Roles over-hydration #1834
+
 ## 5.1.0 - 2021-08-31
 - No longer flush cache on User role/perm assignment changes #1832
   NOTE: You should test your app to be sure that you don't accidentally have deep dependencies on cache resets happening automatically in these cases.
@@ -12,6 +15,8 @@ All notable changes to `laravel-permission` will be documented in this file
 - Teams/Groups feature (see docs, or PR #1804)
 - Customized pivots instead of `role_id`,`permission_id` #1823
 
+## 4.4.1 - 2021-09-01
+- Avoid Roles over-hydration #1834
 
 ## 4.4.0 - 2021-08-28
 - Avoid BC break (removed interface change) on cache change added in 4.3.0 #1826

src/PermissionRegistrar.php

@@ -47,6 +47,9 @@ class PermissionRegistrar
     /** @var string */
     public static $cacheKey;
 
+    /** @var array */
+    private $cachedRoles = [];
+
     /**
      * PermissionRegistrar constructor.
      *
@@ -153,30 +156,36 @@ public function clearClassPermissions()
      */
     private function loadPermissions()
     {
-        if ($this->permissions === null) {
-            $this->permissions = $this->cache->remember(self::$cacheKey, self::$cacheExpirationTime, function () {
-                // make the cache smaller using an array with only required fields
-                return $this->getPermissionClass()->select('id', 'id as i', 'name as n', 'guard_name as g')
-                    ->with('roles:id,id as i,name as n,guard_name as g')->get()
-                    ->map(function ($permission) {
-                        return $permission->only('i', 'n', 'g') +
-                            ['r' => $permission->roles->map->only('i', 'n', 'g')->all()];
-                    })->all();
+        if ($this->permissions !== null) {
+            return;
+        }
+
+        $this->permissions = $this->cache->remember(self::$cacheKey, self::$cacheExpirationTime, function () {
+            // make the cache smaller using an array with only required fields
+            return $this->getPermissionClass()->select('id', 'id as i', 'name as n', 'guard_name as g')
+                ->with('roles:id,id as i,name as n,guard_name as g')->get()
+                ->map(function ($permission) {
+                    return $permission->only('i', 'n', 'g') +
+                        ['r' => $permission->roles->map->only('i', 'n', 'g')->all()];
+                })->all();
+        });
+
+        if (is_array($this->permissions)) {
+            $this->permissions = $this->getPermissionClass()::hydrate(
+                collect($this->permissions)->map(function ($item) {
+                    return ['id' => $item['i'] ?? $item['id'], 'name' => $item['n'] ?? $item['name'], 'guard_name' => $item['g'] ?? $item['guard_name']];
+                })->all()
+            )
+            ->each(function ($permission, $i) {
+                $roles = Collection::make($this->permissions[$i]['r'] ?? $this->permissions[$i]['roles'] ?? [])
+                        ->map(function ($item) {
+                            return $this->getHydratedRole($item);
+                        });
+
+                $permission->setRelation('roles', $roles);
             });
-            if (is_array($this->permissions)) {
-                $this->permissions = $this->getPermissionClass()::hydrate(
-                    collect($this->permissions)->map(function ($item) {
-                        return ['id' => $item['i'] ?? $item['id'], 'name' => $item['n'] ?? $item['name'], 'guard_name' => $item['g'] ?? $item['guard_name']];
-                    })->all()
-                )
-                ->each(function ($permission, $i) {
-                    $permission->setRelation('roles', $this->getRoleClass()::hydrate(
-                        collect($this->permissions[$i]['r'] ?? $this->permissions[$i]['roles'] ?? [])->map(function ($item) {
-                            return ['id' => $item['i'] ?? $item['id'], 'name' => $item['n'] ?? $item['name'], 'guard_name' => $item['g'] ?? $item['guard_name']];
-                        })->all()
-                    ));
-                });
-            }
+
+            $this->cachedRoles = [];
         }
     }
 
@@ -247,4 +256,22 @@ public function getCacheStore(): \Illuminate\Contracts\Cache\Store
     {
         return $this->cache->getStore();
     }
+
+    private function getHydratedRole(array $item)
+    {
+        $roleId = $item['i'] ?? $item['id'];
+
+        if (isset($this->cachedRoles[$roleId])) {
+            return $this->cachedRoles[$roleId];
+        }
+
+        $roleClass = $this->getRoleClass();
+        $roleInstance = new $roleClass;
+
+        return $this->cachedRoles[$roleId] = $roleInstance->newFromBuilder([
+            'id' => $roleId,
+            'name' => $item['n'] ?? $item['name'],
+            'guard_name' => $item['g'] ?? $item['guard_name'],
+        ]);
+    }
 }

tests/CacheTest.php

@@ -247,6 +247,17 @@ public function get_all_permissions_should_use_the_cache()
         $this->assertQueryCount(2);
     }
 
+    /** @test */
+    public function get_all_permissions_should_not_over_hydrate_roles()
+    {
+        $this->testUserRole->givePermissionTo(['edit-articles', 'edit-news']);
+        $permissions = $this->registrar->getPermissions();
+        $roles = $permissions->flatMap->roles;
+
+        // Should have same object reference
+        $this->assertSame($roles[0], $roles[1]);
+    }
+
     /** @test */
     public function it_can_reset_the_cache_with_artisan_command()
     {