From df67c89c3be189d02301dd88416ec4671d8055ca Mon Sep 17 00:00:00 2001 From: Daniel Harding Date: Tue, 9 May 2023 15:24:44 +0300 Subject: [PATCH 1/4] Remove unnecessary return statement --- debug_toolbar/panels/sql/tracking.py | 1 - 1 file changed, 1 deletion(-) diff --git a/debug_toolbar/panels/sql/tracking.py b/debug_toolbar/panels/sql/tracking.py index 565d9244b..a3ec02a3e 100644 --- a/debug_toolbar/panels/sql/tracking.py +++ b/debug_toolbar/panels/sql/tracking.py @@ -63,7 +63,6 @@ def chunked_cursor(*args, **kwargs): connection.cursor = cursor connection.chunked_cursor = chunked_cursor - return cursor def unwrap_cursor(connection): From 217238bced43f9e3045b3197ffc94d5985967c65 Mon Sep 17 00:00:00 2001 From: Daniel Harding Date: Wed, 10 May 2023 14:36:33 +0300 Subject: [PATCH 2/4] Don't try to undo DatabaseWrapper monkey patching Prior to this commit, the SQLPanel would monkey patch the .cursor() and .chunked_cursor() methods of each DatabaseWrapper connection in .enable_instrumentation(), and then undo the monkey patch in .disable_instrumentation(). However, for the same reasons as a6b65a7c509589a635d5c3889ee8ea7a1e10b707, stop trying to undo the monkey patching in .disable_instrumentation() and simply use a .djdt_logger attribute on the DatabaseWrapper to determine if the cursors should be wrapped. --- debug_toolbar/panels/sql/panel.py | 7 ++--- debug_toolbar/panels/sql/tracking.py | 39 +++++++++++----------------- 2 files changed, 19 insertions(+), 27 deletions(-) diff --git a/debug_toolbar/panels/sql/panel.py b/debug_toolbar/panels/sql/panel.py index 90e2ba812..c8576e16f 100644 --- a/debug_toolbar/panels/sql/panel.py +++ b/debug_toolbar/panels/sql/panel.py @@ -10,7 +10,7 @@ from debug_toolbar.panels import Panel from debug_toolbar.panels.sql import views from debug_toolbar.panels.sql.forms import SQLSelectForm -from debug_toolbar.panels.sql.tracking import unwrap_cursor, wrap_cursor +from debug_toolbar.panels.sql.tracking import wrap_cursor from debug_toolbar.panels.sql.utils import contrasting_color_generator, reformat_sql from debug_toolbar.utils import render_stacktrace @@ -190,11 +190,12 @@ def get_urls(cls): def enable_instrumentation(self): # This is thread-safe because database connections are thread-local. for connection in connections.all(): - wrap_cursor(connection, self) + wrap_cursor(connection) + connection._djdt_logger = self def disable_instrumentation(self): for connection in connections.all(): - unwrap_cursor(connection) + connection._djdt_logger = None def generate_stats(self, request, response): colors = contrasting_color_generator() diff --git a/debug_toolbar/panels/sql/tracking.py b/debug_toolbar/panels/sql/tracking.py index a3ec02a3e..e1740dada 100644 --- a/debug_toolbar/panels/sql/tracking.py +++ b/debug_toolbar/panels/sql/tracking.py @@ -3,6 +3,7 @@ import json from time import time +import django.test.testcases from django.utils.encoding import force_str from debug_toolbar import settings as dt_settings @@ -31,10 +32,15 @@ class SQLQueryTriggered(Exception): """Thrown when template panel triggers a query""" -def wrap_cursor(connection, panel): +def wrap_cursor(connection): + # If running a Django SimpleTestCase, which isn't allowed to access the database, + # don't perform any monkey patching. + if isinstance(connection.cursor, django.test.testcases._DatabaseFailure): + return if not hasattr(connection, "_djdt_cursor"): connection._djdt_cursor = connection.cursor connection._djdt_chunked_cursor = connection.chunked_cursor + connection._djdt_logger = None def cursor(*args, **kwargs): # Per the DB API cursor() does not accept any arguments. There's @@ -43,48 +49,33 @@ def cursor(*args, **kwargs): # See: # https://github.com/jazzband/django-debug-toolbar/pull/615 # https://github.com/jazzband/django-debug-toolbar/pull/896 + logger = connection._djdt_logger + cursor = connection._djdt_cursor(*args, **kwargs) + if logger is None: + return cursor if allow_sql.get(): wrapper = NormalCursorWrapper else: wrapper = ExceptionCursorWrapper - return wrapper(connection._djdt_cursor(*args, **kwargs), connection, panel) + return wrapper(cursor, connection, logger) def chunked_cursor(*args, **kwargs): # prevent double wrapping # solves https://github.com/jazzband/django-debug-toolbar/issues/1239 + logger = connection._djdt_logger cursor = connection._djdt_chunked_cursor(*args, **kwargs) - if not isinstance(cursor, BaseCursorWrapper): + if logger is not None and not isinstance(cursor, BaseCursorWrapper): if allow_sql.get(): wrapper = NormalCursorWrapper else: wrapper = ExceptionCursorWrapper - return wrapper(cursor, connection, panel) + return wrapper(cursor, connection, logger) return cursor connection.cursor = cursor connection.chunked_cursor = chunked_cursor -def unwrap_cursor(connection): - if hasattr(connection, "_djdt_cursor"): - # Sometimes the cursor()/chunked_cursor() methods of the DatabaseWrapper - # instance are already monkey patched before wrap_cursor() is called. (In - # particular, Django's SimpleTestCase monkey patches those methods for any - # disallowed databases to raise an exception if they are accessed.) Thus only - # delete our monkey patch if the method we saved is the same as the class - # method. Otherwise, restore the prior monkey patch from our saved method. - if connection._djdt_cursor == connection.__class__.cursor: - del connection.cursor - else: - connection.cursor = connection._djdt_cursor - del connection._djdt_cursor - if connection._djdt_chunked_cursor == connection.__class__.chunked_cursor: - del connection.chunked_cursor - else: - connection.chunked_cursor = connection._djdt_chunked_cursor - del connection._djdt_chunked_cursor - - class BaseCursorWrapper: pass From 32ab3630e642ec4482f386ec2d241214ee1be07f Mon Sep 17 00:00:00 2001 From: Daniel Harding Date: Tue, 9 May 2023 15:32:20 +0300 Subject: [PATCH 3/4] Fix psycopg3 tests Several tests (such as SQLPanelTestCase.test_cursor_wrapper_singleton) are written to ensure that only a single cursor wrapper is instantiated during the test. However, this fails when using Django's psycopg3 backend, since the .last_executed_query() call in NormalCursorWrapper._record() ends up creating an additional cursor (via [1]). To avoid this wrapping this additional cursor, set the DatabaseWrapper's ._djdt_logger attribute to None before calling .last_executed_query() and restore it when finished. This will cause the monkey-patched DatabaseWrapper .cursor() and .chunked_cursor() methods to return the original cursor without wrapping during that call. [1] https://github.com/django/django/blob/4.2.1/django/db/backends/postgresql/psycopg_any.py#L21 --- debug_toolbar/panels/sql/tracking.py | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/debug_toolbar/panels/sql/tracking.py b/debug_toolbar/panels/sql/tracking.py index e1740dada..4add1fce7 100644 --- a/debug_toolbar/panels/sql/tracking.py +++ b/debug_toolbar/panels/sql/tracking.py @@ -144,6 +144,21 @@ def _decode(self, param): except UnicodeDecodeError: return "(encoded string)" + def _last_executed_query(self, sql, params): + """Get the last executed query from the connection.""" + # Django's psycopg3 backend creates a new cursor in its implementation of the + # .last_executed_query() method. To avoid wrapping that cursor, temporarily set + # the DatabaseWrapper's ._djdt_logger attribute to None. This will cause the + # monkey-patched .cursor() and .chunked_cursor() methods to skip the wrapping + # process during the .last_executed_query() call. + self.db._djdt_logger = None + try: + return self.db.ops.last_executed_query( + self.cursor, sql, self._quote_params(params) + ) + finally: + self.db._djdt_logger = self.logger + def _record(self, method, sql, params): alias = self.db.alias vendor = self.db.vendor @@ -176,9 +191,7 @@ def _record(self, method, sql, params): params = { "vendor": vendor, "alias": alias, - "sql": self.db.ops.last_executed_query( - self.cursor, sql, self._quote_params(params) - ), + "sql": self._last_executed_query(sql, params), "duration": duration, "raw_sql": sql, "params": _params, @@ -186,7 +199,9 @@ def _record(self, method, sql, params): "stacktrace": get_stack_trace(skip=2), "start_time": start_time, "stop_time": stop_time, - "is_slow": duration > dt_settings.get_config()["SQL_WARNING_THRESHOLD"], + "is_slow": ( + duration > dt_settings.get_config()["SQL_WARNING_THRESHOLD"] + ), "is_select": sql.lower().strip().startswith("select"), "template_info": template_info, } From e7575e87dc9e2d2560b87d6fd5a123b9398cbd34 Mon Sep 17 00:00:00 2001 From: Tim Schilling Date: Tue, 9 May 2023 20:28:58 -0500 Subject: [PATCH 4/4] Inherit from django.db.backends.utils.CursorWrapper This switches the Debug Toolbar cursor wrappers to inherit from the Django class django.db.backends.utils.CursorWrapper. This reduces some of the code we need. --- debug_toolbar/panels/sql/tracking.py | 46 +++++++++------------------- 1 file changed, 14 insertions(+), 32 deletions(-) diff --git a/debug_toolbar/panels/sql/tracking.py b/debug_toolbar/panels/sql/tracking.py index 4add1fce7..425e4e5cc 100644 --- a/debug_toolbar/panels/sql/tracking.py +++ b/debug_toolbar/panels/sql/tracking.py @@ -4,6 +4,7 @@ from time import time import django.test.testcases +from django.db.backends.utils import CursorWrapper from django.utils.encoding import force_str from debug_toolbar import settings as dt_settings @@ -57,54 +58,47 @@ def cursor(*args, **kwargs): wrapper = NormalCursorWrapper else: wrapper = ExceptionCursorWrapper - return wrapper(cursor, connection, logger) + return wrapper(cursor.cursor, connection, logger) def chunked_cursor(*args, **kwargs): # prevent double wrapping # solves https://github.com/jazzband/django-debug-toolbar/issues/1239 logger = connection._djdt_logger cursor = connection._djdt_chunked_cursor(*args, **kwargs) - if logger is not None and not isinstance(cursor, BaseCursorWrapper): + if logger is not None and not isinstance(cursor, DjDTCursorWrapper): if allow_sql.get(): wrapper = NormalCursorWrapper else: wrapper = ExceptionCursorWrapper - return wrapper(cursor, connection, logger) + return wrapper(cursor.cursor, connection, logger) return cursor connection.cursor = cursor connection.chunked_cursor = chunked_cursor -class BaseCursorWrapper: - pass +class DjDTCursorWrapper(CursorWrapper): + def __init__(self, cursor, db, logger): + super().__init__(cursor, db) + # logger must implement a ``record`` method + self.logger = logger -class ExceptionCursorWrapper(BaseCursorWrapper): +class ExceptionCursorWrapper(DjDTCursorWrapper): """ Wraps a cursor and raises an exception on any operation. Used in Templates panel. """ - def __init__(self, cursor, db, logger): - pass - def __getattr__(self, attr): raise SQLQueryTriggered() -class NormalCursorWrapper(BaseCursorWrapper): +class NormalCursorWrapper(DjDTCursorWrapper): """ Wraps a cursor and logs queries. """ - def __init__(self, cursor, db, logger): - self.cursor = cursor - # Instance of a BaseDatabaseWrapper subclass - self.db = db - # logger must implement a ``record`` method - self.logger = logger - def _quote_expr(self, element): if isinstance(element, str): return "'%s'" % element.replace("'", "''") @@ -246,22 +240,10 @@ def _record(self, method, sql, params): self.logger.record(**params) def callproc(self, procname, params=None): - return self._record(self.cursor.callproc, procname, params) + return self._record(super().callproc, procname, params) def execute(self, sql, params=None): - return self._record(self.cursor.execute, sql, params) + return self._record(super().execute, sql, params) def executemany(self, sql, param_list): - return self._record(self.cursor.executemany, sql, param_list) - - def __getattr__(self, attr): - return getattr(self.cursor, attr) - - def __iter__(self): - return iter(self.cursor) - - def __enter__(self): - return self - - def __exit__(self, type, value, traceback): - self.close() + return self._record(super().executemany, sql, param_list)