rails: Counter cache does not work well with optimistic locking

Counter cache and optimistic locking don’t play well together. The example below illustrated the problem:

require 'active_record'
require 'minitest/autorun'
require 'sqlite3'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection 'sqlite3::memory:'

ActiveRecord::Schema.define do
  create_table :people do |t|
    t.string :name
    t.integer :lock_version, null: false, default: 0
    t.integer :vehicles_count, null: false, default: 0
    t.integer :jobs_count, null: false, default: 0
    t.timestamps
  end

  create_table :jobs do |t|
    t.belongs_to :person
  end

  create_table :vehicles do |t|
    t.belongs_to :owner, polymorphic: true
  end
end

class Person < ActiveRecord::Base
  has_many :jobs,                 dependent: :destroy, counter_cache: true
  has_many :vehicles, as: :owner, dependent: :destroy, counter_cache: true
end

class Job < ActiveRecord::Base
  belongs_to :person, counter_cache: true
end

class Vehicle < ActiveRecord::Base
  belongs_to :owner, polymorphic: true, counter_cache: true
end

class BugTest < Minitest::Test
  def test_locking_simple_non_polymorphic
    person = Person.create!
    person.jobs << Job.create!

    # Fails on the next expression. jobs#<< updates the lock_version in the
    # database, but not in the loaded object. Raises StaleObjectError.

    person.destroy
  end

  def test_locking_simple_polymorphic
    person = Person.create!
    person.vehicles << Vehicle.create!

    # Should fail on the next expression if counter_cache worked for
    # polymorphic relationships. It doesn't at the time of writing so the
    # test passes. Fails on top of #16445

    person.destroy
  end
end

Note that one of the tests passes, because counter cache does not work on polymorphic associations. If that is fixed (#16445), both tests will fail.

As far as I can tell, the expected behaviour is for counter cache to bump the lock_version. It does so in the database, but in some situations it fails to update it in the loaded object. I’ve created a gist with test cases that tests this behaviour in detail. All of the tests currently fail.

One way to get the tests in the gist to pass is to add touch: true to both belongs_to relationships (after applying #16445 and #16448, which fix some bugs). touch: true seems to do a good job of updating lock_version in the loaded model when necessary.

I see three possible ways to resolve this issue:

1. Don’t bump lock_version for counter_cache. This makes some sense to me – the version is not bumped if there is no counter_cache and I don’t see why adding a cache should change this. The user can always intentionally get it by passing touch: true.
2. Have the counter cache do what touch: true does. I gave this a shot, but couldn’t get it to work in all cases. It might introduce some duplication between counter_cache and touch. It makes the counter_cache code very aware of (and non-orthogonal to) the optimistic locking code.
3. Always set touch: true in the association when there is a counter cache. It’s a cheap solution 😄 . Not sure how it can be implemented, but it’s seems to be the simplest thing that will (1) preserve the behaviour and (2) fix the issue. Also – no duplication.

There might be other possibilities that I haven’t thought of.

My personal favourite is the first one. It gives the user the most options and it doesn’t require changing the counter_cache too much. Not changing the lock_version behaviour also makes sense to me. However, I understand it might not be the desired solution.

Thoughts?

/cc @senny

P.S.: Whichever of the three (or more) options makes the most sense, I’d love to try implementing it 😄