Opened 4 years ago

Last modified 13 days ago

#18283 assigned Bug

FileField should not reuse FieldFiles

Reported by: sayane Owned by: trg
Component: Database layer (models, ORM) Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


A simple example:

old_image = mymodel.image
    mymodel.image = do_something_with_image(old_image)
    old_image.close() # transaction manager is in auto commit mode

Expected behavior:

New file is commited to disk and database, old file is removed.

Actual behavior:

New file is commited to disk and database, then this new file is removed from disk.

That's because FileField (or to be more precise: FileDescriptor class) is re-using FieldFile objects instead of creating new FieldFile for every new instance of a file. This leads to unexpected results and data loss (like in example).

Change History (4)

comment:1 Changed 4 years ago by jezdez

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

comment:2 Changed 3 weeks ago by szopu

  • Owner changed from nobody to szopu
  • Status changed from new to assigned

comment:3 Changed 13 days ago by trg

  • Owner changed from szopu to trg

comment:4 Changed 13 days ago by trg

I have a real trouble with reproducing this bug. The description of it is very enigmatic - the submitter didn't mention what do_something_with_image() actually does (for instance, what type of object it returns).
This bug seems to happen only when one takes the FieldFile object from specified model field and modifies it in place. This is illustrated by test_unchanged_fieldfile in the diff below:

The only way I see this problem can be fixed, is defensive shallow copying of the FieldFile object in __get__ and __set__ (I think that doing it for __set__ only is not enough). But this solution has a downside, for instance, this would generate following semantics: = 'somefile.txt'
print(  # returns the previous filename instead of 'somefile.txt'

So my question is: Is it the right way to go?

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