Correctly update counter caches on deletion for has_many :through [#2824 state:resolved]. Also fixed a bunch of other counter cache bugs in the process, as once I fixed this one others started appearing like nobody's business.

2 files changed, 63 insertions, 20 deletions

diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index aff955dd5f..732f63713e 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -45,30 +45,54 @@ module ActiveRecord
           [@reflection.options[:limit], count].compact.min
         end
 
-        def has_cached_counter?
-          @owner.attribute_present?(cached_counter_attribute_name)
+        def has_cached_counter?(reflection = @reflection)
+          @owner.attribute_present?(cached_counter_attribute_name(reflection))
         end
 
-        def cached_counter_attribute_name
-          "#{@reflection.name}_count"
+        def cached_counter_attribute_name(reflection = @reflection)
+          "#{reflection.name}_count"
+        end
+
+        def update_counter(difference, reflection = @reflection)
+          if has_cached_counter?(reflection)
+            counter = cached_counter_attribute_name(reflection)
+            @owner.class.update_counters(@owner.id, counter => difference)
+            @owner[counter] += difference
+            @owner.changed_attributes.delete(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_updates_counter_cache?(reflection = @reflection)
+          counter_name = cached_counter_attribute_name(reflection)
+          reflection.klass.reflect_on_all_associations(:belongs_to).any? { |inverse_reflection|
+            inverse_reflection.counter_cache_column == counter_name
+          }
         end
 
         # Deletes the records according to the <tt>:dependent</tt> option.
         def delete_records(records, method = @reflection.options[:dependent])
-          case method
-          when :destroy
+          if method == :destroy
             records.each { |r| r.destroy }
-          when :delete_all
-            @reflection.klass.delete(records.map { |r| r.id })
+            update_counter(-records.length) unless inverse_updates_counter_cache?
           else
-            updates    = { @reflection.foreign_key => nil }
-            conditions = { @reflection.association_primary_key => records.map { |r| r.id } }
-
-            scoped.where(conditions).update_all(updates)
-          end
+            keys  = records.map { |r| r[@reflection.association_primary_key] }
+            scope = scoped.where(@reflection.association_primary_key => keys)
 
-          if has_cached_counter? && method != :destroy
-            @owner.class.update_counters(@owner.id, cached_counter_attribute_name => -records.size)
+            if method == :delete_all
+              update_counter(-scope.delete_all)
+            else
+              update_counter(-scope.update_all(@reflection.foreign_key => nil))
+            end
           end
         end
 
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 c98ac79dc0..040de7e98f 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -31,6 +31,9 @@ module ActiveRecord
 
           through_association = @owner.send(@reflection.through_reflection.name)
           through_association.create!(construct_join_attributes(record))
+
+          update_counter(1)
+          true
         end
 
       private
@@ -43,19 +46,35 @@ module ActiveRecord
           end
         end
 
-        def deletion_scope(records)
-          @owner.send(@reflection.through_reflection.name).where(construct_join_attributes(*records))
+        def update_through_counter?(method)
+          case method
+          when :destroy
+            !inverse_updates_counter_cache?(@reflection.through_reflection)
+          when :nullify
+            false
+          else
+            true
+          end
         end
 
         def delete_records(records, method = @reflection.options[:dependent])
+          through = @owner.send(:association_proxy, @reflection.through_reflection.name)
+          scope   = through.scoped.where(construct_join_attributes(*records))
+
           case method
           when :destroy
-            deletion_scope(records).destroy_all
+            count = scope.destroy_all.length
           when :nullify
-            deletion_scope(records).update_all(@reflection.source_reflection.foreign_key => nil)
+            count = scope.update_all(@reflection.source_reflection.foreign_key => nil)
           else
-            deletion_scope(records).delete_all
+            count = scope.delete_all
           end
+
+          if @reflection.through_reflection.macro == :has_many && update_through_counter?(method)
+            update_counter(-count, @reflection.through_reflection)
+          end
+
+          update_counter(-count)
         end
 
         def find_target