[Django] #27334: File uploads create copies instead of renaming
Django
----------------------------+----------------------------------------------
Reporter: Adam | Owner: nobody
Chidlow |
Type: Bug | Status: new
Component: | Version: 1.10
Uncategorized |
Severity: Normal | Keywords: FileField, TemporaryUploadedFile
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+----------------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27334>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution: duplicate
Keywords: FileField, | Triage Stage:
TemporaryUploadedFile | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* needs_better_patch: => 0
* component: Uncategorized => File uploads/storage
* resolution: => duplicate
* needs_tests: => 0
* needs_docs: => 0
* type: Bug => Cleanup/optimization
Comment:
Looks like a duplicate of #21602.
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution:
TemporaryUploadedFile | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam Chidlow):
* status: closed => new
* resolution: duplicate =>
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution:
Keywords: FileField, | Triage Stage:
TemporaryUploadedFile | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution:
TemporaryUploadedFile |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution:
Keywords: FileField, | Triage Stage: Accepted
TemporaryUploadedFile |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam Chidlow):
Thanks for taking a second look.
I've written a test and put it and the one line change into a GitHub 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution:
Keywords: FileField, | Triage Stage: Accepted
TemporaryUploadedFile |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution:
Keywords: FileField, | Triage Stage: Accepted
TemporaryUploadedFile |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Keywords: FileField, | Triage Stage: Accepted
TemporaryUploadedFile |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"f734e2d4b2fc4391a4d097b80357724815c1d414" f734e2d]:
{{{
#!CommitTicketReference repository=""
revision="f734e2d4b2fc4391a4d097b80357724815c1d414"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: FileField, | Triage Stage: Accepted
TemporaryUploadedFile |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"89d4d412404d31ef34ae3170c0c056eff55b2a17" 89d4d412]:
{{{
#!CommitTicketReference repository=""
revision="89d4d412404d31ef34ae3170c0c056eff55b2a17"
Fixed #28540 -- Doc'd a change to file upload permissions in Django 1.11.
Behavior changed in f734e2d4b2fc4391a4d097b80357724815c1d414
(refs #27334).
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: FileField, | Triage Stage: Accepted
TemporaryUploadedFile |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"37c0a3365531815b6db5576ca18ba684cc84d12d" 37c0a336]:
{{{
#!CommitTicketReference repository=""
revision="37c0a3365531815b6db5576ca18ba684cc84d12d"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: FileField, | Triage Stage: Accepted
TemporaryUploadedFile |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"b113c6adea2e4b1759bc5dc27b6cc5cc339f633a" b113c6ad]:
{{{
#!CommitTicketReference repository=""
revision="b113c6adea2e4b1759bc5dc27b6cc5cc339f633a"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chidlow | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: FileField, | Triage Stage: Accepted
TemporaryUploadedFile |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"ceae3069ec2f0fd9f53ae901a55b4f9c985a4e78" ceae3069]:
{{{
#!CommitTicketReference repository=""
revision="ceae3069ec2f0fd9f53ae901a55b4f9c985a4e78"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27334#comment:12>