Conversation
Contributor
WalkthroughThis PR refactors database engine lifecycle management in 🚥 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)
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}
Contributor
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}
Contributor
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) |
Contributor
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.
Comment on lines
+40
to
+44
| 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() |
Contributor
There was a problem hiding this comment.
Make shutdown resilient per engine and always clear cache state.
If await db().dispose() fails for one engine, the loop exits early and the other cached engine is not disposed. Also, db() is called multiple times in the same iteration. Capture the engine once, dispose with error isolation, and clear cache in finally.
Proposed fix
async def close_databases() -> None:
"""Close all database connections."""
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()
+ engine = db()
+ try:
+ logger.info("Disposing database engine")
+ await engine.dispose()
+ except Exception:
+ logger.exception("Failed disposing database engine")
+ finally:
+ db.cache_clear()🤖 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` around lines 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().
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #328 +/- ##
==========================================
+ Coverage 94.48% 94.51% +0.02%
==========================================
Files 68 68
Lines 3157 3155 -2
Branches 229 227 -2
==========================================
- Hits 2983 2982 -1
Misses 111 111
+ Partials 63 62 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use functools to cache the database engine instead of private module variables.