Store newly-uploaded files on save rather than assignment

12 files changed, 326 insertions, 138 deletions

diff --git a/activestorage/lib/active_storage/attached.rb b/activestorage/lib/active_storage/attached.rb
index c08fd56652..1b53818581 100644
--- a/activestorage/lib/active_storage/attached.rb
+++ b/activestorage/lib/active_storage/attached.rb
@@ -15,6 +15,10 @@ module ActiveStorage
     end
 
     private
+      def change
+        record.attachment_changes[name]
+      end
+
       def create_blob_from(attachable)
         case attachable
         when ActiveStorage::Blob
@@ -35,6 +39,7 @@ module ActiveStorage
   end
 end
 
+require "active_storage/attached/model"
 require "active_storage/attached/one"
 require "active_storage/attached/many"
-require "active_storage/attached/macros"
+require "active_storage/attached/changes"
diff --git a/activestorage/lib/active_storage/attached/changes.rb b/activestorage/lib/active_storage/attached/changes.rb
new file mode 100644
index 0000000000..1db3906a63
--- /dev/null
+++ b/activestorage/lib/active_storage/attached/changes.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module ActiveStorage
+  module Attached::Changes #:nodoc:
+    extend ActiveSupport::Autoload
+
+    eager_autoload do
+      autoload :CreateOne
+      autoload :CreateMany
+      autoload :CreateOneOfMany
+
+      autoload :DeleteOne
+      autoload :DeleteMany
+    end
+  end
+end
diff --git a/activestorage/lib/active_storage/attached/changes/create_many.rb b/activestorage/lib/active_storage/attached/changes/create_many.rb
new file mode 100644
index 0000000000..3f7ca6a25f
--- /dev/null
+++ b/activestorage/lib/active_storage/attached/changes/create_many.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module ActiveStorage
+  class Attached::Changes::CreateMany #:nodoc:
+    attr_reader :name, :record, :attachables
+
+    def initialize(name, record, attachables)
+      @name, @record, @attachables = name, record, Array(attachables)
+    end
+
+    def attachments
+      @attachments ||= subchanges.collect(&:attachment)
+    end
+
+    def upload
+      subchanges.each(&:upload)
+    end
+
+    def save
+      record.public_send("#{name}_attachments=", attachments)
+    end
+
+    private
+      def subchanges
+        @subchanges ||= attachables.collect { |attachable| build_subchange_from(attachable) }
+      end
+
+      def build_subchange_from(attachable)
+        ActiveStorage::Attached::Changes::CreateOneOfMany.new(name, record, attachable)
+      end
+  end
+end
diff --git a/activestorage/lib/active_storage/attached/changes/create_one.rb b/activestorage/lib/active_storage/attached/changes/create_one.rb
new file mode 100644
index 0000000000..bb59a651ba
--- /dev/null
+++ b/activestorage/lib/active_storage/attached/changes/create_one.rb
@@ -0,0 +1,65 @@
+# frozen_string_literal: true
+
+module ActiveStorage
+  class Attached::Changes::CreateOne #:nodoc:
+    attr_reader :name, :record, :attachable
+
+    def initialize(name, record, attachable)
+      @name, @record, @attachable = name, record, attachable
+    end
+
+    def attachment
+      @attachment ||= find_or_build_attachment
+    end
+
+    def blob
+      @blob ||= find_or_build_blob
+    end
+
+    def upload
+      case attachable
+      when ActionDispatch::Http::UploadedFile, Rack::Test::UploadedFile
+        blob.upload_without_unfurling(attachable.open)
+      when Hash
+        blob.upload_without_unfurling(attachable.fetch(:io))
+      end
+    end
+
+    def save
+      record.public_send("#{name}_attachment=", attachment)
+    end
+
+    private
+      def find_or_build_attachment
+        find_attachment || build_attachment
+      end
+
+      def find_attachment
+        if record.public_send("#{name}_blob") == blob
+          record.public_send("#{name}_attachment")
+        end
+      end
+
+      def build_attachment
+        ActiveStorage::Attachment.new(record: record, name: name, blob: blob)
+      end
+
+      def find_or_build_blob
+        case attachable
+        when ActiveStorage::Blob
+          attachable
+        when ActionDispatch::Http::UploadedFile, Rack::Test::UploadedFile
+          ActiveStorage::Blob.build_after_unfurling \
+            io: attachable.open,
+            filename: attachable.original_filename,
+            content_type: attachable.content_type
+        when Hash
+          ActiveStorage::Blob.build_after_unfurling(attachable)
+        when String
+          ActiveStorage::Blob.find_signed(attachable)
+        else
+          raise "Could not find or build blob: expected attachable, got #{attachable.inspect}"
+        end
+      end
+  end
+end
diff --git a/activestorage/lib/active_storage/attached/changes/create_one_of_many.rb b/activestorage/lib/active_storage/attached/changes/create_one_of_many.rb
new file mode 100644
index 0000000000..7268e87316
--- /dev/null
+++ b/activestorage/lib/active_storage/attached/changes/create_one_of_many.rb
@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+module ActiveStorage
+  class Attached::Changes::CreateOneOfMany < Attached::Changes::CreateOne #:nodoc:
+    private
+      def find_attachment
+        record.public_send("#{name}_attachments").detect { |attachment| attachment.blob_id == blob.id }
+      end
+  end
+end
diff --git a/activestorage/lib/active_storage/attached/changes/delete_many.rb b/activestorage/lib/active_storage/attached/changes/delete_many.rb
new file mode 100644
index 0000000000..5c7fe385de
--- /dev/null
+++ b/activestorage/lib/active_storage/attached/changes/delete_many.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module ActiveStorage
+  class Attached::Changes::DeleteMany #:nodoc:
+    attr_reader :name, :record
+
+    def initialize(name, record)
+      @name, @record = name, record
+    end
+
+    def attachments
+      ActiveStorage::Attachment.none
+    end
+
+    def save
+      record.public_send("#{name}_attachments=", [])
+    end
+  end
+end
diff --git a/activestorage/lib/active_storage/attached/changes/delete_one.rb b/activestorage/lib/active_storage/attached/changes/delete_one.rb
new file mode 100644
index 0000000000..2f7d356613
--- /dev/null
+++ b/activestorage/lib/active_storage/attached/changes/delete_one.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module ActiveStorage
+  class Attached::Changes::DeleteOne #:nodoc:
+    attr_reader :name, :record
+
+    def initialize(name, record)
+      @name, @record = name, record
+    end
+
+    def attachment
+      nil
+    end
+
+    def save
+      record.public_send("#{name}_attachment=", nil)
+    end
+  end
+end
diff --git a/activestorage/lib/active_storage/attached/macros.rb b/activestorage/lib/active_storage/attached/macros.rb
deleted file mode 100644
index 6ad9fc43d7..0000000000
--- a/activestorage/lib/active_storage/attached/macros.rb
+++ /dev/null
@@ -1,122 +0,0 @@
-# frozen_string_literal: true
-
-module ActiveStorage
-  # Provides the class-level DSL for declaring that an Active Record model has attached blobs.
-  module Attached::Macros
-    # Specifies the relation between a single attachment and the model.
-    #
-    #   class User < ActiveRecord::Base
-    #     has_one_attached :avatar
-    #   end
-    #
-    # There is no column defined on the model side, Active Storage takes
-    # care of the mapping between your records and the attachment.
-    #
-    # To avoid N+1 queries, you can include the attached blobs in your query like so:
-    #
-    #   User.with_attached_avatar
-    #
-    # Under the covers, this relationship is implemented as a +has_one+ association to a
-    # ActiveStorage::Attachment record and a +has_one-through+ association to a
-    # ActiveStorage::Blob record. These associations are available as +avatar_attachment+
-    # and +avatar_blob+. But you shouldn't need to work with these associations directly in
-    # most circumstances.
-    #
-    # The system has been designed to having you go through the ActiveStorage::Attached::One
-    # proxy that provides the dynamic proxy to the associations and factory methods, like +attach+.
-    #
-    # If the +:dependent+ option isn't set, the attachment will be purged
-    # (i.e. destroyed) whenever the record is destroyed.
-    def has_one_attached(name, dependent: :purge_later)
-      generated_association_methods.class_eval <<-CODE, __FILE__, __LINE__ + 1
-        def #{name}
-          @active_storage_attached_#{name} ||= ActiveStorage::Attached::One.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"})
-        end
-
-        def #{name}=(attachable)
-          #{name}.attach(attachable)
-        end
-      CODE
-
-      has_one :"#{name}_attachment", -> { where(name: name) }, class_name: "ActiveStorage::Attachment", as: :record, inverse_of: :record, dependent: false
-      has_one :"#{name}_blob", through: :"#{name}_attachment", class_name: "ActiveStorage::Blob", source: :blob
-
-      scope :"with_attached_#{name}", -> { includes("#{name}_attachment": :blob) }
-
-      if dependent == :purge_later
-        after_destroy_commit { public_send(name).purge_later }
-      else
-        before_destroy { public_send(name).detach }
-      end
-
-      ActiveRecord::Reflection.add_attachment_reflection(
-        self,
-        name,
-        ActiveRecord::Reflection.create(:has_one_attached, name, nil, { dependent: dependent }, self)
-      )
-    end
-
-    # Specifies the relation between multiple attachments and the model.
-    #
-    #   class Gallery < ActiveRecord::Base
-    #     has_many_attached :photos
-    #   end
-    #
-    # There are no columns defined on the model side, Active Storage takes
-    # care of the mapping between your records and the attachments.
-    #
-    # To avoid N+1 queries, you can include the attached blobs in your query like so:
-    #
-    #   Gallery.where(user: Current.user).with_attached_photos
-    #
-    # Under the covers, this relationship is implemented as a +has_many+ association to a
-    # ActiveStorage::Attachment record and a +has_many-through+ association to a
-    # ActiveStorage::Blob record. These associations are available as +photos_attachments+
-    # and +photos_blobs+. But you shouldn't need to work with these associations directly in
-    # most circumstances.
-    #
-    # The system has been designed to having you go through the ActiveStorage::Attached::Many
-    # proxy that provides the dynamic proxy to the associations and factory methods, like +#attach+.
-    #
-    # If the +:dependent+ option isn't set, all the attachments will be purged
-    # (i.e. destroyed) whenever the record is destroyed.
-    def has_many_attached(name, dependent: :purge_later)
-      generated_association_methods.class_eval <<-CODE, __FILE__, __LINE__ + 1
-        def #{name}
-          @active_storage_attached_#{name} ||= ActiveStorage::Attached::Many.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"})
-        end
-
-        def #{name}=(attachables)
-          #{name}.attach(attachables)
-        end
-      CODE
-
-      has_many :"#{name}_attachments", -> { where(name: name) }, as: :record, class_name: "ActiveStorage::Attachment", inverse_of: :record, dependent: false do
-        def purge
-          each(&:purge)
-          reset
-        end
-
-        def purge_later
-          each(&:purge_later)
-          reset
-        end
-      end
-      has_many :"#{name}_blobs", through: :"#{name}_attachments", class_name: "ActiveStorage::Blob", source: :blob
-
-      scope :"with_attached_#{name}", -> { includes("#{name}_attachments": :blob) }
-
-      if dependent == :purge_later
-        after_destroy_commit { public_send(name).purge_later }
-      else
-        before_destroy { public_send(name).detach }
-      end
-
-      ActiveRecord::Reflection.add_attachment_reflection(
-        self,
-        name,
-        ActiveRecord::Reflection.create(:has_many_attached, name, nil, { dependent: dependent }, self)
-      )
-    end
-  end
-end
diff --git a/activestorage/lib/active_storage/attached/many.rb b/activestorage/lib/active_storage/attached/many.rb
index d61acb6fad..204d6604c8 100644
--- a/activestorage/lib/active_storage/attached/many.rb
+++ b/activestorage/lib/active_storage/attached/many.rb
@@ -9,7 +9,7 @@ module ActiveStorage
     #
     # All methods called on this proxy object that aren't listed here will automatically be delegated to +attachments+.
     def attachments
