mirror of
https://github.com/retspen/webvirtcloud
synced 2026-11-10 12:16:49 +00:00
fix(appsettings,vrtManager,storages): error handlers that hid the real failure
Reviewing the error handlers outside instances/ turned up three that masked or swallowed the failure they caught: - appsettings/settings.get_settings() runs on every request (middleware) and loads app_settings from the database. Its bare `except: pass` left `entries` unbound on a database error, so the next line raised UnboundLocalError instead; it also caught KeyboardInterrupt/SystemExit. It now catches DatabaseError and returns, keeping the already-loaded values. - vrtManager.network.refresh_dhcp_leases raised a formatted string (`raise f"..."`), which Python rejects with TypeError, hiding the real DHCPLeases failure. It now raises util.OperationError, which the API/HTML error handling already renders as a libvirt error, and clears self.leases. - storages.views.get_volumes caught libvirtError and answered 200 with an empty body, so the caller could not tell a failed pool read from an empty pool. It now returns 400 with the error. The other broad handlers reviewed (appsettings/nwfilters/virtsecrets views, storages/upload cleanup and recovery paths, vrtManager display getters) report the error or are best-effort and were left as they are. Tests: get_settings() survives a database error during the query and during iteration without raising and still loads a real value; refresh_dhcp_leases raises OperationError (keeping the cause) not TypeError; get_volumes returns 400 on a listing failure and the sorted volumes on success. The tests fail on the previous code.
This commit is contained in:
parent
80e11f5eba
commit
e84db7542d
6 changed files with 111 additions and 7 deletions
|
|
@ -1,3 +1,5 @@
|
|||
from django.db import DatabaseError
|
||||
|
||||
from .models import AppSettings
|
||||
|
||||
|
||||
|
|
@ -10,9 +12,8 @@ app_settings = Settings()
|
|||
|
||||
def get_settings():
|
||||
try:
|
||||
entries = AppSettings.objects.all()
|
||||
except:
|
||||
pass
|
||||
|
||||
entries = list(AppSettings.objects.all())
|
||||
except DatabaseError:
|
||||
return # keep the values already loaded; a failed read must not crash the request
|
||||
for entry in entries:
|
||||
setattr(app_settings, entry.key, entry.value)
|
||||
|
|
|
|||
36
appsettings/test_get_settings.py
Normal file
36
appsettings/test_get_settings.py
Normal file
|
|
@ -0,0 +1,36 @@
|
|||
"""get_settings() runs on every request (middleware). A database error must not
|
||||
leave the loop reading an unbound variable (the old bare except did), and must
|
||||
not crash the request chain over a transient read."""
|
||||
|
||||
from unittest.mock import patch
|
||||
|
||||
from django.db import DatabaseError
|
||||
from django.test import TestCase
|
||||
|
||||
from appsettings.models import AppSettings
|
||||
from appsettings.settings import app_settings, get_settings
|
||||
|
||||
|
||||
class GetSettingsTestCase(TestCase):
|
||||
def test_a_database_error_does_not_raise(self):
|
||||
app_settings.MARKER = "kept"
|
||||
with patch("appsettings.settings.AppSettings.objects") as objects:
|
||||
objects.all.side_effect = DatabaseError("db down")
|
||||
get_settings() # must not raise UnboundLocalError or DatabaseError
|
||||
self.assertEqual(app_settings.MARKER, "kept")
|
||||
|
||||
def test_settings_are_loaded_when_the_database_works(self):
|
||||
AppSettings.objects.update_or_create(key="TEST_MARKER_KEY", defaults={"name": "m", "value": "from-db"})
|
||||
try:
|
||||
get_settings()
|
||||
self.assertEqual(app_settings.TEST_MARKER_KEY, "from-db")
|
||||
finally:
|
||||
if hasattr(app_settings, "TEST_MARKER_KEY"):
|
||||
delattr(app_settings, "TEST_MARKER_KEY")
|
||||
|
||||
def test_a_query_that_fails_during_iteration_does_not_raise(self):
|
||||
app_settings.MARKER2 = "kept"
|
||||
with patch("appsettings.settings.AppSettings.objects") as objects:
|
||||
objects.all.return_value.__iter__.side_effect = DatabaseError("cursor died")
|
||||
get_settings()
|
||||
self.assertEqual(app_settings.MARKER2, "kept")
|
||||
Loading…
Add table
Add a link
Reference in a new issue