Fix that models can clobber each others' attachment reflections

Consider the following model definitions:

    class User < ApplicationRecord
      has_one_attached :avatar
    end

    class Group < ApplicationRecord
      has_one_attached :avatar
    end

If you attempt to reflect on the User model's avatar attachment via User.reflect_on_attachment, you could receive a reflection for the Group model's avatar attachment. Fix this by ensuring that each model class uses its own Hash object to track attachment reflections.

3 files changed, 25 insertions, 16 deletions

diff --git a/activestorage/lib/active_storage/reflection.rb b/activestorage/lib/active_storage/reflection.rb
index 04a1b20882..ce248c88b5 100644
--- a/activestorage/lib/active_storage/reflection.rb
+++ b/activestorage/lib/active_storage/reflection.rb
@@ -19,8 +19,8 @@ module ActiveStorage
     end
 
     module ReflectionExtension # :nodoc:
-      def add_attachment_reflection(ar, name, reflection)
-        ar.attachment_reflections.merge!(name.to_s => reflection)
+      def add_attachment_reflection(model, name, reflection)
+        model.attachment_reflections = model.attachment_reflections.merge(name.to_s => reflection)
       end
 
       private
diff --git a/activestorage/test/models/reflection_test.rb b/activestorage/test/models/reflection_test.rb
index da866ca996..98606b0617 100644
--- a/activestorage/test/models/reflection_test.rb
+++ b/activestorage/test/models/reflection_test.rb
@@ -3,27 +3,32 @@
 require "test_helper"
 
 class ActiveStorage::ReflectionTest < ActiveSupport::TestCase
-  test "allows reflecting for all attachment" do
-    expected_classes =
-      User.reflect_on_all_attachments.all? do |reflection|
-        reflection.is_a?(ActiveStorage::Reflection::HasOneAttachedReflection) ||
-          reflection.is_a?(ActiveStorage::Reflection::HasManyAttachedReflection)
-      end
-
-    assert expected_classes
-  end
-
-  test "allows reflecting on a singular has_one_attached attachment" do
+  test "reflecting on a singular attachment" do
     reflection = User.reflect_on_attachment(:avatar)
-
+    assert_equal User, reflection.active_record
     assert_equal :avatar, reflection.name
     assert_equal :has_one_attached, reflection.macro
+    assert_equal :purge_later, reflection.options[:dependent]
   end
 
-  test "allows reflecting on a singular has_many_attached attachment" do
-    reflection = User.reflect_on_attachment(:highlights)
+  test "reflection on a singular attachment with the same name as an attachment on another model" do
+    reflection = Group.reflect_on_attachment(:avatar)
+    assert_equal Group, reflection.active_record
+  end
 
+  test "reflecting on a collection attachment" do
+    reflection = User.reflect_on_attachment(:highlights)
+    assert_equal User, reflection.active_record
     assert_equal :highlights, reflection.name
     assert_equal :has_many_attached, reflection.macro
+    assert_equal :purge_later, reflection.options[:dependent]
+  end
+
+  test "reflecting on all attachments" do
+    reflections = User.reflect_on_all_attachments.sort_by(&:name)
+    assert_equal [ User ], reflections.collect(&:active_record).uniq
+    assert_equal %i[ avatar cover_photo highlights vlogs ], reflections.collect(&:name)
+    assert_equal %i[ has_one_attached has_one_attached has_many_attached has_many_attached ], reflections.collect(&:macro)
+    assert_equal [ :purge_later, false, :purge_later, false ], reflections.collect { |reflection| reflection.options[:dependent] }
   end
 end
diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb
index 573a8e0b0b..9985a76832 100644
--- a/activestorage/test/test_helper.rb
+++ b/activestorage/test/test_helper.rb
@@ -92,3 +92,7 @@ class User < ActiveRecord::Base
   has_many_attached :highlights
   has_many_attached :vlogs, dependent: false
 end
+
+class Group < ActiveRecord::Base
+  has_one_attached :avatar
+end