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

slow hasPermissionTo #550

Closed
andrubeldie opened this Issue Nov 2, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@andrubeldie

andrubeldie commented Nov 2, 2017

Hi,

I have the following context:
142 permissions attached to 27 roles, 2000 users - vast majority with one or more roles, only few with direct permissions, and a blade template that use @can for 101 times in order to generate a menu according to current user's permissions.

rendering this menu is very time consuming (~4 seconds) and I discovered that problem is with hasPermissionTo.

in order to not touch laravel-permissions files the solution was to overwrite the method in the my model and to do some cache:

public function hasPermissionTo($permission, $guardName = null): bool
    {
        if (is_string($permission)) {
            $permission = \Cache::remember('spatie.permissions.'.$permission.'.'.$guardName, 3600, function()use($permission, $guardName){
                return app(Permission::class)->findByName(
                    $permission,
                    $guardName ?? $this->getDefaultGuardName()
                );
            });
        }
        return $this->hasDirectPermission($permission) || $this->hasPermissionViaRole($permission);
    }

what I achieved with this cache is to render that menu in 0.03 seconds.

I write you in hope you'll find a way to optimize this method inside this awesome laravel module :)

@andrubeldie

This comment has been minimized.

andrubeldie commented Nov 2, 2017

in order to test what is slow in the process, I used a console command like this:

    public function handle()
    {
        Auth::loginUsingId(SOME_VALID_USER_ID);
        for ($i=0; $i<100; $i++){
            $time_start = microtime(true);
            auth()->user()->can('some_existent_permission');
            $time_end = microtime(true);
            $t = $time_end - $time_start;
            $time[] = $t;
        }
        dd($time, array_sum($time));
    }
@AlexVanderbist

This comment has been minimized.

Member

AlexVanderbist commented Nov 3, 2017

Hi,

Thanks for all your work looking into this. Ideally these cached permissions would come from app(PermissionRegistrar::class)->getPermissions() as they're already being cached there.

This is definitely something we'll address in the next version of the package!

Thanks again!

@andrubeldie

This comment has been minimized.

andrubeldie commented Nov 3, 2017

lovely!

I would also strongly recommend use ->filter() instead of ->where() on collections (i.e. findByName method of Permission::class) as is 10-15 times faster.

    public function handle()
    {
        $collection_size = 1000;
        $faker = Faker::create();
        for ($i=0; $i < $collection_size; $i++) {
            $a[] = ['firstname' => $faker->firstNameMale, 'lastname' => $faker->lastName];
        }
        $a[] = ['firstname' => 'John', 'lastname' => 'Doe'];
        $c = collect($a);

        $iterations = 1000;
        $tstartwhere = microtime(true);
        for($i=0; $i < $iterations; $i++){
            $c->where('firstname', 'John')->where('lastname', 'Doe');
        }
        $tendwhere = microtime(true);
        $twhere = $tendwhere - $tstartwhere;
        $tstartfilter = microtime(true);
        for($i=0; $i < $iterations; $i++){
            $c->filter(function($value, $key){
                return $value['firstname'] == 'John' && $value['lastname'] == 'Doe';
            });
        }
        $tendfilter = microtime(true);
        $tfilter = $tendfilter - $tstartfilter;

        dd($twhere, $tfilter);
    }
@AlexVanderbist

This comment has been minimized.

Member

AlexVanderbist commented Nov 3, 2017

Good catch as well, very interesting how these methods differ so much in performance... Thanks a lot!

@drbyte

This comment has been minimized.

Collaborator

drbyte commented Nov 3, 2017

@andrubeldie is this what you envisioned, converting where to filter ?

    public static function findByName(string $name, $guardName = null): PermissionContract
    {
        $guardName = $guardName ?? config('auth.defaults.guard');

-        $permission = static::getPermissions()->where('name', $name)->where('guard_name', $guardName)->first();
+        $permission = static::getPermissions()
+            ->filter(function ($value, $key) use ($name, $guardName) {
+                if (! is_null($guardName)) {
+                    return $value['name'] === $name && $value['guard_name'] === $guardName;
+                }
+
+                return $value['name'] === $name;
+            })->first();

        if (! $permission) {
            throw PermissionDoesNotExist::create($name, $guardName);
        }

        return $permission;
    }
@ctf0

This comment has been minimized.

Contributor

ctf0 commented Jan 29, 2018

@drbyte atm any of the hasPermission methods call the permission relation but as we already have the permission cached , cant we use the cached copy instead ?

@drbyte

This comment has been minimized.

Collaborator

drbyte commented Jan 29, 2018

atm any of the hasPermission methods call the permission relation but as we already have the permission cached , cant we use the cached copy instead ?

This package registers all permissions with Laravel's Gate, using the cache. So, when you call can() on the user, you're leveraging the cache.

@ctf0

This comment has been minimized.

Contributor

ctf0 commented Jan 29, 2018

@drbyte i was talking about hasAnyPermission which is used in a middleware, which is why you probably updated the package middleware to

foreach ($permissions as $permission) {
if (app('auth')->user()->can($permission)) {
return $next($request);
}
}

instead

@thibaultlavoisey

This comment has been minimized.

thibaultlavoisey commented Feb 16, 2018

Hello @AlexVanderbist,

Is this issue fixed in the latest version? Our use case is the same as @andrubeldie and we have an overhead of 200% :/

Thank you!

@andrubeldie

This comment has been minimized.

andrubeldie commented Feb 16, 2018

@thibaultlavoisey seems to be still there ...

$permission = static::getPermissions()->where('name', $name)->where('guard_name', $guardName)->first();

@thibaultlavoisey

This comment has been minimized.

thibaultlavoisey commented Feb 20, 2018

@andrubeldie Arf! :/

@AlexVanderbist @freekmurze Is it possible to reopen this issue? Can we help you with a PR to fix this bug ASAP?

Thank you!

@AlexVanderbist

This comment has been minimized.

Member

AlexVanderbist commented Feb 20, 2018

Hey, sorry for not checking in on this issue earlier. Feel free to PR a fix and add me as a reviewer and we'll take a look 👍

@thibaultlavoisey

This comment has been minimized.

thibaultlavoisey commented Feb 20, 2018

@andrubeldie Do you want to make the PR as you're the first person that has reported this issue?

@andrubeldie

This comment has been minimized.

andrubeldie commented Feb 20, 2018

@thibaultlavoisey: arf! it's all yours, go for it. :)

@drbyte

This comment has been minimized.

Collaborator

drbyte commented Apr 16, 2018

Fixed in v2.11.0 -- Thanks to everyone for their input and collaboration!

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