design intent of counter_cache? #8997

Closed
jasl opened this Issue Jan 19, 2013 · 9 comments

Projects

None yet

8 participants

@jasl
[1] pry(main)> Node.last.posts
  Node Load (0.2ms)  SELECT `nodes`.* FROM `nodes` ORDER BY `nodes`.`id` DESC LIMIT 1
  Post Load (0.4ms)  SELECT `posts`.* FROM `posts` WHERE `posts`.`node_id` = 5
=> []
[2] pry(main)> Post.first.node = Node.last
  Node Load (0.1ms)  SELECT `nodes`.* FROM `nodes` ORDER BY `nodes`.`id` DESC LIMIT 1
  SQL (0.3ms)  UPDATE `nodes` SET `posts_count` = COALESCE(`posts_count`, 0) + 1 WHERE `nodes`.`id` = 5
  SQL (0.3ms)  UPDATE `nodes` SET `posts_count` = COALESCE(`posts_count`, 0) - 1 WHERE `nodes`.`id` = 1
=> #<Node id: 5, name: "ttt", cover: nil, description: nil, created_at: "2013-01-18 21:34:14", updated_at: "2013-01-18 22:15:41", posts_count: 0, state: "publish">
[3] pry(main)> Post.first.node
=> #<Node id: 1, name: "System", cover: nil, description: "", created_at: "2012-12-07 18:29:00", updated_at: "2012-12-07 18:40:03", posts_count: 4, state: "system">
[4] pry(main)> Node.last.posts_count
  Node Load (0.4ms)  SELECT `nodes`.* FROM `nodes` ORDER BY `nodes`.`id` DESC LIMIT 1
=> 1
[5] pry(main)> Node.last.posts
  Node Load (0.4ms)  SELECT `nodes`.* FROM `nodes` ORDER BY `nodes`.`id` DESC LIMIT 1
  Post Load (0.3ms)  SELECT `posts`.* FROM `posts` WHERE `posts`.`node_id` = 5
=> []
[6] pry(main)> Node.last.posts_count
  Node Load (0.4ms)  SELECT `nodes`.* FROM `nodes` ORDER BY `nodes`.`id` DESC LIMIT 1
=> 1

updating counter occur in assigning, but the resource maybe not save, that will made counter incorrectly, why don't update counter after resource update?

update
lib/active_record/associations/builder/belongs_to.rb

method_name = "belongs_to_counter_cache_after_create_for_#{name}"
mixin.redefine_method(method_name) do
  record = send(name)
  record.class.increment_counter(cache_column, record.id) unless record.nil?
end
model.after_create(method_name)

method_name = "belongs_to_counter_cache_before_destroy_for_#{name}"
mixin.redefine_method(method_name) do
  record = send(name)
  record.class.decrement_counter(cache_column, record.id) unless record.nil?
end
model.before_destroy(method_name)

we can learn that AR will update counter at resource create and destroy, there is a question: why before destroy not after destroy? although there has transaction to ensure consistency but 'after' is more reasonable.

def replace(record)
  raise_on_type_mismatch(record) if record

  update_counters(record)
  replace_keys(record)
  set_inverse_instance(record)

  @updated = true if record

  self.target = record
end

this will affect on assigning, it will update counter immediately, but has potential problem as I said above.

I think AR should ensure final consistency, so update counter after persistence.

@vad4msiu

+1
I too was surprised when I saw such behavior.

@jasl

@rafaelfranca

is this a bug or design?

@rafaelfranca
Ruby on Rails member

This seems a bug to me

@JonRowe

Has anyone checked out the PR @jasl submitted to solve this?

@mountriv99

+1

The counter_cache is incorrectly updated twice if the association was assigned directly

Example:

class Student < ActiveRecord::Base
  belongs_to :klass, :counter_cache => true
end

class Klass < ActiveRecord::Base
  has_many :students
end

When I tried to assign a klass to a student:

irb(main):035:0> klass = Klass.first
    Klass Load (1.0ms)  SELECT "klasses".* FROM "klasses" ORDER BY "klasses"."id" ASC LIMIT 1
=> #<Klass id: 1, school_id: 1, name: {"en"=>"1A"}, teachers_count: 0, students_count: 0, created_at: "2013-05-19 08:03:31", updated_at: "2013-05-19 08:03:31">
irb(main):036:0> student = Student.first
    Student Load (1.1ms)  SELECT "students".* FROM "students" ORDER BY "students"."id" ASC LIMIT 1
=> #<Student id: 1, school_id: 1, klass_id: nil, number: "1", first_name: {"en"=>"Tony"}, last_name: {"en"=>"Stark"}, gender: "M", dob: "1990-01-01", avatar: nil, password_digest: "$2a$10$nuB7htsWSfNCIDpgJoaXcesntXSo.ST2Db00doggDkQF...", parent_code: "0DA0E1", parents_count: 0, status: 1, created_at: "2013-05-19 08:02:37", updated_at: "2013-05-19 08:45:21">
irb(main):037:0> student.klass = klass
    SQL (1.7ms)  UPDATE "klasses" SET "students_count" = COALESCE("students_count", 0) + 1 WHERE "klasses"."id" = 1
=> #<Klass id: 1, school_id: 1, name: {"en"=>"1A"}, teachers_count: 0, students_count: 0, created_at: "2013-05-19 08:03:31", updated_at: "2013-05-19 08:03:31">
irb(main):038:0> student.save
     (0.3ms)  BEGIN
    SQL (1.3ms)  UPDATE "students" SET "klass_id" = $1, "updated_at" = $2, "first_name" = $3, "last_name" = $4 WHERE "students"."id" = 1  [["klass_id", 1], ["updated_at", Sun, 19 May 2013 08:46:22 UTC +00:00], ["first_name", "\"en\"=>\"Tony\""], ["last_name", "\"en\"=>\"Stark\""]]
    SQL (0.8ms)  UPDATE "klasses" SET "students_count" = COALESCE("students_count", 0) + 1 WHERE "klasses"."id" = 1
     (0.9ms)  COMMIT
=> true

As you can see, there are 2 identical SQL UPDATE statements that updated the counter cache, when only the latter one (after calling save) was expected

@arthurnn
Ruby on Rails member

I am pretty much sure this is a dup of #13304 , Should we close one of them if so?

@rails-bot rails-bot added the stale label Aug 19, 2014
@rails-bot

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@RobinClowers

Looks like this is being tracked here: #14849, so this can probably be closed?

@rafaelfranca
Ruby on Rails member

👍 Thank you for reporting back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment