Skip to content

Use cached settings when building engine URL#87

Merged
shayancoin merged 2 commits intomainfrom
codex/refactor-get_engine_url-to-use-cached-settings-nnol06
Oct 17, 2025
Merged

Use cached settings when building engine URL#87
shayancoin merged 2 commits intomainfrom
codex/refactor-get_engine_url-to-use-cached-settings-nnol06

Conversation

@shayancoin
Copy link
Owner

@shayancoin shayancoin commented Oct 16, 2025

Summary

  • ensure backend/api/db.get_engine_url uses the cached get_settings helper
  • fall back to the configured database URL only when the DATABASE_URL environment variable is missing

Testing

  • pytest (fails: ModuleNotFoundError: No module named 'sqlalchemy')

https://chatgpt.com/codex/tasks/task_e_68f12b954d2c833090852429f79c66f9

Summary by CodeRabbit

  • Bug Fixes
    • Updated database URL configuration handling to properly prioritize environment variables and fallback settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 16, 2025

Walkthrough

The get_engine_url() function in backend/api/db.py is modified to explicitly check if the DATABASE_URL environment variable is unset (None) before falling back to settings.database_url. Previously, the fallback was implicit via the default argument parameter. This changes behavior when DATABASE_URL is empty.

Changes

Cohort / File(s) Summary
Environment variable fallback logic
backend/api/db.py
Modified get_engine_url() to call os.getenv("DATABASE_URL") without a default value and explicitly fall back to settings.database_url only when the environment variable is None. The change affects handling of empty DATABASE_URL values and removes implicit fallback through default parameters.

Sequence Diagram(s)

sequenceDiagram
    participant Code as Caller
    participant GetEngineURL as get_engine_url()
    participant Env as os.getenv()
    participant Settings as settings.database_url

    Code->>GetEngineURL: Request database URL
    GetEngineURL->>Env: os.getenv("DATABASE_URL")
    alt DATABASE_URL is set (including empty string)
        Env-->>GetEngineURL: Return value (may be empty)
        GetEngineURL->>GetEngineURL: Use returned value
    else DATABASE_URL is not set (None)
        Env-->>GetEngineURL: None
        GetEngineURL->>Settings: Fallback to settings
        Settings-->>GetEngineURL: Return settings.database_url
    end
    GetEngineURL->>GetEngineURL: Process with sqlite/PostgreSQL logic
    GetEngineURL-->>Code: Return normalized URL
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Cache API settings helper #23: Modifies backend/api/db.py and changes how the app obtains Settings for get_engine_url(), potentially creating a direct code-level interaction with this PR's changes.
  • Use cached settings factory for engine URL #54: Modifies the same get_engine_url() function in backend/api/db.py with related changes to how DATABASE_URL is read and processed, requiring coordination between PRs.

Poem

🐰 A rabbit hops through database paths,
No hidden defaults, just truthful facts,
When DATABASE_URL is set but bare,
Settings stand ready with gentle care,
Fallback explicit, the logic is clear!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description does not follow the repository’s template because it omits the required “PR Type” and “Short Description” sections and does not include a “Tests Added” section, instead using generic “Summary” and “Testing” headings without the specified structure. Please update the description to match the repository template by adding a “PR Type” field, a concise “Short Description” of the change, and a “Tests Added” section that details any new or updated tests.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely captures the primary change of using the cached settings helper when constructing the database engine URL without extraneous details.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Docstrings were successfully generated.
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/refactor-get_engine_url-to-use-cached-settings-nnol06

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d2a35c4 and b9ac340.

📒 Files selected for processing (1)
  • backend/api/db.py (1 hunks)
🔇 Additional comments (2)
backend/api/db.py (2)

30-38: LGTM! Changes correctly implement the PR objectives.

The explicit None check ensures the fallback to settings.database_url only occurs when DATABASE_URL is unset (None), not when it's set to an empty string. This aligns with the PR description.


33-35: Fallback on empty DATABASE_URL
os.getenv("DATABASE_URL") returns "" when set to an empty string, but if url is None won’t catch this. Change to if not url (or strip whitespace) so empty values also fallback to settings.database_url.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 17, 2025

Note

Docstrings generation - SUCCESS
Generated docstrings for this pull request at #151

coderabbitai bot added a commit that referenced this pull request Oct 17, 2025
…ings-nnol06`

Docstrings generation was requested by @shayancoin.

* #87 (comment)

The following files were modified:

* `backend/api/db.py`
@shayancoin shayancoin merged commit b0b7165 into main Oct 17, 2025
3 of 7 checks passed
shayancoin added a commit that referenced this pull request Oct 18, 2025
…ings-nnol06` (#151)

Docstrings generation was requested by @shayancoin.

* #87 (comment)

The following files were modified:

* `backend/api/db.py`

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Shayan <shayan@coin.link>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant