From 7d6b83eca91bc9a9e8db84dc31a7127302186ed3 Mon Sep 17 00:00:00 2001 From: David Straub Date: Sun, 9 Aug 2026 18:40:21 +0000 Subject: [PATCH 1/3] Reduce server load when polling the transaction history --- gramps_webapi/api/cache.py | 35 ++++- gramps_webapi/api/resources/history.py | 101 ++++++++------ gramps_webapi/api/resources/util.py | 9 +- gramps_webapi/undodb.py | 186 +++++++++++++++++++++---- tests/test_endpoints/test_history.py | 41 ++++++ tests/test_undodb.py | 81 ++++++++++- 6 files changed, 376 insertions(+), 77 deletions(-) diff --git a/gramps_webapi/api/cache.py b/gramps_webapi/api/cache.py index 7c72d34a..8ba18f19 100644 --- a/gramps_webapi/api/cache.py +++ b/gramps_webapi/api/cache.py @@ -6,7 +6,9 @@ import json import os -from flask import g, request +from typing import Collection + +from flask import current_app, g, request from flask_caching import Cache from gramps.gen.errors import HandleError @@ -17,12 +19,16 @@ get_tree_from_jwt, get_tree_from_jwt_or_fail, ) +from gramps_webapi.auth import get_all_user_details from gramps_webapi.auth.const import PERM_VIEW_PRIVATE +from gramps_webapi.const import TREE_MULTI thumbnail_cache = Cache() request_cache = Cache() persistent_cache = Cache() +USER_DICT_CACHE_TIMEOUT = 60 + def get_db_last_change_timestamp(tree_id: str) -> int | float | None: """Get the last change timestamp of the database. @@ -120,6 +126,33 @@ def skip_cache_missing_media(*args, **kwargs) -> bool: ) +def _fetch_user_dict(tree: str | None, include_treeless: bool) -> dict[str, dict]: + """Get a mapping of user IDs to user names from the auth database.""" + users = get_all_user_details( + tree=tree, include_treeless=include_treeless, include_guid=True + ) + return { + str(user["user_id"]): {"name": user["name"], "full_name": user["full_name"]} + for user in users + } + + +def get_user_dict(user_ids: Collection[str] = ()) -> dict[str, dict]: + """Get a mapping of user IDs to user names, cached for a short time. + + A cached mapping missing one of `user_ids` is refetched, so a user that is + not in the cache yet is never reported as unknown. + """ + tree = get_tree_from_jwt() + include_treeless = current_app.config["TREE"] != TREE_MULTI + cache_key = f"user_dict:{tree}:{int(include_treeless)}" + user_dict = request_cache.get(cache_key) + if user_dict is None or not set(user_ids) <= set(user_dict): + user_dict = _fetch_user_dict(tree, include_treeless) + request_cache.set(cache_key, user_dict, timeout=USER_DICT_CACHE_TIMEOUT) + return user_dict + + def make_cache_key_tiles(*args, **kwargs): """Make a cache key for map tiles.""" # max_zoom is a query arg that changes the response, so include it in the key. diff --git a/gramps_webapi/api/resources/history.py b/gramps_webapi/api/resources/history.py index 443e2fff..ecfb73d1 100644 --- a/gramps_webapi/api/resources/history.py +++ b/gramps_webapi/api/resources/history.py @@ -19,21 +19,21 @@ """Database Transaction history endpoints.""" +import hashlib import json from typing import Dict -from flask import Response, current_app +from flask import Response from flask_jwt_extended import get_jwt_identity from gramps.gen.db import REFERENCE_KEY from gramps.gen.db.dbconst import TXNADD, TXNDEL, TXNUPD from marshmallow import Schema from webargs import fields, validate -from ...auth import get_all_user_details from ...auth.const import PERM_ADD_OBJ, PERM_DEL_OBJ, PERM_EDIT_OBJ, PERM_VIEW_PRIVATE -from ...const import TREE_MULTI from ...types import ResponseReturnValue from ..auth import require_permissions +from ..cache import get_user_dict from ..tasks import ( AsyncResult, make_task_response, @@ -44,13 +44,12 @@ from ..util import ( abort_with_message, get_db_handle, - get_tree_from_jwt, get_tree_from_jwt_or_fail, ) from ..blueprint import api_blueprint from . import ProtectedResource from .schemas import UndoTransactionSchema -from .util import reverse_transaction +from .util import etag_unchanged, reverse_transaction trans_code = {"delete": TXNDEL, "add": TXNADD, "update": TXNUPD} @@ -125,8 +124,18 @@ def get(self, args: Dict) -> Response: """Return a list of transactions.""" require_permissions([PERM_VIEW_PRIVATE]) db_handle = get_db_handle() - transactions = [] undodb = db_handle.undodb + + max_id, count = undodb.get_transactions_state( + before=args["before"], + after=args["after"], + before_id=args["before_id"], + after_id=args["after_id"], + ) + etag = transactions_etag(args, max_id, count) + if etag_unchanged(etag): + return transactions_response(None, count=count, etag=etag) + ascending = args.get("sort") != "-id" transactions, count = undodb.get_transactions( page=args["page"], @@ -141,17 +150,11 @@ def get(self, args: Dict) -> Response: ) # replace user IDs by user name - user_dict = get_user_dict() + user_dict = get_user_dict(transaction_user_ids(transactions)) transactions = [ fix_transaction_user(transaction, user_dict) for transaction in transactions ] - res = Response( - response=json.dumps(transactions), - status=200, - mimetype="application/json", - ) - res.headers.add("X-Total-Count", count) - return res + return transactions_response(json.dumps(transactions), count=count, etag=etag) class TransactionHistoryQueryArgs(Schema): @@ -186,9 +189,11 @@ def get(self, args: Dict, transaction_id: int) -> Response: old_data=args["old"], new_data=args["new"], ) + if not transaction: + abort_with_message(404, f"Transaction {transaction_id} not found") # replace user IDs by user name - user_dict = get_user_dict() + user_dict = get_user_dict(transaction_user_ids([transaction])) transaction = fix_transaction_user(transaction, user_dict) return transaction @@ -221,15 +226,11 @@ def get(self, transaction_id: int) -> ResponseReturnValue: # Get the transaction to check db_handle = get_db_handle() undodb = db_handle.undodb - try: - transaction = undodb.get_transaction( - transaction_id=transaction_id, - old_data=True, - new_data=True, - ) - except AttributeError: - abort_with_message(404, f"Transaction {transaction_id} not found") - + transaction = undodb.get_transaction( + transaction_id=transaction_id, + old_data=True, + new_data=True, + ) if not transaction: abort_with_message(404, f"Transaction {transaction_id} not found") @@ -326,16 +327,11 @@ def post(self, args: Dict, transaction_id: int) -> ResponseReturnValue: # Get the transaction to undo db_handle = get_db_handle() undodb = db_handle.undodb - try: - transaction = undodb.get_transaction( - transaction_id=transaction_id, - old_data=True, - new_data=True, - ) - except AttributeError: - # This happens when get_transaction returns None and we try to call _to_dict() - abort_with_message(404, f"Transaction {transaction_id} not found") - + transaction = undodb.get_transaction( + transaction_id=transaction_id, + old_data=True, + new_data=True, + ) if not transaction: abort_with_message(404, f"Transaction {transaction_id} not found") @@ -377,17 +373,34 @@ def post(self, args: Dict, transaction_id: int) -> ResponseReturnValue: return task, 200 -def get_user_dict() -> Dict[str, Dict[str, str]]: - """Get a dictionary with user IDs to user names.""" - tree = get_tree_from_jwt() - is_single = current_app.config["TREE"] != TREE_MULTI - users = get_all_user_details( - tree=tree, include_treeless=is_single, include_guid=True +def transaction_user_ids(transactions: list[Dict]) -> set[str]: + """Get the IDs of the users that committed the given transactions.""" + return {transaction["connection"]["user_id"] for transaction in transactions} + + +def transactions_etag(args: Dict, max_id: int | None, count: int) -> str: + """Build a cache validator for a page of the transaction history. + + The user names resolved into the response are not covered: a rename becomes + visible only once the next transaction is written. + """ + tree_id = get_tree_from_jwt_or_fail() + state = json.dumps([tree_id, max_id, count, args], sort_keys=True, default=str) + return hashlib.sha256(state.encode()).hexdigest() + + +def transactions_response(payload: str | None, count: int, etag: str) -> Response: + """Build the transaction history response, or a 304 if payload is None.""" + res = Response( + response=payload, + status=200 if payload is not None else 304, + mimetype="application/json", ) - return { - str(user["user_id"]): {"name": user["name"], "full_name": user["full_name"]} - for user in users - } + res.headers.add("X-Total-Count", str(count)) + res.headers.add("ETag", f'"{etag}"') + # let the client cache the response, but always revalidate it + res.headers.add("Cache-Control", "no-cache") + return res def fix_transaction_user(transaction, user_dict): diff --git a/gramps_webapi/api/resources/util.py b/gramps_webapi/api/resources/util.py index 82590994..71fd6e2b 100644 --- a/gramps_webapi/api/resources/util.py +++ b/gramps_webapi/api/resources/util.py @@ -1851,11 +1851,16 @@ def normalize_etag(etag: str | None) -> str | None: return etag +def etag_unchanged(etag: str) -> bool: + """Check whether the if none match header agrees with the current etag.""" + old_etag = request.headers.get("If-None-Match") + return bool(old_etag) and normalize_etag(old_etag) == 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: + if etag_unchanged(etag): response.status = "304" response.response = "" return response diff --git a/gramps_webapi/undodb.py b/gramps_webapi/undodb.py index dd3500c8..fb65af8b 100644 --- a/gramps_webapi/undodb.py +++ b/gramps_webapi/undodb.py @@ -24,6 +24,7 @@ from __future__ import annotations import pickle +from collections import defaultdict from contextlib import contextmanager from time import time_ns from typing import Any @@ -47,16 +48,29 @@ LargeBinary, PrimaryKeyConstraint, Text, + and_, create_engine, inspect, + or_, text, ) -from sqlalchemy.orm import DeclarativeBase, mapped_column, relationship, sessionmaker +from sqlalchemy.orm import ( + DeclarativeBase, + Session, + contains_eager, + defer, + mapped_column, + relationship, + sessionmaker, +) from sqlalchemy.sql import func _ = glocale.translation.gettext +# transactions per change query, to keep the statement size bounded +CHANGES_QUERY_CHUNK_SIZE = 500 + def string_to_data_or_list(string: str): unserialized = orjson.loads(string) @@ -159,7 +173,7 @@ class Transaction(Base): connection = relationship("Connection", back_populates="transactions") - def _to_dict(self, old_data: bool = True, new_data: bool = True): + def _to_dict(self, changes: list[dict[str, Any]]): """Return a dict representation of the transaction.""" return { "id": self.id, @@ -169,15 +183,84 @@ def _to_dict(self, old_data: bool = True, new_data: bool = True): "last": self.last, "undo": bool(self.undo), "timestamp": self.timestamp / 1e9, - "changes": [ - change._to_dict(old_data=old_data, new_data=new_data) - for change in self.connection.changes - if self.first is None - or (change.id >= self.first and change.id <= self.last) - ], + "changes": changes, } +def _get_changes( + session: Session, + transactions: list[Transaction], + old_data: bool, + new_data: bool, +) -> dict[int, list[dict[str, Any]]]: + """Return the changes of the given transactions, keyed by transaction ID. + + The history can be requested unpaginated, so the transactions are chunked + to keep the SQL statement bounded. + """ + result: dict[int, list[dict[str, Any]]] = {} + for start in range(0, len(transactions), CHANGES_QUERY_CHUNK_SIZE): + chunk = transactions[start : start + CHANGES_QUERY_CHUNK_SIZE] + result.update(_get_changes_chunk(session, chunk, old_data, new_data)) + return result + + +def _get_changes_chunk( + session: Session, + transactions: list[Transaction], + old_data: bool, + new_data: bool, +) -> dict[int, list[dict[str, Any]]]: + """Return the changes of the given transactions in a single query.""" + # a transaction without a change range covers its whole connection + whole_connection = { + transaction.connection_id + for transaction in transactions + if transaction.first is None + } + conditions = [ + and_( + Change.connection_id == transaction.connection_id, + Change.id >= transaction.first, + Change.id <= transaction.last, + ) + for transaction in transactions + if transaction.first is not None + ] + if whole_connection: + conditions.append(Change.connection_id.in_(whole_connection)) + # the legacy binary columns are never serialised, the JSON ones only on demand + deferred = [defer(Change.old_data), defer(Change.new_data)] + if not old_data: + deferred.append(defer(Change.old_json)) + if not new_data: + deferred.append(defer(Change.new_json)) + changes = ( + session.query(Change) + .filter(or_(*conditions)) + .options(*deferred) + .order_by(Change.connection_id, Change.id) + .all() + ) + changes_by_connection: dict[int, list[Change]] = defaultdict(list) + for change in changes: + changes_by_connection[change.connection_id].append(change) + result = {} + for transaction in transactions: + candidates = changes_by_connection[transaction.connection_id] + if transaction.first is not None: + candidates = [ + change + for change in candidates + if transaction.first <= change.id <= transaction.last + ] + result[transaction.id] = [ + change._to_dict(old_data=old_data, new_data=new_data) + for change in candidates + ] + return result + + class DbUndoSQL(DbUndo): """SQL-based undo database.""" @@ -530,6 +613,54 @@ def undo_sigs(self, sigs, undo): class DbUndoSQLWeb(DbUndoSQL): """SQL-based undo database with additional methods for Web API.""" + def _transactions_query( + self, + session: Session, + before: float | None = None, + after: float | None = None, + before_id: int | None = None, + after_id: int | None = None, + ): + """Build the base query for this tree's transactions.""" + query = ( + session.query(Transaction) + .join(Transaction.connection) + .filter(Connection.tree_id == self.tree_id) + ) + if before: + query = query.filter(Transaction.timestamp < before * 1e9) + if after: + query = query.filter(Transaction.timestamp > after * 1e9) + if before_id is not None: + query = query.filter(Transaction.id < before_id) + if after_id is not None: + query = query.filter(Transaction.id > after_id) + return query + + def get_transactions_state( + self, + before: float | None = None, + after: float | None = None, + before_id: int | None = None, + after_id: int | None = None, + ) -> tuple[int | None, int]: + """Get the highest transaction ID and the number of transactions. + + Transactions are append-only and immutable, so this pair changes + whenever the result of a history query changes. + """ + with self.session_scope() as session: + query = self._transactions_query( + session, + before=before, + after=after, + before_id=before_id, + after_id=after_id, + ) + return query.with_entities( + func.max(Transaction.id), func.count(Transaction.id) + ).one() + def get_transactions( self, page: int = 1, @@ -544,19 +675,13 @@ def get_transactions( ) -> tuple[list[dict[str, Any]], int]: """Get transactions as a JSONifiable list.""" with self.session_scope() as session: - query = ( - session.query(Transaction) - .join(Connection) - .filter(Connection.tree_id == self.tree_id) + query = self._transactions_query( + session, + before=before, + after=after, + before_id=before_id, + after_id=after_id, ) - if before: - query = query.filter(Transaction.timestamp < before * 1e9) - if after: - query = query.filter(Transaction.timestamp > after * 1e9) - if before_id is not None: - query = query.filter(Transaction.id < before_id) - if after_id is not None: - query = query.filter(Transaction.id > after_id) count = query.count() if ascending: query = query.order_by(Transaction.id) @@ -564,9 +689,10 @@ def get_transactions( query = query.order_by(Transaction.id.desc()) if page and pagesize: query = query.limit(pagesize).offset((page - 1) * pagesize) - transactions = query.all() + transactions = query.options(contains_eager(Transaction.connection)).all() + changes = _get_changes(session, transactions, old_data, new_data) return [ - transaction._to_dict(old_data=old_data, new_data=new_data) + transaction._to_dict(changes[transaction.id]) for transaction in transactions ], count @@ -575,17 +701,19 @@ def get_transaction( transaction_id: int, old_data: bool = True, new_data: bool = True, - ) -> list[dict[str, Any]]: + ) -> dict[str, Any] | None: """Get a single transaction as a JSONifiable dict.""" with self.session_scope() as session: - query = ( - session.query(Transaction) - .join(Connection) - .filter(Connection.tree_id == self.tree_id) + transaction = ( + self._transactions_query(session) .filter(Transaction.id == transaction_id) + .options(contains_eager(Transaction.connection)) + .scalar() ) - transaction = query.scalar() - return transaction._to_dict(old_data=old_data, new_data=new_data) + if transaction is None: + return None + changes = _get_changes(session, [transaction], old_data, new_data) + return transaction._to_dict(changes[transaction.id]) def _add_json_columns(undodb: DbUndoSQL) -> None: diff --git a/tests/test_endpoints/test_history.py b/tests/test_endpoints/test_history.py index c4f12290..e4ad7c05 100644 --- a/tests/test_endpoints/test_history.py +++ b/tests/test_endpoints/test_history.py @@ -155,6 +155,47 @@ def test_add_one_plus_one(self): assert change["obj_class"] == "Person" assert change["trans_type"] == 0 + def test_etag_revalidation(self): + headers = get_headers(self.client, "editor", "123") + rv = self.client.post("/api/people/", json={}, headers=headers) + assert rv.status_code == 201 + rv = self.client.get("/api/transactions/history/", headers=headers) + assert rv.status_code == 200 + assert rv.headers["Cache-Control"] == "no-cache" + etag = rv.headers["ETag"] + rv = self.client.get( + "/api/transactions/history/", headers={**headers, "If-None-Match": etag} + ) + assert rv.status_code == 304 + assert rv.data == b"" + assert rv.headers["X-Total-Count"] == "1" + # a new transaction invalidates the client's copy + rv = self.client.post("/api/people/", json={}, headers=headers) + assert rv.status_code == 201 + rv = self.client.get( + "/api/transactions/history/", headers={**headers, "If-None-Match": etag} + ) + assert rv.status_code == 200 + assert len(rv.json) == 2 + assert rv.headers["ETag"] != etag + + def test_etag_depends_on_query(self): + headers = get_headers(self.client, "editor", "123") + for _ in range(2): + rv = self.client.post("/api/people/", json={}, headers=headers) + assert rv.status_code == 201 + rv = self.client.get( + "/api/transactions/history/?page=1&pagesize=1", headers=headers + ) + assert rv.status_code == 200 + etag = rv.headers["ETag"] + rv = self.client.get( + "/api/transactions/history/?page=2&pagesize=1", + headers={**headers, "If-None-Match": etag}, + ) + assert rv.status_code == 200 + assert [transaction["id"] for transaction in rv.json] == [2] + def test_add_modify_delete(self): headers = get_headers(self.client, "editor", "123") rv = self.client.get("/api/transactions/history/", headers=headers) diff --git a/tests/test_undodb.py b/tests/test_undodb.py index 3bae0b8c..79ca6c9b 100644 --- a/tests/test_undodb.py +++ b/tests/test_undodb.py @@ -25,6 +25,7 @@ import tempfile import time import unittest +from unittest.mock import patch from gramps.gen.lib.json_utils import ( object_to_dict, @@ -46,7 +47,7 @@ ) from sqlalchemy import text -from gramps_webapi.undodb import DbUndoSQL +from gramps_webapi.undodb import DbUndoSQL, DbUndoSQLWeb def dict_factory(cursor, row): @@ -225,6 +226,84 @@ def test_undo_redo_modify(self): assert string_to_dict(commit["old_json"]) == object_to_dict(old_person) +class TestGetTransactions(unittest.TestCase): + """Tests for the transaction history queries of `DbUndoSQLWeb`.""" + + def setUp(self): + self.dbdir = tempfile.mkdtemp() + self.db: DbWriteBase = make_database("sqlite") + + def create_undo_manager(): + path = self.db.undolog + return DbUndoSQLWeb(grampsdb=self.db, dburl=f"sqlite:///{path}", tree_id=1) + + self.db._create_undo_manager = create_undo_manager + self.db.load(self.dbdir) + + # separate transactions, all within the same connection + for description, obj_class, add_func in [ + ("Add person", Person, self.db.add_person), + ("Add note", Note, self.db.add_note), + ("Add place", Place, self.db.add_place), + ]: + with DbTxn(description, self.db) as trans: + add_func(obj_class(), trans) + + def tearDown(self): + self.db.close(update=False) + shutil.rmtree(self.dbdir) + + def test_changes_of_shared_connection(self): + undodb = self.db.get_undodb() + transactions, count = undodb.get_transactions() + assert count == 3 + assert {transaction["connection"]["id"] for transaction in transactions} == {1} + assert [transaction["description"] for transaction in transactions] == [ + "Add person", + "Add note", + "Add place", + ] + for transaction, obj_class in zip(transactions, ["Person", "Note", "Place"]): + assert [change["obj_class"] for change in transaction["changes"]] == [ + obj_class + ] + + def test_get_transaction(self): + undodb = self.db.get_undodb() + transaction = undodb.get_transaction(2) + assert transaction["description"] == "Add note" + assert [change["obj_class"] for change in transaction["changes"]] == ["Note"] + assert undodb.get_transaction(99) is None + + def test_changes_of_chunked_transactions(self): + undodb = self.db.get_undodb() + with patch("gramps_webapi.undodb.CHANGES_QUERY_CHUNK_SIZE", 2): + transactions, _ = undodb.get_transactions() + assert [ + change["obj_class"] + for transaction in transactions + for change in transaction["changes"] + ] == ["Person", "Note", "Place"] + + def test_transactions_state(self): + undodb = self.db.get_undodb() + assert undodb.get_transactions_state() == (3, 3) + with DbTxn("Add another person", self.db) as trans: + self.db.add_person(Person(), trans) + assert undodb.get_transactions_state() == (4, 4) + + def test_data_only_included_on_demand(self): + undodb = self.db.get_undodb() + transactions, _ = undodb.get_transactions(old_data=False, new_data=False) + change = transactions[0]["changes"][0] + assert "old_data" not in change + assert "new_data" not in change + transactions, _ = undodb.get_transactions(old_data=True, new_data=True) + change = transactions[0]["changes"][0] + assert change["old_data"] == {} + assert change["new_data"]["_class"] == "Person" + + class TestMigrate(unittest.TestCase): """Tests for the migrate() function (pre-v3.0 → v3.0 undo DB migration).""" From 6cc369128bf8dc985a8887ae45e8243dba09458d Mon Sep 17 00:00:00 2001 From: David Straub Date: Mon, 10 Aug 2026 08:00:02 +0000 Subject: [PATCH 2/3] Address comments --- gramps_webapi/api/cache.py | 2 +- gramps_webapi/api/resources/history.py | 1 + gramps_webapi/undodb.py | 9 +++++++-- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/gramps_webapi/api/cache.py b/gramps_webapi/api/cache.py index 8ba18f19..f1e4718e 100644 --- a/gramps_webapi/api/cache.py +++ b/gramps_webapi/api/cache.py @@ -147,7 +147,7 @@ def get_user_dict(user_ids: Collection[str] = ()) -> dict[str, dict]: include_treeless = current_app.config["TREE"] != TREE_MULTI cache_key = f"user_dict:{tree}:{int(include_treeless)}" user_dict = request_cache.get(cache_key) - if user_dict is None or not set(user_ids) <= set(user_dict): + if user_dict is None or any(user_id not in user_dict for user_id in user_ids): user_dict = _fetch_user_dict(tree, include_treeless) request_cache.set(cache_key, user_dict, timeout=USER_DICT_CACHE_TIMEOUT) return user_dict diff --git a/gramps_webapi/api/resources/history.py b/gramps_webapi/api/resources/history.py index ecfb73d1..a9c0fe0a 100644 --- a/gramps_webapi/api/resources/history.py +++ b/gramps_webapi/api/resources/history.py @@ -147,6 +147,7 @@ def get(self, args: Dict) -> Response: after=args["after"], before_id=args["before_id"], after_id=args["after_id"], + known_count=count, ) # replace user IDs by user name diff --git a/gramps_webapi/undodb.py b/gramps_webapi/undodb.py index fb65af8b..22500319 100644 --- a/gramps_webapi/undodb.py +++ b/gramps_webapi/undodb.py @@ -672,8 +672,13 @@ def get_transactions( after: float | None = None, before_id: int | None = None, after_id: int | None = None, + known_count: int | None = None, ) -> tuple[list[dict[str, Any]], int]: - """Get transactions as a JSONifiable list.""" + """Get transactions as a JSONifiable list. + + `known_count` is returned in place of counting the matching + transactions again. + """ with self.session_scope() as session: query = self._transactions_query( session, @@ -682,7 +687,7 @@ def get_transactions( before_id=before_id, after_id=after_id, ) - count = query.count() + count = query.count() if known_count is None else known_count if ascending: query = query.order_by(Transaction.id) else: From bf7c2e4a6600b5d26c46da0ec44fde712b751584 Mon Sep 17 00:00:00 2001 From: David Straub Date: Mon, 10 Aug 2026 09:12:49 +0000 Subject: [PATCH 3/3] Address comments --- gramps_webapi/api/resources/util.py | 14 +++++++++++--- gramps_webapi/undodb.py | 6 ++++-- tests/test_endpoints/test_history.py | 13 +++++++++++++ 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/gramps_webapi/api/resources/util.py b/gramps_webapi/api/resources/util.py index 71fd6e2b..aaf24af5 100644 --- a/gramps_webapi/api/resources/util.py +++ b/gramps_webapi/api/resources/util.py @@ -1852,9 +1852,17 @@ def normalize_etag(etag: str | None) -> str | None: def etag_unchanged(etag: str) -> bool: - """Check whether the if none match header agrees with the current etag.""" - old_etag = request.headers.get("If-None-Match") - return bool(old_etag) and normalize_etag(old_etag) == etag + """Check whether the if none match header agrees with the current etag. + + The header may carry a comma-separated list of validators. The wildcard + `*` is not treated as a match. + """ + header = request.headers.get("If-None-Match") + if not header: + return False + return any( + normalize_etag(candidate.strip()) == etag for candidate in header.split(",") + ) def return_304_if_unchanged(response: Response, etag: str) -> Response: diff --git a/gramps_webapi/undodb.py b/gramps_webapi/undodb.py index 22500319..882369d7 100644 --- a/gramps_webapi/undodb.py +++ b/gramps_webapi/undodb.py @@ -68,8 +68,10 @@ _ = glocale.translation.gettext -# transactions per change query, to keep the statement size bounded -CHANGES_QUERY_CHUNK_SIZE = 500 +# transactions per change query. Each one contributes a term of three bind +# parameters, so this stays clear of both SQLite's expression depth limit and +# the 999 parameter limit of SQLite versions before 3.32. +CHANGES_QUERY_CHUNK_SIZE = 200 def string_to_data_or_list(string: str): diff --git a/tests/test_endpoints/test_history.py b/tests/test_endpoints/test_history.py index e4ad7c05..6a00bd1d 100644 --- a/tests/test_endpoints/test_history.py +++ b/tests/test_endpoints/test_history.py @@ -179,6 +179,19 @@ def test_etag_revalidation(self): assert len(rv.json) == 2 assert rv.headers["ETag"] != etag + def test_etag_list_of_validators(self): + headers = get_headers(self.client, "editor", "123") + rv = self.client.post("/api/people/", json={}, headers=headers) + assert rv.status_code == 201 + rv = self.client.get("/api/transactions/history/", headers=headers) + assert rv.status_code == 200 + etag = rv.headers["ETag"] + rv = self.client.get( + "/api/transactions/history/", + headers={**headers, "If-None-Match": f'"stale", {etag}'}, + ) + assert rv.status_code == 304 + def test_etag_depends_on_query(self): headers = get_headers(self.client, "editor", "123") for _ in range(2):