Fix db backup#329
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 ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe changes migrate deprecated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
Updates Django’s storage configuration to the new STORAGES-based setup required by django-dbbackup>=5, preventing runtime import failures caused by deprecated DBBACKUP_STORAGE* settings (Fixes #328).
Changes:
- Migrate dbbackup storage configuration from deprecated
DBBACKUP_STORAGE/DBBACKUP_STORAGE_OPTIONStoSTORAGES["dbbackup"]. - Refactor
production.py/development.pyto override onlySTORAGES["staticfiles"]rather than redefining the fullSTORAGESdict. - Add regression tests ensuring
dbbackup.settingsimports successfully with the new configuration and fails when legacy settings are present.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
adit/settings/base.py |
Introduces STORAGES with default and dbbackup storage backends to replace deprecated dbbackup settings. |
adit/settings/production.py |
Switches to overriding only the staticfiles storage backend (whitenoise). |
adit/settings/development.py |
Adds an explicit staticfiles storage backend for development. |
adit/core/tests/test_import_dbbackup.py |
Adds tests to catch the deprecated-settings import RuntimeError regression. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # django default storage and django-dbbackup | ||
| STORAGES = { | ||
| "default": { | ||
| "BACKEND": "django.core.files.storage.FileSystemStorage", | ||
| }, | ||
| "dbbackup": { | ||
| "BACKEND": "django.core.files.storage.FileSystemStorage", | ||
| "OPTIONS": {"location": env.str("DBBACKUP_STORAGE_LOCATION", default="/tmp/backups-adit")}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
STORAGES is now explicitly set in base.py but it does not include a "staticfiles" backend. In environments that only import base (notably adit/settings/test.py), Django’s staticfiles system may raise InvalidStorageError / fail to resolve storages["staticfiles"]. Consider defining a sensible default "staticfiles" entry in base.py (e.g., django.contrib.staticfiles.storage.StaticFilesStorage) and overriding only the backend in development.py/production.py.
There was a problem hiding this comment.
From my point of view, not defining "staticfiles" STORAGES entry while adding new settings outside the bounds of development.py/production.py should not lead to fault-tolerant behavior. In case that happens, Django produces the following error if one tries to compose-up without the setting:
django.core.files.storage.handler.InvalidStorageError: Could not find config for 'staticfiles' in settings.STORAGES.
| # django default storage and django-dbbackup | ||
| STORAGES = { | ||
| "default": { | ||
| "BACKEND": "django.core.files.storage.FileSystemStorage", | ||
| }, | ||
| "dbbackup": { | ||
| "BACKEND": "django.core.files.storage.FileSystemStorage", | ||
| "OPTIONS": {"location": env.str("DBBACKUP_STORAGE_LOCATION", default="/tmp/backups-adit")}, | ||
| }, |
There was a problem hiding this comment.
The PR description/issue notes mention raising the required django-dbbackup version to >=5.0.0, but the changes here switch configuration to STORAGES["dbbackup"] (the new-style config). If the dependency constraint isn’t also updated, environments that resolve django-dbbackup<5 may not recognize this setting and backups could break. Please ensure the dependency/version pin is updated consistently with this new configuration.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the project to the Django 5.1 STORAGES setting, replacing the deprecated DBBACKUP_STORAGE and DBBACKUP_STORAGE_OPTIONS configurations. The changes centralize storage definitions in base.py and update development.py and production.py to modify the STORAGES dictionary instead of overwriting it. A new test file was added to verify that legacy settings correctly trigger a RuntimeError. Feedback was provided regarding the use of the private _storages attribute in tests, which is considered fragile and could cause side effects in parallel test execution.
|
|
||
|
|
||
| def _import_dbbackup_settings(): | ||
| storages._storages.clear() # type: ignore[attr-defined] |
There was a problem hiding this comment.
Accessing and clearing the private _storages attribute of the Django StorageHandler is a fragile approach for a test. While it might be necessary here to force a re-evaluation of settings during the dbbackup import, it can lead to unexpected side effects in other tests that rely on the storage cache, especially if tests are run in parallel. Consider if there is a more public way to reset the storage state or if the test can be isolated further.
![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
🧹 Nitpick comments (3)
adit/core/tests/test_import_dbbackup.py (2)
12-15: Leaks global state; consider a cleanup fixture to restoresys.modulesandstorages._storages.
_import_dbbackup_settingsclears the storages registry and dropsdbbackup.settingsfromsys.modulesbefore re‑importing, but nothing restores prior state after each test. Any subsequent test that touches the storages registry (e.g.,default_storage, media uploads, or other code relying on a cacheddbbackup.settings) can observe a cleared/re-evaluated state depending on collection order, which makes the suite order-dependent and hard to diagnose later.Additionally,
storages._storagesis a private attribute ofdjango.core.files.storage.InMemoryStorageHandler; thetype: ignore[attr-defined]hint already signals this. That's fine for a targeted test, but worth isolating behind a fixture so the coupling is in one place.♻️ Proposed fixture-based isolation
import importlib import sys import pytest from django.core.files.storage import storages DEPRECATED_DBBACKUP_SETTINGS_ERROR = ( "The settings DBBACKUP_STORAGE and DBBACKUP_STORAGE_OPTIONS have been deprecated" ) +@pytest.fixture +def reimport_dbbackup_settings(): + """Restore storages registry and dbbackup.settings module after the test.""" + saved_storages = dict(storages._storages) # type: ignore[attr-defined] + saved_module = sys.modules.get("dbbackup.settings") + try: + yield _import_dbbackup_settings + finally: + storages._storages.clear() # type: ignore[attr-defined] + storages._storages.update(saved_storages) # type: ignore[attr-defined] + if saved_module is not None: + sys.modules["dbbackup.settings"] = saved_module + else: + sys.modules.pop("dbbackup.settings", None) + + def _import_dbbackup_settings(): storages._storages.clear() # type: ignore[attr-defined] sys.modules.pop("dbbackup.settings", None) importlib.import_module("dbbackup.settings") -def test_dbbackup_settings_import_succeeds(): - _import_dbbackup_settings() +def test_dbbackup_settings_import_succeeds(reimport_dbbackup_settings): + reimport_dbbackup_settings() -def test_dbbackup_settings_import_fails_with_legacy_storage_settings(settings): +def test_dbbackup_settings_import_fails_with_legacy_storage_settings( + settings, reimport_dbbackup_settings +): settings.DBBACKUP_STORAGE = "irrelevant" settings.DBBACKUP_STORAGE_OPTIONS = {} with pytest.raises( RuntimeError, match=DEPRECATED_DBBACKUP_SETTINGS_ERROR, ): - _import_dbbackup_settings() + reimport_dbbackup_settings()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adit/core/tests/test_import_dbbackup.py` around lines 12 - 15, The helper _import_dbbackup_settings mutates global state by clearing storages._storages and removing "dbbackup.settings" from sys.modules without restoring them, causing order-dependent test failures; change this by moving the behavior into a pytest fixture that (1) snapshots storages._storages (and the existence/value of sys.modules["dbbackup.settings"]) before clearing, (2) performs the import manipulation (clear storages._storages, pop "dbbackup.settings", importlib.import_module("dbbackup.settings")) for the test, and (3) on teardown restores the original storages._storages content and restores sys.modules["dbbackup.settings"] (or removes it if it did not exist), referencing the existing helper name _import_dbbackup_settings and the symbols storages._storages and sys.modules["dbbackup.settings"] so callers can switch to the fixture without leaking global state.
7-9: Consider pinningdjango-dbbackupversion or loosening the error message expectation for robustness.The test uses regex matching on the constant
"The settings DBBACKUP_STORAGE and DBBACKUP_STORAGE_OPTIONS have been deprecated", which is a prefix of the actual deprecation message. While this currently works, the dependency inpyproject.tomlis unpinned (>=4.2.1), allowing any future version. If upstream changes the deprecation message wording, the test will fail silently until the exception is raised. Either pin the version constraint to stabilize the expected message or loosen the regex match to check only for key terms like"DBBACKUP_STORAGE"and"deprecated".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adit/core/tests/test_import_dbbackup.py` around lines 7 - 9, The test's expectation uses the full DEPRECATED_DBBACKUP_SETTINGS_ERROR constant which is fragile given an unpinned django-dbbackup dependency; update the test in test_import_dbbackup.py to loosen the assertion by checking that the raised deprecation message contains key terms (e.g., "DBBACKUP_STORAGE" and "deprecated") instead of matching the entire DEPRECATED_DBBACKUP_SETTINGS_ERROR string, or alternatively pin the django-dbbackup version in pyproject.toml to a fixed release (e.g., ==4.2.1) to stabilize the exact message; reference DEPRECATED_DBBACKUP_SETTINGS_ERROR in the test when implementing the containment checks.adit/settings/base.py (1)
339-348: Consider including the default"staticfiles"entry in baseSTORAGES.Django replaces (not merges) the
STORAGESdefault when the setting is defined, so this base mapping silently drops the built‑in"staticfiles"entry. It works today only because bothadit/settings/development.pyandadit/settings/production.pyexplicitly re‑addSTORAGES["staticfiles"]. Any future settings module that forgets to do the same (e.g., a newtest.py, a management-script settings, CI helper) will fail to collect static files or raiseInvalidStorageError. Keeping the default here makes the base self-sufficient and removes the duplication between the two environment files.♻️ Proposed fix
# django default storage and django-dbbackup STORAGES = { "default": { "BACKEND": "django.core.files.storage.FileSystemStorage", }, + "staticfiles": { + "BACKEND": "django.contrib.staticfiles.storage.StaticFilesStorage", + }, "dbbackup": { "BACKEND": "django.core.files.storage.FileSystemStorage", "OPTIONS": {"location": env.str("DBBACKUP_STORAGE_LOCATION", default="/tmp/backups-adit")}, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adit/settings/base.py` around lines 339 - 348, The STORAGES dict in base settings currently omits the Django default "staticfiles" entry, which causes replacement of the default mapping; add a "staticfiles" key to the STORAGES mapping in adit/settings/base.py (alongside "default" and "dbbackup") using the Django default backend (e.g., backend "django.contrib.staticfiles.storage.StaticFilesStorage") so the base settings are self-sufficient and other environment-specific settings no longer need to re-add STORAGES["staticfiles"].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@adit/settings/base.py`:
- Around line 339-349: The project pins django-dbbackup >=4.2.1 but the settings
use the STORAGES["dbbackup"] key (and related DBBACKUP_CLEANUP_KEEP behavior)
which requires django-dbbackup 5.x; update the dependency floor in
pyproject.toml from >=4.2.1 to >=5.0.0 so installs pull a compatible
django-dbbackup that understands the "dbbackup" entry under STORAGES and avoids
using the removed legacy DBBACKUP_STORAGE* keys.
---
Nitpick comments:
In `@adit/core/tests/test_import_dbbackup.py`:
- Around line 12-15: The helper _import_dbbackup_settings mutates global state
by clearing storages._storages and removing "dbbackup.settings" from sys.modules
without restoring them, causing order-dependent test failures; change this by
moving the behavior into a pytest fixture that (1) snapshots storages._storages
(and the existence/value of sys.modules["dbbackup.settings"]) before clearing,
(2) performs the import manipulation (clear storages._storages, pop
"dbbackup.settings", importlib.import_module("dbbackup.settings")) for the test,
and (3) on teardown restores the original storages._storages content and
restores sys.modules["dbbackup.settings"] (or removes it if it did not exist),
referencing the existing helper name _import_dbbackup_settings and the symbols
storages._storages and sys.modules["dbbackup.settings"] so callers can switch to
the fixture without leaking global state.
- Around line 7-9: The test's expectation uses the full
DEPRECATED_DBBACKUP_SETTINGS_ERROR constant which is fragile given an unpinned
django-dbbackup dependency; update the test in test_import_dbbackup.py to loosen
the assertion by checking that the raised deprecation message contains key terms
(e.g., "DBBACKUP_STORAGE" and "deprecated") instead of matching the entire
DEPRECATED_DBBACKUP_SETTINGS_ERROR string, or alternatively pin the
django-dbbackup version in pyproject.toml to a fixed release (e.g., ==4.2.1) to
stabilize the exact message; reference DEPRECATED_DBBACKUP_SETTINGS_ERROR in the
test when implementing the containment checks.
In `@adit/settings/base.py`:
- Around line 339-348: The STORAGES dict in base settings currently omits the
Django default "staticfiles" entry, which causes replacement of the default
mapping; add a "staticfiles" key to the STORAGES mapping in
adit/settings/base.py (alongside "default" and "dbbackup") using the Django
default backend (e.g., backend
"django.contrib.staticfiles.storage.StaticFilesStorage") so the base settings
are self-sufficient and other environment-specific settings no longer need to
re-add STORAGES["staticfiles"].
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21a0e8d7-dc4b-44dc-8cae-04f7d6f652ec
📒 Files selected for processing (4)
adit/core/tests/test_import_dbbackup.pyadit/settings/base.pyadit/settings/development.pyadit/settings/production.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Running
dbbackup --clean -v 2with deprecated settings would result in RuntimeError. After updating the settings, the PostgreSQL backup works again.Fixes #328.
Summary by CodeRabbit
Release Notes