From c895b89eaa7cac47c046d5db85a02b2f66cd13b2 Mon Sep 17 00:00:00 2001 From: Brendon Muir Date: Fri, 16 Oct 2026 12:09:16 +1300 Subject: [PATCH 1/6] Find the record's own row regardless of default scopes and key changes `record_scope` is how Mechanisms reads and moves the record's own row, but it applied the model's default scope and used the in-memory primary key: - A default scope `where` that hides the row (e.g. the archived/soft deleted pattern from #59, with the timestamp in the positioning scope) meant restoring a record raised NoMethodError on nil. - A default scope `select` that omits the scope columns raised MissingAttributeError on any scope change. - Changing part of a composite primary key in the same save as the position meant the row couldn't be found and update_position raised. Use `unscoped` and `id_in_database`; this only affects queries for the record's own row, not the list scope. Co-Authored-By: Claude Opus 5.5 --- lib/positioning/mechanisms.rb | 5 ++-- test/models/archivable_item.rb | 7 +++++ test/support/active_record.rb | 9 +++++++ test/test_helper.rb | 1 + test/test_positioning.rb | 49 ++++++++++++++++++++++++++++++++++ 5 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 test/models/archivable_item.rb diff --git a/lib/positioning/mechanisms.rb b/lib/positioning/mechanisms.rb index 5813159..859ead3 100644 --- a/lib/positioning/mechanisms.rb +++ b/lib/positioning/mechanisms.rb @@ -76,8 +76,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 @@ -211,7 +212,7 @@ 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]).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/support/active_record.rb b/test/support/active_record.rb index f1e9522..9cb24e8 100644 --- a/test/support/active_record.rb +++ b/test/support/active_record.rb @@ -45,6 +45,15 @@ 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 diff --git a/test/test_helper.rb b/test/test_helper.rb index e1cfa10..752a55e 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" diff --git a/test/test_positioning.rb b/test/test_positioning.rb index 6cf39ea..3f65701 100644 --- a/test/test_positioning.rb +++ b/test/test_positioning.rb @@ -686,6 +686,55 @@ def test_that_not_destroyed_via_positioning_scope_closes_gap 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 include Minitest::Hooks From 032be8dd4c36cd26502deba5466892ca3df09bc5 Mon Sep 17 00:00:00 2001 From: Brendon Muir Date: Fri, 16 Oct 2026 12:10:30 +1300 Subject: [PATCH 2/6] Treat a NULL position as not occupying a slot `position_was` is memoized with `||=`, which doesn't memoize nil. So for a row whose position is NULL, `move_out_of_the_way` set it to 0 and the next call read back 0, and the record was then treated as if it had been at position 0. The result was a 0 left in the list and every other record shifted down by one, on update, destroy and scope change. With a relative position it raised NoMethodError instead. That affects tables that gained a position column but haven't been healed yet, and destroying a record whose row is already gone, e.g. a repeated request (#39, #30), which raised `undefined method '+' for nil`. Memoize nil properly and treat a NULL position as not being in the list yet: there's no gap to close when it leaves, and it's inserted (rather than moved) when it's given a position. Co-Authored-By: Claude Opus 5.5 --- lib/positioning/mechanisms.rb | 14 +++++--- test/test_positioning.rb | 64 +++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/lib/positioning/mechanisms.rb b/lib/positioning/mechanisms.rb index 859ead3..5d8c4b8 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 @@ -46,7 +49,7 @@ def destroy_position lock_positioning_scope! move_out_of_the_way - contract(positioning_scope, (position_was + 1)..) + contract(positioning_scope, (position_was + 1)..) unless position_was.nil? end end @@ -98,7 +101,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 @@ -212,7 +217,8 @@ def positioning_scope_was end def in_positioning_scope? - @positioned.persisted? && positioning_scope.where(primary_key => [@positioned.id_in_database]).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/test_positioning.rb b/test/test_positioning.rb index 3f65701..d8f4a4d 100644 --- a/test/test_positioning.rb +++ b/test/test_positioning.rb @@ -1425,6 +1425,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 From 16b4b4217a74b09f1f7c2b1ad273d1d53d35b5da Mon Sep 17 00:00:00 2001 From: Brendon Muir Date: Fri, 16 Oct 2026 12:13:35 +1300 Subject: [PATCH 3/6] Use locking reads for positions once the scope is locked Under MySQL's default REPEATABLE READ isolation, a plain SELECT returns the snapshot taken at the transaction's first read, but UPDATEs always see the latest rows. If anything read in the transaction before we took the lock (a validation, a callback, app code), and another transaction moved records while we waited for the lock, `position_was`, the maximum and the relative record's position were computed from stale data while expand/contract operated on current rows. The result was a unique violation or, without a unique index, duplicate and missing positions. Make those reads locking reads, which always see the latest committed rows. `MAX()` can't be combined with `FOR UPDATE` on PostgreSQL, so the maximum is read as the top row instead. PostgreSQL (READ COMMITTED) and SQLite are unaffected either way. Co-Authored-By: Claude Opus 5.5 --- lib/positioning/mechanisms.rb | 14 +++++++---- test/test_positioning.rb | 44 +++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/lib/positioning/mechanisms.rb b/lib/positioning/mechanisms.rb index 5d8c4b8..26371a3 100644 --- a/lib/positioning/mechanisms.rb +++ b/lib/positioning/mechanisms.rb @@ -100,10 +100,13 @@ def position_changed? @positioned.send :"#{@column}_changed?" end + # Reads of positions made after locking the scope are locking reads. Under MySQL's default + # REPEATABLE READ isolation a plain read returns the snapshot taken at the transaction's first + # read, which can predate changes committed while we were waiting for the lock. def position_was return @position_was if defined?(@position_was) - @position_was = record_scope.pick(@column) + @position_was = record_scope.lock.pick(@column) end def move_out_of_the_way @@ -157,7 +160,7 @@ def solidify_position relative_record_or_id end - relative_record_scope = positioning_scope.where(primary_key => [relative_id]) + relative_record_scope = positioning_scope.where(primary_key => [relative_id]).lock unless relative_record_scope.exists? raise Error.new, "relative `#{@column}` record must be in the same scope" @@ -177,7 +180,10 @@ def solidify_position end def last_position - (positioning_scope.maximum(@column) || 0) + (in_positioning_scope? ? 0 : 1) + # MAX() can't be combined with FOR UPDATE on PostgreSQL + maximum = positioning_scope.where.not(@column => nil).reorder(@column => :desc).lock.pick(@column) + + (maximum || 0) + (in_positioning_scope? ? 0 : 1) end def scope_columns @@ -218,7 +224,7 @@ def positioning_scope_was def in_positioning_scope? @positioned.persisted? && - positioning_scope.where(primary_key => [@positioned.id_in_database]).where.not(@column => nil).exists? + positioning_scope.where(primary_key => [@positioned.id_in_database]).where.not(@column => nil).lock.exists? end def positioning_scope_changed? diff --git a/test/test_positioning.rb b/test/test_positioning.rb index d8f4a4d..34e187d 100644 --- a/test/test_positioning.rb +++ b/test/test_positioning.rb @@ -17,6 +17,50 @@ def test_struct_takes_keyword_arguments end class TestTransactionSafety < Minitest::Test + def test_no_stale_reads_after_waiting_for_the_lock + skip "SQLite doesn't support row locking" if ENV["DB"] == "sqlite" + + ActiveRecord::Base.connection_handler.clear_all_connections! + + list = List.create name: "List" + students = ["A", "B", "C", "D", "E"].map do |name| + list.authors.create name: name, type: "Author::Student" + end + + locked = Queue.new + + first_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + ActiveRecord::Base.transaction do + List.lock.find(list.id) + locked << true + sleep 0.5 + list.authors.create name: "New", type: "Author::Student", position: 1 + end + end + end + + locked.pop + + second_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + ActiveRecord::Base.transaction do + # Any read here (validations, callbacks, app code) pins MySQL's REPEATABLE READ snapshot + # before the first thread commits. + Author.where(list: list).count + students[2].update position: 5 + end + end + end + + [first_thread, second_thread].each(&:join) + + assert_equal [[1, "New"], [2, "A"], [3, "B"], [4, "D"], [5, "C"], [6, "E"]], + list.authors.order(:position).pluck(:position, :name) + + list.destroy + end + def test_no_duplicate_row_values_when_creating ActiveRecord::Base.connection_handler.clear_all_connections! From 8e02c2951e8e8dc360d95ec955515fccfeaa056b Mon Sep 17 00:00:00 2001 From: Brendon Muir Date: Mon, 19 Oct 2026 11:21:07 +1300 Subject: [PATCH 4/6] Add changelog entry Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4870e6..77f13c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## [Unreleased] +- Fix scope changes for records hidden by, or partially selected by, a default scope, and when changing part of a composite primary key. + ## [0.4.7] - 2025-08-09 - Fix `solidify_position` so that it properly raises an error for an invalid string position. From f6afcb97cc328e9016e8498a070671c823590769 Mon Sep 17 00:00:00 2001 From: Brendon Muir Date: Mon, 19 Oct 2026 11:21:14 +1300 Subject: [PATCH 5/6] Add changelog entry Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77f13c5..dbcf1cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## [Unreleased] - 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. ## [0.4.7] - 2025-08-09 From 707827a69c9449c56801567ba243371d98f272f4 Mon Sep 17 00:00:00 2001 From: Brendon Muir Date: Mon, 19 Oct 2026 11:21:14 +1300 Subject: [PATCH 6/6] Add changelog entry Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dbcf1cf..37993bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ - 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 stale position reads on MySQL (REPEATABLE READ) after waiting for the scope lock. ## [0.4.7] - 2025-08-09