Opened 9 years ago
Closed 9 years ago
#24836 closed Bug (fixed)
force_text doesn't force SimpleLazyObject into a string
Reported by: | Piotr Maliński | Owned by: | nobody |
---|---|---|---|
Component: | Utilities | Version: | 1.8 |
Severity: | Release blocker | 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
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?
Change History (13)
comment:1 by , 9 years ago
Resolution: | → worksforme |
---|---|
Status: | new → closed |
comment:2 by , 9 years ago
Resolution: | worksforme |
---|---|
Status: | closed → new |
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
comment:3 by , 9 years ago
Component: | Utilities → CSRF |
---|---|
Severity: | Normal → Release blocker |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
Asked 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).
comment:4 by , 9 years ago
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.
comment:5 by , 9 years ago
Yes, it seems to me that this is just a bug in force_text
; it doesn't do what it says on the tin.
comment:6 by , 9 years ago
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
.
comment:7 by , 9 years ago
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.
comment:8 by , 9 years ago
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.
comment:9 by , 9 years ago
Component: | CSRF → Utilities |
---|---|
Has patch: | set |
Patch needs improvement: | set |
PR, but have some test failures on Python 2 that need to be investigated.
comment:11 by , 9 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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.
I added this test to
tests/csrf_tests/tests.py
, but couldn't reproduce a problem:Could you please reopen if you can provide a snippet that reproduces a crash?