Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#11905 closed (fixed)

modelform_factory returns a broken form when given wrong value for fields

Reported by: ben Owned by: kmtracey
Component: Forms Version: 1.1
Severity: Keywords: modelform_factory, modelform, fields
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description (last modified by russellm)

modelform_factory is breaking where you ask for an invalid fields option (even if the field exists):

Here's a doctest that demonstrates:

>>> from django.forms.models import modelform_factory, form_for_fields
>>> from django.contrib.auth.models import User
>>> Form = modelform_factory(User, fields=['id', 'username'])
>>> Form.base_fields
{'id': None, 'username': <django.forms.fields.CharField object at 0xd4e4ad0>}
>>> print Form()
------------------------------------------------------------
Traceback (most recent call last):
  ...
    if self.field.label is None:
AttributeError: 'NoneType' object has no attribute 'label'

Likewise you can specify fields that don't even exist on the model:

>>> Form = modelform_factory(User, fields=['no-field', 'username'])
>>> Form.base_fields
{'no-field': None, 'username': <django.forms.fields.CharField object at 0xd4eec10>}
>>> print Form()
------------------------------------------------------------
Traceback (most recent call last):
  ...
    if self.field.label is None:
AttributeError: 'NoneType' object has no attribute 'label'

This appears to be due to logic in form_for_fields (line 102 in my checkout) in the first case, and fields_for_model (line 170) in the second.

I'd propose some error handling be put in place to give meaningfull exception in either of these cases rather than return a broken form to the user.

I'm happy to submit a patch (I can't do it from here as I'm at work and don't have direct access to django's svn repo) but I can do it from home.

Ben

Attachments (1)

11905.diff (3.5 KB) - added by copelco 5 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 6 years ago by ben

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Sorry screwed up the formatting, try:

>>> from django.forms.models import modelform_factory, form_for_fields
>>> from django.contrib.auth.models import User
>>> Form = modelform_factory(User, fields=['id', 'username'])
>>> Form.base_fields
{'id': None, 'username': <django.forms.fields.CharField object at 0xd4e4ad0>}
>>> print Form()
Traceback (most recent call last):
    ....
    if self.field.label is None:
AttributeError: 'NoneType' object has no attribute 'label'

>>> Form = modelform_factory(User, fields=['no-field', 'username'])
>>> Form.base_fields
{'no-field': None, 'username': <django.forms.fields.CharField object at 0xd4eec10>}
>>> print Form()
Traceback (most recent call last):
    ....
AttributeError: 'NoneType' object has no attribute 'label'

comment:2 Changed 5 years ago by Natim

I wish this could have a patch as mentionned.

I get the same error on the SVN version and I don't know what is wrong.
http://paste.pocoo.org/show/146130/

Thank you.

comment:3 Changed 5 years ago by Johnny82

I have the same error

Caught an exception while rendering: 'NoneType' object has no attribute 'label'


it's occurs on Admin page when i want to use editor to add data to a Model
if comment such models field, and remove it from Admin class for that model, all works fine

techspec = models.ManyToManyField('TechSpec', through = 'ModelTechSpec')

comment:4 Changed 5 years ago by russellm

  • Description modified (diff)
  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 5 years ago by ubernostrum

  • milestone changed from 1.2 to 1.3

While better error reporting is always nice, it's not release-critical and can be bumped to 1.3.

comment:6 Changed 5 years ago by anonymous

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:7 Changed 5 years ago by copelco

  • Owner changed from anonymous to copelco
  • Status changed from assigned to new

comment:8 Changed 5 years ago by copelco

Checking for unknown fields in fields_for_model will break some existing functionality:

  1. django.contrib.auth uses a custom form for user creation that passes invalid fields (password1 and password2) to fields_for_model.
  1. If a declarative field is listed in ModelForm._meta.fields (as described here), but is not an actual field of the Model, an error will be raised with this change when it previously succeeded. Current tests (like this one) will fail with this issue.

To test this, I decided to raise a FieldError in fields_for_model when a passed field is not part of the related model. The diff can be found here. I'd be happy to attempt a fix at both the auth and declarative field issues, but I'd like to get some feedback first. Any thoughts?

comment:9 Changed 5 years ago by copelco

  • Has patch set

After talking with Karen, I decided to look for a different spot to validate the field list. I ended up modifying ModelFormMetaclass to cross check the fields generated from fields_for_model() against the form's declared fields. fields_for_model returns a mapping of field_name->field, but field will be None if field_name is not found on the specified model. If the missing field name is not found in the declared field list, then the field is considered invalid and will raise a FieldError. With this change, the exception is raised when creating the factory, rather than when rending the form. Patch with tests is attached.

comment:10 Changed 5 years ago by copelco

  • Owner changed from copelco to nobody

Changed 5 years ago by copelco

comment:11 Changed 5 years ago by copelco

  • Owner changed from nobody to kmtracey

comment:12 Changed 5 years ago by kmtracey

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

(In [13739]) Fixed #11905: Raise an error on model form creation if a non-existent field was listed in fields. Thanks ben and copelco.

comment:13 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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