File storage and save/commit=False

[shadfc]

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.

[shadfc]

simple patch against r8975

[shadfc]

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.

[shadfc]

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

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

[shadfc]

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.

[Brian Rosner]

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

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.