[Django] #24836: force_text doesn't force SimpleLazyObject into a string
Django
-------------------------------+--------------------
Reporter: riklaunim | Owner: nobody
Type: Uncategorized | Status: new
Component: Utilities | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
With Django 1.8
https://github.com/django/django/commit/8099d33b6553c9ee7de779ae9d191a1bf22adbda
csrf_token now is a SimpleLazyObject. In my view for Facebook
authentication JavaScript made an ajax request to authenticate user and
get data back. To get new csrf_token I used:
{{{
force_text(csrf(request).get('csrf_token'))
}}}
But now json can't encode SimpleLazyObject. Should force_text force it to
string or maybe using vanilla json isn't recommended even more for
anything-Django data?
--
Ticket URL: <https://code.djangoproject.com/ticket/24836>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: riklaunim | Owner: nobody
Type: Uncategorized | Status: closed
Component: Utilities | Version: 1.8
Severity: Normal | Resolution: worksforme
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* needs_better_patch: => 0
* resolution: => worksforme
* needs_tests: => 0
* needs_docs: => 0
Comment:
I added this test to `tests/csrf_tests/tests.py`, but couldn't reproduce a
problem:
{{{
#!python
from django.test import SimpleTestCase
from django.utils.encoding import force_text
class TestContextProcessor(SimpleTestCase):
def test_force_text_on_token(self):
request = HttpRequest()
request.META['CSRF_COOKIE'] = 'test-token'
self.assertEqual(force_text(csrf(request).get('csrf_token')),
'test-token')
}}}
Could you please reopen if you can provide a snippet that reproduces a
crash?
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:1>
Django
Reporter: riklaunim | Owner: nobody
Type: Uncategorized | Status: new
Component: Utilities | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by riklaunim):
* status: closed => new
* resolution: worksforme =>
Comment:
It does compare to string but it's not a string:
{{{
>>> from django.utils.functional import SimpleLazyObject
>>> from django.utils.encoding import force_text
>>> force_text(SimpleLazyObject(lambda: 'aaa'))
<SimpleLazyObject: 'aaa'>
}}}
so:
{{{
>>> force_text(SimpleLazyObject(lambda: 'aaa')) == 'aaa'
True
>>> import json
>>> json.dumps(force_text(SimpleLazyObject(lambda: 'aaa')))
Traceback (most recent call last):
File "<console>", line 1, in <module>
File "/usr/lib/python3.4/json/__init__.py", line 230, in dumps
return _default_encoder.encode(obj)
File "/usr/lib/python3.4/json/encoder.py", line 186, in encode
return encode_basestring_ascii(o)
TypeError: first argument must be a string, not SimpleLazyObject
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:2>
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Uncategorized => Bug
* component: Utilities => CSRF
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Asked [https://github.com/django/django/pull/4085 on the original pull
request] if the author has an idea of how to fix it. If not, we could
revert the breaking patch and add a test for this case (unless the
performance benefits are deemed to be beneficial enough such that breaking
backwards compatibility is okay).
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:3>
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by riklaunim):
It may also be a point for "force_text" - how much should it force to be a
"text" instead of an object:
Similar to smart_text, except that lazy instances are resolved to
strings, rather than kept as lazy objects.
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:4>
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
Yes, it seems to me that this is just a bug in `force_text`; it doesn't do
what it says on the tin.
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:5>
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I'm not quite sure what the fix would look like, but I think
`force_text()` might need special knowledge of `SimpleLazyObject` if we
were to fix it there, which doesn't seem like a good separation of
concerns. The docstring for `SimpleLazyObject` says, "Designed for
compound objects of unknown type. For builtins or objects of known type,
use django.utils.functional.lazy."
Currently `force_text()` does a `six.text_type()` cast and
`type(force_text(obj)` -> `SimpleLazyObject` if `obj` is a
`SimpleLazyObject`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:6>
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
I think the bug is in the interaction between the fact that
`SimpleLazyObject` overrides `isinstance` checks in order to pretend to be
its wrapped type, and the performance shortcut at the top of `force_text`:
{{{
if isinstance(s, six.text_type):
return s
}}}
So in the specific situation of a `SimpleLazyObject` wrapping an object of
`six.text_type`, `force_text` does nothing at all, it doesn't cast to
`six.test_type`. (If it actually did cast, we wouldn't have this problem;
`SimpleLazyObject` does not survive a type cast).
IMO the right fix would be to make the shortcut check stricter:
{{{
if issubclass(type(s), six.text_type):
return s
}}}
Then `force_text` would actually behave as its docstring claims, and
resolve all lazy instances (including `SimpleLazyObject`) to strings.
We could make the check even stricter and require an exact class match --
`if type(s) is six.text_type` -- but then `force_text` would begin
reducing string subclasses to the base string class too. I don't have a
clear intuition that it ought to do that, so I think I'd favor the
`issubclass` version.
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:7>
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by knbk):
Specifically, `if type(s) is six.text_type` would cast a `SafeText`
instance to a plain string, so I'd say that `issubclass` is the way to go
in that case.
The problems with `json.dumps` and the like come from that fact that deep
down, on a C level, a `SimpleLazyObject` is simply not a string. I don't
believe that is something we can fix. Subclasses of `six.text_type` don't
have that problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:8>
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* component: CSRF => Utilities
Comment:
[https://github.com/django/django/pull/4713 PR], but have some test
failures on Python 2 that need to be investigated.
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:9>
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
Comment:
Test failures resolved.
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:10>
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 1.8
Severity: Release blocker | Resolution:
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
Comment:
I'm a bit scared by introducing such a low-level change in a stable
release. Another scenario would be to revert Alex change in 1.8 and
introduce this in master only.
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:11>
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 1.8
Severity: Release blocker | Resolution:
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:"b16f84f15b1344d2a3df8149565cfc8de803eb77" b16f84f1]:
{{{
#!CommitTicketReference repository=""
revision="b16f84f15b1344d2a3df8149565cfc8de803eb77"
[1.8.x] Refs #24836 -- Reverted "Simplified the lazy CSRF token
implementation in csrf context processor."
This reverts commit 8099d33b6553c9ee7de779ae9d191a1bf22adbda as it caused
a regression that cannot be solved without changing force_text() which has
a small risk of introducing regressions. This change will remain in master
along with an update to force_text().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:12>
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: closed
Component: Utilities | Version: 1.8
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"70be31bba7f8658f17235e33862319780c3dfad1" 70be31bb]:
{{{
#!CommitTicketReference repository=""
revision="70be31bba7f8658f17235e33862319780c3dfad1"
Fixed #24836 -- Made force_text() resolve lazy objects.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24836#comment:13>