Skip to content

Commit

Permalink
Implemented fix for FileNotFound errors with Active Storage
Browse files Browse the repository at this point in the history
This commit adds the ability for the application to catch missing
images with Active Storage during an API call should a file become
corrupt or deleted without removing the record through appropriate channels.
This compliments PR solidusio#4026 which prevented the same error on the backend.
The change will return and image with attributes equal to nil and
noimage replacements if an image cannot be sourced.

Reasons for the changes:
- image.attachment.url bypasses rescue check found in ActiveStorageAdapter
- API calls hit height and width prior to url rescue, had to implement
rescue in Attachment#metadata.
- metadata returns hash with strings, not symbols. It appeared that
this caused nil values to be returned regardless if the image was good or not.
  • Loading branch information
cpfergus1 committed Jun 15, 2021
1 parent 3f4a210 commit 1f7d6e0
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 4 deletions.
2 changes: 1 addition & 1 deletion api/app/views/spree/api/images/_image.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@
json.(image, *image_attributes)
json.(image, :viewable_type, :viewable_id)
Spree::Image.attachment_definitions[:attachment][:styles].each do |key, _value|
json.set! "#{key}_url", image.attachment.url(key)
json.set! "#{key}_url", image.url(key)
end
10 changes: 10 additions & 0 deletions api/spec/requests/spree/api/images_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ module Spree
expect(response.status).to eq(204)
expect { product_image.reload }.to raise_error(ActiveRecord::RecordNotFound)
end

it "It returns nil attribute values and noimage urls when the image cannot be found",
if: Spree::Config.image_attachment_module == Spree::Image::ActiveStorageAttachment do
product_image.attachment.blob.update(key: 11)
get spree.api_variant_images_path(product.master.id)
expect(response.status).to eq(200)
expect(json_response[:images].first[:attachment_width]).to be_nil
expect(json_response[:images].first[:attachment_height]).to be_nil
expect(json_response[:images].first[:product_url]).to include("noimage")
end
end

context 'when image belongs to another product' do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ def variant(style = nil)
end

def height
metadata[:height]
metadata["height"]
end

def width
metadata[:width]
metadata["width"]
end

def destroy
Expand All @@ -53,8 +53,9 @@ def destroy

def metadata
analyze unless analyzed?

@attachment.metadata
rescue ActiveStorage::FileNotFoundError
{"identified"=>nil, "width"=>nil, "height"=>nil, "analyzed"=>false}
end

def normalize_styles(styles)
Expand Down

0 comments on commit 1f7d6e0

Please sign in to comment.