-      record.public_send("#{name}_attachments")
+      change.present? ? change.attachments : record.public_send("#{name}_attachments")
     end
 
     # Associates one or several attachments with the current record, saving them to the database.
@@ -50,7 +50,6 @@ module ActiveStorage
     # Directly purges each associated attachment (i.e. destroys the blobs and
     # attachments and deletes the files on the service).
 
-
     ##
     # :method: purge_later
     #
diff --git a/activestorage/lib/active_storage/attached/model.rb b/activestorage/lib/active_storage/attached/model.rb
new file mode 100644
index 0000000000..287c7a9253
--- /dev/null
+++ b/activestorage/lib/active_storage/attached/model.rb
@@ -0,0 +1,150 @@
+# frozen_string_literal: true
+
+module ActiveStorage
+  # Provides the class-level DSL for declaring an Active Record model's attachments.
+  module Attached::Model
+    extend ActiveSupport::Concern
+
+    class_methods do
+      # Specifies the relation between a single attachment and the model.
+      #
+      #   class User < ActiveRecord::Base
+      #     has_one_attached :avatar
+      #   end
+      #
+      # There is no column defined on the model side, Active Storage takes
+      # care of the mapping between your records and the attachment.
+      #
+      # To avoid N+1 queries, you can include the attached blobs in your query like so:
+      #
+      #   User.with_attached_avatar
+      #
+      # Under the covers, this relationship is implemented as a +has_one+ association to a
+      # ActiveStorage::Attachment record and a +has_one-through+ association to a
+      # ActiveStorage::Blob record. These associations are available as +avatar_attachment+
+      # and +avatar_blob+. But you shouldn't need to work with these associations directly in
+      # most circumstances.
+      #
+      # The system has been designed to having you go through the ActiveStorage::Attached::One
+      # proxy that provides the dynamic proxy to the associations and factory methods, like +attach+.
+      #
+      # If the +:dependent+ option isn't set, the attachment will be purged
+      # (i.e. destroyed) whenever the record is destroyed.
+      def has_one_attached(name, dependent: :purge_later)
+        generated_association_methods.class_eval <<-CODE, __FILE__, __LINE__ + 1
+          def #{name}
+            @active_storage_attached_#{name} ||= ActiveStorage::Attached::One.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"})
+          end
+
+          def #{name}=(attachable)
+            attachment_changes["#{name}"] =
+              if attachable.nil?
+                ActiveStorage::Attached::Changes::DeleteOne.new("#{name}", self)
+              else
+                ActiveStorage::Attached::Changes::CreateOne.new("#{name}", self, attachable)
+              end
+          end
+        CODE
+
+        has_one :"#{name}_attachment", -> { where(name: name) }, class_name: "ActiveStorage::Attachment", as: :record, inverse_of: :record, dependent: :destroy
+        has_one :"#{name}_blob", through: :"#{name}_attachment", class_name: "ActiveStorage::Blob", source: :blob
+
+        scope :"with_attached_#{name}", -> { includes("#{name}_attachment": :blob) }
+
+        after_save { attachment_changes[name.to_s]&.save }
+
+        ActiveRecord::Reflection.add_attachment_reflection(
+          self,
+          name,
+          ActiveRecord::Reflection.create(:has_one_attached, name, nil, { dependent: dependent }, self)
+        )
+      end
+
+      # Specifies the relation between multiple attachments and the model.
+      #
+      #   class Gallery < ActiveRecord::Base
+      #     has_many_attached :photos
+      #   end
+      #
+      # There are no columns defined on the model side, Active Storage takes
+      # care of the mapping between your records and the attachments.
+      #
+      # To avoid N+1 queries, you can include the attached blobs in your query like so:
+      #
+      #   Gallery.where(user: Current.user).with_attached_photos
+      #
+      # Under the covers, this relationship is implemented as a +has_many+ association to a
+      # ActiveStorage::Attachment record and a +has_many-through+ association to a
+      # ActiveStorage::Blob record. These associations are available as +photos_attachments+
+      # and +photos_blobs+. But you shouldn't need to work with these associations directly in
+      # most circumstances.
+      #
+      # The system has been designed to having you go through the ActiveStorage::Attached::Many
+      # proxy that provides the dynamic proxy to the associations and factory methods, like +#attach+.
+      #
+      # If the +:dependent+ option isn't set, all the attachments will be purged
+      # (i.e. destroyed) whenever the record is destroyed.
+      def has_many_attached(name, dependent: :purge_later)
+        generated_association_methods.class_eval <<-CODE, __FILE__, __LINE__ + 1
+          def #{name}
+            @active_storage_attached_#{name} ||= ActiveStorage::Attached::Many.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"})
+          end
+
+          def #{name}=(attachables)
+            attachment_changes["#{name}"] =
+              if attachables.nil? || Array(attachables).none?
+                ActiveStorage::Attached::Changes::DeleteMany.new("#{name}", self)
+              else
+                ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, attachables)
+              end
+          end
+        CODE
+
+        has_many :"#{name}_attachments", -> { where(name: name) }, as: :record, class_name: "ActiveStorage::Attachment", inverse_of: :record, dependent: :destroy do
+          def purge
+            each(&:purge)
+            reset
+          end
+
+          def purge_later
+            each(&:purge_later)
+            reset
+          end
+        end
+        has_many :"#{name}_blobs", through: :"#{name}_attachments", class_name: "ActiveStorage::Blob", source: :blob
+
+        scope :"with_attached_#{name}", -> { includes("#{name}_attachments": :blob) }
+
+        after_save { attachment_changes[name.to_s]&.save }
+
+        ActiveRecord::Reflection.add_attachment_reflection(
+          self,
+          name,
+          ActiveRecord::Reflection.create(:has_many_attached, name, nil, { dependent: dependent }, self)
+        )
+      end
+    end
+
+    def committed!(*) #:nodoc:
+      unless destroyed?
+        upload_attachment_changes
+        clear_attachment_changes
+      end
+
+      super
+    end
+
+    def attachment_changes #:nodoc:
+      @attachment_changes ||= {}
+    end
+
+    private
+      def upload_attachment_changes
+        attachment_changes.each_value { |change| change.try(:upload) }
+      end
+
+      def clear_attachment_changes
+        @attachment_changes = {}
+      end
+  end
+end
diff --git a/activestorage/lib/active_storage/attached/one.rb b/activestorage/lib/active_storage/attached/one.rb
index f992cb5f84..960ff99e63 100644
--- a/activestorage/lib/active_storage/attached/one.rb
+++ b/activestorage/lib/active_storage/attached/one.rb
@@ -10,11 +10,11 @@ module ActiveStorage
     # You don't have to call this method to access the attachment's methods as
     # they are all available at the model level.
     def attachment
-      record.public_send("#{name}_attachment")
+      change.present? ? change.attachment : record.public_send("#{name}_attachment")
     end
 
     def blank?
-      attachment.blank?
+      !attached?
     end
 
     # Associates a given attachment with the current record, saving it to the database.
@@ -24,16 +24,10 @@ module ActiveStorage
     #   person.avatar.attach(io: File.open("/path/to/face.jpg"), filename: "face.jpg", content_type: "image/jpg")
     #   person.avatar.attach(avatar_blob) # ActiveStorage::Blob object
     def attach(attachable)
-      blob_was = blob if attached?
-      blob = create_blob_from(attachable)
+      new_blob = create_blob_from(attachable)
 
-      unless blob == blob_was
-        transaction do
-          detach
-          write_attachment build_attachment(blob: blob)
-        end
-
-        blob_was.purge_later if blob_was && dependent == :purge_later
+      if !attached? || new_blob != blob
+        write_attachment build_attachment(blob: new_blob)
       end
     end
 
@@ -69,6 +63,7 @@ module ActiveStorage
     def purge_later
       if attached?
         attachment.purge_later
+        write_attachment nil
       end
     end
 
@@ -76,7 +71,7 @@ module ActiveStorage
       delegate :transaction, to: :record
 
       def build_attachment(blob:)
-        ActiveStorage::Attachment.new(record: record, name: name, blob: blob)
+        Attachment.new(record: record, name: name, blob: blob)
       end
 
       def write_attachment(attachment)
diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb
index 93c5c55b95..9d6a27eabe 100644
--- a/activestorage/lib/active_storage/engine.rb
+++ b/activestorage/lib/active_storage/engine.rb
@@ -62,7 +62,7 @@ module ActiveStorage
       require "active_storage/attached"
 
       ActiveSupport.on_load(:active_record) do
-        extend ActiveStorage::Attached::Macros
+        include ActiveStorage::Attached::Model
       end
     end