owen-it / laravel-auditing

Record the change log from models in Laravel
https://laravel-auditing.com
MIT License
3.05k stars 390 forks source link

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

Closed neil-cldagency closed 4 months ago

neil-cldagency commented 5 months ago

PHP Version

8.3

Laravel Version

11.x

Package Version

13.6.7

Description

Hi there, firstly thanks for the great package!

I'm not sure if this is something I'm doing or what, but I've just installed a fresh copy, followed the instructions to the letter, and it was refusing to keep audit log entries when I'd tried saving in the following manners:

I'd stopped short of making a custom controller just to test this, as I had added the following to my model:

/**
 * The "booted" method of the model.
 */
protected static function booted(): void
{
    static::updated(function (Property $property) {
        dump('updated', $property);
    });
}

And that dump was coming through on the above save methods.

I'd also added an event listener for OwenIt\Auditing\Events\Auditing which was dumping out each time I tried the above save methods as well.

It wasn't until I'd set the threshold option back to 0 (from my preference of 100) that it started actually logging anything.

I've tested this with the threshold at various values and it seems that the prune method might have a bug in it as when the value is anything other than 0, the entire log for that model (and only that model instance, it seems) is removed on each audit event.

Steps To Reproduce

Possible Solutions

No response

parallels999 commented 5 months ago

There are tests for that functionality, also some relateds #946, #933 https://github.com/owen-it/laravel-auditing/blob/22682dd5c800aab90ef261530098625e26a66709/tests/Functional/AuditingTest.php#L292-L310

Maybe PHP_INT_MAX presents some overflow in your environment Could you debug the delete query? https://github.com/owen-it/laravel-auditing/blob/22682dd5c800aab90ef261530098625e26a66709/src/Drivers/Database.php#L25-L30

Try 13.6.5, does it work in that version? Could you write a failing test?

jamiewatsonuk commented 5 months ago

I have the same issue when using this package in my development setup.

When running 13.6.5 (as mentioned above), the threshold appears to work as expected. In the latest version, setting the threshold to anything other than 0, removes all audits for the model being audited.

I see that the tests pass when I check out master to my dev environment. Changing the test driver from sqlite to mysql causes this issue to present itself in the test as well.

There was 1 failure:

1) OwenIt\Auditing\Tests\Functional\AuditingTest::itWillRemoveOlderAuditsAboveTheThreshold
Failed asserting that 0 is identical to 10.

Edit to add that my MySQL version was 8.1.0 when I checked this.

More info (when running against MySQL):

If I change ->delete() to ->toSql() I can see that Laravel generates the expected select query with the offset and limit:

select * from `audit_testing` where `audit_testing`.`auditable_type` = ? and `audit_testing`.`auditable_id` = ? and `audit_testing`.`auditable_id` is not null order by `created_at` desc limit 9223372036854775807 offset 10

When I use the query log (DB::enableQueryLog() & DB::getQueryLog()) to get the executed statement I can see that the offset is not included:

delete from `audit_testing` where `audit_testing`.`auditable_type` = ? and `audit_testing`.`auditable_id` = ? and `audit_testing`.`auditable_id` is not null order by `created_at` desc limit 922337203685477580

The SQLite query is generated appropriately:

delete from "audit_testing" where "rowid" in (select "audit_testing"."rowid" from "audit_testing" where "audit_testing"."auditable_type" = ? and "audit_testing"."auditable_id" = ? and "audit_testing"."auditable_id" is not null order by "created_at" desc limit 9223372036854775807 offset 10

Notice the sub select in the delete statement for SQLite. Seems like this is not implemented for some reason in MySQLGrammar compared with SQLiteGrammar in the Framework itself.

parallels999 commented 5 months ago

src/Illuminate/Database/Query/Grammars/MySqlGrammar.php#L117-L145 Apparently offset has problems when MySql minor than 8.0.11 but there is a fix for that, but with 8.1 that should not happen Needs more research, maybe it's a Laravel bug If you have any idea how to solve it, open a PR

erikn69 commented 5 months ago

Hi, try #948

neil-cldagency commented 5 months ago

Apologies @parallels999 — I've been away on business for a few days and didn't have much time to answer this. Looks like there's a promising PR in the pipeline however, so thanks to @erikn69 / @jamiewatsonuk for their contribs 👍

rebbieboi commented 5 months ago

+1 can confirm this is an issue if you have a threshold set after the latest update

parallels999 commented 5 months ago

can confirm this is an issue

That has already been established, waiting for someone to confirm that the fix works

christian-zippin commented 5 months ago

+1 Confirmed that it is a problem if you have a threshold set after v13.6.5. For now, I set my composer.json to v13.6.5 to make it work.