counter_cache callbacks called twice

[etdsoft]

Gem version: 2.6.1 Rails version: 5.0.1

$ rails new treeapp
# add acts_as_tree to Gemfile and bundle
$ ./bin/rails g scaffold Node label parent_id:integer children_count:integer
$ ./bin/rails db:migrate

The Node class:

class Node < ApplicationRecord
  acts_as_tree counter_cache: true
end

Create the following structure using the browser ( a -> 1 ; a.1 -> 2; a.2 -> 3; b -> 4 ):

root
 |_ a
 |    |_ a.1
 |    |_ a.2
 |_ b

Then through the browser move a.2 under b, browse to /nodes/3/edit update the parent_id to 4 (b node) and submit the form. This is what I get in the log:

Started PATCH "/nodes/3" for ::1 at 2017-02-24 12:55:13 +0100
Processing by NodesController#update as HTML
  Parameters: {"utf8"=>"✓", "authenticity_token"=>"x9Rk", "node"=>{"label"=>"a.2", "parent_id"=>"4", "children_count"=>""}, "commit"=>"Update Node", "id"=>"3"}
  Node Load (0.1ms)  SELECT  "nodes".* FROM "nodes" WHERE "nodes"."id" = ? LIMIT ?  [["id", 3], ["LIMIT", 1]]
   (0.1ms)  begin transaction
  SQL (0.5ms)  UPDATE "nodes" SET "parent_id" = ?, "updated_at" = ? WHERE "nodes"."id" = ?  [["parent_id", 4], ["updated_at", 2017-02-24 11:55:13 UTC], ["id", 3]]
  SQL (0.1ms)  UPDATE "nodes" SET "children_count" = COALESCE("children_count", 0) + 1 WHERE "nodes"."id" = ?  [["id", 4]]
  SQL (0.2ms)  UPDATE "nodes" SET "children_count" = COALESCE("children_count", 0) - 1 WHERE "nodes"."id" = ?  [["id", 1]]
  SQL (0.2ms)  UPDATE "nodes" SET "children_count" = COALESCE("children_count", 0) - 1 WHERE "nodes"."id" = ?  [["id", 1]]
  SQL (0.2ms)  UPDATE "nodes" SET "children_count" = COALESCE("children_count", 0) + 1 WHERE "nodes"."id" = ?  [["id", 4]]
   (2.5ms)  commit transaction
Redirected to http://localhost:3000/nodes/3
Completed 302 Found in 17ms (ActiveRecord: 3.9ms)

It seems like the counter_cache callbacks are being called twice. After this operation :children_count for a is now 0 and children count for b is now 2 when they should be 1 for a and 1 for b.

What am I missing? Thanks!

[felixbuenemann]

Thanks, I can reproduce the issue.

[felixbuenemann]

OK, it looks like the after_update :update_parents_counter_cache is not required on rails 5, I'll have to check with older rails versions as well to develop a proper fix.

[felixbuenemann]

This looks to me to be a rails counter cache bug:

• If I disable the after_update hook, the counts are wrong if I use update_attributes(parent_id: new_parent.id)
• If I don't disable the after_updatehook, the counts are wrong if I use node.parent = new_node; node.save! (this is the bug described here)

[felixbuenemann]

I have created a failing test case for activerecord and will open an upstream bug. There is already a working fix in rails/rails#23357, but it is not yet merged.

[felixbuenemann]

Reported upstream issue rails/rails#28203.

[felixbuenemann]

I'm not sure if there's anything we can do about this problem in acts_as_tree, removing the existing hook would break update, but fix column assignment…

If a fix is merged into rails we could disable our after_update hook with a version check.

I'm open to ideas on how to implement a workaround in acts_as_tree which makes both cases work on current rails versions.

[etdsoft]

Thanks for your help on this @felixbuenemann we're in Rails 5.0.1 and will upgrade to 5.1 as soon as it's out, I'm really not sure what a good workaround would be inside acts_as_tree.

[felixbuenemann]

@etdsoft Maybe you could switch your controller code to use model.update(new_attributes) to work around the issue for now?

[etdsoft]

Unfortunately #update_attributes (which we use) is now just an alias for #update.

