Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#12901 closed (fixed)

Forms _get_validations_exclusions is still not backwards compatible

Reported by: Chris Beaven Owned by: nobody
Component: Forms Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

In my user registration form, I have a field named password, but the modelform's Meta has fields = ('first_name', 'last_name', 'email').

Submitting the form causes a "This field cannot be blank." validation error for the password field.

The current logic doesn't actually refer to those fields, but to the fields dictionary (which will also contain any user-generated fields).

Attachments (4)

12901.diff (664 bytes) - added by Chris Beaven 7 years ago.
12901-against-12454.diff (2.0 KB) - added by Honza Král 7 years ago.
test case and fix
12901-against-12454-2.diff (3.8 KB) - added by Honza Král 7 years ago.
12901-against-12557.diff (615 bytes) - added by ammarr 7 years ago.

Download all attachments as: .zip

Change History (21)

Changed 7 years ago by Chris Beaven

Attachment: 12901.diff added

comment:1 Changed 7 years ago by Chris Beaven

Needs tests: set

comment:2 Changed 7 years ago by Chris Beaven

The original code was introduced in [12206]

comment:3 Changed 7 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

comment:4 Changed 7 years ago by Honza Král

Resolution: worksforme
Status: newclosed
Version: 1.1SVN

Closing this as worksforme - I cannot replicate it and when I look at UserCreationForm in contrib.auth (which isusing the same approach) at

http://code.djangoproject.com/browser/django/trunk/django/contrib/auth/forms.py#L23

It works and is actually tested at

http://code.djangoproject.com/browser/django/trunk/django/contrib/auth/tests/forms.py#L72

Could you please provide a test case or more detailed example of what you are doing?

comment:5 Changed 7 years ago by Chris Beaven

Resolution: worksforme
Status: closedreopened

Hi Honza,

UserCreationForm doesn't use password, it uses password1 which skirts around this backwards incompatibility.
I'll see if I can get around to a test case, but it's pretty obvious (for those that know the code and previous behaviour) that this is an error if you look at the patch.

Changed 7 years ago by Honza Král

Attachment: 12901-against-12454.diff added

test case and fix

comment:6 Changed 7 years ago by Honza Král

Needs tests: unset

I see what you had a problem with, managed to produce a test case and a fix. Sorry for the initial confusion.

comment:7 Changed 7 years ago by Chris Beaven

Triage Stage: AcceptedReady for checkin

comment:8 Changed 7 years ago by jkocherhans

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

This fixes the original problem, but it also breaks unique validation. Here's the test that breaks http://code.djangoproject.com/browser/django/trunk/tests/modeltests/model_formsets/models.py?rev=12369#L935

I'm don't have a strong opinion here. By replacing a default form field should you be opting out of unique validation on that field as well? Seems like either way would be a surprise.

comment:9 Changed 7 years ago by Chris Beaven

I'm not really "replacing a default form field" because of the meta option, but I understand what you're saying.

IMO, if the fields meta option is used, I think that it's pretty explicit that those are the *only* fields should be want tied to the model.

Changed 7 years ago by Honza Král

Attachment: 12901-against-12454-2.diff added

comment:10 Changed 7 years ago by jkocherhans

Resolution: fixed
Status: reopenedclosed

(In [12496]) Fixed #12901. Exclude overridden form fields from model field validation. Thanks, Honza Král.

comment:11 Changed 7 years ago by ammarr

From the test case, it works with the following code:

class IncompleteCategoryForm(ModelForm): 
    url = forms.CharField(required=False) 	 
    class Meta: 
        fields = ('name', 'slug') 
        model = Category 

but not this:

class IncompleteCategoryForm(ModelForm): 
    url = forms.CharField(required=False) 	 
    class Meta: 
        # removed field tuple
        model = Category 

Is this intentional? IMO, if you declare a field manually, then it should be excluded from validation in any case (regardless of whether the field tuple is present or not).

comment:12 in reply to:  11 ; Changed 7 years ago by Honza Král

Replying to ammarr:

From the test case, it works with the following code:

class IncompleteCategoryForm(ModelForm): 
    url = forms.CharField(required=False) 	 
    class Meta: 
        fields = ('name', 'slug') 
        model = Category 

but not this:

class IncompleteCategoryForm(ModelForm): 
    url = forms.CharField(required=False) 	 
    class Meta: 
        # removed field tuple
        model = Category 

Is this intentional? IMO, if you declare a field manually, then it should be excluded from validation in any case (regardless of whether the field tuple is present or not).

No. The key difference here is that if you omit the field in Meta.fields, it doesn't get populated on the model so validation doesn't make sense. If you, however, just override the field definition, the model still get populated with this field and validation should then be run.

comment:13 Changed 7 years ago by ammarr

I see (and agree with) what you're saying, but the resultant behavior is not backwards compatible. The code snippet without the fields tuple validates in 1.1.1.

comment:14 in reply to:  12 Changed 7 years ago by ammarr

Replying to Honza_Kral:

No. The key difference here is that if you omit the field in Meta.fields, it doesn't get populated on the model so validation doesn't make sense. If you, however, just override the field definition, the model still get populated with this field and validation should then be run.

In addition to the previous message, the only way to exclude a field from model validation is to provide the entire fields tuple, exclude that one field, and then manually declare it. This behavior isn't symmetrical though (declare a field manually, and then just exclude it using the exclude tuple doesn't work). Perhaps this check should be added:

elif self._meta.exclude and field in self._meta.exclude:
exclude.append(f.name) 

Changed 7 years ago by ammarr

Attachment: 12901-against-12557.diff added

comment:15 Changed 7 years ago by ammarr

Resolution: fixed
Status: closedreopened

comment:16 Changed 7 years ago by jkocherhans

Resolution: fixed
Status: reopenedclosed

(In [12590]) Fixed #12901. Again. Model validation will not be performed on excluded fields that were overridden in the form. Thanks, ammarr.

comment:17 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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