Improve handling of non-string values for 'override_tag's 'default_html' argument (#224)

Author: alxbridge

CHANGELOG.md

@@ -6,6 +6,10 @@
 
 - Add support for Django 5.0 ([#241](https://github.com/torchbox/django-pattern-library/pull/241))
 
+### Changed
+
+- From Django >= 4.0, calls to `Node.render()` must always return a string, but this app previously allowed non-string values to be passed in the `default_html` parameter to `override_tag`. Passing a non-string now raises a `TypeError` when using Django >= 4.0, and raises a warning for older versions ([issue #211](https://github.com/torchbox/django-pattern-library/issues/211)).
+
 ## [1.1.0](https://github.com/torchbox/django-pattern-library/releases/tag/v1.1.0) - 2023-10-25
 
 ### Added

pattern_library/monkey_utils.py

@@ -1,5 +1,7 @@
+import inspect
 import logging
-from typing import Optional
+import typing
+import warnings
 
 import django
 from django.template.library import SimpleNode
@@ -11,7 +13,9 @@
 
 
 def override_tag(
-    register: django.template.Library, name: str, default_html: Optional[str] = None
+    register: django.template.Library,
+    name: str,
+    default_html: typing.Optional[typing.Any] = UNSPECIFIED,
 ):
     """
     An utility that helps you override original tags for use in your pattern library.
@@ -79,6 +83,23 @@ def node_render(context):
                     # See https://github.com/torchbox/django-pattern-library/issues/166.
                     return str(result)
                 elif default_html is not UNSPECIFIED:
+                    # Ensure default_html is a string.
+                    if not isinstance(default_html, str):
+                        # Save the caller for the override tag in case it's needed for error reporting.
+                        trace = inspect.stack()[1]
+                        if django.VERSION < (4, 0):
+                            warnings.warn(
+                                "default_html argument to override_tag should be a string to ensure compatibility "
+                                'with Django >= 4.0 (line %s in "%s")'
+                                % (trace.lineno, trace.filename),
+                                Warning,
+                            )
+                        else:
+                            raise TypeError(
+                                'default_html argument to override_tag must be a string (line %s in "%s")'
+                                % (trace.lineno, trace.filename)
+                            )
+
                     # Render provided default;
                     # if no stub data supplied.
                     return default_html

tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.html.fail

@@ -0,0 +1,5 @@
+{% load test_tags_invalid %}
+
+MARMA{% default_html_tag_invalid empty_string %}LADE01
+MARMA{% default_html_tag_invalid none %}LADE02
+MARMA{% default_html_tag_invalid dict %}LADE03

tests/templatetags/test_tags_invalid.py

@@ -0,0 +1,15 @@
+from django import template
+
+from pattern_library.monkey_utils import override_tag
+
+register = template.Library()
+
+
+@register.simple_tag()
+def default_html_tag_invalid(arg=None):
+    "Just raise an exception, never do anything"
+    raise Exception("default_tag raised an exception")
+
+
+# Test overriding tag with a default_html that's not valid in Django >= 4.0
+override_tag(register, "default_html_tag_invalid", default_html=[1, 2, 3])

tests/tests/test_tags.py

@@ -1,4 +1,9 @@
-from django.test import SimpleTestCase
+from unittest.mock import patch
+
+from django.shortcuts import render
+from django.test import RequestFactory, SimpleTestCase
+
+from pattern_library import get_pattern_context_var_name
 
 from .utils import reverse
 
@@ -43,3 +48,54 @@ def test_falsey_default_html_overide(self):
         self.assertContains(response, "POTATO1")
         self.assertContains(response, "POTANoneTO2")
         self.assertContains(response, "POTA0TO3")
+
+
+class TagsTestFailCase(SimpleTestCase):
+    def test_bad_default_html_warning(self):
+        """
+        Test that the library raises a warning when passing a non-string `default_html` argument to `override_tag`
+        in Django < 4.0
+        """
+        with patch("django.VERSION", (3, 2, 0, "final", 0)):
+            with self.assertWarns(Warning) as cm:
+                template_name = (
+                    "patterns/atoms/tags_test_atom/invalid_tags_test_atom.html.fail"
+                )
+                request = RequestFactory().get("/")
+
+                # Rendering the template with a non-string `default_html` argument will cause Django >= 4 to raise
+                # a `TypeError`, which we need to catch and ignore in order to check that the warning is raised
+                try:
+                    render(
+                        request,
+                        template_name,
+                        context={get_pattern_context_var_name(): True},
+                    )
+                except TypeError:
+                    pass
+
+            self.assertIn(
+                "default_html argument to override_tag should be a string to ensure compatibility with Django",
+                str(cm.warnings[0]),
+            )
+
+    def test_bad_default_html_error(self):
+        """
+        Test that the library raises a TypeError when passing a non-string `default_html` argument to `override_tag`
+        in Django >= 4.0
+        """
+        with patch("django.VERSION", (4, 2, 0, "final", 0)):
+            with self.assertRaises(TypeError) as cm:
+                template_name = (
+                    "patterns/atoms/tags_test_atom/invalid_tags_test_atom.html.fail"
+                )
+                request = RequestFactory().get("/")
+                render(
+                    request,
+                    template_name,
+                    context={get_pattern_context_var_name(): True},
+                )
+            self.assertIn(
+                "default_html argument to override_tag must be a string",
+                str(cm.exception),
+            )