TypeError with Django 4.1
kbayliss opened this issue · 4 comments
kbayliss commented
Issue Summary
Using with latest Django causes a TypeError (traceback below) that prevents the pattern library from rendering patterns.
Technical details
- Python: 3.11.2
- Django: 4.1.7
Traceback
11:09:06 web.1 | Traceback (most recent call last):
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/core/handlers/exception.py", line 56, in inner
11:09:06 web.1 | response = get_response(request)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
11:09:06 web.1 | response = wrapped_callback(request, *callback_args, **callback_kwargs)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/views/decorators/cache.py", line 62, in _wrapped_view_func
11:09:06 web.1 | response = view_func(request, *args, **kwargs)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/views/generic/base.py", line 103, in view
11:09:06 web.1 | return self.dispatch(request, *args, **kwargs)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/views/generic/base.py", line 142, in dispatch
11:09:06 web.1 | return handler(request, *args, **kwargs)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/utils/decorators.py", line 46, in _wrapper
11:09:06 web.1 | return bound_method(*args, **kwargs)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/views/decorators/clickjacking.py", line 36, in wrapped_view
11:09:06 web.1 | resp = view_func(*args, **kwargs)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/pattern_library/views.py", line 95, in get
11:09:06 web.1 | rendered_pattern = render_pattern(request, pattern_template_name)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/pattern_library/utils.py", line 227, in render_pattern
11:09:06 web.1 | return render_to_string(template_name, request=request, context=context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/loader.py", line 62, in render_to_string
11:09:06 web.1 | return template.render(context, request)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/backends/django.py", line 61, in render
11:09:06 web.1 | return self.template.render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 175, in render
11:09:06 web.1 | return self._render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/test/utils.py", line 111, in instrumented_test_render
11:09:06 web.1 | return self.nodelist.render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 1005, in render
11:09:06 web.1 | return SafeString("".join([node.render_annotated(context) for node in self]))
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 1005, in <listcomp>
11:09:06 web.1 | return SafeString("".join([node.render_annotated(context) for node in self]))
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 966, in render_annotated
11:09:06 web.1 | return self.render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/pattern_library/loader_tags.py", line 38, in render
11:09:06 web.1 | return super().render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/loader_tags.py", line 157, in render
11:09:06 web.1 | return compiled_parent._render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/test/utils.py", line 111, in instrumented_test_render
11:09:06 web.1 | return self.nodelist.render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 1005, in render
11:09:06 web.1 | return SafeString("".join([node.render_annotated(context) for node in self]))
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 1005, in <listcomp>
11:09:06 web.1 | return SafeString("".join([node.render_annotated(context) for node in self]))
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 966, in render_annotated
11:09:06 web.1 | return self.render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/pattern_library/loader_tags.py", line 38, in render
11:09:06 web.1 | return super().render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/loader_tags.py", line 157, in render
11:09:06 web.1 | return compiled_parent._render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/test/utils.py", line 111, in instrumented_test_render
11:09:06 web.1 | return self.nodelist.render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 1005, in render
11:09:06 web.1 | return SafeString("".join([node.render_annotated(context) for node in self]))
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 1005, in <listcomp>
11:09:06 web.1 | return SafeString("".join([node.render_annotated(context) for node in self]))
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 966, in render_annotated
11:09:06 web.1 | return self.render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/loader_tags.py", line 63, in render
11:09:06 web.1 | result = block.nodelist.render(context)
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 1005, in render
11:09:06 web.1 | return SafeString("".join([node.render_annotated(context) for node in self]))
11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11:09:06 web.1 | TypeError: sequence item 3: expected str instance, NoneType found
alxbridge commented
This relates to commit #854e9b0 on django.template.base.NodeList.render(), where rendered child nodes are no longer converted to str before they're joined together. This means that child nodes which render as None cause the error to be thrown when included in a join.
As noted on that commit, the output of render_annotated() shouldn't need to be converted to str, because Node.render() must return a string. So something needs changing inside django-pattern-library to make sure this is enforced.
alxbridge commented
See issue #166 & related PR #188 where the same change was fixed for parts of this project.
It looks to me like this previous fix was incomplete - the value of default_html is checked against a constant for UNSPECIFIED, but this still passes if it's None, triggering the error.
@thibaudcolas What should the correct behaviour here be?
- Do an additional check that
default_htmlisn'tNone - Return an empty string if
default_htmlisNone - Identify why
Noneis sometimes supplied as thedefault_htmlvalue and change that - Something else
bcdickinson commented
Firstly, it looks like there's a mistake in the code in override_tag(). The default_html is not UNSPECIFIED condition will always be true because UNSPECIFIED is never assigned as the value of default_html. It should be the default variable of the kwarg, but it's not!
That check was originally intended to prevent falsey values being disregarded so that, for example, you could pass default_html=None or default_html=False and have it respected in the output. In retrospect that's unnecessary/bad. The return value here always need to be string, more so since the changes in Django that @alxbridge mentions above.
My proposed fix would be to:
- Fix the default value of the
default_htmlkwarg so it's actuallyUNSPECIFIED - Make sure that we only ever return strings. There are two approaches to this:
- Change the return value to
str(default_html). Quick, fixes the problem, but potentially changes the rendered output in existing pattern library implementations? It feels like it's papering over something we shouldn't allow, rather than fixing the problem, but it's the least disruptive approach. - Raise a
TypeErrorifdefault_htmlisn't a string. More "correct", but also more likely to cause existing pattern libraries to break (loudly, helpfully!). I don't know how much of a concern this is, and we could just make sure we flag the change in the changelog and don't release it as a minor/patch version.
- Change the return value to
alxbridge commented
@bcdickinson Please see the PR linked above, which aims to implement your proposed fix. I've made the handling for non-string default_html dependent on the version of Django used, so this shouldn't be a breaking change for anyone.