Skip to content

Cache API settings helper #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shayancoin merged 3 commits into from
Oct 15, 2025
Merged

Cache API settings helper #23

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
403826d
Cache API settings helper
shayancoin Oct 15, 2025
2d7f27a
Reuse cached Hygraph service
shayancoin Oct 15, 2025
c9efcdd
Merge branch 'main' into codex/add-lru_cache-for-get_settings-helper
shayancoin Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions backend/api/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Configuration settings for the FastAPI application."""

from functools import lru_cache
from typing import Any, Dict

from pydantic import Field
Expand Down Expand Up @@ -45,6 +46,13 @@ class Settings(BaseSettings):
)


@lru_cache()
def get_settings() -> Settings:
"""Return a cached instance of :class:`Settings`."""

return Settings()
Comment on lines +49 to +53
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider making the cache size explicit.

The implementation is correct, but since get_settings() takes no parameters, only one instance will ever be cached. Consider using @lru_cache (no parentheses) or @lru_cache(maxsize=1) to make the singleton intent clearer.

-@lru_cache()
+@lru_cache
 def get_settings() -> Settings:
     """Return a cached instance of :class:`Settings`."""
-
     return Settings()

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In backend/api/config.py around lines 49 to 53 the decorator uses @lru_cache()
which creates a cache with the default maxsize (128) even though get_settings()
has no parameters; change the decorator to either @lru_cache (no parentheses) or
@lru_cache(maxsize=1) so the singleton intent is explicit and only one Settings
instance is ever cached.



def get_fastapi_settings() -> Dict[str, Any]:
"""Get FastAPI application settings.

Expand Down
4 changes: 2 additions & 2 deletions backend/api/db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from sqlalchemy import create_engine
from sqlalchemy.orm import DeclarativeBase, sessionmaker

from api.config import Settings
from api.config import get_settings


class Base(DeclarativeBase):
Expand All @@ -32,7 +32,7 @@ def _normalize_postgres_url(url: str) -> str:
def get_engine_url() -> str:
"""Resolve the database URL from env or settings with sane defaults."""

settings = Settings()
settings = get_settings()
url = os.getenv("DATABASE_URL", settings.database_url)
if url.startswith("sqlite"):
# SQLite works as-is
Expand Down
4 changes: 2 additions & 2 deletions backend/api/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from fastapi import FastAPI
from starlette.middleware.cors import CORSMiddleware

from api.config import Settings
from api.config import get_settings
from api.routes import router as api_router
from api.routes_materials import router as materials_router
from api.routes_modules import router as modules_router
Expand All @@ -21,7 +21,7 @@
logger = logging.getLogger(__name__)

# Initialize settings
settings = Settings()
settings = get_settings()

# Create the FastAPI app
app = FastAPI(
Expand Down
12 changes: 8 additions & 4 deletions backend/api/routes_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,22 @@

from fastapi import APIRouter, Depends, Header, HTTPException, Request

from api.config import Settings
from api.config import get_settings
from services.hygraph_service import HygraphService


logger = logging.getLogger(__name__)
router = APIRouter(prefix="/api/sync", tags=["sync"])

# Using hygraph_token as webhook secret in MVP; adjust if separate secret provided
settings = get_settings()
_hygraph_service = HygraphService(webhook_secret=settings.hygraph_token)
Comment on lines +36 to +38
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Verify initialization safety and confirm lru_cache decorator.

Module-level initialization creates several concerns:

  1. Import-time failure risk: If get_settings() or HygraphService initialization fails (e.g., missing env vars, invalid token), the entire module import fails rather than failing gracefully at request time.

  2. Testability degradation: The singleton is created once at import, making it harder to:

    • Mock the service in tests
    • Reset state between test cases
    • Test with different configurations

  3. Missing cache decorator: The relevant code snippet shows get_settings() without an @lru_cache decorator, which contradicts the PR description stating "lru_cache-backed get_settings helper." This means Settings() is instantiated on every call to get_settings(), negating the caching benefit at this layer.

Run the following script to verify the lru_cache decorator and search for potential test files that might be affected:

🏁 Script executed:

#!/bin/bash
# Verify lru_cache decorator and find test files

echo "=== Checking get_settings implementation for lru_cache decorator ==="
rg -A 3 -B 1 'def get_settings' backend/api/config.py

echo -e "\n=== Finding test files that might mock or use HygraphService ==="
fd -e py test backend/tests --exec rg -l 'HygraphService|routes_sync'

echo -e "\n=== Checking for existing test patterns around settings ==="
rg -n 'Settings|get_settings' backend/tests/ --type py -C 2 2>/dev/null || echo "No tests directory found or no matches"

Length of output: 359

Lazy-initialize HygraphService instead of module-level instantiation
Avoid calling get_settings() and creating _hygraph_service at import time (backend/api/routes_sync.py:17–19). Import-time init can crash the app on missing/invalid env vars and makes tests hard to mock or reset. Move this into a factory or a get_hygraph_service() function (with internal caching) so it fails gracefully at runtime and is mockable in tests.

🤖 Prompt for AI Agents 
In backend/api/routes_sync.py around lines 17 to 19, avoid calling
get_settings() and instantiating HygraphService at import time; instead create a
get_hygraph_service() factory that lazy-initializes and caches a module-level
_hygraph_service on first call (calling get_settings() there), so creation
happens at runtime, failures surface gracefully, and tests can monkeypatch or
reset the cached instance; ensure the factory returns the cached instance on
subsequent calls and provide a simple way to reset or replace the cached service
in tests if needed.



def get_hygraph_service() -> HygraphService:
settings = Settings()
# Using hygraph_token as webhook secret in MVP; adjust if separate secret provided
return HygraphService(webhook_secret=settings.hygraph_token)
"""Provide the shared Hygraph service instance."""

return _hygraph_service


@router.post("/hygraph")
Expand Down