Skip to content

Commit

Permalink
Load configured Active Storage plugins during boot
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
skipkayhil authored and zzak committed Jan 7, 2025
1 parent 1edb808 commit 7c8e351
Show file tree
Hide file tree
Showing 12 changed files with 167 additions and 103 deletions.
2 changes: 1 addition & 1 deletion activestorage/app/models/active_storage/variation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 4 additions & 0 deletions activestorage/lib/active_storage.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: []
Expand Down Expand Up @@ -379,5 +381,7 @@ module Transformers

autoload :Transformer
autoload :ImageProcessingTransformer
autoload :Vips
autoload :ImageMagick
end
end
5 changes: 5 additions & 0 deletions activestorage/lib/active_storage/analyzer/image_analyzer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
21 changes: 10 additions & 11 deletions activestorage/lib/active_storage/analyzer/image_analyzer/vips.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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/
Expand Down
42 changes: 37 additions & 5 deletions activestorage/lib/active_storage/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
72 changes: 72 additions & 0 deletions activestorage/lib/active_storage/transformers/image_magick.rb
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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 ]
Expand All @@ -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
Expand Down
11 changes: 11 additions & 0 deletions activestorage/lib/active_storage/transformers/vips.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

module ActiveStorage
module Transformers
class Vips < ImageProcessingTransformer
def processor
ImageProcessing::Vips
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -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
5 changes: 2 additions & 3 deletions activestorage/test/analyzer/image_analyzer/vips_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading

0 comments on commit 7c8e351

Please sign in to comment.