Record not touched when removing associated has_many through

[thomasfedb]

Steps to reproduce

https://gist.github.com/thomasfedb/04ca6b5d476679e6550d4b849531cf42

class Post < ActiveRecord::Base
  has_many :taggings
  has_many :tags, through: :taggings
end

class Tagging < ActiveRecord::Base
  belongs_to :post, touch: true
  belongs_to :tag
end

class Tag < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_has_many_through_touch
    post = Post.create!
    tag = Tag.create!

    updated_at_before_tag_added = post.updated_at
    post.update(tags: [tag])
    updated_at_after_tag_added = post.updated_at

    assert_equal 1, post.tags.count
    assert updated_at_before_tag_added < updated_at_after_tag_added

    updated_at_before_tag_removed = post.updated_at
    post.update(tags: [])
    updated_at_after_tag_removed = post.updated_at

    assert_equal 0, post.tags.count
    assert updated_at_before_tag_removed < updated_at_after_tag_removed,
      "post is not touched"
  end
end

Expected behavior

The post record should be touched both when the tag is added and when the tag is removed, as this results in a tagging being created and destroyed, which is configured with touch: true

Actual behavior

The post record is touched when the tagging is created, but not when it is destroyed

System configuration

Rails version: 5.1.1 Ruby version: 2.4.0

[andrehjr]

Hey guys,

I dig some digging on this issue, if I'm not mistaken when the association is being updated, it ends up calling the following:

delete_or_destroy(records, options[:dependent])

when it reaches the delete_records on the HasManyThroughAssociation, considering there's no :dependent configured, it calls delete_all instead of destroy_all.

Which is why it's not properly propagating the callbacks on deletion. A quick work-around would be to add dependent: :destroy on the tags association to force that behavior.

class Post < ActiveRecord::Base
  has_many :taggings
  has_many :tags, through: :taggings, dependent: :destroy
end

Not sure if that's the expected behavior, but I'll be more than happy to wrap up a PR to fix that behavior if needed.

[thomasfedb]

Pretty sure the work-around would result in some unintended consequences, destroying the tags that were on a post when the post is destroyed?

I'm fairly confident that the current behaviour is a bug.

[andrehjr]

Hey @thomasfedb,

Actually, on has_many through associations only the 'link' between the objects is deleted. Tags would not be deleted if you destroy a Post, even if you added dependent: :destroy on the through association.

According to the docs here and here. The 'delete' method is called by default, meaning that no callbacks are triggered at all. When you specify the dependent: :destroy, you're saying that you want those callbacks triggered. You can see that here.

Automatic deletion of join models is direct, no destroy callbacks are triggered.

The answer is that it is assumed that deletion on an association is about removing the link between the owner and the associated object(s), rather than necessarily the associated objects themselves. So with has_and_belongs_to_many and has_many :through, the join records will be deleted, but the associated records won't.

However, I can see why you'd want to propagate the 'touches' to the Post when you remove an element. Just tried explaining how it's working right now.

[thomasfedb]

It's interesting to see this bit in delete_records:

if source_reflection.options[:counter_cache] && method != :destroy
  counter = source_reflection.counter_cache_column
  klass.decrement_counter counter, records.map(&:id)
end

It seems this is because counter_cache is normally implemented as a after_update callback that would not be triggered by delete. Perhaps a similar workaround is needed for touch also?

[Nerian]

So if I have a has_many :floors, through: :buildings on a City model and I want to remove all floors, but not the buildings of a city, how do I do this? Basically, how do I keep the :through model but removed the floors? I assume city.floors.destroy_all will nuke the buildings and don't remove the floors. I want to remove the floors and leave the buildings.

[thomasfedb]

Why do you assume that @Nerian? Have you tried?

[Nerian]

@thomasfedb Sure :)

From what I have found in the docs, whenever you try to do a destroy_all on a has_many through: Rails will only remove the join model.

[thomasfedb]

@Nerian I'd suggest you go build a quick Rails app with 3 models and give it a try! If you can't find your answer you'd be better of posting on StackOverflow than here on a tangentially related bug report.

[Hirurg103]

This bug is reproducible if you are using update method to add/remove associated objects. I can confirm that the issue is present in the following Rails versions:

Rails version: 5.1.1 - 5.1.7, 5.2.0 -5.2.3

It is not reproducible if you are using the destroy or destroy_all methods:

post.tags.destroy tag
# or 
post.tags.destroy_all

I am adding the full code which you can save into issue_with_touch_in_rails_5.rb and run with ruby issue_with_touch_in_rails_5.rb

# issue_with_touch_in_rails_5.rb

begin require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "http://rubygems.org"
  gem "activerecord", "5.1.1"
  gem 'sqlite3', '< 1.4'
  gem "pry"
end

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

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do

  create_table "posts", force: :cascade do |t|
    t.text "text"
    t.integer "touched_count", default: 0
    t.timestamps
  end

  create_table "taggings", force: :cascade do |t|
    t.integer "post_id", null: false
    t.integer "tag_id", null: false
  end

  create_table "tags", force: :cascade do |t|
    t.text "text"
    t.timestamps
  end

end

class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true
end

class Post < ActiveRecord::Base
  has_many :taggings
  has_many :tags, through: :taggings

  after_touch { increment!(:touched_count) }
end

class Tagging < ActiveRecord::Base
  belongs_to :post, touch: true
  belongs_to :tag
end

class Tag < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_has_many_through_touch
    post = Post.create!
    tag = Tag.create!

    touched_count_before_tag_added = post.touched_count
    post.update(tags: [tag])
    touched_count_after_tag_added = post.touched_count

    assert_equal 1, post.tags.count
    assert touched_count_before_tag_added + 1 == touched_count_after_tag_added

    touched_count_before_tag_removed = post.touched_count
    post.update(tags: [])
    # the bug is not reproducible if you replace `update` with `destroy` or `destroy_all`
    # post.tags.destroy tag
    # post.tags.destroy_all
    touched_count_after_tag_removed = post.touched_count

    assert_equal 0, post.tags.count
    assert touched_count_before_tag_removed + 1 == touched_count_after_tag_removed,
      "post is not touched"
  end
end

I had similar issue with HATM association with touch: true and counter cache in Rails 5.2.0: a join model didn't trigger the touch callback on an associated object when linked objects were added/removed to the associated object via the join model. You can see more details in this Github issue