Mini_magick is required inside `download_blob_to_tempfile` block but used outside

joelcogen · June 20, 2022, 1:32pm

In ActiveStorage::Analyzer::ImageAnalyzer::ImageMagick#read_image

rails/rails/blob/main/activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb#L14


      
          # This analyzer relies on the third-party {MiniMagick}[https://github.com/minimagick/minimagick] gem. MiniMagick requires
          # the {ImageMagick}[http://www.imagemagick.org] system library.
          class Analyzer::ImageAnalyzer::ImageMagick < Analyzer::ImageAnalyzer
            def self.accept?(blob)
              super && ActiveStorage.variant_processor == :mini_magick
            end
          
          
  private
              def read_image
                begin
                  require "mini_magick"
                rescue LoadError
                  logger.info "Skipping image analysis because the mini_magick gem isn't installed"
                  return {}
                end
          
          
      download_blob_to_tempfile do |file|
                  image = instrument("mini_magick") do
                    MiniMagick::Image.new(file.path)
                  end

require "mini_magick" is called inside the block passed to download_blob_to_tempfile. However, it is used outside that block at L30:

rescue MiniMagick::Error => error

That means if download_blob_to_tempfile raises an error and mini_magick hasn’t been previously required, L30 raises a NameError instead of skipping the rescue block and propagating the error.

The fix seems really simple: inverse L13 and L14 to require mini_magick at the level it is used.

sdubois · June 20, 2022, 5:18pm

Did you observe the NameError you described? By reading the flow of this code it seems to me that the rescue clause can only get triggered after the L14 require is executed, because Minimagick errors are apparently raised only starting at L17.

joelcogen · June 20, 2022, 6:35pm

Yes, it occurred in my code.

The issue is not errors raised by mini_magick, but when an error is trigger by download_blob_to_tempfile before yielding the block.

In my case, the file couldn’t be found on the remote storage. Therefore the block is never called, mini_magick is never required, and the rescue fails because the exception MiniMagick::Error hasn’t been defined.

sdubois · June 20, 2022, 8:08pm

Ok understood, thanks for the explanation, it sounds worth submitting a PR

joelcogen · June 20, 2022, 9:00pm

The thing is I tried to reproduce inside the corresponding test but require is called separately, and even if I bypass the call, it’s already required by previous tests. So I can submit a PR but I really don’t see how to attach a test or even a script to reproduce the issue.

sdubois · June 21, 2022, 6:22am

Why is require called separately in the test, if the code being tested already takes care of it?

Maybe you could undefine Minimagick before the test begins.

joelcogen · June 21, 2022, 12:59pm

github.com

rails/rails/blob/main/activestorage/test/analyzer/image_analyzer/image_magick_test.rb#L68


      
              end
            end
          
          
  private
              def analyze_with_image_magick
                previous_processor, ActiveStorage.variant_processor = ActiveStorage.variant_processor, :mini_magick
                require "mini_magick"
          
          
      yield
              rescue LoadError
                ENV["CI"] ? raise : skip("Variant processor image_magick is not installed")
              ensure
                ActiveStorage.variant_processor = previous_processor
              end
          end

It seems like the test is designed so the test suite will pass on a machine that doesn’t have mini_magick and depends on the require to assert it.

The other issue is that mini_magick may be required by a previous test.

That being said, the potential issue and change seem pretty self-explanatory, so maybe we’re fine as long as the existing tests still pass?

sdubois · June 21, 2022, 1:02pm

That being said, the potential issue and change seem pretty self-explanatory, so maybe we’re fine as long as the existing tests still pass?

I would assume so.

joelcogen · June 21, 2022, 1:37pm

github.com/rails/rails

require mini_magick at the level it is being used

rails:main ← joelcogen:fix-require-mini_magick

opened 01:36PM - 21 Jun 22 UTC

joelcogen

+9 -7

### Summary [Discussion on rubyonrails-core](https://discuss.rubyonrails.org/…t/mini-magick-is-required-inside-download-blob-to-tempfile-block-but-used-outside/80867) `require "mini_magick"` is called inside the block passed to `download_blob_to_tempfile`, but used outside that block at L30: ``` rescue MiniMagick::Error => error ``` `MiniMagick::Error` needs to be defined outside the block, or a `NameError` will occur if `download_blob_to_tempfile` fails and the block is never yielded, so mini_magick is never required. I have witnessed the error on a live app, because the file was removed from S3 causing `download_blob_to_tempfile` to fail and never yield the block. Because mini_magick hadn't been required before, `MiniMagick::Error` is not defined, and a `NameError` is raised instead of the current error being propagated.

Topic		Replies	Views
uninitialized constant MiniMagic (NameError) rubyonrails-talk	2	240	August 13, 2007
MiniMagick... Core Dump when Reszing Images? rubyonrails-talk	3	178	May 30, 2007
attachment_fu + mini_magick troubleshooting rubyonrails-talk	0	96	January 5, 2008
Uninitialized constant Files::Magick -still around rubyonrails-talk	19	457	March 17, 2011
Image Magic rubyonrails-talk	3	106	June 15, 2010

Mini_magick is required inside `download_blob_to_tempfile` block but used outside

Related topics

More Resources