Fix staleness bug for application export #722
Conversation
| qs = self.get_queryset() | ||
| cache.set(key, qs, 60 * 60) |
There was a problem hiding this comment.
Is there a reason we're caching the queryset instead of the data?
There was a problem hiding this comment.
We only get the data after running the queryset through the serializer, while I wanted the cache contents itself to be serializer agnostic given that export uses a different serializer. On second thought though, get_queryset() might be evaluated lazily so I would need to rethink this.
There was a problem hiding this comment.
Do you think .objects.all() would work @aviupadhyayula?
There was a problem hiding this comment.
From what I've read, it looks like the Django caching library forces evaluation upon insertion. We can test this later (force Django to log each query evaluation in settings). I'm more concerned about breaking our pattern of not caching querysets anywhere.
Honestly, is there a reason to cache this view? Querying the list of applicants will be done very rarely. Some latency on invocation is preferable to leaving a couple applicants off the UI or downloaded CSV. Not caching also obviates this concern.
There was a problem hiding this comment.
That's fair, caching definitely isn't necessary, at least for the CSV export (I didn't know that clubs used this feature until very recently). However, the cache invalidation logic was already previously written for the list view, and application-related views also contribute to site-level performance (I'm not sure how strict our transaction isolation levels are if at all, but you could imagine that querying all applications repeatedly, especially while new upsert transactions could still occur, is bad during application season).
Currently, the CSV export for applications relies on its own (Django built-in) cache, which results in staleness. This is especially an issue when club leaders export applications shortly (< 2 hours) after an application is due, as the export would not include many application submissions that were recently submitted.
This PR makes the route use the viewset list() function, which has valid manual caching and related invalidations on application submission. It also refactors list() to cache the non-serialized data to facilitate this.