Code

Opened 6 years ago

Closed 3 years ago

Last modified 3 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: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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.

Attachments (0)

Change History (9)

comment:1 Changed 6 years ago by PhiR

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 5 years ago by Gulopine

  • Component changed from Core framework to File uploads/storage

comment:3 Changed 4 years ago by floledermann

  • 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 Changed 4 years ago by russellm

  • milestone set to 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 Changed 4 years ago by jezdez

Couldn't agree more with Malcolm's argument.

comment:6 Changed 4 years ago by floledermann

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 Changed 3 years ago by carljm

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:8 Changed 3 years ago by carljm

(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 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.