Address PR feedback with condition etag functions instead of older approach using model modified field see HEA-872

Author: girumb

apps/baseline/tests/test_viewsets.py

@@ -1,12 +1,16 @@
 import json
 import logging
 import warnings
+from datetime import timedelta
 from io import StringIO
 
 import pandas as pd
 from bs4 import BeautifulSoup
 from django.contrib.auth.models import User
+from django.core.cache import cache
 from django.urls import reverse
+from django.utils.http import http_date
+from django.utils.timezone import now
 from rest_framework.test import APITestCase
 
 from baseline.models import LivelihoodZoneBaseline
@@ -588,6 +592,35 @@ def test_filter_by_wealth_characteristic(self):
         self.assertEqual(response.status_code, 200)
         self.assertEqual(len(response.json()), 1)
 
+    def test_conditional_request_headers(self):
+
+        cache.clear()  # Clear cache to ensure clean state
+        # Test that response includes ETag and Last-Modified headers
+        response = self.client.get(self.url)
+        self.assertEqual(response.status_code, 200)
+        self.assertIn("ETag", response.headers)
+        self.assertIn("Last-Modified", response.headers)
+        self.assertTrue(response.headers["ETag"].startswith('W/"'))  # Weak ETag format
+
+        # Test If-None-Match returns 304 when not modified
+        etag = response.headers["ETag"]
+        response = self.client.get(self.url, HTTP_IF_NONE_MATCH=etag)
+        self.assertEqual(response.status_code, 304)
+
+        # Test If-None-Match returns 200 when data is modified
+        cache.clear()  # Clear cache before testing modified data
+        baseline = self.data[0]
+        baseline.population_source = "Updated source"
+        baseline.save()
+        response = self.client.get(self.url, HTTP_IF_NONE_MATCH=etag)
+        self.assertEqual(response.status_code, 200)
+        self.assertNotEqual(response.headers["ETag"], etag)
+
+        # Test If-Modified-Since with future date returns 304
+        future_date = http_date((now() + timedelta(days=1)).timestamp())
+        response = self.client.get(self.url, HTTP_IF_MODIFIED_SINCE=future_date)
+        self.assertEqual(response.status_code, 304)
+
 
 class LivelihoodZoneBaselineFacetedSearchViewTestCase(APITestCase):
     def setUp(self):

apps/baseline/viewsets.py

@@ -5,7 +5,7 @@
 from django.utils.decorators import method_decorator
 from django.utils.translation import override
 from django.views.decorators.cache import cache_page
-from django.views.decorators.http import etag
+from django.views.decorators.http import condition
 from django_filters import rest_framework as filters
 from django_filters.filters import CharFilter
 from rest_framework.permissions import AllowAny
@@ -15,7 +15,7 @@
 
 from common.fields import translation_fields
 from common.filters import MultiFieldFilter, UpperCaseFilter
-from common.utils import get_etag_for_cachedrequest
+from common.utils import make_condition_funcs
 from common.viewsets import AggregatingViewSet, BaseModelViewSet
 
 from .models import (
@@ -97,6 +97,9 @@
     WildFoodGatheringSerializer,
 )
 
+# Create condition functions for LivelihoodZoneBaseline endpoint caching
+get_baseline_etag, get_baseline_last_modified = make_condition_funcs(LivelihoodZoneBaseline)
+
 
 class SourceOrganizationFilterSet(filters.FilterSet):
     class Meta:
@@ -268,8 +271,8 @@ def get_serializer_class(self):
             return LivelihoodZoneBaselineGeoSerializer  # Use GeoFeatureModelSerializer for GeoJSON
         return LivelihoodZoneBaselineSerializer
 
+    @method_decorator(condition(etag_func=get_baseline_etag, last_modified_func=get_baseline_last_modified))
     @method_decorator(cache_page(60 * 60 * 24))  # Cache on server for 24 hours
-    @method_decorator(etag(get_etag_for_cachedrequest))
     def list(self, request, *args, **kwargs):
         return super().list(request, *args, **kwargs)
 

apps/common/utils.py

@@ -8,14 +8,12 @@
 from datetime import datetime, timedelta
 from io import BytesIO, StringIO
 from pathlib import Path
-from urllib.parse import parse_qs, urlencode, urlparse
 
 import pandas as pd
 from django.apps import apps
 from django.db.migrations.operations.base import Operation
+from django.db.models import Max
 from django.forms.models import modelform_factory
-from django.utils.cache import set_response_etag
-from django.utils.timezone import now
 from openpyxl.utils import get_column_letter
 from treebeard.mp_tree import MP_Node
 
@@ -355,75 +353,27 @@ def get_all_subclasses(cls):
 normalize = str.maketrans(normal_map)
 
 
-class Timer:
+def make_condition_funcs(model):
     """
-    See http://www.machinalis.com/blog/how-to-unit-test-python/
+    Create etag and last_modfied functions for use with Django's condition decorator.
+    All models inheriting from common.Model have a 'modified' timestamp field from TimeStampedModel.
     """
 
-    def start(self):
-        self._start = now()
+    def get_last_modified(request, *args, **kwargs):
+        # Return the most recent modification timestamp for the model.
+        result = model.objects.aggregate(last_modified=Max("modified"))
+        return result["last_modified"]
 
-    def stop(self):
-        self._stop = now()
+    def get_etag(request, *args, **kwargs):
+        """
+        Return a weak ETag based on the most recent modification timestamp.
 
-    def elapsed(self):
-        try:
-            return self._stop - self._start
-        except AttributeError:
-            return now() - self._start
-
-    def assertHasDate(self, date):
-        assert self._start <= date <= self._stop, "%s was not during the timer" % date
-
-    def assertNotHasDate(self, date):
-        assert not (self._start <= date <= self._stop), "%s was during the timer" % date
-
-
-@contextlib.contextmanager
-def timekeeper():
-    t = Timer()
-    t.start()
-    try:
-        yield t
-    finally:
-        t.stop()
-
-
-def get_etag_for_cachedrequest(request, *args, **kwargs):
-    """
-    Generate an ETag for the request based on path and query parameters.
-    If the request includes a _refresh parameter, returns None to force a cache miss.
-    """
-    u = urlparse(request.get_full_path())
-    query = parse_qs(u.query, keep_blank_values=True)
-    query.pop("_store_result", None)
-
-    # Check for refresh parameter - if present, return None to force cache miss
-    _refresh = query.pop("_refresh", None)
-    if _refresh:
-        return None
-
-    u = u._replace(query=urlencode(sorted(query.items()), True))
-    path = u.geturl()
-
-    if "format" in kwargs:
-        path += f"|format={kwargs['format']}"
-
-    etag_hash = hashlib.md5(path.encode()).hexdigest()
-
-    return f'"{etag_hash}"'
-
-
-def set_etag_for_response(response):
-    """
-    Add the etag header to a response.
-
-    This is a thin wrapper around `django.utils.cache.set_response_etag`, to report
-    the time taken to calculate the header.
-    """
-    with timekeeper() as t:
-        response = set_response_etag(response)
-    if response.has_header("ETag"):
-        logger.info("Created etag header in %s seconds" % round(t.elapsed().total_seconds(), 2))
+        Uses weak ETag (W/ prefix) because we're comparing semantic equivalence of the data,
+        not byte-for-byte equality. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/ETag
+        """
+        last_modified = get_last_modified(request, *args, **kwargs)
+        if last_modified is None:
+            return None
+        return f'W/"{hashlib.md5(last_modified.isoformat().encode()).hexdigest()}"'
 
-    return response
+    return get_etag, get_last_modified