-
Notifications
You must be signed in to change notification settings - Fork 2
[ELI-619] - cache campaign configs unless we send a reserved consumer id #610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4eab9bf
deb8465
90ad257
a6b8563
0bc087e
7b295e1
2a4efe5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,7 @@ | ||
| import json | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using TTLCache would simplify this a bit e.g. import json from aws_xray_sdk.core import xray_recorder from eligibility_signposting_api.config.constants import CACHE_TTL_SECONDS, RESERVED_TEST_CONSUMER_IDS BucketName = NewType("BucketName", str) logger = logging.getLogger(name) campaign_config_cache: TTLCache[str, list[CampaignConfig]] = TTLCache(maxsize=1, ttl=CACHE_TTL_SECONDS) @service |
||
| import logging | ||
| import os | ||
| import time | ||
| from collections.abc import Generator | ||
| from typing import Annotated, NewType | ||
|
|
||
|
|
@@ -7,9 +10,11 @@ | |
| from wireup import Inject, service | ||
|
|
||
| from eligibility_signposting_api.model.campaign_config import CampaignConfig, Rules | ||
| from eligibility_signposting_api.config.constants import TTL, RESERVED_TEST_CONSUMER_IDS | ||
|
|
||
| BucketName = NewType("BucketName", str) | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| @service | ||
| class CampaignRepo: | ||
|
|
@@ -25,13 +30,55 @@ def __init__( | |
| super().__init__() | ||
| self.s3_client = s3_client | ||
| self.bucket_name = bucket_name | ||
| self._campaign_configs_cache: list[CampaignConfig] | None = None | ||
| self._cache_expiry_epoch: float = 0.0 | ||
| self._cache_ttl_seconds: int = int(TTL.get(os.getenv("ENVIRONMENT"), 0)) | ||
|
|
||
| def get_campaign_configs(self, consumer_id: str) -> Generator[CampaignConfig, None, None]: | ||
| now = time.time() | ||
| cache_enabled = self._cache_ttl_seconds > 0 | ||
| cache_valid = ( | ||
| cache_enabled | ||
| and consumer_id not in RESERVED_TEST_CONSUMER_IDS | ||
| and self._campaign_configs_cache is not None | ||
| and now < self._cache_expiry_epoch | ||
| ) | ||
|
|
||
| def get_campaign_configs(self) -> Generator[CampaignConfig]: | ||
| with xray_recorder.in_subsegment("CampaignRepo.get_campaign_configs"): | ||
| if cache_valid: | ||
| logger.info("Using cached campaign configs") | ||
| yield from self._campaign_configs_cache | ||
| return | ||
|
|
||
| logger.info( | ||
| "Refreshing campaign configs from S3 (consumer_id=%s, ttl_seconds=%s)", | ||
| consumer_id, | ||
| self._cache_ttl_seconds, | ||
| ) | ||
| campaign_configs = self._load_campaign_configs_from_s3() | ||
|
|
||
| if cache_enabled and consumer_id not in RESERVED_TEST_CONSUMER_IDS: | ||
| self._campaign_configs_cache = campaign_configs | ||
| self._cache_expiry_epoch = now + self._cache_ttl_seconds | ||
|
|
||
| yield from campaign_configs | ||
|
|
||
| def _load_campaign_configs_from_s3(self) -> list[CampaignConfig]: | ||
| campaign_configs: list[CampaignConfig] = [] | ||
|
|
||
| with xray_recorder.in_subsegment("CampaignRepo.load_campaign_configs_from_s3"): | ||
| with xray_recorder.in_subsegment("list_objects"): | ||
| campaign_objects = self.s3_client.list_objects(Bucket=self.bucket_name) | ||
|
|
||
| with xray_recorder.in_subsegment("get_objects"): | ||
| for campaign_object in campaign_objects["Contents"]: | ||
| response = self.s3_client.get_object(Bucket=self.bucket_name, Key=f"{campaign_object['Key']}") | ||
| for campaign_object in campaign_objects.get("Contents", []): | ||
| response = self.s3_client.get_object( | ||
| Bucket=self.bucket_name, | ||
| Key=f"{campaign_object['Key']}", | ||
| ) | ||
| body = response["Body"].read() | ||
| yield Rules.model_validate(json.loads(body)).campaign_config | ||
| campaign_configs.append( | ||
| Rules.model_validate(json.loads(body)).campaign_config | ||
| ) | ||
|
|
||
| return campaign_configs | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| import io | ||
| import json | ||
| from unittest.mock import MagicMock | ||
|
|
||
| import pytest | ||
|
|
||
| from eligibility_signposting_api.repos.campaign_repo import CampaignRepo, BucketName | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we'd then import campaign_config_cache here |
||
| from tests.fixtures.builders.model.rule import CampaignConfigFactory | ||
|
|
||
|
|
||
| def make_s3_body(payload: dict): | ||
| return {"Body": io.BytesIO(json.dumps(payload).encode("utf-8"))} | ||
|
|
||
|
|
||
| class TestCampaignRepo: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and can have a fixture to clear the cache e.g. |
||
| @pytest.fixture | ||
| def mock_s3_client(self): | ||
| return MagicMock() | ||
|
|
||
| @pytest.fixture | ||
| def repo(self, mock_s3_client): | ||
| return CampaignRepo( | ||
| s3_client=mock_s3_client, | ||
| bucket_name=BucketName("test-bucket"), | ||
| ) | ||
|
|
||
| @pytest.fixture | ||
| def rules_payload(self): | ||
| campaign_config = CampaignConfigFactory.build() | ||
| return { | ||
| "campaign_config": campaign_config.model_dump(mode="json") | ||
| } | ||
|
|
||
| def test_get_campaign_configs_loads_from_s3(self, repo, mock_s3_client, rules_payload): | ||
| mock_s3_client.list_objects.return_value = { | ||
| "Contents": [{"Key": "rsv.json"}] | ||
| } | ||
| mock_s3_client.get_object.return_value = make_s3_body(rules_payload) | ||
|
|
||
| result = list(repo.get_campaign_configs("consumer_id")) | ||
|
|
||
| assert len(result) == 1 | ||
| assert result[0].id == rules_payload["campaign_config"]["id"] | ||
|
|
||
| mock_s3_client.list_objects.assert_called_once_with(Bucket="test-bucket") | ||
| mock_s3_client.get_object.assert_called_once_with( | ||
| Bucket="test-bucket", | ||
| Key="rsv.json", | ||
| ) | ||
|
|
||
| def test_get_campaign_configs_uses_cache_within_ttl( | ||
| self, | ||
| repo, | ||
| mock_s3_client, | ||
| monkeypatch, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... wouldn't need monkeypatch as could then call the cache.clear to force the refresh (later on) |
||
| ): | ||
| repo._cache_ttl_seconds = 60 | ||
|
|
||
| first_config = CampaignConfigFactory.build(version=1) | ||
|
|
||
| mock_s3_client.list_objects.return_value = { | ||
| "Contents": [{"Key": "rsv.json"}] | ||
| } | ||
| mock_s3_client.get_object.return_value = make_s3_body( | ||
| {"campaign_config": first_config.model_dump(mode="json")} | ||
| ) | ||
|
|
||
| monkeypatch.setattr("time.time", lambda: 1000.0) | ||
|
|
||
| first = list(repo.get_campaign_configs("consumer_id")) | ||
| second = list(repo.get_campaign_configs("consumer_id")) | ||
|
|
||
| assert first[0].version == 1 | ||
| assert second[0].version == 1 | ||
| assert mock_s3_client.list_objects.call_count == 1 | ||
| assert mock_s3_client.get_object.call_count == 1 | ||
|
|
||
| def test_get_campaign_configs_refreshes_after_ttl_expiry( | ||
| self, | ||
| repo, | ||
| mock_s3_client, | ||
| monkeypatch, | ||
| ): | ||
| repo._cache_ttl_seconds = 60 | ||
|
|
||
| first_config = CampaignConfigFactory.build(version=1) | ||
| second_config = CampaignConfigFactory.build(version=2) | ||
|
|
||
| mock_s3_client.list_objects.return_value = { | ||
| "Contents": [{"Key": "rsv.json"}] | ||
| } | ||
| mock_s3_client.get_object.side_effect = [ | ||
| make_s3_body({"campaign_config": first_config.model_dump(mode="json")}), | ||
| make_s3_body({"campaign_config": second_config.model_dump(mode="json")}), | ||
| ] | ||
|
|
||
| current_time = {"value": 1000.0} | ||
| monkeypatch.setattr("time.time", lambda: current_time["value"]) | ||
|
|
||
| first = list(repo.get_campaign_configs("consumer_id")) | ||
| current_time["value"] = 1030.0 | ||
| second = list(repo.get_campaign_configs("consumer_id")) | ||
| current_time["value"] = 1061.0 | ||
| third = list(repo.get_campaign_configs("test-consumer-1")) | ||
|
|
||
| assert first[0].version == 1 | ||
| assert second[0].version == 1 | ||
| assert third[0].version == 2 | ||
| assert mock_s3_client.list_objects.call_count == 2 | ||
| assert mock_s3_client.get_object.call_count == 2 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could just be env vars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g.
import os
from typing import Literal
URL_PREFIX = "patient-check"
RULE_STOP_DEFAULT = False
NHS_NUMBER_HEADER = "nhs-login-nhs-number"
CONSUMER_ID = "NHSE-Product-ID"
ALLOWED_CONDITIONS = Literal["COVID", "FLU", "MMR", "RSV"]
CONSUMER_MAPPING_FILE_NAME = "consumer_mapping_config.json"
RESERVED_TEST_CONSUMER_IDS = {"test-consumer-1", "test-consumer-2", "test-consumer-3"}
CACHE_TTL_SECONDS = int(os.getenv("CONFIG_CACHE_TTL_SECONDS", "1800"))