use new style middleware and catch exceptions to be sure to clear request in context (#1188)

* use new style middleware

also, catch exceptions to be sure to clear request in context

* Raise exception variable for explicitness

* Test HistoricalRecords.context.request is deleted

* Updated docs for #1188

Added a changelog entry, and removed the
"Using django-webtest with Middleware" section in `common_issues.rst`,
as it's not an issue anymore now that `HistoricalRecords.context.request`
is deleted in all cases (due to using a `finally` block).

* Removed deleting request variable in admin tests

This code was added in 341aec1 - when
`HistoryRequestMiddleware` didn't delete the
`HistoricalRecords.context.request` variable. The middleware class does
now, so this can be removed.

* Refactored HistoricalRecords.context.request test

Now it also tests that the `request` attribute of
`HistoricalRecords.context` actually exists while handling each request.

---------

Co-authored-by: Anders <[email protected]>

Author: manelclos

CHANGES.rst

@@ -16,6 +16,8 @@ Unreleased
 - Dropped support for Django 4.0, which reached end-of-life on 2023-04-01 (gh-1202)
 - Added support for Django 4.2 (gh-1202)
 - Made ``bulk_update_with_history()`` return the number of model rows updated (gh-1206)
+- Fixed ``HistoryRequestMiddleware`` not cleaning up after itself (i.e. deleting
+  ``HistoricalRecords.context.request``) under some circumstances (gh-1188)
 
 3.3.0 (2023-03-08)
 ------------------

docs/common_issues.rst

@@ -124,29 +124,6 @@ Tracking Custom Users
     cannot be set directly on a swapped user model because of the user
     foreign key to track the user making changes.
 
-Using django-webtest with Middleware
-------------------------------------
-
-When using django-webtest_ to test your Django project with the
-django-simple-history middleware, you may run into an error similar to the
-following::
-
-    django.db.utils.IntegrityError: (1452, 'Cannot add or update a child row: a foreign key constraint fails (`test_env`.`core_historicaladdress`, CONSTRAINT `core_historicaladdress_history_user_id_0f2bed02_fk_user_user_id` FOREIGN KEY (`history_user_id`) REFERENCES `user_user` (`id`))')
-
-.. _django-webtest: https://github.com/django-webtest/django-webtest
-
-This error occurs because ``django-webtest`` sets
-``DEBUG_PROPAGATE_EXCEPTIONS`` to true preventing the middleware from cleaning
-up the request. To solve this issue, add the following code to any
-``clean_environment`` or ``tearDown`` method that
-you use:
-
-.. code-block:: python
-
-    from simple_history.middleware import HistoricalRecords
-    if hasattr(HistoricalRecords.context, 'request'):
-        del HistoricalRecords.context.request
-
 Using F() expressions
 ---------------------
 ``F()`` expressions, as described here_, do not work on models that have

simple_history/middleware.py

@@ -1,20 +1,23 @@
-from django.utils.deprecation import MiddlewareMixin
-
 from .models import HistoricalRecords
 
 
-class HistoryRequestMiddleware(MiddlewareMixin):
+class HistoryRequestMiddleware:
     """Expose request to HistoricalRecords.
 
     This middleware sets request as a local context/thread variable, making it
     available to the model-level utilities to allow tracking of the authenticated user
     making a change.
     """
 
-    def process_request(self, request):
-        HistoricalRecords.context.request = request
+    def __init__(self, get_response):
+        self.get_response = get_response
 
-    def process_response(self, request, response):
-        if hasattr(HistoricalRecords.context, "request"):
+    def __call__(self, request):
+        HistoricalRecords.context.request = request
+        try:
+            response = self.get_response(request)
+        except Exception as e:
+            raise e
+        finally:
             del HistoricalRecords.context.request
         return response

simple_history/tests/tests/test_admin.py

@@ -60,12 +60,6 @@ class AdminSiteTest(TestCase):
     def setUp(self):
         self.user = User.objects.create_superuser("user_login", "[email protected]", "pass")
 
-    def tearDown(self):
-        try:
-            del HistoricalRecords.context.request
-        except AttributeError:
-            pass
-
     def login(self, user=None, superuser=None):
         user = user or self.user
         if superuser is not None:

simple_history/tests/tests/test_middleware.py

@@ -1,8 +1,11 @@
 from datetime import date
+from unittest import mock
 
+from django.http import HttpResponse
 from django.test import TestCase, override_settings
 from django.urls import reverse
 
+from simple_history.models import HistoricalRecords
 from simple_history.tests.custom_user.models import CustomUser
 from simple_history.tests.models import (
     BucketDataRegisterRequestUser,
@@ -146,6 +149,38 @@ def test_bucket_member_is_set_on_create_view_when_logged_in(self):
 
         self.assertListEqual([h.history_user_id for h in history], [member1.id])
 
+    # The `request` attribute of `HistoricalRecords.context` should be deleted
+    # even if this setting is set to `True`
+    @override_settings(DEBUG_PROPAGATE_EXCEPTIONS=True)
+    @mock.patch("simple_history.tests.view.MockableView.get")
+    def test_request_attr_is_deleted_after_each_response(self, func_mock):
+        """https://github.com/jazzband/django-simple-history/issues/1189"""
+
+        def assert_has_request_attr(has_attr: bool):
+            self.assertEqual(hasattr(HistoricalRecords.context, "request"), has_attr)
+
+        def mocked_get(*args, **kwargs):
+            assert_has_request_attr(True)
+            response_ = HttpResponse(status=200)
+            response_.historical_records_request = HistoricalRecords.context.request
+            return response_
+
+        func_mock.side_effect = mocked_get
+        self.client.force_login(self.user)
+        mockable_url = reverse("mockable")
+
+        assert_has_request_attr(False)
+        response = self.client.get(mockable_url)
+        assert_has_request_attr(False)
+        # Check that the `request` attr existed while handling the request
+        self.assertEqual(response.historical_records_request.user, self.user)
+
+        func_mock.side_effect = RuntimeError()
+        with self.assertRaises(RuntimeError):
+            self.client.get(mockable_url)
+        # The request variable should be deleted even if an exception was raised
+        assert_has_request_attr(False)
+
 
 @override_settings(**middleware_override_settings)
 class MiddlewareBulkOpsTest(TestCase):

simple_history/tests/urls.py

@@ -4,6 +4,7 @@
 from simple_history.tests.view import (
     BucketDataRegisterRequestUserCreate,
     BucketDataRegisterRequestUserDetail,
+    MockableView,
     PollBulkCreateView,
     PollBulkCreateWithDefaultUserView,
     PollBulkUpdateView,
@@ -55,4 +56,5 @@
         PollBulkUpdateWithDefaultUserView.as_view(),
         name="poll-bulk-update-with-default-user",
     ),
+    path("mockable/", MockableView.as_view(), name="mockable"),
 ]

simple_history/tests/view.py

@@ -109,3 +109,10 @@ class BucketDataRegisterRequestUserCreate(CreateView):
 class BucketDataRegisterRequestUserDetail(DetailView):
     model = BucketDataRegisterRequestUser
     fields = ["data"]
+
+
+class MockableView(View):
+    """This view exists to easily mock a response."""
+
+    def get(self, request, *args, **kwargs):
+        return HttpResponse(status=200)