calling self.errors in a form in a formset breaks deletion

[Carles Pina i Estany]

Hi,

Yesterday I found that calling self.errors in the constructor of a Form
that is in an InlineFormSet (or any formset I guess): the "delete" of a
form doesn't work anymore. I wonder if this is a bug in Django (for this
case, if a new bug is needed, I'm happy to open it... or be referred to
an existing bug) or if I missed some bit of the documentation.

Summary:
BaseForm.errors does:
https://github.com/django/django/blob/master/django/forms/forms.py#L169

The DELETE field in self.fields is added in the form.fields via BaseFormSet after calling the constructor of the form:
https://github.com/django/django/blob/master/django/forms/formsets.py#L175

So calling self.errors cleans the data but DELETE doesn't end up in
self.clean_fields (because it's not in self.fields). When DELETE is
added as a field self.clean_fields is not updated.

(I'm talking about this:
https://github.com/django/django/blob/master/django/forms/formsets.py#L390
being executed too late; or self.errors being called too early)

The full story is here:
https://groups.google.com/forum/#!topic/django-users/wO9H0cNcLfI

Thank you very much,

--
Carles Pina i Estany
https://carles.pina.cat

[Carlton Gibson]

Bon dia Carles.

I'm inclined to say that calling self.errors in __init__ is at least holding it wrong.

I'm not sure what if anything we might do about that. DRF's serializers have all kinds of

"you have to call is_valid() before accessing .data" type assertions, but they only catch the

obvious cases: we still get reports of unusual combinations not working on the tracker.

So I think trying to enforce usage is probably a loosing battle.

Without thinking about it, I don't know exactly where I'd add the field on the way out but

coming in I'd look to see if allow_error was present in clean()

https://docs.djangoproject.com/en/3.0/ref/forms/validation/#validating-fields-with-clean

I hope that helps.

Kind Regards,

Carlton

[Carles Pina i Estany]

Hi,

On Jun/19/2020, Carlton Gibson wrote:
> Bon dia Carles.

I'm very impressed! :-)

> I'm inclined to say that calling self.errors in __init__ is at least *holding
> it wrong*.

:-) it almost worked! (the only known issue was for the BaseForm not
deleting the model of my ModelForm... for what I can see though, it
might have other problems!).

> I'm not sure what if anything we might do about that. DRF's serializers
> have all kinds of
> "you have to call is_valid() before accessing .data" type assertions, but

I'm not familiar with the internals of DRF (Django REST Framework I
understand) :-( yet :-)

In Django I see that is_valid() is only "return is_bound and not self.errors:"
https://github.com/django/django/blob/master/django/forms/forms.py#L177

So I didn't think that to access .data I should call is_valid(). Anyway,
what I want to do is to avoid using .data at all.

> they only catch the
> obvious cases: we still get reports of unusual combinations not working on
> the tracker.
> So I think trying to enforce usage is probably a loosing battle.
>
> Without thinking about it, I don't know exactly where I'd add the field on
> the way out but
> coming in I'd look to see if allow_error was present in clean()
> https://docs.djangoproject.com/en/3.0/ref/forms/validation/#validating-fields-with-clean

Yes, I did this (if there is a BooleanField enabled I ignore some checks
in the clean)

My main problem was: ModelSet has a form that is not valid. When the
BaseForm (ModelForm in my case) is constructed again (because is_valid() is
False so the view reloads the form again showing errors to the user): if
it's not valid for a specific reason I need to add a BooleanField (to do
what you said in the clean(): ignore one of the validations).

Yesterday I implemented: in the __init__() get the data using .data (I
don't like it though) and execute my logic again to determine the cause
of the error. My first idea was to access .errors and look for a
specific error (then the problem is that the FormSet was not deleting
the Model if the user clicked on deleting a form from the formset)

For what I can see in Django code: If BaseFormSet.add_fields() called
form.full_clean() (only needed if add_fields had added a new field): I
think that if the user deletes a form (so f{prefix}-DELETE='on' is
received) the Model that the Form holds (in my case it is a ModelForm)
would have been deleted as expected. I don't know if this is going to
backfire in some places or some use cases.

(BaseFormSet.add_fields for reference: https://github.com/django/django/blob/master/django/forms/formsets.py#L373 )

Sorry if this sounds like a brain twist!

If using .errors in the form __init__ is unsupported: no problem!
Thanks to this email I have an idea on how to avoid it!
I'll check if the documentation says that and I've missed it.

If using .errors in the form __init__ is supported I might look a bit
more into this.

Cheers,

> --
> You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/3ed8045c-52b8-4647-9fe6-c0326aaf923do%40googlegroups.com.

[Carlton Gibson]

Hi Carles.

Supported/unsupported isn't always clear cut... if you bend it far it far enough maybe it's just undefined what the behaviour is.

Certainly, adding fields based on whether the field data is itself if invalid is going to lead into sticky territory. When you add FormSets to the mix all the more so.

My reference to DRF was just that there are attempts to narrow down (incorrect) calling patterns there but, given issues still come up, it's clear that it's not feasible to cover all cases. 

I think this is probably a usage issue really, "how do I...?", rather than a development question... (Thought: would adding the field in form_invalid() before re-rendering, then checking for that in clean() save the whole __init__() issue...? — I don't know: you have to play :)

Sorry I don't have an instant answer.

Bon cap de setmana!

Carlton

[David Smith]

Hi Carles,

Hope you are well.

You mentioned you are using crispy-forms. :-)

Have you seen this part of the docs, it allows you to update forms 'on the go'.

I've not really played with this but it sounds similar to what you need?

https://django-crispy-forms.readthedocs.io/en/latest/dynamic_layouts.html