diff --git a/CHANGELOG.md b/CHANGELOG.md index 11ade075..f393496f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to ### Changed - Restrict dashboards access to instructors and administrators +- Change cache key to indicator attributes instead of LRS query parameters ### Fixed diff --git a/src/api/core/warren/indicators/mixins.py b/src/api/core/warren/indicators/mixins.py index 8e515a94..7a25fa7e 100644 --- a/src/api/core/warren/indicators/mixins.py +++ b/src/api/core/warren/indicators/mixins.py @@ -2,6 +2,7 @@ import hashlib import inspect +import json import logging from abc import ABC, abstractmethod from functools import cached_property, reduce @@ -44,10 +45,6 @@ class Cacheable(Protocol): """A protocol defining the structure for cacheable objects.""" - def get_lrs_query(self): - """Get the LRS query for fetching statements.""" - ... - async def compute(self): """Perform operations to get a computed value.""" ... @@ -67,17 +64,17 @@ def db_session(self) -> Session: def cache_key(self) -> str: """Calculate the indicator cache key. - The cache key is composed of the class name and a hexadecimal digest - of the LRS query hash (excluding the since and until fields if present), - e.g. fooindicator-44709d6fcb83d92a76dcb0b668c98e1b1d3dafe7. + The cache key is composed of the class name and a hexadecimal digest of the + instance attributes, e.g. + fooindicator-44709d6fcb83d92a76dcb0b668c98e1b1d3dafe7. + + Note that instance attributes need to be serializable, or at least implement the + __str__ method. """ - lrs_query_hash = hashlib.sha256( - self.get_lrs_query() - .json(exclude={"since", "until"}, exclude_none=True, exclude_unset=True) - .encode() - ).hexdigest() - return f"{self.__class__.__name__.lower()}-{lrs_query_hash}" + attributes = json.dumps(vars(self), sort_keys=True, default=str) + attributes_hash = hashlib.sha256(attributes.encode()).hexdigest() + return f"{self.__class__.__name__.lower()}-{attributes_hash}" async def get_cache(self) -> Union[CacheEntry, None]: """Get cached results matching the cache key from the database.""" @@ -180,6 +177,21 @@ class IncrementalCacheMixin(CacheMixin, CacheableIncrementally, ABC): frame: Frames + @cached_property + def cache_key(self) -> str: + """Calculate the indicator cache key. + + The cache key is composed of the class name and a hexadecimal digest + of the class instance attributes (excluding the span_range field if present), + e.g. fooindicator-44709d6fcb83d92a76dcb0b668c98e1b1d3dafe7. + + """ + attr_copy = vars(self).copy() + attr_copy.pop("span_range", None) + attributes = json.dumps(attr_copy, sort_keys=True, default=str) + attributes_hash = hashlib.sha256(attributes.encode()).hexdigest() + return f"{self.__class__.__name__.lower()}-{attributes_hash}" + @staticmethod @abstractmethod def merge(a: Any, b: Any) -> Any: diff --git a/src/api/core/warren/tests/indicators/test_mixins.py b/src/api/core/warren/tests/indicators/test_mixins.py index 024cafed..e9b810ae 100644 --- a/src/api/core/warren/tests/indicators/test_mixins.py +++ b/src/api/core/warren/tests/indicators/test_mixins.py @@ -28,8 +28,10 @@ def test_cache_key_calculation(): class MyIndicator(BaseIndicator, CacheMixin): """Dummy indicator.""" - def get_lrs_query(self) -> LRSStatementsQuery: - return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played") + course_id: str + + def get_lrs_query(self): + pass async def fetch_statements(self): pass @@ -38,39 +40,26 @@ async def compute(self): pass indicator = MyIndicator() - lrs_query = '{"verb": "https://w3id.org/xapi/video/verbs/played"}' - expected = f"myindicator-{hashlib.sha256(lrs_query.encode()).hexdigest()}" + attributes = '{"span_range": null}' + expected = f"myindicator-{hashlib.sha256(attributes.encode()).hexdigest()}" assert indicator.cache_key == expected + indicator = MyIndicator(course_id="test_id") + attributes = '{"course_id": "test_id", "span_range": null}' + expected = f"myindicator-{hashlib.sha256(attributes.encode()).hexdigest()}" + assert indicator.cache_key == expected -def test_cache_key_calculation_with_lrs_query_datetime_range(): - """Test the cache key calculation when the LRS query as date time range - (since/until) parameters set. - - """ # noqa: D205 - - class MyIndicator(BaseIndicator, CacheMixin): - """Dummy indicator.""" - - def get_lrs_query(self) -> LRSStatementsQuery: - return LRSStatementsQuery( - verb="https://w3id.org/xapi/video/verbs/played", - since=datetime(2023, 1, 1), - until=datetime(2023, 2, 1), - ) - - async def fetch_statements(self): - pass - - async def compute(self): - pass - - indicator = MyIndicator() - # We exclude "since" and "until" parameters from the LRS query to calculate - # the cache key - lrs_query = '{"verb": "https://w3id.org/xapi/video/verbs/played"}' - expected = f"myindicator-{hashlib.sha256(lrs_query.encode()).hexdigest()}" + span_range = DatetimeRange(since="2023-01-01 10:42", until="2023-01-02 12:22") + indicator = MyIndicator(course_id="test_id", span_range=span_range) + attributes = ( + '{"course_id": "test_id", ' + '"span_range": "since=datetime.datetime(2023, 1, 1, 10, 42, tzinfo=tzutc()) ' + 'until=datetime.datetime(2023, 1, 2, 12, 22, tzinfo=tzutc())"}' + ) + expected = f"myindicator-{hashlib.sha256(attributes.encode()).hexdigest()}" assert indicator.cache_key == expected + indicator2 = MyIndicator(span_range=span_range, course_id="test_id") + assert indicator2.cache_key == expected @pytest.mark.anyio @@ -82,7 +71,7 @@ class MyIndicator(BaseIndicator, CacheMixin): """Dummy indicator.""" def get_lrs_query(self) -> LRSStatementsQuery: - return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played") + pass async def fetch_statements(self): pass @@ -117,7 +106,7 @@ class MyIndicator(BaseIndicator, CacheMixin): """Dummy indicator.""" def get_lrs_query(self) -> LRSStatementsQuery: - return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played") + pass async def fetch_statements(self): pass @@ -162,7 +151,7 @@ class MyIndicatorA(BaseIndicator, CacheMixin): """Dummy indicator.""" def get_lrs_query(self) -> LRSStatementsQuery: - return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played") + pass async def fetch_statements(self): pass @@ -174,7 +163,7 @@ class MyIndicatorB(BaseIndicator, CacheMixin): """Dummy indicator.""" def get_lrs_query(self) -> LRSStatementsQuery: - return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played") + pass async def fetch_statements(self): pass @@ -205,7 +194,7 @@ class MyIndicator(BaseIndicator, CacheMixin): """Dummy indicator.""" def get_lrs_query(self) -> LRSStatementsQuery: - return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played") + pass async def fetch_statements(self): pass @@ -230,7 +219,7 @@ class MyIndicator(BaseIndicator, CacheMixin): """Dummy indicator.""" def get_lrs_query(self) -> LRSStatementsQuery: - return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played") + pass async def fetch_statements(self): pass @@ -258,7 +247,7 @@ class MyIndicator(BaseIndicator, CacheMixin): """Dummy indicator.""" def get_lrs_query(self) -> LRSStatementsQuery: - return None + pass async def fetch_statements(self): pass @@ -364,7 +353,7 @@ class MyIndicator(BaseIndicator, CacheMixin): """Dummy indicator.""" def get_lrs_query(self) -> LRSStatementsQuery: - return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played") + pass async def fetch_statements(self): pass @@ -393,7 +382,7 @@ class MyIndicator(BaseIndicator, CacheMixin): """Dummy mocked indicator.""" def get_lrs_query(self) -> LRSStatementsQuery: - return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played") + pass async def fetch_statements(self): pass @@ -439,7 +428,7 @@ class MyIndicator(BaseIndicator, CacheMixin): """Dummy indicator.""" def get_lrs_query(self) -> LRSStatementsQuery: - return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played") + pass async def fetch_statements(self): pass