Opened 7 years ago

Last modified 5 years 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 7 years ago.
simple patch against r8975
filefield_commit_8975.diff (1.3 KB) - added by shadfc 7 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 (15)

comment:1 Changed 7 years ago by shadfc

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from Filestorage backend interface for save() should accept save/commit argument to Filestorage backend doesn't get or use "save" argument

comment:2 Changed 7 years ago by anonymous

  • Summary changed from Filestorage backend doesn't get or use "save" argument to Filestorage backend doesn't get "save" argument

Changed 7 years ago by shadfc

simple patch against r8975

comment:3 Changed 7 years ago by shadfc

  • Has patch set
  • Needs tests set
  • Owner changed from nobody to shadfc
  • Status changed from new to assigned
  • Summary changed from Filestorage backend doesn't get "save" argument to File storage and save/commit=False
  • Triage Stage changed from Unreviewed to Design 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.

Changed 7 years ago by shadfc

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

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

  • Has patch unset
  • Needs tests unset
  • Triage Stage changed from Design decision needed to Accepted

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

  • Triage Stage changed from Accepted to Unreviewed

comment:7 Changed 7 years ago by shadfc

  • Owner shadfc deleted
  • Status changed from assigned to new

comment:8 Changed 7 years ago by brosner

  • Triage Stage changed from Unreviewed to Accepted

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:9 Changed 6 years ago by adamnelson

Any chance of this getting into milestone:1.2 ?

comment:10 Changed 5 years ago by adamnelson

  • Needs documentation set
  • Patch needs improvement set

comment:11 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:12 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:13 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

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