collectiveidea / audited

Audited (formerly acts_as_audited) is an ORM extension that logs all changes to your Rails models.
MIT License
3.38k stars 662 forks source link

Avoid orphans on destroy to fix issues with MySQL #247

Open ledermann opened 8 years ago

ledermann commented 8 years ago

Currently I stumbled upon a serious issue using audited with MySQL.

MySQL doesn't manage persistent AUTOINCREMENT values for InnoDB tables: After a server restart, MySQL calculates the next value by doing a SELECT MAX(id)+1 FROM table. Because of this, a value can be reused if you delete the last inserted record. Since 10 years ago there is a bugreport for this.

This is a problem with audited, because in its current implementation it creates orphans if a record gets destroyed (audit record associated with a non-existing auditable / associated).

A possible solution could be:

  1. Use dependent: :nullify for both has_many associations
  2. Set :auditable to NULL for audits with action == 'destroy'

What do you think? Are you open for a PR to fix this issue?

ledermann commented 8 years ago

Consider the following scenario to understand why the current implementation of the gem is fatal:

  1. Assuming we have the model "Document" enabled for auditing. The database is MySQL, the ID field is defined as int(11) NOT NULL AUTO_INCREMENT (pretty standard)
  2. Lots of documents are created, some of them were deleted. After adding Document with id 99, a document with id 100 was created and deleted afterwards. There are two Audit records for this document, both with auditable_id == 100. Both audits violate referential integrity, because there is no document with ID 100 anymore.
  3. Now the MySQL server gets restartet (e.g. because of an software update). Because the AUTOINC status is not persistent, the MySQL server will recalculate the value for the documents table by doing a SELECT MAX(id)+1 FROM documents. It will be 99+1 == 100.
  4. A new document is created. It will get the id 100, which means it reuses the id of a previously destroyed record. A new Audit record is created with auditable_id == 100

Now, Document.find(100).audits.count == 3, which is a serious bug, because the newly created document is linked with unrelated audits.