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)
Change History (14)
comment:1 by , 16 years ago
Summary: | Filestorage backend interface for save() should accept save/commit argument → Filestorage backend doesn't get or use "save" argument |
---|
comment:2 by , 16 years ago
Summary: | Filestorage backend doesn't get or use "save" argument → Filestorage backend doesn't get "save" argument |
---|
by , 16 years ago
Attachment: | storage_save_8975.diff added |
---|
comment:3 by , 16 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Owner: | changed from | to
Status: | new → assigned |
Summary: | Filestorage backend doesn't get "save" argument → File storage and save/commit=False |
Triage Stage: | Unreviewed → 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.
by , 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 , 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 , 16 years ago
Has patch: | unset |
---|---|
Needs tests: | unset |
Triage Stage: | Design decision needed → 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 by , 16 years ago
Triage Stage: | Accepted → Unreviewed |
---|
comment:7 by , 16 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:8 by , 16 years ago
Triage Stage: | Unreviewed → 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:10 by , 14 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
comment:11 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
simple patch against r8975