Opened 16 years ago

Last modified 20 months ago

#8912 new New feature

File storage and save/commit=False

Reported by: shadfc Owned by:
Component: File uploads/storage Version: 1.0
Severity: Normal Keywords:
Cc: jay.wineinger@… Triage Stage: Accepted
Has patch: no Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Background: I noticed this when playing with save_formset() on ModelAdmin in order to modify the data for a FileField on an inline model before it gets sent to S3 via a custom filestorage backend.

In admin.py

    def save_formset(self, request, form, formset, change):
        instances = formset.save(commit=False)
        for instance in instances:
            if not instance.id:
                raise Exception('testing file creation')
            instance.save()
        formset.save_m2m()

Now, when saving a new inline instance which has a file field, the exception is raised before that instance gets save()'d. I kept noticing, however, that I would still get the full uploaded file with the proper name in my S3 account. So, I raised an exception in the _save() on my custom backend and walked it back to the formset.save(commit=False) call.

It seems to me that the commit=False should keep file writes from happening; it doesn't. Looking in django/db/models/fields/files.py, the save() takes a save argument but doesn't use it when dealing with the save call to the storage backend. If the argument was passed along to the save on the storage backend, then it could decide whether or not to actually write the file.

Attachments (2)

storage_save_8975.diff (3.1 KB ) - added by shadfc 16 years ago.
simple patch against r8975
filefield_commit_8975.diff (1.3 KB ) - added by shadfc 16 years ago.
simpler patch which just requires save = True before "really doing stuff" in filefield save() and delete()

Download all attachments as: .zip

Change History (14)

comment:1 by shadfc, 16 years ago

Summary: Filestorage backend interface for save() should accept save/commit argumentFilestorage backend doesn't get or use "save" argument

comment:2 by anonymous, 16 years ago

Summary: Filestorage backend doesn't get or use "save" argumentFilestorage backend doesn't get "save" argument

by shadfc, 16 years ago

Attachment: storage_save_8975.diff added

simple patch against r8975

comment:3 by shadfc, 16 years ago

Has patch: set
Needs tests: set
Owner: changed from nobody to shadfc
Status: newassigned
Summary: Filestorage backend doesn't get "save" argumentFile storage and save/commit=False
Triage Stage: UnreviewedDesign decision needed

The attached patch basically just propagates the save argument in FieldFile.save() into storage.save() and then into storage._save(). This gives the custom backend designer control of the behavior.

Looking at what happens in storage.save() raised a few questions as to my approach. The call to storage.save() in FieldFile.save() expects the name of the saved file to be returned. So, if the backend gets a save=False argument and doesn't actually save the file, what should it return? It could just generate the expected filename -- the name it would have saved the content to, but that has no guarantee that when it really does save to the backend that that name is still available.

Should a save=False on FieldFile.save() just not call the stoarge.save() and set the FieldFile's name to something non-definite ('unsaved file')?

Also, I was just looking at FieldFile.delete() and saw that it has the same problem -- the storage.delete() call is not dependent on save=True and will always happen.

Input from people smarter than me would be welcome on whether I am wrong about all of this.

by shadfc, 16 years ago

Attachment: filefield_commit_8975.diff added

simpler patch which just requires save = True before "really doing stuff" in filefield save() and delete()

comment:4 by shadfc, 16 years ago

The second patch, which I just added, is much simpler than the first and just modifies the save() and delete() methods on FieldFile (typo in the patch description says FileField). It wont do anything permanent like calling the storage backend unless save=True.

Since save() requires a name from the storage backend, I've changed that to use the original name passed to save() and add on "(unsaved)" to the end of it for clarity's sake.

Now delete() really only closes the file.

I think I prefer this implementation. I don't see any real reason that a storage backend designer would want/need to know if save() was called with save=False.

Also, just as a note, the first patch is incomplete because I only noticed delete()'s behavior after I had posted it here, so that patch only affects save() and _save() on various classes.

comment:5 by shadfc, 16 years ago

Has patch: unset
Needs tests: unset
Triage Stage: Design decision neededAccepted

After actually testing out these things, I've realized that I just don't understand enough and these patches don't do what I thought they would.

comment:6 by shadfc, 16 years ago

Triage Stage: AcceptedUnreviewed

comment:7 by shadfc, 16 years ago

Owner: shadfc removed
Status: assignednew

comment:8 by Brian Rosner, 16 years ago

Triage Stage: UnreviewedAccepted

No need to go through the whole triage stage yourself. ;) I am marking accepted based on my decision in the mailing list. http://groups.google.com/group/django-developers/browse_frm/thread/e2057d165c3a0c12

comment:10 by Adam Nelson, 14 years ago

Needs documentation: set
Patch needs improvement: set

comment:11 by Luke Plant, 14 years ago

Severity: Normal
Type: New feature

comment:12 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:13 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

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