-
Notifications
You must be signed in to change notification settings - Fork 4
Caching layer: Redis + /status, /clear, /stats + TTL + Hit/Miss #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Caching layer: Redis + /status, /clear, /stats + TTL + Hit/Miss #3
Conversation
dec1belPP
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Jes360, thank you for the PR. Can you please retrofit this with @Jasi-with-noe implementation of the Graph service #1 that has been merged? This prevents duplicate graph services. Also clear up te conflicts. Thank you!
|
|
Hey @Jes360, could you also resolve the conflicts? Thank you. |
dec1belPP
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Jes360, thank you fixing up the conflicts. This PR successfully implements a Redis-based caching layer with middleware for GET response caching and management endpoints. While the core functionality meets the ticket requirements, there are several issues that must be addressed before merging.
1. Header Loss in Cached Responses
In lines 22 - 25 in cache_middleware.py, original response headers (Cache-Control, ETag, etc.) are discarded. This results in incorrect caching behavior, broken client expectations etc.
2. No Error Handling in Middleware
At line 20 in cache_middleware.py, Redis failures will crash all GET requests and result in a single point of failure.
3. Inconsistent Caching Logic
In lines 18 - 26 in cache_middleware.py, caching still occurs even when CACHE_ENABLED=False. This results in unexpected behavior.
4. Dependency Management
Both redis and aioredis are declared. Use redis with redis.asyncio instead. This reduces version conflicts and maintenance overhead.
5. Configuration Issues
Unused PREFIX constant in cache.py at line 6.
7. No Connection Management
Missing proper Redis connection management
…dit-api into jesti/cache-implementation
Description
Introduces a Redis-backed caching layer to improve performance of Graph API calls and reduce repeated network traffic.
Changes
• Middleware: CacheMiddleware caches successful GET responses when caching is enabled
• Cache API: Added /cache/status, /cache/clear, /cache/stats endpoints
• Cache Service: CacheService wraps Redis with get/set/delete/clear/stats, tracks hit/miss, and uses namespaced keys
Graph API:
• Graph Service now looks up users and /me in cache before calling Microsoft Graph
• Caches responses with TTL, truncates token in keys to avoid storing secrets.
• Added /graph/users and /graph/me endpoints
Integration: app/main.py updated to include middleware and new routers.
• Dependencies: Added redis and aioredis in pyproject.toml.
Testing
• Start Redis and run the API with CACHE_ENABLED=true.
• Verify TTL expiry works.
• Check /cache/stats for hit/miss counts.
• Use /cache/clear to flush Redis.
Planner Link
https://teams.microsoft.com/l/entity/com.microsoft.teamspace.tab.planner/mytasks?tenantId=d02378ec-1688-46d5-8540-1c28b5f470f6&webUrl=https%3A%2F%2Ftasks.teams.microsoft.com%2Fteamsui%2FpersonalApp%2Falltasklists&context=%7B%22subEntityId%22%3A%22%2Fv1%2Fassignedtome%2Fview%2Fboard%2Ftask%2FYZ9xTus_hEO4v7u7_hxt3MgAK1-X%22%7D