From d81b778b16c0b571c238c70124bfb713fa0b2bc4 Mon Sep 17 00:00:00 2001 From: tschilling Date: Mon, 27 May 2024 19:14:51 -0500 Subject: [PATCH 1/2] Limit the cases for E001 to likely scenarios Only when the user is customizing both the show toolbar callback setting and the URLs aren't installed will the underlying NoReverseMatch error occur. --- debug_toolbar/apps.py | 32 +++++++++++++- docs/changes.rst | 4 ++ docs/configuration.rst | 10 +++++ tests/test_checks.py | 99 +++++++++++++++++++++++++++--------------- 4 files changed, 109 insertions(+), 36 deletions(-) diff --git a/debug_toolbar/apps.py b/debug_toolbar/apps.py index a2e977d84..a49875bac 100644 --- a/debug_toolbar/apps.py +++ b/debug_toolbar/apps.py @@ -5,10 +5,12 @@ from django.conf import settings from django.core.checks import Error, Warning, register from django.middleware.gzip import GZipMiddleware +from django.urls import NoReverseMatch, reverse from django.utils.module_loading import import_string from django.utils.translation import gettext_lazy as _ -from debug_toolbar import settings as dt_settings +from debug_toolbar import APP_NAME, settings as dt_settings +from debug_toolbar.settings import CONFIG_DEFAULTS class DebugToolbarConfig(AppConfig): @@ -213,7 +215,33 @@ def debug_toolbar_installed_when_running_tests_check(app_configs, **kwargs): """ Check that the toolbar is not being used when tests are running """ - if not settings.DEBUG and dt_settings.get_config()["IS_RUNNING_TESTS"]: + # Check if show toolbar callback has changed + show_toolbar_changed = ( + dt_settings.get_config()["SHOW_TOOLBAR_CALLBACK"] + != CONFIG_DEFAULTS["SHOW_TOOLBAR_CALLBACK"] + ) + try: + # Check if the toolbar's urls are installed + reverse(f"{APP_NAME}:render_panel") + toolbar_urls_installed = True + except NoReverseMatch: + toolbar_urls_installed = False + + # If the user is using the default SHOW_TOOLBAR_CALLBACK, + # then the middleware will respect the change to settings.DEBUG. + # However, if the user has changed the callback to: + # DEBUG_TOOLBAR_CONFIG = {"SHOW_TOOLBAR_CALLBACK": lambda request: DEBUG} + # where DEBUG is not settings.DEBUG, then it won't pick up that Django' + # test runner has changed the value for settings.DEBUG, and the middleware + # will inject the toolbar, while the URLs aren't configured leading to a + # NoReverseMatch error. + likely_error_setup = show_toolbar_changed and not toolbar_urls_installed + + if ( + not settings.DEBUG + and dt_settings.get_config()["IS_RUNNING_TESTS"] + and likely_error_setup + ): return [ Error( "The Django Debug Toolbar can't be used with tests", diff --git a/docs/changes.rst b/docs/changes.rst index a0215d09c..8f4951f95 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -4,6 +4,10 @@ Change log Pending ------- +* Limit ``E001`` check to likely error cases when the + ``SHOW_TOOLBAR_CALLBACK`` has changed, but the toolbar's URL + paths aren't installed. + 4.4.2 (2024-05-27) ------------------ diff --git a/docs/configuration.rst b/docs/configuration.rst index 04694aceb..df6337adf 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -152,6 +152,16 @@ Toolbar options implication is that it is possible to execute arbitrary SQL through the SQL panel when the ``SECRET_KEY`` value is leaked somehow. + .. warning:: + + Do not use + ``DEBUG_TOOLBAR_CONFIG = {"SHOW_TOOLBAR_CALLBACK": lambda request: DEBUG}`` + in your project's settings.py file. The toolbar expects to use + ``django.conf.settings.DEBUG``. Using your project's setting's ``DEBUG`` + is likely to cause unexpected when running your tests. This is because + Django automatically sets ``settings.DEBUG = False``, but your project's + setting's ``DEBUG`` will still be set to ``True``. + .. _OBSERVE_REQUEST_CALLBACK: * ``OBSERVE_REQUEST_CALLBACK`` diff --git a/tests/test_checks.py b/tests/test_checks.py index 827886db1..27db92a9d 100644 --- a/tests/test_checks.py +++ b/tests/test_checks.py @@ -1,9 +1,9 @@ from unittest.mock import patch -from django.core.checks import Error, Warning, run_checks +from django.core.checks import Warning, run_checks from django.test import SimpleTestCase, override_settings +from django.urls import NoReverseMatch -from debug_toolbar import settings as dt_settings from debug_toolbar.apps import debug_toolbar_installed_when_running_tests_check @@ -239,39 +239,70 @@ def test_check_w007_invalid(self, mocked_guess_type): ], ) - def test_debug_toolbar_installed_when_running_tests(self): - with self.settings(DEBUG=True): - # Update the config options because self.settings() - # would require redefining DEBUG_TOOLBAR_CONFIG entirely. - dt_settings.get_config()["IS_RUNNING_TESTS"] = True - errors = debug_toolbar_installed_when_running_tests_check(None) - self.assertEqual(len(errors), 0) - - dt_settings.get_config()["IS_RUNNING_TESTS"] = False - errors = debug_toolbar_installed_when_running_tests_check(None) - self.assertEqual(len(errors), 0) - with self.settings(DEBUG=False): - dt_settings.get_config()["IS_RUNNING_TESTS"] = False - errors = debug_toolbar_installed_when_running_tests_check(None) - self.assertEqual(len(errors), 0) + @patch("debug_toolbar.apps.reverse") + def test_debug_toolbar_installed_when_running_tests(self, reverse): + params = [ + { + "debug": True, + "running_tests": True, + "show_callback_changed": True, + "urls_installed": False, + "errors": False, + }, + { + "debug": False, + "running_tests": False, + "show_callback_changed": True, + "urls_installed": False, + "errors": False, + }, + { + "debug": False, + "running_tests": True, + "show_callback_changed": False, + "urls_installed": False, + "errors": False, + }, + { + "debug": False, + "running_tests": True, + "show_callback_changed": True, + "urls_installed": True, + "errors": False, + }, + { + "debug": False, + "running_tests": True, + "show_callback_changed": True, + "urls_installed": False, + "errors": True, + }, + ] + for config in params: + with self.subTest(**config): + config_setting = { + "RENDER_PANELS": False, + "IS_RUNNING_TESTS": config["running_tests"], + "SHOW_TOOLBAR_CALLBACK": ( + (lambda *args: True) + if config["show_callback_changed"] + else "debug_toolbar.middleware.show_toolbar" + ), + } + if config["urls_installed"]: + reverse.side_effect = lambda *args: None + else: + reverse.side_effect = NoReverseMatch() - dt_settings.get_config()["IS_RUNNING_TESTS"] = True - errors = debug_toolbar_installed_when_running_tests_check(None) - self.assertEqual( - errors, - [ - Error( - "The Django Debug Toolbar can't be used with tests", - hint="Django changes the DEBUG setting to False when running " - "tests. By default the Django Debug Toolbar is installed because " - "DEBUG is set to True. For most cases, you need to avoid installing " - "the toolbar when running tests. If you feel this check is in error, " - "you can set `DEBUG_TOOLBAR_CONFIG['IS_RUNNING_TESTS'] = False` to " - "bypass this check.", - id="debug_toolbar.E001", - ) - ], - ) + with self.settings( + DEBUG=config["debug"], DEBUG_TOOLBAR_CONFIG=config_setting + ): + errors = debug_toolbar_installed_when_running_tests_check(None) + if config["errors"]: + self.assertEqual(len(errors), 1) + self.assertEqual(errors[0].id, "debug_toolbar.E001") + else: + self.assertEqual(len(errors), 0) @override_settings( DEBUG_TOOLBAR_CONFIG={ From c6bd6026a3ec9762bac11e87ca32855f3cdb559f Mon Sep 17 00:00:00 2001 From: tschilling Date: Mon, 27 May 2024 19:48:13 -0500 Subject: [PATCH 2/2] Introduce debug_toolbar_urls to simplify installation While this isn't a huge improvement, it provides a hook for us to more easily introduce library level controls for when urls are installed. It also eliminates the user from having to write an if statement for the most basic of installations. --- debug_toolbar/toolbar.py | 28 +++++++++++++++++++++++++++- docs/changes.rst | 2 ++ docs/installation.rst | 16 +++++++++------- example/urls.py | 11 +++-------- tests/test_toolbar.py | 17 +++++++++++++++++ 5 files changed, 58 insertions(+), 16 deletions(-) create mode 100644 tests/test_toolbar.py diff --git a/debug_toolbar/toolbar.py b/debug_toolbar/toolbar.py index fc07543b5..04502ab09 100644 --- a/debug_toolbar/toolbar.py +++ b/debug_toolbar/toolbar.py @@ -2,16 +2,18 @@ The main DebugToolbar class that loads and renders the Toolbar. """ +import re import uuid from collections import OrderedDict from functools import lru_cache from django.apps import apps +from django.conf import settings from django.core.exceptions import ImproperlyConfigured from django.dispatch import Signal from django.template import TemplateSyntaxError from django.template.loader import render_to_string -from django.urls import path, resolve +from django.urls import include, path, re_path, resolve from django.urls.exceptions import Resolver404 from django.utils.module_loading import import_string from django.utils.translation import get_language, override as lang_override @@ -186,3 +188,27 @@ def observe_request(request): Determine whether to update the toolbar from a client side request. """ return True + + +def debug_toolbar_urls(prefix="__debug__"): + """ + Return a URL pattern for serving toolbar in debug mode. + + from django.conf import settings + from debug_toolbar.toolbar import debug_toolbar_urls + + urlpatterns = [ + # ... the rest of your URLconf goes here ... + ] + debug_toolbar_urls() + """ + if not prefix: + raise ImproperlyConfigured("Empty urls prefix not permitted") + elif not settings.DEBUG: + # No-op if not in debug mode. + return [] + return [ + re_path( + r"^%s/" % re.escape(prefix.lstrip("/")), + include("debug_toolbar.urls"), + ), + ] diff --git a/docs/changes.rst b/docs/changes.rst index 8f4951f95..012b4ce3f 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -7,6 +7,8 @@ Pending * Limit ``E001`` check to likely error cases when the ``SHOW_TOOLBAR_CALLBACK`` has changed, but the toolbar's URL paths aren't installed. +* Introduce helper function ``debug_toolbar_urls`` to + simplify installation. 4.4.2 (2024-05-27) ------------------ diff --git a/docs/installation.rst b/docs/installation.rst index 657450fac..9200504b7 100644 --- a/docs/installation.rst +++ b/docs/installation.rst @@ -95,14 +95,14 @@ Add django-debug-toolbar's URLs to your project's URLconf: .. code-block:: python from django.urls import include, path + from debug_toolbar.toolbar import debug_toolbar_urls urlpatterns = [ - # ... - path("__debug__/", include("debug_toolbar.urls")), - ] + # ... the rest of your URLconf goes here ... + ] + debug_toolbar_urls() -This example uses the ``__debug__`` prefix, but you can use any prefix that -doesn't clash with your application's URLs. +By default this uses the ``__debug__`` prefix for the paths, but you can +use any prefix that doesn't clash with your application's URLs. 5. Add the Middleware @@ -180,11 +180,13 @@ You should also modify your URLconf file: .. code-block:: python + from django.conf import settings + from debug_toolbar.toolbar import debug_toolbar_urls + if not settings.TESTING: urlpatterns = [ *urlpatterns, - path("__debug__/", include("debug_toolbar.urls")), - ] + ] + debug_toolbar_urls() Alternatively, you can check out the :ref:`IS_RUNNING_TESTS ` option. diff --git a/example/urls.py b/example/urls.py index 7569a57f9..b64aa1c37 100644 --- a/example/urls.py +++ b/example/urls.py @@ -1,8 +1,8 @@ -from django.conf import settings from django.contrib import admin -from django.urls import include, path +from django.urls import path from django.views.generic import TemplateView +from debug_toolbar.toolbar import debug_toolbar_urls from example.views import increment urlpatterns = [ @@ -34,9 +34,4 @@ ), path("admin/", admin.site.urls), path("ajax/increment", increment, name="ajax_increment"), -] - -if settings.ENABLE_DEBUG_TOOLBAR: - urlpatterns += [ - path("__debug__/", include("debug_toolbar.urls")), - ] +] + debug_toolbar_urls() diff --git a/tests/test_toolbar.py b/tests/test_toolbar.py new file mode 100644 index 000000000..7155d3fcb --- /dev/null +++ b/tests/test_toolbar.py @@ -0,0 +1,17 @@ +from django.core.exceptions import ImproperlyConfigured + +from debug_toolbar.toolbar import debug_toolbar_urls +from tests.base import BaseTestCase + + +class DebugToolbarUrlsTestCase(BaseTestCase): + def test_empty_prefix_errors(self): + with self.assertRaises(ImproperlyConfigured): + debug_toolbar_urls(prefix="") + + def test_empty_when_debug_is_false(self): + self.assertEqual(debug_toolbar_urls(), []) + + def test_has_path(self): + with self.settings(DEBUG=True): + self.assertEqual(len(debug_toolbar_urls()), 1)