Close the gap on destroy only once the row is actually deleted

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
 

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

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?

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

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

test/models/restriction.rb

@@ -0,0 +1,3 @@
+class Restriction < ActiveRecord::Base
+  belongs_to :restricted_item
+end

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
 

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"

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