Opened 13 years ago

Closed 9 years ago

Last modified 9 years ago

#15590 closed New feature (fixed)

Document how to change the path of a FileField

Reported by: Simon Litchfield Owned by: Jorge Barata
Component: Documentation Version: dev
Severity: Normal Keywords: filefield imagefield path filename
Cc: ledermann@…, kitsunde@…, cmawebsite@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Currently we can't set filefield/imagefield's underlying filename, without loading the file and feeding it's contents into save(). Obviously this is grossly inefficient.

This is a simple patch that allows you to set the path like so --

instance.myfile.path = 'uploads/new-path.avi'

Previously, instance.myfile.path is read only. If you set instance.myfile then instance.myfile.path etc will raise a ValueError, The 'myfile' attribute has no file associated with it.

Works with FileField, ImageField, and anything else that uses a File object that is subclassed from FieldFile.

Check stackoverflow, google, etc- lots of people banging their heads trying to figure out why they can't simply set the filename.

Attachments (1)

filefield-path-set.diff (592 bytes ) - added by Simon Litchfield 13 years ago.

Download all attachments as: .zip

Change History (18)

by Simon Litchfield, 13 years ago

Attachment: filefield-path-set.diff added

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

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedDesign decision needed

I might be missing something here, but this doesn't seem like a very good idea to me -- at least, not in the way it's proposed.

The file path is readonly because it points to an actual file. You can't just change the path value, because then it will point to a file location that won't exist. And changing the file path as a column operation isn't something that (to me) is an obvious interface for a 'file move' operation. There's also the problem of whether the file backends support atomic move operations (which may be problematic on some platforms).

However, if there's mass confusion, there is something that needs to be looked at here -- even if it's just a documentation fix or a tightening of error messages. Further discussion required.

comment:2 by Simon Litchfield, 13 years ago

I agree, "moving" a file should be explicit, something like myfield.move(src, target).

The issue here, is cases where other processes or whatever will place a file in a certain location, and you just need to "set" that location in your database. Whether or not a "move" is involved. Currently you can't do that. Allowing 'path' to be set makes sense in these situations.

Remember, django can't take the stance that it guarantees a file will be there just because it says so in the database. It's up to the developer to make sure they don't set .path to some non-existant rubbish, in the same way they need to make sure the file is there.

People are doing crazy stuff like opening the whole file and saving it back using the save() method (since save and delete are the only methods provided in docs).

The only other workaround is ugly, I don't think we wanna be adding this to the docs --

model_instance.myfile = model_instance.myfile.field.attr_class(instance, model_instance.myfile.field, 'my-filename.jpg')

comment:3 by Luke Plant, 13 years ago

Type: New feature

comment:4 by Luke Plant, 13 years ago

Severity: Normal

comment:5 by Flo Ledermann, 13 years ago

Cc: ledermann@… added
Easy pickings: unset
UI/UX: unset

This may be a slightly different usecase or a subset of the cases covered by this issue, but my approach to setting a FileField to contain a server-processed file is using a helper class copied together from django.core.files.uploadedfile.UploadedFile and django.core.files.uploadedfile.TemporaryUploadedFile :

import os
from django.conf import settings
from django.core.files.base import File
from django.core.files import temp as tempfile


class DjangoTempFile(File):

    def __init__(self, name=None, suffix='.temp', content_type=None, size=None, charset=None):
        if settings.FILE_UPLOAD_TEMP_DIR:
            file = tempfile.NamedTemporaryFile(suffix=suffix,
                dir=settings.FILE_UPLOAD_TEMP_DIR)
        else:
            file = tempfile.NamedTemporaryFile(suffix=suffix)

        super(DjangoTempFile, self).__init__(file, name)

        self.content_type = content_type

        # TODO check if these could be removed
        self.size = size
        self.charset = charset

    def __repr__(self):
        return "<%s: %s (%s)>" % (
            self.__class__.__name__, smart_str(self.name), self.content_type)

    def temporary_file_path(self):
        """
        Returns the full path of this file.
        """
        return self.file.name

    def close(self):
        try:
            return self.file.close()
        except OSError, e:
            if e.errno != 2:
                # Means the file was moved or deleted before the tempfile
                # could unlink it.  Still sets self.file.close_called and
                # calls self.file.file.close() before the exception
                raise

    def _get_name(self):
        return self._name

    def _set_name(self, name):
        # Sanitize the file name so that it can't be dangerous.
        if name is not None:
            # Just use the basename of the file -- anything else is dangerous.
            name = os.path.basename(name)

            # File names longer than 255 characters can cause problems on older OSes.
            if len(name) > 255:
                name, ext = os.path.splitext(name)
                name = name[:255 - len(ext)] + ext

        self._name = name

    name = property(_get_name, _set_name)

