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)

Screen Shot 2013-09-06 at 9.48.17 PM.png (152.3 KB ) - added by kamni 11 years ago.
Screenshot demonstrating that the bug is no longer valid
Screen Shot 2013-09-07 at 1.49.02 AM.png (172.9 KB ) - added by kamni 11 years ago.
Screenshot disconfirming the 'change_view' portion of the bug

Download all attachments as: .zip

Change History (21)

comment:1 by Marc Tamlyn, 11 years ago

Component: Uncategorizedcontrib.admin
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:2 by kamni, 11 years ago

Owner: changed from nobody to kamni
Status: newassigned

by kamni, 11 years ago

Screenshot demonstrating that the bug is no longer valid

comment:3 by kamni, 11 years ago

Please disregard the first uploaded screenshot, as there was a mistake in the setup.

Last edited 11 years ago by kamni (previous) (diff)

by kamni, 11 years ago

Screenshot disconfirming the 'change_view' portion of the bug

comment:4 by kamni, 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.

There is a pull request that includes the automated tests to reproduce the bug at: https://github.com/kamni/django/pull/1

Version 0, edited 11 years ago by kamni (next)

comment:5 by kamni, 11 years ago

Version: 1.5master

comment:6 by kamni, 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)

Last edited 11 years ago by kamni (previous) (diff)

comment:7 by Tim Graham, 11 years ago

Patch needs improvement: set

If we can reuse existing models for the tests that would be ideal.

comment:8 by kamni, 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 Daniele Procida, 11 years ago

Cc: Daniele Procida added

comment:10 by Tim Graham, 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 Daniele Procida, 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.

comment:12 by anonymous, 11 years ago

timo: would the Validation on a ModelForm link be the appropriate place for documentation, or would you recommend somewhere else?

in reply to:  12 comment:13 by kamni, 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 Tim Graham, 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 anonymous, 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 Tim Graham, 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 kamni, 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 Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In c74504c2dd21974571ab72805fbfc8d4d76ce151:

Fixed #20522 - Allowed use of partially validated object in ModelAdmin.add_view formset validation.

Updated ModelAdmin to use form.instance when passing parent model to
child inlines for add_view. There is effectively no change in the
change_view since the previously passed 'obj' is the same as form.instance.

Thanks to meshy for report, and EvilDMP and timo for review.

Note: See TracTickets for help on using tickets.
Back to Top