feat(api): cache markets endpoint for 5 minutes#136
Conversation
|
@ayushchoudhary01 is attempting to deploy a commit to the karan3431's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
🎉 Thank you for your Pull Request! We're thrilled to have your contribution to FreshScan AI. Before we review, please make sure you have:
A maintainer will review your code as soon as possible! |
|
Warning
|
| Layer / File(s) | Summary |
|---|---|
Dependency and cache initialization backend/requirements.txt, backend/main.py |
Adds fastapi-cache2==0.2.2 to requirements, imports FastAPICache, InMemoryBackend, and cache in main.py, and initializes FastAPICache with InMemoryBackend during app lifespan startup. |
Cached markets endpoint backend/main.py |
Extracts the Supabase vendor query and response formatting into _get_markets_cached() decorated with @cache(expire=300, namespace="markets"), and updates the /api/v1/maps/markets route to delegate to that helper. |
Cache invalidation on trust score update backend/vendors.py |
Imports FastAPICache and inserts an awaited FastAPICache.clear(namespace="markets") call after vendor trust score fields are written to the database inside recalculate_trust_score. |
Cache unit tests backend/tests/test_cache.py |
Adds an autouse fixture initializing FastAPICache/InMemoryBackend, and four async tests covering: same-value within TTL, expiry after TTL, explicit namespace clear forcing recompute, and non-caching of raised exceptions. |
Sequence Diagram(s)
sequenceDiagram
participant Client
participant MarketsRoute as /api/v1/maps/markets
participant CacheHelper as _get_markets_cached
participant InMemoryBackend
participant Supabase
Client->>MarketsRoute: GET /api/v1/maps/markets
MarketsRoute->>CacheHelper: await _get_markets_cached()
CacheHelper->>InMemoryBackend: lookup namespace=markets
alt Cache hit (within 300s)
InMemoryBackend-->>CacheHelper: cached payload
else Cache miss
CacheHelper->>Supabase: query vendor map data
Supabase-->>CacheHelper: vendor rows
CacheHelper->>InMemoryBackend: store result (TTL 300s)
CacheHelper-->>CacheHelper: build markets list
end
CacheHelper-->>MarketsRoute: markets payload
MarketsRoute-->>Client: JSON response
Note over vendors.py,InMemoryBackend: On trust score recalculation
vendors.py->>InMemoryBackend: FastAPICache.clear(namespace="markets")
InMemoryBackend-->>vendors.py: cache invalidated
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
- jpdevhub/FreshScanAi#111: Modifies the same
/api/v1/maps/marketshandler inbackend/main.py, addingslowapirate-limiting to the endpoint that this PR now wraps with caching.
Suggested labels
enhancement, backend, Hard
Suggested reviewers
- jpdevhub
🐇 Five minutes in memory, the markets shall stay,
No Supabase calls on each vendor display!
Trust scores recalculate — poof — cache goes clear,
In-memory magic keeps the map data near.
A fresh-scan rabbit hops faster today! 🥕
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely summarizes the main change: implementing a 5-minute cache for the markets endpoint. |
| Linked Issues check | ✅ Passed | The PR fully implements issue #128 requirements: caches /api/v1/maps/markets for 5 minutes using fastapi-cache2 in-memory backend, reducing database load as specified. |
| Out of Scope Changes check | ✅ Passed | All changes are directly scoped to implementing the caching requirement: FastAPICache initialization, cached endpoint implementation, cache invalidation on vendor updates, and cache tests. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Line 793: The condition checking v.get("lat") and v.get("lng") uses truthiness
evaluation which incorrectly filters out valid coordinates when the value is 0,
since 0 is falsy in Python. Change the condition to explicitly check whether the
values are not None instead of relying on truthiness, so that coordinates with
value 0 are properly included.
In `@backend/vendors.py`:
- Around line 132-133: The FastAPICache.clear() call with namespace "markets" is
not wrapped in error handling, which allows cache backend failures to propagate
and fail the entire write operation. Wrap the await
FastAPICache.clear(namespace="markets") call in its own try-except block to
catch any exceptions, optionally logging them for debugging purposes, but
allowing the function to continue and return success since the database write
itself was successful and cache invalidation should be treated as a best-effort
operation independent from the primary database operation.
🪄 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 Plus
Run ID: e94afc55-843a-44dc-a53d-4b5e537ae48e
📒 Files selected for processing (4)
backend/main.pybackend/requirements.txtbackend/tests/test_cache.pybackend/vendors.py
| "vendors": int(v.get("vendor_count") or 1), | ||
| } | ||
| for i, v in enumerate(resp.data or []) | ||
| if v.get("lat") and v.get("lng") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Zero coordinates are incorrectly filtered out.
Line 793 uses truthiness checks, so valid 0 latitude/longitude values are excluded. Use explicit None checks.
Proposed fix
- if v.get("lat") and v.get("lng")
+ if v.get("lat") is not None and v.get("lng") is not None📝 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 v.get("lat") and v.get("lng") | |
| if v.get("lat") is not None and v.get("lng") is not None |
🤖 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` at line 793, The condition checking v.get("lat") and
v.get("lng") uses truthiness evaluation which incorrectly filters out valid
coordinates when the value is 0, since 0 is falsy in Python. Change the
condition to explicitly check whether the values are not None instead of relying
on truthiness, so that coordinates with value 0 are properly included.
| await FastAPICache.clear(namespace="markets") | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, explore the file structure and understand the context
fd -t f "vendors.py" backend/
# Check file size
wc -l backend/vendors.py
# Map the file structure with ast-grep
ast-grep outline backend/vendors.py --view compactRepository: jpdevhub/FreshScanAi
Length of output: 375
🏁 Script executed:
# Read the context around lines 132-133
sed -n '120,145p' backend/vendors.py
# Also map the file structure with correct view option
ast-grep outline backend/vendors.py --view expandedRepository: jpdevhub/FreshScanAi
Length of output: 1278
🏁 Script executed:
# Get the full function to see the complete picture
sed -n '60,145p' backend/vendors.pyRepository: jpdevhub/FreshScanAi
Length of output: 3413
🏁 Script executed:
# Search for other cache clear patterns in the codebase
rg "FastAPICache.clear" backend/ -A 2 -B 2
# Check if there are other cache operations that might have error handling
rg "FastAPICache" backend/ -A 2 -B 2Repository: jpdevhub/FreshScanAi
Length of output: 2532
Wrap cache eviction in try-except to prevent successful writes from failing.
The await FastAPICache.clear() call at lines 132-133 is not wrapped in its own error handling. If cache eviction fails, the exception propagates to the outer catch-all handler and converts the successful database update into a 500 response. Cache invalidation should be best-effort (with optional logging) so that write success is independent from cache backend failures.
Proposed fix
- await FastAPICache.clear(namespace="markets")
+ try:
+ await FastAPICache.clear(namespace="markets")
+ except Exception:
+ # Keep DB write success independent from cache backend failures.
+ pass📝 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.
| await FastAPICache.clear(namespace="markets") | |
| try: | |
| await FastAPICache.clear(namespace="markets") | |
| except Exception: | |
| # Keep DB write success independent from cache backend failures. | |
| pass | |
🤖 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/vendors.py` around lines 132 - 133, The FastAPICache.clear() call
with namespace "markets" is not wrapped in error handling, which allows cache
backend failures to propagate and fail the entire write operation. Wrap the
await FastAPICache.clear(namespace="markets") call in its own try-except block
to catch any exceptions, optionally logging them for debugging purposes, but
allowing the function to continue and return success since the database write
itself was successful and cache invalidation should be treated as a best-effort
operation independent from the primary database operation.
1 participant
Description
Closes #128.
Implements a 5-minute caching layer on the
/api/v1/maps/marketsendpoint to reduce database load on this frequently-hit route, as described in the issue.Approach:
fastapi-cache2withInMemoryBackend, as discussed in the issue thread — no Redis instance is currently provisioned on this repo's deployment target (HF Spaces/Render/Railway), so in-memory avoids adding new infra while still satisfying the issue's "Redis or In-Memory" requirement.Changes:
backend/main.py:FastAPICache.init(InMemoryBackend(), prefix="freshscanai-cache")added to the existinglifespancontext manager.get_marketsinto a thin route handler (get_markets) and a cached helper (_get_markets_cached, decorated with@cache(expire=300, namespace="markets")). This split ensures the DB-failure fallback response is never cached — only successful DB reads are stored, so a transient Supabase error doesn't get served back for the full 5 minutes.backend/vendors.py:recalculate_trust_scorenow callsawait FastAPICache.clear(namespace="markets")after a successful vendor update, so the map reflects new trust scores immediately rather than waiting out the TTL.backend/requirements.txt: addedfastapi-cache2==0.2.2.backend/tests/test_cache.py(new): unit tests against the realfastapi_cache2API — cache hit/miss, TTL expiry,FastAPICache.clear()invalidation, and confirming exceptions are never cached.Note for maintainer:
backend/tests/test_cache.pyis a new file and is not yet included in.github/workflows/ci.yml's test command (pytest tests/test_ci.py, which runs only that one file by name). I've verified it manually (pytest tests/test_cache.py -v, all passing) but wanted to flag this rather than modifyci.ymlwithout discussion. Happy to add it to the CI command if preferred.Checklist
npm run lintpasses with no errors (no frontend changes in this PR)npm run buildcompiles without TypeScript errors (no frontend changes in this PR)python -m pytestpasses (including new tests I added) — verifiedtests/test_ci.py(9 passed) andtests/test_cache.py(4 passed).envfiles, API keys, secrets, model weights, or__pycache__in this diffmain, not mergedSummary by CodeRabbit
New Features
Tests
Chores