diff --git a/lib/positioning/mechanisms.rb b/lib/positioning/mechanisms.rb index d6bfa69..e8f564d 100644 --- a/lib/positioning/mechanisms.rb +++ b/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 diff --git a/test/models/list.rb b/test/models/list.rb index b718fcc..020067e 100644 --- a/test/models/list.rb +++ b/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 diff --git a/test/models/optimistic_locking_item.rb b/test/models/optimistic_locking_item.rb new file mode 100644 index 0000000..785ff22 --- /dev/null +++ b/test/models/optimistic_locking_item.rb @@ -0,0 +1,5 @@ +class OptimisticLockingItem < ActiveRecord::Base + belongs_to :list + + positioned on: :list +end diff --git a/test/models/timestamps_item.rb b/test/models/timestamps_item.rb new file mode 100644 index 0000000..5d12a2e --- /dev/null +++ b/test/models/timestamps_item.rb @@ -0,0 +1,5 @@ +class TimestampsItem < ActiveRecord::Base + belongs_to :list + + positioned on: :list +end diff --git a/test/support/active_record.rb b/test/support/active_record.rb index ca4a300..16f596b 100644 --- a/test/support/active_record.rb +++ b/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 diff --git a/test/test_helper.rb b/test/test_helper.rb index a2ec3a9..9f9346a 100644 --- a/test/test_helper.rb +++ b/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" diff --git a/test/test_optimistic_locking.rb b/test/test_optimistic_locking.rb new file mode 100644 index 0000000..c62f7c7 --- /dev/null +++ b/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 diff --git a/test/test_timestamps_item.rb b/test/test_timestamps_item.rb new file mode 100644 index 0000000..93aa05b --- /dev/null +++ b/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