Fix database prune, offset not working on some dbs

[erikn69]

• Closes #947

[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)

[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

[giolaza]

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

[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 https://github.com/owen-it/laravel-auditing/issues/946 https://github.com/owen-it/laravel-auditing/blob/27e1b3121a6b34f9635d2dbe84c13ca11ed2bee0/src/Drivers/Database.php#L32

[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 😅

[neil-cldagency]

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