Code

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#10121 closed (fixed)

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

Reported by: Uninen Owned by: kmtracey
Component: contrib.admin Version: master
Severity: Keywords: admin, ImageField, save
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

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.

Attachments (2)

10121.diff (1.5 KB) - added by kmtracey 5 years ago.
form-file-save.diff (1.4 KB) - added by Alex 5 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 follow-up: Changed 5 years ago by jsmullyan

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 5 years ago by kmtracey

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.

comment:3 in reply to: ↑ 2 Changed 5 years ago by Uninen

  • Owner changed from nobody to Uninen
  • Status changed from new to assigned

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.

comment:4 Changed 5 years ago by anonymous

  • Owner Uninen deleted
  • Status changed from assigned to new

comment:5 follow-up: Changed 5 years ago by Valera Grishin

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

comment:6 in reply to: ↑ 5 ; follow-up: Changed 5 years ago by 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. 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.)

comment:7 in reply to: ↑ 6 Changed 5 years ago by 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.

comment:8 Changed 5 years ago by anonymous

  • Cc valera.grishin@… added

comment:9 Changed 5 years ago by ramiro

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

comment:10 Changed 5 years ago by kmtracey

  • Summary changed from ImageField loses data when re-saving from admin to ImageField (and FileField) loses data when re-saving from admin

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

comment:11 Changed 5 years ago by kmtracey

#10179 was another report of this.

Changed 5 years ago by kmtracey

comment:12 follow-up: Changed 5 years ago by kmtracey

  • Triage Stage changed from Unreviewed to Accepted

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

Changed 5 years ago by Alex

comment:13 follow-up: Changed 5 years ago by 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.

comment:14 in reply to: ↑ 12 Changed 5 years ago by 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.

comment:15 in reply to: ↑ 13 Changed 5 years ago by kmtracey

  • Owner set to kmtracey
  • Status changed from new to assigned

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.

comment:16 Changed 5 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:17 Changed 5 years ago by Valera_Grishin

  • Cc valera.grishin@… removed

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.