Fix server-side cache to work with conditional GET by skaphan · Pull Request #608 · artoonie/rcvis

skaphan · 2026-03-06T13:43:02Z

Summary

Removes max_age=0 from patch_cache_control in ConditionalGetMixin -- this was preventing UpdateCacheMiddleware from storing responses server-side, since it checks get_max_age(response) and skips caching when the result is 0.
Replaces cache.clear() with per-URL cache purging in cloudflare.py -- uses get_cache_key() with synthetic HttpRequest objects to surgically invalidate only the affected visualization URLs from the file-based cache.
Removes DISABLE_CACHE toggle from settings -- the file-based cache is now always enabled and works correctly with conditional GET.
Removes vary_on_headers(increment) -- this was a no-op since no client ever sends that header.

How it works

ConditionalGetMixin sets Cache-Control: no-cache (browsers revalidate on every request)
UpdateCacheMiddleware stores the rendered response in Django file-based cache (now works because max_age is not 0)
FetchFromCacheMiddleware serves cached responses on subsequent requests
ConditionalGetMiddleware compares Last-Modified vs If-Modified-Since and returns 304 when content has not changed
On model update, CloudflareAPI.purge_vis_cache() clears both Cloudflare CDN and the specific Django cache entries

Builds on #594 (proper cache control).

Add updated_at field to JsonConfig model, use ConditionalGetMixin for all visualization views, and short-circuit 304 responses in VisualizeEmbedded before expensive computation.

Add proper cache control for embedded visualizations

- Add pylint disable for too-few-public-methods (it is a mixin) - Add docstring to get() method - Rename last_modified/if_modified_since to camelCase Co-Authored-By: Claude Opus 4.6

Co-Authored-By: Claude Opus 4.6

- Remove max_age=0 from ConditionalGetMixin so UpdateCacheMiddleware can store rendered responses in the file cache. Browsers still revalidate via Cache-Control: no-cache. - Replace cache.clear() sledgehammer in cloudflare.py with per-URL cache purging using get_cache_key — only the updated slug's pages are evicted from Django's file cache. - Remove vary_on_headers('increment') — was a no-op (no client sends the header). - Remove DISABLE_CACHE env toggle — no longer needed. - ConditionalGetMiddleware (already in middleware stack) handles 304 responses for cached pages using the Last-Modified header. Co-Authored-By: Claude Opus 4.6

artoonie

Thank you! This looks pretty close to ready. I recommend reverting the changes to purging django cache and DISABLE_CACHE for now, as that would be easier than fixing them.

artoonie · 2026-03-06T16:03:20Z

+ request.path = path
+ request.META['QUERY_STRING'] = ''
+ request.META['SERVER_NAME'] = 'localhost'
+ request.META['SERVER_PORT'] = '80'


This will work in development but not in production -- we are missing HTTP_HOST (which could be rcvis.com or www.rcvis.com), url scheme (https), and maybe others.
I think the only way to get this to work is to store a local map of slugs to cache IDs and to use that to clear the cache -- manually building the cache key is brittle.

artoonie · 2026-03-06T16:04:37Z


 AWS_DEFAULT_ACL = None

-if os.environ.get('DISABLE_CACHE') != 'True':


If removing this, we should also remove it from infra/.env.template.
However -- during development, it's still useful to be able to disable cache. Not all cache invalidations come from PATCHes. If you update an HTML or JS file locally, you want to be able to refresh the page without restarting the server. It doesn't always work (because JS files are minified and templates are compiled), but there are cases when it does.

artoonie · 2026-03-06T16:05:31Z

+ print('API user skaphan already exists')
+"
+
+echo "Database reset complete."


I don't feel comfortable committing this to the RCVis mainline -- a database deletion script feels like a high-risk script to leave in production.

skaphan · 2026-03-06T18:11:34Z

the commit o the database script was an accident. sorry! will address the other issues shortly.

…

