Merge commit 'mislav/counter_cache'

3 files changed, 95 insertions, 63 deletions

diff --git a/activerecord/lib/active_record/counter_cache.rb b/activerecord/lib/active_record/counter_cache.rb
index cbebded995..9601ed6afd 100644
--- a/activerecord/lib/active_record/counter_cache.rb
+++ b/activerecord/lib/active_record/counter_cache.rb
@@ -16,11 +16,15 @@ module ActiveRecord
     def reset_counters(id, *counters)
       object = find(id)
       counters.each do |association|
-        child_class = reflect_on_association(association).klass
-        counter_name = child_class.reflect_on_association(self.name.downcase.to_sym).counter_cache_column
+        child_class = reflect_on_association(association.to_sym).klass
+        belongs_name = self.name.demodulize.underscore.to_sym
+        counter_name = child_class.reflect_on_association(belongs_name).counter_cache_column
 
-        connection.update("UPDATE #{quoted_table_name} SET #{connection.quote_column_name(counter_name)} = #{object.send(association).count} WHERE #{connection.quote_column_name(primary_key)} = #{quote_value(object.id)}", "#{name} UPDATE")
+        self.unscoped.where(arel_table[self.primary_key].eq(object.id)).arel.update({
+          arel_table[counter_name] => object.send(association).count
+        })
       end
+      return true
     end
 
     # A generic "counter updater" implementation, intended primarily to be
@@ -53,19 +57,13 @@ module ActiveRecord
     #   #    SET comment_count = comment_count + 1,
     #   #  WHERE id IN (10, 15)
     def update_counters(id, counters)
-      updates = counters.inject([]) { |list, (counter_name, increment)|
-        sign = increment < 0 ? "-" : "+"
-        list << "#{connection.quote_column_name(counter_name)} = COALESCE(#{connection.quote_column_name(counter_name)}, 0) #{sign} #{increment.abs}"
-      }.join(", ")
-
-      if id.is_a?(Array)
-        ids_list = id.map {|i| quote_value(i)}.join(', ')
-        condition = "IN  (#{ids_list})"
-      else
-        condition = "= #{quote_value(id)}"
+      updates = counters.map do |counter_name, value|
+        operator = value < 0 ? '-' : '+'
+        quoted_column = connection.quote_column_name(counter_name)
+        "#{quoted_column} = COALESCE(#{quoted_column}, 0) #{operator} #{value.abs}"
       end
 
-      update_all(updates, "#{connection.quote_column_name(primary_key)} #{condition}")
+      update_all(updates.join(', '), primary_key => id )
     end
 
     # Increment a number field by one, usually representing a count.
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index 0c7723c0e6..1d883f7ea8 100755
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -708,55 +708,6 @@ class BasicsTest < ActiveRecord::TestCase
     assert Topic.find(2).approved?
   end
 
-  def test_increment_counter
-    Topic.increment_counter("replies_count", 1)
-    assert_equal 2, Topic.find(1).replies_count
-
-    Topic.increment_counter("replies_count", 1)
-    assert_equal 3, Topic.find(1).replies_count
-  end
-
-  def test_decrement_counter
-    Topic.decrement_counter("replies_count", 2)
-    assert_equal(-1, Topic.find(2).replies_count)
-
-    Topic.decrement_counter("replies_count", 2)
-    assert_equal(-2, Topic.find(2).replies_count)
-  end
-
-  def test_reset_counters
-    assert_equal 1, Topic.find(1).replies_count
-
-    Topic.increment_counter("replies_count", 1)
-    assert_equal 2, Topic.find(1).replies_count
-
-    Topic.reset_counters(1, :replies)
-    assert_equal 1, Topic.find(1).replies_count
-  end
-
-  def test_update_counter
-    category = categories(:general)
-    assert_nil category.categorizations_count
-    assert_equal 2, category.categorizations.count
-
-    Category.update_counters(category.id, "categorizations_count" => category.categorizations.count)
-    category.reload
-    assert_not_nil category.categorizations_count
-    assert_equal 2, category.categorizations_count
-
-    Category.update_counters(category.id, "categorizations_count" => category.categorizations.count)
-    category.reload
-    assert_not_nil category.categorizations_count
-    assert_equal 4, category.categorizations_count
-
-    category_2 = categories(:technology)
-    count_1, count_2 = (category.categorizations_count || 0), (category_2.categorizations_count || 0)
-    Category.update_counters([category.id, category_2.id], "categorizations_count" => 2)
-    category.reload; category_2.reload
-    assert_equal count_1 + 2, category.categorizations_count
-    assert_equal count_2 + 2, category_2.categorizations_count
-  end
-
   def test_update_all
     assert_equal Topic.count, Topic.update_all("content = 'bulk updated!'")
     assert_equal "bulk updated!", Topic.find(1).content
