[Django] #27058: {% for %} tag unpacking: less strict checks
Django
--------------------------------------+--------------------
Reporter: sergei-maertens | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
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
`TypeError`s.
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/27058>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: sergei-maertens | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
+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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27058#comment:1>
Django
Reporter: sergei-maertens | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
{{{#!python
@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')})
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27058#comment:2>
Django
Reporter: sergei-maertens | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27058#comment:3>
Django
Reporter: sergei-maertens | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by sergei-maertens):
Replying to [comment:2 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.
> {{{#!python
> @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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27058#comment:4>
Django
Reporter: sergei-maertens | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27058#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: sergei-maertens | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/7091 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27058#comment:6>
Django
Reporter: sergei-maertens | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27058#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"937d752d3deabebe60dfbe9ff9823772730f336a" 937d752d]:
{{{
#!CommitTicketReference repository=""
revision="937d752d3deabebe60dfbe9ff9823772730f336a"
Fixed #27058 -- Reallowed the {% for %} tag to unpack any iterable.
Thanks Sergei Maertens for the report and patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27058#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"020ba4bf91aec9b13f415ace5cd538958ce3b608" 020ba4b]:
{{{
#!CommitTicketReference repository=""
revision="020ba4bf91aec9b13f415ace5cd538958ce3b608"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27058#comment:9>