Merge pull request #21486 from bogdan/refactor-has-many-counter-cache

HasManyAssociation: moved half of counter cache code to reflection

3 files changed, 60 insertions, 70 deletions

diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 19cdd7f470..f8211ef9fb 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -50,7 +50,7 @@ module ActiveRecord
       end
 
       def empty?
-        if has_cached_counter?
+        if reflection.has_cached_counter?
           size.zero?
         else
           super
@@ -73,8 +73,8 @@ module ActiveRecord
         # If the collection is empty the target is set to an empty array and
         # the loaded flag is set to true as well.
         def count_records
-          count = if has_cached_counter?
-            owner._read_attribute cached_counter_attribute_name
+          count = if reflection.has_cached_counter?
+            owner._read_attribute reflection.counter_cache_column
           else
             scope.count
           end
@@ -87,77 +87,26 @@ module ActiveRecord
           [association_scope.limit_value, count].compact.min
         end
 
-
-        # Returns whether a counter cache should be used for this association.
-        #
-        # The counter_cache option must be given on either the owner or inverse
-        # association, and the column must be present on the owner.
-        def has_cached_counter?(reflection = reflection())
-          if reflection.options[:counter_cache] || (inverse = inverse_which_updates_counter_cache(reflection)) && inverse.options[:counter_cache]
-            owner.attribute_present?(cached_counter_attribute_name(reflection))
-          end
-        end
-
-        def cached_counter_attribute_name(reflection = reflection())
-          if reflection.options[:counter_cache]
-            reflection.options[:counter_cache].to_s
-          else
-            "#{reflection.name}_count"
-          end
-        end
-
         def update_counter(difference, reflection = reflection())
           update_counter_in_database(difference, reflection)
           update_counter_in_memory(difference, reflection)
         end
 
         def update_counter_in_database(difference, reflection = reflection())
-          if has_cached_counter?(reflection)
-            counter = cached_counter_attribute_name(reflection)
-            owner.class.update_counters(owner.id, counter => difference)
+          if reflection.has_cached_counter?
+            owner.class.update_counters(owner.id, reflection.counter_cache_column => difference)
           end
         end
 
         def update_counter_in_memory(difference, reflection = reflection())
-          if counter_must_be_updated_by_has_many?(reflection)
-            counter = cached_counter_attribute_name(reflection)
+          if reflection.counter_must_be_updated_by_has_many?
+            counter = reflection.counter_cache_column
+            owner[counter] ||= 0
             owner[counter] += difference
             owner.send(:clear_attribute_changes, counter) # eww
           end
         end
 
-        # This shit is nasty. We need to avoid the following situation:
-        #
-        #   * An associated record is deleted via record.destroy
-        #   * Hence the callbacks run, and they find a belongs_to on the record with a
-        #     :counter_cache options which points back at our owner. So they update the
-        #     counter cache.
-        #   * In which case, we must make sure to *not* update the counter cache, or else
-        #     it will be decremented twice.
-        #
-        # Hence this method.
-        def inverse_which_updates_counter_cache(reflection = reflection())
-          counter_name = cached_counter_attribute_name(reflection)
-          inverse_which_updates_counter_named(counter_name, reflection)
-        end
-        alias inverse_updates_counter_cache? inverse_which_updates_counter_cache
-
-        def inverse_which_updates_counter_named(counter_name, reflection)
-          reflection.klass._reflections.values.find { |inverse_reflection|
-            inverse_reflection.belongs_to? &&
-            inverse_reflection.counter_cache_column == counter_name
-          }
-        end
-
-        def inverse_updates_counter_in_memory?(reflection)
-          inverse = inverse_which_updates_counter_cache(reflection)
-          inverse && inverse == reflection.inverse_of
-        end
-
-        def counter_must_be_updated_by_has_many?(reflection)
-          !inverse_updates_counter_in_memory?(reflection) && has_cached_counter?(reflection)
-        end
-
         def delete_count(method, scope)
           if method == :delete_all
             scope.delete_all
