ModelFormsets do not check pk fields of deleted forms in validation

[Raphael Gaschignard]

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.

[Tim Graham]

I think ignoring deleted forms for pks that don't exist is preferable as it doesn't seem the user would be able to correct any validation errors if we went with the first solution. However, this seems like it could be a duplicate of #14877 -- can you confirm there is still a problem on 1.7?

[Tim Graham]

Please reopen if so.