Task/dogpile orm event invalidation

[whabanks]

Summary | Résumé

This PR experimentally implements an ORM-event driven invalidation strategy for Service entities. Rather than calling an explicit invalidate_service_cache_keys() function on every path that mutates service and service related data; we instead listen to after_flush, after_commit, and after_rollback sqlalchemy session events, weaving cache invalidation calls into that process. (similar in nature to aspect oriented programming)

Note:
it looks messy currently because it was rebased with 3011 in order to be testable. The bulk of the changes are in:

• app/cache/service_cache_events.py

This nets us:

• Centralized invalidation logic
• Reduced code complexity, and improve maintainability particularly on larger models where multiple mutating code paths are involved
• Tighter control over when we invalidate i.e. if a transaction is rolled back there's no need to invalidate existing cache
• Since we're handling this logic much closer to the actual time the DB is updated, we reduce the chances of stale cache reads after a successful write.

Related Issues | Cartes liées

Test instructions | Instructions pour tester la modification

1. Access a service, note the cache key is created
2. Navigate to service settings and turn off email sending (or change any setting) -> note the cache key was updated
3. Add or remove a team member from a service -> note the cache key reflects this change

Release Instructions | Instructions pour le déploiement

None.

Reviewer checklist | Liste de vérification du réviseur

• This PR does not break existing functionality.
• This PR does not violate GCNotify's privacy policies.
• This PR does not raise new security concerns. Refer to our GC Notify Risk Register document on our Google drive.
• This PR does not significantly alter performance.
• Additional required documentation resulting of these changes is covered (such as the README, setup instructions, a related ADR or the technical documentation).

⚠ If boxes cannot be checked off before merging the PR, they should be moved to the "Release Instructions" section with appropriate steps required to verify before release. For example, changes to celery code may require tests on staging to verify that performance has not been affected.

[jimleroyer]

I am concerned that this approach (and the other PR too but maybe slightly less) might be difficult to scale, i.e. every time a developer wants certain cache to be added, they would have to make sure proper specific events fire, and that these are for service IDs, is that right? And when we want to add a cache specific to users, and templates, and keys, and etc.. we'd have also to add specific glue code for these? The cache isn't generic enough to handle these automatically?

[whabanks]

every time a developer wants certain cache to be added, they would have to make sure proper specific events fire, and that these are for service IDs, is that right?

With the current implementation, yes this would have to be done to accommodate more than just the service entity.

And when we want to add a cache specific to users, and templates, and keys, and etc.. we'd have also to add specific glue code for these? The cache isn't generic enough to handle these automatically?

The baseline dogpile library isn't smart enough to graph and evaluate the potential entity relationships and invalidate corresponding keys on it's own. One possibility to facilitate a generic implementation would be to leverage a registry that centrally stores (and also serves to document) entity <-> cache key dependency metadata.
Then developers only need to update the registry (simple JSON or YAML file?). A mapper could resolve the entity -> cache key dependencies using that registry, and generically invalidate keys / groups of keys.

Something like this:

The two new things here would be the registry and dependency resolver.

• The session events are already in place
• The change collector would be adjusted to generically map collected ids to an entity type
• The cache invalidator would be changed to map entities to cache keys base on the registry and execute the invalidation

[whabanks]

Closing in favour of this PR because I royally forked up the commit history on this PR with a prior rebase. Given there's is still much work to do I'd like to be merciful to reviewers. Will capture the current conversation in that PR as well.