diff --git a/CHANGES.rst b/CHANGES.rst index d5af46459..40c8852ff 100644 --- a/CHANGES.rst +++ b/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) ------------------ diff --git a/docs/common_issues.rst b/docs/common_issues.rst index 1e932fea5..aaba81a8b 100644 --- a/docs/common_issues.rst +++ b/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 diff --git a/simple_history/middleware.py b/simple_history/middleware.py index 048ec3fdf..9e469c173 100644 --- a/simple_history/middleware.py +++ b/simple_history/middleware.py @@ -1,9 +1,7 @@ -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 @@ -11,10 +9,15 @@ class HistoryRequestMiddleware(MiddlewareMixin): 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 diff --git a/simple_history/tests/tests/test_admin.py b/simple_history/tests/tests/test_admin.py index 9c5336c6f..2b441030b 100644 --- a/simple_history/tests/tests/test_admin.py +++ b/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", "u@example.com", "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: diff --git a/simple_history/tests/tests/test_middleware.py b/simple_history/tests/tests/test_middleware.py index 7707cfae7..53e20fe28 100644 --- a/simple_history/tests/tests/test_middleware.py +++ b/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): diff --git a/simple_history/tests/urls.py b/simple_history/tests/urls.py index 493e990ef..acb6cb120 100644 --- a/simple_history/tests/urls.py +++ b/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"), ] diff --git a/simple_history/tests/view.py b/simple_history/tests/view.py index 0d9e49f4b..20a18fa17 100644 --- a/simple_history/tests/view.py +++ b/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)