Extract variation value object

6 files changed, 84 insertions, 65 deletions

diff --git a/lib/active_storage/blob.rb b/lib/active_storage/blob.rb
index a9d9b8771c..6bd3941cd8 100644
--- a/lib/active_storage/blob.rb
+++ b/lib/active_storage/blob.rb
@@ -32,8 +32,9 @@ class ActiveStorage::Blob < ActiveRecord::Base
     end
   end
 
-  # We can't wait until the record is first saved to have a key for it
+
   def key
+    # We can't wait until the record is first saved to have a key for it
     self[:key] ||= self.class.generate_unique_secure_token
   end
 
@@ -41,10 +42,11 @@ class ActiveStorage::Blob < ActiveRecord::Base
     ActiveStorage::Filename.new(self[:filename])
   end
 
-  def variant(variation)
-    ActiveStorage::Variant.new(self, variation: variation)
+  def variant(transformations)
+    ActiveStorage::Variant.new(self, ActiveStorage::Variation.new(transformations))
   end
 
+
   def url(expires_in: 5.minutes, disposition: :inline)
     service.url key, expires_in: expires_in, disposition: disposition, filename: filename
   end
diff --git a/lib/active_storage/engine.rb b/lib/active_storage/engine.rb
index b04925a9fd..11227a4e04 100644
--- a/lib/active_storage/engine.rb
+++ b/lib/active_storage/engine.rb
@@ -28,7 +28,7 @@ module ActiveStorage
 
       config.after_initialize do |app|
         ActiveStorage::VerifiedKeyWithExpiration.verifier = \
-        ActiveStorage::Variant.verifier = \
+        ActiveStorage::Variation.verifier = \
           Rails.application.message_verifier('ActiveStorage')
       end
     end
diff --git a/lib/active_storage/variant.rb b/lib/active_storage/variant.rb
index 4145ee644d..8be51eba92 100644
--- a/lib/active_storage/variant.rb
+++ b/lib/active_storage/variant.rb
@@ -1,82 +1,40 @@
 require "active_storage/blob"
-require "active_support/core_ext/object/inclusion"
 require "mini_magick"
 
 class ActiveStorage::Variant
-  class_attribute :verifier
-
-  ALLOWED_TRANSFORMATIONS = %i(
-    resize rotate format flip fill monochrome orient quality roll scale sharpen shave shear size thumbnail
-    transparent transpose transverse trim background bordercolor compress crop
-  )
-
   attr_reader :blob, :variation
   delegate :service, to: :blob
 
-  def self.find_or_process_by!(blob_key:, encoded_variant_key:)
-    new(ActiveStorage::Blob.find_by!(key: blob_key), variation: verifier.verify(encoded_variant_key)).processed
-  end
-
-  def self.encode_key(variation)
-    verifier.generate(variation)
+  class << self
+    def find_or_process_by!(blob_key:, variation_key:)
+      new(ActiveStorage::Blob.find_by!(key: blob_key), variation: ActiveStorage::Variation.decode(variation_key)).processed
+    end
   end
 
-  def initialize(blob, variation:)
+  def initialize(blob, variation)
     @blob, @variation = blob, variation
   end
 
   def processed
-    process unless processed?
+    process unless service.exist?(key)
     self
   end
 
-  def url(expires_in: 5.minutes, disposition: :inline)
-    service.url blob_variant_key, expires_in: expires_in, disposition: disposition, filename: blob.filename
+  def key
+    "variants/#{blob.key}/#{variation.key}"
   end
 
-  def key
-    verifier.generate(variation)
+  def url(expires_in: 5.minutes, disposition: :inline)
+    service.url key, expires_in: expires_in, disposition: disposition, filename: blob.filename
   end
 
 
   private
-    def processed?
-      service.exist?(blob_variant_key)
-    end
-
     def process
-      upload_variant transform(download_blob)
-    end
-
-    def download_blob
-      service.download(blob.key)
-    end
-
-    def upload_variant(variant)
-      service.upload blob_variant_key, variant
-    end
-
-    def blob_variant_key
-      "variants/#{blob.key}/#{key}"
+      service.upload key, transform(service.download(blob.key))
     end
 
     def transform(io)
-      File.open \
-        MiniMagick::Image.read(io).tap { |transforming_image|
-          variation.each do |(method, argument)|
-            if method = allowed_transformational_method(method.to_sym)
-              if argument.blank? || argument == true
-                transforming_image.public_send(method)
-              else
-                # FIXME: Consider whitelisting allowed arguments as well?
-                transforming_image.public_send(method, argument)
-              end
-            end
-          end
-        }.path
-    end
-
-    def allowed_transformational_method(method)
-      method.presence_in(ALLOWED_TRANSFORMATIONS)
+      File.open MiniMagick::Image.read(io).tap { |image| variation.transform(image) }.path
     end
 end
diff --git a/lib/active_storage/variation.rb b/lib/active_storage/variation.rb
new file mode 100644
index 0000000000..abff288ac1
--- /dev/null
+++ b/lib/active_storage/variation.rb
@@ -0,0 +1,53 @@
+require "active_support/core_ext/object/inclusion"
+
+# A set of transformations that can be applied to a blob to create a variant.
+class ActiveStorage::Variation
+  class_attribute :verifier
+
+  ALLOWED_TRANSFORMATIONS = %i(
+    resize rotate format flip fill monochrome orient quality roll scale sharpen shave shear size thumbnail
+    transparent transpose transverse trim background bordercolor compress crop
+  )
+
+  attr_reader :transformations
+
+  class << self
+    def decode(key)
+      new verifier.verify(key)
+    end
+    
+    def encode(transformations)
+      verifier.generate(transformations)
+    end
+  end
+
+  def initialize(transformations)
+    @transformations = transformations
+  end
+
+  def transform(image)
+    transformations.each do |(method, argument)|
+      next unless eligible_transformation?(method)
+
+      if eligible_argument?(argument)
+        image.public_send(method, argument)
+      else
+        image.public_send(method)
+      end
+    end
+  end
+
+  def key
+    self.class.encode(transformations)
+  end
+
+  private
+    def eligible_transformation?(method)
+      method.to_sym.in?(ALLOWED_TRANSFORMATIONS)
+    end
+
+    # FIXME: Consider whitelisting allowed arguments as well?
+    def eligible_argument?(argument)
+      argument.present? && argument != true
+    end
+end
diff --git a/test/test_helper.rb b/test/test_helper.rb
index af379ad35a..e98b6e0afc 100644
--- a/test/test_helper.rb
+++ b/test/test_helper.rb
@@ -15,7 +15,6 @@ rescue Errno::ENOENT
   {}
 end
 
-
 require "active_storage/service/disk_service"
 require "tmpdir"
 ActiveStorage::Blob.service = ActiveStorage::Service::DiskService.new(root: Dir.mktmpdir("active_storage_tests"))
@@ -24,8 +23,8 @@ ActiveStorage::Service.logger = ActiveSupport::Logger.new(STDOUT)
 require "active_storage/verified_key_with_expiration"
 ActiveStorage::VerifiedKeyWithExpiration.verifier = ActiveSupport::MessageVerifier.new("Testing")
 
-require "active_storage/variant"
-ActiveStorage::Variant.verifier = ActiveSupport::MessageVerifier.new("Testing")
+require "active_storage/variation"
+ActiveStorage::Variation.verifier = ActiveSupport::MessageVerifier.new("Testing")
 
 class ActiveSupport::TestCase
   private
diff --git a/test/variation_test.rb b/test/variation_test.rb
index 8e569f908c..d138682005 100644
--- a/test/variation_test.rb
+++ b/test/variation_test.rb
@@ -3,10 +3,17 @@ require "database/setup"
 require "active_storage/variant"
 
 class ActiveStorage::VariationTest < ActiveSupport::TestCase
-  test "square variation" do
-    blob = ActiveStorage::Blob.create_after_upload! \
-      io: File.open(File.expand_path("../fixtures/files/racecar.jpg", __FILE__)), filename: "racecar.jpg", content_type: "image/jpeg"
+  setup do
+    @blob = ActiveStorage::Blob.create_after_upload! \
+        filename: "racecar.jpg", content_type: "image/jpeg",
+        io: File.open(File.expand_path("../fixtures/files/racecar.jpg", __FILE__))
+  end
+
+  test "resized variation" do
+    assert_match /racecar.jpg/, @blob.variant(resize: "100x100").processed.url
+  end
 
-    assert_match /racecar.jpg/, blob.variant(resize: "100x100").processed.url
+  test "resized and monochrome variation" do
+    assert_match /racecar.jpg/, @blob.variant(resize: "100x100", monochrome: true).processed.url
   end
 end