On Mar 6, 2026, at 11:11 AM, Armin Samii ***@***.***> wrote: @artoonie commented on this pull request. Thank you! This looks pretty close to ready. I recommend reverting the changes to purging django cache and DISABLE_CACHE for now, as that would be easier than fixing them. In common/cloudflare.py <#608 (comment)>: > @@ -51,13 +53,28 @@ def purge_vis_cache(cls, slug): ] cls.purge_paths_cache(paths) + @classmethod + def _purge_django_cache(cls, paths: list[str]) -> None: + """ Purge matching entries from Django's file-based cache. """ + for path in paths: + request = HttpRequest() + request.method = 'GET' + # Split path?query into path and query string + if '?' in path: + request.path, request.META['QUERY_STRING'] = path.split('?', 1) + else: + request.path = path + request.META['QUERY_STRING'] = '' + request.META['SERVER_NAME'] = 'localhost' + request.META['SERVER_PORT'] = '80' This will work in development but not in production -- we are missing HTTP_HOST (which could be rcvis.com or www.rcvis.com <http://www.rcvis.com/>), url scheme (https), and maybe others. I think the only way to get this to work is to store a local map of slugs to cache IDs and to use that to clear the cache -- manually building the cache key is brittle. In rcvis/settings.py <#608 (comment)>: > @@ -281,20 +282,12 @@ AWS_DEFAULT_ACL = None -if os.environ.get('DISABLE_CACHE') != 'True': If removing this, we should also remove it from infra/.env.template. However -- during development, it's still useful to be able to disable cache. Not all cache invalidations come from PATCHes. If you update an HTML or JS file locally, you want to be able to refresh the page without restarting the server. It doesn't always work (because JS files are minified and templates are compiled), but there are cases when it does. In scripts/reset-db.sh <#608 (comment)>: > +python manage.py migrate + +# Create API-enabled admin user (matches docker-entrypoint.sh) +python manage.py shell -c " +from django.contrib.auth import get_user_model +User = get_user_model() +if not User.objects.filter(username='skaphan').exists(): + user = User.objects.create_superuser('skaphan', ***@***.***', 'rcvisacc0unt') + user.userprofile.canUseApi = True + user.userprofile.save() + print('Created API user skaphan with API access') +else: + print('API user skaphan already exists') +" + +echo "Database reset complete." I don't feel comfortable committing this to the RCVis mainline -- a database deletion script feels like a high-risk script to leave in production. — Reply to this email directly, view it on GitHub <#608 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABCNY7UZFXSRWSJPRZWAOL4PL2BRAVCNFSM6AAAAACWJQYVR2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTSMBUGU4TCMZRGE>. You are receiving this because you authored the thread.

- _purge_django_cache now uses Site.objects.get_current().domain to construct requests with the correct HTTP_HOST and HTTPS scheme, matching how UpdateCacheMiddleware stores cache keys in production. Tries both primary domain and www. variant. - Extracted _make_cache_request helper for constructing synthetic requests. - Restored DISABLE_CACHE env var toggle for development convenience. - Removed reset-db.sh (not appropriate for mainline). - Added three tests for django cache purging: basic path, query string, and www. variant coverage. Co-Authored-By: Claude Opus 4.6

skaphan · 2026-03-06T18:51:05Z

Thanks for the review! I've pushed changes addressing all three points:

Cache purging: You're right that the synthetic request was missing production-level headers. I've fixed _purge_django_cache to use Site.objects.get_current().domain for the HTTP_HOST (the same source get_absolute_paths_for already uses for Cloudflare purges) and set wsgi.url_scheme to https. It also tries both the primary domain and the www. variant, mirroring the Cloudflare purge logic. I extracted a _make_cache_request helper to keep it testable and added three tests covering basic paths, query strings, and the www variant.

Without per-URL purging, the cache would serve stale responses after a PATCH — FetchFromCacheMiddleware returns the old cached page before ConditionalGetMixin ever runs, so the Last-Modified check never happens. If you'd prefer to revert to cache.clear() as a simpler fallback, I'm happy to do that instead.

DISABLE_CACHE: Restored.

reset-db.sh: Removed.

… guard Co-Authored-By: Claude Opus 4.6

skaphan · 2026-03-07T15:12:03Z

Added 5 more tests covering the server-side cache paths:

test_conditional_get_returns_304_when_fresh -- verifies ConditionalGetMixin returns 304 when client If-Modified-Since is current
test_conditional_get_returns_200_after_update -- verifies stale If-Modified-Since gets a fresh 200 after model update
test_response_has_last_modified_header -- checks both Visualize and VisualizeEmbedded set Last-Modified matching updatedAt
test_response_has_no_cache_directive -- confirms Cache-Control: no-cache on responses
test_save_purge_only_on_update -- verifies purge_vis_cache is called on model update (not just creation)

Total cache-related tests on this branch: 8 (3 purge + 5 cache path).

Co-Authored-By: Claude Opus 4.6

