santigarcor / laratrust

Handle roles and permissions in your Laravel application
MIT License
2.18k stars 376 forks source link

isAbleTo function is very slow #645

Open joh3rd opened 1 year ago

joh3rd commented 1 year ago

Describe the bug The isAbleTofunction is very slow. I have many users with many teams and many permissions. In one case, a user has more than 8000 rows in the permission_user table. This is because of the user is assigned to many teams with a lot of permissions in every team. I'm checking permissions like that:

$user->isAbleTo(['permission_one', 'permission_two'], $teamId, true);

The DB query itself is super fast. I logged the raw query and tried that. But the isAbleTo function is very slow (more than two seconds in this case).

websitevirtuoso commented 1 year ago

Please provide repo with 8000 records and we are able to help to debug and improve code

GregPeden commented 10 months ago

I happened across this issue while looking for anotherr... I fixed this by re-writing the "isAbleTo" method on User model:

  public function isAbleToOptimized($permissions, $team = null, $excludeGlobalRoles = false, $withDirectPermissions = false)
  {
    $teamId = Helper::getIdFor($team, 'team');
    return $this->roles()->hasByNonDependentSubquery('permissions', function (Builder $q) use ($permissions, $excludeGlobalRoles) {
      if (is_array($permissions)) {
        $q->whereIn('name', $permissions);
      } else {
        $q->where('name', $permissions);
      }
    })->where(function ($q) use ($teamId, $excludeGlobalRoles) {
      $q->where('role_user.team_id', $teamId)
        ->when(!$excludeGlobalRoles && $teamId !== null, function (Builder $q) {
          $q->orWhere('role_user.team_id', null);
        });
    })->exists();
  }

I wrote this to make use of this package: https://github.com/mpyw/eloquent-has-by-non-dependent-subquery

But I think you can do it with built-in Laravel methods now since MySQL has been updated to optimize queries that Laravel generates much better.

This makes better use of the SQL index. I can't remember if I made changes to the indexing on the laratrust tables as well... but the execution time of this is completely fixed and scales with many records.

WillGoldstein commented 1 month ago

Is @GregPeden 's fix going to be merged? Or has it been?

Edit: I see that's not feasible now since the package his fix depends on is archived and isn't compatible with Laravel 11. This issue seems significant to me. @santigarcor mind sharing your thoughts?

WillGoldstein commented 1 month ago

@joh3rd question for you: why aren't you using roles instead of permissions in this case? I am testing by creating many users, assigning each user a role, and found the $user->isAbleTo(['permission_one', 'permission_two'], $teamId, true); function to be a negligible 10ms tax. I have a similar scenario where a user could be assigned to many teams, each with lots of permissions, but if you use roles, you don't need to populate the permission_user table. And since permissions can be derived from the role, you can still do the granular check of a permission via isAbleTo.

GregPeden commented 1 month ago

Edit: I see that's not feasible now since the package his fix depends on is archived and isn't compatible with Laravel 11. This issue seems significant to me. @santigarcor mind sharing your thoughts?

I think using that solution has been deemed obsolete for anyone running MySQL 8.0.16 or later. It's fine to use Laravel's built-in "whereHas" method.

joh3rd commented 1 month ago

@joh3rd question for you: why aren't you using roles instead of permissions in this case? I am testing by creating many users, assigning each user a role, and found the $user->isAbleTo(['permission_one', 'permission_two'], $teamId, true); function to be a negligible 10ms tax. I have a similar scenario where a user could be assigned to many teams, each with lots of permissions, but if you use roles, you don't need to populate the permission_user table. And since permissions can be derived from the role, you can still do the granular check of a permission via isAbleTo.

@WillGoldstein Yes i agree that this would make sense but in my case not possible because i created a user admin interface where admins can manage roles and permissions for their users. They have the possibility to make roles dynamically but they also have the possibility to give certain permissions to users without creating a role. And some admins still just assign permissions without having a role in between. Anyway, i'll check again with Laravel 11 and MySQL 8 as soon as i upgraded to this version :)