Opened 13 months ago

Closed 13 months ago

Last modified 13 months ago

#34899 closed Bug (fixed)

Model Field.choices callable support is not actually lazy

Reported by: Adam Johnson Owned by: Nick Pope
Component: Database layer (models, ORM) Version: 5.0
Severity: Release blocker Keywords: choices, callable, lazy
Cc: Natalia Bidart, Nick Pope Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

#24561 added support for callables to model Field.choices, with the docs declaring that using a callable “can be particularly handy” when you need I/O like a database query. Whilst the model Field supports this by only lazily calling choices, the formfield() method ends up iterating over choices, leading to the callable being run at import time with a ModelField definition.

Here’s a minimal example (source):

from django import forms
from django.db import models

ready = False


def animals():
    if not ready:
        raise RuntimeError("Not ready to load animals")

    return [
        (1, "Aardvark"),
        (2, "Banana"),
    ]


class User(models.Model):
    spirit_animal = models.IntegerField(choices=animals)


class UserForm(forms.ModelForm):
    class Meta:
        model = User
        fields = ["spirit_animal"]


ready = True

On Django 5.0a1 it fails with:

$ ./manage.py shell
Traceback (most recent call last):
  File "/..././manage.py", line 21, in <module>
    main()
  ...
  File "/.../example/models.py", line 21, in <module>
    class UserForm(forms.ModelForm):
  File "/.../.venv/lib/python3.11/site-packages/django/forms/models.py", line 309, in __new__
    fields = fields_for_model(
             ^^^^^^^^^^^^^^^^^
  File "/.../.venv/lib/python3.11/site-packages/django/forms/models.py", line 234, in fields_for_model
    formfield = f.formfield(**kwargs)
                ^^^^^^^^^^^^^^^^^^^^^
  File "/.../.venv/lib/python3.11/site-packages/django/db/models/fields/__init__.py", line 2142, in formfield
    return super().formfield(
           ^^^^^^^^^^^^^^^^^^
  File "/.../.venv/lib/python3.11/site-packages/django/db/models/fields/__init__.py", line 1118, in formfield
    defaults["choices"] = self.get_choices(include_blank=include_blank)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/.../.venv/lib/python3.11/site-packages/django/db/models/fields/__init__.py", line 1054, in get_choices
    choices = list(self.choices)
              ^^^^^^^^^^^^^^^^^^
  File "/.../.venv/lib/python3.11/site-packages/django/utils/choices.py", line 17, in __iter__
    yield from normalize_choices(self.func())
                                 ^^^^^^^^^^^
  File "/.../example/models.py", line 9, in animals
    raise RuntimeError("Not ready to load animals")
RuntimeError: Not ready to load animals

I encountered this bug whilst trying to update django-countries to support Django 5.0 (https://github.com/SmileyChris/django-countries/pull/438). Its sorted, translated list of countries is exactly what the callable choices feature is intended for.

Change History (10)

comment:1 by Natalia Bidart, 13 months ago

Cc: ngpope added
Triage Stage: UnreviewedAccepted

Accepting following the reproducer, I'll be able to take a look after `DjangoCon. Adding Nick as cc as well.

comment:2 by Natalia Bidart, 13 months ago

Cc: Nick Pope added; ngpope removed

comment:3 by Apoorva Pandey, 13 months ago

Here https://github.com/django/django/blob/f6629ee2c986d3bf59b4c1b3058f370a00bdc573/django/db/models/fields/__init__.py#L1115
if we add a check for CallableChoiceIterator and set defaults["choices"] as the choices callable instead of calling the get_choices method directly, that seems to fix this issue.

if isinstance(self.choices, CallableChoiceIterator):
    defaults["choices"] = self.choices
else:
    include_blank = self.blank or not (
        self.has_default() or "initial" in kwargs
    )
    defaults["choices"] = self.get_choices(include_blank=include_blank)

The only caveat with this solution is that the blank choice has to be set within the choices callable and would no longer be handled by get_choices as it relies on checking if a blank choice exists as one of the available choices which itself requires calling the choices callable.

I have tested this using the example posted in the issue description and it works. If this solution looks good I would be happy to land a PR.

comment:4 by Nick Pope, 13 months ago

Has patch: set
Keywords: choices callable lazy added
Owner: changed from nobody to Nick Pope
Patch needs improvement: set
Status: newassigned

comment:5 by Nick Pope, 13 months ago

Patch needs improvement: unset

comment:6 by Natalia Bidart, 13 months ago

Triage Stage: AcceptedReady for checkin

comment:7 by Natalia <124304+nessita@…>, 13 months ago

In 74afcee:

Refs #34899 -- Extracted Field.flatchoices to flatten_choices helper function.

Co-authored-by: Natalia Bidart <124304+nessita@…>

comment:8 by Natalia <124304+nessita@…>, 13 months ago

Resolution: fixed
Status: assignedclosed

In 171f91d:

Fixed #34899 -- Added blank choice to forms' callable choices lazily.

comment:9 by Natalia <124304+nessita@…>, 13 months ago

In bbe90f3:

[5.0.x] Refs #34899 -- Extracted Field.flatchoices to flatten_choices helper function.

Co-authored-by: Natalia Bidart <124304+nessita@…>

Backport of 74afcee234f8be989623ccc7c28b9fb97fb548f0 from main

comment:10 by Natalia <124304+nessita@…>, 13 months ago

In cc5901f:

[5.0.x] Fixed #34899 -- Added blank choice to forms' callable choices lazily.

Backport of 171f91d9ef5177850c2f12b26dd732785f6ac034 from main

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