Merge branch 'master' into feature/reselect-method

12 files changed, 94 insertions, 136 deletions

diff --git a/activestorage/app/assets/javascripts/activestorage.js b/activestorage/app/assets/javascripts/activestorage.js
index d3fd795a3a..375eb6b533 100644
--- a/activestorage/app/assets/javascripts/activestorage.js
+++ b/activestorage/app/assets/javascripts/activestorage.js
@@ -855,14 +855,22 @@
     return DirectUploadsController;
   }();
   var processingAttribute = "data-direct-uploads-processing";
+  var submitButtonsByForm = new WeakMap();
   var started = false;
   function start() {
     if (!started) {
       started = true;
+      document.addEventListener("click", didClick, true);
       document.addEventListener("submit", didSubmitForm);
       document.addEventListener("ajax:before", didSubmitRemoteElement);
     }
   }
+  function didClick(event) {
+    var target = event.target;
+    if (target.tagName == "INPUT" && target.type == "submit" && target.form) {
+      submitButtonsByForm.set(target.form, target);
+    }
+  }
   function didSubmitForm(event) {
     handleFormSubmissionEvent(event);
   }
@@ -894,7 +902,7 @@
     }
   }
   function submitForm(form) {
-    var button = findElement(form, "input[type=submit]");
+    var button = submitButtonsByForm.get(form) || findElement(form, "input[type=submit]");
     if (button) {
       var _button = button, disabled = _button.disabled;
       button.disabled = false;
@@ -909,6 +917,7 @@
       button.click();
       form.removeChild(button);
     }
+    submitButtonsByForm.delete(form);
   }
   function disable(input) {
     input.disabled = true;
diff --git a/activestorage/app/controllers/active_storage/base_controller.rb b/activestorage/app/controllers/active_storage/base_controller.rb
index 59312ac8df..b27d2bd8aa 100644
--- a/activestorage/app/controllers/active_storage/base_controller.rb
+++ b/activestorage/app/controllers/active_storage/base_controller.rb
@@ -1,10 +1,8 @@
 # frozen_string_literal: true
 
-# The base controller for all ActiveStorage controllers.
+# The base class for all Active Storage controllers.
 class ActiveStorage::BaseController < ActionController::Base
-  protect_from_forgery with: :exception
+  include ActiveStorage::SetCurrent
 
-  before_action do
-    ActiveStorage::Current.host = request.base_url
-  end
+  protect_from_forgery with: :exception
 end
diff --git a/activestorage/app/controllers/active_storage/disk_controller.rb b/activestorage/app/controllers/active_storage/disk_controller.rb
index 75cc11d6ff..7bd641ab9a 100644
--- a/activestorage/app/controllers/active_storage/disk_controller.rb
+++ b/activestorage/app/controllers/active_storage/disk_controller.rb
@@ -13,6 +13,8 @@ class ActiveStorage::DiskController < ActiveStorage::BaseController
     else
       head :not_found
     end
+  rescue Errno::ENOENT
+    head :not_found
   end
 
   def update
diff --git a/activestorage/app/controllers/concerns/active_storage/set_current.rb b/activestorage/app/controllers/concerns/active_storage/set_current.rb
new file mode 100644
index 0000000000..597afe7064
--- /dev/null
+++ b/activestorage/app/controllers/concerns/active_storage/set_current.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+# Sets the <tt>ActiveStorage::Current.host</tt> attribute, which the disk service uses to generate URLs.
+# Include this concern in custom controllers that call ActiveStorage::Blob#service_url,
+# ActiveStorage::Variant#service_url, or ActiveStorage::Preview#service_url so the disk service can
+# generate URLs using the same host, protocol, and base path as the current request.
+module ActiveStorage::SetCurrent
+  extend ActiveSupport::Concern
+
+  included do
+    before_action do
+      ActiveStorage::Current.host = request.base_url
+    end
+  end
+end
diff --git a/activestorage/app/javascript/activestorage/ujs.js b/activestorage/app/javascript/activestorage/ujs.js
index 08c535470d..f5353389ef 100644
--- a/activestorage/app/javascript/activestorage/ujs.js
+++ b/activestorage/app/javascript/activestorage/ujs.js
@@ -2,16 +2,25 @@ import { DirectUploadsController } from "./direct_uploads_controller"
 import { findElement } from "./helpers"
 
 const processingAttribute = "data-direct-uploads-processing"
+const submitButtonsByForm = new WeakMap
 let started = false
 
 export function start() {
   if (!started) {
     started = true
+    document.addEventListener("click", didClick, true)
     document.addEventListener("submit", didSubmitForm)
     document.addEventListener("ajax:before", didSubmitRemoteElement)
   }
 }
 
+function didClick(event) {
+  const { target } = event
+  if (target.tagName == "INPUT" && target.type == "submit" && target.form) {
+    submitButtonsByForm.set(target.form, target)
+  }
+}
+
 function didSubmitForm(event) {
   handleFormSubmissionEvent(event)
 }
@@ -49,7 +58,8 @@ function handleFormSubmissionEvent(event) {
 }
 
 function submitForm(form) {
-  let button = findElement(form, "input[type=submit]")
+  let button = submitButtonsByForm.get(form) || findElement(form, "input[type=submit]")
+
   if (button) {
     const { disabled } = button
     button.disabled = false
@@ -64,6 +74,7 @@ function submitForm(form) {
     button.click()
     form.removeChild(button)
   }
+  submitButtonsByForm.delete(form)
 }
 
 function disable(input) {
diff --git a/activestorage/app/jobs/active_storage/analyze_job.rb b/activestorage/app/jobs/active_storage/analyze_job.rb
index 2a952f9f74..804ee4557a 100644
--- a/activestorage/app/jobs/active_storage/analyze_job.rb
+++ b/activestorage/app/jobs/active_storage/analyze_job.rb
@@ -2,6 +2,8 @@
 
 # Provides asynchronous analysis of ActiveStorage::Blob records via ActiveStorage::Blob#analyze_later.
 class ActiveStorage::AnalyzeJob < ActiveStorage::BaseJob
+  retry_on ActiveStorage::IntegrityError, attempts: 10, wait: :exponentially_longer
+
   def perform(blob)
     blob.analyze
   end
diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb
index e7f2615b0f..53aa9f0237 100644
--- a/activestorage/app/models/active_storage/blob.rb
+++ b/activestorage/app/models/active_storage/blob.rb
@@ -35,6 +35,10 @@ class ActiveStorage::Blob < ActiveRecord::Base
 
   scope :unattached, -> { left_joins(:attachments).where(ActiveStorage::Attachment.table_name => { blob_id: nil }) }
 
+  before_destroy(prepend: true) do
+    raise ActiveRecord::InvalidForeignKey if attachments.exists?
+  end
+
   class << self
     # You can used the signed ID of a blob to refer to it on the client side without fear of tampering.
     # This is particularly helpful for direct uploads where the client-side needs to refer to the blob
diff --git a/activestorage/app/models/active_storage/blob/identifiable.rb b/activestorage/app/models/active_storage/blob/identifiable.rb
index 049e45dc3e..2c17ddc25f 100644
--- a/activestorage/app/models/active_storage/blob/identifiable.rb
+++ b/activestorage/app/models/active_storage/blob/identifiable.rb
@@ -15,6 +15,10 @@ module ActiveStorage::Blob::Identifiable
     end
 
     def download_identifiable_chunk
-      service.download_chunk key, 0...4.kilobytes
+      if byte_size.positive?
+        service.download_chunk key, 0...4.kilobytes
+      else
+        ""
+      end
     end
 end
diff --git a/activestorage/app/models/active_storage/filename.rb b/activestorage/app/models/active_storage/filename.rb
index bebb5e61b3..2a03e0173d 100644
--- a/activestorage/app/models/active_storage/filename.rb
+++ b/activestorage/app/models/active_storage/filename.rb
@@ -3,8 +3,6 @@
 # Encapsulates a string representing a filename to provide convenient access to parts of it and sanitization.
 # A Filename instance is returned by ActiveStorage::Blob#filename, and is comparable so it can be used for sorting.
 class ActiveStorage::Filename
-  require_dependency "active_storage/filename/parameters"
-
   include Comparable
 
   class << self
@@ -60,10 +58,6 @@ class ActiveStorage::Filename
     @filename.encode(Encoding::UTF_8, invalid: :replace, undef: :replace, replace: "�").strip.tr("\u{202E}%$|:;/\t\r\n\\", "-")
   end
 
-  def parameters #:nodoc:
-    Parameters.new self
-  end
-
   # Returns the sanitized version of the filename.
   def to_s
     sanitized.to_s
diff --git a/activestorage/app/models/active_storage/filename/parameters.rb b/activestorage/app/models/active_storage/filename/parameters.rb
deleted file mode 100644
index fb9ea10e49..0000000000
--- a/activestorage/app/models/active_storage/filename/parameters.rb
+++ /dev/null
@@ -1,36 +0,0 @@
-# frozen_string_literal: true
-
-class ActiveStorage::Filename::Parameters #:nodoc:
-  attr_reader :filename
-
-  def initialize(filename)
-    @filename = filename
-  end
-
-  def combined
-    "#{ascii}; #{utf8}"
-  end
-
-  TRADITIONAL_ESCAPED_CHAR = /[^ A-Za-z0-9!#$+.^_`|~-]/
-
-  def ascii
-    'filename="' + percent_escape(I18n.transliterate(filename.sanitized), TRADITIONAL_ESCAPED_CHAR) + '"'
-  end
-
-  RFC_5987_ESCAPED_CHAR = /[^A-Za-z0-9!#$&+.^_`|~-]/
-
-  def utf8
-    "filename*=UTF-8''" + percent_escape(filename.sanitized, RFC_5987_ESCAPED_CHAR)
-  end
-
-  def to_s
-    combined
-  end
-
-  private
-    def percent_escape(string, pattern)
-      string.gsub(pattern) do |char|
-        char.bytes.map { |byte| "%%%02X" % byte }.join
-      end
-    end
-end
diff --git a/activestorage/app/models/active_storage/variant.rb b/activestorage/app/models/active_storage/variant.rb
index 056fd2be99..ea57fa5f78 100644
--- a/activestorage/app/models/active_storage/variant.rb
+++ b/activestorage/app/models/active_storage/variant.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "ostruct"
+
 # Image blobs can have variants that are the result of a set of transformations applied to the original.
 # These variants are used to create thumbnails, fixed-size avatars, or any other derivative image from the
 # original.
@@ -51,7 +53,7 @@
 # * {ImageProcessing::Vips}[https://github.com/janko-m/image_processing/blob/master/doc/vips.md#methods]
 # * {ruby-vips reference}[http://www.rubydoc.info/gems/ruby-vips/Vips/Image]
 class ActiveStorage::Variant
-  WEB_IMAGE_CONTENT_TYPES = %w( image/png image/jpeg image/jpg image/gif )
+  WEB_IMAGE_CONTENT_TYPES = %w[ image/png image/jpeg image/jpg image/gif ]
 
   attr_reader :blob, :variation
   delegate :service, to: :blob
@@ -95,37 +97,35 @@ class ActiveStorage::Variant
 
     def process
       blob.open do |image|
-        transform image do |output|
-          upload output
-        end
+        transform(image) { |output| upload(output) }
       end
     end
 
-
-    def filename
-      if WEB_IMAGE_CONTENT_TYPES.include?(blob.content_type)
-        blob.filename
-      else
-        ActiveStorage::Filename.new("#{blob.filename.base}.png")
-      end
+    def transform(image, &block)
+      variation.transform(image, format: format, &block)
     end
 
-    def content_type
-      blob.content_type.presence_in(WEB_IMAGE_CONTENT_TYPES) || "image/png"
+    def upload(file)
+      service.upload(key, file)
     end
 
-    def transform(image)
-      format = "png" unless WEB_IMAGE_CONTENT_TYPES.include?(blob.content_type)
-      result = variation.transform(image, format: format)
 
-      begin
-        yield result
-      ensure
-        result.close!
-      end
+    def specification
+      @specification ||=
+        if 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
 
-    def upload(file)
-      service.upload(key, file)
-    end
+    delegate :filename, :content_type, :format, to: :specification
+
+    class Specification < OpenStruct; end
 end
diff --git a/activestorage/app/models/active_storage/variation.rb b/activestorage/app/models/active_storage/variation.rb
index ae1376a6cf..3adc2407e5 100644
--- a/activestorage/app/models/active_storage/variation.rb
+++ b/activestorage/app/models/active_storage/variation.rb
@@ -47,13 +47,9 @@ class ActiveStorage::Variation
   # 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)
+  def transform(file, format: nil, &block)
     ActiveSupport::Notifications.instrument("transform.active_storage") do
-      if processor
-        image_processing_transform(file, format)
-      else
-        mini_magick_transform(file, format)
-      end
+      transformer.transform(file, format: format, &block)
     end
   end
 
@@ -63,63 +59,22 @@ class ActiveStorage::Variation
   end
 
   private
-    # Applies image transformations using the ImageProcessing gem.
-    def image_processing_transform(file, format)
-      operations = transformations.each_with_object([]) do |(name, argument), list|
-        if name.to_s == "combine_options"
-          ActiveSupport::Deprecation.warn("The ImageProcessing ActiveStorage variant backend doesn't need :combine_options, as it already generates a single MiniMagick command. In Rails 6.1 :combine_options will not be supported anymore.")
-          list.concat argument.keep_if { |key, value| value.present? }.to_a
-        elsif argument.present?
-          list << [name, argument]
+    def transformer
+      if ActiveStorage.variant_processor
+        begin
+          require "image_processing"
+        rescue LoadError
+          ActiveSupport::Deprecation.warn <<~WARNING
+            Generating image variants will require the image_processing gem in Rails 6.1.
+            Please add `gem 'image_processing', '~> 1.2'` to your Gemfile.
+          WARNING
+
+          ActiveStorage::Transformers::MiniMagickTransformer.new(transformations)
+        else
+          ActiveStorage::Transformers::ImageProcessingTransformer.new(transformations)
         end
-      end
-
-      processor
-        .source(file)
-        .loader(page: 0)
-        .convert(format)
-        .apply(operations)
-        .call
-    end
-
-    # Applies image transformations using the MiniMagick gem.
-    def mini_magick_transform(file, format)
-      image = MiniMagick::Image.new(file.path, file)
-
-      transformations.each do |name, argument_or_subtransformations|
-        image.mogrify do |command|
-          if name.to_s == "combine_options"
-            argument_or_subtransformations.each do |subtransformation_name, subtransformation_argument|
-              pass_transform_argument(command, subtransformation_name, subtransformation_argument)
-            end
-          else
-            pass_transform_argument(command, name, argument_or_subtransformations)
-          end
-        end
-      end
-
-      image.format(format) if format
-
-      image.tempfile.tap(&:open)
-    end
-
-    # Returns the ImageProcessing processor class specified by `ActiveStorage.variant_processor`.
-    def processor
-      begin
-        require "image_processing"
-      rescue LoadError
-        ActiveSupport::Deprecation.warn("Using mini_magick gem directly is deprecated and will be removed in Rails 6.1. Please add `gem 'image_processing', '~> 1.2'` to your Gemfile.")
-        return nil
-      end
-
-      ImageProcessing.const_get(ActiveStorage.variant_processor.to_s.camelize) if ActiveStorage.variant_processor
-    end
-
-    def pass_transform_argument(command, method, argument)
-      if argument == true
-        command.public_send(method)
-      elsif argument.present?
-        command.public_send(method, argument)
+      else
+        ActiveStorage::Transformers::MiniMagickTransformer.new(transformations)
       end
     end
 end