Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't fail ImageAnalyzer on unsupported types #36072

Merged
merged 1 commit into from Apr 24, 2019

Conversation

Projects
None yet
3 participants
@itsWill
Copy link
Contributor

commented Apr 23, 2019

Summary

Fix: #36065

The ImageAnalyzer passes a image to ImageMagick without checking if the
image is supported by ImageMagick. This patch checks that image is
supported and if not logs an error and returns an empty hash instead of
raising an error. This is the same error handling we do when we
encounter a LoadError when mini_magick is not installed.

Other Information

This is a breaking change I was hoping to get a quick feedback on the idea and see if this is an approach worth persuing. We skip the image analysis on a LoadError so I thought we could skip it on an unsupported type as well.

One option is to also make this behaviour configurable.

Another option is to raise a generic ActiveStorage::UnsupportedImage error so clients don't have to rescue a low level MiniMagick::Error.

@rails-bot rails-bot bot added the activestorage label Apr 23, 2019

if image.valid?
yield image
else
logger.info "Skipping image analysis because mini_magick doesn't support the file"

This comment has been minimized.

Copy link
@itsWill

itsWill Apr 23, 2019

Author Contributor

I'm tempted to move the rescue LoadError here since having the logging happen in the two methods makes it a bit harder to read, if we like this approach I'm happy to amend the PR.

Note: this is also a breaking change since previously unsupported images raised an error.

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Apr 24, 2019

Member

This message is misleading: it’s ImageMagick that fails to decode the image, not MiniMagick.

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Apr 24, 2019

Member

Please do move the LoadError rescue down.

@@ -29,4 +29,13 @@ class ActiveStorage::Analyzer::ImageAnalyzerTest < ActiveSupport::TestCase
assert_equal 792, metadata[:width]
assert_equal 584, metadata[:height]
end

test "analyzing an unsupported image type returns nil width and height metadata" do
blob = create_file_blob(filename: "bad_file.bad", content_type: "image/bad_type")

This comment has been minimized.

Copy link
@itsWill

itsWill Apr 23, 2019

Author Contributor

Create a new fixture here, but we could also mock and stub MiniMagick::Image object, though I like the fixtures.

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Apr 24, 2019

Member

Use create_blob(data: "", ...) to avoid the empty fixture.

Show resolved Hide resolved activestorage/lib/active_storage/analyzer/image_analyzer.rb
if image.valid?
yield image
else
logger.info "Skipping image analysis because mini_magick doesn't support the file"

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Apr 24, 2019

Member

This message is misleading: it’s ImageMagick that fails to decode the image, not MiniMagick.


test "analyzing an unsupported image type returns nil width and height metadata" do
blob = create_file_blob(filename: "bad_file.bad", content_type: "image/bad_type")

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Apr 24, 2019

Member

✂️ this blank line.

@@ -29,4 +29,13 @@ class ActiveStorage::Analyzer::ImageAnalyzerTest < ActiveSupport::TestCase
assert_equal 792, metadata[:width]
assert_equal 584, metadata[:height]
end

test "analyzing an unsupported image type returns nil width and height metadata" do

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Apr 24, 2019

Member

Name this test “analyzing an unsupported image type” to match the others in this file. The test itself is clear as to what outcome it expects.

@@ -29,4 +29,13 @@ class ActiveStorage::Analyzer::ImageAnalyzerTest < ActiveSupport::TestCase
assert_equal 792, metadata[:width]
assert_equal 584, metadata[:height]
end

test "analyzing an unsupported image type returns nil width and height metadata" do
blob = create_file_blob(filename: "bad_file.bad", content_type: "image/bad_type")

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Apr 24, 2019

Member

Use create_blob(data: "", ...) to avoid the empty fixture.

@georgeclaghorn
Copy link
Member

left a comment

Can you please add a CHANGELOG entry?

Don't fail ImageAnalyzer on unsupported types
Fix: #36065

The IamgeAnalyzer passes a image to ImageMagick without checking if the
image is supported by ImageMagick. This patch checks that image is
supported and if not logs an error and returns an empty hash instead of
raising an error. This is the same error handling we do when we
encounter a LoadError when mini_magick is not installed.

@itsWill itsWill force-pushed the itsWill:dont_fail_on_unsuported_images branch from 88fd624 to 6133dad Apr 24, 2019

@itsWill

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

👍 done and squashed!

@georgeclaghorn georgeclaghorn merged commit c7b03de into rails:master Apr 24, 2019

2 checks passed

buildkite/rails Build #60733 passed (10 minutes, 54 seconds)
Details
codeclimate All good!
Details
@ConfusedVorlon

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

That was quick. Thank you.

For anyone coming to this issue before this reaches production; I wrote up a quick description on how to work around it with a custom analyzer.

https://blog.hobbyistsoftware.com/2019/04/active-storage-adding-a-custom-analyzer-for-unsupported-image-type/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.