Opened 8 years ago

Closed 8 years ago

#23994 closed Uncategorized (duplicate)

ModelFormsets do not check pk fields of deleted forms in validation

Reported by: Raphael Gaschignard Owned by: nobody
Component: Forms Version: 1.6
Severity: Normal Keywords: formset
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (2)

comment:1 Changed 8 years ago by 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?

comment:2 Changed 8 years ago by Tim Graham

Resolution: duplicate
Status: newclosed

Please reopen if so.

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