Set ETags correctly to enable frontend caching (#643)

* Update pre-commit

* Set etags correctly for frontend caching

* Fix mypy error

* Remove 412 check in delete

* Remove test

* Comment out 412 test in put

Author: DavidMStraub

.pre-commit-config.yaml

@@ -1,15 +1,15 @@
 repos:
   - repo: https://github.com/pycqa/isort
-    rev: 5.13.2
+    rev: 6.0.1
     hooks:
       - id: isort
         args: ["--profile", "black"]
   - repo: https://github.com/psf/black
-    rev: 24.10.0
+    rev: 25.1.0
     hooks:
       - id: black
   - repo: https://github.com/pre-commit/mirrors-mypy
-    rev: v1.14.1
+    rev: v1.15.0
     hooks:
     -   id: mypy
         args: [--ignore-missing-imports, --no-strict-optional]

gramps_webapi/api/resources/base.py

@@ -62,7 +62,6 @@
 from .util import (
     abort_with_message,
     add_object,
-    app_has_semantic_search,
     filter_missing_files,
     fix_object_dict,
     get_backlinks,
@@ -240,22 +239,19 @@ def get(self, args: dict, handle: str) -> ResponseReturnValue:
         except HandleError:
             abort(404)
         locale = get_locale_for_language(args["locale"], default=True)
-        get_etag = hash_object(obj)
         return self.response(
-            200, self.full_object(obj, args, locale=locale), args, etag=get_etag
+            200,
+            self.full_object(obj, args, locale=locale),
+            args,
         )
 
     def delete(self, handle: str) -> ResponseReturnValue:
         """Delete the object."""
         require_permissions([PERM_DEL_OBJ])
         try:
-            obj = self.get_object_from_handle(handle)
+            self.get_object_from_handle(handle)
         except HandleError:
             abort(404)
-        get_etag = hash_object(obj)
-        for etag in request.if_match:
-            if etag != get_etag:
-                abort_with_message(412, "Resource does not match provided ETag")
         trans_dict = delete_object(
             self.db_handle_writable, handle, self.gramps_class_name
         )
@@ -389,7 +385,10 @@ def get(self, args: dict) -> ResponseReturnValue:
             if obj is None:
                 abort(404)
             return self.response(
-                200, [self.full_object(obj, args, locale=locale)], args, total_items=1
+                200,
+                [self.full_object(obj, args, locale=locale)],
+                args,
+                total_items=1,
             )
 
         # load all objects to memory

gramps_webapi/api/resources/emit.py

@@ -19,13 +19,15 @@
 
 """Gramps Json Encoder."""
 
-from typing import Any, Dict, Optional
+import hashlib
+from typing import Any, Optional
 
 import gramps.gen.lib as lib
 from flask import Response, current_app, json
 from gramps.gen.db import DbBookmarks
 from gramps.gen.lib.baseobj import BaseObject
-from werkzeug.datastructures import Headers
+
+from .util import return_304_if_unchanged
 
 
 def default(obj: Any):
@@ -49,9 +51,10 @@ def response(
         self,
         status: int = 200,
         payload: Optional[Any] = None,
-        args: Optional[Dict] = None,
+        args: Optional[dict] = None,
         total_items: int = -1,
         etag: Optional[str] = None,
+        cache_control: Optional[str] = None,
     ) -> Response:
         """Prepare response."""
         if payload is None:
@@ -69,22 +72,34 @@ def response(
             self.filter_skip_keys = args["skipkeys"]
         else:
             self.filter_skip_keys = []
-
+        response_string = json.dumps(
+            self.extract_objects(payload),
+            ensure_ascii=False,
+            sort_keys=True,
+            default=default,
+        )
         res = Response(
             status=status,
-            response=json.dumps(
-                self.extract_objects(payload),
-                ensure_ascii=False,
-                sort_keys=True,
-                default=default,
-            ),
+            response=response_string,
             mimetype="application/json",
         )
 
         if total_items > -1:
             res.headers.add("X-Total-Count", str(total_items))
-        if etag:
-            res.headers.add("ETag", etag)
+
+        if not etag:
+            # by default, use the hash of the response as ETag
+            etag = hashlib.sha256(response_string.encode("utf-8")).hexdigest()
+        res.headers.add("ETag", f'"{etag}"')
+
+        if cache_control:
+            res.headers.add("Cache-Control", cache_control)
+        else:
+            # by default, use a no-cache policy: allow the browser to cache,
+            # but always revalidate with the server
+            res.headers.add("Cache-Control", "no-cache")
+
+        res = return_304_if_unchanged(res, etag=etag)
         return res
 
     def is_null(self, value: Any) -> bool:
@@ -97,7 +112,7 @@ def is_null(self, value: Any) -> bool:
             pass
         return False
 
-    def extract_object(self, obj: Any, apply_filter=True) -> Dict:
+    def extract_object(self, obj: Any, apply_filter=True) -> dict:
         """Extract and filter attributes for a Gramps object."""
         data = {}
         for key, value in obj.__class__.__dict__.items():

gramps_webapi/api/resources/util.py

@@ -22,7 +22,6 @@
 
 from __future__ import annotations
 
-import json
 import os
 from hashlib import sha256
 from http import HTTPStatus
@@ -32,7 +31,7 @@
 import gramps.gen.lib
 import jsonschema
 from celery import Task
-from flask import current_app
+from flask import Response, current_app, request
 from gramps.gen.const import GRAMPS_LOCALE as glocale
 from gramps.gen.db import KEY_TO_CLASS_MAP, DbTxn
 from gramps.gen.db.base import DbReadBase, DbWriteBase
@@ -1295,3 +1294,28 @@ def dry_run_import(
 def app_has_semantic_search() -> bool:
     """Indicate whether the app supports semantic search."""
     return bool(current_app.config.get("VECTOR_EMBEDDING_MODEL"))
+
+
+def normalize_etag(etag: str | None) -> str | None:
+    """Normalize an Etag"""
+    if not etag:
+        return None
+    # Remove weak validator (W/) and suffix like :zstd or -gzip
+    if "/" in etag:
+        etag = etag.split("/", 1)[1]
+    if ":" in etag:
+        etag = etag.split(":", 1)[0]
+    elif "-" in etag:
+        etag = etag.split("-", 1)[0]
+    etag = etag.strip('"')
+    return etag
+
+
+def return_304_if_unchanged(response: Response, etag: str) -> Response:
+    """Change the response status to 304 if the if none match header agrees
+    with the current etag."""
+    old_etag = request.headers.get("If-None-Match")
+    if old_etag and normalize_etag(old_etag) == etag:
+        response.status = "304"
+        response.response = ""
+    return response

tests/test_endpoints/test_delete.py

@@ -135,61 +135,63 @@ def test_delete_permissions(self):
         self.assertEqual(out[0]["old"]["_class"], "Note")
         self.assertEqual(out[0]["type"], "delete")
 
-    def test_delete_right_etag(self):
-        handle = make_handle()
-        obj = {
-            "handle": handle,
-            "text": {"_class": "StyledText", "string": "My first note."},
-        }
+    # TODO add back etag tests after reimplementing in the endpoint
 
-        headers_admin = get_headers(self.client, "admin", "123")
-        rv = self.client.post("/api/notes/", json=obj, headers=headers_admin)
-        self.assertEqual(rv.status_code, 201)
-        rv = self.client.get(f"/api/notes/{handle}", headers=headers_admin)
-        self.assertEqual(rv.status_code, 200)
-        etag = rv.headers["ETag"]
-        rv = self.client.delete(
-            f"/api/notes/{handle}",
-            headers={**headers_admin, "If-Match": etag},
-        )
-        self.assertEqual(rv.status_code, 200)
-        # check it is gone
-        rv = self.client.get(f"/api/notes/{handle}", headers=headers_admin)
-        self.assertEqual(rv.status_code, 404)
+    # def test_delete_right_etag(self):
+    #     handle = make_handle()
+    #     obj = {
+    #         "handle": handle,
+    #         "text": {"_class": "StyledText", "string": "My first note."},
+    #     }
 
-    def test_delete_wrong_etag(self):
-        handle = make_handle()
-        obj = {
-            "handle": handle,
-            "text": {"_class": "StyledText", "string": "My first note."},
-        }
-        obj_new = {
-            "handle": handle,
-            "text": {"_class": "StyledText", "string": "My updated note."},
-        }
-        headers_admin = get_headers(self.client, "admin", "123")
-        # POST
-        rv = self.client.post("/api/notes/", json=obj, headers=headers_admin)
-        self.assertEqual(rv.status_code, 201)
-        # GET
-        rv = self.client.get(f"/api/notes/{handle}", headers=headers_admin)
-        self.assertEqual(rv.status_code, 200)
-        etag = rv.headers["ETag"]
-        # PUT
-        rv = self.client.put(
-            f"/api/notes/{handle}", json=obj_new, headers=headers_admin
-        )
-        self.assertEqual(rv.status_code, 200)
-        # DELETE
-        rv = self.client.delete(
-            f"/api/notes/{handle}",
-            headers={**headers_admin, "If-Match": etag},
-        )
-        # fails!
-        self.assertEqual(rv.status_code, 412)
-        # check it is still there
-        rv = self.client.get(f"/api/notes/{handle}", headers=headers_admin)
-        self.assertEqual(rv.status_code, 200)
+    #     headers_admin = get_headers(self.client, "admin", "123")
+    #     rv = self.client.post("/api/notes/", json=obj, headers=headers_admin)
+    #     self.assertEqual(rv.status_code, 201)
+    #     rv = self.client.get(f"/api/notes/{handle}", headers=headers_admin)
+    #     self.assertEqual(rv.status_code, 200)
+    #     etag = rv.headers["ETag"]
+    #     rv = self.client.delete(
+    #         f"/api/notes/{handle}",
+    #         headers={**headers_admin, "If-Match": etag},
+    #     )
+    #     self.assertEqual(rv.status_code, 200)
+    #     # check it is gone
+    #     rv = self.client.get(f"/api/notes/{handle}", headers=headers_admin)
+    #     self.assertEqual(rv.status_code, 404)
+
+    # def test_delete_wrong_etag(self):
+    #     handle = make_handle()
+    #     obj = {
+    #         "handle": handle,
+    #         "text": {"_class": "StyledText", "string": "My first note."},
+    #     }
+    #     obj_new = {
+    #         "handle": handle,
+    #         "text": {"_class": "StyledText", "string": "My updated note."},
+    #     }
+    #     headers_admin = get_headers(self.client, "admin", "123")
+    #     # POST
+    #     rv = self.client.post("/api/notes/", json=obj, headers=headers_admin)
+    #     self.assertEqual(rv.status_code, 201)
+    #     # GET
+    #     rv = self.client.get(f"/api/notes/{handle}", headers=headers_admin)
+    #     self.assertEqual(rv.status_code, 200)
+    #     etag = rv.headers["ETag"]
+    #     # PUT
+    #     rv = self.client.put(
+    #         f"/api/notes/{handle}", json=obj_new, headers=headers_admin
+    #     )
+    #     self.assertEqual(rv.status_code, 200)
+    #     # DELETE
+    #     rv = self.client.delete(
+    #         f"/api/notes/{handle}",
+    #         headers={**headers_admin, "If-Match": etag},
+    #     )
+    #     # fails!
+    #     self.assertEqual(rv.status_code, 412)
+    #     # check it is still there
+    #     rv = self.client.get(f"/api/notes/{handle}", headers=headers_admin)
+    #     self.assertEqual(rv.status_code, 200)
 
     def test_search_delete_note(self):
         """Test whether deleting a note updates the search index correctly."""

tests/test_endpoints/test_put.py

@@ -129,19 +129,23 @@ def test_update_permission(self):
         rv = self.client.put(
             f"/api/notes/{handle}",
             json=obj_new,
-            headers={**headers_admin, "If-Match": etag},
+            headers={
+                **headers_admin,
+                #  "If-Match": etag
+            },
         )
         self.assertEqual(rv.status_code, 200)
         # check it has changed
         rv = self.client.get(f"/api/notes/{handle}", headers=headers_guest)
         self.assertEqual(rv.json["text"]["string"], "My second note.")
-        # try again with old ETag
-        rv = self.client.put(
-            f"/api/notes/{handle}",
-            json=obj_newer,
-            headers={**headers_admin, "If-Match": etag},
-        )
-        self.assertEqual(rv.status_code, 412)
+        # Restore this check after reimplementing clash checks
+        # # try again with old ETag
+        # rv = self.client.put(
+        #     f"/api/notes/{handle}",
+        #     json=obj_newer,
+        #     headers={**headers_admin, "If-Match": etag},
+        # )
+        # self.assertEqual(rv.status_code, 412)
 
     def test_update_permission_editor(self):
         """Test update permissions for an editor."""