Do not store thumbnails in cache if there are errors in the response #2148
Conversation
📝 WalkthroughWalkthroughThe photos controller's thumbnail caching mechanism was refactored to replace a Rails.cache.fetch block with an explicit read-check pattern. The method now checks the cache first, and if empty, computes the thumbnail response, caches it on success, and then returns the response in all scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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
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 review |
✅ Actions performedReview triggered.
|
![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
🤖 Fix all issues with AI agents
In @app/controllers/api/v1/photos_controller.rb:
- Around line 23-24: Cache key for photo thumbnails omits params[:source],
causing collisions across sources; update both the cache read and write to
include the source (e.g., append params[:source] or a normalized default) so the
key uniquely identifies source + id (use the same composite key when reading and
writing, and guard/normalize params[:source] to avoid nil/unsafe values).
- Around line 26-30: The cached value must be a plain hash rather than the
HTTParty::Response object; change the Rails.cache.write call in
Photos::Thumbnail usage to store only the response fields, e.g. build a hash
like { body: response.body, code: response.code, success: response.success? }
keyed by "photo_thumbnail_#{params[:id]}", and ensure any code that reads from
cache (the same controller or any consumer) expects and uses that hash
(accessing .dig or hash keys instead of calling methods on an
HTTParty::Response).
🧹 Nitpick comments (2)
app/controllers/api/v1/photos_controller.rb (2)
28-30: Consider checking for empty response bodies in addition to success status.The PR description mentions preventing "empty images" from being cached, but the code only checks
response.success?. HTTP 200 responses can still have empty bodies. Consider adding an additional check for empty or invalid image data.💡 Suggested enhancement to check for empty bodies
- if response.success? + if response.success? && response.body.present? Rails.cache.write("photo_thumbnail_#{params[:id]}", response, expires_in: 1.day) end
28-30: Optional: Use modifier form for single-line conditional.RuboCop suggests using modifier
ifsyntax for improved readability when the body is a single line.♻️ RuboCop-compliant alternative
- if response.success? - Rails.cache.write("photo_thumbnail_#{params[:id]}", response, expires_in: 1.day) - end + Rails.cache.write("photo_thumbnail_#{params[:id]}", response, expires_in: 1.day) if response.success?Based on static analysis hints.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/v1/photos_controller.rb
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rb: Always handle location data with appropriate precision and privacy considerations
Leverage PostGIS features for geographic calculations rather than Ruby-based solutions
Use thelonlatcolumn in thepointstable for geographic calculations
Consider database indexes for geographic queries to improve performance
Never log or expose user location data inappropriatelyUse
bundle exec rubocopto enforce Ruby code formatting before commits
Files:
app/controllers/api/v1/photos_controller.rb
app/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
app/**/*.rb: When implementing features that interact with stats, use thepublic_accessible?method to check if a stat can be publicly accessed
Support UUID-based access in API endpoints when appropriate for public sharing
Respect expiration settings and disable sharing when expired for public stat sharing
Only expose minimal necessary data in public sharing contexts
app/**/*.rb: Use PostGIS geometries for imports, exports, sharing, and trip analytics
Use Devise for authentication and Pundit for authorization
Files:
app/controllers/api/v1/photos_controller.rb
**/*.{rb,js,ts,tsx,yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
Use two-space indentation
Files:
app/controllers/api/v1/photos_controller.rb
app/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Use snake_case for filenames
Files:
app/controllers/api/v1/photos_controller.rb
**/*.{rb,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use CamelCase for class names
Files:
app/controllers/api/v1/photos_controller.rb
app/**/*.{rb,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Queue non-trivial work to Sidekiq workers instead of blocking requests
Files:
app/controllers/api/v1/photos_controller.rb
🧬 Code graph analysis (1)
app/controllers/api/v1/photos_controller.rb (2)
app/controllers/api_controller.rb (1)
current_api_user(35-37)app/services/photos/thumbnail.rb (1)
call(12-17)
🪛 RuboCop (1.82.1)
app/controllers/api/v1/photos_controller.rb
[convention] 28-28: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
(Style/IfUnlessModifier)
| cached = Rails.cache.read("photo_thumbnail_#{params[:id]}") | ||
| return cached if cached |
There was a problem hiding this comment.
Cache key should include the source parameter.
The cache key "photo_thumbnail_#{params[:id]}" doesn't include the source parameter. If the same photo ID exists in both Immich and Photoprism, one source's thumbnail will overwrite the other's in the cache, causing incorrect thumbnails to be served.
🔧 Proposed fix to include source in cache key
- cached = Rails.cache.read("photo_thumbnail_#{params[:id]}")
+ cached = Rails.cache.read("photo_thumbnail_#{source}_#{params[:id]}")
return cached if cached
response = Photos::Thumbnail.new(current_api_user, source, params[:id]).call
if response.success?
- Rails.cache.write("photo_thumbnail_#{params[:id]}", response, expires_in: 1.day)
+ Rails.cache.write("photo_thumbnail_#{source}_#{params[:id]}", response, expires_in: 1.day)
end🤖 Prompt for AI Agents
In @app/controllers/api/v1/photos_controller.rb around lines 23 - 24, Cache key
for photo thumbnails omits params[:source], causing collisions across sources;
update both the cache read and write to include the source (e.g., append
params[:source] or a normalized default) so the key uniquely identifies source +
id (use the same composite key when reading and writing, and guard/normalize
params[:source] to avoid nil/unsafe values).
| response = Photos::Thumbnail.new(current_api_user, source, params[:id]).call | ||
|
|
||
| if response.success? | ||
| Rails.cache.write("photo_thumbnail_#{params[:id]}", response, expires_in: 1.day) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check Rails cache configuration in environment files
rg -n "config.cache_store" config/environments/
# Check for custom serializers
rg -n "serializer" config/Repository: Freika/dawarich
Length of output: 536
🏁 Script executed:
# Find and examine the Photos::Thumbnail class
fd -t f "thumbnail" app/ | head -20Repository: Freika/dawarich
Length of output: 91
🏁 Script executed:
# Search for Photos::Thumbnail class definition
rg -n "class.*Thumbnail" app/Repository: Freika/dawarich
Length of output: 117
🏁 Script executed:
# Check how responses are typically handled in the codebase with HTTParty
rg -n "HTTParty" app/ -A 3 -B 1 | head -50Repository: Freika/dawarich
Length of output: 2048
🏁 Script executed:
# Read the full Photos::Thumbnail class
cat -n app/services/photos/thumbnail.rbRepository: Freika/dawarich
Length of output: 1715
🏁 Script executed:
# Search for where the cached photo_thumbnail is retrieved/used
rg -n "photo_thumbnail" app/ -A 3 -B 1Repository: Freika/dawarich
Length of output: 1366
🏁 Script executed:
# Check if there's a method that retrieves and uses the cached response
rg -n "Rails.cache.read" app/controllers/api/v1/photos_controller.rb -A 3 -B 1Repository: Freika/dawarich
Length of output: 284
🏁 Script executed:
# Read the full photos_controller.rb to see how the response is used
cat -n app/controllers/api/v1/photos_controller.rbRepository: Freika/dawarich
Length of output: 2234
🏁 Script executed:
# Search for HTTParty response serialization in Rails
web_search "HTTParty Net::HTTPResponse serialize Redis Rails cache"Repository: Freika/dawarich
Length of output: 109
🌐 Web query:
HTTParty response Net::HTTPResponse serialize Redis Rails cache serialization issue
💡 Result:
Short answer: don’t cache the raw HTTParty/Net::HTTPResponse object — cache a plain serializable hash (body, code, headers, parsed_response) instead.
Why: HTTParty::Response tries to implement _dump/_load but it embeds request/response objects (Net::HTTPResponse/Net::HTTPRequest) that aren’t reliably Marshal/JSON serializable across Ruby versions or JVM/native extensions. Rails’ RedisCacheStore uses a coder (Marshal by default) and will fail or produce brittle data when asked to persist low-level Net::HTTP objects. (rubydoc.info)
Recommended pattern:
- Extract the pieces you need and store a simple Hash or JSON (safe for Redis).
- Rehydrate into a lightweight wrapper when reading back.
Example:
- write:
cached = { body: resp.body, code: resp.code.to_i, headers: resp.headers.to_h, parsed: resp.parsed_response }
Rails.cache.write(key, cached, expires_in: 10.minutes) - read:
cached = Rails.cache.read(key)
if cacheduse cached[:parsed] or build a minimal object
else
resp = HTTParty.get(url)
Rails.cache.write(key, { body: resp.body, code: resp.code.to_i, headers: resp.headers.to_h, parsed: resp.parsed_response })
end
If you must persist the whole HTTParty::Response, be aware HTTParty defines _dump/_load but it relies on Marshal for request/response internals — this is fragile; prefer serializing only primitives instead. (rubydoc.info)
If you want, tell me which fields you need and I’ll give a ready-made serializer/deserializer.
Citations:
- 1: https://www.rubydoc.info/gems/httparty/HTTParty/Response?utm_source=openai
- 2: https://www.rubydoc.info/gems/httparty/HTTParty/Response?utm_source=openai
🏁 Script executed:
# Check if there are any tests that show response caching/deserialization
rg -n "photo_thumbnail" app/ --type rb | head -20Repository: Freika/dawarich
Length of output: 85
🏁 Script executed:
# Look for any response handling patterns in the codebase
rg -n "response\.(body|code|success)" app/controllers/ -B 2 -A 2 | head -50Repository: Freika/dawarich
Length of output: 1133
🏁 Script executed:
# Search for other response caching patterns in the codebase
rg -n "Rails.cache.write.*response" app/ -A 1 -B 1Repository: Freika/dawarich
Length of output: 319
🏁 Script executed:
# Check if there are any error handling logs related to cache deserialization
rg -n "cache" app/controllers/ | grep -i "error\|fail\|rescue"Repository: Freika/dawarich
Length of output: 41
Cache only the response body and code, not the entire HTTParty response object.
HTTParty::Response embeds Net::HTTPResponse and Net::HTTPRequest objects that don't serialize reliably with Redis' default Marshal coder. When deserialized from cache, the object may lose methods or data, causing .success?, .body, or .code to fail unexpectedly. Extract and cache only the necessary fields as a plain hash: { body: response.body, code: response.code, success: response.success? }.
🧰 Tools
🪛 RuboCop (1.82.1)
[convention] 28-28: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
(Style/IfUnlessModifier)
🤖 Prompt for AI Agents
In @app/controllers/api/v1/photos_controller.rb around lines 26 - 30, The cached
value must be a plain hash rather than the HTTParty::Response object; change the
Rails.cache.write call in Photos::Thumbnail usage to store only the response
fields, e.g. build a hash like { body: response.body, code: response.code,
success: response.success? } keyed by "photo_thumbnail_#{params[:id]}", and
ensure any code that reads from cache (the same controller or any consumer)
expects and uses that hash (accessing .dig or hash keys instead of calling
methods on an HTTParty::Response).
1 participant
Closes #2060
Currently, the images loaded from Immich (I guess also Photoprism) are always stored in cache, no matter whether the server responded successfully or not.
This is a simple PR to make sure no errors (or empty images) are stored in cache.
I can't find where to suggest updates to the documentation, but this page must be updated to indicate that the
asset.viewis also required, as explained in the original issue.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.