Avoid ActiveStorage n+1s#6477
Closed
BenAkroyd wants to merge 5 commits into
Closed
Conversation
Returns the right preload args for the configured image attachment backend (deep AS chain in ActiveStorage mode, [] in Paperclip mode), letting callers preload uniformly without branching on configuration.
Thread Spree::Asset.attachment_preloads through the include_images preload so search results don't N+1 on AS attachment + blob lookups. Adds a regression spec.
github-actions
Bot
added
changelog:solidus_api
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6477 +/- ##
=======================================
Coverage 89.68% 89.68%
=======================================
Files 991 991
Lines 20832 20835 +3
=======================================
+ Hits 18683 18686 +3
Misses 2149 2149 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Thread Spree::Asset.attachment_preloads through product_includes and StockItemsController#variant_scope so styled thumbnail URLs don't N+1 under ActiveStorage.
…trollers Thread Spree::Asset.attachment_preloads through the products index and the orders variant picker so styled thumbnail URLs don't N+1 under ActiveStorage. Replaces the bespoke ActiveStorage conditional in OrdersController#variants_for with the helper.
Thread Spree::Asset.attachment_preloads through find_order so variant image URLs in the order JSON response don't N+1 under ActiveStorage.
jarednorman
previously approved these changes
May 1, 2026
tvdeyen
requested changes
May 1, 2026
tvdeyen
left a comment
tvdeyen
left a comment
Member
There was a problem hiding this comment.
Very nice, Ben. Thanks. I think it would be more future proof if we would add the preload config into the appropriate adapter. Also ActiveStorage has a build in preload method that is named after the has_one_attached association
| def self.attachment_preloads | ||
| return [] unless reflect_on_association(:attachment_attachment) | ||
|
|
||
| [{attachment_attachment: {blob: {variant_records: {image_attachment: :blob}}}}] |
Member
There was a problem hiding this comment.
We can make use of the ActiveStorage preloading
with_attached_attachmentAlso, maybe we should add this into the adapter instead of making the duck typing here.
Contributor
Author
|
thanks yall. @tvdeyen I've done something similar on CS's repo. I'll update today/tomorrow here |
Member
|
@BenAkroyd do you want to continue working on this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Spree::Asset (and therefore Image) can be setup for ActiveStorage or paperclip. When its ActiveStorage, there are extra classes involved- attachment, blob, and in some cases, variant. None of those are preloaded currently in most places.
As things would break if preload were attempted and kt-paperclip was being used, a helper method has been added that checks config, and defines the appropriate includes. This helper is referenced in a variety of controllers, addressing some n+1s.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: