Mass Bans?

[irazasyed]

What's the most efficient way to handle mass bans?

Right now looping through all bannable models and then applying the ban is making 3 queries each (So if I was to ban say 1k users, that's 3k queries and isn't good TBH). So if I was to do mass ban, then it's not really efficient.

So I wanna know if there's a better way to achieve this?

[irazasyed]

Please take a look at these tests I did on a project where I have this use-case.

My current implementation which is very simple and works based on status column with an int value and makes one query (The other query is auth, so we can exclude that from calculation for this).

Current implementation: 41ms / 1 Query:

VS

Laravel-Ban: 407ms / 297 Queries.

[antonkomarev]

Interesting use case @irazasyed! I haven't thought about it before. It will require refactoring to decrease queries count because right now logic as simple as possible.

Here is how it works:

1. We are creating Ban model (1st query).
2. In created method of BanObserver we are getting bannable model (2nd query).
3. In created method of BanObserver we are updating banned_at timestamp flag (3rd query).
4. In created method of BanObserver we are firing ModelWasBanned event.

[antonkomarev]

There is a way to remove 2nd query if we'll remove updating of the Bannable model from BanObserver and place it in BanService.

But then we'll lose ability to auto-ban model on ban model creation:

$ban = $user->bans()->create([
    'comment' => 'Enjoy your ban',
    'expired_at' => '+1 month',
]);

Then we will be able to introduce banMany method and perform mass update of all Bannable models in one query.

By this way for 100 bans we'll have 100 queries with bans creations and 1 more with updating timed flags. We can't create all bans in one query because we wouldn't be able to fire ModelWasBanned events in this case.

[antonkomarev]

There is one more possible solution. We'll lose ability to auto-ban model on ban model creation too.

Remove created event from BanObserver and add firing of ModelWasBanned event to BanService ban method. Then add listener which will perform updating of the Bannable model on background (via queue). Then banMany method will accept iterable collection of Bannable models and call ban method for each of them.

By this way for 100 bans we'll have 100 queries with bans creations, 100 queued jobs and 100 more queries in those queues with updating Bannables timed flags.

[irazasyed]

By this way for 100 bans we'll have 100 queries with bans creations, 100 queued jobs and 100 more queries in those queues with updating Bannables timed flags.

That sounds like a lot of processing and work using Queues and all.

Can't we just use createMany directly somehow using the Ban model and bulk update the Bannables timed flag in one query and then fire the event with all this info.

Anyone listening to that event can simply check if it's a collection or a single Bannable model, etc. (whatever is being passed -- I haven't checked in-depth).

I'll explore and play around with this and see if I can figure something out.

[antonkomarev]

I'm not sure it's good to fire event with all banned models as collection, because it will be harder to make users to listen only their own models events.

[irazasyed]

So I just setup a job to ban many users and while it does the job fine but I'm now facing another issue due to the observer as I'm banning the users over an API that's stateless and because of the way the observer is implemented, it now won't fill the created_by_id and created_by_type and there is no way to manually fill that either (Tried passing these two attributes to the ban method and also forced too). It's always null.

I think you should make these two values fillable and let me pass the $bannedBy modal manually (In my case, I can get the user who took this action over API using their token and API guard -- FYI, I'm using Laravel Passport).

I think we need to look into this to also optimize the number of queries somehow. Perhaps, Your first solution is good enough unless you have alternate ideas you thought of over the past month or so!

[antonkomarev]

@irazasyed I agree that Observers usage should be revised. This feature is sweet, but I've started to face many issues with applications which using them a lot. Their behaviour is not obvious. It's not that easy to omit event in Observer (especially if you need to skip only part of it).

[irazasyed]

@antonkomarev So until that is revised and or we come up with some solution, Can you at least add those two columns to the fillables array? That would be great for the time being and would be very helpful.

[antonkomarev]

@irazasyed made created_by_type & created_by_id filalble in PR #35. It is available in v3.5.0.

[irazasyed]

Thank you so much @antonkomarev. Appreciated :)