From 46d9c3822ab73dcf8852815379ed00b40bf555cb Mon Sep 17 00:00:00 2001 From: imperosol Date: Sat, 20 Dec 2025 09:04:18 +0100 Subject: [PATCH] remove cache calls to fetch user membership --- club/models.py | 53 ++++++++--------------------------- club/tests/test_membership.py | 37 ------------------------ core/tests/test_core.py | 21 +------------- 3 files changed, 12 insertions(+), 99 deletions(-) diff --git a/club/models.py b/club/models.py index 3c0f720f..fd9d5f83 100644 --- a/club/models.py +++ b/club/models.py @@ -26,7 +26,6 @@ from __future__ import annotations from typing import Iterable, Self from django.conf import settings -from django.core.cache import cache from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.core.validators import RegexValidator, validate_email from django.db import models, transaction @@ -187,9 +186,6 @@ class Club(models.Model): self.page.save(force_lock=True) def delete(self, *args, **kwargs) -> tuple[int, dict[str, int]]: - # Invalidate the cache of this club and of its memberships - for membership in self.members.ongoing().select_related("user"): - cache.delete(f"membership_{self.id}_{membership.user.id}") self.board_group.delete() self.members_group.delete() return super().delete(*args, **kwargs) @@ -210,24 +206,15 @@ class Club(models.Model): """Method to see if that object can be edited by the given user.""" return self.has_rights_in_club(user) - def get_membership_for(self, user: User) -> Membership | None: - """Return the current membership the given user. + @cached_property + def current_members(self) -> list[Membership]: + return self.members.ongoing().select_related("user").order_by("-role") - Note: - The result is cached. - """ + def get_membership_for(self, user: User) -> Membership | None: + """Return the current membership of the given user.""" if user.is_anonymous: return None - membership = cache.get(f"membership_{self.id}_{user.id}") - if membership == "not_member": - return None - if membership is None: - membership = self.members.filter(user=user, end_date=None).first() - if membership is None: - cache.set(f"membership_{self.id}_{user.id}", "not_member") - else: - cache.set(f"membership_{self.id}_{user.id}", membership) - return membership + return next((m for m in self.current_members if m.user_id == user.id), None) def has_rights_in_club(self, user: User) -> bool: return user.is_in_group(pk=self.board_group_id) @@ -295,28 +282,20 @@ class MembershipQuerySet(models.QuerySet): and add them in the club groups they should be in. Be aware that this adds three db queries : - one to retrieve the updated memberships, - one to perform group removal and one to perform - group attribution. + + - one to retrieve the updated memberships + - one to perform group removal + - and one to perform group attribution. """ nb_rows = super().update(**kwargs) if nb_rows == 0: - # if no row was affected, no need to refresh the cache + # if no row was affected, no need to edit club groups return 0 - cache_memberships = {} memberships = set(self.select_related("club")) # delete all User-Group relations and recreate the necessary ones - # It's more concise to write and more reliable Membership._remove_club_groups(memberships) Membership._add_club_groups(memberships) - for member in memberships: - cache_key = f"membership_{member.club_id}_{member.user_id}" - if member.end_date is None: - cache_memberships[cache_key] = member - else: - cache_memberships[cache_key] = "not_member" - cache.set_many(cache_memberships) return nb_rows def delete(self) -> tuple[int, dict[str, int]]: @@ -339,12 +318,6 @@ class MembershipQuerySet(models.QuerySet): nb_rows, rows_counts = super().delete() if nb_rows > 0: Membership._remove_club_groups(memberships) - cache.set_many( - { - f"membership_{m.club_id}_{m.user_id}": "not_member" - for m in memberships - } - ) return nb_rows, rows_counts @@ -408,9 +381,6 @@ class Membership(models.Model): self._remove_club_groups([self]) if self.end_date is None: self._add_club_groups([self]) - cache.set(f"membership_{self.club_id}_{self.user_id}", self) - else: - cache.set(f"membership_{self.club_id}_{self.user_id}", "not_member") def get_absolute_url(self): return reverse("club:club_members", kwargs={"club_id": self.club_id}) @@ -431,7 +401,6 @@ class Membership(models.Model): def delete(self, *args, **kwargs): self._remove_club_groups([self]) super().delete(*args, **kwargs) - cache.delete(f"membership_{self.club_id}_{self.user_id}") @staticmethod def _remove_club_groups( diff --git a/club/tests/test_membership.py b/club/tests/test_membership.py index 2420043d..e24fe9d0 100644 --- a/club/tests/test_membership.py +++ b/club/tests/test_membership.py @@ -72,25 +72,6 @@ class TestMembershipQuerySet(TestClub): expected.sort(key=lambda i: i.id) assert members == expected - def test_update_invalidate_cache(self): - """Test that the `update` queryset method properly invalidate cache.""" - mem_skia = self.simple_board_member.memberships.get(club=self.club) - cache.set(f"membership_{mem_skia.club_id}_{mem_skia.user_id}", mem_skia) - self.simple_board_member.memberships.update(end_date=localtime(now()).date()) - assert ( - cache.get(f"membership_{mem_skia.club_id}_{mem_skia.user_id}") - == "not_member" - ) - - mem_richard = self.richard.memberships.get(club=self.club) - cache.set( - f"membership_{mem_richard.club_id}_{mem_richard.user_id}", mem_richard - ) - self.richard.memberships.update(role=5) - new_mem = self.richard.memberships.get(club=self.club) - assert new_mem != "not_member" - assert new_mem.role == 5 - def test_update_change_club_groups(self): """Test that `update` set the user groups accordingly.""" user = baker.make(User) @@ -112,24 +93,6 @@ class TestMembershipQuerySet(TestClub): assert not user.groups.contains(members_group) assert not user.groups.contains(board_group) - def test_delete_invalidate_cache(self): - """Test that the `delete` queryset properly invalidate cache.""" - mem_skia = self.simple_board_member.memberships.get(club=self.club) - mem_comptable = self.president.memberships.get(club=self.club) - cache.set(f"membership_{mem_skia.club_id}_{mem_skia.user_id}", mem_skia) - cache.set( - f"membership_{mem_comptable.club_id}_{mem_comptable.user_id}", mem_comptable - ) - - # should delete the subscriptions of simple_board_member and president - self.club.members.ongoing().board().delete() - - for membership in (mem_skia, mem_comptable): - cached_mem = cache.get( - f"membership_{membership.club_id}_{membership.user_id}" - ) - assert cached_mem == "not_member" - def test_delete_remove_from_groups(self): """Test that `delete` removes from club groups""" user = baker.make(User) diff --git a/core/tests/test_core.py b/core/tests/test_core.py index 01419621..631e5b51 100644 --- a/core/tests/test_core.py +++ b/core/tests/test_core.py @@ -22,19 +22,17 @@ from bs4 import BeautifulSoup from django.contrib.auth.hashers import make_password from django.contrib.auth.models import Permission from django.core import mail -from django.core.cache import cache from django.core.exceptions import ValidationError from django.core.mail import EmailMessage from django.test import Client, RequestFactory, TestCase from django.urls import reverse -from django.utils.timezone import now from django.views.generic import View from django.views.generic.base import ContextMixin from model_bakery import baker from pytest_django.asserts import assertInHTML, assertRedirects from antispam.models import ToxicDomain -from club.models import Club, Membership +from club.models import Club from core.baker_recipes import subscriber_user from core.markdown import markdown from core.models import AnonymousUser, Group, Page, User, validate_promo @@ -436,23 +434,6 @@ class TestUserIsInGroup(TestCase): with self.assertNumQueries(0): self.public_user.is_in_group(pk=group_not_in.id) - def test_cache_properly_cleared_membership(self): - """Test that when the membership of a user end, - the cache is properly invalidated. - """ - membership = baker.make(Membership, club=self.club, user=self.public_user) - cache.clear() - self.club.get_membership_for(self.public_user) # this should populate the cache - assert membership == cache.get( - f"membership_{self.club.id}_{self.public_user.id}" - ) - membership.end_date = now() - timedelta(minutes=5) - membership.save() - cached_membership = cache.get( - f"membership_{self.club.id}_{self.public_user.id}" - ) - assert cached_membership == "not_member" - def test_not_existing_group(self): """Test that searching for a not existing group returns False.