HasManyAssociation: moved half of counter cache code to reflection

Current implementation has a lot of utility methods that accept
reflection call a lot of methods on it and exit.

E.g. has_counter_cache?(reflection)

It causes confusion and inability to cache result of the method even
through it always returns the same result for the same reflection
object.
It can be done easier without access to the association context
by moving code into reflection itself.

e.g. reflection.has_counter_cache?

Reflection is less complex object than association so moving code there
automatically makes it simplier to understand.

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 5360db6a19..0394f89623 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -94,7 +94,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).
@@ -159,6 +160,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
@@ -321,14 +370,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