Add optimistic locking and timestamp coverage for positioned list items

lib/positioning/mechanisms.rb

@@ -70,12 +70,6 @@ def primary_key
       base_class.primary_key
     end
 
-    def quoted_column
-      with_connection do |connection|
-        connection.quote_table_name_for_assignment base_class.table_name, @column
-      end
-    end
-
     def record_scope
       base_class.where primary_key => [@positioned.id]
     end
@@ -102,17 +96,90 @@ def position_was
 
     def move_out_of_the_way
       position_was # Memoize the original position before changing it
-      record_scope.update_all @column => 0
+      update_scope(record_scope, {@column => 0})
     end
 
     def expand(scope, range)
-      scope.where(@column => range).update_all "#{quoted_column} = #{quoted_column} * -1"
-      scope.where(@column => ..-1).update_all "#{quoted_column} = #{quoted_column} * -1 + 1"
+      update_scope(scope.where(@column => range), {@column => negate_position})
+      update_scope(scope.where(@column => ..-1), {@column => negate_position_with_offset(1)})
     end
 
     def contract(scope, range)
-      scope.where(@column => range).update_all "#{quoted_column} = #{quoted_column} * -1"
-      scope.where(@column => ..-1).update_all "#{quoted_column} = #{quoted_column} * -1 - 1"
+      update_scope(scope.where(@column => range), {@column => negate_position})
+      update_scope(scope.where(@column => ..-1), {@column => negate_position_with_offset(-1)})
+    end
+
+    def update_scope(scope, updates)
+      updates = updates.dup
+      updates.merge!(timestamp_updates)
+      return if updates.empty?
+
+      manager = Arel::UpdateManager.new
+      manager.table(base_class.arel_table)
+      manager.set(build_assignments(updates))
+      arel_constraints(scope.arel).each { |constraint| manager.where(constraint) }
+
+      with_connection do |connection|
+        connection.update(manager, "Positioning update")
+      end
+    end
+
+    def arel_constraints(arel)
+      return arel.constraints if arel.respond_to?(:constraints)
+
+      arel.ast.wheres
+    end
+
+    def build_assignments(updates)
+      updates.map do |column, value|
+        attribute = base_class.arel_table[column]
+        unless value.is_a?(Arel::Nodes::Node) || value.is_a?(Arel::Attributes::Attribute)
+          value = Arel::Nodes.build_quoted(value, attribute)
+        end
+        [attribute, value]
+      end
+    end
+
+    def position_attribute
+      base_class.arel_table[@column]
+    end
+
+    def negate_position
+      Arel::Nodes::Multiplication.new(position_attribute, Arel::Nodes.build_quoted(-1, position_attribute))
+    end
+
+    def negate_position_with_offset(offset)
+      base = negate_position
+      return base if offset.zero?
+
+      adjustment = Arel::Nodes.build_quoted(offset.abs, position_attribute)
+      offset.positive? ? Arel::Nodes::Addition.new(base, adjustment) : Arel::Nodes::Subtraction.new(base, adjustment)
+    end
+
+    def timestamp_updates
+      columns = timestamp_columns
+      return {} if columns.empty?
+
+      time = current_time
+      columns.each_with_object({}) { |column, updates| updates[column] = time }
+    end
+
+    def timestamp_columns
+      return [] unless base_class.record_timestamps
+
+      if base_class.respond_to?(:timestamp_attributes_for_update_in_model, true)
+        base_class.send(:timestamp_attributes_for_update_in_model)
+      else
+        base_class.send(:timestamp_attributes_for_update)
+      end
+    end
+
+    def current_time
+      if base_class.respond_to?(:current_time_from_proper_timezone, true)
+        base_class.send(:current_time_from_proper_timezone)
+      else
+        Time.now
+      end
     end
 
     def solidify_position

test/models/list.rb

@@ -1,5 +1,7 @@
 class List < ActiveRecord::Base
   has_many :items, -> { order(:position) }, dependent: :destroy
+  has_many :optimistic_locking_items, -> { order(:position) }, dependent: :destroy
+  has_many :timestamps_items, -> { order(:position) }, dependent: :destroy
   has_many :new_items, -> { order(:position) }, dependent: :destroy
   has_many :default_scope_items, -> { order(:position) }, dependent: :destroy
   has_many :composite_primary_key_items, -> { order(:position) }, dependent: :destroy

