feat: smart caching strategy for dashboard & analytics (#127)#396
Open
sinatragianpaolo-oc wants to merge 2 commits intorohitdash08:mainfrom
Open
feat: smart caching strategy for dashboard & analytics (#127)#396sinatragianpaolo-oc wants to merge 2 commits intorohitdash08:mainfrom
sinatragianpaolo-oc wants to merge 2 commits intorohitdash08:mainfrom
Conversation
- Tiered TTL constants: STATIC(1h) > AI_RESULT(30m) > ANALYTICS(10m) > DASHBOARD(5m) > SHORT(1m) - cache() decorator for route-level caching - AI budget suggestions cached 30min (expensive Gemini calls) - Categories cached 1h, invalidated on create/update/delete - GET /insights/cache-stats — hit/miss monitoring endpoint - DELETE /insights/cache-stats — reset counters - Graceful degradation: Redis errors never crash routes - 20 tests covering operations, TTL, graceful degradation, HTTP Closes rohitdash08#127
…ent cache-stats access
Fix 1 — get_cache_stats used bytes keys (b'hits') instead of str ('hits')
Redis is initialised with decode_responses=True, so hgetall returns str
keys. Using b'hits' always fell back to the default 0, making hit/miss
counters permanently zero. Changed to raw.get('hits', 0) and added an
inline comment explaining the invariant.
Fix 2 — cached() now guards against None from get_json()
result.get_json(force=True) returns None when the response body is not
valid JSON. Storing None in Redis would poison the key: every subsequent
cache hit would return null JSON instead of calling the real handler.
Added: if data is None: return result before cache_set, with comment.
Fix 3 — _increment_hit / _increment_miss now log on Redis failure
Previously bare 'except: pass' swallowed all errors silently, making it
impossible to diagnose Redis connectivity issues via logs. Changed to
'except Exception as exc: logger.debug(...)' so failures appear in debug
logs without affecting production noise level.
Fix 4 — /insights/cache-stats access policy documented
The route was open to all authenticated users without an admin guard,
while issue description mentioned /reminders/job-runs has one. Added
explicit docstring comment documenting the deliberate decision: the data
exposed (hit/miss counters, Redis memory) contains no PII; any authenticated
user may consult it. Comment includes snippet for adding an admin check.
Tests added:
- test_increment_hit_redis_failure_logged (caplog, fix 3)
- test_increment_miss_redis_failure_logged (caplog, fix 3)
- test_get_cache_stats_reads_string_keys (fix 1)
- test_get_cache_stats_bytes_keys_would_be_zero (fix 1 regression proof)
- test_cached_decorator_skips_none_json (fix 2)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds smart caching strategy for dashboard & analytics to fix #127.
Tiered TTLs (STATIC/ANALYTICS/DASHBOARD/AI_RESULT/SHORT),
cached()route decorator, explicit invalidation on mutations,flask cache-statsendpoint. Graceful degradation: cache errors never crash routes.Review fixes (latest commit)
BUG:
get_cache_statsused bytes keys —hgetallreturnsstrkeys when Redis is initialised withdecode_responses=True(as it is here). Usingb'hits'/b'misses'always fell through to the default0, making counters permanently zero. Fixed toraw.get('hits', 0)/raw.get('misses', 0)with an inline comment documenting the invariant.cached()decorator:Nonefromget_json()now skipped —result.get_json(force=True)returnsNonewhen the response body is not valid JSON (empty body, non-JSON content type, etc.). StoringNonein Redis would poison the key: every subsequent cache hit would return a null JSON response instead of calling the real handler. Addedif data is None: return resultguard beforecache_set, with explanatory comment._increment_hit/_increment_missnow log on Redis failure — previouslyexcept: passswallowed all errors silently, making Redis connectivity issues invisible. Changed toexcept Exception as exc: logger.debug(...)— failures appear in debug logs without adding production noise./insights/cache-statsaccess policy documented — the endpoint was open to all authenticated users with no admin guard. Added an explicit docstring on bothGETandDELETEexplaining the deliberate decision: the exposed data (hit/miss counters, Redis memory) contains no PII, so any authenticated user may consult it. The comment also shows the snippet needed to add an admin check if the deployment requires it.New tests (5):
test_increment_hit_redis_failure_logged/test_increment_miss_redis_failure_logged— caplog asserts the debug message is emittedtest_get_cache_stats_reads_string_keys— mockhgetallwith str keys, asserts correct hit/miss valuestest_get_cache_stats_bytes_keys_would_be_zero— regression proof that bytes keys are not usedtest_cached_decorator_skips_none_json— assertscache_setis never called whenget_json()returnsNoneCloses #127