Remove session caching (#6)

Caching visits in the request.session seems like a good idea, but it's causing problems under load - leading to a lot of django.db.IntegrityError warnings whilst trying to save duplicate hashes.

Back to the route 1 approach - just look it up in the database. Adds a db read on each request. If that becomes an issue I'll revisit caching. Lesson learned again - always go simple.

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "django-user-visit"
-version = "0.3.3"
+version = "0.4"
 description = "Django app used to track user visits."
 license = "MIT"
 authors = ["YunoJuno <[email protected]>"]

tests/test_middleware.py

@@ -4,44 +4,29 @@
 import freezegun
 import pytest
 from django.contrib.auth.models import User
-from django.contrib.sessions.backends.base import SessionBase
 from django.core.exceptions import MiddlewareNotUsed
 from django.http import HttpResponse
 from django.test import Client
+from django.utils import timezone
 
-from user_visit.middleware import (
-    SESSION_KEY,
-    UserVisitMiddleware,
-    cache_visit,
-    visit_is_cached,
-)
+from user_visit.middleware import UserVisitMiddleware, save_user_visit
 from user_visit.models import UserVisit, UserVisitManager
 
 
-class TestMiddlewareFunctions:
-    @pytest.mark.parametrize(
-        "hash_value,cached_value,result",
-        (
-            ("", "", False),
-            ("", "foo", False),
-            ("foo", "", False),
-            ("foo", "bar", False),
-            ("bar", "bar", True),
-        ),
+@pytest.mark.django_db
+def test_save_user_visit():
+    """Test standalone save method handles db.IntegrityError."""
+    user = User.objects.create(username="Yoda")
+    timestamp = timezone.now()
+    uv = UserVisit.objects.create(
+        user=user,
+        session_key="test",
+        ua_string="Chrome",
+        remote_addr="127.0.0.1",
+        timestamp=timestamp,
     )
-    def test_visit_is_cached(self, hash_value, cached_value, result):
-        session = {SESSION_KEY: cached_value}
-        visit = UserVisit(hash=hash_value)
-        assert visit_is_cached(visit, session) == result
-
-    @pytest.mark.parametrize("hash_value,cached", (("", False), ("bar", True),))
-    def test_cache_visit(self, hash_value, cached):
-        """Check that hash is not stored if empty."""
-        session = {}
-        visit = UserVisit(hash=hash_value)
-        assert SESSION_KEY not in session
-        cache_visit(visit, session)
-        assert (SESSION_KEY in session) == cached
+    uv.id = None
+    save_user_visit(uv)
 
 
 @pytest.mark.django_db
@@ -86,24 +71,13 @@ def test_middleware__new_day(self):
             client.get("/")
             assert UserVisit.objects.count() == 2
 
-    def test_middleware__duplicate(self):
-        """Check that middleware handles duplicate uuids."""
-        user = User.objects.create_user("Fred")
-        client = Client()
-        client.force_login(user)
-        client.get("/")
-        with mock.patch("user_visit.middleware.visit_is_cached", return_value=False):
-            client.get("/")
-
     def test_middleware__db_integrity_error(self):
-        """Check that middleware stores hash when db raises duplicate hash error."""
+        """Check that a failing save doesn't kill middleware."""
         user = User.objects.create_user("Fred")
         client = Client()
         client.force_login(user)
-        assert not client.session.get(SESSION_KEY)
         with mock.patch.object(UserVisit, "save", side_effect=django.db.IntegrityError):
             client.get("/")
-            assert client.session[SESSION_KEY]
 
     @mock.patch("user_visit.middleware.RECORDING_DISABLED", True)
     def test_middleware__disabled(self):

user_visit/middleware.py

@@ -2,7 +2,6 @@
 import typing
 
 import django.db
-from django.contrib.sessions.backends.base import SessionBase
 from django.core.exceptions import MiddlewareNotUsed
 from django.http import HttpRequest, HttpResponse
 from django.utils import timezone
@@ -13,57 +12,29 @@
 
 logger = logging.getLogger(__name__)
 
-# used to store unique hash of the visit
-SESSION_KEY = "user_visit.hash"
 
-
-def visit_is_cached(user_visit: UserVisit, session: SessionBase) -> bool:
-    """Return True if the visit is already in the request session."""
-    if not user_visit.hash:
-        return False
-    return session.get(SESSION_KEY) == user_visit.hash
-
-
-def cache_visit(user_visit: UserVisit, session: SessionBase) -> None:
-    """Cache UserVisit in session."""
-    if not user_visit.hash:
-        return
-    session[SESSION_KEY] = user_visit.hash
+def save_user_visit(user_visit: UserVisit) -> None:
+    """Save the user visit and handle db.IntegrityError."""
+    try:
+        user_visit.save()
+    except django.db.IntegrityError:
+        logger.warning("Error saving user visit (hash='%s')", user_visit.hash)
 
 
 class UserVisitMiddleware:
-    """
-    Middleware to record user visits.
-
-    This middleware caches visits on a daily basis. Same user, device, IP
-    and session - no new visit.
-
-    """
+    """Middleware to record user visits."""
 
     def __init__(self, get_response: typing.Callable) -> None:
         if RECORDING_DISABLED:
             raise MiddlewareNotUsed("UserVisit recording has been disabled")
         self.get_response = get_response
 
     def __call__(self, request: HttpRequest) -> typing.Optional[HttpResponse]:
-        # this method will fail hard if session or auth middleware are not configured.
         if request.user.is_anonymous:
             return self.get_response(request)
 
         uv = UserVisit.objects.build(request, timezone.now())
-        if visit_is_cached(uv, request.session):
-            return self.get_response(request)
+        if not UserVisit.objects.filter(hash=uv.hash).exists():
+            save_user_visit(uv)
 
-        try:
-            uv.save()
-        except django.db.IntegrityError as ex:
-            logger.warning("Error saving user visit (hash='%s'): %s", uv.hash, ex)
-            logger.debug("Session hash='%s'", request.session.get(SESSION_KEY, ""))
-            logger.debug("UserVisit.session_key='%s'", uv.session_key)
-            logger.debug("UserVisit.remote_addr='%s'", uv.remote_addr)
-            logger.debug("UserVisit.ua_string='%s'", uv.ua_string)
-            logger.debug("UserVisit.user_id='%s'", uv.user_id)
-        # if the database has raised an IntegrityError it means the hash is already
-        # stored, but is not in the session for some reason.
-        cache_visit(uv, request.session)
         return self.get_response(request)