test/models/optimistic_locking_item.rb

@@ -0,0 +1,5 @@
+class OptimisticLockingItem < ActiveRecord::Base
+  belongs_to :list
+
+  positioned on: :list
+end

test/models/timestamps_item.rb

@@ -0,0 +1,5 @@
+class TimestampsItem < ActiveRecord::Base
+  belongs_to :list
+
+  positioned on: :list
+end

test/support/active_record.rb

@@ -30,6 +30,24 @@
 
     add_index :items, [:list_id, :position], unique: true
 
+    create_table :optimistic_locking_items, force: true do |t|
+      t.string :name
+      t.integer :position, null: false
+      t.integer :lock_version, null: false, default: 0
+      t.references :list, null: false
+    end
+
+    add_index :optimistic_locking_items, [:list_id, :position], unique: true
+
+    create_table :timestamps_items, force: true do |t|
+      t.string :name
+      t.integer :position, null: false
+      t.references :list, null: false
+      t.timestamps null: false
+    end
+
+    add_index :timestamps_items, [:list_id, :position], unique: true
+
     create_table :new_items, force: true do |t|
       t.string :name
       t.integer :position

test/test_helper.rb

@@ -8,6 +8,8 @@
 
 require_relative "models/list"
 require_relative "models/item"
+require_relative "models/optimistic_locking_item"
+require_relative "models/timestamps_item"
 require_relative "models/new_item"
 require_relative "models/default_scope_item"
 require_relative "models/composite_primary_key_item"

test/test_optimistic_locking.rb

@@ -0,0 +1,32 @@
+require "test_helper"
+
+class TestOptimisticLocking < Minitest::Test
+  include Minitest::Hooks
+
+  def around
+    ActiveRecord::Base.transaction do
+      super
+      raise ActiveRecord::Rollback
+    end
+  end
+
+  def test_position_updates_do_not_allow_stale_writes_on_loaded_models
+    list = List.create name: "List"
+    first_item = list.optimistic_locking_items.create name: "First Item"
+    second_item = list.optimistic_locking_items.create name: "Second Item"
+    third_item = list.optimistic_locking_items.create name: "Third Item"
+
+    scope = list.optimistic_locking_items.order(:position)
+    assert_equal [1, 2, 3], scope.pluck(:position)
+
+    third_item.update! position: 1
+
+    assert_equal [third_item.id, first_item.id, second_item.id], scope.pluck(:id)
+
+    assert_equal 0, OptimisticLockingItem.where(id: first_item.id).pick(:lock_version)
+
+    first_item.update! name: "Updated First Item"
+
+    assert_equal "Updated First Item", OptimisticLockingItem.find(first_item.id).name
+  end
+end

test/test_timestamps_item.rb

@@ -0,0 +1,31 @@
+require "test_helper"
+
+class TestTimestampsItem < Minitest::Test
+  include Minitest::Hooks
+
+  def around
+    ActiveRecord::Base.transaction do
+      super
+      raise ActiveRecord::Rollback
+    end
+  end
+
+  def test_updated_at_is_touched_when_other_items_are_repositioned
+    list = List.create name: "List"
+    first_item = list.timestamps_items.create name: "First Item"
+    second_item = list.timestamps_items.create name: "Second Item"
+    third_item = list.timestamps_items.create name: "Third Item"
+
+    before_updated_ats = list.timestamps_items.order(:id).pluck(:id, :updated_at).to_h
+
+    third_item.update! position: 1
+
+    after_updated_ats = list.timestamps_items.order(:id).pluck(:id, :updated_at).to_h
+
+    assert_equal [third_item.id, first_item.id, second_item.id],
+      list.timestamps_items.order(:position).pluck(:id)
+    assert_operator after_updated_ats[first_item.id], :>, before_updated_ats[first_item.id]
+    assert_operator after_updated_ats[second_item.id], :>, before_updated_ats[second_item.id]
+    assert_operator after_updated_ats[third_item.id], :>, before_updated_ats[third_item.id]
+  end
+end