Opened 4 years ago

Closed 4 years ago

#18829 closed Bug (fixed)

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

Reported by: facundo.olano@… Owned by: Klaas van Schelven
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 4 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 4 years ago by thikonom

Easy pickings: set
Owner: changed from nobody to thikonom
Status: newassigned

Changed 4 years ago by thikonom

Attachment: 18829.diff added

comment:2 Changed 4 years ago by thikonom

Has patch: set

comment:3 Changed 4 years ago by melinath

Summary: ModelChoiceIterator returns the queryset length whenModelChoiceIterator doesn't adjust its length to account for an empty item.
Triage Stage: UnreviewedAccepted

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 4 years ago by Klaas van Schelven

Owner: changed from thikonom to Klaas van Schelven

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 4 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: assignedclosed

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