Add cache to /endpoints/publisher/listing#5462
Conversation
|
|
||
| ## Getting Started | ||
|
|
||
| 1. Ensure Docker is installed. |
There was a problem hiding this comment.
every time we run snapcraft locally do we need to do this? or is it just for testing redis?
There was a problem hiding this comment.
This is for testing redis locally only.
There was a problem hiding this comment.
Pull Request Overview
This PR adds Redis-based caching to the publisher listing endpoint to improve performance by caching snap metadata retrieved from the dashboard API.
- Introduces a new
cache/module with Redis utility wrapper and Docker Compose setup for local development - Implements caching in
get_listing_data()with a 1-hour TTL, keyed by user ID and snap name - Adds cache invalidation when metadata or screenshots are successfully updated via
post_listing_data()
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| webapp/endpoints/publisher/listing.py | Integrates Redis caching for snap info retrieval and adds cache invalidation after successful updates |
| cache/cache_utility.py | Creates RedisCache instance with namespace and TTL configuration |
| cache/docker-compose.yaml | Provides Docker Compose setup for local Redis development server |
| cache/README.md | Documents how to run and interact with local Redis for development |
| 5. To stop the container, run: | ||
| ``` | ||
| docker compose down | ||
| ``` |
There was a problem hiding this comment.
Missing information about environment variable configuration. The cache utility requires Redis connection settings (host, port, etc.), but the README doesn't explain how to configure these for local development.
Consider adding a section explaining required environment variables or configuration, such as:
## Configuration
The cache utility connects to Redis using the following environment variables:
- `REDIS_HOST` (default: localhost)
- `REDIS_PORT` (default: 6379)
Make sure Redis is running before starting the application.
| ``` |
Configuration
The cache utility connects to Redis using the following environment variables:
REDIS_HOST(default: localhost)REDIS_PORT(default: 6379)
Make sure Redis is running before starting the application.
| snap_details = redis_cache.get(snap_info_key, expected_type=dict) | ||
|
|
||
| if not snap_details: | ||
| snap_details = dashboard.get_snap_info(flask.session, snap_name) | ||
| redis_cache.set(snap_info_key, snap_details, ttl=3600) |
There was a problem hiding this comment.
Missing error handling for cache operations. If Redis is unavailable or encounters an error, the redis_cache.get() call could raise an exception, causing the entire request to fail. The application should gracefully fall back to fetching data directly from the API when the cache is unavailable.
Consider wrapping cache operations in try-except blocks:
snap_info_key = get_snap_info_key(snap_name)
snap_details = None
try:
snap_details = redis_cache.get(snap_info_key, expected_type=dict)
except Exception:
# Log the error and continue without cache
pass
if not snap_details:
snap_details = dashboard.get_snap_info(flask.session, snap_name)
try:
redis_cache.set(snap_info_key, snap_details, ttl=3600)
except Exception:
# Log the error but don't fail the request
pass| snap_details = redis_cache.get(snap_info_key, expected_type=dict) | |
| if not snap_details: | |
| snap_details = dashboard.get_snap_info(flask.session, snap_name) | |
| redis_cache.set(snap_info_key, snap_details, ttl=3600) | |
| snap_details = None | |
| try: | |
| snap_details = redis_cache.get(snap_info_key, expected_type=dict) | |
| except Exception: | |
| # Optionally log the error, but continue without cache | |
| pass | |
| if not snap_details: | |
| snap_details = dashboard.get_snap_info(flask.session, snap_name) | |
| try: | |
| redis_cache.set(snap_info_key, snap_details, ttl=3600) | |
| except Exception: | |
| # Optionally log the error, but don't fail the request | |
| pass |
| # Invalidate cache after successful metadata update | ||
| snap_info_key = get_snap_info_key(snap_name) | ||
| redis_cache.delete(snap_info_key) |
There was a problem hiding this comment.
Missing error handling for cache invalidation. If Redis is unavailable, the redis_cache.delete() call could raise an exception, potentially causing a successful metadata update to appear as failed to the user.
Consider wrapping the cache delete operation in a try-except block:
# Invalidate cache after successful metadata update
snap_info_key = get_snap_info_key(snap_name)
try:
redis_cache.delete(snap_info_key)
except Exception:
# Log the error but don't fail the request
pass| # Invalidate cache after successful screenshot update | ||
| snap_info_key = get_snap_info_key(snap_name) | ||
| redis_cache.delete(snap_info_key) |
There was a problem hiding this comment.
Missing error handling for cache invalidation. If Redis is unavailable, the redis_cache.delete() call could raise an exception, potentially causing a successful screenshot update to appear as failed to the user.
Consider wrapping the cache delete operation in a try-except block:
# Invalidate cache after successful screenshot update
snap_info_key = get_snap_info_key(snap_name)
try:
redis_cache.delete(snap_info_key)
except Exception:
# Log the error but don't fail the request
pass| user_id = flask.session.get( | ||
| "publisher", {} | ||
| ).get("identity_url", "").split("+id")[1] | ||
|
|
There was a problem hiding this comment.
The split("+id")[1] operation will raise an IndexError if the identity_url doesn't contain "+id" or if it ends with "+id". This can occur if the session data is malformed or if the identity URL format changes.
Consider adding error handling or validation:
def get_snap_info_key(snap_name):
identity_url = flask.session.get("publisher", {}).get("identity_url", "")
if "+id" not in identity_url:
raise ValueError("Invalid identity_url format")
user_id = identity_url.split("+id")[1]
if not user_id:
raise ValueError("User ID not found in identity_url")
return f"{user_id}:snap_info:{snap_name}"| user_id = flask.session.get( | |
| "publisher", {} | |
| ).get("identity_url", "").split("+id")[1] | |
| identity_url = flask.session.get("publisher", {}).get("identity_url", "") | |
| if "+id" not in identity_url: | |
| raise ValueError("Invalid identity_url format: '+id' not found") | |
| parts = identity_url.split("+id", 1) | |
| if len(parts) < 2 or not parts[1]: | |
| raise ValueError("User ID not found in identity_url") | |
| user_id = parts[1] |
alvaromateo
left a comment
There was a problem hiding this comment.
LGTM
The only thing that I would like to add if possible are a bit of logging for when we set/delete info in Redis, to be able to track the flow of our program properly. If I remember correctly there were some logs already in the RedisCache utility? Otherwise we should add them.
@alvaromateo we have logs already in the RedisCache utility in storeAPI module |
I would update the failing tests and ping you for re-approval |
81aec92 to
e72fec8
Compare
95e174f to
f1ae166
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5462 +/- ##
==========================================
- Coverage 66.80% 0 -66.81%
==========================================
Files 113 0 -113
Lines 3714 0 -3714
Branches 965 0 -965
==========================================
- Hits 2481 0 -2481
+ Misses 1098 0 -1098
+ Partials 135 0 -135 🚀 New features to boost your workflow:
|
Done
How to QA
Testing
Issue / Card
Fixes #
Screenshots