Too many calls to policy ???

Hello guys,

I would like to ask for a bit of an advice from someone more experienced with app optimization. One of the projects I am starting to work with uses policies to protect the models. I am checking it view by view, model by model and I am fixing some issues like N+1 queries that it struggles with. One thing I am noticing though that baffles my mind is the huge amount of calls to policy checks I can see in the debug bar. For example, when I log in as admin, I can get to the list of users. For every user, the blade view uses @can('delete') directive to check if the user viewing the page is an admin and if he has a right to delete. In the debug bar I can see 26 calls to the gates and it takes 3.5 seconds to load the page ( I have already removed the unnecessary loading of other related models as they are not needed for the page at all). I do not thing checking the gates/policies 26 times is too much, but I have this on a page with only a few users. What would happen if I called such check on a page with 100 or more models? I find this call to @can within a @foreach loop to be a bit inefficient.

I am working of a premise here that if the auth()->user()->isAdmin() returns true, that it is true for all the loaded models (users in this case). User that is not an admin can not view this page. And if I take some user related models like projects for example, a regular user would only be able to view his own projects, so a check if the user can delete a project would be redundant.

So here is the question. Can I call the gate once at the top of the page and store the result in a a variable that I can then check within the loop?

fylzero

4 years ago

Level 67

@martin.sinansky@gmail.com It sounds like your isAdmin() check may be making a db call every-time the model is referenced. Anytime you see the same data call being made and you cant address it as a simple N+1 fix, it could be useful to reach for Cache::remember() to store the data retrieval in memory so the app is only making the call once. For things like permissions, this can be made fairly short-term as you'll want permissions to properly reflect when updated. Alternately you can store them in cache forever and just replace or bust them once they are updated.

https://laravel.com/docs/8.x/cache#retrieve-store

NicolasMica

4 years ago

Level 42

Hi 👋

Could we see the isAdmin method definition (or the policy used in the can directive) ? As @fylzero pointed out, it seems that the method does a database query instead of relying on the loaded data. It can probably be solved with eager loading.

martin.sinansky

4 years ago

Level 3

@NicolasMica: Eager loading (plus limiting the extracted columns) helped with the queries to DB and decreasing the memory usage. However, on any load of a list.blade.php, when I check the debugbar I see 8 different viewAny checks. Today I figured out why... because the sidemenu is built with 8 @can('viewAny', App\XXXXXXXX::class) directlives. :D

My issue with the policies was not with the DB queries, but calling the same gate every single time I am throwing a model into the view from a collection. For a regular user, who can view projects but he can only edit/update his own, it makes sense... but for admin, checking within a loop if he is able to edit anything seems like a redundant call. That is the main reason why I was asking if it is possible to store the value of the auth->user()-> isAdmin() check and somehow combine it with the @can('update', $xxxx)

something like

@if($isAdmin  or @can(update,$xxxx))
		//do whatever magic is needed
@endif

because ion the pages where pagination was not used, it slowed the page loading quite significantly.

NicolasMica

4 years ago

Level 42

@martin.sinansky Calling the can multiple times should not be a problem in itself. In your particular case, you could create a before method in you policy, which will be executed before any policy, enabling you to skip the resource intensive policy check.

public function before(User $user, $ability)
{
		if ($user->isAdmin()) {
				return true;
		}
}

But to get back to your original question, you could absolutely use a variable to store the ability once. For example you may call the policy in the controller once (if it is not related to a specific model) and store the result in a variable that you would use in your foreach loop.

$canDelete = $request->user()->can('delete');

martin.sinansky

4 years ago

Level 3

@NicolasMica The project actually has a before on each of the policies, so the admin check is the only one that happens. So your suggestion is actually going to help me limit the unnecessary traffic in case of an admin login. As for admin the rule is he can view, update, or delete anything. So one check at the top and I am set to go.

Thanks a lot!

Please or to participate in this conversation.