Teams performance is terrible on GAE
ajGulati05 opened this issue · 27 comments
Describe the bug
I have a team model and a user model. About 4 roles per team and 10 permissions. So if 1000 teams thats about 4,000 records. I am using database as the cache driver. Using the hasRoles trait on both teams and users.
Trying to get the Roles a team has and what permissions map to those roles in a team.
$team->roles()->with('permissions')->get()
$team->roles()->get()
Even
$team->getRoleNames() is taking over a minute any suggestion?
Versions
You can use composer show to get the version numbers of:
- spatie/laravel-permission package version: 5.5
- illuminate/framework package 8.75 laravel version
PHP version: 7.4
Database version: 5.7
To Reproduce
Steps to reproduce the behavior:
Here is my example code and/or tests showing the problem in my app:
Expected behavior
A bit more performant
Additional context
Add any other context about the problem here.
Environment (please complete the following information, because it helps us investigate better):
- OS: macOS
- Version 12.2.1
- Laravel Sail for Laravel 8
I am using database as the cache driver
Use a real cache driver
$team->roles()->with('permissions')->get()
$team->roles()->get()
$team->getRoleNames()
Did you add hasRoles trait on Team Model? that is not right
Team model are on your implementation, not on this package
Look for index on db team_id fields, if it not exists create it
Also you can use global roles, for avoid duplicate values on each team
Feel free to make a PR for improve that or use an alternative
Lines 95 to 100 in d1734f8
Also
Lines 15 to 21 in d1734f8
Using the hasRoles trait on teams
I looked at global roles - the use case doesn’t work. Since each team can have roles but a different permission set.
IAnything you think from the above snippets I posted that I could do to improve ?
Using the hasRoles trait on teams
Ah I’ll check this out than you
Anything you think from the above snippets I posted that I could do to improve ?
Use Redis or Memcached as cache driver, or at least file
spatie/laravel-permission package version: 5.5
Use the 5.5.5 version
PHP version: 7.4
Upgrade to php 8.1 and laravel 9.x
Upgrade to php 8.1 and laravel 9.x
Using GAE upgrading to laravel 9 and php81 unable to deploy because of dependency issues . That another issue.
Ill see if its possible to use file on GAE.
Using GAE
Is Google App Engine's database operating over a slow network connection?
@drbyte maybe #1935 trait is neccessary, it was closed by @spatie-bot without any review
@parallels999 yes, @ajGulati05 could explore adding such a trait manually to their own application, to see whether it helps.
I suspect it's a latency issue with the database; chances are that a cache like redis/memcached will be most helpful. File-based caching may still have a network latency issue in that environment.
Working on using redis and the app contained in the same docker instance deployed on GAE. The bigger issue is when you update remove/add permission to a roll. Using revokePermissionTo and givePermissionTo.
Will update once I have an answer here.
@drbyte I don't think its a latency issue with the db - but ill have a quick look at that as well
The bigger issue is when you update remove/add permission to a role. Using revokePermissionTo and givePermissionTo.
I'm not surprised. Those methods purge the cache, causing it to need to be rebuilt on next query.
@drbyte I see just looking at the doc the only functions that do not purge cache are
$user->assignRole('writer');
$user->removeRole('writer');
$user->syncRoles(params);
I would like to be able to change the permission on a role based on how a team sees fit. I might have to work on creating the cache return the result without rebuilding the cache first but that does not seem like a long term solution.
What about instead using roles for each team(4 roles each team), just use permissions directly for each user/team without roles, cache is going to be really small if there are only 10 permissions
It is generally best to code your app around permissions only ... and your app always checks for permissions, not roles.
laravel-permission/docs/best-practices/roles-vs-permissions.md
Lines 6 to 10 in a6e4122
For changing that, you could make a script, searching all the teams with roles, now loop on each role and assing the role permissions on each user, after that remove the role
@parallels999 won't work since teams can define what permission a role has.
Is there any chance you can set up a very basic application to demonstrate this? Including a data-seeder that sets it up so it's easy to replicate the problem.
Doesn't need a UI really; perhaps just a couple tests that demonstrate the problem via the command-line.
won't work since teams can define what permission a role has.
@ajGulati05 it can work because there is a team pivot on model_has_permission, look at this
Source: https://github.com/spatie/laravel-permission/pull/1804
@parallels999 won't work since teams can define what permission a role has.
I'm saying forget about roles use permissions only, and set permision for user on each team
It is generally best to code your app around permissions only
We have to use roles due to whats coming down the pipeline. The users can change the name of roles based in how they manage their teams and additionally add more roles.
In he code I don't see a way to pass in a custom attribute to the cache like passing in the team_id. Thinking what if the cache key is based on team_id rather than permission. Thoughts?
Thinking what if the cache key is based on team_id rather than permission. Thoughts?
It is the same idea of #2204, but for teams
@erikn69 thanks for this here is my thought process in tackling this
- Overwrite the PermissionRegistrar.php file using composer.json autoload ( or fork the repo)
- In the new PermissionRegistrar.php remove the call for initializingCache in the constructor
- initializeCache under setPermissionTeamId if cache key does not exist.
- Create a getter for cache key which would be something like - config('permission.cache.key').":".getPermissionsTeamId();
- Replace self::cacheKey for with the getter
Then use #2204 to only build permissions role for a team and not for everyone.
Thoughts?
@erikn69 Wondering if you had a chance to see the above proposed solution?
I think something like that needs a lot of work and could be prone to problems, but if you're going to do it in a fork you can change what you need
Just looking to confirm that I'm not misusing this feature, along the same line of questioning as some of the posts above.
I have three roles
- Default
- Team manager
- Super admin
Default and super admin are global and not specific to any team.
Users can be assigned as a team manager on specific teams.
All team managers have the same permissions regardless of which team they are on.
I believe what I have to do is create the same "Team manager" role for every single team in the database, and for each new role, I have to attach all the permissions to that role.
This seems suboptimal for my use case where I don't need fine control over the permissions for each team manager on their team.
Am I using this feature correctly?
Am I using this feature correctly?
Not
Am I using this feature correctly?
Not
Alright, thank you. Looks like I need to create them all as global and then leverage the current team select to switch to a user and manage users team roles from there.
It would be nice if there was an interface that wrapped this in a more elegant way rather than setting a global current team, in the very least push and pop current team. If pull requests are welcomed happy to potentially contribute.
If pull requests are welcomed happy to potentially contribute.
Yes, they are welcome, feel free to make it better and don't forget adding some tests, also avoid breaking changes
Maybe this #2075??