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

Improve performance - don't pull down all abilities #263

Closed
garygreen opened this issue Mar 8, 2018 · 7 comments
Closed

Improve performance - don't pull down all abilities #263

garygreen opened this issue Mar 8, 2018 · 7 comments
Labels

Comments

@garygreen
Copy link

As discussed in Ref: #261

Currently when checking for permissions it has to load ALL abilities for a user in order to determine if they have access - if you have hundreds of abilities for users this doesn't seem like an efficient way to check.

In our system, you can authorize people to be able to see your private pictures:

Bouncer::allow($user)->to('view-private-pictures', $user2)

In theory, a person could authorize hundreds of people - which means they will receive hundreds of abilities. Whenever an ability is checked, it will load in all of those other abilities just to check, even if they are obviously not relevant.

I was kind of hoping it would just do a DB lookup to get the ability by name, or any abilities which are relevant.

@garygreen
Copy link
Author

garygreen commented Mar 8, 2018

@JosephSilber as per your comments:

For people that don't use single model abilities that much, pulling down all abilities at once is actually better.

I think it's more of a general issue of number of abilties you have in your system rather than if you use model abilties or not. You could have very granular level of abilities (hundreds) and not have any model abilties. Pulling down hundreds of modeling and instantiating them every request I can't see how that is "better"

I do plan on creating a separate Clipboard that only pulls down the relevant abilities for the given query, and swap that out when calling Bouncer::dontCache().

So your suggesting that you need to disable the bouncers cache completely in order to improve performance of checking some abilities? I'm not sure I see them as mutually exclusive - for example, you could cache certain abilities used on most requests, or attempt to pull down only relevant abilities when checking e.g. the "all abilities" one and ones relevant for a model checking, if applicable

Some kind of FetchRelevantAbilities class would be great.

@garygreen garygreen changed the title Improve performance - don't pull down all abilties Improve performance - don't pull down all abilities Mar 8, 2018
@JosephSilber
Copy link
Owner

You could have very granular level of abilities (hundreds) and not have any model abilties [abilities].

I fail to see how you could reasonably have hundreds of abilities for a single user without using single model abilities (unless your app has dozens of models).

I prefer to optimize the default configuration for simpler setups. People with more advanced setups are more used to customizing their tools, and would know to swap out the clipboard (I'll obviously add it to the docs).

So your [you're] suggesting that you need to disable the bouncers [bouncer's] cache completely in order to improve performance of checking some abilities?

That was just a quick answer. I'll definitely think this through properly when I get to implementing it. If I go through with this, I'll probably have 2 additional clipboards: one that I've outlined, the other that caches the fetched abilities. Will play around and see what makes sense.

@garygreen
Copy link
Author

You could have very granular level of abilities (hundreds) and not have any model abilties [abilities].

I fail to see how you could reasonably have hundreds of abilities for a single user without using single model abilities (unless your app has dozens of models).

Sorry I worded that poorly. What I meant by "model abilities" is the granting permission to specific models / ids, I refer to them as "model abilities" I'm not sure what the correct word is. I wasn't suggesting to get rid of the per-model abilities, it's absolutely fine as it is.

That was just a quick answer. I'll definitely think this through properly when I get to implementing it.

Excellent, it would definitely help improve performance in larger applications.

@JosephSilber
Copy link
Owner

JosephSilber commented Mar 29, 2018

Just pushed a huge update that completely rewrites the non-caching clipboard from the ground up, to only fetch the relevant abilities.

Doing this required totally refactoring all clipboards, and also required a major refactor of all tests in Bouncer's codebase.

Before I merge this into master, I'd like to have a little more confidence that it actually works. Once we get this merged into master, the next step would be to build a caching version of this clipboard. This has been merged into master.

It would be extremely helpful if you could test this out, by updating your composer.json file and setting bouncer's version to dev-optimize-clipboard dev-master 🙏

@mailnike
Copy link

Hi, i would like to know how this will behave if I have more than 200 permissions, more than 100+ roles i.e. 100*200 = role permissions assignment (SaaS product). I am facing some memory issues with Spatie, but it seems like this package can also cause same issue, could you please share your experience or an opinion.

@JosephSilber
Copy link
Owner

JosephSilber commented Jul 24, 2018

@mailnike the actual roles aren't pulled from the database when checking authorization. Bouncer queries the database using the assigned roles, but doesn't actually fetch the roles out of the database.

The way Bouncer currently works, checking for authorization does pull all of the user's abilities from the database at once. This is preferable if a single user doesn't have too many abilities, since this means we only have to pull data from the DB once, and we can then run as many different checks as we need without going back to the DB again.


However, if a single user in your app may have hundreds of abilities, it may be more efficient to only pull the relevant information from the DB for any given authorization check. You can accomplish this in the master branch by turning off Bouncer's cache:

Bouncer::dontCache();

If you think your app is better off not pulling all abilities at once, stick this ☝️ piece of code in your service provider and you'll be good to go.

You can read more about it in #276

@mailnike
Copy link

@JosephSilber great reply. Thank you +1

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

No branches or pull requests

3 participants