diff --git a/activerecord/test/cases/counter_cache_test.rb b/activerecord/test/cases/counter_cache_test.rb
new file mode 100755
index 0000000000..377de168b9
--- /dev/null
+++ b/activerecord/test/cases/counter_cache_test.rb
@@ -0,0 +1,83 @@
+require 'cases/helper'
+require 'models/topic'
+require 'models/reply'
+require 'models/category'
+require 'models/categorization'
+
+class CounterCacheTest < ActiveRecord::TestCase
+  fixtures :topics, :categories, :categorizations
+
+  class SpecialTopic < ::Topic
+    has_many :special_replies, :foreign_key => 'parent_id'
+  end
+
+  class SpecialReply < ::Reply
+    belongs_to :special_topic, :foreign_key => 'parent_id', :counter_cache => 'replies_count'
+  end
+
+  setup do
+    @topic = Topic.find(1)
+  end
+
+  test "increment counter" do
+    assert_difference '@topic.reload.replies_count' do
+      Topic.increment_counter(:replies_count, @topic.id)
+    end
+  end
+
+  test "decrement counter" do
+    assert_difference '@topic.reload.replies_count', -1 do
+      Topic.decrement_counter(:replies_count, @topic.id)
+    end
+  end
+
+  test "reset counters" do
+    # throw the count off by 1
+    Topic.increment_counter(:replies_count, @topic.id)
+
+    # check that it gets reset
+    assert_difference '@topic.reload.replies_count', -1 do
+      Topic.reset_counters(@topic.id, :replies)
+    end
+  end
+  
+  test "reset counters with string argument" do
+    Topic.increment_counter('replies_count', @topic.id)
+
+    assert_difference '@topic.reload.replies_count', -1 do
+      Topic.reset_counters(@topic.id, 'replies')
+    end
+  end
+
+  test "reset counters with modularized and camelized classnames" do
+    special = SpecialTopic.create!(:title => 'Special')
+    SpecialTopic.increment_counter(:replies_count, special.id)
+
+    assert_difference 'special.reload.replies_count', -1 do
+      SpecialTopic.reset_counters(special.id, :special_replies)
+    end
+  end
+
+  test "update counter with initial null value" do
+    category = categories(:general)
+    assert_equal 2, category.categorizations.count
+    assert_nil category.categorizations_count
+
+    Category.update_counters(category.id, :categorizations_count => category.categorizations.count)
+    assert_equal 2, category.reload.categorizations_count
+  end
+
+  test "update counter for decrement" do
+    assert_difference '@topic.reload.replies_count', -3 do
+      Topic.update_counters(@topic.id, :replies_count => -3)
+    end
+  end
+
+  test "update counters of multiple records" do
+    t1, t2 = topics(:first, :second)
+
+    assert_difference ['t1.reload.replies_count', 't2.reload.replies_count'], 2 do
+      Topic.update_counters([t1.id, t2.id], :replies_count => 2)
+    end
+  end
+end