Opened 4 years ago

Closed 4 months ago

Last modified 4 months ago

#15590 closed New feature (fixed)

Document how to change the path of a FileField

Reported by: simon29 Owned by: jorgebg
Component: Documentation Version: master
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 simon29 4 years ago.

Download all attachments as: .zip

Change History (18)

Changed 4 years ago by simon29

comment:1 Changed 4 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 4 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 4 years ago by lukeplant

  • Type set to New feature

comment:4 Changed 4 years ago by lukeplant

  • Severity set to Normal

comment:5 Changed 4 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 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:7 Changed 4 years ago by kitsunde

  • Cc kitsunde@… added

comment:8 Changed 2 years 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 2 years 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 13 months 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.

comment:11 Changed 5 months ago by collinanderson

  • Cc cmawebsite@… added
  • Component changed from File uploads/storage to Documentation
  • Easy pickings set
  • Has patch unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from FileField path isn't settable to Document 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 Changed 4 months ago by timgraham

  • Owner changed from simon29 to timgraham
  • Status changed from new to assigned

comment:13 Changed 4 months ago by timgraham

  • Owner timgraham deleted
  • Status changed from assigned to new

comment:14 Changed 4 months ago by jorgebg

  • Owner set to jorgebg
  • Status changed from new to assigned

comment:15 Changed 4 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 Changed 4 months ago by Tim Graham <timograham@…>

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 Changed 4 months ago by Tim Graham <timograham@…>

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