id	summary	reporter	owner	description	type	status	component	version	severity	resolution	keywords	cc	stage	has_patch	needs_docs	needs_tests	needs_better_patch	easy	ui_ux
23994	ModelFormsets do not check pk fields of deleted forms in validation	Raphael Gaschignard	nobody	"
If you have a Formset, there's a way to mark certain forms as deleted through the {{{can_delete}}} parameter

In {{{BaseFormset.is_valid}}}:

{{{
        for i in range(0, self.total_form_count()):
            form = self.forms[i]
            if self.can_delete:
                if self._should_delete_form(form):
                    # This form is going to be deleted so any of its errors
                    # should not cause the entire formset to be invalid.
                    continue
            forms_valid &= form.is_valid()
}}}

So forms not marked for deleted will not trigger validation errors, which sounds good to me.
Among other things it will not validate the primary key field of a model form.

ModelFormsets give us a save method to deal with saving insteances created with formset

There's a helper function to deal with saving forms created from existing objects. We decide which
forms represent ""existing objects"" by the presence of a pk field in the form data


In {{{BaseModelFormset.save_existing_objects}}}:

{{{
          for form in self.initial_forms:
            pk_name = self._pk_field.name
            raw_pk_value = form._raw_value(pk_name)

            # clean() for different types of PK fields can sometimes return
            # the model instance, and sometimes the PK. Handle either.
            pk_value = form.fields[pk_name].clean(raw_pk_value)
            pk_value = getattr(pk_value, 'pk', pk_value)

}}}

Here we are assuming that the pk field clean is valid! When it's not then this throws a ValidationError.

so you can do 
{{{
if modelformset.is_valid():
    modelformset.save()
}}}

and have a validation error on a call to {{{save}}}. To my knowledge this is counter to the general pattern of ""Validation Errors should be caught by {{{is_valid}}}""


I think this could be fixed in two ways:

   First way would be for the clean of modelformsets to check the pk values on deleted forms , which would trigger validation errors on the {{{is_valid}}}.

   A solution I would prefer is for us to ignore deleted forms for pk values that don't exist. So a try/catch in the {{{save_existing_objects}}} method that would skip over deleted forms with no found instance in the queryset. This would prevent issues such as concurrent edition (load same form twice, delete same item on both, submit the request twice), where formsets blow up. 
"	Uncategorized	closed	Forms	1.6	Normal	duplicate	formset		Unreviewed	0	0	0	0	0	0