force_text doesn't force SimpleLazyObject into a string

[Piotr Maliński]

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?

[Tim Graham]

I added this test to tests/csrf_tests/tests.py, but couldn't reproduce a problem:

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?

[Piotr Maliński]

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

[Tim Graham]

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).

[Piotr Maliński]

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.

[Carl Meyer]

Yes, it seems to me that this is just a bug in force_text; it doesn't do what it says on the tin.

[Tim Graham]

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.

[Carl Meyer]

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.

[Marten Kenbeek]

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.

[Tim Graham]

​PR, but have some test failures on Python 2 that need to be investigated.

[Tim Graham]

Test failures resolved.

[Claude Paroz]

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.

[Tim Graham <timograham@…>]

In b16f84f1:

[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().

[Tim Graham <timograham@…>]

In 70be31bb:

Fixed #24836 -- Made force_text() resolve lazy objects.