Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#27334 closed Cleanup/optimization (fixed)

File uploads could rename temporary files rather than copying them

Reported by: Adam Chidlow Owned by: nobody
Component: File uploads/storage Version: 1.10
Severity: Normal Keywords: FileField, TemporaryUploadedFile
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Using default settings, when a file is uploaded and is greater than 2.5MB it will get saved as a temporary file. However even if that temporary file is on the same file system as MEDIA_ROOT, it will be copied instead of moved.

This appears to be because FileField.pre_save passes the FieldFile as 'content' to Storage.save(), instead of FieldFile.file, which is the TemporaryUploadedFile having a temporary_file_path attribute thus invoking file_move_safe.

This appears to have been the behaviour from the beginning: https://github.com/django/django/blame/master/django/db/models/fields/files.py#L296

For large uploaded files this has some undesirable consequences, it increases the time for the request to complete, and when running Django certain ways (at least under gunicorn using gthread workers), the temporary file won't be closed until that worker has recieved another request, so it's taking up double the storage for that time.

Change History (12)

comment:1 by Tim Graham, 8 years ago

Component: UncategorizedFile uploads/storage
Resolution: duplicate
Status: newclosed
Summary: File uploads create copies instead of renamingFile uploads could rename temporary files rather than copying them
Type: BugCleanup/optimization

Looks like a duplicate of #21602.

comment:2 by Adam Chidlow, 8 years ago

Resolution: duplicate
Status: closednew

I'm pretty sure this is not a duplicate of #21602. From what I understand that ticket is talking about potential race conditions in a few different scenarios. In this case, the consequences of solving this ticket would solve one particular case of that ticket, but solving the linked ticket wouldn't necessarily do so in a way that solves this ticket as well.

Furthermore, I think this ticket should just be a one-line fix.

def pre_save(self, model_instance, add):
    "Returns field's value just before saving."
    file = super(FileField, self).pre_save(model_instance, add)
    if file and not file._committed:
        # Commit the file to storage prior to saving the model
        file.save(file.name, file, save=False)
    return file

Should change the file.save call to:

        file.save(file.name, file.file, save=False)

This will work, since the only times _committed is False is if the file has been deleted (in which case bool(file) is False), or a FileField has been set to an instance of File that is not also an instance of FieldFile, in which case passing the File object that was set to the field as the contents to Storage.save is perfectly valid.

comment:3 by Tim Graham, 8 years ago

I didn't look into the specifics but existing tests are passing with that change which is a good sign. Can you write a new test?

comment:4 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

comment:5 by Adam Chidlow, 8 years ago

Thanks for taking a second look.

I've written a test and put it and the one line change into a pull request.

This is my first pull request to django, if I've made any mistakes in coding conventions or the pull request process please let me know and I'll correct them.

Last edited 8 years ago by Tim Graham (previous) (diff)

comment:6 by Tim Graham, 8 years ago

Has patch: set
Patch needs improvement: set

comment:7 by Tim Graham, 8 years ago

Patch needs improvement: unset

comment:8 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In f734e2d:

Fixed #27334 -- Allowed FileField to move rather than copy a file.

When a FileField is set to an instance of File that is not also an
instance of FieldFile, pre_save() passes that object as the contents to
Storage.save(). This allows the file to be moved rather than copied
to the upload destination.

comment:9 by Tim Graham <timograham@…>, 6 years ago

In 89d4d412:

Fixed #28540 -- Doc'd a change to file upload permissions in Django 1.11.

Behavior changed in f734e2d4b2fc4391a4d097b80357724815c1d414
(refs #27334).

comment:10 by Tim Graham <timograham@…>, 6 years ago

In 37c0a336:

[2.1.x] Fixed #28540 -- Doc'd a change to file upload permissions in Django 1.11.

Behavior changed in f734e2d4b2fc4391a4d097b80357724815c1d414
(refs #27334).

Backport of 89d4d412404d31ef34ae3170c0c056eff55b2a17 from master

comment:11 by Tim Graham <timograham@…>, 6 years ago

In b113c6ad:

[2.0.x] Fixed #28540 -- Doc'd a change to file upload permissions in Django 1.11.

Behavior changed in f734e2d4b2fc4391a4d097b80357724815c1d414
(refs #27334).

Backport of 89d4d412404d31ef34ae3170c0c056eff55b2a17 from master

comment:12 by Tim Graham <timograham@…>, 6 years ago

In ceae3069:

[1.11.x] Fixed #28540 -- Doc'd a change to file upload permissions in Django 1.11.

Behavior changed in f734e2d4b2fc4391a4d097b80357724815c1d414
(refs #27334).

Backport of 89d4d412404d31ef34ae3170c0c056eff55b2a17 from master

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