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

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

Closed
neil-cldagency opened this issue Jun 10, 2024 · 8 comments · Fixed by #948
Closed

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

neil-cldagency opened this issue Jun 10, 2024 · 8 comments · Fixed by #948
Labels
bug Issue, error or unexpected behavior

Comments

@neil-cldagency
Copy link

neil-cldagency commented Jun 10, 2024

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:

  • Via "save" buttons in Filament PHP
  • Via tinker ->save() method on a model instance (with console set to true in the config)

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

  • Install auditing tool
  • Configure as normal
  • Create at least one auditable model
  • Set threshold to anything other than 0
  • Trigger an update/create etc event

Possible Solutions

No response

@parallels999
Copy link

parallels999 commented Jun 10, 2024

There are tests for that functionality, also some relateds #946, #933

public function itWillRemoveOlderAuditsAboveTheThreshold()
{
$this->app['config']->set('audit.threshold', 10);
$this->app['config']->set('audit.events', [
'updated',
]);
$article = factory(Article::class)->create([
'reviewed' => 1,
]);
foreach (range(0, 99) as $count) {
$article->update([
'reviewed' => ($count % 2),
]);
}
$this->assertSame(10, $article->audits()->count());
}

Maybe PHP_INT_MAX presents some overflow in your environment
Could you debug the delete query?

if (($threshold = $model->getAuditThreshold()) > 0) {
return $model->audits()
->latest()
->offset($threshold)->limit(PHP_INT_MAX)
->delete() > 0;
}

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

@jamiewatsonuk
Copy link

jamiewatsonuk commented Jun 12, 2024

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
Copy link

parallels999 commented Jun 12, 2024

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
Copy link
Contributor

erikn69 commented Jun 12, 2024

Hi, try #948

@neil-cldagency
Copy link
Author

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
Copy link

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

@parallels999
Copy link

can confirm this is an issue

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

@parallels999 parallels999 added the bug Issue, error or unexpected behavior label Jun 17, 2024
@christian-zippin
Copy link

+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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue, error or unexpected behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants