Opened 3 years ago

Closed 2 years ago

#18829 closed Bug (fixed)

ModelChoiceIterator doesn't adjust its length to account for an empty item.

Reported by: facundo.olano@… Owned by: vanschelven
Component: Forms Version: 1.4
Severity: Normal Keywords: ModelChoiceIterator ModelForm
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In forms/models.py, the ModelChoiceIterator defines an __iter__ method which yields an empty entry if empty_label is not None, but the __len__ method always returns the length of the queryset, no matter if the empty choice is present.

def __iter__(self):
    if self.field.empty_label is not None:
        yield (u"", self.field.empty_label)
    if self.field.cache_choices:
        if self.field.choice_cache is None:
            self.field.choice_cache = [
                self.choice(obj) for obj in self.queryset.all()
            ]
        for choice in self.field.choice_cache:
            yield choice
    else:
        for obj in self.queryset.all():
            yield self.choice(obj)

def __len__(self):
    return len(self.queryset)

This can lead to erratic behavior when iterating. For example, the following template code wont hold the expected output, because the forloop.last will be True before for the second to last item, causing the semicolon to render on the last line and not in the previous one.

{% for choice_id, choice_name in my_form.my_field.field.choices %}
    {{ choice_id }} - {{ choice_name }} {% if not forloop.last %};{% endif %}
{% endfor %}

This would produce an output like:

 - -----;
1 - One;
2 - Two
3 - Three;

Attachments (1)

18829.diff (455 bytes) - added by thikonom 3 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 3 years ago by thikonom

  • Easy pickings set
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to thikonom
  • Patch needs improvement unset
  • Status changed from new to assigned

Changed 3 years ago by thikonom

comment:2 Changed 3 years ago by thikonom

  • Has patch set

comment:3 Changed 2 years ago by melinath

  • Summary changed from ModelChoiceIterator returns the queryset length when to ModelChoiceIterator doesn't adjust its length to account for an empty item.
  • Triage Stage changed from Unreviewed to Accepted

The issue is valid. Personally, I don't like using booleans as ints, but apparently it's part of the spec, not an implementation detail [1], so I can't complain super much. Pretty sure this needs tests, though.

[1] http://stackoverflow.com/questions/2764017/is-false-0-and-true-1-in-python-an-implementation-detail-or-is-it-guarante

comment:4 Changed 2 years ago by vanschelven

  • Owner changed from thikonom to vanschelven

I've added a test and bike-shedded the implementation; available as pull request on github

https://github.com/django/django/pull/798

comment:5 Changed 2 years ago by Claude Paroz <claude@…>

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

In 1c11ee63459c4362affbd6a20f2ddf9c2ecd8dce:

Fixed #18829 -- Fixed ModelChoiceIterator length

Thanks facundo.olano at gmail.com for the report and thikonom for
the initial patch.

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