diff --git a/CHANGELOG.md b/CHANGELOG.md index 72ef121..ece69a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,9 @@ ## [Unreleased] - Fix the healer not locking scopes without a `belongs_to` association (or with a nil one). +- Fix scope changes for records hidden by, or partially selected by, a default scope, and when changing part of a composite primary key. +- Fix lists being corrupted when a record with a NULL position (or whose row is already gone) is updated or destroyed. +- Fix a halted destroy (e.g. `dependent: :restrict_with_error`) inside a transaction leaving the list corrupted. ## [0.4.7] - 2025-08-09 diff --git a/lib/positioning.rb b/lib/positioning.rb index 8c52a50..b3641c5 100644 --- a/lib/positioning.rb +++ b/lib/positioning.rb @@ -53,7 +53,7 @@ def positioned(on: [], column: :position) before_create { Mechanisms.new(self, column).create_position } before_update { Mechanisms.new(self, column).update_position } - before_destroy { Mechanisms.new(self, column).destroy_position } + around_destroy { |positioned, block| Mechanisms.new(positioned, column).destroy_position(&block) } define_singleton_method(:"heal_#{column}_column!") do |order = column| Healer.new(self, column, order).heal diff --git a/lib/positioning/mechanisms.rb b/lib/positioning/mechanisms.rb index 5813159..c59812a 100644 --- a/lib/positioning/mechanisms.rb +++ b/lib/positioning/mechanisms.rb @@ -31,7 +31,10 @@ def update_position solidify_position move_out_of_the_way - if positioning_scope_changed? + # A record without a position doesn't occupy a slot, so there's no gap to close + if position_was.nil? + expand(positioning_scope, position..) + elsif positioning_scope_changed? contract(positioning_scope_was, position_was..) expand(positioning_scope, position..) elsif position_was > position @@ -42,11 +45,18 @@ def update_position end def destroy_position - unless destroyed_via_positioning_scope? + if destroyed_via_positioning_scope? + yield + else lock_positioning_scope! - move_out_of_the_way - contract(positioning_scope, (position_was + 1)..) + position_was # Memoize the original position while the row still exists + + yield + + # Only close the gap once the row is gone. A later before_destroy callback can halt the destroy, + # and inside an outer transaction that rollback is swallowed. + contract(positioning_scope, (position_was + 1)..) if @positioned.destroyed? && !position_was.nil? end end @@ -76,8 +86,9 @@ def quoted_column end end + # The record's row as it is in the database, regardless of default scopes or in-memory key changes def record_scope - base_class.where primary_key => [@positioned.id] + base_class.unscoped.where primary_key => [@positioned.id_in_database] end def position @@ -97,7 +108,9 @@ def position_changed? end def position_was - @position_was ||= record_scope.pick(@column) + return @position_was if defined?(@position_was) + + @position_was = record_scope.pick(@column) end def move_out_of_the_way @@ -211,7 +224,8 @@ def positioning_scope_was end def in_positioning_scope? - @positioned.persisted? && positioning_scope.where(primary_key => [@positioned.id]).exists? + @positioned.persisted? && + positioning_scope.where(primary_key => [@positioned.id_in_database]).where.not(@column => nil).exists? end def positioning_scope_changed? diff --git a/test/models/archivable_item.rb b/test/models/archivable_item.rb new file mode 100644 index 0000000..28c3fdc --- /dev/null +++ b/test/models/archivable_item.rb @@ -0,0 +1,7 @@ +class ArchivableItem < ActiveRecord::Base + belongs_to :list + + positioned on: [:list, :archived_at] + + default_scope { where(archived_at: nil) } +end diff --git a/test/models/restricted_item.rb b/test/models/restricted_item.rb new file mode 100644 index 0000000..7410693 --- /dev/null +++ b/test/models/restricted_item.rb @@ -0,0 +1,7 @@ +class RestrictedItem < ActiveRecord::Base + belongs_to :list + + positioned on: :list + + has_many :restrictions, dependent: :restrict_with_error +end diff --git a/test/models/restriction.rb b/test/models/restriction.rb new file mode 100644 index 0000000..ae05858 --- /dev/null +++ b/test/models/restriction.rb @@ -0,0 +1,3 @@ +class Restriction < ActiveRecord::Base + belongs_to :restricted_item +end diff --git a/test/support/active_record.rb b/test/support/active_record.rb index 6031e92..f1d1997 100644 --- a/test/support/active_record.rb +++ b/test/support/active_record.rb @@ -61,6 +61,15 @@ def configure_connection add_index :default_scope_items, [:list_id, :position], unique: true + create_table :archivable_items, force: true do |t| + t.string :name + t.integer :position, null: false + t.datetime :archived_at + t.references :list, null: false + end + + add_index :archivable_items, [:list_id, :archived_at, :position], unique: true + create_table :composite_primary_key_items, primary_key: [:item_id, :account_id], force: true do |t| t.integer :item_id, null: false t.integer :account_id, null: false @@ -133,6 +142,18 @@ def configure_connection add_index :posts, [:blog_id, :position], unique: true add_index :posts, :order, unique: true + + create_table :restricted_items, force: true do |t| + t.string :name + t.integer :position, null: false + t.references :list, null: false + end + + add_index :restricted_items, [:list_id, :position], unique: true + + create_table :restrictions, force: true do |t| + t.references :restricted_item, null: false + end end end diff --git a/test/test_helper.rb b/test/test_helper.rb index e1cfa10..506e6d5 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -12,6 +12,7 @@ require_relative "models/default_scope_item" require_relative "models/composite_primary_key_item" require_relative "models/composite_foreign_key_item" +require_relative "models/archivable_item" require_relative "models/product" require_relative "models/category" require_relative "models/categorised_item" @@ -21,3 +22,5 @@ require_relative "models/blog" require_relative "models/post" require_relative "models/entity" +require_relative "models/restricted_item" +require_relative "models/restriction" diff --git a/test/test_positioning.rb b/test/test_positioning.rb index 6cf39ea..b9da9aa 100644 --- a/test/test_positioning.rb +++ b/test/test_positioning.rb @@ -684,6 +684,73 @@ def test_that_not_destroyed_via_positioning_scope_closes_gap assert_equal [1, 2], [first_item.reload, third_item.reload].map(&:position) end + + def test_that_a_halted_destroy_leaves_the_list_intact + list = List.create name: "First List" + first_item = RestrictedItem.create list: list, name: "First Item" + second_item = RestrictedItem.create list: list, name: "Second Item" + third_item = RestrictedItem.create list: list, name: "Third Item" + + second_item.restrictions.create + + # Within an outer transaction (as here), the rollback of the halted destroy is swallowed + refute second_item.destroy + + assert_equal [1, 2, 3], [first_item.reload, second_item.reload, third_item.reload].map(&:position) + + third_item.destroy + + assert_equal [1, 2], [first_item.reload, second_item.reload].map(&:position) + end +end + +class TestRecordScope < Minitest::Test + include Minitest::Hooks + + def around + ActiveRecord::Base.transaction do + super + raise ActiveRecord::Rollback + end + end + + def test_that_a_default_scope_select_does_not_prevent_a_scope_change + first_list = List.create name: "First List" + second_list = List.create name: "Second List" + first_list.default_scope_items.create name: "First Item" + second_item = first_list.default_scope_items.create name: "Second Item" + + second_item = DefaultScopeItem.unscoped.find(second_item.id) + second_item.update list: second_list + + assert_equal [[second_list.id, 1]], DefaultScopeItem.unscoped.where(id: second_item.id).pluck(:list_id, :position) + end + + def test_that_a_record_hidden_by_a_default_scope_can_change_scope + list = List.create name: "List" + first_item = ArchivableItem.create list: list, name: "First Item" + second_item = ArchivableItem.create list: list, name: "Second Item" + third_item = ArchivableItem.create list: list, name: "Third Item" + + second_item.update archived_at: Time.current + assert_equal [1, 2], [first_item.reload, third_item.reload].map(&:position) + + second_item = ArchivableItem.unscoped.find(second_item.id) + second_item.update archived_at: nil + + assert_equal [1, 2, 3], [first_item.reload, third_item.reload, second_item.reload].map(&:position) + end + + def test_that_a_primary_key_change_does_not_lose_track_of_the_record + list = List.create name: "List" + first_item = list.composite_primary_key_items.create item_id: 1, account_id: 1, name: "First Item" + second_item = list.composite_primary_key_items.create item_id: 2, account_id: 2, name: "Second Item" + third_item = list.composite_primary_key_items.create item_id: 3, account_id: 3, name: "Third Item" + + first_item.update account_id: 4, position: 3 + + assert_equal [1, 2, 3], [second_item.reload, third_item.reload, first_item.reload].map(&:position) + end end class TestPositioningColumns < Minitest::Test @@ -1376,6 +1443,70 @@ def test_destruction end end +class TestNullPositions < Minitest::Test + include Minitest::Hooks + + def around + ActiveRecord::Base.transaction do + super + raise ActiveRecord::Rollback + end + end + + def setup + @list = List.create name: "List" + @other_list = List.create name: "Other List" + @legacy_item = @list.new_items.create name: "Legacy Item" + @first_item = @list.new_items.create name: "First Item" + @second_item = @list.new_items.create name: "Second Item" + + # A row that predates positioning (or hasn't been healed yet) + @legacy_item.update_columns position: nil + @first_item.update_columns position: 1 + @second_item.update_columns position: 2 + end + + def test_that_a_null_position_can_be_given_a_position + @legacy_item.update position: 1 + + assert_equal [1, 2, 3], [@legacy_item.reload, @first_item.reload, @second_item.reload].map(&:position) + end + + def test_that_a_null_position_can_be_moved_to_the_end + @legacy_item.update position: :last + + assert_equal [1, 2, 3], [@first_item.reload, @second_item.reload, @legacy_item.reload].map(&:position) + end + + def test_that_a_null_position_can_be_positioned_relatively + @legacy_item.update position: {after: @first_item} + + assert_equal [1, 2, 3], [@first_item.reload, @legacy_item.reload, @second_item.reload].map(&:position) + end + + def test_that_destroying_a_null_position_leaves_the_list_intact + @legacy_item.destroy + + assert_equal [1, 2], [@first_item.reload, @second_item.reload].map(&:position) + end + + def test_that_moving_a_null_position_to_another_scope_leaves_the_list_intact + @legacy_item.update list: @other_list + + assert_equal [1, 2], [@first_item.reload, @second_item.reload].map(&:position) + assert_equal 1, @legacy_item.reload.position + end + + def test_that_destroying_a_record_whose_row_is_already_gone_does_not_raise + item = @list.items.create name: "Item" + Item.where(id: item.id).delete_all + + item.destroy + + assert item.destroyed? + end +end + class TestSTIPositioning < Minitest::Test include Minitest::Hooks