Code

Opened 3 years ago

Last modified 5 days ago

#15590 new New feature

FileField path isn't settable

Reported by: simon29 Owned by: simon29
Component: File uploads/storage Version: master
Severity: Normal Keywords: filefield imagefield path filename
Cc: ledermann@…, kitsunde@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no 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 simon29 3 years ago.

Download all attachments as: .zip

Change History (11)

Changed 3 years ago by simon29

comment:1 Changed 3 years ago by russellm

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design 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 Changed 3 years ago by simon29

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

  • Type set to New feature

comment:4 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:5 Changed 3 years ago by floledermann

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

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:7 Changed 3 years ago by kitsunde

  • Cc kitsunde@… added

comment:8 Changed 15 months ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 15 months ago by simon29

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 Changed 5 days ago by claudep

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from simon29 to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.