Admin formset validation cannot take submitted model instance into account when form not valid.

[Charlie Denton]

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.

[kamni]

Screenshot demonstrating that the bug is no longer valid

[kamni]

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

[kamni]

Screenshot disconfirming the 'change_view' portion of the bug

[kamni]

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

[kamni]

Pull request for tests at ​https://github.com/django/django/pull/1590

[kamni]

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)

[Tim Graham]

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

[kamni]

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.

[Tim Graham]

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.

[Daniele Procida]

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.

[anonymous]

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

[kamni]

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.

[Tim Graham]

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

[anonymous]

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?

[Tim Graham]

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.

[kamni]

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.

[kamni]

Pull request at ​https://github.com/django/django/pull/1960

[Tim Graham <timograham@…>]

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.