You can use this file to write to from whatever code you want, and then call model.image_field.save(name, temp_file_instance) which will pick up the temporary_file_path method and copy the file over instead of re-reading it. The advantage is that you benefit from Django's name-collision resolving, and external processes should probably not write to the MEDIA_ROOT anyway but use a temporary file instead.

Maybe Django should provide such a class as part of the official API?

comment:6 by Jacob, 12 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:7 by Kit Sunde, 12 years ago

Cc: kitsunde@… added

comment:8 by Jacob, 11 years ago

Triage Stage: Design decision neededAccepted

I'm unconvinced of the specifics (see Russ's comments; allowing the path to be settable seems like a foot-gun), but something needs to be done here. Marking accepted; hopefully someone can come up with a better API.

comment:9 by Simon Litchfield, 11 years ago

Just to clarify, all that's wanted here is a way to set a value in the database. Currently it's not possible unless you resort to raw SQL or the 'ugly hack' I posted above. Every other field is "settable". Django is assuming control of my files<->database; and I don't always necessarily want that, particularly if I go as far as deliberately using the assignment operator.

That said I agree with Russ, we could certainly use a move() type method (though I'm not sure if that's what people have been asking for).

comment:10 by Claude Paroz, 10 years ago

I think that the OP use case is solved by setting the name instead of the path property (name being relative to the field storage base location), as demonstrated by the following test case:

diff --git a/tests/file_storage/tests.py b/tests/file_storage/tests.py
index c3800cb..573914b 100644
--- a/tests/file_storage/tests.py
+++ b/tests/file_storage/tests.py
@@ -560,6 +560,26 @@ class FileFieldStorageTests(unittest.TestCase):
         with temp_storage.open('tests/stringio') as f:
             self.assertEqual(f.read(), b'content')
 
+    def test_filefield_name_updatable(self):
+        """
+        Test that the name attribute of a FileField can be changed to an existing file.
+        """
+        obj1 = Storage()
+        obj1.normal.save("django_test.txt", ContentFile("content"))
+
+        initial_path = obj1.normal.path
+        initial_name = obj1.normal.name
+        new_path = initial_path.replace("test.txt", "test2.txt")
+        new_name = initial_name.replace("test.txt", "test2.txt")
+        # Rename the underlying file object
+        os.rename(initial_path, new_path)
+        obj1.normal.name = new_name
+        obj1.save()
+
+        obj1 = Storage.objects.get(pk=obj1.pk)
+        self.assertEqual(obj1.normal.name, new_name)
+        self.assertEqual(obj1.normal.path, new_path)
+
 
 # Tests for a race condition on file saving (#4948).
 # This is written in such a way that it'll always pass on platforms

If confirmed, this should be of course made clearer in the documentation.

comment:11 by Collin Anderson, 9 years ago

Cc: cmawebsite@… added
Component: File uploads/storageDocumentation
Easy pickings: set
Has patch: unset
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Summary: FileField path isn't settableDocument how to change the path of a FileField

I agree setting .name is the way to go. Just needs to be better documented.

comment:12 by Tim Graham, 9 years ago

Owner: changed from Simon Litchfield to Tim Graham
Status: newassigned

comment:13 by Tim Graham, 9 years ago

Owner: Tim Graham removed
Status: assignednew

comment:14 by Jorge Barata, 9 years ago

Owner: set to Jorge Barata
Status: newassigned

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

Resolution: fixed
Status: assignedclosed

In 931a340:

Fixed #15590 -- Documented how the path of a FileField can be changed.

Thanks simon29 for report, and freakboy3742, floledermann,
jacob, claudep and collinanderson for discussing the task.

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

In 18f11072:

[1.7.x] Fixed #15590 -- Documented how the path of a FileField can be changed.

Thanks simon29 for report, and freakboy3742, floledermann,
jacob, claudep and collinanderson for discussing the task.

Backport of 931a340f1feca05b7a9f95efb9a3ba62b93b37f9 from master

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

In 2bddc74:

[1.8.x] Fixed #15590 -- Documented how the path of a FileField can be changed.

Thanks simon29 for report, and freakboy3742, floledermann,
jacob, claudep and collinanderson for discussing the task.

Backport of 931a340f1feca05b7a9f95efb9a3ba62b93b37f9 from master

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