And following the steps in the description of this issue (e.g. letting Rails generate the scaffold, the controller gets created using @node.update().

It seems that we'll just have to wait it out...

[frankliu81]

@felixbuenemann ,

1. I am currently seeing with Rails 4.2.7.1, and acts_as_tree 2.7.1, that when updating my model with update_attributes, there is a children_count increment right after assign_attributes(attributes) in active_record

ie.
UPDATE "entities" SET "children_count" = COALESCE("children_count", 0) + 1 WHERE "entities"."id" = $1  [["id", 39]]

And then follow by another increment by update_parents_counter_cache, causing the counter cache value to be updated twice.

Here are the binding.pry in ActiveRecord and ActsAsTree: which are hit:

From: /Users/frank_liu/src/projects/.bundle/ruby/2.3.0/gems/activerecord-4.2.7.1/lib/active_record/persistence.rb @ line 251 ActiveRecord::Persistence#update:
247: def update(attributes)
    248:   # The following transaction covers any possible database side-effects of the
    249:   # attributes assignment. For example, setting the IDs of a child collection.
    250:   with_transaction_returning_status do
 => 251:     binding.pry
    252:     assign_attributes(attributes)
    253:     binding.pry
    254:     save
    255:   end
    256: end
[1] pry(#<Entity>)> exit
D, [2018-07-30T15:24:56.685859 #10300] DEBUG -- :   Entity Load (0.4ms)  SELECT "entities".* FROM "entities" WHERE "entities"."id" IN (40, 41, 52)
D, [2018-07-30T15:24:56.688516 #10300] DEBUG -- :   Entity Load (0.3ms)  SELECT "entities".* FROM "entities" WHERE "entities"."parent_id" = $1  ORDER BY entities.title ASC  [["parent_id", 39]]
D, [2018-07-30T15:24:56.692572 #10300] DEBUG -- :   Account Load (0.3ms)  SELECT  "accounts".* FROM "accounts" WHERE "accounts"."id" = $1 LIMIT 1  [["id", 2]]
D, [2018-07-30T15:24:56.699930 #10300] DEBUG -- :   Entity Load (0.3ms)  SELECT "entities".* FROM "entities" WHERE "entities"."parent_id" = $1  ORDER BY entities.title ASC  [["parent_id", 52]]
D, [2018-07-30T15:24:56.706264 #10300] DEBUG -- :    (0.4ms)  SELECT "entities"."id" FROM "entities" WHERE "entities"."account_id" = 2
D, [2018-07-30T15:24:56.710495 #10300] DEBUG -- :   SQL (0.5ms)  UPDATE "entities" SET "parent_id" = $1, "updated_at" = $2 WHERE "entities"."id" = $3  [["parent_id", 39], ["updated_at", "2018-07-30 22:24:56.702417"], ["id", 52]]
D, [2018-07-30T15:24:56.712130 #10300] DEBUG -- :   SQL (0.3ms)  UPDATE "entities" SET "children_count" = COALESCE("children_count", 0) + 1 WHERE "entities"."id" = $1  [["id", 39]]
From: /Users/frank_liu/src/projects/.bundle/ruby/2.3.0/gems/acts_as_tree-2.7.1/lib/acts_as_tree.rb @ line 370 ActsAsTree::InstanceMethods#update_parents_counter_cache:
368: def update_parents_counter_cache
    369:   counter_cache_column = self.class.children_counter_cache_column
 => 370:   binding.pry
    371:   if parent_id_changed?
    372:     self.class.decrement_counter(counter_cache_column, parent_id_was)
    373:     self.class.increment_counter(counter_cache_column, parent_id)
    374:   end
    375: end

2. Also, it seems like the linked issue in ActiveRecord rails/rails#23357 is just sitting stale now for over a year. Is there anything blocking for the fix?

[felixbuenemann]

Does it work correctly, if you override the update_parents_counter_cache method with an empty method?

def update_parents_counter_cache
end

Or is the decrement not done? In that case you could try:

def update_parents_counter_cache
  counter_cache_column = self.class.children_counter_cache_column
  if parent_id_changed?
    self.class.decrement_counter(counter_cache_column, parent_id_was)
    # self.class.increment_counter(counter_cache_column, parent_id)
  end
end

I'm no longer trying to contribute fixes to rails, it's just too painful.

[frankliu81]

@felixbuenemann

With Rails 4.2.7.1, and acts_as_tree 2.7.1, it does seem to work correctly if I override the update_parents_counter_cache method with an empty method.

I tried out,

1. Creating a child node with the parent_id assigned on creation. ie.

child_node = Node.new
child_node.title = "Child Node"
child_node.parent_id = parent_node.id
child_node.save

In this case, the update_parents_counter_cache method is not called, but the count increments by 1. When the child node is deleted, the count is decremented by 1.

2. Creating a child node first. Then update the parent_id

child_node = Node.new
child_node.title = "Child Node"
child_node.save
child_node.parent_id = parent_node.id
child_node.save

In this case, the update_parents_counter_cache method is called, if I override it with an empty method, then the count increments by 1. When the child node is deleted, the count is decremented by 1.