diff --git a/allianceauth/authentication/task_statistics/event_series.py b/allianceauth/authentication/task_statistics/event_series.py index 03865103..9ac1f15d 100644 --- a/allianceauth/authentication/task_statistics/event_series.py +++ b/allianceauth/authentication/task_statistics/event_series.py @@ -5,59 +5,27 @@ import logging from typing import List, Optional from pytz import utc -from redis import Redis, RedisError +from redis import Redis -from allianceauth.utils.cache import get_redis_client +from .helpers import get_redis_client_or_stub logger = logging.getLogger(__name__) -class _RedisStub: - """Stub of a Redis client. - - It's purpose is to prevent EventSeries objects from trying to access Redis - when it is not available. e.g. when the Sphinx docs are rendered by readthedocs.org. - """ - - def delete(self, *args, **kwargs): - pass - - def incr(self, *args, **kwargs): - return 0 - - def zadd(self, *args, **kwargs): - pass - - def zcount(self, *args, **kwargs): - pass - - def zrangebyscore(self, *args, **kwargs): - pass - - class EventSeries: """API for recording and analyzing a series of events.""" _ROOT_KEY = "ALLIANCEAUTH_EVENT_SERIES" - def __init__(self, key_id: str, redis: Redis = None) -> None: - self._redis = get_redis_client() if not redis else redis - try: - if not self._redis.ping(): - raise RuntimeError() - except (AttributeError, RedisError, RuntimeError): - logger.exception( - "Failed to establish a connection with Redis. " - "This EventSeries object is disabled.", - ) - self._redis = _RedisStub() + def __init__(self, key_id: str, redis: Optional[Redis] = None) -> None: + self._redis = get_redis_client_or_stub() if not redis else redis self._key_id = str(key_id) self.clear() @property def is_disabled(self): """True when this object is disabled, e.g. Redis was not available at startup.""" - return isinstance(self._redis, _RedisStub) + return hasattr(self._redis, "IS_STUB") @property def _key_counter(self): @@ -97,7 +65,7 @@ class EventSeries: self._redis.delete(self._key_counter) def count(self, earliest: dt.datetime = None, latest: dt.datetime = None) -> int: - """Count of events, can be restricted to given timeframe. + """Count of events, can be restricted to given time frame. Args: - earliest: Date of first events to count(inclusive), or -infinite if not specified diff --git a/allianceauth/authentication/task_statistics/helpers.py b/allianceauth/authentication/task_statistics/helpers.py index 7b887be7..b75fb39c 100644 --- a/allianceauth/authentication/task_statistics/helpers.py +++ b/allianceauth/authentication/task_statistics/helpers.py @@ -1,21 +1,63 @@ """Helpers for Task Statistics.""" +import logging from typing import Optional +from redis import Redis, RedisError + from django.core.cache import cache +from allianceauth.utils.cache import get_redis_client + +logger = logging.getLogger(__name__) + + +class _RedisStub: + """Stub of a Redis client. + + It's purpose is to prevent EventSeries objects from trying to access Redis + when it is not available. e.g. when the Sphinx docs are rendered by readthedocs.org. + """ + + IS_STUB = True + + def delete(self, *args, **kwargs): + pass + + def incr(self, *args, **kwargs): + return 0 + + def zadd(self, *args, **kwargs): + pass + + def zcount(self, *args, **kwargs): + pass + + def zrangebyscore(self, *args, **kwargs): + pass + class ItemCounter: - """A process safe item counter.""" + """A process safe item counter. + + Args: + - name: Unique name for the counter + - minimum: Counter can not go below the minimum, when set + - redis: A Redis client. Will use AA's cache client by default + """ CACHE_KEY_BASE = "allianceauth-item-counter" DEFAULT_CACHE_TIMEOUT = 24 * 3600 - def __init__(self, name: str) -> None: + def __init__( + self, name: str, minimum: Optional[int] = None, redis: Optional[Redis] = None + ) -> None: if not name: raise ValueError("Must define a name") self._name = str(name) + self._minimum = minimum + self._redis = get_redis_client_or_stub() if not redis else redis @property def _cache_key(self) -> str: @@ -23,7 +65,11 @@ class ItemCounter: def reset(self, init_value: int = 0): """Reset counter to initial value.""" - cache.set(self._cache_key, init_value, self.DEFAULT_CACHE_TIMEOUT) + with self._redis.lock(f"{self.CACHE_KEY_BASE}-reset"): + if self._minimum is not None and init_value < self._minimum: + raise ValueError("Can not reset below minimum") + + cache.set(self._cache_key, init_value, self.DEFAULT_CACHE_TIMEOUT) def incr(self, delta: int = 1): """Increment counter by delta.""" @@ -34,11 +80,29 @@ class ItemCounter: def decr(self, delta: int = 1): """Decrement counter by delta.""" - try: - cache.decr(self._cache_key, delta) - except ValueError: - pass + with self._redis.lock(f"{self.CACHE_KEY_BASE}-decr"): + if self._minimum is not None and self.value() == self._minimum: + return + try: + cache.decr(self._cache_key, delta) + except ValueError: + pass def value(self) -> Optional[int]: """Return current value or None if not yet initialized.""" return cache.get(self._cache_key) + + +def get_redis_client_or_stub() -> Redis: + """Return AA's default cache client or a stub if Redis is not available.""" + redis = get_redis_client() + try: + if not redis.ping(): + raise RuntimeError() + except (AttributeError, RedisError, RuntimeError): + logger.exception( + "Failed to establish a connection with Redis. " + "This EventSeries object is disabled.", + ) + return _RedisStub() + return redis diff --git a/allianceauth/authentication/task_statistics/tests/test_event_series.py b/allianceauth/authentication/task_statistics/tests/test_event_series.py index 70804cf4..17889180 100644 --- a/allianceauth/authentication/task_statistics/tests/test_event_series.py +++ b/allianceauth/authentication/task_statistics/tests/test_event_series.py @@ -1,48 +1,19 @@ import datetime as dt -from unittest.mock import patch from pytz import utc -from redis import RedisError from django.test import TestCase from django.utils.timezone import now from allianceauth.authentication.task_statistics.event_series import ( EventSeries, - _RedisStub, ) +from allianceauth.authentication.task_statistics.helpers import _RedisStub MODULE_PATH = "allianceauth.authentication.task_statistics.event_series" class TestEventSeries(TestCase): - def test_should_abort_without_redis_client(self): - # when - with patch(MODULE_PATH + ".get_redis_client") as mock: - mock.return_value = None - events = EventSeries("dummy") - # then - self.assertTrue(events._redis, _RedisStub) - self.assertTrue(events.is_disabled) - - def test_should_disable_itself_if_redis_not_available_1(self): - # when - with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client: - mock_get_master_client.return_value.ping.side_effect = RedisError - events = EventSeries("dummy") - # then - self.assertIsInstance(events._redis, _RedisStub) - self.assertTrue(events.is_disabled) - - def test_should_disable_itself_if_redis_not_available_2(self): - # when - with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client: - mock_get_master_client.return_value.ping.return_value = False - events = EventSeries("dummy") - # then - self.assertIsInstance(events._redis, _RedisStub) - self.assertTrue(events.is_disabled) - def test_should_add_event(self): # given events = EventSeries("dummy") @@ -166,3 +137,15 @@ class TestEventSeries(TestCase): results = events.all() # then self.assertEqual(len(results), 2) + + def test_should_not_report_as_disabled_when_initialized_normally(self): + # given + events = EventSeries("dummy") + # when/then + self.assertFalse(events.is_disabled) + + def test_should_report_as_disabled_when_initialized_with_redis_stub(self): + # given + events = EventSeries("dummy", redis=_RedisStub()) + # when/then + self.assertTrue(events.is_disabled) diff --git a/allianceauth/authentication/task_statistics/tests/test_helpers.py b/allianceauth/authentication/task_statistics/tests/test_helpers.py new file mode 100644 index 00000000..757ae38e --- /dev/null +++ b/allianceauth/authentication/task_statistics/tests/test_helpers.py @@ -0,0 +1,142 @@ +from unittest import TestCase +from unittest.mock import patch + +from redis import RedisError + +from allianceauth.authentication.task_statistics.helpers import ( + ItemCounter, _RedisStub, get_redis_client_or_stub, +) + +MODULE_PATH = "allianceauth.authentication.task_statistics.helpers" + +COUNTER_NAME = "test-counter" + + +class TestItemCounter(TestCase): + def test_can_create_counter(self): + # when + counter = ItemCounter(COUNTER_NAME) + # then + self.assertIsInstance(counter, ItemCounter) + + def test_can_reset_counter_to_default(self): + # given + counter = ItemCounter(COUNTER_NAME) + # when + counter.reset() + # then + self.assertEqual(counter.value(), 0) + + def test_can_reset_counter_to_custom_value(self): + # given + counter = ItemCounter(COUNTER_NAME) + # when + counter.reset(42) + # then + self.assertEqual(counter.value(), 42) + + def test_can_increment_counter_by_default(self): + # given + counter = ItemCounter(COUNTER_NAME) + counter.reset(0) + # when + counter.incr() + # then + self.assertEqual(counter.value(), 1) + + def test_can_increment_counter_by_custom_value(self): + # given + counter = ItemCounter(COUNTER_NAME) + counter.reset(0) + # when + counter.incr(8) + # then + self.assertEqual(counter.value(), 8) + + def test_can_decrement_counter_by_default(self): + # given + counter = ItemCounter(COUNTER_NAME) + counter.reset(9) + # when + counter.decr() + # then + self.assertEqual(counter.value(), 8) + + def test_can_decrement_counter_by_custom_value(self): + # given + counter = ItemCounter(COUNTER_NAME) + counter.reset(9) + # when + counter.decr(8) + # then + self.assertEqual(counter.value(), 1) + + def test_can_decrement_counter_below_zero(self): + # given + counter = ItemCounter(COUNTER_NAME) + counter.reset(0) + # when + counter.decr(1) + # then + self.assertEqual(counter.value(), -1) + + def test_can_not_decrement_counter_below_minimum(self): + # given + counter = ItemCounter(COUNTER_NAME, minimum=0) + counter.reset(0) + # when + counter.decr(1) + # then + self.assertEqual(counter.value(), 0) + + def test_can_not_reset_counter_below_minimum(self): + # given + counter = ItemCounter(COUNTER_NAME, minimum=0) + # when/then + with self.assertRaises(ValueError): + counter.reset(-1) + + def test_can_not_init_without_name(self): + # when/then + with self.assertRaises(ValueError): + ItemCounter(name="") + + def test_can_ignore_invalid_values_when_incrementing(self): + # given + counter = ItemCounter(COUNTER_NAME) + counter.reset(0) + # when + with patch(MODULE_PATH + ".cache.incr") as m: + m.side_effect = ValueError + counter.incr() + # then + self.assertEqual(counter.value(), 0) + + def test_can_ignore_invalid_values_when_decrementing(self): + # given + counter = ItemCounter(COUNTER_NAME) + counter.reset(1) + # when + with patch(MODULE_PATH + ".cache.decr") as m: + m.side_effect = ValueError + counter.decr() + # then + self.assertEqual(counter.value(), 1) + + +class TestGetRedisClient(TestCase): + def test_should_return_mock_if_redis_not_available_1(self): + # when + with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client: + mock_get_master_client.return_value.ping.side_effect = RedisError + result = get_redis_client_or_stub() + # then + self.assertIsInstance(result, _RedisStub) + + def test_should_return_mock_if_redis_not_available_2(self): + # when + with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client: + mock_get_master_client.return_value.ping.return_value = False + result = get_redis_client_or_stub() + # then + self.assertIsInstance(result, _RedisStub) diff --git a/allianceauth/authentication/task_statistics/tests/test_item_counter.py b/allianceauth/authentication/task_statistics/tests/test_item_counter.py deleted file mode 100644 index da6f49c1..00000000 --- a/allianceauth/authentication/task_statistics/tests/test_item_counter.py +++ /dev/null @@ -1,74 +0,0 @@ -from unittest import TestCase - -from allianceauth.authentication.task_statistics.helpers import ItemCounter - -COUNTER_NAME = "test-counter" - - -class TestItemCounter(TestCase): - def test_can_create_counter(self): - # when - counter = ItemCounter(COUNTER_NAME) - # then - self.assertIsInstance(counter, ItemCounter) - - def test_can_reset_counter_to_default(self): - # given - counter = ItemCounter(COUNTER_NAME) - # when - counter.reset() - # then - self.assertEqual(counter.value(), 0) - - def test_can_reset_counter_to_custom_value(self): - # given - counter = ItemCounter(COUNTER_NAME) - # when - counter.reset(42) - # then - self.assertEqual(counter.value(), 42) - - def test_can_increment_counter_by_default(self): - # given - counter = ItemCounter(COUNTER_NAME) - counter.reset(0) - # when - counter.incr() - # then - self.assertEqual(counter.value(), 1) - - def test_can_increment_counter_by_custom_value(self): - # given - counter = ItemCounter(COUNTER_NAME) - counter.reset(0) - # when - counter.incr(8) - # then - self.assertEqual(counter.value(), 8) - - def test_can_decrement_counter_by_default(self): - # given - counter = ItemCounter(COUNTER_NAME) - counter.reset(9) - # when - counter.decr() - # then - self.assertEqual(counter.value(), 8) - - def test_can_decrement_counter_by_custom_value(self): - # given - counter = ItemCounter(COUNTER_NAME) - counter.reset(9) - # when - counter.decr(8) - # then - self.assertEqual(counter.value(), 1) - - def test_can_decrement_counter_below_zero(self): - # given - counter = ItemCounter(COUNTER_NAME) - counter.reset(0) - # when - counter.decr(1) - # then - self.assertEqual(counter.value(), -1)