Skip to content

Page d’accueil: Éviter le plantage en cas de rupture de la connexion au cache#5282

Merged
calummackervoy merged 2 commits intomasterfrom
calum/cache-failsafe
Dec 20, 2024
Merged

Page d’accueil: Éviter le plantage en cas de rupture de la connexion au cache#5282
calummackervoy merged 2 commits intomasterfrom
calum/cache-failsafe

Conversation

@calummackervoy
Copy link
Contributor

🤔 Pourquoi ?

La campagne active est mise en cache pour être utilisée dans une modale qui s'affiche sur le site. Si la connexion au cache est interrompue, une erreur 500 sera générée.

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?

Comment on lines 74 to 98
@freeze_time("2024-01-31")
def test_cache_failsafe(self):
cache_backend = caches["failsafe"]

with (
mock.patch.object(cache_backend, "get", return_value=None),
mock.patch.object(cache_backend, "has_key", return_value=None),
mock.patch.object(cache_backend, "set", return_value=None),
):
campaign = AnnouncementCampaignFactory(start_date=date(2024, 1, 1), with_item=True)
assert active_announcement_campaign(None)["active_campaign_announce"] == campaign
campaign.delete()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je ne comprends pas ce test, et il passe sur master, il ne sert donc pas à la non-regression.

Pourquoi ne pas bloquer l’accès au cache (insérer des paramètres incorrects pour Redis, voir

def test_access_with_bad_url(self, settings):
with socket.create_server(("localhost", 0)) as s:
empty_port = s.getsockname()[1]
s.close()
cache = UnclearableCache(
f"redis://localhost:{empty_port}",
{
"OPTIONS": {
"CLIENT_CLASS": "itou.utils.cache.FailSafeRedisCacheClient",
},
},
)
with mock.patch("itou.utils.cache.capture_exception") as sentry_mock:
assert cache.get("foo") is None
sentry_mock.assert_called_once()
[args, kwargs] = sentry_mock.call_args
[exception] = args
# django-redis chains redis original exceptions through ConnectionInterrupted
[exc_msg] = exception.__cause__.args if exception.__cause__ else exception.args
# Message error code depends on the platform (Mac or Linux). Should be a variation of the following ones:
# Error 99 connecting to localhost:{empty_port}. Cannot assign requested address.
# Error 111 connecting to localhost:{empty_port}. Connection refused.
assert re.match(r"Error \d+ connecting to localhost", exc_msg)
assert kwargs == {}
) et essayer d’accéder à une page via une requête HTTP classique (client.get(...))?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L'assertion cible que même quand la cache est down, le active_campaign_announce sera disponible dans la contexte.

J'ai ajouté tes suggestions (j'espère que le test est plus comprehensive et clair maintenant), et dans 4df845e j'ai ajouté à FailSafeRedisCacheClient le support de l'argument default de get (SENTINEL_ACTIVE_ANNOUNCEMENT)

@francoisfreitag francoisfreitag dismissed their stale review December 18, 2024 16:41

Ça m’a l’air bien mieux !

Copy link
Member

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je suis off aujourd’hui, ça attendra mon retour ou une autre relecture. Au premier coup d’œil, ça m’a l’air top.

capture_exception(e)
# None is the return for a GET where the key does not exist.
return None
return CONNECTION_INTERRUPTED # return for a GET where the key does not exist.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

CONNECTION_INTERRUPTED is used as a sentinel value by Django-redis to support default values
The active campaign is cached for use in a modal which renders on every page. If the connection to the cache is severed, it will cause a 500 error

Add failing_cache fixture for tests
@calummackervoy calummackervoy added this pull request to the merge queue Dec 20, 2024
Merged via the queue into master with commit 6d6df25 Dec 20, 2024
9 checks passed
@calummackervoy calummackervoy deleted the calum/cache-failsafe branch December 20, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants