#10121 closed (fixed)
ImageField (and FileField) loses data when re-saving from admin
Reported by: | Ville Säävuori | Owned by: | Karen Tracey |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Keywords: | admin, ImageField, save | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
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)
Change History (19)
follow-up: 2 comment:1 by , 16 years ago
follow-up: 3 comment:2 by , 16 years ago
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 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | new → 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 by , 16 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
follow-up: 6 comment:5 by , 16 years ago
I have the same problem.
Any workarounds before it is fixed in trunk?
follow-up: 7 comment:6 by , 16 years ago
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 by , 16 years ago
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 by , 16 years ago
Cc: | added |
---|
comment:9 by , 16 years ago
#10255 reported the same problem with FileField
s and was closed as a duplicate.
comment:10 by , 16 years ago
Summary: | ImageField loses data when re-saving from admin → ImageField (and FileField) loses data when re-saving from admin |
---|
Change summary to reflect the problem is not specific to ImageField.
by , 16 years ago
Attachment: | 10121.diff added |
---|
follow-up: 14 comment:12 by , 16 years ago
Triage Stage: | Unreviewed → 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.)
by , 16 years ago
Attachment: | form-file-save.diff added |
---|
follow-up: 15 comment:13 by , 16 years ago
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 by , 16 years ago
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 restoredsave_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 by , 16 years ago
Owner: | set to |
---|---|
Status: | new → 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 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → 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 by , 16 years ago
Cc: | removed |
---|
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.