feat: Split Settings into Database and Operational Subsets - #31
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExtracts database configuration into a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@verdict-backend/app/config.py`:
- Around line 39-41: The module currently instantiates DatabaseSettings eagerly
when get_database_settings is imported; change get_database_settings to return a
lazy proxy that defers creating the real DatabaseSettings until the first
attribute access. Implement a small LazyDatabaseSettings inside
get_database_settings (or nearby) that accepts a factory (lambda:
DatabaseSettings()), implements __getattr__ to instantiate and cache the real
DatabaseSettings on first access and then delegate attribute lookups, keep the
`@lru_cache` on get_database_settings, and return the LazyDatabaseSettings
instance so existing call sites using settings.foo continue to work without
triggering full Settings initialization at import time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b71b4df-c70a-4432-bc48-d2c35567ad77
📒 Files selected for processing (2)
verdict-backend/alembic/env.pyverdict-backend/app/config.py
| @lru_cache | ||
| def get_database_settings() -> DatabaseSettings: | ||
| return DatabaseSettings() |
There was a problem hiding this comment.
get_database_settings() is still blocked by eager full-settings initialization.
Importing get_database_settings still executes Line 50, so app.config eagerly builds Settings() and requires asset_inventory_url/cmdb_url up front. In verdict-backend/alembic/env.py Line 10, that means Alembic can still fail during import before get_database_settings() is ever used, which defeats the DB-only split.
Minimal fix that preserves existing `settings.foo` call sites
`@lru_cache`
def get_settings() -> Settings:
"""Cached settings instance"""
return Settings()
-settings = get_settings()
+class _SettingsProxy:
+ def __getattr__(self, name: str):
+ return getattr(get_settings(), name)
+
+settings = _SettingsProxy()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@verdict-backend/app/config.py` around lines 39 - 41, The module currently
instantiates DatabaseSettings eagerly when get_database_settings is imported;
change get_database_settings to return a lazy proxy that defers creating the
real DatabaseSettings until the first attribute access. Implement a small
LazyDatabaseSettings inside get_database_settings (or nearby) that accepts a
factory (lambda: DatabaseSettings()), implements __getattr__ to instantiate and
cache the real DatabaseSettings on first access and then delegate attribute
lookups, keep the `@lru_cache` on get_database_settings, and return the
LazyDatabaseSettings instance so existing call sites using settings.foo continue
to work without triggering full Settings initialization at import time.
There was a problem hiding this comment.
This is incorrect. get_settings is executed on line 50 and fails if the env vars are not available. get_database_settings is only executed within function bodies run_migrations_offline / run_migrations_online.
There was a problem hiding this comment.
🧩 Analysis chain
Let me check the alembic/env.py file to verify the exact import and call patterns before responding.
🏁 Script executed:
#!/bin/bash
cat verdict-backend/alembic/env.py
Length of output: 2168
@cschwartz, you're right that get_database_settings() itself is only invoked inside the run_migrations_offline/run_migrations_online function bodies and is not eagerly called — that part of my original comment was incorrectly framed.
The residual concern is narrower: because from app.config import get_database_settings causes Python to execute the entire app/config.py module on first import, the module-level statement settings = get_settings() (line 50) will still fire at that point, which instantiates the full Settings class and requires asset_inventory_url/cmdb_url to be present. If the intent of the DB/operational split is to allow an Alembic-only environment to run with only the database env vars set, line 50 would still prevent that.
If that is not the goal of this PR (i.e., all env vars are expected to be present in every runtime context, and the split is purely a code-organisation concern), then your design is correct and my original comment was indeed off-base. Happy to close this out if that's the case.
✏️ Learnings added
Learnt from: cschwartz
Repo: cschwartz/verdict PR: 31
File: verdict-backend/app/config.py:39-41
Timestamp: 2026-03-14T14:51:09.345Z
Learning: In `verdict-backend/alembic/env.py`, `get_database_settings()` is imported from `app.config` at the module level but is only *called* inside the `run_migrations_offline()` and `run_migrations_online()` function bodies — it is not eagerly invoked. The module-level `settings = get_settings()` in `verdict-backend/app/config.py` (line 50) is a separate concern that fires on first import of the module.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
verdict-backend/app/database_config.py (1)
21-27: Special characters in credentials may break the URL.If
database_user,database_password, ordatabase_namecontain characters like@,:,/, or%, the constructed URL will be malformed or misinterpreted by SQLAlchemy.♻️ Proposed fix using urllib.parse.quote_plus
from functools import lru_cache +from urllib.parse import quote_plus from pydantic import Field, SecretStr from pydantic_settings import BaseSettings, SettingsConfigDict`@property` def database_url(self) -> str: + user = quote_plus(self.database_user) + password = quote_plus(self.database_password.get_secret_value()) + name = quote_plus(self.database_name) return ( f"postgresql+psycopg2://" - f"{self.database_user}:{self.database_password.get_secret_value()}" + f"{user}:{password}" - f"@{self.database_host}:{self.database_port}/{self.database_name}" + f"@{self.database_host}:{self.database_port}/{name}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@verdict-backend/app/database_config.py` around lines 21 - 27, The constructed database_url in the database_url property can be broken by special characters in database_user, database_password.get_secret_value(), or database_name; update database_url to percent-encode these components using urllib.parse.quote_plus (import it at top) and apply quote_plus to the username, password (use the secret value), and database name before formatting the f-string so the returned URL is safe for SQLAlchemy to parse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@verdict-backend/app/config.py`:
- Around line 16-18: Remove the duplicate cached DatabaseSettings provider in
config.py: delete the local get_database_settings() definition and instead
import and re-export the single source of truth from database_config (e.g. from
database_config import get_database_settings); this ensures only one `@lru_cache`
is used across the app and avoids separate caches for the same settings.
---
Nitpick comments:
In `@verdict-backend/app/database_config.py`:
- Around line 21-27: The constructed database_url in the database_url property
can be broken by special characters in database_user,
database_password.get_secret_value(), or database_name; update database_url to
percent-encode these components using urllib.parse.quote_plus (import it at top)
and apply quote_plus to the username, password (use the secret value), and
database name before formatting the f-string so the returned URL is safe for
SQLAlchemy to parse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8607e857-d7f1-4692-8733-999988667e2a
📒 Files selected for processing (3)
verdict-backend/alembic/env.pyverdict-backend/app/config.pyverdict-backend/app/database_config.py
1 participant
Implements #27
Summary by CodeRabbit