From 166766ebee4fabba8ab9da22f7621d793d46ea2c Mon Sep 17 00:00:00 2001 From: Pelle Koster Date: Mon, 9 Dec 2024 13:59:12 +0100 Subject: [PATCH 1/5] improve preferences type hinting --- src/argus/auth/models.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/argus/auth/models.py b/src/argus/auth/models.py index cc2064011..356cc7992 100644 --- a/src/argus/auth/models.py +++ b/src/argus/auth/models.py @@ -1,5 +1,7 @@ +from __future__ import annotations + import functools -from typing import Any, Optional, Union, Protocol +from typing import Any, Dict, Optional, Type, Union, Protocol from django.contrib.auth.models import AbstractUser, Group from django.db import models @@ -198,7 +200,7 @@ def update_context(self, context: dict[str, Any]) -> dict[str, Any]: pass -class Preferences(models.Model): +class Preferences(models.Model, PreferencesBase): class Meta: verbose_name = "User Preferences" verbose_name_plural = "Users' Preferences" @@ -206,7 +208,7 @@ class Meta: models.UniqueConstraint(name="unique_preference", fields=["user", "namespace"]), ] - NAMESPACES = {} + NAMESPACES: Dict[str, Type[Preferences]] = {} user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="preferences") namespace = models.CharField(blank=False, max_length=255) @@ -215,15 +217,15 @@ class Meta: objects = PreferencesManager() unregistered = UnregisteredPreferencesManager() - # storage for field forms in preference - FORMS = None - # storage for field defaults in preference - _FIELD_DEFAULTS = None - # django methods # called when subclass is constructing itself def __init_subclass__(cls, **kwargs): + assert isinstance(getattr(cls, "FORMS", None), dict), f"{cls.__name__}.FORMS must be a dictionary" + assert isinstance( + getattr(cls, "_FIELD_DEFAULTS", None), dict + ), f"{cls.__name__}._FIELD_DEFAULTS must be a dictionary" + super().__init_subclass__(**kwargs) cls.NAMESPACES[cls._namespace] = cls From 706ca2b9fd36dad5ec3aeb47fd4544b9b13ffaf3 Mon Sep 17 00:00:00 2001 From: Pelle Koster Date: Tue, 10 Dec 2024 15:10:38 +0100 Subject: [PATCH 2/5] limit database round trips --- src/argus/auth/models.py | 47 +++++++++++++++++++++++---------------- tests/auth/test_models.py | 17 +++++++++----- 2 files changed, 39 insertions(+), 25 deletions(-) diff --git a/src/argus/auth/models.py b/src/argus/auth/models.py index 356cc7992..2b40353fd 100644 --- a/src/argus/auth/models.py +++ b/src/argus/auth/models.py @@ -1,7 +1,7 @@ from __future__ import annotations import functools -from typing import Any, Dict, Optional, Type, Union, Protocol +from typing import Any, Dict, List, Optional, Type, Union, Protocol from django.contrib.auth.models import AbstractUser, Group from django.db import models @@ -94,19 +94,23 @@ def is_used(self): # user is not considered in use return False - def get_or_create_preferences(self): - Preferences.ensure_for_user(self) - return self.preferences.filter(namespace__in=Preferences.NAMESPACES) + def get_or_create_preferences(self) -> List[Preferences]: + return Preferences.ensure_for_user(self) def get_preferences_context(self): pref_sets = self.get_or_create_preferences() prefdict = {} for pref_set in pref_sets: - prefdict[pref_set._namespace] = pref_set.get_context() + prefdict[pref_set.namespace] = pref_set.get_context() return prefdict def get_namespaced_preferences(self, namespace): - return self.get_or_create_preferences().get(namespace=namespace) + print("here", namespace) + obj, _ = Preferences.objects.get_or_create(user=self, namespace=namespace) + if not obj.preferences and (defaults := obj.get_defaults()): + obj.preferences = defaults + obj.save() + return obj class PreferencesManager(models.Manager): @@ -116,13 +120,6 @@ def get_queryset(self): def get_by_natural_key(self, user, namespace): return self.get(user=user, namespace=namespace) - def create_missing_preferences(self): - precount = Preferences.objects.count() - for namespace, subclass in Preferences.NAMESPACES.items(): - for user in User.objects.all(): - Preferences.ensure_for_user(user) - return (precount, Preferences.objects.count()) - def get_all_defaults(self): prefdict = {} for namespace, subclass in Preferences.NAMESPACES.items(): @@ -200,7 +197,7 @@ def update_context(self, context: dict[str, Any]) -> dict[str, Any]: pass -class Preferences(models.Model, PreferencesBase): +class Preferences(models.Model): class Meta: verbose_name = "User Preferences" verbose_name_plural = "Users' Preferences" @@ -217,6 +214,10 @@ class Meta: objects = PreferencesManager() unregistered = UnregisteredPreferencesManager() + # must be set by the subclasses + FORMS: dict[str, forms.Form] + _FIELD_DEFAULTS: dict[str, Any] + # django methods # called when subclass is constructing itself @@ -252,12 +253,20 @@ def get_defaults(cls): return cls._FIELD_DEFAULTS.copy() if cls._FIELD_DEFAULTS else {} @classmethod - def ensure_for_user(cls, user): + def ensure_for_user(cls, user) -> List[Preferences]: + all_preferences = {p.namespace: p for p in user.preferences.all()} + valid_preferences = [] + for namespace, subclass in cls.NAMESPACES.items(): - obj, _ = subclass.objects.get_or_create(user=user, namespace=namespace) - if not obj.preferences and (defaults := subclass.get_defaults()): - obj.preferences = defaults - obj.save() + if namespace in all_preferences: + valid_preferences.append(all_preferences[namespace]) + continue + obj = subclass.objects.create(user=user, namespace=namespace) + obj.preferences = subclass.get_defaults() + obj.save() + valid_preferences.append(obj) + + return valid_preferences def update_context(self, context): "Override this to change what is put in context" diff --git a/tests/auth/test_models.py b/tests/auth/test_models.py index 9fc501f03..f31b79246 100644 --- a/tests/auth/test_models.py +++ b/tests/auth/test_models.py @@ -70,7 +70,9 @@ def test_is_not_dormant_if_actor_of_event(self): class UserMiscMethodTests(TestCase): - def test_get_preferences_context_returns_dict_of_all_properly_registered_preferences(self): + def test_get_preferences_context_returns_dict_of_all_properly_registered_preferences( + self, + ): user = PersonUserFactory() results = user.get_preferences_context() @@ -138,6 +140,12 @@ def test_get_instance_converts_to_subclass(self): instance = instances[0] self.assertIsInstance(instance, MyPreferences) + def test_get_namespaced_preferences_creates_preferences_with_defaults(self): + user = PersonUserFactory() + pref_set = user.get_namespaced_preferences(namespace=MyPreferences._namespace) + self.assertIsInstance(pref_set, MyPreferences) + self.assertEqual(pref_set.preferences, {"magic_number": 42}) + def test_vanilla_get_context_dumps_preferences_field_including_defaults(self): user = PersonUserFactory() @@ -236,18 +244,15 @@ def test_get_by_natural_key_fetches_preference_of_correct_namespace(self): result = Preferences.objects.get_by_natural_key(user, MyPreferences._namespace) self.assertEqual(prefs, result) - def test_create_missing_preferences_creates_all_namespaces_for_all_users(self): + def test_get_or_create_preferences_creates_all_namespaces_for_user(self): user1 = PersonUserFactory() - user2 = PersonUserFactory() # no preferences yet self.assertFalse(Preferences.objects.filter(user=user1).exists()) - self.assertFalse(Preferences.objects.filter(user=user2).exists()) - Preferences.objects.create_missing_preferences() + user1.get_or_create_preferences() # three each (two from tests, one from app) self.assertEqual(Preferences.objects.filter(user=user1).count(), 3) - self.assertEqual(Preferences.objects.filter(user=user2).count(), 3) def test_get_all_defaults_returns_all_prefs_defaults(self): defaults = Preferences.objects.get_all_defaults() From 3ad58a0d9e106c38de72ccceb42701ca4c5897d8 Mon Sep 17 00:00:00 2001 From: Pelle Koster Date: Wed, 11 Dec 2024 07:52:28 +0100 Subject: [PATCH 3/5] further reduce database queries to preferences --- src/argus/auth/models.py | 6 +----- src/argus/auth/utils.py | 14 ++++++++------ src/argus/htmx/dateformat/views.py | 4 ++-- src/argus/htmx/incidents/views.py | 8 +++----- src/argus/htmx/themes/views.py | 4 ++-- src/argus/htmx/user/views.py | 4 ++-- tests/auth/test_models.py | 10 ---------- 7 files changed, 18 insertions(+), 32 deletions(-) diff --git a/src/argus/auth/models.py b/src/argus/auth/models.py index 2b40353fd..e0f542f20 100644 --- a/src/argus/auth/models.py +++ b/src/argus/auth/models.py @@ -94,18 +94,14 @@ def is_used(self): # user is not considered in use return False - def get_or_create_preferences(self) -> List[Preferences]: - return Preferences.ensure_for_user(self) - def get_preferences_context(self): - pref_sets = self.get_or_create_preferences() + pref_sets = Preferences.ensure_for_user(self) prefdict = {} for pref_set in pref_sets: prefdict[pref_set.namespace] = pref_set.get_context() return prefdict def get_namespaced_preferences(self, namespace): - print("here", namespace) obj, _ = Preferences.objects.get_or_create(user=self, namespace=namespace) if not obj.preferences and (defaults := obj.get_defaults()): obj.preferences = defaults diff --git a/src/argus/auth/utils.py b/src/argus/auth/utils.py index ad383aaa2..50b75566b 100644 --- a/src/argus/auth/utils.py +++ b/src/argus/auth/utils.py @@ -1,5 +1,6 @@ from copy import deepcopy import logging +from typing import Any, Tuple from django.conf import settings from django.contrib.auth.backends import ModelBackend, RemoteUserBackend @@ -57,10 +58,11 @@ def get_preference(request, namespace, preference): return prefs.get_preference(preference) -def save_preference(request, data, namespace, preference): +def get_or_update_preference(request, data, namespace, preference) -> Tuple[Any, bool]: """Save the single preference given in data to the given namespace - Returns True on success, otherwise False + Returns a tuple (value, success) with value the value of the preference and succces a boolean + indicating wether the preference was succesfully updated """ prefs = get_preference_obj(request, namespace) value = prefs.get_preference(preference) @@ -68,21 +70,21 @@ def save_preference(request, data, namespace, preference): if not data.get(preference, None): LOG.debug("Failed to change %s, not in input: %s", preference, data) - return False + return value, False form = prefs.FORMS[preference](data) if not form.is_valid(): messages.warning(request, f"Failed to change {preference}, invalid input") LOG.warning("Failed to change %s, invalid input: %s", preference, data) - return False + return value, False old_value = deepcopy(value) # Just in case value is mutable.. value = form.cleaned_data[preference] if value == old_value: LOG.debug("Did not change %s: no change", preference) - return False + return value, False prefs.save_preference(preference, value) messages.success(request, f"Changed {preference}: {old_value} → {value}") LOG.info("Changed %s: %s → %s", preference, old_value, value) - return True + return value, True diff --git a/src/argus/htmx/dateformat/views.py b/src/argus/htmx/dateformat/views.py index 3983095c7..2bfb3078b 100644 --- a/src/argus/htmx/dateformat/views.py +++ b/src/argus/htmx/dateformat/views.py @@ -6,7 +6,7 @@ from django.http import HttpResponse from django_htmx.http import HttpResponseClientRefresh -from argus.auth.utils import save_preference +from argus.auth.utils import get_or_update_preference from argus.htmx.incidents.views import HtmxHttpRequest from .constants import DATETIME_FORMATS @@ -22,5 +22,5 @@ def dateformat_names(request: HtmxHttpRequest) -> HttpResponse: @require_POST def change_dateformat(request: HtmxHttpRequest) -> HttpResponse: - save_preference(request, request.POST, "argus_htmx", "datetime_format_name") + get_or_update_preference(request, request.POST, "argus_htmx", "datetime_format_name") return HttpResponseClientRefresh() diff --git a/src/argus/htmx/incidents/views.py b/src/argus/htmx/incidents/views.py index 26708b920..c59a172f4 100644 --- a/src/argus/htmx/incidents/views.py +++ b/src/argus/htmx/incidents/views.py @@ -13,7 +13,7 @@ from django_htmx.middleware import HtmxDetails from django_htmx.http import HttpResponseClientRefresh -from argus.auth.utils import get_preference, save_preference +from argus.auth.utils import get_or_update_preference from argus.incident.models import Incident from argus.util.datetime_utils import make_aware @@ -117,10 +117,8 @@ def incident_list(request: HtmxHttpRequest) -> HttpResponse: # Standard Django pagination - page_size = get_preference(request, "argus_htmx", "page_size") - success = save_preference(request, request.GET, "argus_htmx", "page_size") - if success: - page_size = get_preference(request, "argus_htmx", "page_size") + page_size, _ = get_or_update_preference(request, request.GET, "argus_htmx", "page_size") + paginator = Paginator(object_list=qs, per_page=page_size) page_num = params.pop("page", "1") page = paginator.get_page(page_num) diff --git a/src/argus/htmx/themes/views.py b/src/argus/htmx/themes/views.py index 15df291bf..87621fc0e 100644 --- a/src/argus/htmx/themes/views.py +++ b/src/argus/htmx/themes/views.py @@ -6,7 +6,7 @@ from django.http import HttpResponse from django_htmx.http import HttpResponseClientRefresh -from argus.auth.utils import save_preference +from argus.auth.utils import get_or_update_preference from argus.htmx.constants import THEME_NAMES from argus.htmx.incidents.views import HtmxHttpRequest @@ -23,7 +23,7 @@ def theme_names(request: HtmxHttpRequest) -> HttpResponse: @require_POST def change_theme(request: HtmxHttpRequest) -> HttpResponse: - success = save_preference(request, request.POST, "argus_htmx", "theme") + success = get_or_update_preference(request, request.POST, "argus_htmx", "theme") if success: return render(request, "htmx/themes/_current_theme.html") return HttpResponseClientRefresh() diff --git a/src/argus/htmx/user/views.py b/src/argus/htmx/user/views.py index 4eb0832be..fb54d2145 100644 --- a/src/argus/htmx/user/views.py +++ b/src/argus/htmx/user/views.py @@ -3,7 +3,7 @@ from django.views.decorators.http import require_GET, require_POST from django_htmx.http import HttpResponseClientRefresh -from argus.auth.utils import save_preference +from argus.auth.utils import get_or_update_preference from argus.htmx.constants import ALLOWED_PAGE_SIZES from argus.htmx.incidents.views import HtmxHttpRequest @@ -17,7 +17,7 @@ def page_size_names(request: HtmxHttpRequest) -> HttpResponse: @require_POST def change_page_size(request: HtmxHttpRequest) -> HttpResponse: - save_preference(request, request.POST, "argus_htmx", "page_size") + get_or_update_preference(request, request.POST, "argus_htmx", "page_size") return HttpResponseClientRefresh() diff --git a/tests/auth/test_models.py b/tests/auth/test_models.py index f31b79246..8570d48c9 100644 --- a/tests/auth/test_models.py +++ b/tests/auth/test_models.py @@ -244,16 +244,6 @@ def test_get_by_natural_key_fetches_preference_of_correct_namespace(self): result = Preferences.objects.get_by_natural_key(user, MyPreferences._namespace) self.assertEqual(prefs, result) - def test_get_or_create_preferences_creates_all_namespaces_for_user(self): - user1 = PersonUserFactory() - - # no preferences yet - self.assertFalse(Preferences.objects.filter(user=user1).exists()) - - user1.get_or_create_preferences() - # three each (two from tests, one from app) - self.assertEqual(Preferences.objects.filter(user=user1).count(), 3) - def test_get_all_defaults_returns_all_prefs_defaults(self): defaults = Preferences.objects.get_all_defaults() self.assertIsInstance(defaults, dict) From 0dbecf85a387e442dc09c8a09ecd2e814b2e152d Mon Sep 17 00:00:00 2001 From: Pelle Koster Date: Wed, 11 Dec 2024 09:08:54 +0100 Subject: [PATCH 4/5] process feedback and cleanup --- src/argus/auth/models.py | 4 ++-- src/argus/auth/utils.py | 4 ++-- src/argus/htmx/themes/views.py | 2 +- tests/auth/test_models.py | 4 +--- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/argus/auth/models.py b/src/argus/auth/models.py index e0f542f20..15068b191 100644 --- a/src/argus/auth/models.py +++ b/src/argus/auth/models.py @@ -1,7 +1,7 @@ from __future__ import annotations import functools -from typing import Any, Dict, List, Optional, Type, Union, Protocol +from typing import Any, List, Optional, Type, Union, Protocol from django.contrib.auth.models import AbstractUser, Group from django.db import models @@ -201,7 +201,7 @@ class Meta: models.UniqueConstraint(name="unique_preference", fields=["user", "namespace"]), ] - NAMESPACES: Dict[str, Type[Preferences]] = {} + NAMESPACES: dict[str, Type[Preferences]] = {} user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="preferences") namespace = models.CharField(blank=False, max_length=255) diff --git a/src/argus/auth/utils.py b/src/argus/auth/utils.py index 50b75566b..5fcad5891 100644 --- a/src/argus/auth/utils.py +++ b/src/argus/auth/utils.py @@ -61,8 +61,8 @@ def get_preference(request, namespace, preference): def get_or_update_preference(request, data, namespace, preference) -> Tuple[Any, bool]: """Save the single preference given in data to the given namespace - Returns a tuple (value, success) with value the value of the preference and succces a boolean - indicating wether the preference was succesfully updated + Returns a tuple (value, success). Value is the value of the preference, and success a boolean + indicating whether the preference was successfully updated """ prefs = get_preference_obj(request, namespace) value = prefs.get_preference(preference) diff --git a/src/argus/htmx/themes/views.py b/src/argus/htmx/themes/views.py index 87621fc0e..091ae1e3a 100644 --- a/src/argus/htmx/themes/views.py +++ b/src/argus/htmx/themes/views.py @@ -23,7 +23,7 @@ def theme_names(request: HtmxHttpRequest) -> HttpResponse: @require_POST def change_theme(request: HtmxHttpRequest) -> HttpResponse: - success = get_or_update_preference(request, request.POST, "argus_htmx", "theme") + _, success = get_or_update_preference(request, request.POST, "argus_htmx", "theme") if success: return render(request, "htmx/themes/_current_theme.html") return HttpResponseClientRefresh() diff --git a/tests/auth/test_models.py b/tests/auth/test_models.py index 8570d48c9..cf4318607 100644 --- a/tests/auth/test_models.py +++ b/tests/auth/test_models.py @@ -70,9 +70,7 @@ def test_is_not_dormant_if_actor_of_event(self): class UserMiscMethodTests(TestCase): - def test_get_preferences_context_returns_dict_of_all_properly_registered_preferences( - self, - ): + def test_get_preferences_context_returns_dict_of_all_properly_registered_preferences(self): user = PersonUserFactory() results = user.get_preferences_context() From f8ea8cb8b0c8f43d1c5171772675ba97971493cd Mon Sep 17 00:00:00 2001 From: Pelle Koster Date: Wed, 11 Dec 2024 10:06:12 +0100 Subject: [PATCH 5/5] add changelog fragment --- changelog.d/1082.changed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/1082.changed.md diff --git a/changelog.d/1082.changed.md b/changelog.d/1082.changed.md new file mode 100644 index 000000000..3a05bdf05 --- /dev/null +++ b/changelog.d/1082.changed.md @@ -0,0 +1 @@ +argus.htmx: reduce number of queries to preferences db table