ImageField (and FileField) loses data when re-saving from admin

[Ville Säävuori]

I have a following model with an ImageField:

class PublicCategoryManager(models.Manager):
    """
    Custom manager for displaying only public categories.
    """
    def get_query_set(self):
        return super(PublicCategoryManager, self).get_query_set().filter(is_public=True)
    
class Category(models.Model):
    """
    Category for products. Can relate to itself.
    """
    name = models.CharField(max_length=200)
    parent = models.ForeignKey('self', blank=True, null=True, related_name='child', verbose_name='parent')
    image = models.ImageField(blank=True, null=True, upload_to='categories/')
    is_public = models.BooleanField(default=True)

	 ...

    objects = models.Manager()
    public_objects = PublicCategoryManager()

    def save(self, force_insert=False, force_update=False):
        if not self.date_added:
            self.date_added = datetime.now()
        ...
        super(Category, self).save(force_insert, force_update)

it has a following Admin class

class CategoryAdmin(admin.ModelAdmin):
    ...
    save_as = True
    save_on_top = True
    prepopulated_fields = {"slug": ("name",)}
    raw_id_fields = ('parent',)

Image works OK when first saving it. But when re-saving the object (with of without editind any data) from the admin, the data on the imagefield (in db) is erased. When saving the data from python shell everything works OK.

We're using Python 2.5 and the latest SVN version (r9791) of Django.

[Jacob Smullyan]

I'm seeing exactly the same thing. The symptom is that the admin form is rendered without the image data, so any submit will lose it. The 1.0.X branch does not have this bug.

[Karen Tracey]

Replying to jsmullyan:

The 1.0.X branch does not have this bug.

That makes me wonder if this was introduced by r9766. There's a note in #10044 of problems with image width and height fields not being recorded properly in admin after r9766. It would be interesting to know if you see the problem on r9766, but not r9765.

[Ville Säävuori]

Replying to kmtracey:

That makes me wonder if this was introduced by r9766. There's a note in #10044 of problems with image width and height fields not being recorded properly in admin after r9766. It would be interesting to know if you see the problem on r9766, but not r9765.

I can, in fact, verify that at least in our case the bug is present in r9766 but not in r9765.

[Valera Grishin]

I have the same problem.
Any workarounds before it is fixed in trunk?

[Karen Tracey]

Replying to Valera Grishin:

I have the same problem.
Any workarounds before it is fixed in trunk?

The simplest workaround is to back off to r9765, or use the 1.0.X branch instead of trunk. If, however, you want to pick up trunk-only changes that have been made since then, just not r9766, you could, in your local tree, do a reverse merge to undo the effects of r9766, as is described here, for example:

​http://markphip.blogspot.com/2007/01/how-to-undo-commit-in-subversion.html

(minus the commit part). Those are the workarounds I can think of. (If someone who is having trouble with the effects of r9766 could investigate and come up with a fix that achieves the goals of r9766 without the problems it introduced, that would be even better.)

[Valera Grishin]

Replying to kmtracey:

Replying to Valera Grishin:

I have the same problem.
Any workarounds before it is fixed in trunk?

The simplest workaround is to back off to r9765, or use the 1.0.X branch instead of trunk.

I'll take the r9765 as it is the easiest way. Thanks for hint.

[Ramiro Morales]

#10255 reported the same problem with FileFields and was closed as a duplicate.

[Karen Tracey]

Change summary to reflect the problem is not specific to ImageField.

[Karen Tracey]

#10179 was another report of this.

[Karen Tracey]

I've attached a patch that adds a test for this case, and a possible fix. The added test passes on 1.0 and the current 1.0.X branch, fails on current trunk, and passes with the fix. The fix is to simply restore the old save_form_data method override that existed in FileField before r9766. I really have no idea if that is correct or appropriate, so review from someone with a clue in this area would be appreciated. (The entire test suite does pass with the restored save_form_data method, so if its removal was necessary for something added in r9766 that doesn't appear to be fully tested by the tests added there.)

Once this is fixed, we're left with the situation that once you have set a value to a blank=True FileField, there is no way with a vanilla ModelForm to set it back to blank, since if you just don't supply a file in the POST data the old value is used. But that is the way things used to be and seems to be a lesser evil than requiring re-upload of existing files in order to edit any other field in a model instance without losing the file setting. (#7048 looks to cover being able to "re-blank" FileFields in admin, but I don't know if the solution being investigated there provides a general answer for ModelFields or if it is admin-specific.)

[Alex Gaynor]

I've attached a patch that reinstates the save_form_data but still using the assignment, that way we still defer saving of the file to it's final on-disk location until the model is saved.

[Valera_Grishin]

Replying to kmtracey:

I've attached a patch that adds a test for this case, and a possible fix. The added test passes on 1.0 and the current 1.0.X branch, fails on current trunk, and passes with the fix. The fix is to simply restore the old save_form_data method override that existed in FileField before r9766. I really have no idea if that is correct or appropriate, so review from someone with a clue in this area would be appreciated. (The entire test suite does pass with the restored save_form_data method, so if its removal was necessary for something added in r9766 that doesn't appear to be fully tested by the tests added there.)

Once this is fixed, we're left with the situation that once you have set a value to a blank=True FileField, there is no way with a vanilla ModelForm to set it back to blank, since if you just don't supply a file in the POST data the old value is used. But that is the way things used to be and seems to be a lesser evil than requiring re-upload of existing files in order to edit any other field in a model instance without losing the file setting. (#7048 looks to cover being able to "re-blank" FileFields in admin, but I don't know if the solution being investigated there provides a general answer for ModelFields or if it is admin-specific.)

I think all these tickets (#10121, #10125, #7048) shows us that simply using standard browser widget for file uploads isn't a perfectly suitable for concepts like those behind ImageField and FileField.

That is the file upload widget is good for, well, uploading a file. But guessing whether the formerly uploaded file or image must be automatically deleted or kept is a work for an additional control (a checkbox obviosly). So, the FileField/ImageField must be presented with upload widget and checkbox simultaneously. Not sure though if current Django version supports multiple form elements for single database field.

[Karen Tracey]

Replying to Alex:

I've attached a patch that reinstates the save_form_data but still using the assignment, that way we still defer saving of the file to it's final on-disk location until the model is saved.

Ah, OK, I think I understand. And the subtle difference between whether the file data is saved during save_form_data or later is a bit hard to test, I guess. We're then left with a save_form_data whose entire reason for being, I think, is to prevent re-blanking of an already-set blank=True FileField. Perhaps future work to support re-blanking such fields will get rid of the need for this, but for now I guess it is OK to restore the old status quo.

[Karen Tracey]

(In [9840]) Fixed #10121: Restored code lost in r9766 that prevented overwriting an already-set blank=True FileField with blank. This would happen, for instance, in the admin if an object with a FileField was edited/saved but the file not re-uploaded, resulting in the association to the previously-uploaded file being lost. Adding the ability to re-blank FileFields when they are once set is the subject of a different ticket (#7048), for now the pre-9766 behavior here has just been restored in order to avoid unexpected data loss. Thanks to Alex for help in understanding how to fix this without un-doing the intent of r9766.