Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

update behavior of counter_cache #9236

Closed
wants to merge 2 commits into from

5 participants

@jasl

I update behavior of counter_cache, the reason has described at #8997.

@jasl

@rafaelfranca sorry to trouble you again, I fix this but changed it's behavior, please check.
add_counter_cache_callbacks seems ugly but I didn't have another better way, also I tried to make update counter after destroy but failed cause ActiveModel::Dirty doesn't support.

@jasl

@vad4msiu seems no one attention this

activerecord/lib/active_record/locking/optimistic.rb
@@ -159,7 +159,7 @@ def reset_locking_column
# Make sure the lock version column gets updated when counters are
# updated.
- def update_counters(id, counters)
+ def update_counters(id, specifiey_primary_key = nil, counters)
@parndt
parndt added a note

This is supposed to be specified_primary_key.

@jasl
jasl added a note

@parndt fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robin850 robin850 added this to the 4.2.0 milestone
@jeremy
Owner
@jeremy jeremy closed this
@sadfuzzy

Letter 'd' instead of 'y' - specified_primary_key

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 10, 2013
  1. @jasl

    update behavior of counter_cache

    jasl authored
Commits on Apr 11, 2013
  1. @jasl

    fix wrong spell

    jasl authored
This page is out of date. Refresh to see the latest.
View
6 activerecord/CHANGELOG.md
@@ -1,5 +1,11 @@
## Rails 4.0.0 (unreleased) ##
+* Update behavior of `counter_cache`, the counter will update after saving instead of assigning,
+ support update counters by assign `{foreign_key}_id`.
+ Avoiding resource assigned but not be saved but the counter has updated.
+
+ *Jasl*
+
* Raise ArgumentError instead of generate invalid SQL when empty hash is used in where clause value
Roberto Miranda
View
32 activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -10,7 +10,6 @@ def handle_dependency
def replace(record)
raise_on_type_mismatch(record) if record
- update_counters(record)
replace_keys(record)
set_inverse_instance(record)
@@ -34,29 +33,6 @@ def find_target?
!loaded? && foreign_key_present? && klass
end
- def update_counters(record)
- counter_cache_name = reflection.counter_cache_column
-
- if counter_cache_name && owner.persisted? && different_target?(record)
- if record
- record.class.increment_counter(counter_cache_name, record.id)
- end
-
- if foreign_key_present?
- klass.decrement_counter(counter_cache_name, target_id)
- end
- end
- end
-
- # Checks whether record is different to the current target, without loading it
- def different_target?(record)
- if record.nil?
- owner[reflection.foreign_key]
- else
- record.id != owner[reflection.foreign_key]
- end
- end
-
def replace_keys(record)
if record
owner[reflection.foreign_key] = record[reflection.association_primary_key(record.class)]
@@ -76,14 +52,6 @@ def invertible_for?(record)
inverse && inverse.macro == :has_one
end
- def target_id
- if options[:primary_key]
- owner.send(reflection.name).try(:id)
- else
- owner[reflection.foreign_key]
- end
- end
-
def stale_state
owner[reflection.foreign_key] && owner[reflection.foreign_key].to_s
end
View
70 activerecord/lib/active_record/associations/builder/belongs_to.rb
@@ -23,12 +23,12 @@ def add_counter_cache_callbacks(reflection)
cache_column = reflection.counter_cache_column
mixin.class_eval <<-CODE, __FILE__, __LINE__ + 1
- def belongs_to_counter_cache_after_create_for_#{name}
+ def belongs_to_counter_cache_after_create_for_#{reflection.foreign_key}
record = #{name}
record.class.increment_counter(:#{cache_column}, record.id) unless record.nil?
end
- def belongs_to_counter_cache_before_destroy_for_#{name}
+ def belongs_to_counter_cache_before_destroy_for_#{reflection.foreign_key}
unless marked_for_destruction?
record = #{name}
record.class.decrement_counter(:#{cache_column}, record.id) unless record.nil?
@@ -36,8 +36,62 @@ def belongs_to_counter_cache_before_destroy_for_#{name}
end
CODE
- model.after_create "belongs_to_counter_cache_after_create_for_#{name}"
- model.before_destroy "belongs_to_counter_cache_before_destroy_for_#{name}"
+ if options[:polymorphic]
+ mixin.class_eval <<-CODE, __FILE__, __LINE__ + 1
+ def belongs_to_counter_cache_after_update_for_#{reflection.foreign_key}
+ type_change = #{reflection.foreign_type}_change
+ value_change = #{reflection.foreign_key}_change
+
+ return if type_change.nil? && value_change.nil?
+ if type_change.nil? && value_change
+ klass = #{reflection.foreign_type}.safe_constantize
+ klass.decrement_counter(:#{cache_column}, value_change[0]) unless value_change[0].nil?
+ klass.increment_counter(:#{cache_column}, value_change[1]) unless value_change[1].nil?
+ elsif type_change and value_change
+ return if type_change[0].nil? && value_change[0].nil?
+ unless type_change[0].nil? || value_change[0].nil?
+ type_change[0].safe_constantize.decrement_counter(:#{cache_column}, value_change[0])
+ end
+ unless type_change[1].nil? || value_change[1].nil?
+ type_change[1].safe_constantize.increment_counter(:#{cache_column}, value_change[1])
+ end
+ else
+ unless type_change[0].nil?
+ type_change[0].safe_constantize.decrement_counter(:#{cache_column}, #{reflection.foreign_key})
+ end
+ unless type_change[1].nil?
+ type_change[1].safe_constantize.increment_counter(:#{cache_column}, #{reflection.foreign_key})
+ end
+ end
+ end
+ CODE
+ elsif options[:primary_key]
+ mixin.class_eval <<-CODE, __FILE__, __LINE__ + 1
+ def belongs_to_counter_cache_after_update_for_#{reflection.foreign_key}
+ if change = #{reflection.foreign_key}_change
+ unless change[0].nil?
+ #{reflection.class_name}.decrement_counter(:#{cache_column}, change[0], "#{ options[:primary_key].to_s }")
+ end
+ unless change[1].nil?
+ #{reflection.class_name}.increment_counter(:#{cache_column}, change[1], "#{ options[:primary_key].to_s }")
+ end
+ end
+ end
+ CODE
+ else
+ mixin.class_eval <<-CODE, __FILE__, __LINE__ + 1
+ def belongs_to_counter_cache_after_update_for_#{reflection.foreign_key}
+ if change = #{reflection.foreign_key}_change
+ #{reflection.class_name}.decrement_counter(:#{cache_column}, change[0]) unless change[0].nil?
+ #{reflection.class_name}.increment_counter(:#{cache_column}, change[1]) unless change[1].nil?
+ end
+ end
+ CODE
+ end
+
+ model.after_create "belongs_to_counter_cache_after_create_for_#{reflection.foreign_key}"
+ model.before_destroy "belongs_to_counter_cache_before_destroy_for_#{reflection.foreign_key}"
+ model.after_update "belongs_to_counter_cache_after_update_for_#{reflection.foreign_key}"
klass = reflection.class_name.safe_constantize
klass.attr_readonly cache_column if klass && klass.respond_to?(:attr_readonly)
@@ -45,7 +99,7 @@ def belongs_to_counter_cache_before_destroy_for_#{name}
def add_touch_callbacks(reflection)
mixin.class_eval <<-CODE, __FILE__, __LINE__ + 1
- def belongs_to_touch_after_save_or_destroy_for_#{name}
+ def belongs_to_touch_after_save_or_destroy_for_#{reflection.foreign_key}
record = #{name}
unless record.nil?
@@ -54,9 +108,9 @@ def belongs_to_touch_after_save_or_destroy_for_#{name}
end
CODE
- model.after_save "belongs_to_touch_after_save_or_destroy_for_#{name}"
- model.after_touch "belongs_to_touch_after_save_or_destroy_for_#{name}"
- model.after_destroy "belongs_to_touch_after_save_or_destroy_for_#{name}"
+ model.after_save "belongs_to_touch_after_save_or_destroy_for_#{reflection.foreign_key}"
+ model.after_touch "belongs_to_touch_after_save_or_destroy_for_#{reflection.foreign_key}"
+ model.after_destroy "belongs_to_touch_after_save_or_destroy_for_#{reflection.foreign_key}"
end
def valid_dependent_options
View
21 activerecord/lib/active_record/counter_cache.rb
@@ -51,7 +51,8 @@ def reset_counters(id, *counters)
# * +id+ - The id of the object you wish to update a counter on or an Array of ids.
# * +counters+ - An Array of Hashes containing the names of the fields
# to update as keys and the amount to update the field by as values.
- #
+ # * +specified_primary_key+ - Specify primary key column name, this is optional and
+ # default is table primary column.
# ==== Examples
#
# # For the Post with id of 5, decrement the comment_count by 1, and
@@ -69,14 +70,14 @@ def reset_counters(id, *counters)
# # UPDATE posts
# # SET comment_count = COALESCE(comment_count, 0) + 1
# # WHERE id IN (10, 15)
- def update_counters(id, counters)
+ def update_counters(id, specified_primary_key = nil, counters)
+ pk = specified_primary_key || primary_key
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
-
- where(primary_key => id).update_all updates.join(', ')
+ where(pk => id).update_all updates.join(', ')
end
# Increment a numeric field by one, via a direct SQL update.
@@ -90,13 +91,15 @@ def update_counters(id, counters)
#
# * +counter_name+ - The name of the field that should be incremented.
# * +id+ - The id of the object that should be incremented or an Array of ids.
+ # * +specified_primary_key+ - Specify primary key column name, this is optional and
+ # default is table primary column.
#
# ==== Examples
#
# # Increment the post_count column for the record with an id of 5
# DiscussionBoard.increment_counter(:post_count, 5)
- def increment_counter(counter_name, id)
- update_counters(id, counter_name => 1)
+ def increment_counter(counter_name, id, specified_primary_key = nil)
+ update_counters(id, specified_primary_key, counter_name => 1)
end
# Decrement a numeric field by one, via a direct SQL update.
@@ -108,13 +111,15 @@ def increment_counter(counter_name, id)
#
# * +counter_name+ - The name of the field that should be decremented.
# * +id+ - The id of the object that should be decremented or an Array of ids.
+ # * +specified_primary_key+ - Specify primary key column name, this is optional and
+ # default is table primary column.
#
# ==== Examples
#
# # Decrement the post_count column for the record with an id of 5
# DiscussionBoard.decrement_counter(:post_count, 5)
- def decrement_counter(counter_name, id)
- update_counters(id, counter_name => -1)
+ def decrement_counter(counter_name, id, specified_primary_key = nil)
+ update_counters(id, specified_primary_key, counter_name => -1)
end
end
end
View
2  activerecord/lib/active_record/locking/optimistic.rb
@@ -159,7 +159,7 @@ def reset_locking_column
# Make sure the lock version column gets updated when counters are
# updated.
- def update_counters(id, counters)
+ def update_counters(id, specified_primary_key = nil, counters)
counters = counters.merge(locking_column => 1) if locking_enabled?
super
end
View
95 activerecord/test/cases/associations/belongs_to_associations_test.rb
@@ -240,7 +240,13 @@ def test_belongs_to_counter_with_assigning_nil
c.post = nil
- assert_equal 1, Post.find(p.id).comments.size
+ assert_equal 2, Post.find(p.id).comments.size
+
+ assert_raise ActiveRecord::StatementInvalid do
+ c.save
+ end
+
+ assert_equal 2, Post.find(p.id).comments.size
end
def test_belongs_to_with_primary_key_counter
@@ -253,12 +259,22 @@ def test_belongs_to_with_primary_key_counter
reply.topic_with_primary_key = debate2
+ assert_equal 1, debate.reload.replies_count
+ assert_equal 0, debate2.reload.replies_count
+
+ reply.save!
+
assert_equal 0, debate.reload.replies_count
assert_equal 1, debate2.reload.replies_count
reply.topic_with_primary_key = nil
assert_equal 0, debate.reload.replies_count
+ assert_equal 1, debate2.reload.replies_count
+
+ reply.save!
+
+ assert_equal 0, debate.reload.replies_count
assert_equal 0, debate2.reload.replies_count
end
@@ -267,8 +283,8 @@ def test_belongs_to_counter_with_reassigning
t2 = Topic.create("title" => "t2")
r1 = Reply.new("title" => "r1", "content" => "r1")
r1.topic = t1
+ r1.save!
- assert r1.save
assert_equal 1, Topic.find(t1.id).replies.size
assert_equal 0, Topic.find(t2.id).replies.size
@@ -278,17 +294,70 @@ def test_belongs_to_counter_with_reassigning
r1.topic = t2
end
- assert r1.save
+ r1.save!
+
assert_equal 0, Topic.find(t1.id).replies.size
assert_equal 1, Topic.find(t2.id).replies.size
r1.topic = nil
assert_equal 0, Topic.find(t1.id).replies.size
+ assert_equal 1, Topic.find(t2.id).replies.size
+
+ r1.save!
+
+ assert_equal 0, Topic.find(t1.id).replies.size
assert_equal 0, Topic.find(t2.id).replies.size
r1.topic = t1
+ assert_equal 0, Topic.find(t1.id).replies.size
+ assert_equal 0, Topic.find(t2.id).replies.size
+
+ r1.save!
+
+ assert_equal 1, Topic.find(t1.id).replies.size
+ assert_equal 0, Topic.find(t2.id).replies.size
+
+ r1.destroy
+
+ assert_equal 0, Topic.find(t1.id).replies.size
+ assert_equal 0, Topic.find(t2.id).replies.size
+ end
+
+ def test_belongs_to_counter_with_reassigning_using_foreign_key
+ t1 = Topic.create("title" => "t1")
+ t2 = Topic.create("title" => "t2")
+ r1 = Reply.new("title" => "r1", "content" => "r1")
+ r1.parent_id = t1.id
+ r1.save
+
+ assert_equal 1, Topic.find(t1.id).replies.size
+ assert_equal 0, Topic.find(t2.id).replies.size
+
+ r1.parent_id = t2.id
+ r1.save
+
+ assert_equal 0, Topic.find(t1.id).replies.size
+ assert_equal 1, Topic.find(t2.id).replies.size
+
+ r1.parent_id = nil
+
+ assert_equal 0, Topic.find(t1.id).replies.size
+ assert_equal 1, Topic.find(t2.id).replies.size
+
+ r1.save
+
+ assert_equal 0, Topic.find(t1.id).replies.size
+ assert_equal 0, Topic.find(t2.id).replies.size
+
+ r1.parent_id = t1.id
+
+ assert_equal 0, Topic.find(t1.id).replies.size
+ assert_equal 0, Topic.find(t2.id).replies.size
+
+ r1.save
+
assert_equal 1, Topic.find(t1.id).replies.size
assert_equal 0, Topic.find(t2.id).replies.size
@@ -303,14 +372,14 @@ def test_belongs_to_reassign_with_namespaced_models_and_counters
t2 = Web::Topic.create("title" => "t2")
r1 = Web::Reply.new("title" => "r1", "content" => "r1")
r1.topic = t1
+ r1.save!
- assert r1.save
assert_equal 1, Web::Topic.find(t1.id).replies.size
assert_equal 0, Web::Topic.find(t2.id).replies.size
r1.topic = Web::Topic.find(t2.id)
+ r1.save!
- assert r1.save
assert_equal 0, Web::Topic.find(t1.id).replies.size
assert_equal 1, Web::Topic.find(t2.id).replies.size
end
@@ -407,6 +476,11 @@ def test_counter_cache
reply = Reply.create(:title => "re: zoom", :content => "speedy quick!")
reply.topic = topic
+ assert_equal 0, topic.reload[:replies_count]
+ assert_equal 0, topic.replies.size
+
+ reply.save!
+
assert_equal 1, topic.reload[:replies_count]
assert_equal 1, topic.replies.size
@@ -421,6 +495,9 @@ def test_custom_counter_cache
silly = SillyReply.create(:title => "gaga", :content => "boo-boo")
silly.reply = reply
+ assert_equal 0, reply.reload[:replies_count]
+ assert_equal 0, reply.replies.size
+ silly.save
assert_equal 1, reply.reload[:replies_count]
assert_equal 1, reply.replies.size
@@ -648,9 +725,15 @@ def test_polymorphic_counter_cache
post = posts(:welcome)
comment = comments(:greetings)
+ assert_difference lambda { post.reload.taggings_count }, -0 do
+ assert_difference 'comment.reload.taggings_count', +0 do
+ tagging.taggable = comment
+ end
+ end
+
assert_difference lambda { post.reload.taggings_count }, -1 do
assert_difference 'comment.reload.taggings_count', +1 do
- tagging.taggable = comment
+ tagging.save!
end
end
end
View
14 activerecord/test/cases/counter_cache_test.rb
@@ -30,17 +30,29 @@ class ::SpecialReply < ::Reply
end
test "increment counter" do
- assert_difference '@topic.reload.replies_count' do
+ assert_difference '@topic.reload.replies_count', +1 do
Topic.increment_counter(:replies_count, @topic.id)
end
end
+ test "increment counter using another primary key" do
+ assert_difference '@topic.reload.replies_count', +1 do
+ Topic.increment_counter(:replies_count, @topic.title, "title")
+ end
+ end
+
test "decrement counter" do
assert_difference '@topic.reload.replies_count', -1 do
Topic.decrement_counter(:replies_count, @topic.id)
end
end
+ test "decrement counter using another primary key" do
+ assert_difference '@topic.reload.replies_count', -1 do
+ Topic.decrement_counter(:replies_count, @topic.title, "title")
+ end
+ end
+
test "reset counters" do
# throw the count off by 1
Topic.increment_counter(:replies_count, @topic.id)
Something went wrong with that request. Please try again.