Opened 4 years ago

Closed 2 years ago

#16663 closed Bug (needsinfo)

Model field choices should accept empty iterator

Reported by: danielnaab Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: danielnaab Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The base Field class constructor uses the following shorthand to initialize the choices for a field (line 96 of django/db/model/fields/__init__.py):

self._choices = choices or []

In Python, an empty iterator evaluates to False, so if we did:

class MyModel(models.Model):
    my_field = models.IntegerField(choices=my_list_like_object)

_choices will default to an empty list if the my_list_like_object is permanently or temporarilly empty at time of class definition. However, the documentation clearly states the intent of allowing any iterable as a choices kwarg:

Finally, note that choices can be any iterable object -- not necessarily a list or tuple. This lets you construct choices dynamically. But if you find yourself hacking choices to be dynamic, you're probably better off using a proper database table with a ForeignKey. choices is meant for static data that doesn't change much, if ever.

So I propose making a small change to the above line:

if choices == None:
    self._choices = []
else:
    self._choices = choices

Attachments (2)

16663-failing-test.patch (2.2 KB) - added by aaugustin 4 years ago.
16663-docs.patch (1.1 KB) - added by aaugustin 4 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 years ago by danielnaab

  • Cc danielnaab added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by aaugustin

I respectfully disagree with this sentence:

In Python, an empty iterator evaluates to False

To the best of my knowledge, an iterator always evaluate to True, no matter if it's empty:

>>> bool(iter(()))
True

However, at first sight, the intent here is to use None instead of [] as the default value for choices, to avoid the usual mutable-default-value issue. Hence, a strict check against None looks more appropriate.


I began working on a patch (first patch attached)... and failed horribly.

In fact, if we want to handle properly the case when an empty list is passed in the choices argument, we mustn't set self._choices = [] when the choices argument isn't passed. Otherwise we can no longer tell if (a) no choices were given or (b) a empty list was given, that will be populated later. This becomes ugly very quickly because Django uses if <field>.choices in many places. I'm afraid a lot of third-party code (like custom fields) also uses this idiom.

In particular, Django only creates get_FOO_display when <field>.choices evaluates to True when the field is initialized, which is why the test in my patch fails.


So, even if we implement a strict check (self._choices = [] if choices is None else choices), it still won't be possible to use an empty list as initial value of choices. Fixing this properly is backwards-incompatible. So I recommend not changing anything :)

Since iterators always evaluate to True, this shouldn't affect too many people. We could put a note in the docs (second patch attached, it just adds the word "non-empty" and rewraps the paragraph).

Last edited 4 years ago by aaugustin (previous) (diff)

Changed 4 years ago by aaugustin

Changed 4 years ago by aaugustin

comment:3 Changed 4 years ago by aaugustin

  • Easy pickings unset
  • Has patch set
  • Triage Stage changed from Unreviewed to Design decision needed

comment:4 Changed 4 years ago by danielnaab

I suspected there would be side effects of making the default self._choices == None instead of the empty list, which is why I proposed the simpler solution. A list-like object with length of zero is functionally identical to an empty list, so I don't see what possible side effects there could be to my suggestion (although setting choices to None is more sensible in an absolute sense).

comment:5 Changed 4 years ago by jacob

danielnaab, can you clarify the real-world usecase here? I'm having trouble figuring out if this is actually a bug or something more abstract. Thanks!

comment:6 Changed 2 years ago by aaugustin

  • Resolution set to needsinfo
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top