Merge pull request rails#51726 from Shopify/sym-associations

Use symbols as keys for `_reflections`

Author: byroot

activerecord/lib/active_record/associations.rb

@@ -2114,7 +2114,7 @@ def destroy_associations
           end
 
           has_many name, scope, **hm_options, &extension
-          _reflections[name.to_s].parent_reflection = habtm_reflection
+          _reflections[name].parent_reflection = habtm_reflection
         end
       end
   end

activerecord/lib/active_record/associations/preloader/branch.rb

@@ -9,7 +9,13 @@ class Branch # :nodoc:
         attr_writer :preloaded_records
 
         def initialize(association:, children:, parent:, associate_by_default:, scope:)
-          @association = association
+          @association = if association
+            begin
+              @association = association.to_sym
+            rescue NoMethodError
+              raise ArgumentError, "Association names must be Symbol or String, got: #{association.class.name}"
+            end
+          end
           @parent = parent
           @scope = scope
           @associate_by_default = associate_by_default

activerecord/lib/active_record/message_pack.rb

@@ -79,7 +79,7 @@ def build_entry(record)
       end
 
       def add_cached_associations(record, entry)
-        record.class.reflections.each_value do |reflection|
+        record.class.normalized_reflections.each_value do |reflection|
           if record.association_cached?(reflection.name)
             entry << reflection.name << encode(record.association(reflection.name).target)
           end

activerecord/lib/active_record/reflection.rb

@@ -22,12 +22,12 @@ def create(macro, name, scope, options, ar)
 
       def add_reflection(ar, name, reflection)
         ar.clear_reflections_cache
-        name = -name.to_s
+        name = name.to_sym
         ar._reflections = ar._reflections.except(name).merge!(name => reflection)
       end
 
       def add_aggregate_reflection(ar, name, reflection)
-        ar.aggregate_reflections = ar.aggregate_reflections.merge(-name.to_s => reflection)
+        ar.aggregate_reflections = ar.aggregate_reflections.merge(name.to_sym => reflection)
       end
 
       private
@@ -68,14 +68,18 @@ def reflect_on_all_aggregations
       #   Account.reflect_on_aggregation(:balance) # => the balance AggregateReflection
       #
       def reflect_on_aggregation(aggregation)
-        aggregate_reflections[aggregation.to_s]
+        aggregate_reflections[aggregation.to_sym]
       end
 
       # Returns a Hash of name of the reflection as the key and an AssociationReflection as the value.
       #
-      #   Account.reflections # => {"balance" => AggregateReflection}
+      #   Account.reflections # => {balance: => AggregateReflection}
       #
       def reflections
+        normalized_reflections.stringify_keys
+      end
+
+      def normalized_reflections # :nodoc
         @__reflections ||= begin
           ref = {}
 
@@ -84,13 +88,13 @@ def reflections
 
             if parent_reflection
               parent_name = parent_reflection.name
-              ref[parent_name.to_s] = parent_reflection
+              ref[parent_name] = parent_reflection
             else
               ref[name] = reflection
             end
           end
 
-          ref
+          ref.freeze
         end
       end
 
@@ -105,7 +109,7 @@ def reflections
       #   Account.reflect_on_all_associations(:has_many)  # returns an array of all has_many associations
       #
       def reflect_on_all_associations(macro = nil)
-        association_reflections = reflections.values
+        association_reflections = normalized_reflections.values
         association_reflections.select! { |reflection| reflection.macro == macro } if macro
         association_reflections
       end
@@ -116,16 +120,18 @@ def reflect_on_all_associations(macro = nil)
       #   Invoice.reflect_on_association(:line_items).macro  # returns :has_many
       #
       def reflect_on_association(association)
-        reflections[association.to_s]
+        normalized_reflections[association.to_sym]
       end
 
       def _reflect_on_association(association) # :nodoc:
-        _reflections[association.to_s]
+        _reflections[association.to_sym]
       end
 
       # Returns an array of AssociationReflection objects for all associations which have <tt>:autosave</tt> enabled.
       def reflect_on_all_autosave_associations
-        reflections.values.select { |reflection| reflection.options[:autosave] }
+        reflections = normalized_reflections.values
+        reflections.select! { |reflection| reflection.options[:autosave] }
+        reflections
       end
 
       def clear_reflections_cache # :nodoc:
@@ -1159,7 +1165,7 @@ def check_validity!
         end
 
         if parent_reflection.nil?
-          reflections = active_record.reflections.keys.map(&:to_sym)
+          reflections = active_record.normalized_reflections.keys
 
           if reflections.index(through_reflection.name) > reflections.index(name)
             raise HasManyThroughOrderError.new(active_record.name, self, through_reflection)

activerecord/test/cases/associations/eager_test.rb

@@ -1557,10 +1557,15 @@ def test_preloading_has_many_through_with_custom_scope
   end
 
   test "preload with invalid argument" do
-    exception = assert_raises(ActiveRecord::AssociationNotFoundError) do
+    exception = assert_raises(ArgumentError) do
       Author.preload(10).to_a
     end
-    assert_match(/Association named '10' was not found on Author; perhaps you misspelled it\?/, exception.message)
+    assert_match(/Association names must be Symbol or String, got: Integer/, exception.message)
+
+    exception = assert_raises(ActiveRecord::AssociationNotFoundError) do
+      Author.preload(:does_not_exists).to_a
+    end
+    assert_match(/Association named 'does_not_exists' was not found on Author; perhaps you misspelled it\?/, exception.message)
   end
 
   test "associations with extensions are not instance dependent" do

activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb

@@ -928,7 +928,7 @@ def test_redefine_habtm
   end
 
   def test_habtm_with_reflection_using_class_name_and_fixtures
-    assert_not_nil Developer._reflections["shared_computers"]
+    assert_not_nil Developer._reflections[:shared_computers]
     # Checking the fixture for named association is important here, because it's the only way
     # we've been able to reproduce this bug
     assert_not_nil File.read(File.expand_path("../../fixtures/developers.yml", __dir__)).index("shared_computers")

activerecord/test/cases/autosave_association_test.rb

@@ -170,6 +170,7 @@ def test_cyclic_autosaves_do_not_add_multiple_validations
   private
     def assert_no_difference_when_adding_callbacks_twice_for(model, association_name)
       reflection = model.reflect_on_association(association_name)
+      assert_not_nil reflection
       assert_no_difference "callbacks_for_model(#{model.name}).length" do
         model.send(:add_autosave_association_callbacks, reflection)
       end