Prevent UI crash on FileNotFound errors with Active Storage #4103
Conversation
core/app/models/concerns/spree/active_storage_adapter/attachment.rb
Outdated
Show resolved
Hide resolved
cpfergus1
changed the title
cpfergus1
force-pushed
the
patch-image-file-not-found
branch
3 times, most recently
from
01503ad to
072b0aa
Compare
core/app/models/concerns/spree/active_storage_adapter/attachment.rb
Outdated
Show resolved
Hide resolved
cpfergus1
force-pushed
the
patch-image-file-not-found
branch
5 times, most recently
from
e75b537 to
0d8ad7c
Compare
There was a problem hiding this comment.
@cpfergus1 thanks for working on this issue!
Unless we want to patch ActiveStorage directly, this change looks reasonable to me.
Also, having nil instead of 0 for height and width seems to be the right way to me.
On the other hand, not really sure we want nil for identified and analyzed, for example the latter seems to be always set to true by rails, after extracting the image metadata.
Do you have any opinion on this?
My thoughts on this are that if the object cannot be found, it cannot be analyzed or identified since it doesn't exist. This behavior can be witnessed on images_controller_spec.rb when examining the image blob. If you attempt to run analyze on it - it will fail due to the image not being found, so the object has not been analyzed. |
cpfergus1
force-pushed
the
patch-image-file-not-found
branch
7 times, most recently
from
a3aa5f6 to
00eb433
Compare
There was a problem hiding this comment.
Many thanks for your continuous work on it, @cpfergus1! A last minor suggestion, it would be nice to add the id of the attachment in the warning message, so that users can't more easily find it.
It'd be also better to rebase from master before merging 🙏
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.
cpfergus1
force-pushed
the
patch-image-file-not-found
branch
from
00eb433 to
c2b06a5
Compare
waiting-for-dev
added
changelog:solidus_api
|
Thanks, @cpfergus1. We only need another review. Pinging @kennyadsl as he also looked into it. |
Description
In a previous PR #4026, a fix was implemented to prevent an application from crashing by capturing an exception and providing a replacement image to prevent a non-operable UI. In issue #4095 it was found that the implemented fix did not capture all failure points that may be caused by a corrupt or FileNotFound issue.
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. This compliments PR #4026 which prevented the same error on the backend. The change will return a json image package with attributes equal to nil and noimage replacements if an
image cannot be sourced. This should be back-ported to 3.0 if cleared for merging.
Checklist: