Fix database prune, offset not working on some dbs

[erikn69]

• Closes Audited logs deleted on fresh install with threshold set to > 0 #947

[willpower232]

I was going to wait for @neil-cldagency to confirm his problem was resolved before reviewing but yeah, this is a great fix 👌

Is it worth changing the tests to use mysql rather than sqlite? Or running both somehow?

[neil-cldagency]

Hi all — apologies, been deep in development. Will look at this tomorrow AM (BST).

[parallels999]

Is it worth changing the tests to use mysql rather than sqlite? Or running both somehow?

It would be a good idea, I have seen other packages test various DRIVERs
spatie/laravel-permission/.github/workflows/test-cache-drivers.yml

[neil-cldagency]

Getting a consistently occurring error when I attempt to edit any models while the threshold is > 0. Looks like you're using a query not all engines support. Fair enough if it's MySQL only but I presume the package should support at least 5.7+?

Will defer to @parallels999 on that one!

SQLSTATE[42000]: Syntax error or access violation: 1235 This version of MySQL doesn't yet support 'LIMIT & IN/ALL/ANY/SOME subquery' 

delete from
  `audits`
where
  `audits`.`auditable_type` = App \ Models \ Property
  and `audits`.`auditable_id` = 1
  and `audits`.`auditable_id` is not null
  and `id` not in (
    select
      `id`
    from
      `audits`
    where
      `audits`.`auditable_type` = App \ Models \ Property
      and `audits`.`auditable_id` = 1
      and `audits`.`auditable_id` is not null
    order by
      `created_at` desc
    limit
      2
  )

MySQL version is 8.0.33-0ubuntu0.20.04.2 (supplied via ddev)

[willpower232]

I didn't realise I already had a suitable test environment for this problem and I'm now sad that current MySQL versions don't support the elegance of this fix.

I think the query could be re-written as a join but writing one manually in Laravel makes my head hurt 😅

The other alternative would be getting the IDs first which seems to work in a pinch.

if (($threshold = $model->getAuditThreshold()) > 0) {
    $class = get_class($model->audits()->getModel());
    $keyName = (new $class)->getKeyName();

    $keys = $model->audits()
        ->select($keyName)
        ->limit($threshold)
        ->latest()
        ->pluck($keyName);

    return $model->audits()
        ->whereNotIn(
            $keyName,
            $keys
        )
        ->delete() > 0;
}

I don't prune audits so don't have a bias any particular way.

[erikn69]

Ok, i will reverse it for now, I think it would be the best since I cannot test old versions of the db at the moment
But the problem in case of a high threshold would return, read #946

i think the query could be re-written as a join

I'll give it a try

[neil-cldagency]

The other alternative would be getting the IDs first which seems to work in a pinch.

From memory, Laravel itself makes use of "get IDs first then in" SQL patterns, so unless there's a realistic chance of exceeding query length limits I wouldn't have anything against using that pattern in this case personally — depends on that risk though, I'd have thought.

[erikn69]

@willpower232, @neil-cldagency hi, could you test "join" version?

[willpower232]

Looking good to me, it behaves as I think it should in my weird setup. With a threshold of 1 the SQL looks like this and looks like it includes all the correct wheres

delete `audits`
from `audits`
left join (
	select `id`
	from `audits`
	where `audits`.`auditable_type` = "App\\Models\\User"
	and `audits`.`auditable_id` = 3
	and `audits`.`auditable_id` is not null
	order by `created_at` desc limit 1
) as `audit_threshold` on `audits`.`id` = `audit_threshold`.`id`
where `audits`.`auditable_type` = "App\\Models\\User"
and `audits`.`auditable_id` = 3
and `audits`.`auditable_id` is not null
and `audit_threshold`.`id` is null

Fixed

[giolaza]

this fix works, but not sure using left join is good idea for optimization

[giolaza]

these fixes work

[willpower232]

@neil-cldagency do you have time to test the fix or should we ship based on what else we've seen?

[willpower232]

@giolaza as you can read above, the left join is the best option, the most elegant solution isn't supported by mysql, and plucking out the IDs would eventually cause a problem for someone

[giolaza]

@willpower232 plucking IDs caused issues when you used it in a subquery with a limit. Maybe a better solution is using Laravel's ->pluck('id').

Before the update, you were using pluck to get the IDs to delete. If we analyze the situation, I think this is the best solution.

Situations:

1. From model creation/package installation, the limit was more than 0. This means that the number of rows to delete should be small.
2. If it was unlimited and then became limited, the first log removal will cause a load, then it goes back to the first argument.

[parallels999]

maybe a better solution is using Laravel's ->pluck('id')

@giolaza originally used ->pluck('id')(v13.6.5), but apparently that caused #946

laravel-auditing/src/Drivers/Database.php

Line 32 in 27e1b31

->pluck('id');

[neil-cldagency]

@neil-cldagency do you have time to test the fix or should we ship based on what else we've seen?

Happy to let others test here @willpower232 as I'm knee deep in an implementation at the moment and testing involves reverting a FilamentPHP plugin each time 😅

[willpower232]

These changes fixed the problem of removing all audits so happy with this

[neil-cldagency]

Apologies all, I hadn't checked who regularly maintains this project. Thanks to everyone involved, though!