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

[yan-hoose]

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:

rails/activerecord/lib/active_record/associations/belongs_to_association.rb

Line 98 in c21c413

def update_counters(by)

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:

rails/activerecord/test/cases/locking_test.rb

Line 499 in c01a744

assert_equal 1, car.reload.wheels_count

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:

rails/activerecord/test/cases/locking_test.rb

Line 499 in c01a744

assert_equal 1, car.reload.wheels_count

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

rails/activerecord/lib/active_record/persistence.rb

Lines 861 to 874 in c21c413

	# Wrapper around #increment that writes the update to the database.
	# Only +attribute+ is updated; the record itself is not saved.
	# This means that any other modified attributes will still be dirty.
	# Validations and callbacks are skipped. Supports the +touch+ option from
	# +update_counters+, see that for more.
	# Returns +self+.
	def increment!(attribute, by = 1, touch: nil)
	increment(attribute, by)
	change = public_send(attribute) - (public_send(:"#{attribute}_in_database") || 0)
	self.class.update_counters(id, attribute => change, touch: touch)
	public_send(:"clear_#{attribute}_change")
	self
	end

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:

rails/activerecord/lib/active_record/associations.rb

Line 1750 in 7b1f65e

# [:touch]

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

rails/activerecord/lib/active_record/associations/belongs_to_association.rb

Lines 98 to 102 in 746d115

	def update_counters(by)
	if require_counter_update? && foreign_key_present?
	if target && !stale_target?
	target.increment!(reflection.counter_cache_column, by, touch: reflection.options[:touch])
	else

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

rails/activerecord/lib/active_record/callbacks.rb

Lines 451 to 453 in 746d115

	def increment!(attribute, by = 1, touch: nil) # :nodoc:
	touch ? _run_touch_callbacks { super } : super
	end

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.