Opened 5 years ago

Closed 5 years ago

#12513 closed (duplicate)

UnresolvableValidationError far too greedy

Reported by: skyl Owned by: nobody
Component: Forms Version: master
Severity: Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This should be an okay modelform:

from django.contrib.auth.models import User

class FailForm(forms.ModelForm):

    class Meta:
        model = User
        fields = ('username',)

I want to see:

>>> f = FailForm({})
>>> f.is_valid()
False

Instead, expecting False, I get:

UnresolvableValidationError: {'password': [u'This field cannot be blank.']}

line ~270 django.forms.models

Change History (7)

comment:1 Changed 5 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by Honza_Kral

  • Triage Stage changed from Accepted to Design decision needed

Hi, this form will NEVER validate. If we just raised ValidationError (resulting in is_valid() == False) we would confuse both the programmer and the user.

The programmer will be confused because they would think it's the user's error and the user will get a validation error that wouldn't appear anywhere or, if we make it appear somewhere, will appear with no association to it's field (of whose existence the user doesn't have to be aware). Also the user has no way of fixing this validation error simply because this is the programmers fault - they contructed a FailForm and raising an exception is usually accepted behavior for mistakes in code.

I am changing this to Design decision needed

comment:3 Changed 5 years ago by skyl

  • Summary changed from UnresolvableValidationError far too greedy line ~ to UnresolvableValidationError far too greedy
>>> profile_form = SecretQuestionForm({})
>>> SecretQuestionForm.base_fields
{'secret_question': <django.forms.fields.CharField object at 0xa8557ec>, 'answer': <django.forms.fields.CharField object at 0
xa85588c>}
>>> profile_form.errors # I really only want errors from the included fields
{'answer': [u'This field is required.'], 'primary_contact': [u'This field cannot be null.'], 'secret_question': [u'This field
 is required.'], 'user': [u'This field cannot be null.']}
>>> form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) # I want this to be .is_valid()
>>> form.errors
{'primary_contact': [u'This field cannot be null.'], 'user': [u'This field cannot be null.']}
>>> form.is_valid() # this would be Unresolvable but for my hacks ...
False

I think that maybe a Meta option in the modelform specifying whether or not to check fields that aren't included would be prudent.

The current trunk breaks vast swaths of my 1.1 code. So, for backwards compatibility I would vote for the modelform behaving like it used to as default.

comment:4 Changed 5 years ago by Honza_Kral

I know what you mean, I just feel it's very wrong to have ModelForm validate and then fail horribly when calling .save() on it or on the model it produces. It maybe be backwards incompatible, but imo it would is a bug in 1.1.

In your case you should pass in an instance with primary_contact and user already set or set it in your form's .clean() before call to super.clean()

I leave it to core devs to decide this.

comment:5 Changed 5 years ago by skyl

I see what you're saying and have learned more about forms in investigating this. But, I think that the following should be a stable convention:

form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
if form.is_valid():
    p = form.save(commit=False)
    p.user = request.user
    p.primary_contact = somecontact
    p.save()

I process so many forms like that.

comment:6 Changed 5 years ago by simon

That's certainly the convention I've been using.

This ticket looks like it relates to the one I just filed about the admin: #12521

comment:7 Changed 5 years ago by jkocherhans

  • Resolution set to duplicate
  • Status changed from new to closed

This is essentially the same as #12521 and I've added a patch over there, so I'm closing this one. I'll use the other ticket to track this issue.

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