Implement ActiveStorage::Attached::{One,Many}#attach in terms of changes

7 files changed, 183 insertions, 67 deletions

diff --git a/activestorage/lib/active_storage/attached.rb b/activestorage/lib/active_storage/attached.rb
index 1b53818581..0ccf76f00b 100644
--- a/activestorage/lib/active_storage/attached.rb
+++ b/activestorage/lib/active_storage/attached.rb
@@ -1,7 +1,5 @@
 # frozen_string_literal: true
 
-require "action_dispatch"
-require "action_dispatch/http/upload"
 require "active_support/core_ext/module/delegation"
 
 module ActiveStorage
@@ -18,24 +16,6 @@ module ActiveStorage
       def change
         record.attachment_changes[name]
       end
-
-      def create_blob_from(attachable)
-        case attachable
-        when ActiveStorage::Blob
-          attachable
-        when ActionDispatch::Http::UploadedFile, Rack::Test::UploadedFile
-          ActiveStorage::Blob.create_after_upload! \
-            io: attachable.open,
-            filename: attachable.original_filename,
-            content_type: attachable.content_type
-        when Hash
-          ActiveStorage::Blob.create_after_upload!(attachable)
-        when String
-          ActiveStorage::Blob.find_signed(attachable)
-        else
-          nil
-        end
-      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
index 3f7ca6a25f..af19328a61 100644
--- a/activestorage/lib/active_storage/attached/changes/create_many.rb
+++ b/activestorage/lib/active_storage/attached/changes/create_many.rb
@@ -12,6 +12,10 @@ module ActiveStorage
       @attachments ||= subchanges.collect(&:attachment)
     end
 
+    def blobs
+      @blobs ||= subchanges.collect(&:blob)
+    end
+
     def upload
       subchanges.each(&:upload)
     end
diff --git a/activestorage/lib/active_storage/attached/changes/create_one.rb b/activestorage/lib/active_storage/attached/changes/create_one.rb
index 98aea36861..5812fd2b08 100644
--- a/activestorage/lib/active_storage/attached/changes/create_one.rb
+++ b/activestorage/lib/active_storage/attached/changes/create_one.rb
@@ -1,5 +1,8 @@
 # frozen_string_literal: true
 
+require "action_dispatch"
+require "action_dispatch/http/upload"
+
 module ActiveStorage
   class Attached::Changes::CreateOne #:nodoc:
     attr_reader :name, :record, :attachable
diff --git a/activestorage/lib/active_storage/attached/many.rb b/activestorage/lib/active_storage/attached/many.rb
index 073cc013d8..25f88284df 100644
--- a/activestorage/lib/active_storage/attached/many.rb
+++ b/activestorage/lib/active_storage/attached/many.rb
@@ -12,19 +12,26 @@ module ActiveStorage
       change.present? ? change.attachments : record.public_send("#{name}_attachments")
     end
 
-    # Associates one or several attachments with the current record, saving them to the database.
+    # Returns all attached blobs.
+    def blobs
+      change.present? ? change.blobs : record.public_send("#{name}_blobs")
+    end
+
+    # Attaches one or more +attachables+ to the record.
+    #
+    # If the record is persisted and unchanged, the attachments are saved to
+    # the database immediately. Otherwise, they'll be saved to the DB when the
+    # record is next saved.
     #
     #   document.images.attach(params[:images]) # Array of ActionDispatch::Http::UploadedFile objects
     #   document.images.attach(params[:signed_blob_id]) # Signed reference to blob from direct upload
     #   document.images.attach(io: File.open("/path/to/racecar.jpg"), filename: "racecar.jpg", content_type: "image/jpg")
     #   document.images.attach([ first_blob, second_blob ])
     def attach(*attachables)
-      attachables.flatten.collect do |attachable|
-        if record.new_record?
-          attachments.build(record: record, blob: create_blob_from(attachable))
-        else
-          attachments.create!(record: record, blob: create_blob_from(attachable))
-        end
+      if record.persisted? && !record.changed?
+        record.update(name => blobs + attachables.flatten)
+      else
+        record.public_send("#{name}=", blobs + attachables.flatten)
       end
     end
 
diff --git a/activestorage/lib/active_storage/attached/one.rb b/activestorage/lib/active_storage/attached/one.rb
index 4a6bb1ffaa..c039226fcd 100644
--- a/activestorage/lib/active_storage/attached/one.rb
+++ b/activestorage/lib/active_storage/attached/one.rb
@@ -17,17 +17,21 @@ module ActiveStorage
       !attached?
     end
 
-    # Associates a given attachment with the current record, saving it to the database.
+    # Attaches an +attachable+ to the record.
+    #
+    # If the record is persisted and unchanged, the attachment is saved to
+    # the database immediately. Otherwise, it'll be saved to the DB when the
+    # record is next saved.
     #
     #   person.avatar.attach(params[:avatar]) # ActionDispatch::Http::UploadedFile object
     #   person.avatar.attach(params[:signed_blob_id]) # Signed reference to blob from direct upload
     #   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)
-      new_blob = create_blob_from(attachable)
-
-      if !attached? || new_blob != blob
-        write_attachment build_attachment(blob: new_blob)
+      if record.persisted? && !record.changed?
+        record.update(name => attachable)
+      else
+        record.public_send("#{name}=", attachable)
       end
     end
 
@@ -68,12 +72,6 @@ module ActiveStorage
     end
 
     private
-      delegate :transaction, to: :record
-
-      def build_attachment(blob:)
-        Attachment.new(record: record, name: name, blob: blob)
-      end
-
       def write_attachment(attachment)
         record.public_send("#{name}_attachment=", attachment)
       end
diff --git a/activestorage/test/models/attached/many_test.rb b/activestorage/test/models/attached/many_test.rb
index a9ff6eecc6..719032786b 100644
--- a/activestorage/test/models/attached/many_test.rb
+++ b/activestorage/test/models/attached/many_test.rb
@@ -39,6 +39,73 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     assert_equal "video.mp4", @user.highlights.second.filename.to_s
   end
 
+  test "attaching existing blobs to an existing, changed record" do
+    @user.name = "Tina"
+    assert @user.changed?
+
+    @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg")
+    assert_equal "funky.jpg", @user.highlights.first.filename.to_s
+    assert_equal "town.jpg", @user.highlights.second.filename.to_s
+    assert_not @user.highlights.first.persisted?
+    assert_not @user.highlights.second.persisted?
+    assert @user.will_save_change_to_name?
+
+    @user.save!
+    assert_equal "funky.jpg", @user.highlights.reload.first.filename.to_s
+    assert_equal "town.jpg", @user.highlights.second.filename.to_s
+  end
+
+  test "attaching existing blobs from signed IDs to an existing, changed record" do
+    @user.name = "Tina"
+    assert @user.changed?
+
+    @user.highlights.attach create_blob(filename: "funky.jpg").signed_id, create_blob(filename: "town.jpg").signed_id
+    assert_equal "funky.jpg", @user.highlights.first.filename.to_s
+    assert_equal "town.jpg", @user.highlights.second.filename.to_s
+    assert_not @user.highlights.first.persisted?
+    assert_not @user.highlights.second.persisted?
+    assert @user.will_save_change_to_name?
+
+    @user.save!
+    assert_equal "funky.jpg", @user.highlights.reload.first.filename.to_s
+    assert_equal "town.jpg", @user.highlights.second.filename.to_s
+  end
+
+  test "attaching new blobs from Hashes to an existing, changed record" do
+    @user.name = "Tina"
+    assert @user.changed?
+
+    @user.highlights.attach(
+      { io: StringIO.new("STUFF"), filename: "funky.jpg", content_type: "image/jpg" },
+      { io: StringIO.new("THINGS"), filename: "town.jpg", content_type: "image/jpeg" })
+
+    assert_equal "funky.jpg", @user.highlights.first.filename.to_s
+    assert_equal "town.jpg", @user.highlights.second.filename.to_s
+    assert_not @user.highlights.first.persisted?
+    assert_not @user.highlights.second.persisted?
+    assert @user.will_save_change_to_name?
+
+    @user.save!
+    assert_equal "funky.jpg", @user.highlights.reload.first.filename.to_s
+    assert_equal "town.jpg", @user.highlights.second.filename.to_s
+  end
+
+  test "attaching new blobs from uploaded files to an existing, changed record" do
+    @user.name = "Tina"
+    assert @user.changed?
+
+    @user.highlights.attach fixture_file_upload("racecar.jpg"), fixture_file_upload("video.mp4")
+    assert_equal "racecar.jpg", @user.highlights.first.filename.to_s
+    assert_equal "video.mp4", @user.highlights.second.filename.to_s
+    assert_not @user.highlights.first.persisted?
+    assert_not @user.highlights.second.persisted?
+    assert @user.will_save_change_to_name?
+
+    @user.save!
+    assert_equal "racecar.jpg", @user.highlights.reload.first.filename.to_s
+    assert_equal "video.mp4", @user.highlights.second.filename.to_s
+  end
+
   test "updating an existing record to attach existing blobs" do
     @user.update! highlights: [ create_file_blob(filename: "racecar.jpg"), create_file_blob(filename: "video.mp4") ]
     assert_equal "racecar.jpg", @user.highlights.first.filename.to_s
@@ -220,7 +287,7 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     end
   end
 
-  test "attaching new blob from Hashes to a new record" do
+  test "attaching new blobs from Hashes to a new record" do
     User.new(name: "Jason").tap do |user|
       user.highlights.attach(
         { io: StringIO.new("STUFF"), filename: "funky.jpg", content_type: "image/jpg" },
@@ -229,16 +296,22 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
       assert user.new_record?
       assert user.highlights.first.new_record?
       assert user.highlights.second.new_record?
-      assert_not user.highlights.first.blob.new_record?
-      assert_not user.highlights.second.blob.new_record?
+      assert user.highlights.first.blob.new_record?
+      assert user.highlights.second.blob.new_record?
       assert_equal "funky.jpg", user.highlights.first.filename.to_s
       assert_equal "town.jpg", user.highlights.second.filename.to_s
-      assert ActiveStorage::Blob.service.exist?(user.highlights.first.key)
-      assert ActiveStorage::Blob.service.exist?(user.highlights.second.key)
+      assert_not ActiveStorage::Blob.service.exist?(user.highlights.first.key)
+      assert_not ActiveStorage::Blob.service.exist?(user.highlights.second.key)
 
       user.save!
+      assert user.highlights.first.persisted?
+      assert user.highlights.second.persisted?
+      assert user.highlights.first.blob.persisted?
+      assert user.highlights.second.blob.persisted?
       assert_equal "funky.jpg", user.reload.highlights.first.filename.to_s
       assert_equal "town.jpg", user.highlights.second.filename.to_s
+      assert ActiveStorage::Blob.service.exist?(user.highlights.first.key)
+      assert ActiveStorage::Blob.service.exist?(user.highlights.second.key)
     end
   end
 
@@ -248,16 +321,22 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
       assert user.new_record?
       assert user.highlights.first.new_record?
       assert user.highlights.second.new_record?
-      assert_not user.highlights.first.blob.new_record?
-      assert_not user.highlights.second.blob.new_record?
+      assert user.highlights.first.blob.new_record?
+      assert user.highlights.second.blob.new_record?
       assert_equal "racecar.jpg", user.highlights.first.filename.to_s
       assert_equal "video.mp4", user.highlights.second.filename.to_s
-      assert ActiveStorage::Blob.service.exist?(user.highlights.first.key)
-      assert ActiveStorage::Blob.service.exist?(user.highlights.second.key)
+      assert_not ActiveStorage::Blob.service.exist?(user.highlights.first.key)
+      assert_not ActiveStorage::Blob.service.exist?(user.highlights.second.key)
 
       user.save!
+      assert user.highlights.first.persisted?
+      assert user.highlights.second.persisted?
+      assert user.highlights.first.blob.persisted?
+      assert user.highlights.second.blob.persisted?
       assert_equal "racecar.jpg", user.reload.highlights.first.filename.to_s
       assert_equal "video.mp4", user.highlights.second.filename.to_s
+      assert ActiveStorage::Blob.service.exist?(user.highlights.first.key)
+      assert ActiveStorage::Blob.service.exist?(user.highlights.second.key)
     end
   end
 
diff --git a/activestorage/test/models/attached/one_test.rb b/activestorage/test/models/attached/one_test.rb
index 177a6c84e6..6d2b8eaaf6 100644
--- a/activestorage/test/models/attached/one_test.rb
+++ b/activestorage/test/models/attached/one_test.rb
@@ -32,6 +32,58 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
     assert_equal "racecar.jpg", @user.avatar.filename.to_s
   end
 
+  test "attaching an existing blob to an existing, changed record" do
+    @user.name = "Tina"
+    assert @user.changed?
+
+    @user.avatar.attach create_blob(filename: "funky.jpg")
+    assert_equal "funky.jpg", @user.avatar.filename.to_s
+    assert_not @user.avatar.persisted?
+    assert @user.will_save_change_to_name?
+
+    @user.save!
+    assert_equal "funky.jpg", @user.reload.avatar.filename.to_s
+  end
+
+  test "attaching an existing blob from a signed ID to an existing, changed record" do
+    @user.name = "Tina"
+    assert @user.changed?
+
+    @user.avatar.attach create_blob(filename: "funky.jpg").signed_id
+    assert_equal "funky.jpg", @user.avatar.filename.to_s
+    assert_not @user.avatar.persisted?
+    assert @user.will_save_change_to_name?
+
+    @user.save!
+    assert_equal "funky.jpg", @user.reload.avatar.filename.to_s
+  end
+
+  test "attaching a new blob from a Hash to an existing, changed record" do
+    @user.name = "Tina"
+    assert @user.changed?
+
+    @user.avatar.attach io: StringIO.new("STUFF"), filename: "town.jpg", content_type: "image/jpg"
+    assert_equal "town.jpg", @user.avatar.filename.to_s
+    assert_not @user.avatar.persisted?
+    assert @user.will_save_change_to_name?
+
+    @user.save!
+    assert_equal "town.jpg", @user.reload.avatar.filename.to_s
+  end
+
+  test "attaching a new blob from an uploaded file to an existing, changed record" do
+    @user.name = "Tina"
+    assert @user.changed?
+
+    @user.avatar.attach fixture_file_upload("racecar.jpg")
+    assert_equal "racecar.jpg", @user.avatar.filename.to_s
+    assert_not @user.avatar.persisted?
+    assert @user.will_save_change_to_name?
+
+    @user.save!
+    assert_equal "racecar.jpg", @user.reload.avatar.filename.to_s
+  end
+
   test "updating an existing record to attach an existing blob" do
     @user.update! avatar: create_blob(filename: "funky.jpg")
     assert_equal "funky.jpg", @user.avatar.filename.to_s
@@ -71,7 +123,7 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
     end
   end
 
-  test "successfully replacing an existing, independent attachment on an existing record" do
+  test "replacing an existing, independent attachment on an existing record" do
     @user.cover_photo.attach create_blob(filename: "funky.jpg")
 
     assert_no_enqueued_jobs only: ActiveStorage::PurgeJob do
@@ -81,20 +133,7 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
     assert_equal "town.jpg", @user.cover_photo.filename.to_s
   end
 
-  test "unsuccessfully replacing an existing attachment on an existing record" do
-    @user.avatar.attach create_blob(filename: "funky.jpg")
-
-    assert_no_enqueued_jobs do
-      assert_raises do
-        @user.avatar.attach nil
-      end
-    end
-
-    assert_equal "funky.jpg", @user.avatar.filename.to_s
-    assert ActiveStorage::Blob.service.exist?(@user.avatar.key)
-  end
-
-  test "replacing an existing attachment on an existing record with the same blob" do
+  test "replacing an attached blob on an existing record with itself" do
     create_blob(filename: "funky.jpg").tap do |blob|
       @user.avatar.attach blob
 
@@ -149,7 +188,7 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
       @user.avatar.attach blob
 
       assert_no_enqueued_jobs do
-        assert_no_changes -> { @user.avatar_attachment.id } do
+        assert_no_changes -> { @user.reload.avatar_attachment.id } do
           @user.update! avatar: blob
         end
       end
@@ -248,12 +287,15 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
       user.avatar.attach io: StringIO.new("STUFF"), filename: "town.jpg", content_type: "image/jpg"
       assert user.new_record?
       assert user.avatar.attachment.new_record?
-      assert_not user.avatar.blob.new_record?
+      assert user.avatar.blob.new_record?
       assert_equal "town.jpg", user.avatar.filename.to_s
-      assert ActiveStorage::Blob.service.exist?(user.avatar.key)
+      assert_not ActiveStorage::Blob.service.exist?(user.avatar.key)
 
       user.save!
+      assert user.avatar.attachment.persisted?
+      assert user.avatar.blob.persisted?
       assert_equal "town.jpg", user.reload.avatar.filename.to_s
+      assert ActiveStorage::Blob.service.exist?(user.avatar.key)
     end
   end
 
@@ -262,12 +304,15 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
       user.avatar.attach fixture_file_upload("racecar.jpg")
       assert user.new_record?
       assert user.avatar.attachment.new_record?
-      assert_not user.avatar.blob.new_record?
+      assert user.avatar.blob.new_record?
       assert_equal "racecar.jpg", user.avatar.filename.to_s
-      assert ActiveStorage::Blob.service.exist?(user.avatar.key)
+      assert_not ActiveStorage::Blob.service.exist?(user.avatar.key)
 
       user.save!
+      assert user.avatar.attachment.persisted?
+      assert user.avatar.blob.persisted?
       assert_equal "racecar.jpg", user.reload.avatar.filename.to_s
+      assert ActiveStorage::Blob.service.exist?(user.avatar.key)
     end
   end