From 7c8e351d01f1ad877c0200599567ba736e13a910 Mon Sep 17 00:00:00 2001 From: Hartley McGuire Date: Sat, 14 May 2022 18:36:48 -0400 Subject: [PATCH] Load configured Active Storage plugins during boot Previously, the parts of Active Storage using ruby-vips, mini_magick, or image_processing would try to load gems only when used. This strategy works well because it allows apps that don't need these features to easily ignore them and not have to depend on gems they don't need. However, the downsides to this strategy are that it requires loading code during requests and that it moves potential error messages into request logs instead of those errors being immediately visible on boot. This commit addresses these issues by restructing how the Vips and Image Magick transformers/analyzers are organized so that they will be loaded (if configured) on boot along with whatever dependencies they need. For now, if :vips or :mini_magick is specified as the Active Storage :variant_processor, not having the required gem in the Gemfile will print a deprecation warning instead of erroring because it is likely many apps are currently configured this way. --- .../app/models/active_storage/variation.rb | 2 +- activestorage/lib/active_storage.rb | 4 ++ .../active_storage/analyzer/image_analyzer.rb | 5 ++ .../analyzer/image_analyzer/image_magick.rb | 17 ++--- .../analyzer/image_analyzer/vips.rb | 21 +++--- activestorage/lib/active_storage/engine.rb | 42 +++++++++-- .../transformers/image_magick.rb | 72 +++++++++++++++++++ .../image_processing_transformer.rb | 72 ++----------------- .../lib/active_storage/transformers/vips.rb | 11 +++ .../image_analyzer/image_magick_test.rb | 5 +- .../test/analyzer/image_analyzer/vips_test.rb | 5 +- activestorage/test/models/variant_test.rb | 14 +++- 12 files changed, 167 insertions(+), 103 deletions(-) create mode 100644 activestorage/lib/active_storage/transformers/image_magick.rb create mode 100644 activestorage/lib/active_storage/transformers/vips.rb diff --git a/activestorage/app/models/active_storage/variation.rb b/activestorage/app/models/active_storage/variation.rb index d1e0c17a5473f..24769b4de03e8 100644 --- a/activestorage/app/models/active_storage/variation.rb +++ b/activestorage/app/models/active_storage/variation.rb @@ -82,6 +82,6 @@ def digest private def transformer - ActiveStorage::Transformers::ImageProcessingTransformer.new(transformations.except(:format)) + ActiveStorage.variant_transformer.new(transformations.except(:format)) end end diff --git a/activestorage/lib/active_storage.rb b/activestorage/lib/active_storage.rb index 0d0b913d02242..fcda880c218bc 100644 --- a/activestorage/lib/active_storage.rb +++ b/activestorage/lib/active_storage.rb @@ -49,6 +49,8 @@ module ActiveStorage mattr_accessor :verifier mattr_accessor :variant_processor, default: :mini_magick + mattr_accessor :variant_transformer + mattr_accessor :queues, default: {} mattr_accessor :previewers, default: [] @@ -379,5 +381,7 @@ module Transformers autoload :Transformer autoload :ImageProcessingTransformer + autoload :Vips + autoload :ImageMagick end end diff --git a/activestorage/lib/active_storage/analyzer/image_analyzer.rb b/activestorage/lib/active_storage/analyzer/image_analyzer.rb index efa0d6f668b4f..2fa0f01ae74f4 100644 --- a/activestorage/lib/active_storage/analyzer/image_analyzer.rb +++ b/activestorage/lib/active_storage/analyzer/image_analyzer.rb @@ -12,6 +12,11 @@ module ActiveStorage # ActiveStorage::Analyzer::ImageAnalyzer::ImageMagick.new(blob).metadata # # => { width: 4104, height: 2736 } class Analyzer::ImageAnalyzer < Analyzer + extend ActiveSupport::Autoload + + autoload :Vips + autoload :ImageMagick + def self.accept?(blob) blob.image? end diff --git a/activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb b/activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb index 3cc57e872f0fb..2d5d43b20e759 100644 --- a/activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb +++ b/activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb @@ -1,22 +1,17 @@ # frozen_string_literal: true +begin + gem "mini_magick" +rescue LoadError => error + raise error unless error.message.match?(/ruby-vips/) +end + module ActiveStorage # 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) diff --git a/activestorage/lib/active_storage/analyzer/image_analyzer/vips.rb b/activestorage/lib/active_storage/analyzer/image_analyzer/vips.rb index 7e682b3b75fda..dcb27e953f53c 100644 --- a/activestorage/lib/active_storage/analyzer/image_analyzer/vips.rb +++ b/activestorage/lib/active_storage/analyzer/image_analyzer/vips.rb @@ -1,22 +1,18 @@ # frozen_string_literal: true +begin + gem "ruby-vips" + require "ruby-vips" +rescue LoadError => error + raise error unless error.message.match?(/ruby-vips/) +end + module ActiveStorage # This analyzer relies on the third-party {ruby-vips}[https://github.com/libvips/ruby-vips] gem. Ruby-vips requires # the {libvips}[https://libvips.github.io/libvips/] system library. class Analyzer::ImageAnalyzer::Vips < Analyzer::ImageAnalyzer - def self.accept?(blob) - super && ActiveStorage.variant_processor == :vips - end - private def read_image - begin - require "ruby-vips" - rescue LoadError - logger.info "Skipping image analysis because the ruby-vips gem isn't installed" - return {} - end - download_blob_to_tempfile do |file| image = instrument("vips") do # ruby-vips will raise Vips::Error if it can't find an appropriate loader for the file @@ -35,6 +31,9 @@ def read_image logger.error "Skipping image analysis due to a Vips error: #{error.message}" {} end + rescue ::Vips::Error => error + logger.error "Skipping image analysis due to an Vips error: #{error.message}" + {} end ROTATIONS = /Right-top|Left-bottom|Top-right|Bottom-left/ diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb index 645a1ea6f83a7..01b8c48ac5d65 100644 --- a/activestorage/lib/active_storage/engine.rb +++ b/activestorage/lib/active_storage/engine.rb @@ -12,8 +12,6 @@ require "active_storage/previewer/video_previewer" require "active_storage/analyzer/image_analyzer" -require "active_storage/analyzer/image_analyzer/image_magick" -require "active_storage/analyzer/image_analyzer/vips" require "active_storage/analyzer/video_analyzer" require "active_storage/analyzer/audio_analyzer" @@ -27,7 +25,7 @@ class Engine < Rails::Engine # :nodoc: config.active_storage = ActiveSupport::OrderedOptions.new config.active_storage.previewers = [ ActiveStorage::Previewer::PopplerPDFPreviewer, ActiveStorage::Previewer::MuPDFPreviewer, ActiveStorage::Previewer::VideoPreviewer ] - config.active_storage.analyzers = [ ActiveStorage::Analyzer::ImageAnalyzer::Vips, ActiveStorage::Analyzer::ImageAnalyzer::ImageMagick, ActiveStorage::Analyzer::VideoAnalyzer, ActiveStorage::Analyzer::AudioAnalyzer ] + config.active_storage.analyzers = [ ActiveStorage::Analyzer::VideoAnalyzer, ActiveStorage::Analyzer::AudioAnalyzer ] config.active_storage.paths = ActiveSupport::OrderedOptions.new config.active_storage.queues = ActiveSupport::InheritableOptions.new config.active_storage.precompile_assets = true @@ -86,9 +84,43 @@ class Engine < Rails::Engine # :nodoc: initializer "active_storage.configs" do config.after_initialize do |app| ActiveStorage.logger = app.config.active_storage.logger || Rails.logger - ActiveStorage.variant_processor = app.config.active_storage.variant_processor || :mini_magick + ActiveStorage.variant_processor = app.config.active_storage.variant_processor ActiveStorage.previewers = app.config.active_storage.previewers || [] - ActiveStorage.analyzers = app.config.active_storage.analyzers || [] + + begin + analyzer, transformer = + case ActiveStorage.variant_processor + when :vips + [ + ActiveStorage::Analyzer::ImageAnalyzer::Vips, + ActiveStorage::Transformers::Vips + ] + when :mini_magick + [ + ActiveStorage::Analyzer::ImageAnalyzer::ImageMagick, + ActiveStorage::Transformers::ImageMagick + ] + end + + ActiveStorage.analyzers = [analyzer].concat(app.config.active_storage.analyzers || []) + ActiveStorage.variant_transformer = transformer + rescue LoadError => error + case error.message + when /libvips/ + ActiveStorage.logger.warn <<~WARNING.squish + Using vips to process variants requires the libvips library. + Please install libvips using the instructions on the libvips website. + WARNING + when /image_processing/ + ActiveStorage.logger.warn <<~WARNING.squish + Generating image variants require the image_processing gem. + Please add `gem 'image_processing', '~> 1.2'` to your Gemfile. + WARNING + else + raise + end + end + ActiveStorage.paths = app.config.active_storage.paths || {} ActiveStorage.routes_prefix = app.config.active_storage.routes_prefix || "/rails/active_storage" ActiveStorage.draw_routes = app.config.active_storage.draw_routes != false diff --git a/activestorage/lib/active_storage/transformers/image_magick.rb b/activestorage/lib/active_storage/transformers/image_magick.rb new file mode 100644 index 0000000000000..5080048583586 --- /dev/null +++ b/activestorage/lib/active_storage/transformers/image_magick.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +module ActiveStorage + module Transformers + class ImageMagick < ImageProcessingTransformer + private + def processor + ImageProcessing::MiniMagick + end + + def validate_transformation(name, argument) + method_name = name.to_s.tr("-", "_") + + unless ActiveStorage.supported_image_processing_methods.include?(method_name) + raise UnsupportedImageProcessingMethod, <<~ERROR.squish + The provided transformation method is not supported: #{method_name}. + ERROR + end + + if argument.present? + if argument.is_a?(String) || argument.is_a?(Symbol) + validate_arg_string(argument) + elsif argument.is_a?(Array) + validate_arg_array(argument) + elsif argument.is_a?(Hash) + validate_arg_hash(argument) + end + end + + super + end + + def validate_arg_string(argument) + unsupported_arguments = ActiveStorage.unsupported_image_processing_arguments.any? do |bad_arg| + argument.to_s.downcase.include?(bad_arg) + end + + raise UnsupportedImageProcessingArgument if unsupported_arguments + end + + def validate_arg_array(argument) + argument.each do |arg| + if arg.is_a?(Integer) || arg.is_a?(Float) + next + elsif arg.is_a?(String) || arg.is_a?(Symbol) + validate_arg_string(arg) + elsif arg.is_a?(Array) + validate_arg_array(arg) + elsif arg.is_a?(Hash) + validate_arg_hash(arg) + end + end + end + + def validate_arg_hash(argument) + argument.each do |key, value| + validate_arg_string(key) + + if value.is_a?(Integer) || value.is_a?(Float) + next + elsif value.is_a?(String) || value.is_a?(Symbol) + validate_arg_string(value) + elsif value.is_a?(Array) + validate_arg_array(value) + elsif value.is_a?(Hash) + validate_arg_hash(value) + end + end + end + end + end +end diff --git a/activestorage/lib/active_storage/transformers/image_processing_transformer.rb b/activestorage/lib/active_storage/transformers/image_processing_transformer.rb index fe33cc638c2e8..958d0d76f1db7 100644 --- a/activestorage/lib/active_storage/transformers/image_processing_transformer.rb +++ b/activestorage/lib/active_storage/transformers/image_processing_transformer.rb @@ -25,22 +25,9 @@ def process(file, format:) call end - def processor - ImageProcessing.const_get(ActiveStorage.variant_processor.to_s.camelize) - end - def operations transformations.each_with_object([]) do |(name, argument), list| - if ActiveStorage.variant_processor == :mini_magick - validate_transformation(name, argument) - end - - if name.to_s == "combine_options" - raise ArgumentError, <<~ERROR.squish - Active Storage's ImageProcessing transformer doesn't support :combine_options, - as it always generates a single command. - ERROR - end + validate_transformation(name, argument) if argument.present? list << [ name, argument ] @@ -49,61 +36,12 @@ def operations end def validate_transformation(name, argument) - method_name = name.to_s.tr("-", "_") - - unless ActiveStorage.supported_image_processing_methods.any? { |method| method_name == method } - raise UnsupportedImageProcessingMethod, <<~ERROR.squish - One or more of the provided transformation methods is not supported. + if name.to_s == "combine_options" + raise ArgumentError, <<~ERROR.squish + Active Storage's ImageProcessing transformer doesn't support :combine_options, + as it always generates a single command. ERROR end - - if argument.present? - if argument.is_a?(String) || argument.is_a?(Symbol) - validate_arg_string(argument) - elsif argument.is_a?(Array) - validate_arg_array(argument) - elsif argument.is_a?(Hash) - validate_arg_hash(argument) - end - end - end - - def validate_arg_string(argument) - unsupported_arguments = ActiveStorage.unsupported_image_processing_arguments.any? do |bad_arg| - argument.to_s.downcase.include?(bad_arg) - end - - raise UnsupportedImageProcessingArgument if unsupported_arguments - end - - def validate_arg_array(argument) - argument.each do |arg| - if arg.is_a?(Integer) || arg.is_a?(Float) - next - elsif arg.is_a?(String) || arg.is_a?(Symbol) - validate_arg_string(arg) - elsif arg.is_a?(Array) - validate_arg_array(arg) - elsif arg.is_a?(Hash) - validate_arg_hash(arg) - end - end - end - - def validate_arg_hash(argument) - argument.each do |key, value| - validate_arg_string(key) - - if value.is_a?(Integer) || value.is_a?(Float) - next - elsif value.is_a?(String) || value.is_a?(Symbol) - validate_arg_string(value) - elsif value.is_a?(Array) - validate_arg_array(value) - elsif value.is_a?(Hash) - validate_arg_hash(value) - end - end end end end diff --git a/activestorage/lib/active_storage/transformers/vips.rb b/activestorage/lib/active_storage/transformers/vips.rb new file mode 100644 index 0000000000000..df1efc318bfd5 --- /dev/null +++ b/activestorage/lib/active_storage/transformers/vips.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module ActiveStorage + module Transformers + class Vips < ImageProcessingTransformer + def processor + ImageProcessing::Vips + end + end + end +end diff --git a/activestorage/test/analyzer/image_analyzer/image_magick_test.rb b/activestorage/test/analyzer/image_analyzer/image_magick_test.rb index 26c0451b233a4..5b78a8e7a375f 100644 --- a/activestorage/test/analyzer/image_analyzer/image_magick_test.rb +++ b/activestorage/test/analyzer/image_analyzer/image_magick_test.rb @@ -60,13 +60,12 @@ class ActiveStorage::Analyzer::ImageAnalyzer::ImageMagickTest < ActiveSupport::T private def analyze_with_image_magick - previous_processor, ActiveStorage.variant_processor = ActiveStorage.variant_processor, :mini_magick - require "mini_magick" + previous_analyzers, ActiveStorage.analyzers = ActiveStorage.analyzers, [ActiveStorage::Analyzer::ImageAnalyzer::ImageMagick] yield rescue LoadError ENV["BUILDKITE"] ? raise : skip("Variant processor image_magick is not installed") ensure - ActiveStorage.variant_processor = previous_processor + ActiveStorage.analyzers = previous_analyzers end end diff --git a/activestorage/test/analyzer/image_analyzer/vips_test.rb b/activestorage/test/analyzer/image_analyzer/vips_test.rb index 0062de4aedc24..6cb90fe55a98d 100644 --- a/activestorage/test/analyzer/image_analyzer/vips_test.rb +++ b/activestorage/test/analyzer/image_analyzer/vips_test.rb @@ -60,13 +60,12 @@ class ActiveStorage::Analyzer::ImageAnalyzer::VipsTest < ActiveSupport::TestCase private def analyze_with_vips - previous_processor, ActiveStorage.variant_processor = ActiveStorage.variant_processor, :vips - require "ruby-vips" + previous_analyzers, ActiveStorage.analyzers = ActiveStorage.analyzers, [ActiveStorage::Analyzer::ImageAnalyzer::Vips] yield rescue LoadError ENV["BUILDKITE"] ? raise : skip("Variant processor vips is not installed") ensure - ActiveStorage.variant_processor = previous_processor + ActiveStorage.analyzers = previous_analyzers end end diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb index fa9495d26695c..c7a402b1b125d 100644 --- a/activestorage/test/models/variant_test.rb +++ b/activestorage/test/models/variant_test.rb @@ -295,11 +295,21 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase private def process_variants_with(processor) - previous_processor, ActiveStorage.variant_processor = ActiveStorage.variant_processor, processor + previous_transformer = ActiveStorage.variant_transformer + ActiveStorage.variant_transformer = + case processor + when :vips + ActiveStorage::Transformers::Vips + when :mini_magick + ActiveStorage::Transformers::ImageMagick + else + raise "#{processor.inspect} is not a valid image transformer" + end + yield rescue LoadError ENV["BUILDKITE"] ? raise : skip("Variant processor #{processor.inspect} is not installed") ensure - ActiveStorage.variant_processor = previous_processor + ActiveStorage.variant_transformer = previous_transformer end end