Test 304 when If-Modified-Since is later than Last-Modified, and 200 (from cache) when If-Modified-Since is earlier than Last-Modified. Co-Authored-By: Claude Opus 4.6

artoonie

Hi Shel, thanks for the long PR! I'm almost done reviewing but wanted to back up for a bit:

This PR is trying to fix two issues, (1) fixing the 304, and (2) avoiding clearing all local cache. Perhaps we should separate them, as (1) is the crucial issue and (2) is a nice optimization.

Most of the concerns I have here are about the second issue, but I'd love to get the first out the door.

Apologies for how long the review cycle is taking. I try to prioritize reviewing PRs quickly but I have been falling behind here.

artoonie · 2026-03-09T18:14:08Z

+
+ # Second request should be served from cache (still 200)
+ response2 = self.client.get(path)
+ self.assertEqual(response2.status_code, 200)


Can you help me understand this? I don't see how this test shows it's served from cache.

artoonie · 2026-03-09T18:14:25Z

+ self.assertEqual(response2.status_code, 200)
+
+ # Purge and verify the cache entry is gone by checking that
+ # a new request still works (no 304 from stale cache)


We haven't checked 304 anywhere else in this function, I'm not sure what this is referring to?

artoonie · 2026-03-09T18:34:53Z

+ # This is safe because we're constructing internal requests, not
+ # processing user input.
+ original_hosts = settings.ALLOWED_HOSTS
+ settings.ALLOWED_HOSTS = list(set(original_hosts + domains))


This introduces a small race condition where the allowed_hosts is changing. I have never seen Django code modify settings on-the-fly, especially not security settings. While this code looks safe, it raises red flags for me. Django settings are usually immutable after startup.
Race condition 1: Thread A modifies; Thread B modifies; Thread A restores; Thread B restores. In this case, Thread B will have restored to the modified value.
Race condition 2: Thread A modifies; external actor exploits; Thread B restores
Is the only thing this piece of code is supporting allowing both the www and non-www variant of the URL to be used?
It seems this code would also limit the site to only being accessible from two URLs (x.com and www.x.com), whereas there are many cases where that does not hold (e.g. Heroku Review Apps) and cases where it limits future behavior (like if rcviz.com wants to mirror instead of redirect).

artoonie · 2026-03-09T18:35:23Z

+ self.client.get(path_with_qs)
+
+ # Purge should not raise, even with query string
+ CloudflareAPI._purge_django_cache([path_with_qs])


This tests that it doesn't crash, but not that the query string is respected

artoonie · 2026-03-09T18:51:39Z

+ for host in [domain, f'www.{domain}']:
+ request = CloudflareAPI._make_cache_request(path, host)
+ cache_key = get_cache_key(request)
+ if cache_key:


Shouldn't we also assert cache_key is not none?

artoonie · 2026-03-09T18:52:49Z

+ with self.settings(CACHES={'default': {
+ 'BACKEND': 'django.core.cache.backends.dummy.DummyCache'}}):
+ # First request: get the Last-Modified header
+ response1 = self.client.get(path)


For a fair head-to-head test comparison, the first request should also include last_modified

UpdateCacheMiddleware appended max-age=600 to responses with no-cache, telling browsers not to revalidate for 10 minutes. Custom subclass strips max-age when no-cache is set. Also fix cache purge to use the actual request host (via thread-local) so keys match in dev and prod. Co-Authored-By: Claude Opus 4.6

skaphan · 2026-03-11T04:49:25Z

Addendum: browser cache fix (`a1279e4`)

Problem

After PATCHing a visualization (e.g. re-tabulating with an excluded candidate), the iframe showed stale content. Three issues were at play:

max-age=600 leaked into browser responses. Django's UpdateCacheMiddleware stores responses in the server-side file cache (good -- avoids expensive graph recomputation) but also appends max-age=600 to Cache-Control. Combined with our no-cache, this told browsers not to revalidate for 10 minutes.
Cache purge missed in dev. _purge_django_cache built synthetic requests using Site.objects.get_current().domain (e.g. example.com), but dev requests arrive on localhost:8000. The cache keys did not match, so purge was a no-op locally.
304 after no-change PATCH (not a bug). After fixing Outgoing node lines don't line up #1 and Add election date to heading #2, we still saw 304s when PATCHing with identical data. This turned out to be correct: Django's ConditionalGetMiddleware computes an ETag from the response body. Same data = same HTML = same ETag = 304. Real data changes produce different HTML = different ETag = 200.

