Set default variant format from blob content type

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)

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

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 }

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

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

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

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

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