@@ -175,7 +124,7 @@ module ActiveRecord
         def delete_records(records, method)
           if method == :destroy
             records.each(&:destroy!)
-            update_counter(-records.length) unless inverse_updates_counter_cache?
+            update_counter(-records.length) unless reflection.inverse_updates_counter_cache?
           else
             scope = self.scope.where(reflection.klass.primary_key => records)
             update_counter(-delete_count(method, scope))
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index 1aa6a2ca74..deb0f8c9f5 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -110,7 +110,7 @@ module ActiveRecord
         def update_through_counter?(method)
           case method
           when :destroy
-            !inverse_updates_counter_cache?(through_reflection)
+            !through_reflection.inverse_updates_counter_cache?
           when :nullify
             false
           else
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index f8913eba06..2b648978b3 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -99,7 +99,8 @@ module ActiveRecord
       #   @api public
       def reflect_on_all_associations(macro = nil)
         association_reflections = reflections.values
-        macro ? association_reflections.select { |reflection| reflection.macro == macro } : association_reflections
+        association_reflections.select! { |reflection| reflection.macro == macro } if macro
+        association_reflections
       end
 
       # Returns the AssociationReflection object for the +association+ (use the symbol).
@@ -168,6 +169,54 @@ module ActiveRecord
         scope_chain.flatten
       end
 
+      def counter_cache_column
+        if belongs_to?
+          if options[:counter_cache] == true
+            "#{active_record.name.demodulize.underscore.pluralize}_count"
+          elsif options[:counter_cache]
+            options[:counter_cache].to_s
+          end
+        else
+          options[:counter_cache] ? options[:counter_cache].to_s : "#{name}_count"
+        end
+      end
+
+      # This shit is nasty. We need to avoid the following situation:
+      #
+      #   * An associated record is deleted via record.destroy
+      #   * Hence the callbacks run, and they find a belongs_to on the record with a
+      #     :counter_cache options which points back at our owner. So they update the
+      #     counter cache.
+      #   * In which case, we must make sure to *not* update the counter cache, or else
+      #     it will be decremented twice.
+      #
+      # Hence this method.
+      def inverse_which_updates_counter_cache
+        return @inverse_which_updates_counter_cache if defined?(@inverse_which_updates_counter_cache)
+        @inverse_which_updates_counter_cache = klass.reflect_on_all_associations(:belongs_to).find do |inverse|
+          inverse.counter_cache_column == counter_cache_column
+        end
+      end
+      alias inverse_updates_counter_cache? inverse_which_updates_counter_cache
+
+      def inverse_updates_counter_in_memory?
+        inverse_of && inverse_which_updates_counter_cache == inverse_of
+      end
+
+      # Returns whether a counter cache should be used for this association.
+      #
+      # The counter_cache option must be given on either the owner or inverse
+      # association, and the column must be present on the owner.
+      def has_cached_counter?
+        options[:counter_cache] ||
+          inverse_which_updates_counter_cache && inverse_which_updates_counter_cache.options[:counter_cache] &&
+          !!active_record.columns_hash[counter_cache_column]
+      end
+
+      def counter_must_be_updated_by_has_many?
+        !inverse_updates_counter_in_memory? && has_cached_counter?
+      end
+
       def alias_candidate(name)
         "#{plural_name}_#{name}"
       end
@@ -330,14 +379,6 @@ module ActiveRecord
         @active_record_primary_key ||= options[:primary_key] || primary_key(active_record)
       end
 
-      def counter_cache_column
-        if options[:counter_cache] == true
-          "#{active_record.name.demodulize.underscore.pluralize}_count"
-        elsif options[:counter_cache]
-          options[:counter_cache].to_s
-        end
-      end
-
       def check_validity!
         check_validity_of_inverse!
       end