un1t/django-cleanup

transaction rollback handling

Closed this issue · 3 comments

I'm not sure if it's possible to fix this, but I found an issue with a transaction rollback. Basically, when a a model with an image is created and saved inside a transaction, and the transaction then gets rollbacked, the data is not in the db table, but the image is still stored on disk.

I found this issue while writing tests. When I use django.test.TestCase, and work with models, at the end of the test django rollbacks the changes, but the images created during instance.save() calls are still on the disk, which prevents me from running multiple tests in parallel, because I would have to manually delete the files at the end of each test, which would be concurrent. Also, It's not good in an app if you're using db transactions.

Demo code:

try:
    with transaction.atomic():
        self.create_model_with_image()
        raise ValidationError('INVALID')
except ValidationError as e:
    print(e)

This is not a scenario that is supported (yet?), this library deletes files after the file referenced on the model is changed or when the model is deleted.

(also:
Testing this library can only be done with:
https://docs.djangoproject.com/en/2.0/topics/testing/tools/#django.test.TransactionTestCase
)

It appears we would not be able to implement this:
https://docs.djangoproject.com/en/1.9/topics/db/transactions/#why-no-rollback-hook

But there is a solution mentioned there:

The solution is simple: instead of doing something during the atomic block (transaction) and then undoing it if the transaction fails, use on_commit() to delay doing it in the first place until after the transaction succeeds. It’s a lot easier to undo something you never did in the first place!

Perhaps try saving to a temp file in a temp directory (which gets old files deleted every so often), and use an on_commit to move the file to the correct spot.

Can also try using random file names in your tests to prevent contention during parallel runs.

I'm closing this issue as invalid.

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.