Code

Opened 11 months ago

Closed 5 months ago

#20522 closed Bug (fixed)

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

Reported by: meshy Owned by: kamni
Component: contrib.admin Version: master
Severity: Normal Keywords: admin formset validation
Cc: EvilDMP 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 7 months 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 7 months ago.
Screenshot disconfirming the 'change_view' portion of the bug

Download all attachments as: .zip

Change History (21)

comment:1 Changed 11 months ago by mjtamlyn

  • Component changed from Uncategorized to contrib.admin
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

comment:2 Changed 8 months ago by kamni

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

Changed 7 months ago by kamni

Screenshot demonstrating that the bug is no longer valid

comment:3 Changed 7 months ago by kamni

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

Version 0, edited 7 months ago by kamni (next)

Changed 7 months ago by kamni

Screenshot disconfirming the 'change_view' portion of the bug

comment:4 Changed 7 months ago by 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.

Last edited 7 months ago by kamni (previous) (diff)

comment:5 Changed 7 months ago by kamni

  • Version changed from 1.5 to master

comment:6 Changed 7 months ago by kamni

  • 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 7 months ago by kamni (previous) (diff)

comment:7 Changed 7 months ago by timo

  • Patch needs improvement set

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

comment:8 Changed 7 months ago by 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.

comment:9 Changed 7 months ago by EvilDMP

  • Cc EvilDMP added

comment:10 Changed 7 months ago by timo

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 Changed 7 months ago by EvilDMP

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 follow-up: Changed 5 months ago by anonymous

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

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

comment:14 Changed 5 months ago by timo

  • 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 Changed 5 months ago by 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?

comment:16 Changed 5 months ago by timo

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 Changed 5 months ago by 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.

comment:19 Changed 5 months ago by Tim Graham <timograham@…>

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

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.

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.