diff --git a/Gemfile.lock b/Gemfile.lock index 779eca7bb0..a5a3320f53 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -69,6 +69,7 @@ PATH activerecord (= 6.1.0.alpha) activesupport (= 6.1.0.alpha) marcel (~> 0.3.1) + mimemagic (~> 0.3.2) activesupport (6.1.0.alpha) concurrent-ruby (~> 1.0, >= 1.0.2) i18n (>= 1.6, < 2) diff --git a/activestorage/activestorage.gemspec b/activestorage/activestorage.gemspec index fdf5c2f33d..f15f9bfccf 100644 --- a/activestorage/activestorage.gemspec +++ b/activestorage/activestorage.gemspec @@ -36,5 +36,6 @@ Gem::Specification.new do |s| s.add_dependency "activejob", version s.add_dependency "activerecord", version - s.add_dependency "marcel", "~> 0.3.1" + s.add_dependency "marcel", "~> 0.3.1" + s.add_dependency "mimemagic", "~> 0.3.2" end diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 1415e586bb..8ba9d7a9da 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -324,6 +324,10 @@ class ActiveStorage::Blob < ActiveRecord::Base ActiveStorage.content_types_allowed_inline.include?(content_type) end + def web_image? + ActiveStorage.web_image_content_types.include?(content_type) + end + def service_metadata if forcibly_serve_as_binary? { content_type: ActiveStorage.binary_content_type, disposition: :attachment, filename: filename } diff --git a/activestorage/app/models/active_storage/blob/representable.rb b/activestorage/app/models/active_storage/blob/representable.rb index c8fb9a2948..9029ea8611 100644 --- a/activestorage/app/models/active_storage/blob/representable.rb +++ b/activestorage/app/models/active_storage/blob/representable.rb @@ -30,7 +30,7 @@ module ActiveStorage::Blob::Representable # variable, call ActiveStorage::Blob#variable?. def variant(transformations) if variable? - variant_class.new(self, transformations) + variant_class.new(self, ActiveStorage::Variation.wrap(transformations).default_to(default_variant_transformations)) else raise ActiveStorage::InvariableError end @@ -95,6 +95,26 @@ module ActiveStorage::Blob::Representable end private + def default_variant_transformations + { format: default_variant_format } + end + + def default_variant_format + if web_image? + format || :png + else + :png + end + end + + def format + if filename.extension.present? && MimeMagic.by_extension(filename.extension)&.to_s == content_type + filename.extension + else + MimeMagic.new(content_type).extensions.first + end + end + def variant_class ActiveStorage.track_variants ? ActiveStorage::VariantWithRecord : ActiveStorage::Variant end diff --git a/activestorage/app/models/active_storage/variant.rb b/activestorage/app/models/active_storage/variant.rb index f38925fdde..5a63d145b9 100644 --- a/activestorage/app/models/active_storage/variant.rb +++ b/activestorage/app/models/active_storage/variant.rb @@ -55,7 +55,7 @@ require "ostruct" class ActiveStorage::Variant attr_reader :blob, :variation delegate :service, to: :blob - delegate :filename, :content_type, to: :specification + delegate :content_type, to: :variation def initialize(blob, variation_or_variation_key) @blob, @variation = blob, ActiveStorage::Variation.wrap(variation_or_variation_key) @@ -90,6 +90,10 @@ class ActiveStorage::Variant service.download key, &block end + def filename + ActiveStorage::Filename.new "#{blob.filename.base}.#{variation.format}" + end + alias_method :content_type_for_serving, :content_type def forced_disposition_for_serving #:nodoc: @@ -108,29 +112,9 @@ class ActiveStorage::Variant def process blob.open do |input| - variation.transform(input, format: format) do |output| + variation.transform(input) do |output| service.upload(key, output, content_type: content_type) end end end - - - def specification - @specification ||= - if ActiveStorage.web_image_content_types.include?(blob.content_type) - Specification.new \ - filename: blob.filename, - content_type: blob.content_type, - format: nil - else - Specification.new \ - filename: ActiveStorage::Filename.new("#{blob.filename.base}.png"), - content_type: "image/png", - format: "png" - end - end - - delegate :format, to: :specification - - class Specification < OpenStruct; end end diff --git a/activestorage/app/models/active_storage/variant_with_record.rb b/activestorage/app/models/active_storage/variant_with_record.rb index ad0f0cd7e3..a10e0578e8 100644 --- a/activestorage/app/models/active_storage/variant_with_record.rb +++ b/activestorage/app/models/active_storage/variant_with_record.rb @@ -32,14 +32,9 @@ class ActiveStorage::VariantWithRecord private def transform_blob blob.open do |input| - if blob.content_type.in?(ActiveStorage.web_image_content_types) - variation.transform(input) do |output| - yield io: output, filename: blob.filename, content_type: blob.content_type, service_name: blob.service.name - end - else - variation.transform(input, format: "png") do |output| - yield io: output, filename: "#{blob.filename.base}.png", content_type: "image/png", service_name: blob.service.name - end + variation.transform(input) do |output| + yield io: output, filename: "#{blob.filename.base}.#{variation.format}", + content_type: variation.content_type, service_name: blob.service.name end end end diff --git a/activestorage/app/models/active_storage/variation.rb b/activestorage/app/models/active_storage/variation.rb index 8e10c63a4f..9054e37f79 100644 --- a/activestorage/app/models/active_storage/variation.rb +++ b/activestorage/app/models/active_storage/variation.rb @@ -43,16 +43,30 @@ class ActiveStorage::Variation @transformations = transformations.deep_symbolize_keys end + def default_to(defaults) + self.class.new transformations.reverse_merge(defaults) + end + # Accepts a File object, performs the +transformations+ against it, and - # saves the transformed image into a temporary file. If +format+ is specified - # it will be the format of the result image, otherwise the result image - # retains the source format. - def transform(file, format: nil, &block) + # saves the transformed image into a temporary file. + def transform(file, &block) ActiveSupport::Notifications.instrument("transform.active_storage") do transformer.transform(file, format: format, &block) end end + def format + transformations.fetch(:format, :png).tap do |format| + if MimeMagic.by_extension(format).nil? + raise ArgumentError, "Invalid variant format (#{format.inspect})" + end + end + end + + def content_type + MimeMagic.by_extension(format).to_s + end + # Returns a signed key for all the +transformations+ that this variation was instantiated with. def key self.class.encode(transformations) @@ -64,6 +78,10 @@ class ActiveStorage::Variation private def transformer + transformer_class.new(transformations.except(:format)) + end + + def transformer_class if ActiveStorage.variant_processor begin require "image_processing" @@ -73,12 +91,12 @@ class ActiveStorage::Variation Please add `gem 'image_processing', '~> 1.2'` to your Gemfile. WARNING - ActiveStorage::Transformers::MiniMagickTransformer.new(transformations) + ActiveStorage::Transformers::MiniMagickTransformer else - ActiveStorage::Transformers::ImageProcessingTransformer.new(transformations) + ActiveStorage::Transformers::ImageProcessingTransformer end else - ActiveStorage::Transformers::MiniMagickTransformer.new(transformations) + ActiveStorage::Transformers::MiniMagickTransformer end end end diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb index c4779fdf8d..8ee373608e 100644 --- a/activestorage/test/models/variant_test.rb +++ b/activestorage/test/models/variant_test.rb @@ -180,6 +180,14 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase end end + test "PNG variation of JPEG blob" do + blob = create_file_blob(filename: "racecar.jpg") + variant = blob.variant(format: :png).processed + assert_equal "racecar.png", variant.filename.to_s + assert_equal "image/png", variant.content_type + assert_equal "PNG", read_image(variant).type + end + test "variation of invariable blob" do assert_raises ActiveStorage::InvariableError do create_file_blob(filename: "report.pdf", content_type: "application/pdf").variant(resize: "100x100") @@ -192,18 +200,29 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase assert_operator variant.url.length, :<, 785 end - test "works for vips processor" do - ActiveStorage.variant_processor = :vips - blob = create_file_blob(filename: "racecar.jpg") - variant = blob.variant(thumbnail_image: 100).processed + test "thumbnail variation of JPEG blob processed with VIPS" do + process_variants_with :vips do + blob = create_file_blob(filename: "racecar.jpg") + variant = blob.variant(thumbnail_image: 100).processed - image = read_image(variant) - assert_equal 100, image.width - assert_equal 67, image.height - rescue LoadError - # libvips not installed - ensure - ActiveStorage.variant_processor = :mini_magick + image = read_image(variant) + assert_equal 100, image.width + assert_equal 67, image.height + end + end + + test "thumbnail variation of extensionless GIF blob processed with VIPS" do + process_variants_with :vips do + blob = ActiveStorage::Blob.create_and_upload!(io: file_fixture("image.gif").open, filename: "image", content_type: "image/gif") + variant = blob.variant(resize_to_fit: [100, 100]).processed + + image = read_image(variant) + assert_equal "image.gif", variant.filename.to_s + assert_equal "image/gif", variant.content_type + assert_equal "GIF", image.type + assert_equal 100, image.width + assert_equal 100, image.height + end end test "passes content_type on upload" do @@ -217,4 +236,14 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase blob.variant(resize: "100x100").processed end end + + private + def process_variants_with(processor) + previous_processor, ActiveStorage.variant_processor = ActiveStorage.variant_processor, processor + yield + rescue LoadError + skip "Variant processor #{processor.inspect} is not installed" + ensure + ActiveStorage.variant_processor = previous_processor + end end