Opened 10 years ago

Last modified 18 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


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.


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

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 call.

It seems to me that the commit=False should keep file writes from happening; it doesn't. Looking in django/db/models/fields/, 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 10 years ago.
simple patch against r8975
filefield_commit_8975.diff (1.3 KB) - added by shadfc 10 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 Changed 10 years ago by shadfc

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

comment:2 Changed 10 years ago by anonymous

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

Changed 10 years ago by shadfc

Attachment: storage_save_8975.diff added

simple patch against r8975

comment:3 Changed 10 years ago by shadfc

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 into and then into storage._save(). This gives the custom backend designer control of the behavior.

Looking at what happens in raised a few questions as to my approach. The call to in 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 just not call the 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.

Changed 10 years ago by shadfc

Attachment: filefield_commit_8975.diff added

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

comment:4 Changed 10 years ago by shadfc

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 Changed 10 years ago by shadfc

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 Changed 10 years ago by shadfc

Triage Stage: AcceptedUnreviewed

comment:7 Changed 10 years ago by shadfc

Owner: shadfc deleted
Status: assignednew

comment:8 Changed 10 years ago by Brian Rosner

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.

comment:10 Changed 8 years ago by Adam Nelson

Needs documentation: set
Patch needs improvement: set

comment:11 Changed 7 years ago by Luke Plant

Severity: Normal
Type: New feature

comment:12 Changed 6 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:13 Changed 6 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

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