Opened 11 years ago
Closed 11 years ago
#20522 closed Bug (fixed)
Admin formset validation cannot take submitted model instance into account when form not valid.
Reported by: | Charlie Denton | Owned by: | kamni |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | admin formset validation |
Cc: | Daniele Procida | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
In the admin, if the validation on an inline formset depends upon the data from the main form it cannot be correctly validated when the main form is not valid. This is because the instance from the incomplete form is not passed into the formset unless it is completely valid. Instead, on add (https://github.com/django/django/blob/1.5.1/django/contrib/admin/options.py#L988-L994), we get a completely blank new instance, or on change (https://github.com/django/django/blob/1.5.1/django/contrib/admin/options.py#L1085-L1091), we are passed the original instance.
I understand that the current way of doing things may stop invalid data being sent to the formsets but I believe, as this is the data that has been submitted, and the majority of it will likely be correct, that this would be better than the nothing we currently get.
I propose that lines 994 and 1091 both change to new_object = form.instance
. This would mean that in a fair number of cases the errors on the main form would not need to be fixed before the errors on the inline form become visible.
I hope that my concerns/intention are clear, but if not, please ask and I will produce a working example and patch for examination.
Attachments (2)
Change History (21)
comment:1 by , 11 years ago
Component: | Uncategorized → contrib.admin |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:2 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
by , 11 years ago
Attachment: | Screen Shot 2013-09-06 at 9.48.17 PM.png added |
---|
comment:3 by , 11 years ago
Please disregard the first uploaded screenshot, as there was a mistake in the setup.
by , 11 years ago
Attachment: | Screen Shot 2013-09-07 at 1.49.02 AM.png added |
---|
Screenshot disconfirming the 'change_view' portion of the bug
comment:4 by , 11 years ago
I was able to confirm the first part of the bug (add_view), but could not replicate any bug for change_view. Please see the second screenshot attached to this bug for an example of the latter. I have also created a repository for test code to manually duplicate the issue (https://github.com/kamni/django_20522). Instructions for duplicating these findings are in the README, and can be used later to confirm that the bug has been fixed.
comment:5 by , 11 years ago
Version: | 1.5 → master |
---|
Pull request for tests at https://github.com/django/django/pull/1590
comment:6 by , 11 years ago
Has patch: | set |
---|
Bug fixed. Patch for review: https://github.com/django/django/pull/1590
Please provide feedback on whether/how the commits should be squashed (e.g., one commit for the tests and one for the fix, or as one single commit)
comment:7 by , 11 years ago
Patch needs improvement: | set |
---|
If we can reuse existing models for the tests that would be ideal.
comment:8 by , 11 years ago
In order to test this bug, we need two models where the validation of one of the child's fields is contingent on the validation of one of the parent's fields, with a special admin that handled this validation. I was hesitant to alter an existing set of models/admin out of concern of breaking other testing.
I already confirmed that adding the new models was valid in this particular case with EvilDMP and one other core dev at the sprints before submitting this patch. If you have suggestions about which models I should change instead, please let me know.
comment:9 by , 11 years ago
Cc: | added |
---|
comment:10 by , 11 years ago
I sympathize with that concern and am ok adding the additional models. I left a couple more comments on the PR.
In wondering why the code wasn't written like this in the first place, I recalled a warning in Validation on a ModelForm.
"The cleaning process modifies the model instance passed to the ModelForm constructor in various ways. For instance, any date fields on the model are converted into actual date objects. Failed validation may leave the underlying model instance in an inconsistent state and therefore it’s not recommended to reuse it."
As the OP points out, hopefully "this will be better than the nothing we currently get", but I mention it in case a documentation update might be helpful somewhere. It seems like it could actually break existing formset validation code if model attributes are different types when the form is invalid which makes me a little nervous.
comment:11 by , 11 years ago
kamni: I've merged your branch with recent updates to django master, at https://github.com/evildmp/django/tree/kamni-bug/20522; tests still pass. It may be worth merging those updates into your branch as it will make it easier for others to work with it.
follow-up: 13 comment:12 by , 11 years ago
timo: would the Validation on a ModelForm link be the appropriate place for documentation, or would you recommend somewhere else?
comment:13 by , 11 years ago
Replying to anonymous:
timo: would the Validation on a ModelForm link be the appropriate place for documentation, or would you recommend somewhere else?
This comment is mine.
comment:14 by , 11 years ago
Easy pickings: | unset |
---|
This change would be specific to contrib.admin so the documentation would go there (along with a backwards incompatible note in the release notes). I guess I'm -0 on this change for the backwards compatibility concern and the advice in our own docs that we shouldn't reuse model instances that have failed validation. Feel free to persuade me otherwise (or find another core dev who really wants this).
comment:15 by , 11 years ago
So one thing to consider: as you pointed out for the tests, the change_view tests already pass with the old code (i.e., the bug is only for the 'add' view -- see comment 4). I even considered the possibility that my tests were running validation against the old version of the model and that's why it was passing; but when I updated the tests so that the formset matched the expected data from the old version but the updated version would cause invalid fields (i.e., the tests should fail if the code wasn't reusing a model with failed validation), the tests still passed with the old options.py.
This seems to imply that the change_view is already disregarding the "Don't re-use models that have failed validation". Clearly we have inconsistent behavior between two views that needs to be remedied in one way or another.
So should we move forward with this ticket and make the add_view consistent with what change_view already seems to be doing, or should we open a new ticket to revert the change_view's behavior to be more consistent with Validation on a ModelForm?
comment:16 by , 11 years ago
Good point. In that case, I'm comfortable with this change. Please go ahead and squash your commits and make sure your commit message confirms to our guidelines.
comment:17 by , 11 years ago
The bug fix is now a single commit.
I also added a second commit with the requested documentation regarding Validation on a ModelForm. It can be found in contrib.admin.index under the InlineModelAdmin.form section. A review for the docs would be much appreciated.
comment:19 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Screenshot demonstrating that the bug is no longer valid