fix: persistent admin settings correction#839
Merged
afairgiant merged 3 commits intomainfrom Apr 23, 2026
Merged
Conversation
- Added functionality to persist updated backup and trash retention settings, minimum and maximum backup counts, and user registration allowance in the database. - Implemented loading of persisted settings during application startup to ensure configuration consistency. - Improved error handling for loading persisted settings to prevent application failures.
📝 WalkthroughWalkthroughIntroduce persisted admin-configurable settings: new module to serialize/deserialize selected settings to Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API (admin backup endpoint)
participant Persist as PersistModule
participant DB as system_settings DB
participant Settings as InMemory settings
Client->>API: POST /api/v1/admin/backups/settings/retention (payload)
API->>API: validate fields -> collect pending updates
API->>Persist: persist_setting(db, key, value, commit=False) [for each pending]
Persist->>DB: system_setting.set_setting(key, serialized_value) (flush)
DB-->>Persist: ack
API->>DB: db.commit()
DB-->>API: commit ack
API->>Settings: apply values to in-memory settings
API-->>Client: 200 OK (updated settings)
sequenceDiagram
participant Startup as App Startup
participant DB as system_settings DB
participant Persist as PersistModule
participant Settings as InMemory settings
Startup->>DB: create SessionLocal()
Startup->>Persist: load_persisted_settings(db)
Persist->>DB: system_setting.get_setting(key) [for each registered key]
DB-->>Persist: stored_value / null
alt stored_value present and parses
Persist->>Settings: overwrite settings.<attr> with parsed value
else parse/validation fails
Persist->>Startup: log warning, continue
end
Persist-->>Startup: completed
Startup->>DB: close session
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/v1/admin/backup.py (1)
642-714:⚠️ Potential issue | 🟠 MajorAvoid partial state commits on mixed-validity or mid-write failures.
The function currently validates and writes incrementally. If a later field fails validation (or persistence), earlier fields may already be persisted/mutated, but the API returns an error. This produces partial updates from a failed request.
💡 Proposed direction
async def update_retention_settings(...): try: - updated_settings = {} + updated_settings = {} + pending_updates = [] + # validate all requested fields first; don't mutate/persist yet if settings_update.backup_retention_days is not None: if settings_update.backup_retention_days < 1: raise HTTPException(...) - settings.BACKUP_RETENTION_DAYS = settings_update.backup_retention_days - persist_setting(db, KEY_BACKUP_RETENTION_DAYS, settings_update.backup_retention_days) - updated_settings["backup_retention_days"] = settings_update.backup_retention_days + pending_updates.append( + ("backup_retention_days", "BACKUP_RETENTION_DAYS", KEY_BACKUP_RETENTION_DAYS, settings_update.backup_retention_days) + ) # ...same pattern for other fields... + # persist first (ideally in one DB transaction), then mutate in-memory + for public_name, attr_name, key_name, value in pending_updates: + persist_setting(db, key_name, value) + for public_name, attr_name, key_name, value in pending_updates: + setattr(settings, attr_name, value) + updated_settings[public_name] = valueFor full atomicity across multiple keys, persist all keys in a single transaction (the current underlying per-key commit behavior should be adjusted accordingly).
🧹 Nitpick comments (1)
tests/api/test_retention_settings_persistence.py (1)
100-107: Add regression tests for two high-risk edge cases.Current coverage is strong, but it misses:
- malformed persisted boolean (e.g.,
"not-a-bool") should be skipped, and- mixed-validity multi-field update should not partially persist on failure.
Also applies to: 133-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/test_retention_settings_persistence.py` around lines 100 - 107, Add two regression tests to tests/api/test_retention_settings_persistence.py: one that posts a malformed boolean (e.g., json={"some_boolean_setting": "not-a-bool"}) and asserts a 400 response and that system_setting.get_setting(db_session, <that_boolean_KEY>) remains None, and another that posts a multi-field update where one field is valid and the other invalid (e.g., {"backup_retention_days": 7, "some_other_field": "invalid"}), asserts 400, and verifies no partial persistence (both KEY_BACKUP_RETENTION_DAYS via system_setting.get_setting and the other field's key remain unchanged/None); follow the existing test pattern using admin_client, db_session, restore_settings and RETENTION_URL and mirror the assertion style from test_invalid_retention_does_not_persist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/core/persisted_settings.py`:
- Around line 33-35: The _parse_bool helper currently coerces any non-"true"
string to False; change it to validate strictly by accepting only "true" or
"false" (case-insensitive, trimmed) and reject other inputs (e.g., raise
ValueError) so callers like load_persisted_settings can detect malformed values
and skip them instead of silently applying False; update the function
_parse_bool to perform this strict check and propagate/raise an error on invalid
strings.
---
Nitpick comments:
In `@tests/api/test_retention_settings_persistence.py`:
- Around line 100-107: Add two regression tests to
tests/api/test_retention_settings_persistence.py: one that posts a malformed
boolean (e.g., json={"some_boolean_setting": "not-a-bool"}) and asserts a 400
response and that system_setting.get_setting(db_session, <that_boolean_KEY>)
remains None, and another that posts a multi-field update where one field is
valid and the other invalid (e.g., {"backup_retention_days": 7,
"some_other_field": "invalid"}), asserts 400, and verifies no partial
persistence (both KEY_BACKUP_RETENTION_DAYS via system_setting.get_setting and
the other field's key remain unchanged/None); follow the existing test pattern
using admin_client, db_session, restore_settings and RETENTION_URL and mirror
the assertion style from test_invalid_retention_does_not_persist.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b3272aa-efc2-49b3-9346-ba0509e5fe13
📒 Files selected for processing (4)
app/api/v1/admin/backup.pyapp/core/persisted_settings.pyapp/core/startup.pytests/api/test_retention_settings_persistence.py
- Enhanced the update_retention_settings function to validate all fields before applying changes, ensuring atomicity during updates. - Introduced a pending list to stage validated settings for persistence, preventing partial updates on validation failure. - Updated the boolean parsing function to raise an error for invalid inputs, improving robustness. - Added tests to verify correct handling of invalid types and atomicity during updates.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/v1/admin/backup.py`:
- Around line 733-736: The loop currently sets in-memory settings before
persisting and calls persist_setting(db, key, value) per item, risking partial
commits; change to first persist all pending entries inside a single DB
transaction (use the active DB session/transaction and call persist_setting or
system_setting.set_setting for each but defer committing until all are
validated), only after the transaction successfully commits apply the in-memory
mutations (setattr(settings, attr, value)) and update
updated_settings[response_name] = value; reference the pending iteration,
persist_setting, and settings symbols to locate and implement the transactional
persistence-then-apply swap.
In `@app/core/persisted_settings.py`:
- Around line 97-115: Parsed persisted settings from PERSISTED_SETTINGS via
system_setting.get_setting are applied directly to settings using setattr after
entry.parse; add semantic validation between entry.parse(...) and
setattr(settings, entry.attr, parsed) to enforce numeric bounds and invariants
(e.g., ensure BACKUP_RETENTION_DAYS > 0, ensure any *_MIN <= *_MAX and other
domain rules for the specific entry.key values), and if a value fails these
checks skip applying it and emit a logger.warning similar to the existing
malformed branch (include LogFields.CATEGORY "app", LogFields.EVENT
"persisted_setting_invalid", "setting_key": entry.key and LogFields.ERROR
describing the validation failure) so invalid DB rows are ignored instead of
being set at startup.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f333130f-8259-4cc7-b466-2bb955915ca6
📒 Files selected for processing (3)
app/api/v1/admin/backup.pyapp/core/persisted_settings.pytests/api/test_retention_settings_persistence.py
…ettings - Improved the update_retention_settings function to ensure atomicity by staging all changes in a single transaction, preventing partial updates on failure. - Added validation for persisted settings to reject semantically invalid values, enhancing data integrity during loading. - Updated the persist_setting function to support transaction control, allowing for staged writes. - Introduced tests to verify rollback behavior on mid-loop database failures and handling of invalid persisted settings.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/api/v1/admin/backup.py (1)
744-746: Minor: Unused loop variablekey.The
keyvariable is unpacked but not used in this loop (it was used in the persist phase). Use_to indicate intentional discard.🧹 Proposed fix
- for attr, key, value, response_name in pending: + for attr, _, value, response_name in pending: setattr(settings, attr, value) updated_settings[response_name] = value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/v1/admin/backup.py` around lines 744 - 746, The loop unpacks a `key` variable that isn't used; update the unpacking in the loop that iterates over `pending` (the line starting with "for attr, key, value, response_name in pending:") to use `_` instead of `key` (e.g., "for attr, _, value, response_name in pending:") so the unused value is intentionally discarded and clarify intent where `setattr(settings, attr, value)` and `updated_settings[response_name] = value` are performed.app/core/persisted_settings.py (1)
64-102: Consider adding cross-field invariant validation to the registry.The current
validatecallbacks only check individual field constraints (_require_positive_int). Cross-field invariants likeBACKUP_MIN_COUNT <= BACKUP_MAX_COUNTare enforced at the API layer but not duringload_persisted_settings.This means direct DB manipulation could introduce invalid cross-field state that would be loaded on restart. Consider adding a post-load validation phase or per-entry validators that can access the current settings state.
Low priority since the primary write path (the API) already validates these invariants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/core/persisted_settings.py` around lines 64 - 102, The registry currently only runs per-entry validators (PersistedSetting.validate) so load_persisted_settings can load inconsistent cross-field state (e.g., BACKUP_MIN_COUNT > BACKUP_MAX_COUNT); add a post-load invariant check in load_persisted_settings (or extend PersistedSetting to accept a validator that receives the full settings dict) that verifies cross-field rules such as KEY_BACKUP_MIN_COUNT <= KEY_BACKUP_MAX_COUNT and raises a ValidationError or similar when violated; update PERSISTED_SETTINGS handling to either call that new post-load validator or register a cross-field validator to ensure direct DB edits fail on load.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/api/v1/admin/backup.py`:
- Around line 744-746: The loop unpacks a `key` variable that isn't used; update
the unpacking in the loop that iterates over `pending` (the line starting with
"for attr, key, value, response_name in pending:") to use `_` instead of `key`
(e.g., "for attr, _, value, response_name in pending:") so the unused value is
intentionally discarded and clarify intent where `setattr(settings, attr,
value)` and `updated_settings[response_name] = value` are performed.
In `@app/core/persisted_settings.py`:
- Around line 64-102: The registry currently only runs per-entry validators
(PersistedSetting.validate) so load_persisted_settings can load inconsistent
cross-field state (e.g., BACKUP_MIN_COUNT > BACKUP_MAX_COUNT); add a post-load
invariant check in load_persisted_settings (or extend PersistedSetting to accept
a validator that receives the full settings dict) that verifies cross-field
rules such as KEY_BACKUP_MIN_COUNT <= KEY_BACKUP_MAX_COUNT and raises a
ValidationError or similar when violated; update PERSISTED_SETTINGS handling to
either call that new post-load validator or register a cross-field validator to
ensure direct DB edits fail on load.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d69c62af-3a17-43b5-a298-3e21d055dd5c
📒 Files selected for processing (4)
app/api/v1/admin/backup.pyapp/core/persisted_settings.pyapp/crud/system_setting.pytests/api/test_retention_settings_persistence.py
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.
Summary
This pull request introduces robust persistence for admin-configurable settings, ensuring that changes made via the admin API (such as backup retention policies and user registration toggles) are saved to the database and survive application restarts. It adds a new persistence mechanism, updates the relevant API endpoint to use it, and includes comprehensive regression tests to prevent future issues with settings reverting on restart.
Persistence of Admin Settings:
app/core/persisted_settings.pyto manage runtime-persisted admin settings, including serialization, deserialization, and loading/saving to thesystem_settingstable. This ensures settings likeALLOW_USER_REGISTRATIONand retention policies are preserved across restarts.load_persisted_settingsis called to override in-memory defaults with any persisted values from the database, providing seamless restoration of admin preferences.API Endpoint Enhancements:
update_retention_settingsendpoint inapp/api/v1/admin/backup.pyto persist changes to the database using the new persistence mechanism whenever an admin modifies relevant settings. [1] [2] [3] [4] [5] [6]Testing and Reliability:
tests/api/test_retention_settings_persistence.pywith regression tests to ensure settings persist as expected, including tests for correct persistence, restoration on startup, and handling of invalid or malformed data.Related issue
#835
Type of change
Testing
Checklist
npm run lintandnpm run buildpasspytest) if backend code changedconsole.logleft in the difft()translationsSummary by CodeRabbit
New Features
Bug Fixes
Tests