From b6a00c6a10b92e84348cf875b0e08d8ae0d7a709 Mon Sep 17 00:00:00 2001 From: Tim Schilling Date: Wed, 2 Aug 2023 08:48:57 -0500 Subject: [PATCH 1/4] Patch CursorWrapper dynamically to allow multiple base classes. When the debug SQL logs are enabled, the wrapper class is CursorDebugWrapper not CursorWrapper. Since we have inspections based on that specific class they are removing the CursorDebugWrapper causing the SQL logs to not appear. This attempts to dynamically patch the CursorWrapper or CursorDebugWrapper with the functionality we need. This doesn't do a full regression test, but it may be possible to get it to work with: TEST_ARGS='--debug-sql' make test Which causes the current set of tests to fail since they are keyed to CursorWrapper. --- debug_toolbar/panels/sql/tracking.py | 30 +++++++++++------ docs/changes.rst | 2 ++ tests/panels/test_sql.py | 48 +++++++++++++++++----------- 3 files changed, 52 insertions(+), 28 deletions(-) diff --git a/debug_toolbar/panels/sql/tracking.py b/debug_toolbar/panels/sql/tracking.py index 14b2cb7ab..b90bb4dd1 100644 --- a/debug_toolbar/panels/sql/tracking.py +++ b/debug_toolbar/panels/sql/tracking.py @@ -5,7 +5,6 @@ from time import perf_counter import django.test.testcases -from django.db.backends.utils import CursorWrapper from django.utils.encoding import force_str from debug_toolbar.utils import get_stack_trace, get_template_info @@ -60,34 +59,45 @@ def cursor(*args, **kwargs): cursor = connection._djdt_cursor(*args, **kwargs) if logger is None: return cursor - wrapper = NormalCursorWrapper if allow_sql.get() else ExceptionCursorWrapper - return wrapper(cursor.cursor, connection, logger) + mixin = NormalCursorWrapper if allow_sql.get() else ExceptionCursorWrapper + return patch_cursor_wrapper_with_mixin(cursor.__class__, mixin)( + 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, DjDTCursorWrapper): + if logger is not None and not isinstance(cursor, DjDTCursorWrapperMixin): if allow_sql.get(): - wrapper = NormalCursorWrapper + mixin = NormalCursorWrapper else: - wrapper = ExceptionCursorWrapper - return wrapper(cursor.cursor, connection, logger) + mixin = ExceptionCursorWrapper + return patch_cursor_wrapper_with_mixin(cursor.__class__, mixin)( + cursor.cursor, connection, logger + ) return cursor connection.cursor = cursor connection.chunked_cursor = chunked_cursor -class DjDTCursorWrapper(CursorWrapper): +def patch_cursor_wrapper_with_mixin(base_wrapper, mixin): + class DjDTCursorWrapper(mixin, base_wrapper): + pass + + return DjDTCursorWrapper + + +class DjDTCursorWrapperMixin: def __init__(self, cursor, db, logger): super().__init__(cursor, db) # logger must implement a ``record`` method self.logger = logger -class ExceptionCursorWrapper(DjDTCursorWrapper): +class ExceptionCursorWrapper(DjDTCursorWrapperMixin): """ Wraps a cursor and raises an exception on any operation. Used in Templates panel. @@ -97,7 +107,7 @@ def __getattr__(self, attr): raise SQLQueryTriggered() -class NormalCursorWrapper(DjDTCursorWrapper): +class NormalCursorWrapper(DjDTCursorWrapperMixin): """ Wraps a cursor and logs queries. """ diff --git a/docs/changes.rst b/docs/changes.rst index e06d4c615..5b7aaf1c1 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -15,6 +15,8 @@ Pending resolving to the wrong content type. * Fixed SQL statement recording under PostgreSQL for queries encoded as byte strings. +* Patch the ``CursorWrapper`` class with a mixin class to support multiple + base wrapper classes. 4.1.0 (2023-05-15) ------------------ diff --git a/tests/panels/test_sql.py b/tests/panels/test_sql.py index d6b31ca2b..5c59097ff 100644 --- a/tests/panels/test_sql.py +++ b/tests/panels/test_sql.py @@ -2,12 +2,13 @@ import datetime import os import unittest -from unittest.mock import patch +from unittest.mock import call, patch import django from asgiref.sync import sync_to_async from django.contrib.auth.models import User from django.db import connection, transaction +from django.db.backends.utils import CursorWrapper from django.db.models import Count from django.db.utils import DatabaseError from django.shortcuts import render @@ -68,39 +69,44 @@ def test_recording_chunked_cursor(self): self.assertEqual(len(self.panel._queries), 1) @patch( - "debug_toolbar.panels.sql.tracking.NormalCursorWrapper", - wraps=sql_tracking.NormalCursorWrapper, + "debug_toolbar.panels.sql.tracking.patch_cursor_wrapper_with_mixin", + wraps=sql_tracking.patch_cursor_wrapper_with_mixin, ) - def test_cursor_wrapper_singleton(self, mock_wrapper): + def test_cursor_wrapper_singleton(self, mock_patch_cursor_wrapper): sql_call() - # ensure that cursor wrapping is applied only once - self.assertEqual(mock_wrapper.call_count, 1) + mock_patch_cursor_wrapper.assert_called_once_with( + CursorWrapper, sql_tracking.NormalCursorWrapper + ) @patch( - "debug_toolbar.panels.sql.tracking.NormalCursorWrapper", - wraps=sql_tracking.NormalCursorWrapper, + "debug_toolbar.panels.sql.tracking.patch_cursor_wrapper_with_mixin", + wraps=sql_tracking.patch_cursor_wrapper_with_mixin, ) - def test_chunked_cursor_wrapper_singleton(self, mock_wrapper): + def test_chunked_cursor_wrapper_singleton(self, mock_patch_cursor_wrapper): sql_call(use_iterator=True) # ensure that cursor wrapping is applied only once - self.assertEqual(mock_wrapper.call_count, 1) + mock_patch_cursor_wrapper.assert_called_once_with( + CursorWrapper, sql_tracking.NormalCursorWrapper + ) @patch( - "debug_toolbar.panels.sql.tracking.NormalCursorWrapper", - wraps=sql_tracking.NormalCursorWrapper, + "debug_toolbar.panels.sql.tracking.patch_cursor_wrapper_with_mixin", + wraps=sql_tracking.patch_cursor_wrapper_with_mixin, ) - async def test_cursor_wrapper_async(self, mock_wrapper): + async def test_cursor_wrapper_async(self, mock_patch_cursor_wrapper): await sync_to_async(sql_call)() - self.assertEqual(mock_wrapper.call_count, 1) + mock_patch_cursor_wrapper.assert_called_once_with( + CursorWrapper, sql_tracking.NormalCursorWrapper + ) @patch( - "debug_toolbar.panels.sql.tracking.NormalCursorWrapper", - wraps=sql_tracking.NormalCursorWrapper, + "debug_toolbar.panels.sql.tracking.patch_cursor_wrapper_with_mixin", + wraps=sql_tracking.patch_cursor_wrapper_with_mixin, ) - async def test_cursor_wrapper_asyncio_ctx(self, mock_wrapper): + async def test_cursor_wrapper_asyncio_ctx(self, mock_patch_cursor_wrapper): self.assertTrue(sql_tracking.allow_sql.get()) await sync_to_async(sql_call)() @@ -116,7 +122,13 @@ async def task(): await asyncio.create_task(task()) # Because it was called in another context, it should not have affected ours self.assertTrue(sql_tracking.allow_sql.get()) - self.assertEqual(mock_wrapper.call_count, 1) + self.assertEqual( + mock_patch_cursor_wrapper.call_args_list, + [ + call(CursorWrapper, sql_tracking.NormalCursorWrapper), + call(CursorWrapper, sql_tracking.ExceptionCursorWrapper), + ], + ) def test_generate_server_timing(self): self.assertEqual(len(self.panel._queries), 0) From f692468a15cd799ac85207410d2347a051c4235d Mon Sep 17 00:00:00 2001 From: Tim Schilling Date: Thu, 3 Aug 2023 07:30:24 -0500 Subject: [PATCH 2/4] Allow mixin as a valid word in our docs. --- docs/spelling_wordlist.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index 2ab01758c..7a15d9aeb 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -30,6 +30,7 @@ memcache memcached middleware middlewares +mixin mousedown mouseup multi From 1ade554ab421facf5be38f084b4babccb4101f51 Mon Sep 17 00:00:00 2001 From: Tim Schilling Date: Thu, 3 Aug 2023 07:55:50 -0500 Subject: [PATCH 3/4] Support tests with --debug-sql --- tests/panels/test_sql.py | 45 ++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/tests/panels/test_sql.py b/tests/panels/test_sql.py index 5c59097ff..7c0bece78 100644 --- a/tests/panels/test_sql.py +++ b/tests/panels/test_sql.py @@ -8,7 +8,7 @@ from asgiref.sync import sync_to_async from django.contrib.auth.models import User from django.db import connection, transaction -from django.db.backends.utils import CursorWrapper +from django.db.backends.utils import CursorDebugWrapper, CursorWrapper from django.db.models import Count from django.db.utils import DatabaseError from django.shortcuts import render @@ -75,8 +75,13 @@ def test_recording_chunked_cursor(self): def test_cursor_wrapper_singleton(self, mock_patch_cursor_wrapper): sql_call() # ensure that cursor wrapping is applied only once - mock_patch_cursor_wrapper.assert_called_once_with( - CursorWrapper, sql_tracking.NormalCursorWrapper + self.assertIn( + mock_patch_cursor_wrapper.mock_calls, + [ + [call(CursorWrapper, sql_tracking.NormalCursorWrapper)], + # CursorDebugWrapper is used if the test is called with `--debug-sql` + [call(CursorDebugWrapper, sql_tracking.NormalCursorWrapper)], + ], ) @patch( @@ -87,8 +92,13 @@ def test_chunked_cursor_wrapper_singleton(self, mock_patch_cursor_wrapper): sql_call(use_iterator=True) # ensure that cursor wrapping is applied only once - mock_patch_cursor_wrapper.assert_called_once_with( - CursorWrapper, sql_tracking.NormalCursorWrapper + self.assertIn( + mock_patch_cursor_wrapper.mock_calls, + [ + [call(CursorWrapper, sql_tracking.NormalCursorWrapper)], + # CursorDebugWrapper is used if the test is called with `--debug-sql` + [call(CursorDebugWrapper, sql_tracking.NormalCursorWrapper)], + ], ) @patch( @@ -98,8 +108,13 @@ def test_chunked_cursor_wrapper_singleton(self, mock_patch_cursor_wrapper): async def test_cursor_wrapper_async(self, mock_patch_cursor_wrapper): await sync_to_async(sql_call)() - mock_patch_cursor_wrapper.assert_called_once_with( - CursorWrapper, sql_tracking.NormalCursorWrapper + self.assertIn( + mock_patch_cursor_wrapper.mock_calls, + [ + [call(CursorWrapper, sql_tracking.NormalCursorWrapper)], + # CursorDebugWrapper is used if the test is called with `--debug-sql` + [call(CursorDebugWrapper, sql_tracking.NormalCursorWrapper)], + ], ) @patch( @@ -122,11 +137,19 @@ async def task(): await asyncio.create_task(task()) # Because it was called in another context, it should not have affected ours self.assertTrue(sql_tracking.allow_sql.get()) - self.assertEqual( - mock_patch_cursor_wrapper.call_args_list, + + self.assertIn( + mock_patch_cursor_wrapper.mock_calls, [ - call(CursorWrapper, sql_tracking.NormalCursorWrapper), - call(CursorWrapper, sql_tracking.ExceptionCursorWrapper), + [ + call(CursorWrapper, sql_tracking.NormalCursorWrapper), + call(CursorWrapper, sql_tracking.ExceptionCursorWrapper), + ], + # CursorDebugWrapper is used if the test is called with `--debug-sql` + [ + call(CursorDebugWrapper, sql_tracking.NormalCursorWrapper), + call(CursorDebugWrapper, sql_tracking.ExceptionCursorWrapper), + ], ], ) From 6d1b8aab502d40c6762f77052f67f9694fc82580 Mon Sep 17 00:00:00 2001 From: tschilling Date: Thu, 10 Aug 2023 20:13:36 -0500 Subject: [PATCH 4/4] Rename new mixin classes. --- debug_toolbar/panels/sql/tracking.py | 11 ++++------- tests/panels/test_sql.py | 20 ++++++++++---------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/debug_toolbar/panels/sql/tracking.py b/debug_toolbar/panels/sql/tracking.py index b90bb4dd1..0c53dc2c5 100644 --- a/debug_toolbar/panels/sql/tracking.py +++ b/debug_toolbar/panels/sql/tracking.py @@ -59,7 +59,7 @@ def cursor(*args, **kwargs): cursor = connection._djdt_cursor(*args, **kwargs) if logger is None: return cursor - mixin = NormalCursorWrapper if allow_sql.get() else ExceptionCursorWrapper + mixin = NormalCursorMixin if allow_sql.get() else ExceptionCursorMixin return patch_cursor_wrapper_with_mixin(cursor.__class__, mixin)( cursor.cursor, connection, logger ) @@ -70,10 +70,7 @@ def chunked_cursor(*args, **kwargs): logger = connection._djdt_logger cursor = connection._djdt_chunked_cursor(*args, **kwargs) if logger is not None and not isinstance(cursor, DjDTCursorWrapperMixin): - if allow_sql.get(): - mixin = NormalCursorWrapper - else: - mixin = ExceptionCursorWrapper + mixin = NormalCursorMixin if allow_sql.get() else ExceptionCursorMixin return patch_cursor_wrapper_with_mixin(cursor.__class__, mixin)( cursor.cursor, connection, logger ) @@ -97,7 +94,7 @@ def __init__(self, cursor, db, logger): self.logger = logger -class ExceptionCursorWrapper(DjDTCursorWrapperMixin): +class ExceptionCursorMixin(DjDTCursorWrapperMixin): """ Wraps a cursor and raises an exception on any operation. Used in Templates panel. @@ -107,7 +104,7 @@ def __getattr__(self, attr): raise SQLQueryTriggered() -class NormalCursorWrapper(DjDTCursorWrapperMixin): +class NormalCursorMixin(DjDTCursorWrapperMixin): """ Wraps a cursor and logs queries. """ diff --git a/tests/panels/test_sql.py b/tests/panels/test_sql.py index 7c0bece78..932a0dd92 100644 --- a/tests/panels/test_sql.py +++ b/tests/panels/test_sql.py @@ -78,9 +78,9 @@ def test_cursor_wrapper_singleton(self, mock_patch_cursor_wrapper): self.assertIn( mock_patch_cursor_wrapper.mock_calls, [ - [call(CursorWrapper, sql_tracking.NormalCursorWrapper)], + [call(CursorWrapper, sql_tracking.NormalCursorMixin)], # CursorDebugWrapper is used if the test is called with `--debug-sql` - [call(CursorDebugWrapper, sql_tracking.NormalCursorWrapper)], + [call(CursorDebugWrapper, sql_tracking.NormalCursorMixin)], ], ) @@ -95,9 +95,9 @@ def test_chunked_cursor_wrapper_singleton(self, mock_patch_cursor_wrapper): self.assertIn( mock_patch_cursor_wrapper.mock_calls, [ - [call(CursorWrapper, sql_tracking.NormalCursorWrapper)], + [call(CursorWrapper, sql_tracking.NormalCursorMixin)], # CursorDebugWrapper is used if the test is called with `--debug-sql` - [call(CursorDebugWrapper, sql_tracking.NormalCursorWrapper)], + [call(CursorDebugWrapper, sql_tracking.NormalCursorMixin)], ], ) @@ -111,9 +111,9 @@ async def test_cursor_wrapper_async(self, mock_patch_cursor_wrapper): self.assertIn( mock_patch_cursor_wrapper.mock_calls, [ - [call(CursorWrapper, sql_tracking.NormalCursorWrapper)], + [call(CursorWrapper, sql_tracking.NormalCursorMixin)], # CursorDebugWrapper is used if the test is called with `--debug-sql` - [call(CursorDebugWrapper, sql_tracking.NormalCursorWrapper)], + [call(CursorDebugWrapper, sql_tracking.NormalCursorMixin)], ], ) @@ -142,13 +142,13 @@ async def task(): mock_patch_cursor_wrapper.mock_calls, [ [ - call(CursorWrapper, sql_tracking.NormalCursorWrapper), - call(CursorWrapper, sql_tracking.ExceptionCursorWrapper), + call(CursorWrapper, sql_tracking.NormalCursorMixin), + call(CursorWrapper, sql_tracking.ExceptionCursorMixin), ], # CursorDebugWrapper is used if the test is called with `--debug-sql` [ - call(CursorDebugWrapper, sql_tracking.NormalCursorWrapper), - call(CursorDebugWrapper, sql_tracking.ExceptionCursorWrapper), + call(CursorDebugWrapper, sql_tracking.NormalCursorMixin), + call(CursorDebugWrapper, sql_tracking.ExceptionCursorMixin), ], ], )