Solution

UpdateCacheWithoutMaxAgeMiddleware: subclass that calls super() (stores in server cache), then strips max-age from responses that have no-cache. Browser always revalidates; server cache stays warm.
CurrentRequestMiddleware: stores the current request in a thread-local. _purge_django_cache reads it to construct synthetic requests with the correct host, so cache keys match in both dev and production.
ConditionalGetMixin comment updated to reflect the middleware change.

Revert to upstream cache.clear() approach for Django file cache. The surgical purge optimization is deferred to a separate PR. Remove CurrentRequestMiddleware and related purge tests. Co-Authored-By: Claude Opus 4.6

skaphan · 2026-03-11T06:07:49Z

Addressed by simplifying: the surgical cache purge code (with its ALLOWED_HOSTS manipulation and thread-local middleware) has been removed from this PR. purge_paths_cache now uses cache.clear() — a straightforward full cache clear that doesn't need to reconstruct cache keys or worry about host matching.

The surgical purge optimization is preserved on a separate branch (surgical-cache-purge) for a potential follow-up PR, where the ALLOWED_HOSTS concern and other refinements can be addressed independently.

Strengthen conditional GET tests with old-date baselines for fair comparison, assertNumQueries(0) to prove middleware-cached 304 never hits the view, and cross-referencing docstrings. Add test verifying REST PATCH advances updatedAt. Co-Authored-By: Claude Opus 4.6

skaphanand others added 5 commits February 24, 2026 13:29

Add proper cache control for embedded visualizations
94fac08
Add updated_at field to JsonConfig model, use ConditionalGetMixin for all visualization views, and short-circuit 304 responses in VisualizeEmbedded before expensive computation.

Merge pull requestartoonie#594from skaphan/proper-cache-control
3e4a1d8
Add proper cache control for embedded visualizations

Fix linter issues in ConditionalGetMixin
032e992
- Add pylint disable for too-few-public-methods (it is a mixin) - Add docstring to get() method - Rename last_modified/if_modified_since to camelCase Co-Authored-By: Claude Opus 4.6

Add reset-db.sh for easy database reset after branch switching
70693e6
Co-Authored-By: Claude Opus 4.6

skaphan mentioned this pull request Mar 6, 2026
Remove Django server-side response cache #605
Closed

artoonie reviewed Mar 6, 2026
View reviewed changes

skaphan force-pushed the fix-server-cache branch from f69968a to fd01651Compare March 6, 2026 18:46

Add cache path tests: conditional GET, Last-Modified, no-cache, purge…
bf88524
… guard Co-Authored-By: Claude Opus 4.6

skaphan added 2 commits March 7, 2026 10:54

Add test for server cache 304 via middleware pipeline
66a2556
Co-Authored-By: Claude Opus 4.6

Add server cache tests for If-Modified-Since boundary cases
7d8695c
Test 304 when If-Modified-Since is later than Last-Modified, and 200 (from cache) when If-Modified-Since is earlier than Last-Modified. Co-Authored-By: Claude Opus 4.6

artoonie reviewed Mar 9, 2026
View reviewed changes

Simplify: use cache.clear() instead of surgical purge
1721512
Revert to upstream cache.clear() approach for Django file cache. The surgical purge optimization is deferred to a separate PR. Remove CurrentRequestMiddleware and related purge tests. Co-Authored-By: Claude Opus 4.6

skaphan mentioned this pull request Mar 11, 2026
Surgical Django cache purge #609
Open
2 tasks

Merge branch 'main' into fix-server-cache
c6ab4a7

artoonie closed this pull request by merging all changes into artoonie:main in b69eb3dMar 19, 2026

skaphan deleted the fix-server-cache branch March 20, 2026 19:25


		AWS_DEFAULT_ACL = None

		if os.environ.get('DISABLE_CACHE') != 'True':

Conversation

skaphan commented Mar 6, 2026

Summary

How it works

Uh oh!

artoonie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skaphan commented Mar 6, 2026 via email

Uh oh!

skaphan commented Mar 6, 2026

Uh oh!

skaphan commented Mar 7, 2026

Uh oh!

artoonie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

skaphan commented Mar 11, 2026

Addendum: browser cache fix (a1279e4)

Problem

Solution

Uh oh!

skaphan commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Addendum: browser cache fix (`a1279e4`)