Db engine cache refactor#328
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR refactors database engine lifecycle management in src/database/setup.py. A new _create_engine helper consolidates SQLAlchemy async engine construction with URL logging and standard configuration (echo and pool_recycle=3600). The user_database() and expdb_database() accessor functions replace manual module-level singleton variables with 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
close_databases, you calldb()twice per iteration (forurlanddispose()), which is unnecessary; consider assigningengine = db()once and usingenginefor both logging and disposal before clearing the cache. - The new log line in
_create_enginelogs the fulldb_url, which may include credentials; consider usingdb_url.render_as_string(hide_password=True)or similar to avoid leaking sensitive information in logs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `close_databases`, you call `db()` twice per iteration (for `url` and `dispose()`), which is unnecessary; consider assigning `engine = db()` once and using `engine` for both logging and disposal before clearing the cache.
- The new log line in `_create_engine` logs the full `db_url`, which may include credentials; consider using `db_url.render_as_string(hide_password=True)` or similar to avoid leaking sensitive information in logs.
## Individual Comments
### Comment 1
<location path="src/database/setup.py" line_range="20" />
<code_context>
database=db_config.database,
)
+
+ logger.info("Creating database engine for {db_url}", db_url=db_url)
return create_async_engine(
db_url,
</code_context>
<issue_to_address>
**🚨 issue (security):** Avoid logging the full DB URL as it may expose credentials or other sensitive details.
`db_url`’s string/repr may include the username and password. Please log only non-sensitive parts (e.g., driver, host, database name) by masking the password (e.g., `db_url.set(password="***")`) or constructing a sanitized string instead.
</issue_to_address>
### Comment 2
<location path="src/database/setup.py" line_range="42" />
<code_context>
- _expdb_engine = None
+ for db in (user_database, expdb_database):
+ if db.cache_info().currsize == 1:
+ logger.info("Disposing of engine connected to {db_url}", db_url=db().url)
+ await db().dispose()
+ db.cache_clear()
</code_context>
<issue_to_address>
**🚨 issue (security):** The dispose log message may leak the DB URL; consider masking as with the creation log.
As with the creation log, `db().url` can include credentials and internal host details. Please log a sanitized/partial value instead (e.g., mask password or log only host and database name).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/database/setup.py`:
- Line 20: The info-level logs currently print the full db_url via
logger.info("Creating database engine for {db_url}", db_url=db_url) (and the
similar call around line 42); change these to avoid exposing the full URL:
either log a redacted identifier/alias (e.g., derive a short db_name or masked
string from db_url) at info level, and move the full db_url logging to
logger.debug(...), or simply change the present logger.info(...) calls to
logger.debug(...) and add a new logger.info(...) that prints only the redacted
alias; update both occurrences that reference db_url.
- Around line 40-44: The loop over (user_database, expdb_database) calls db()
multiple times and can abort if await db().dispose() raises; update the loop to
call db() once per iteration (e.g., engine = db()), check engine and
cache_info(), then perform await engine.dispose() inside a try/except to isolate
errors per engine and always call db.cache_clear() in a finally block so the
cache is cleared even if dispose fails; reference the existing symbols
user_database, expdb_database, db.cache_info(), await ...dispose(), and
db.cache_clear().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ff4b47cb-c9f0-40d5-8e23-95284baed773
📒 Files selected for processing (1)
src/database/setup.py
| database=db_config.database, | ||
| ) | ||
|
|
||
| logger.info("Creating database engine for {db_url}", db_url=db_url) |
There was a problem hiding this comment.
Reduce database URL exposure in info logs.
Line 20 and Line 42 log connection URLs at info level. Even with password masking, this can leak infrastructure/user metadata into normal production logs. Prefer a redacted identifier (e.g., db name alias) or move full URL logging to debug.
Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/database/setup.py` at line 20, The info-level logs currently print the
full db_url via logger.info("Creating database engine for {db_url}",
db_url=db_url) (and the similar call around line 42); change these to avoid
exposing the full URL: either log a redacted identifier/alias (e.g., derive a
short db_name or masked string from db_url) at info level, and move the full
db_url logging to logger.debug(...), or simply change the present
logger.info(...) calls to logger.debug(...) and add a new logger.info(...) that
prints only the redacted alias; update both occurrences that reference db_url.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #328 +/- ##
==========================================
- Coverage 94.48% 94.46% -0.03%
==========================================
Files 68 68
Lines 3157 3159 +2
Branches 229 227 -2
==========================================
+ Hits 2983 2984 +1
- Misses 111 113 +2
+ Partials 63 62 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Use functools to cache the database engine instead of private module variables.