[Django] #34723: `TypeError` when loading a Django app with incorrect type of `choices`
Django
-------------------------------------+-------------------------------------
Reporter: Natalia | Owner: nobody
Bidart |
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
While reviewing the PR for #31262, I was performing some local changes to
ensure that the error messages shown by the check framework were accurate
and correct, and I changed a `choices` definition to be just an int. This
change made the reloader fail with an exception:
{{{
File "/home/nessita/fellowship/projectfromrepo/ticket_31262/models.py",
line 31, in <module>
class OtherTestModelForm(forms.ModelForm):
File "/home/nessita/fellowship/django/django/forms/models.py", line 308,
in __new__
fields = fields_for_model(
^^^^^^^^^^^^^^^^^
File "/home/nessita/fellowship/django/django/forms/models.py", line 233,
in fields_for_model
formfield = f.formfield(**kwargs)
^^^^^^^^^^^^^^^^^^^^^
File
"/home/nessita/fellowship/django/django/db/models/fields/__init__.py",
line 2384, in formfield
return super().formfield(
^^^^^^^^^^^^^^^^^^
File
"/home/nessita/fellowship/django/django/db/models/fields/__init__.py",
line 2136, in formfield
return super().formfield(
^^^^^^^^^^^^^^^^^^
File
"/home/nessita/fellowship/django/django/db/models/fields/__init__.py",
line 1112, in formfield
defaults["choices"] = self.get_choices(include_blank=include_blank)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File
"/home/nessita/fellowship/django/django/db/models/fields/__init__.py",
line 1048, in get_choices
choices = list(self.choices)
^^^^^^^^^^^^^^^^^^
TypeError: 'int' object is not iterable
}}}
I believe that the code in `Field.get_choices` could be a little more
robust, so exceptions are not raised on incorrect choices type. Ideally
we'd let the check framework run and report these issues is a friendlier
manner.
--
Ticket URL: <https://code.djangoproject.com/ticket/34723>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
> I believe that the code in `Field.get_choices` could be a little more
robust, so exceptions are not raised on incorrect choices type.
I'm not sure it's worth changing, it's clearly user error that's easy to
notice, understand, and fix. Furthermore, there are other places that
assume `choices` is iterable (e.g. `flatchoices`). Are we going to add
protection to all of them? You can find many such edge cases in Django
source code, when a type is assumed. IMO, we don't need to be
overprotective. Especially since this is already covered by a system
check.
> Ideally we'd let the check framework run and report these issues is a
friendlier manner.
Autoreloader cannot run checks after each change, this would have bad
impact on developers experience. We even added an option to not run checks
even once, check out #32296.
--
Ticket URL: <https://code.djangoproject.com/ticket/34723#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Natalia Bidart | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):
Replying to [comment:1 Mariusz Felisiak]:
> > I believe that the code in `Field.get_choices` could be a little more
robust, so exceptions are not raised on incorrect choices type.
>
> I'm not sure it's worth changing, it's clearly user error that's easy to
notice, understand, and fix. Furthermore, there are other places that
assume `choices` is iterable (e.g. `flatchoices`). Are we going to add
protection to all of them? You can find many such edge cases in Django
source code, when a type is assumed. IMO, we don't need to be
overprotective. Especially since this is already covered by a system
check.
Fair enough!
> > Ideally we'd let the check framework run and report these issues is a
friendlier manner.
Right, my point is that the check framework never reports this issue (not
even on first start) because Django crashes before the checks are run.
> Autoreloader cannot run checks after each change, this would have bad
impact on developers experience. We even added an option to not run checks
even once, check out #32296.
Thanks for the pointer, though note that it seems that system checks are
run on every reload (if no special option is passed):
{{{
(djangodev) [nessita@socrates projectfromrepo]$ python -Wall manage.py
runserver 0.0.0.0:9000
Watching for file changes with StatReloader
Performing system checks...
System check identified no issues (0 silenced).
July 21, 2023 - 13:36:06
Django version 5.0.dev20230720170441, using settings
'projectfromrepo.settings'
Starting development server at http://0.0.0.0:9000/
Quit the server with CONTROL-C.
/home/nessita/fellowship/projectfromrepo/ticket_34711/models.py changed,
reloading.
Watching for file changes with StatReloader
Performing system checks...
System check identified no issues (0 silenced).
July 21, 2023 - 13:36:22
Django version 5.0.dev20230720170441, using settings
'projectfromrepo.settings'
Starting development server at http://0.0.0.0:9000/
Quit the server with CONTROL-C.
/home/nessita/fellowship/projectfromrepo/ticket_34711/models.py changed,
reloading.
Watching for file changes with StatReloader
Performing system checks...
Exception in thread django-main-thread:
Traceback (most recent call last):
File "/usr/lib/python3.11/threading.py", line 1038, in _bootstrap_inner
self.run()
File "/usr/lib/python3.11/threading.py", line 975, in run
self._target(*self._args, **self._kwargs)
File "/home/nessita/fellowship/django/django/utils/autoreload.py", line
64, in wrapper
fn(*args, **kwargs)
File
"/home/nessita/fellowship/django/django/core/management/commands/runserver.py",
line 133, in inner_run
self.check(display_num_errors=True)
File "/home/nessita/fellowship/django/django/core/management/base.py",
line 556, in check
raise SystemCheckError(msg)
django.core.management.base.SystemCheckError: SystemCheckError: System
check identified some issues:
ERRORS:
ticket_34711.OtherTestModel.asset2: (fields.E009) 'max_length' is too
small to fit the longest value in 'choices' (4 characters).
System check identified 1 issue (0 silenced).
}}}
See all the `Performing system checks` when a file is changed. I'm not
arguing the ticket resolution, I just thought it'd be nice to not have
Django crashing before even running the checks, but I agree the reported
crash is clear and easily fixable.
--
Ticket URL: <https://code.djangoproject.com/ticket/34723#comment:2>