Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#15126 closed Bug (fixed)

Misleading error in ModelForm

Reported by: ingo@… Owned by: nobody
Component: Forms Version: 1.2
Severity: Normal Keywords: modelform fields attributeerror widget subset
Cc: bmispelon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi,

setting the fields attribute in the Meta-class of a ModelForm to a tuple with only one value specified (without the trailing ',') results in a string. After expiriencing hours of bugtracking i mentioned this fact. The django code in this case doesnt exactly point out, whats causing the problem, because a string as a fields-attribute value also is an iterable object, so it iterates over every single character of this string(handling every charactar as a field) and finaly raises a AttributeError ('ΝοneType' object has no attribute 'widget') when calling the is_valid() method.
It was unnecessarily complicated to recognize the cause for this exception. After this i found, that the python-docs (http://docs.python.org/tutorial/datastructures.html#tuples-and-sequences) also mention this 'ugly' issuse to be fixed by appending a ','.
Perhaps the django-code should help people saving thier time in this case and evaluate if the fields-attribute contains a string or a valid tuple. One solution would be to convert a string to a list or to raise a more meanigful error. Another approach could be a more explicit hint in the ModelForm-Documentation(by mentioning the error it would cause not appending a ',').

Attachments (1)

django15126.testfields.diff (3.1 KB) - added by anonymous 5 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 5 years ago by russellm

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

The "single-item tuple" bug is a frequent cause of trouble, especially for newcomers to Python; however, in this case, we are in a position to raise an error.

Changed 5 years ago by anonymous

comment:2 Changed 5 years ago by bpeschier

The above patch tests both fields and exclude for the error. It also includes an extension of the forms-testsuite which tests whether or not the validation is done.

I have two comments with the patch; it would be nice if somebody would reflect on them

  1. The patch tests specifically for 'str' because I did not want to impair fancy gizmos and create a regression (the docs are not specific on what kind of list fields should be). Testing for string tests for the common mistake without restricting other iterables
  1. I was not that familiar with testing exceptions thrown by class definitions, so I would appreciate a second opinion on them :-)

comment:3 Changed 5 years ago by bpeschier

  • Has patch set

comment:4 Changed 4 years ago by jaddison

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 4 years ago by ptone

  • Easy pickings unset
  • Patch needs improvement set
  • UI/UX unset

I think the test for str is reasonable.

I think this would be better implemented in the init of ModelFormOptions

also the test should use assertRaises to actually test that the assertion is raised on instantiation, rather than fail after a class is defined, which doesn't actually test your code.

A couple minor points:

  • I believe you don't need to test for None, isinstance should suffice
  • the error should be that the option should be a iterable of strings, not a single string - list is too specific here
  • You don't need to determine the type of the errant value, as you only raise the exception for str type anyway.

comment:6 Changed 2 years ago by bmispelon

  • Cc bmispelon@… added
  • Patch needs improvement unset

I fixed up the proposed patch a little and brought it up to date, adressing some of the points made above.

I opted not to put the validation inside the ModelFormOptions.__init__, so that I would have access to the ModelForm's class in order to provide a more helpful message.

Here's the corresponding pull request: https://github.com/django/django/pull/1000

I added two extra tests (on top of the two new ones for this particular feature):

  • Make sure that a FieldError is raised when passing an inexisting field to Meta.fields.
  • Make sure that no errors are raised when passing an inexisting field to Meta.exclude.

comment:7 Changed 2 years ago by Baptiste Mispelon <bmispelon@…>

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

In f9dc1379b874f80694a80e95f0f266a3d42f7368:

Fix #15126: Better error message when passing invalid options to ModelForm.Meta.

comment:8 Changed 2 years ago by Jannis Leidel <jannis@…>

In b5c0b3d1d9328e67baa9c7cc98ac38be3d94a0ec:

Merge pull request #1000 from bmispelon/ticket-15126

Fix #15126: Better error message when passing invalid options to ModelFo...

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