bug - usage of acts_as_list and counter_cache will break the after_touch callback

[yfxie]

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  gem 'activerecord', "6.1.1"
  gem 'sqlite3'
  gem 'mocha'
  gem 'acts_as_list'
end

require "active_record"
require "minitest/autorun"
require 'mocha/minitest'
require "logger"

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 :labels_count, default: 0
    t.timestamps
  end

  create_table :labels, force: true do |t|
    t.integer :post_id
    t.integer :position, null: false, default: 1
    t.string :name
    t.timestamps
  end
end

class Post < ActiveRecord::Base
  has_many :labels

  after_touch :ping

  def ping
  end
end

class Label < ActiveRecord::Base
  belongs_to :post, touch: true, counter_cache: :labels_count
  acts_as_list
end

class BugTest < Minitest::Test
  def setup
    @post = Post.create
  end

  # success
  def test_create
    Post.any_instance.expects(:ping).once
    @post.labels.create(name: 'abc')
  end

  # success
  def test_update
    label = @post.labels.create(name: 'abc')
    Post.any_instance.expects(:ping).once
    label.update(name: '123')
  end

  # failure
  def test_destroy
    label = @post.labels.create(name: 'abc')
    Post.any_instance.expects(:ping).once
    label.destroy
  end
end

If we remove counter_cache and the test will pass. I found the bug due to before_destroy :reload(here)

So the following code is a failure case too

class Label < ActiveRecord::Base
  belongs_to :post, touch: true, counter_cache: :labels_count
  # acts_as_list
  before_destroy :reload
end

For any reason, I think it shouldn't call reload at callbacks.

[brendon]

Hi @yfxie, this one runs deep! :) It's been around for a while. I don't have time to dig too deep at the moment but if you're able to figure out why we're doing this in the first place that'd help heaps! If we can remove this, or replace it with something that achieves the same outcome (without the bug) then that would be great.

Here's what I know:

• reload has been there as long as I've been maintaining this gem. For a while it was replaced by lock! but this was causing problems so it was reverted. https://github.com/brendon/acts_as_list/commit/d0b2b815782374bb77b77b98dee7258c9adae589
• This is where reload was added https://github.com/brendon/acts_as_list/commit/8bc26426d3104876191fc579cc4fced21ee9e2bb

There are no issue references there that I could find, but it seems that we need to reload the position of items just before delete if we're deleting lots of them as their position will have changed and is used in shuffling things around.

[yfxie]

If I'm free, I will try to figure out whether we can replace it. Thanks for your detailed explanation.

[brendon]

Closing this due to inactivity. Check out https://github.com/brendon/positioning as an alternative gem that doesn't exhibit this issue.