Codex/add lru cache for get settings helper#42
Conversation
Docstrings generation was requested by @shayancoin. * #23 (comment) The following files were modified: * `backend/api/config.py` * `backend/api/routes_sync.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. WalkthroughDocstrings expanded in backend/api/config.py for FastAPI settings metadata. A new helper _error_envelope was added in backend/api/routes_sync.py to standardize error responses. No changes to public signatures or existing control flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Router/Handler
participant E as _error_envelope (new)
participant S as Standardized Error Response
C->>R: Request triggers error path
R->>E: Build error envelope(code, message, details)
Note right of E: New helper constructs { ok: false, error: {...} }
E-->>R: Dict payload
R-->>C: S: JSON error response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Lines 30 to 33 in 989947e
Import swap leaves Settings undefined in DB helper
The module now imports get_settings, but get_engine_url still instantiates Settings() when resolving the database URL. Because Settings is no longer imported, importing api.db raises a NameError as soon as ENGINE_URL = get_engine_url() executes, preventing the app (and Alembic) from starting. Replace the call with get_settings() or re-import Settings so the function can run.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/api/main.py (1)
37-43: Bypass of cached singleton defeats optimization purpose.Line 40 creates a new
Settings()instance instead of using the cached singleton viaget_settings(), which:
- Triggers redundant environment variable parsing
- Bypasses the caching optimization introduced in this PR
- Could lead to inconsistent configuration if environment changes between calls
Apply this diff to use the cached singleton:
-from api.config import Settings as _Settings +from api.config import get_settings # Ensure tables exist in SQLite (dev/test); migrations still handle Postgres -if _Settings().database_url.startswith("sqlite"): +if get_settings().database_url.startswith("sqlite"): @app.on_event("startup") async def _ensure_sqlite_tables() -> None: Base.metadata.create_all(bind=engine)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
backend/api/config.py(2 hunks)backend/api/db.py(1 hunks)backend/api/main.py(2 hunks)backend/api/routes_sync.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
backend/api/db.py (2)
backend/api/config.py (1)
get_settings(50-58)backend/api/security.py (1)
get_settings(19-21)
backend/api/routes_sync.py (3)
backend/api/config.py (1)
get_settings(50-58)backend/api/security.py (1)
get_settings(19-21)backend/services/hygraph_service.py (1)
HygraphService(11-80)
backend/api/main.py (2)
backend/api/config.py (1)
get_settings(50-58)backend/api/security.py (1)
get_settings(19-21)
🔇 Additional comments (3)
backend/api/config.py (1)
49-58: LGTM! Clean singleton implementation.The
@lru_cache()decorator correctly ensures a single Settings instance is reused across calls, preventing redundant environment variable parsing and improving performance.backend/api/routes_sync.py (2)
41-49: Good singleton accessor pattern.The
get_hygraph_service()function provides clean access to the module-level singleton, improving testability and encapsulation.
50-62: Well-documented error helper.The
_error_envelope()function provides consistent error formatting with clear parameter documentation.
| from sqlalchemy.orm import DeclarativeBase, sessionmaker | ||
|
|
||
| from api.config import Settings | ||
| from api.config import get_settings |
There was a problem hiding this comment.
Incomplete refactoring will cause runtime NameError.
The import was changed to get_settings, but line 32 still attempts to instantiate Settings() directly, which will raise a NameError since Settings is no longer imported.
Apply this diff to use the imported function:
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"):
return url
return _normalize_postgres_url(url)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from api.config import get_settings | |
| def get_engine_url() -> str: | |
| """Resolve the database URL from env or settings with sane defaults.""" | |
| settings = get_settings() | |
| url = os.getenv("DATABASE_URL", settings.database_url) | |
| if url.startswith("sqlite"): | |
| return url | |
| return _normalize_postgres_url(url) |
🤖 Prompt for AI Agents
In backend/api/db.py around line 10 and line 32, the refactor changed the import
to get_settings but the code still calls Settings(), which will raise NameError;
replace the direct instantiation with a call to the imported function (e.g.,
settings = get_settings()) so the module uses the provided factory function
instead of the removed Settings symbol.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify no other direct Settings instantiations remain.
The relevant code snippets show that backend/api/security.py also has a get_settings() function that creates a new Settings() instance without caching, which creates inconsistency with the cached version in config.py.
Run the following script to identify all direct Settings() instantiations and duplicate get_settings implementations:
🏁 Script executed:
#!/bin/bash
# Description: Find all Settings instantiations and get_settings implementations
echo "=== Direct Settings() instantiations ==="
rg -nP '\bSettings\s*\(\s*\)' --type=py
echo -e "\n=== get_settings function definitions ==="
ast-grep --pattern 'def get_settings($$$) -> $$$:
$$$'
echo -e "\n=== Imports of Settings class ==="
rg -nP '^from .* import.*\bSettings\b' --type=pyLength of output: 1136
Consolidate Settings instantiation and implement caching
- Remove direct
Settings()calls indb.py(line 32) andsecurity.py(line 21); useconfig.get_settings(). - Delete the duplicated
get_settings()insecurity.pyand importconfig.get_settings. - Add caching to
config.get_settings()(e.g.,@functools.lru_cache) so it returns a single sharedSettingsinstance.
🤖 Prompt for AI Agents
In backend/api/db.py around line 10 and referencing usage at line 32, replace
the direct Settings() instantiation with a call to api.config.get_settings(),
and similarly update backend/api/security.py usage at line 21 to import and call
api.config.get_settings() instead of creating Settings() directly; remove the
duplicate get_settings() implementation in security.py and import the single
get_settings from config; finally, modify api.config.get_settings() to be cached
(e.g., decorate with functools.lru_cache or equivalent) so it returns a single
shared Settings instance across modules.
| # Using hygraph_token as webhook secret in MVP; adjust if separate secret provided | ||
| settings = get_settings() | ||
| _hygraph_service = HygraphService(webhook_secret=settings.hygraph_token) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify webhook secret configuration is intentional.
The comment indicates hygraph_token is being reused as the webhook secret. Ensure this is the intended configuration and that using the same credential for both API access and webhook validation aligns with security requirements.
Consider querying your external documentation or configuration management to confirm:
🔍 Additional research query:
What is the intended configuration for Hygraph webhook secrets in the paform project? Should it use a separate secret from the API token?
💡 Result:
Short answer: Yes — use a separate webhook secret, not the API token.
Why: webhook secrets are meant only to verify origin/signature of incoming requests; API tokens grant access to the Hygraph API and should be scoped and rotated differently. Keeping them separate follows least privilege, limits blast radius if one is leaked, and makes rotation/permissions simpler.
Use a dedicated webhook secret instead of the API token
Replace the webhook_secret parameter to use a separate config value:
backend/api/routes_sync.py lines 36–38:
-_hygraph_service = HygraphService(webhook_secret=settings.hygraph_token)
+_hygraph_service = HygraphService(webhook_secret=settings.hygraph_webhook_secret)Add hygraph_webhook_secret to your settings and environment.
🤖 Prompt for AI Agents
In backend/api/routes_sync.py around lines 36 to 38, the code currently uses
settings.hygraph_token as the webhook_secret; change this to use a dedicated
settings.hygraph_webhook_secret instead. Add a new configuration entry
hygraph_webhook_secret to the settings loader and environment, validate/raise if
missing, and pass settings.hygraph_webhook_secret to
HygraphService(webhook_secret=...) so the API token and webhook secret remain
separate.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @shayancoin. * #42 (comment) The following files were modified: * `backend/api/routes_sync.py`
Docstrings generation was requested by @shayancoin. * #42 (comment) The following files were modified: * `backend/api/routes_sync.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
PR Type
[Feature | Fix | Documentation | Other() ]
Short Description
...
Tests Added
...
Summary by CodeRabbit
Documentation
Refactor