feat: implement Redis caching for database queries (closes #396)#852
feat: implement Redis caching for database queries (closes #396)#852singhanurag0317-bit wants to merge 1 commit into
Conversation
|
@singhanurag0317-bit is attempting to deploy a commit to the ritesh Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds Redis-backed caching for system settings and ticket data with graceful fallback to Supabase. System settings, ticket lists, and individual ticket details are now cached with 5-minute TTLs. Ticket mutations invalidate affected cache entries to maintain consistency. Redis initialization at startup includes connection ping and fallback when unavailable. ChangesRedis Caching Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
backend/main.py (2)
738-742: 💤 Low valueSame falsy check issue as
get_tickets.While less likely to matter here (a found ticket should have data), the pattern is inconsistent. Consider using
is not Nonefor consistency.♻️ Optional consistency fix
- if redis_client and res.data: + if redis_client and res.data is not None: try: redis_client.setex(cache_key, 300, json.dumps(res.data))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/main.py` around lines 738 - 742, The current conditional uses truthy checks for redis_client and res.data which can incorrectly skip caching when these objects are falsy-but-valid; change the if to explicitly check redis_client is not None (and if you want consistency also check res.data is not None) before calling redis_client.setex with cache_key — locate the block that references redis_client, res.data, cache_key and the setex call and replace the truthy checks with explicit "is not None" checks and keep the existing try/except around setex.
88-115: 💤 Low valueConsider TTL variation by entity type.
All cached entities use a fixed 5-minute TTL. System settings change rarely and could use a longer TTL (e.g., 15-30 minutes), while ticket lists change frequently and might benefit from a shorter TTL (e.g., 2-3 minutes) to reduce stale data visibility.
This is a nice-to-have optimization that can be deferred.
Also applies to: 575-599, 722-744
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/main.py` around lines 88 - 115, The current code hardcodes a 5-minute TTL (300s) when caching via redis_client.setex for system settings (cache_key and setex usage in the shown block), which should be variable by entity type; add a TTL lookup (e.g., TTL_MAP = {"system_settings": 900-1800, "ticket_list": 120-180, "default": 300}) and compute ttl = TTL_MAP.get("system_settings", TTL_MAP["default"]) before calling redis_client.setex(cache_key, ttl, json.dumps(result)), and apply the same pattern to other cache sites that use fixed 300s (the other setex/get uses noted in the diff) so each entity uses its configured TTL constant instead of the hardcoded 300.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/main.py`:
- Around line 593-597: The current guard `if redis_client and res.data:` skips
caching when res.data is an empty list; change it to check for existence rather
than truthiness (e.g., `res.data is not None`) so empty lists are cached; update
the block around `redis_client`, `res.data`, `cache_key`, and the `setex` call
to use that non-None check and keep the existing try/except for `setex` errors.
- Around line 776-778: The code calls redis_client.keys("tickets:list:*") which
blocks Redis; replace this with a non-blocking SCAN-based approach (e.g., use
redis_client.scan_iter or scan with cursor) to iterate matching keys and collect
or delete them in batches, and then call redis_client.delete in batched chunks
(instead of deleting all at once). Alternatively, if the update payload contains
specific company IDs, use those IDs to construct exact keys like
"tickets:list:{company_id}" and delete only those keys rather than using the
wildcard; update the logic around redis_client.keys, redis_client.delete, and
the cache invalidation path to use scan_iter/batched delete or targeted deletes
by company ID.
In `@scratch/test_caching.py`:
- Around line 135-157: The tearDown currently does main.TICKETS_DB =
self.orig_tickets_db which can reattach a mutated original list; instead capture
a shallow copy in setUp (e.g., self.orig_tickets_db = list(main.TICKETS_DB)) and
in tearDown restore the contents with clear/extend semantics (e.g.,
main.TICKETS_DB.clear(); main.TICKETS_DB.extend(self.orig_tickets_db)) so that
the TICKETS_DB reference remains the same but its contents are reliably
restored; apply this change around the setUp and tearDown handling of
main.TICKETS_DB and keep restoration for main.redis_client, main.supabase, and
main.duplicate_service as-is.
- Around line 265-284: The test payload for TicketSaveRequest includes undefined
fields is_potential_duplicate and parent_ticket_id which will trigger Pydantic
validation errors; either remove these keys from the payload in
scratch/test_caching.py or add them as optional fields on the TicketSaveRequest
model (in main.py) with appropriate types (e.g., is_potential_duplicate:
Optional[bool] = False and parent_ticket_id: Optional[str] = None) so the test
and model definitions match. Ensure you update only the TicketSaveRequest class
or the test payload (not both) and keep types/defaults consistent with existing
fields like auto_resolve and parent semantics.
---
Nitpick comments:
In `@backend/main.py`:
- Around line 738-742: The current conditional uses truthy checks for
redis_client and res.data which can incorrectly skip caching when these objects
are falsy-but-valid; change the if to explicitly check redis_client is not None
(and if you want consistency also check res.data is not None) before calling
redis_client.setex with cache_key — locate the block that references
redis_client, res.data, cache_key and the setex call and replace the truthy
checks with explicit "is not None" checks and keep the existing try/except
around setex.
- Around line 88-115: The current code hardcodes a 5-minute TTL (300s) when
caching via redis_client.setex for system settings (cache_key and setex usage in
the shown block), which should be variable by entity type; add a TTL lookup
(e.g., TTL_MAP = {"system_settings": 900-1800, "ticket_list": 120-180,
"default": 300}) and compute ttl = TTL_MAP.get("system_settings",
TTL_MAP["default"]) before calling redis_client.setex(cache_key, ttl,
json.dumps(result)), and apply the same pattern to other cache sites that use
fixed 300s (the other setex/get uses noted in the diff) so each entity uses its
configured TTL constant instead of the hardcoded 300.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5681c18-52bc-4a6e-a60d-8d268637112a
📒 Files selected for processing (3)
backend/main.pybackend/requirements.txtscratch/test_caching.py
| if redis_client and res.data: | ||
| try: | ||
| redis_client.setex(cache_key, 300, json.dumps(res.data)) | ||
| except Exception as ce: | ||
| print(f"[WARNING] Redis write error for {cache_key}: {ce}") |
There was a problem hiding this comment.
Empty lists won't be cached.
Line 593 checks if redis_client and res.data:, but an empty list evaluates to False, so queries returning no tickets will skip caching and hit Supabase on every request.
🔧 Proposed fix
- if redis_client and res.data:
+ if redis_client and res.data is not None:
try:
redis_client.setex(cache_key, 300, json.dumps(res.data))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if redis_client and res.data: | |
| try: | |
| redis_client.setex(cache_key, 300, json.dumps(res.data)) | |
| except Exception as ce: | |
| print(f"[WARNING] Redis write error for {cache_key}: {ce}") | |
| if redis_client and res.data is not None: | |
| try: | |
| redis_client.setex(cache_key, 300, json.dumps(res.data)) | |
| except Exception as ce: | |
| print(f"[WARNING] Redis write error for {cache_key}: {ce}") |
🧰 Tools
🪛 Ruff (0.15.14)
[warning] 596-596: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/main.py` around lines 593 - 597, The current guard `if redis_client
and res.data:` skips caching when res.data is an empty list; change it to check
for existence rather than truthiness (e.g., `res.data is not None`) so empty
lists are cached; update the block around `redis_client`, `res.data`,
`cache_key`, and the `setex` call to use that non-None check and keep the
existing try/except for `setex` errors.
| list_keys = redis_client.keys("tickets:list:*") | ||
| if list_keys: | ||
| redis_client.delete(*list_keys) |
There was a problem hiding this comment.
KEYS command blocks Redis in production.
Line 776 uses redis_client.keys("tickets:list:*"), which scans all keys and blocks Redis until complete. With many cache entries, this can freeze Redis and impact all users.
🔄 Recommended fix using SCAN
- list_keys = redis_client.keys("tickets:list:*")
- if list_keys:
- redis_client.delete(*list_keys)
+ # Use SCAN to avoid blocking Redis
+ cursor = 0
+ while True:
+ cursor, keys = redis_client.scan(cursor, match="tickets:list:*", count=100)
+ if keys:
+ redis_client.delete(*keys)
+ if cursor == 0:
+ break
print(f"[Redis Cache] Invalidated detail and list caches on update for ticket: {ticket_id}")Alternatively, track affected company IDs in the update payload and delete only specific keys instead of using wildcards.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| list_keys = redis_client.keys("tickets:list:*") | |
| if list_keys: | |
| redis_client.delete(*list_keys) | |
| # Use SCAN to avoid blocking Redis | |
| cursor = 0 | |
| while True: | |
| cursor, keys = redis_client.scan(cursor, match="tickets:list:*", count=100) | |
| if keys: | |
| redis_client.delete(*keys) | |
| if cursor == 0: | |
| break |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/main.py` around lines 776 - 778, The code calls
redis_client.keys("tickets:list:*") which blocks Redis; replace this with a
non-blocking SCAN-based approach (e.g., use redis_client.scan_iter or scan with
cursor) to iterate matching keys and collect or delete them in batches, and then
call redis_client.delete in batched chunks (instead of deleting all at once).
Alternatively, if the update payload contains specific company IDs, use those
IDs to construct exact keys like "tickets:list:{company_id}" and delete only
those keys rather than using the wildcard; update the logic around
redis_client.keys, redis_client.delete, and the cache invalidation path to use
scan_iter/batched delete or targeted deletes by company ID.
| def setUp(self): | ||
| # Save original clients to restore after tests | ||
| self.orig_redis = main.redis_client | ||
| self.orig_supabase = main.supabase | ||
| self.orig_tickets_db = list(main.TICKETS_DB) | ||
| self.orig_duplicate_service = main.duplicate_service | ||
|
|
||
| # Create mocks | ||
| self.mock_redis = MagicMock() | ||
| self.mock_supabase = MagicMock() | ||
| self.mock_duplicate_service = MagicMock() | ||
|
|
||
| main.redis_client = self.mock_redis | ||
| main.supabase = self.mock_supabase | ||
| main.duplicate_service = self.mock_duplicate_service | ||
| main.TICKETS_DB.clear() | ||
|
|
||
| def tearDown(self): | ||
| # Restore original clients | ||
| main.redis_client = self.orig_redis | ||
| main.supabase = self.orig_supabase | ||
| main.TICKETS_DB = self.orig_tickets_db | ||
| main.duplicate_service = self.orig_duplicate_service |
There was a problem hiding this comment.
tearDown reference reassignment may not restore state correctly.
Line 156 reassigns main.TICKETS_DB = self.orig_tickets_db, but if tests modified the original list reference captured in setUp (line 139), those modifications persist. Use a copy or clear/extend pattern instead.
🔧 Proposed fix
def setUp(self):
# Save original clients to restore after tests
self.orig_redis = main.redis_client
self.orig_supabase = main.supabase
- self.orig_tickets_db = list(main.TICKETS_DB)
+ self.orig_tickets_db = list(main.TICKETS_DB) # Create a copy
self.orig_duplicate_service = main.duplicate_service
# Create mocks
self.mock_redis = MagicMock()
self.mock_supabase = MagicMock()
self.mock_duplicate_service = MagicMock()
main.redis_client = self.mock_redis
main.supabase = self.mock_supabase
main.duplicate_service = self.mock_duplicate_service
main.TICKETS_DB.clear()
def tearDown(self):
# Restore original clients
main.redis_client = self.orig_redis
main.supabase = self.orig_supabase
- main.TICKETS_DB = self.orig_tickets_db
+ main.TICKETS_DB.clear()
+ main.TICKETS_DB.extend(self.orig_tickets_db)
main.duplicate_service = self.orig_duplicate_service🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scratch/test_caching.py` around lines 135 - 157, The tearDown currently does
main.TICKETS_DB = self.orig_tickets_db which can reattach a mutated original
list; instead capture a shallow copy in setUp (e.g., self.orig_tickets_db =
list(main.TICKETS_DB)) and in tearDown restore the contents with clear/extend
semantics (e.g., main.TICKETS_DB.clear();
main.TICKETS_DB.extend(self.orig_tickets_db)) so that the TICKETS_DB reference
remains the same but its contents are reliably restored; apply this change
around the setUp and tearDown handling of main.TICKETS_DB and keep restoration
for main.redis_client, main.supabase, and main.duplicate_service as-is.
| payload = TicketSaveRequest( | ||
| user_id="843dfe99-70dd-4283-8eaf-c1bc70047b59", | ||
| subject="Test Subject", | ||
| description="Test Description", | ||
| category="Software", | ||
| subcategory="General", | ||
| priority="Low", | ||
| assigned_team="Software Team", | ||
| status="pending", | ||
| auto_resolve=False, | ||
| is_duplicate=False, | ||
| confidence=0.95, | ||
| company="RITESH PVT LTD", | ||
| company_id="76d16bf6-2ee9-44ad-b64e-ad5ecf0a079b", | ||
| is_potential_duplicate=False, | ||
| parent_ticket_id=None, | ||
| sla_breach_at="2026-06-01T00:00:00Z", | ||
| routing_confidence=0.95, | ||
| metadata={} | ||
| ) |
There was a problem hiding this comment.
Test payload includes undefined model fields.
Lines 279-280 use is_potential_duplicate and parent_ticket_id, but these fields are not defined in the TicketSaveRequest model (lines 126-148 in main.py). This will cause Pydantic validation errors when the test runs.
🐛 Proposed fix
payload = TicketSaveRequest(
user_id="843dfe99-70dd-4283-8eaf-c1bc70047b59",
subject="Test Subject",
description="Test Description",
category="Software",
subcategory="General",
priority="Low",
assigned_team="Software Team",
status="pending",
auto_resolve=False,
is_duplicate=False,
confidence=0.95,
company="RITESH PVT LTD",
company_id="76d16bf6-2ee9-44ad-b64e-ad5ecf0a079b",
- is_potential_duplicate=False,
- parent_ticket_id=None,
sla_breach_at="2026-06-01T00:00:00Z",
routing_confidence=0.95,
metadata={}
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| payload = TicketSaveRequest( | |
| user_id="843dfe99-70dd-4283-8eaf-c1bc70047b59", | |
| subject="Test Subject", | |
| description="Test Description", | |
| category="Software", | |
| subcategory="General", | |
| priority="Low", | |
| assigned_team="Software Team", | |
| status="pending", | |
| auto_resolve=False, | |
| is_duplicate=False, | |
| confidence=0.95, | |
| company="RITESH PVT LTD", | |
| company_id="76d16bf6-2ee9-44ad-b64e-ad5ecf0a079b", | |
| is_potential_duplicate=False, | |
| parent_ticket_id=None, | |
| sla_breach_at="2026-06-01T00:00:00Z", | |
| routing_confidence=0.95, | |
| metadata={} | |
| ) | |
| payload = TicketSaveRequest( | |
| user_id="843dfe99-70dd-4283-8eaf-c1bc70047b59", | |
| subject="Test Subject", | |
| description="Test Description", | |
| category="Software", | |
| subcategory="General", | |
| priority="Low", | |
| assigned_team="Software Team", | |
| status="pending", | |
| auto_resolve=False, | |
| is_duplicate=False, | |
| confidence=0.95, | |
| company="RITESH PVT LTD", | |
| company_id="76d16bf6-2ee9-44ad-b64e-ad5ecf0a079b", | |
| sla_breach_at="2026-06-01T00:00:00Z", | |
| routing_confidence=0.95, | |
| metadata={} | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scratch/test_caching.py` around lines 265 - 284, The test payload for
TicketSaveRequest includes undefined fields is_potential_duplicate and
parent_ticket_id which will trigger Pydantic validation errors; either remove
these keys from the payload in scratch/test_caching.py or add them as optional
fields on the TicketSaveRequest model (in main.py) with appropriate types (e.g.,
is_potential_duplicate: Optional[bool] = False and parent_ticket_id:
Optional[str] = None) so the test and model definitions match. Ensure you update
only the TicketSaveRequest class or the test payload (not both) and keep
types/defaults consistent with existing fields like auto_resolve and parent
semantics.
|
Hi @singhanurag0317-bit! 🙌 Thank you so much for your excellent contribution: "feat: implement Redis caching for database queries (closes #396)"! We really appreciate the high-quality code and effort you have put into the platform. Just a quick, friendly heads-up as we prepare our manual merging and verification queues—please make sure to complete all the mandatory community steps listed below. Once those manual steps are verified, we'll get your PR officially merged into the Let's build something amazing together! 🚀🔥 🌟 Community Support & Network Steps (Take 10 Seconds!)As we prepare our manual verification and merging queues, please make sure you have taken a moment to complete these required steps to finalize your points:
Note: Having these steps completed manually is required before your PR points are officially cleared. |
ritesh-1918
added
gssoc
|
Hi @singhanurag0317-bit! 🙌 Thank you so much for your excellent contribution: "feat: implement Redis caching for database queries (closes #396)"! We really appreciate the high-quality code and effort you have put into the platform. Just a quick, friendly heads-up as we prepare our manual merging and verification queues—please make sure to complete all the mandatory community steps listed below. Once those manual steps are verified, we'll get your PR officially merged into the Let's build something amazing together! 🚀🔥 🌟 Community Support & Network Steps (Take 10 Seconds!)As we prepare our manual verification and merging queues, please make sure you have taken a moment to complete these required steps to finalize your points:
Note: Having these steps completed manually is required before your PR points are officially cleared. |
2 participants
Description
This pull request implements a highly resilient Redis caching layer for database queries to address Issue #396 ("Feature: Implement Redis caching for database queries in HELPDESK.AI"). It minimizes Supabase database overhead by caching high-frequency read queries with auto-invalidation on inserts and updates, and guarantees full resilience in environments where a Redis instance is offline.
Key Features
redis>=5.0.0withinbackend/main.py. The connection initializes gracefully using theREDIS_URLenvironment variable, returningredis_client = Noneon failure to ensure zero boot impact if Redis is unavailable.get_system_settingsunder keysystem_settings:{company_id}for 300 seconds (5 minutes).get_ticketsunder keytickets:list:{company_id or 'all'}for 300 seconds.get_ticket_by_idunder keytickets:detail:{ticket_id}for 300 seconds.tickets:list:*) upon ticket creation insidesave_ticket.tickets:detail:{ticket_id}) and list caches (tickets:list:*) upon ticket updates insideupdate_ticket.Noneor encounters runtime errors, the backend outputs a non-blocking warning and falls back to direct Supabase database operations.Verification
python -m py_compile backend/main.pyensuring clear compilation and imports.scratch/test_caching.pyverifying all caching flows, fallback logic, and invalidation behaviors (7 tests passed).Summary by CodeRabbit
Release Notes
New Features
Tests
Chores