From b79ba81e050df79d6df39390f63120deffe499de Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 1 Mar 2026 14:38:28 +0000 Subject: [PATCH] perf(db): add indexes on query-hot columns and Redis caching layer - Add database indexes on FileRecord.created_at, FileRecord.mime_type, ProcessingLog.file_id, ProcessingLog.timestamp, and FileProcessingStep.status for faster filtering, sorting, and joins. - Add _ensure_indexes() migration for existing databases. - Create app/utils/cache.py with fail-open Redis GET/SET/DELETE helpers. - Cache MIME types dropdown query in files view (120s TTL). - Optimize batch status query to load only needed columns. Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- app/database.py | 30 +++++ app/models.py | 10 +- app/utils/cache.py | 122 +++++++++++++++++++ app/utils/file_status.py | 10 +- app/views/files.py | 10 +- tests/test_cache.py | 212 +++++++++++++++++++++++++++++++++ tests/test_database_indexes.py | 83 +++++++++++++ 7 files changed, 467 insertions(+), 10 deletions(-) create mode 100644 app/utils/cache.py create mode 100644 tests/test_cache.py create mode 100644 tests/test_database_indexes.py diff --git a/app/database.py b/app/database.py index f48b1ba5..61492804 100644 --- a/app/database.py +++ b/app/database.py @@ -165,6 +165,36 @@ def _run_schema_migrations(engine: Any) -> None: conn.execute(text("CREATE INDEX IF NOT EXISTS ix_saved_searches_user_id ON saved_searches (user_id)")) logger.info("Migration complete: 'saved_searches' table created") + # Migration: Add performance indexes for common query patterns + _ensure_indexes(engine, inspector) + + +def _ensure_indexes(engine: Any, inspector: Any) -> None: + """ + Create performance indexes for common query patterns. + + Each ``CREATE INDEX IF NOT EXISTS`` is idempotent and safe to run + on every startup. The indexes target the columns most frequently + used in file listing/filtering, status computation and log retrieval. + """ + from sqlalchemy import text + + _PERF_INDEXES = [ + ("ix_files_created_at", "files", "created_at"), + ("ix_files_mime_type", "files", "mime_type"), + ("ix_processing_logs_file_id", "processing_logs", "file_id"), + ("ix_processing_logs_timestamp", "processing_logs", "timestamp"), + ("ix_file_processing_steps_status", "file_processing_steps", "status"), + ] + + table_names = inspector.get_table_names() + with engine.begin() as conn: + for idx_name, table, column in _PERF_INDEXES: + if table in table_names: + conn.execute(text(f"CREATE INDEX IF NOT EXISTS {idx_name} ON {table} ({column})")) + + logger.info("Performance indexes ensured") + def get_db() -> Generator[Session, None, None]: """ diff --git a/app/models.py b/app/models.py index 81d0d5e5..03974588 100644 --- a/app/models.py +++ b/app/models.py @@ -46,7 +46,7 @@ class FileRecord(Base): file_size = Column(Integer, nullable=False) # MIME type or extension (optional) - mime_type = Column(String) + mime_type = Column(String, index=True) # Deduplication tracking: True if this file is a duplicate of another file # When a duplicate is detected, this file record is created but marked as duplicate @@ -68,7 +68,7 @@ class FileRecord(Base): document_title = Column(String, nullable=True) # Timestamp when we inserted this record - created_at = Column(DateTime(timezone=True), server_default=func.now()) + created_at = Column(DateTime(timezone=True), server_default=func.now(), index=True) class FileProcessingStep(Base): @@ -82,7 +82,7 @@ class FileProcessingStep(Base): id = Column(Integer, primary_key=True, index=True) file_id = Column(Integer, ForeignKey(_FILES_ID_FK), nullable=False, index=True) step_name = Column(String, nullable=False, index=True) # e.g., "hash_file", "upload_to_dropbox" - status = Column(String, nullable=False) # "pending", "in_progress", "success", "failure", "skipped" + status = Column(String, nullable=False, index=True) # "pending", "in_progress", "success", "failure", "skipped" started_at = Column(DateTime(timezone=True), nullable=True) # When step started completed_at = Column(DateTime(timezone=True), nullable=True) # When step finished (success/failure) error_message = Column(Text, nullable=True) # Error message if status is "failure" @@ -95,13 +95,13 @@ class FileProcessingStep(Base): class ProcessingLog(Base): __tablename__ = "processing_logs" id = Column(Integer, primary_key=True, index=True) - file_id = Column(Integer, ForeignKey(_FILES_ID_FK), nullable=True) # Optional file association + file_id = Column(Integer, ForeignKey(_FILES_ID_FK), nullable=True, index=True) # Optional file association task_id = Column(String, index=True) # Celery task ID step_name = Column(String) # e.g., "OCR", "convert_to_pdf", "upload_s3" status = Column(String) # "pending", "in_progress", "success", "failure" message = Column(String, nullable=True) # Error text or success note detail = Column(Text, nullable=True) # Verbose worker log output for diagnostics - timestamp = Column(DateTime(timezone=True), server_default=func.now()) + timestamp = Column(DateTime(timezone=True), server_default=func.now(), index=True) class ApplicationSettings(Base): diff --git a/app/utils/cache.py b/app/utils/cache.py new file mode 100644 index 00000000..a3e582a9 --- /dev/null +++ b/app/utils/cache.py @@ -0,0 +1,122 @@ +""" +Lightweight Redis caching layer for frequently accessed data. + +Provides a thin wrapper around Redis GET/SET with JSON serialisation and +configurable TTLs. All operations are **fail-open**: if Redis is +unavailable the caller simply gets a cache miss and falls back to the +database or other source of truth. + +Usage:: + + from app.utils.cache import cache_get, cache_set, cache_delete + + # Try cache first + value = cache_get("my_key") + if value is None: + value = expensive_query() + cache_set("my_key", value, ttl=300) +""" + +import json +import logging +from typing import Any + +import redis + +logger = logging.getLogger(__name__) + +#: Prefix applied to all cache keys to avoid collisions with other Redis users. +_KEY_PREFIX = "docuelevate:cache:" + +#: Module-level Redis client – lazily initialised on first use. +_redis_client: redis.Redis | None = None + + +def _get_redis() -> redis.Redis | None: + """Return a shared Redis client, or *None* if Redis is unreachable.""" + global _redis_client + if _redis_client is not None: + return _redis_client + try: + from app.config import settings + + _redis_client = redis.from_url(settings.redis_url, socket_connect_timeout=2, decode_responses=True) + # Quick connectivity check + _redis_client.ping() + return _redis_client + except Exception as exc: + logger.debug(f"Redis cache unavailable: {exc}") + _redis_client = None + return None + + +def cache_get(key: str) -> Any | None: + """ + Retrieve a cached value by *key*. + + Returns the deserialised Python object, or ``None`` on cache miss or + Redis error. + """ + client = _get_redis() + if client is None: + return None + try: + raw = client.get(f"{_KEY_PREFIX}{key}") + if raw is None: + return None + return json.loads(raw) + except Exception as exc: + logger.debug(f"Cache get failed for {key}: {exc}") + return None + + +def cache_set(key: str, value: Any, ttl: int = 300) -> None: + """ + Store *value* under *key* with a time-to-live of *ttl* seconds. + + Silently ignores errors so callers are never blocked by cache issues. + """ + client = _get_redis() + if client is None: + return + try: + client.setex(f"{_KEY_PREFIX}{key}", ttl, json.dumps(value)) + except Exception as exc: + logger.debug(f"Cache set failed for {key}: {exc}") + + +def cache_delete(key: str) -> None: + """ + Remove *key* from the cache. + + Silently ignores errors. + """ + client = _get_redis() + if client is None: + return + try: + client.delete(f"{_KEY_PREFIX}{key}") + except Exception as exc: + logger.debug(f"Cache delete failed for {key}: {exc}") + + +def cache_delete_pattern(pattern: str) -> None: + """ + Remove all keys matching *pattern* (glob-style) from the cache. + + Silently ignores errors. + """ + client = _get_redis() + if client is None: + return + try: + full_pattern = f"{_KEY_PREFIX}{pattern}" + cursor = 0 + while True: + cursor, keys = client.scan(cursor, match=full_pattern, count=100) + if keys: + client.delete(*keys) + if cursor == 0: + break + except Exception as exc: + logger.debug(f"Cache delete pattern failed for {pattern}: {exc}") diff --git a/app/utils/file_status.py b/app/utils/file_status.py index 75c84526..7b81ae77 100644 --- a/app/utils/file_status.py +++ b/app/utils/file_status.py @@ -106,9 +106,15 @@ def get_files_processing_status(db: Session, file_ids: List[int]) -> Dict[int, D if settings.enable_deduplication: REAL_STEPS.add("check_for_duplicates") - # Get all REAL steps for these files in one query + # Get all REAL steps for these files in one query (load only needed columns) steps = ( - db.query(FileProcessingStep) + db.query( + FileProcessingStep.file_id, + FileProcessingStep.step_name, + FileProcessingStep.status, + FileProcessingStep.updated_at, + FileProcessingStep.created_at, + ) .filter(FileProcessingStep.file_id.in_(file_ids), FileProcessingStep.step_name.in_(REAL_STEPS)) .all() ) diff --git a/app/views/files.py b/app/views/files.py index 64839983..92245809 100644 --- a/app/views/files.py +++ b/app/views/files.py @@ -8,6 +8,7 @@ from typing import Optional from fastapi import Depends, HTTPException, Query, Request from sqlalchemy.orm import Session +from app.utils.cache import cache_get, cache_set from app.utils.file_queries import apply_status_filter from app.utils.file_status import get_files_processing_status from app.views.base import APIRouter, get_db, logger, require_login, templates @@ -149,9 +150,12 @@ def files_page( # Calculate pagination info total_pages = (total_items + per_page - 1) // per_page - # Get unique MIME types for filter dropdown - mime_types = db.query(FileRecord.mime_type).distinct().filter(FileRecord.mime_type.isnot(None)).all() - mime_types = [mt[0] for mt in mime_types if mt[0]] + # Get unique MIME types for filter dropdown (cached) + mime_types = cache_get("mime_types") + if mime_types is None: + raw = db.query(FileRecord.mime_type).distinct().filter(FileRecord.mime_type.isnot(None)).all() + mime_types = [mt[0] for mt in raw if mt[0]] + cache_set("mime_types", mime_types, ttl=120) # Debug output logger.info(f"Retrieved {len(files_with_status)} files from database (page {page}/{total_pages})") diff --git a/tests/test_cache.py b/tests/test_cache.py new file mode 100644 index 00000000..59ec3dab --- /dev/null +++ b/tests/test_cache.py @@ -0,0 +1,212 @@ +""" +Tests for the Redis caching utility module. +""" + +import json +from unittest.mock import MagicMock, patch + +import pytest + +from app.utils.cache import ( + _KEY_PREFIX, + cache_delete, + cache_delete_pattern, + cache_get, + cache_set, +) + + +@pytest.fixture(autouse=True) +def _reset_redis_client(): + """Reset the module-level Redis client between tests.""" + import app.utils.cache as cache_mod + + cache_mod._redis_client = None + yield + cache_mod._redis_client = None + + +# --------------------------------------------------------------------------- +# cache_get +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_cache_get_returns_none_when_redis_unavailable(): + """cache_get should return None when Redis is not reachable.""" + with patch("app.utils.cache._get_redis", return_value=None): + assert cache_get("some_key") is None + + +@pytest.mark.unit +def test_cache_get_returns_none_on_miss(): + """cache_get should return None when the key does not exist.""" + mock_client = MagicMock() + mock_client.get.return_value = None + with patch("app.utils.cache._get_redis", return_value=mock_client): + assert cache_get("nonexistent") is None + mock_client.get.assert_called_once_with(f"{_KEY_PREFIX}nonexistent") + + +@pytest.mark.unit +def test_cache_get_returns_deserialized_value(): + """cache_get should deserialise the stored JSON string.""" + mock_client = MagicMock() + mock_client.get.return_value = json.dumps(["application/pdf", "image/png"]) + with patch("app.utils.cache._get_redis", return_value=mock_client): + result = cache_get("mime_types") + assert result == ["application/pdf", "image/png"] + + +@pytest.mark.unit +def test_cache_get_returns_none_on_exception(): + """cache_get should not raise when Redis throws an error.""" + mock_client = MagicMock() + mock_client.get.side_effect = Exception("connection lost") + with patch("app.utils.cache._get_redis", return_value=mock_client): + assert cache_get("key") is None + + +# --------------------------------------------------------------------------- +# cache_set +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_cache_set_stores_json_with_ttl(): + """cache_set should serialise value as JSON and set with TTL.""" + mock_client = MagicMock() + with patch("app.utils.cache._get_redis", return_value=mock_client): + cache_set("my_key", {"a": 1}, ttl=60) + mock_client.setex.assert_called_once_with(f"{_KEY_PREFIX}my_key", 60, json.dumps({"a": 1})) + + +@pytest.mark.unit +def test_cache_set_uses_default_ttl(): + """cache_set should default to 300 seconds TTL.""" + mock_client = MagicMock() + with patch("app.utils.cache._get_redis", return_value=mock_client): + cache_set("key", "val") + _, args, _ = mock_client.setex.mock_calls[0] + assert args[1] == 300 + + +@pytest.mark.unit +def test_cache_set_noop_when_redis_unavailable(): + """cache_set should silently do nothing when Redis is down.""" + with patch("app.utils.cache._get_redis", return_value=None): + cache_set("key", "val") # Should not raise + + +@pytest.mark.unit +def test_cache_set_ignores_exception(): + """cache_set should not raise on Redis errors.""" + mock_client = MagicMock() + mock_client.setex.side_effect = Exception("write failed") + with patch("app.utils.cache._get_redis", return_value=mock_client): + cache_set("key", "val") # Should not raise + + +# --------------------------------------------------------------------------- +# cache_delete +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_cache_delete_removes_key(): + """cache_delete should delete the prefixed key.""" + mock_client = MagicMock() + with patch("app.utils.cache._get_redis", return_value=mock_client): + cache_delete("old_key") + mock_client.delete.assert_called_once_with(f"{_KEY_PREFIX}old_key") + + +@pytest.mark.unit +def test_cache_delete_noop_when_redis_unavailable(): + """cache_delete should silently do nothing when Redis is down.""" + with patch("app.utils.cache._get_redis", return_value=None): + cache_delete("key") # Should not raise + + +@pytest.mark.unit +def test_cache_delete_ignores_exception(): + """cache_delete should not raise on Redis errors.""" + mock_client = MagicMock() + mock_client.delete.side_effect = Exception("delete failed") + with patch("app.utils.cache._get_redis", return_value=mock_client): + cache_delete("key") # Should not raise + + +# --------------------------------------------------------------------------- +# cache_delete_pattern +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_cache_delete_pattern_scans_and_deletes(): + """cache_delete_pattern should use SCAN to find and delete matching keys.""" + mock_client = MagicMock() + # Simulate SCAN returning keys in one batch then cursor 0 + mock_client.scan.return_value = (0, [f"{_KEY_PREFIX}mime_types", f"{_KEY_PREFIX}mime_list"]) + with patch("app.utils.cache._get_redis", return_value=mock_client): + cache_delete_pattern("mime_*") + mock_client.scan.assert_called_once_with(0, match=f"{_KEY_PREFIX}mime_*", count=100) + mock_client.delete.assert_called_once_with(f"{_KEY_PREFIX}mime_types", f"{_KEY_PREFIX}mime_list") + + +@pytest.mark.unit +def test_cache_delete_pattern_noop_when_redis_unavailable(): + """cache_delete_pattern should silently do nothing when Redis is down.""" + with patch("app.utils.cache._get_redis", return_value=None): + cache_delete_pattern("mime_*") # Should not raise + + +@pytest.mark.unit +def test_cache_delete_pattern_handles_empty_scan(): + """cache_delete_pattern should not call delete when SCAN returns no keys.""" + mock_client = MagicMock() + mock_client.scan.return_value = (0, []) + with patch("app.utils.cache._get_redis", return_value=mock_client): + cache_delete_pattern("none_*") + mock_client.delete.assert_not_called() + + +@pytest.mark.unit +def test_cache_delete_pattern_ignores_exception(): + """cache_delete_pattern should not raise on Redis errors.""" + mock_client = MagicMock() + mock_client.scan.side_effect = Exception("scan failed") + with patch("app.utils.cache._get_redis", return_value=mock_client): + cache_delete_pattern("x*") # Should not raise + + +# --------------------------------------------------------------------------- +# _get_redis +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_get_redis_returns_none_on_connection_failure(): + """_get_redis should return None when Redis connection fails.""" + import app.utils.cache as cache_mod + + with patch("app.utils.cache.redis.from_url", side_effect=Exception("refused")): + result = cache_mod._get_redis() + assert result is None + + +@pytest.mark.unit +def test_get_redis_caches_client(): + """_get_redis should reuse the cached client on subsequent calls.""" + import app.utils.cache as cache_mod + + mock_client = MagicMock() + mock_client.ping.return_value = True + + with patch("app.utils.cache.redis.from_url", return_value=mock_client): + first = cache_mod._get_redis() + second = cache_mod._get_redis() + + assert first is second + # from_url should only have been called once + assert first is mock_client diff --git a/tests/test_database_indexes.py b/tests/test_database_indexes.py new file mode 100644 index 00000000..e5d34a55 --- /dev/null +++ b/tests/test_database_indexes.py @@ -0,0 +1,83 @@ +""" +Tests for database performance indexes and schema migrations. +""" + +import pytest +from sqlalchemy import create_engine, inspect, text +from sqlalchemy.orm import sessionmaker +from sqlalchemy.pool import StaticPool + +from app.database import Base, _ensure_indexes + + +@pytest.fixture +def engine_with_tables(): + """Create an in-memory SQLite engine with all tables.""" + engine = create_engine( + "sqlite:///:memory:", + connect_args={"check_same_thread": False}, + poolclass=StaticPool, + ) + # Import models to register them with Base + from app.models import ( # noqa: F401 + ApplicationSettings, + FileProcessingStep, + FileRecord, + ProcessingLog, + ) + + Base.metadata.create_all(bind=engine) + return engine + + +@pytest.mark.unit +def test_ensure_indexes_creates_expected_indexes(engine_with_tables): + """_ensure_indexes should create all performance indexes.""" + engine = engine_with_tables + inspector = inspect(engine) + + # Run the migration + _ensure_indexes(engine, inspector) + + # Refresh inspector after DDL + inspector = inspect(engine) + + expected = { + "files": {"ix_files_created_at", "ix_files_mime_type"}, + "processing_logs": {"ix_processing_logs_file_id", "ix_processing_logs_timestamp"}, + "file_processing_steps": {"ix_file_processing_steps_status"}, + } + + for table, idx_names in expected.items(): + actual_indexes = {idx["name"] for idx in inspector.get_indexes(table)} + for name in idx_names: + assert name in actual_indexes, f"Index {name} missing from {table}; found {actual_indexes}" + + +@pytest.mark.unit +def test_ensure_indexes_idempotent(engine_with_tables): + """_ensure_indexes should be safe to run multiple times.""" + engine = engine_with_tables + inspector = inspect(engine) + + _ensure_indexes(engine, inspector) + # Running again should not raise + inspector2 = inspect(engine) + _ensure_indexes(engine, inspector2) + + +@pytest.mark.unit +def test_model_indexes_declared(): + """Verify key indexes are declared in SQLAlchemy model metadata.""" + from app.models import FileProcessingStep, FileRecord, ProcessingLog + + # FileRecord.created_at should be indexed + assert FileRecord.__table__.c.created_at.index is True + # FileRecord.mime_type should be indexed + assert FileRecord.__table__.c.mime_type.index is True + # ProcessingLog.file_id should be indexed + assert ProcessingLog.__table__.c.file_id.index is True + # ProcessingLog.timestamp should be indexed + assert ProcessingLog.__table__.c.timestamp.index is True + # FileProcessingStep.status should be indexed + assert FileProcessingStep.__table__.c.status.index is True