When using both touch and counter_cache on a belongs_to association, updated_at on the associated object instance is not updated

[yan-hoose]

trafficstars

Hi everyone! I ran into an issue with a counter cached belongs_to association.

In short, when I have an association with only touch: true, it works as expected:

class Comment < ActiveRecord::Base
  belongs_to :post, touch: true
end
# updated_at is updated on the Post instance after doing this
post.comments.create!

But when I add counter_cache: true to the same association, it stops working as expected:

class Comment < ActiveRecord::Base
  belongs_to :post, touch: true, counter_cache: true
end
# updated_at is NOT updated on the Post instance after doing this (it has been updated in the DB however)
post.comments.create!

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  #gem "rails", "6.0.4.6"
  #gem "rails", "6.1.4.6"
  #gem "rails", "7.0.2.2"
  gem "rails", github: "rails/rails", branch: "main"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.integer :comments_count, null: false, default: 0
    t.timestamps
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  # works as expected
  #belongs_to :post, touch: true

  # does not work as expected
  belongs_to :post, touch: true, counter_cache: true
end

class BugTest < Minitest::Test
  def test_post_instance_updated
    post = Post.create!
    previous_updated_at = post.updated_at.to_f.to_s

    post.comments.create!

    # expecting post.updated_at to be different from the previous value
    # after creating a new comment on this post
    assert previous_updated_at != post.updated_at.to_f.to_s
  end
end

Expected behavior

Updated updated_at should be reflected on the Post instance in addition to being updated in the database.

Actual behavior

Updated updated_at is not reflected on the Post instance. It is however correctly updated in the database.

System configuration

Rails version:

• 6.0.4.6
• 6.1.4.6
• 7.0.2.2
• main

Ruby version: 2.7.5

[mansakondo]

This issue seems to be caused from the fact that when touch: true and counter_cache: true are used together, this method is called: https://github.com/rails/rails/blob/c21c4139e1ceae0289a7ae2cd597b4705a24a71b/activerecord/lib/active_record/associations/belongs_to_association.rb#L98 From which, an update_all query is performed instead of a regular update.

