Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#32243 closed Cleanup/optimization (fixed)

Clarify docs on manually setting and saving a FileField.

Reported by: Gordon Wrigley Owned by: Joshua Massover
Component: Documentation Version: 3.1
Severity: Normal Keywords:
Cc: Joshua Massover Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The documentation talks about how to set a filefield on a new object and how to set a filefield using a form but I couldn't find anything that talked about how to directly set a filefield on an existing object.
https://docs.djangoproject.com/en/3.1/topics/http/file-uploads/#handling-uploaded-files-with-a-model

Maybe those docs could use some extra detail on how you can set an existing file field by either assigning a ContentFile (with a name) to the field and saving the instance, or by calling save on the field and passing a ContentFile and a name. And particularly how the name is required in both cases and will be passed through upload_to.

Further figuring this out for myself was substantially complicated by the following behaviour:

In [24]: e=MyModel.objects.first()

In [25]: e.my_file
Out[25]: <FieldFile: None>

In [26]: e.my_file=ContentFile(content=b"fred")

In [27]: e.save()

In [28]: e=MyModel.objects.first()

In [29]: e.my_file
Out[29]: <FieldFile: None>

In [30]: e.my_file=ContentFile(content=b"bob", name="bob.txt")

In [31]: e.save()

In [32]: e=MyModel.objects.first()

In [33]: e.my_file
Out[33]: <FieldFile: files/5bc2fe4c-4262-4134-9397-c740de5a7edf/bob.txt>

In [34]: e.my_file.open().read()
Out[34]: b'bob'

Particularly 26-29 where setting the filefield to a ContentFile with no name and then saving is effectively just ignored with no error.
Is this expected behaviour?

Change History (18)

comment:1 by Carlton Gibson, 4 years ago

Component: UncategorizedDocumentation
Type: UncategorizedCleanup/optimization

Hi Gordon. Thanks for the report.

I agree this is an area that folks find confusing. Happy to review suggested changes.

I think the right place for a clarification would be in the Using files in models section of the Files topic documentation.

Perhaps a review of the tests in tests/files to ensure we've covered all the relevant cases would also be in order.

comment:2 by Carlton Gibson, 4 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Carlton Gibson, 4 years ago

Summary: Saving filefields.Clarify docs on manually setting and saving a FileField.

comment:4 by Hasanul Islam, 4 years ago

Owner: changed from nobody to Hasanul Islam
Status: newassigned

in reply to:  3 ; comment:5 by Hasanul Islam, 4 years ago

Hi Carlton,
As the file is not being saved without the name attribute of the file, shouldn't we generate the name property randomly if not provided by the user?

If yes, I will update the relevant codes implementing random name generation, otherwise, I will update the documentation mentioning name is required. However, I think we should not enforce the user to provide the name attribute. I would prefer to update the code.

in reply to:  5 comment:6 by Jesse, 4 years ago

Replying to Hasanul Islam:

Hi Carlton,
As the file is not being saved without the name attribute of the file, shouldn't we generate the name property randomly if not provided by the user?

If yes, I will update the relevant codes implementing random name generation, otherwise, I will update the documentation mentioning name is required. However, I think we should not enforce the user to provide the name attribute. I would prefer to update the code.

TL;DR: Allow Files (not just ContentFiles) with a blank/None name. In FileField.save(), add a check after the generate_filename() call to make sure the file actually has a name once we're ready to save it to disk.

When saving the model it seems to still use the upload_to argument. If you pass something for upload_to it will overwrite whatever was passed for name.

Here's an example of my use case. A user can upload a picture through the admin panel. Whenever a new picture is uploaded I create a thumbnail for it.

def photo_upload_path(instance, original_filename):
    filename = hashlib.sha256(str(time.time()).encode("utf-8")).hexdigest()
    filename = filename[:16]
    return f"photos/{filename}.jpg"


def thumbnail_upload_path(instance, original_filename):
    filename = os.path.basename(instance.photo.name)
    return f"photos/thumbnails/{filename}"


class MyModel(models.Model):
    photo = models.ImageField(upload_to=photo_upload_path)

    # Hidden in Django admin. Managed by us.
    photo_thumbnail = models.ImageField(upload_to=thumbnail_upload_path)

    def save(self):
        img = Image.open(self.photo.path)
        img.thumbnail(settings.THUMBNAIL_SIZE)

        contents = io.BytesIO()
        img.save(contents, "JPEG")

        self.photo_thumbnail = ContentFile(contents.getvalue(), name="foo")

        super.save()

In this case, the photo gets renamed to photos/<hash>.jpg, and I want thumbnails to go to photos/thumbnails/<hash>.jpg.

I resize the image, save its contents to a BytesIO object, and create the ContentFile object. However I have to pass in a bogus name for the ContentFile even though it will use the upload_to to overwrite it. If I pass an empty name Django seems to throw the file away and it doesn't get saved.

Last edited 4 years ago by Jesse (previous) (diff)

comment:7 by Joshua Massover, 3 years ago

#33388 was closed as a duplicate to this one. I included an example but not in the Using files in models as Carlton suggested, I put it in Handling uploaded files Glad to move it where ever if this does not suffice.

https://github.com/django/django/pull/15245

Last edited 3 years ago by Joshua Massover (previous) (diff)

comment:8 by Joshua Massover, 3 years ago

Cc: Joshua Massover added
Has patch: set
Owner: changed from Hasanul Islam to Joshua Massover

comment:9 by Mariusz Felisiak, 3 years ago

Patch needs improvement: set

See comment.

in reply to:  9 comment:10 by Joshua Massover, 3 years ago

Replying to Mariusz Felisiak:

See comment.

Updated the PR

comment:11 by Joshua Massover, 3 years ago

PR updated from more feedback

comment:12 by Joshua Massover, 3 years ago

Updated again from more feedback!

comment:13 by Joshua Massover, 3 years ago

Patch needs improvement: unset

comment:14 by Carlton Gibson, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In c9d6e35:

Fixed #32243 -- Added docs examples for manually saving Files.

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 76c80d9:

[4.0.x] Fixed #32243 -- Added docs examples for manually saving Files.

Backport of c9d6e3595cfd0aa58cde1656bd735ecfcd7a872b from main

comment:17 by GitHub <noreply@…>, 3 years ago

In 25514b60:

Refs #32243 -- Fixed typo in docs/topics/files.txt.

comment:18 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 3714b44:

[4.0.x] Refs #32243 -- Fixed typo in docs/topics/files.txt.

Backport of 25514b604a64686ba603bf10a8a63390dc38b79d from main

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