Opened 16 years ago

Closed 13 years ago

Last modified 12 years ago

#6456 closed (fixed)

FileField deletes files even when model instance is deleted in a transaction that is rolled back

Reported by: durdinator Owned by: nobody
Component: File uploads/storage Version: dev
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I've got a model with a few FileFields. I've got code resembling this:

class SomeObject(models.Model):
    ...

class RelatedFile(models.Model):
    filename = models.FileField(...)
    owner = models.ForeignKey(SomeObject, related_name='related_files')

@commit_on_success
def update_files(request, id):
    obj = get_object_or_404(SomeObject, pk=id)
    obj.related_files.all().delete()
    ... more code - which depending on external conditions can raise an exception ...

Since the FileField hooks the RelatedFile's post_delete signal, it deletes the original files when the delete() method is called. Then if the transaction is rolled back, the rows in the database, not being deleted after all, point to nonexistent files.

Change History (9)

comment:1 by Philippe Raoult, 16 years ago

Triage Stage: UnreviewedAccepted

right now there's no signal for commit/rollback, and I guess we'd need that to make sure the files aren't deleted yet. See #5415 for the implementation of something similar.

comment:2 by Marty Alchin, 15 years ago

Component: Core frameworkFile uploads/storage

comment:3 by Flo Ledermann, 13 years ago

Needs documentation: set

This is a severe dataloss bug that has bitten me badly recently. Suggesting raising the priority of this, although it's too late for 1.3 now. I'll try to at least provide a test case and a doc patch as soon as i find some time.

comment:4 by Russell Keith-Magee, 13 years ago

milestone: 1.3

It's not too late at all. 1.3 final won't be released until mid January at the earliest.

I think this adds weight to Malcolm's argument at DjangoCon that FileField shouldn't *ever* delete a file, and the cleanup task should be left to an external process.

comment:5 by Jannis Leidel, 13 years ago

Couldn't agree more with Malcolm's argument.

comment:6 by Flo Ledermann, 13 years ago

For what it's worth, as someone who has been bitten by this bug, I absolutely agree that FileField should never delete files in the filesystem, especially if we cannot guarantee transactional integrity (which we probably cannot given the mechanisms we currently have).

comment:7 by Carl Meyer, 13 years ago

Resolution: fixed
Status: newclosed

(In [15321]) Fixed #6456 - Excised FileField file deletion to avoid data loss. Thanks to durdinator for the report.

comment:8 by Carl Meyer, 13 years ago

(In [15322]) [1.2.X] Fixed #6456 - Excised FileField file deletion to avoid data loss. Thanks to durdinator for the report.

Backport of r15321 from trunk. Backported despite slight backwards-incompatibility due to potential for data loss.

comment:9 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

Note: See TracTickets for help on using tickets.
Back to Top