Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Idea] Use cache on models(Optional trait) #1833

Closed
wants to merge 1 commit into from

Conversation

erikn69
Copy link
Contributor

@erikn69 erikn69 commented Aug 31, 2021

Maybe needs work

According #1760 with https://stackoverflow.com/a/56263511 info

Optional trait for cache models relations, using config('session.lifetime') as expiration time

@MrRio
Copy link

MrRio commented Sep 15, 2021

This could be really good 👀

@freekmurze
Copy link
Member

I'm going to pass on this for now.

@freekmurze freekmurze closed this Oct 29, 2021
@cmple
Copy link

cmple commented Nov 1, 2021

@freekmurze can you make model caching a top priority? Currently it's sending 4 SQL requests with each page load, which I think it not really efficient.
COME-ON-MAN--CACHE-THIS
Thank you!!

@Restingo
Copy link

@erikn69 what happend to this PR?

@erikn69
Copy link
Contributor Author

erikn69 commented Jan 25, 2022

I'm going to pass on this for now.

@Restingo was closed, you can implement it on your own app if you need it

@Restingo
Copy link

@erikn69 thanks for your fast reply! Is it right that the current state of the package doesn't cache the relations (like this PR would do) so this is also the reason for performance issues with a large amount of roles and/or permissions?

Do I need to fork the package completely or can I only change the role and permission classes to implement it myself?

@erikn69
Copy link
Contributor Author

erikn69 commented Jan 25, 2022

You can change your models and add the trait on User.php, needs a workaround but works, no forks needed

@Restingo
Copy link

So I don't need to implement the changes in the two traits and src/PermissionRegistrar.php?

@erikn69
Copy link
Contributor Author

erikn69 commented Jan 25, 2022

needs a workaround but works

depends on your needs, works for me without those changes, with a workaround

src/PermissionRegistrar.php already has that change

public function getCacheRepository(): Repository
{
return $this->cache;
}

@Restingo
Copy link

Would you be so kind to paste the workaround? Would be very helpful. Thank you!

@erikn69
Copy link
Contributor Author

erikn69 commented Jan 25, 2022

Try this if you are not using teams

use Illuminate\Database\Eloquent\Collection;
use Spatie\Permission\PermissionRegistrar;

trait HasPermissionsCache {
    protected function getPermissionCacheKey(string $relation): string {
        return sprintf(PermissionRegistrar::$cacheKey.'.'.str_replace('\\','.',$this->getMorphClass()).'.%d.'.$relation, $this->getKey());
    }
    private function getCachedPermissions(string $relation){
        if ($this->relationLoaded($relation)) {
            return $this->getRelationValue($relation);
        }
        
        $cache = app(PermissionRegistrar::class)->getCacheRepository();
        $cacheIsTaggable = $cache->getStore() instanceof \Illuminate\Cache\TaggableStore;
        $class = $relation === 'roles' ? $this->getRoleClass() : $this->getPermissionClass();
        $collection = $class::hydrate(
            collect(($cacheIsTaggable?$cache->tags(['permissions']):$cache)->remember($this->getPermissionCacheKey($relation), config('session.lifetime', 120)* 60, function () use($relation) {
                return $this->getRelationValue($relation)->map(function ($data) {
                    return ['i' => $data->id, 'n' => $data->name, 'g' => $data->guard_name];
                })->all();
            }))
            ->map(function ($item) { return ['id' => $item['i'], 'name' => $item['n'], 'guard_name' => $item['g']]; })->all()
        );
        $this->setRelation($relation, $collection);
        return $collection;
    }
    public function getRolesAttribute(): Collection {
        return $this->getCachedPermissions('roles');
    }
    public function getPermissionsAttribute(): Collection {
        return $this->getCachedPermissions('permissions');
    }
    public function forgetModelAssignedPermissions() {
        $cache = app(PermissionRegistrar::class)->getCacheRepository();
        $cache->forget($this->getPermissionCacheKey('roles'));
        $cache->forget($this->getPermissionCacheKey('permissions'));
    }
    public static function forgetAllModelsAssignedPermissions(){
        $cache = app(PermissionRegistrar::class)->getCacheRepository();
        $cacheIsTaggable = $cache->getStore() instanceof \Illuminate\Cache\TaggableStore;
        if($cacheIsTaggable){
            $cache->tags(['permissions'])->flush();
        }else{
            static::select((new static)->getKeyName())->get()->each(function ($model) use ($cache) {
                $cache->forget($model->getPermissionCacheKey('roles'));
                $cache->forget($model->getPermissionCacheKey('permissions'));
            });
        }
    }
}

@Restingo
Copy link

Thanks I'll try!

@erikn69
Copy link
Contributor Author

erikn69 commented Jan 25, 2022

Or better on User model

public function load($relations){
    $relations = is_string($relations) ? func_get_args() : $relations;
    foreach ($relations as $i=>$relation) {
        $method = 'forgetModelCached' . ucfirst((string) $relation);
        if (in_array($relation, ['roles', 'permissions']) && method_exists($this, $method)) {
            $this->$method();
            $this->loadCachedRelation($relation);
            unset($relations[$i]);
        }
    }
    return empty($relations) ? $this : parent::load($relations);
}
public function loadMissing($relations){
    $relations = is_string($relations) ? func_get_args() : $relations;
    foreach ($relations as $i=>$relation) {
        $method = 'forgetModelCached' . ucfirst((string) $relation);
        if (in_array($relation, ['roles', 'permissions']) && method_exists($this, $method)) {
            if (!$this->relationLoaded($relation)) {
                $this->loadCachedRelation($relation);
            }
            unset($relations[$i]);
        }
    }
    return empty($relations) ? $this : parent::loadMissing($relations);
}

And the same HasPermissionsCache of this PR

@Restingo
Copy link

So even with this tweak I've got a loading time with 72 roles assigned of 1.06 s and with only one role it's around 200 ms. Is there another way to improve?

@Restingo
Copy link

But I can see that the cache is working with Redis as expected!

@erikn69
Copy link
Contributor Author

erikn69 commented Jan 25, 2022

feel free to make a PR for improve

I have a lot of permissions, only 12 roles
image
Redis
image

$cache = app(PermissionRegistrar::class)->getCacheRepository();
$cacheIsTaggable = $cache->getStore() instanceof \Illuminate\Cache\TaggableStore;
if ($cacheIsTaggable) {
$cache->tags(['permissions'.str_replace('\\', '.', $this->getMorphClass())])->flush();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the use of $this in static context wrong?

Copy link
Contributor Author

@erikn69 erikn69 Jan 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe needs work

👍 This PR is closed, fell free to make all the fixes you need on your implementation

@JohnRoux
Copy link

This would be really nice on many projects.
I understand not wanting to have it as the default, but having this as an optional trait / feature flag would be incredible! <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants