Preserve existing attachment assignment behavior for upgraded apps

Assigning to a collection of attachments appends rather than replacing, as in 5.2. Existing 5.2 apps that rely on this behavior will no longer break when they're upgraded to 6.0.

For apps generated on 6.0 or newer, assigning replaces the existing attachments in the collection. #attach should be used to add new attachments to the collection without removing existing ones.

I expect that we'll deprecate the old behavior in 6.1.

Closes #36374.

4 files changed, 53 insertions, 10 deletions

diff --git a/activestorage/lib/active_storage.rb b/activestorage/lib/active_storage.rb
index 5c5da551ae..75eb63f0b4 100644
--- a/activestorage/lib/active_storage.rb
+++ b/activestorage/lib/active_storage.rb
@@ -43,18 +43,26 @@ module ActiveStorage
 
   mattr_accessor :logger
   mattr_accessor :verifier
+  mattr_accessor :variant_processor, default: :mini_magick
+
   mattr_accessor :queues, default: {}
+
   mattr_accessor :previewers, default: []
-  mattr_accessor :analyzers, default: []
-  mattr_accessor :variant_processor, default: :mini_magick
+  mattr_accessor :analyzers,  default: []
+
   mattr_accessor :paths, default: {}
-  mattr_accessor :variable_content_types, default: []
+
+  mattr_accessor :variable_content_types,           default: []
+  mattr_accessor :binary_content_type,              default: "application/octet-stream"
   mattr_accessor :content_types_to_serve_as_binary, default: []
-  mattr_accessor :content_types_allowed_inline, default: []
-  mattr_accessor :binary_content_type, default: "application/octet-stream"
+  mattr_accessor :content_types_allowed_inline,     default: []
+
   mattr_accessor :service_urls_expire_in, default: 5.minutes
+
   mattr_accessor :routes_prefix, default: "/rails/active_storage"
 
+  mattr_accessor :replace_on_assign_to_many, default: false
+
   module Transformers
     extend ActiveSupport::Autoload
 
diff --git a/activestorage/lib/active_storage/attached/model.rb b/activestorage/lib/active_storage/attached/model.rb
index ae7f0685f2..06864a846f 100644
--- a/activestorage/lib/active_storage/attached/model.rb
+++ b/activestorage/lib/active_storage/attached/model.rb
@@ -93,12 +93,19 @@ module ActiveStorage
           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)
+            if ActiveStorage.replace_on_assign_to_many
+              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
+            else
+              if !attachables.nil? || Array(attachables).any?
+                attachment_changes["#{name}"] =
+                  ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, #{name}.blobs + attachables)
               end
+            end
           end
         CODE
 
diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb
index f70d0a512a..e88e7fa14e 100644
--- a/activestorage/lib/active_storage/engine.rb
+++ b/activestorage/lib/active_storage/engine.rb
@@ -79,6 +79,8 @@ module ActiveStorage
         ActiveStorage.service_urls_expire_in = app.config.active_storage.service_urls_expire_in || 5.minutes
         ActiveStorage.content_types_allowed_inline = app.config.active_storage.content_types_allowed_inline || []
         ActiveStorage.binary_content_type = app.config.active_storage.binary_content_type || "application/octet-stream"
+
+        ActiveStorage.replace_on_assign_to_many = app.config.active_storage.replace_on_assign_to_many || false
       end
     end
 
diff --git a/activestorage/test/models/attached/many_test.rb b/activestorage/test/models/attached/many_test.rb
index 878e284049..39ddecb041 100644
--- a/activestorage/test/models/attached/many_test.rb
+++ b/activestorage/test/models/attached/many_test.rb
@@ -269,6 +269,24 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "updating an existing record with attachments when appending on assign" do
+    append_on_assign do
+      @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg")
+
+      assert_difference -> { @user.reload.highlights.count }, +2 do
+        @user.update! highlights: [ create_blob(filename: "whenever.jpg"), create_blob(filename: "wherever.jpg") ]
+      end
+
+      assert_no_difference -> { @user.reload.highlights.count } do
+        @user.update! highlights: [ ]
+      end
+
+      assert_no_difference -> { @user.reload.highlights.count } do
+        @user.update! highlights: nil
+      end
+    end
+  end
+
   test "attaching existing blobs to a new record" do
     User.new(name: "Jason").tap do |user|
       user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg")
@@ -538,4 +556,12 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
       User.remove_method :highlights
     end
   end
+
+  private
+    def append_on_assign
+      ActiveStorage.replace_on_assign_to_many, previous = false, ActiveStorage.replace_on_assign_to_many
+      yield
+    ensure
+      ActiveStorage.replace_on_assign_to_many = previous
+    end
 end