Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#27058 closed Cleanup/optimization (fixed)

Reallow the {% for %} tag to unpack any iterable

Reported by: Sergei Maertens Owned by: nobody
Component: Template system Version: 1.10
Severity: Normal Keywords:
Cc: 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

In 1.10 there are now checks done before unpacking in a for loop:

{% for foo, bar in items %}
    ...
{% endfor %}

Where it is verified that everey item in items has len(item) == 2 in this particular case.

However, the check is implemented so that len(item) is taken only if the item is an instance of tuple or list, while custom objects may also implement def __len__(self):. Because the custom object does not subclass from tuple or list, it's length is set to 1, leading to ValueErrors.

I have a working patch at https://github.com/django/django/compare/master...sergei-maertens:duck-type-forloop-unpack-len?expand=1, tests are passing. The check no longer checks if the item is an instance of tuple/list, but just tries to get the len(item) and handles TypeErrors.

The only case I could imagine was where the items would be an iterable of strings, but even then it turns out the check is correct (as it is technically possible to unpack strings).

Change History (9)

comment:1 Changed 3 years ago by Claude Paroz

Triage Stage: UnreviewedAccepted

+1 for one less isinstance if possible. The tricky thing might be to handle strings with backwards compatibility.

Did you spot a regression in 1.10?

comment:2 Changed 3 years ago by Tim Graham

The ticket for the deprecation and error message is #13408. That code isn't new in 1.10 (rather in 1.8) so there's no regression as far as I understand. I removed the code comment about "To complete this deprecation" earlier today (ba749f8f87dd60b20aeaefb84ee182f192746ebf) as that deprecation did complete in 1.10.

Here's a test case that passes on master but fails with your proposed patch. As to whether or not that's a case to protect against, I guess it's probably better to assume that the programmer won't make a mistake like that if it prevents other use cases.

@setup({'for-tag-unpack15': '{% for x,y in items %}{{ x }}:{{ y }}/{% endfor %}'})
def test_for_tag_unpack15(self):
    with self.assertRaisesMessage(ValueError, 'Need 2 values to unpack in for loop; got 1.'):
        self.engine.render_to_string('for-tag-unpack15', {'items': ('ab', 'ac')})

comment:3 Changed 3 years ago by Sergei Maertens

I'm not sure that I completely understand - are you saying this protection was introduced in 1.8 already? Because that would surprise me, since the package tested against Django 1.7, 1.8 and 1.9 before and only started failing against 1.10.

comment:4 in reply to:  2 Changed 3 years ago by Sergei Maertens

Replying to timgraham:

Here's a test case that passes on master but fails with your proposed patch. As to whether or not that's a case to protect against, I guess it's probably better to assume that the programmer won't make a mistake like that if it prevents other use cases.

@setup({'for-tag-unpack15': '{% for x,y in items %}{{ x }}:{{ y }}/{% endfor %}'})
def test_for_tag_unpack15(self):
    with self.assertRaisesMessage(ValueError, 'Need 2 values to unpack in for loop; got 1.'):
        self.engine.render_to_string('for-tag-unpack15', {'items': ('ab', 'ac')})

Ah yes, this was the test case I was trying to come up with because I _knew_ there was something with checking string lengths. I think I would classify this test case as incorrect, since in pure python for x, y in ('ab', 'ac'): is correct as well. I'd call this a logical programmer error that's the responsability of the end-user.

comment:5 Changed 3 years ago by Tim Graham

My understanding is that the change in 1.10 was to convert a deprecation warning to an exception: 3bbebd06adc36f31877a9c0af6c20c5b5a71a900. Maybe you missing those warnings in earlier versions? Or else the issue is something different, in which case, could you provide a test case and bisect to find the commit where the behavior changed?

comment:6 Changed 3 years ago by Tim Graham

Has patch: set
Summary: {% for %} tag unpacking: less strict checksReallow the {% for %} tag to unpack any iterable

PR. Even if this did raise warnings in older versions, I guess it should be backported to 1.10 if the ValueError is now prohibiting the use case.

comment:7 Changed 3 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

comment:8 Changed 3 years ago by GitHub <noreply@…>

Resolution: fixed
Status: newclosed

In 937d752d:

Fixed #27058 -- Reallowed the {% for %} tag to unpack any iterable.

Thanks Sergei Maertens for the report and patch.

comment:9 Changed 3 years ago by Tim Graham <timograham@…>

In 020ba4b:

[1.10.x] Fixed #27058 -- Reallowed the {% for %} tag to unpack any iterable.

Thanks Sergei Maertens for the report and patch.

Backport of 937d752d3deabebe60dfbe9ff9823772730f336a from master

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