D, [2022-02-20T01:12:00.789588 #30720] DEBUG -- :   Post Update All (0.1ms)  UPDATE "posts" SET "comments_count" = COALESCE("comments_count", 0) + ?, "updated_at" = ? WHERE "posts"."id" = ?  [["comments_count", 1], ["updated_at", "2022-02-20 00:12:00.789206"], ["id", 1]]

As update_all will ignore callbacks, updated_at will only be updated in the database but not in memory.

[mansakondo]

If think there's something wrong with this test since it passes though updated_at isn't updated:

# Running:

....
Last: 2022-02-20 17:33:24 UTC
Current: 2022-02-20 17:33:24 UTC
.................................................

Finished in 5.473564s, 9.6829 runs/s, 41.2894 assertions/s.
53 runs, 226 assertions, 0 failures, 0 errors, 0 skips

[nvasilevski]

If think there's something wrong with this test since it passes though updated_at isn't updated:

I haven't tried it myself yet, but at first glance it seems like you should be able to make it fail by removing reload from here: https://github.com/rails/rails/blob/c01a744674d2edc4a35cc8f5b4cab46a03dbb454/activerecord/test/cases/locking_test.rb#L499 or moving the updated_at assertion above the reload

Essentially reload loads the updated updated_at value

[mansakondo]

If think there's something wrong with this test since it passes though updated_at isn't updated:

I haven't tried it myself yet, but at first glance it seems like you should be able to make it fail by removing reload from here:

https://github.com/rails/rails/blob/c01a744674d2edc4a35cc8f5b4cab46a03dbb454/activerecord/test/cases/locking_test.rb#L499

or moving the updated_at assertion above the reload Essentially reload loads the updated updated_at value

Nice catch!

[mansakondo]

At this point, I don't know if it's actually an issue that needs to be fixed, or if it's just that reload should be called. In which case, maybe that this behavior should be documented.

[nickborromeo]

I haven't tried it myself yet, but at first glance it seems like you should be able to make it fail by removing reload from here:

agreed. Removing the reload essentially is what the repro script does.

As update_all will ignore callbacks, updated_at will only be updated in the database but not in memory.

this is also true, and I believe this behavior is documented in https://github.com/rails/rails/blob/c21c4139e1ceae0289a7ae2cd597b4705a24a71b/activerecord/lib/active_record/persistence.rb#L861-L874

In which case, maybe that this behavior should be documented.

I would tend to say 👍 to this, but I am not sure where exactly this should be documented 🤔

[mansakondo]

I think that we should maybe document it here: https://github.com/rails/rails/blob/7b1f65e8f56738aaa8dd15f19d3ac5f4385ff229/activerecord/lib/active_record/associations.rb#L1750 Saying that calling reload is necessary to load the new updated_at value, when :touch is used with :counter_cache.

[nickborromeo]

yup, I think it should be in that area as well, but what can be confusing is that this behavior only happens when the two options are used together, however this page documents each of the options individually. Lets say that that alright, then maybe it should be in under counter_cache instead since the path it takes it through the counter_cache option with touch being passed in.

[mansakondo]

I'm not sure if I get that part:

Lets say that that alright, then maybe it should be in under counter_cache instead since the path it takes it through the counter_cache option with touch being passed in.

I was thinking that since only updated_at is affected, then it would more relevant to document it under :touch rather than :counter_cache.

[nickborromeo]

I could be wrong in how I traced this, so please feel free to correct me, but when we have the counter_cache option set on the belongs_to relationship, we will eventually hit this method

https://github.com/rails/rails/blob/746d115b9ada2f037686b81cb63286429138e503/activerecord/lib/active_record/associations/belongs_to_association.rb#L98-L102

When we call the increment! method we take the value of the touch option that is passed in the same belongs_to relation; which will run the touch callbacks

https://github.com/rails/rails/blob/746d115b9ada2f037686b81cb63286429138e503/activerecord/lib/active_record/callbacks.rb#L451-L453

This is why I think any documentation changes that we might make should be bundled together with counter_cache since this behavior seems to happen when touch is set while also setting counter_cache and not the other way around (semantically)

[mansakondo]

Ah I get it now. Where and how would you document this ?

[nickborromeo]

I am still not sure 😅 if you want to take a stab at it, I am sure the folks on the team will be able to give feedback if we really need to document this or not

[mansakondo]

Yeah you're right.

[alexanderholder]

Just got caught on this myself, first place I looked was the same place as this comment. I think it does make sense to be with touch as it appears that touch doesn't work and the doc already is implying that it works under that same section: Please note that with touching no validation is performed and only the +after_touch+, +after_commit+ and +after_rollback+ callbacks are executed.

[henrik]

Got bitten by this too.

I was debugging an issue where paper_trail was constantly creating backdated versions, and it turned out to be this. PaperTrail by default uses a record's updated_at as the created_at of its version history records. In this case, touch: true caused PaperTrail to create another version, but updated_at remained unchanged, making it backdated.

[courtneymiller2010]

It's also worth noting on this issue that when counter_cache: true and touch: true are used together, none of the callbacks are fired. Based on the touch documentation, which says that only the +after_touch+, +after_commit+ and +after_rollback+ callbacks are executed. However, that is not the case when the two features are combined.

See this PR:WIP: belongs_to w/ counter_cache, touch: true doesn't execute after_commit callback on parent model, which was sadly closed due to staleness.

[appyspaces]

I was looking into implementing counter cache and it seems that the latest documentation in Rails 8.0.2 Guides shows how to use counter_cache with touch:

class Book < ApplicationRecord
  belongs_to :author, touch: :books_updated_at,
    counter_cache: true
end

Source: https://guides.rubyonrails.org/association_basics.html#options

[alexanderholder]

To be clear, I am saying that in my opinion this is a confirmed bug. The script in the issues body confirms this - the touch simply does not run what-so-ever. I put up a PR a year ago to fix this that is awaiting review if anyone would like to help get it over the line: https://github.com/rails/rails/pull/52692