Merge pull request #15 from joaomarcos96/opt_out_advisory_lock

Support opting out of Advisory Lock

README.md

@@ -66,6 +66,10 @@ positioned on: :type
 belongs_to :list
 belongs_to :category
 positioned on: [:list, :category, :enabled]
+
+# If you do not want to use Advisory Lock, you can disable it entirely by passing the 'advisory_lock' flag as false
+belongs_to :list
+positioned on: :list, advisory_lock: false
 ```
 
 ### Manipulating Positioning
@@ -243,6 +247,33 @@ https://github.com/brendon/positioning/blob/main/lib/positioning/advisory_lock.r
 
 If you have any concerns or improvements please file a GitHub issue.
 
+### Opting out of Advisory Lock
+
+There are cases where Advisory Lock may be unwanted or unnecessary, for instance, if you already lock the parent record in **every** operation that will touch the database on the positioned item, **everywhere** in your application.
+
+Example of such scenario in your application:
+
+```ruby
+list = List.create(name: "List")
+
+list.with_lock do
+  item_a = list.items.create(name: "Item A")
+  item_b = list.items.create(name: "Item B")
+  item_c = list.items.create(name: "Item C")
+
+  item_c.update(position: {before: item_a})
+
+  item_a.destroy
+end
+```
+
+Therefore, making sure you already have another mechanism to avoid race conditions, you can opt out of Advisory Lock by setting `advisory_lock` to `false` when declaring positioning:
+
+```ruby
+belongs_to :list
+positioned on: :list, advisory_lock: false
+```
+
 ## Development
 
 After checking out the repo, run `bin/setup` to install dependencies. Then, run `rake test` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment.

lib/positioning.rb

@@ -18,7 +18,7 @@ def positioning_columns
         @positioning_columns ||= {}
       end
 
-      def positioned(on: [], column: :position)
+      def positioned(on: [], column: :position, advisory_lock: true)
         unless base_class?
           raise Error.new "can't be called on an abstract class or STI subclass."
         end
@@ -43,18 +43,22 @@ def positioned(on: [], column: :position)
             super(position)
           end
 
-          advisory_lock = AdvisoryLock.new(base_class, column)
+          if advisory_lock
+            advisory_lock_callback = AdvisoryLock.new(base_class, column)
 
-          before_create advisory_lock
-          before_update advisory_lock
-          before_destroy advisory_lock
+            before_create advisory_lock_callback
+            before_update advisory_lock_callback
+            before_destroy advisory_lock_callback
+          end
 
           before_create { Mechanisms.new(self, column).create_position }
           before_update { Mechanisms.new(self, column).update_position }
           before_destroy { Mechanisms.new(self, column).destroy_position }
 
-          after_commit advisory_lock
-          after_rollback advisory_lock
+          if advisory_lock
+            after_commit advisory_lock_callback
+            after_rollback advisory_lock_callback
+          end
         end
       end
     end

test/models/item_without_advisory_lock.rb

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

test/models/list.rb

@@ -1,4 +1,5 @@
 class List < ActiveRecord::Base
   has_many :items, -> { order(:position) }, dependent: :destroy
+  has_many :item_without_advisory_locks, -> { order(:position) }, dependent: :destroy
   has_many :authors, -> { order(:position) }, dependent: :destroy
 end

test/support/active_record.rb

@@ -21,6 +21,14 @@
 
     add_index :items, [:list_id, :position], unique: true
 
+    create_table :item_without_advisory_locks, force: true do |t|
+      t.string :name
+      t.integer :position, null: false
+      t.references :list, null: false
+    end
+
+    add_index :item_without_advisory_locks, [:list_id, :position], unique: true
+
     create_table :categories, force: true do |t|
       t.string :name
       t.integer :position, null: false

test/test_positioning.rb

@@ -2,6 +2,7 @@
 
 require_relative "models/list"
 require_relative "models/item"
+require_relative "models/item_without_advisory_lock"
 require_relative "models/category"
 require_relative "models/categorised_item"
 require_relative "models/author"
@@ -47,6 +48,89 @@ def test_no_duplicate_row_values
 
     list.destroy
   end
+
+  def test_no_duplicate_row_values_when_advisory_lock_is_disabled_and_parent_record_is_locked
+    ActiveRecord::Base.connection_handler.clear_all_connections!
+
+    list = List.create name: "List"
+    items = []
+
+    2.times do
+      threads = 3.times.map do
+        Thread.new do
+          ActiveRecord::Base.connection_pool.with_connection do
+            list.with_lock do
+              items << list.item_without_advisory_locks.create(name: "Item")
+            end
+          end
+        end
+      end
+      threads.each(&:join)
+    end
+
+    assert_equal (1..items.length).to_a, list.item_without_advisory_locks.map(&:position)
+
+    list.destroy
+  end
+
+  def test_no_duplicate_row_values_when_updating_and_advisory_lock_is_disabled_and_parent_record_is_locked
+    ActiveRecord::Base.connection_handler.clear_all_connections!
+
+    list = List.create name: "List"
+    item_a = list.item_without_advisory_locks.create name: "Item A"
+    item_b = list.item_without_advisory_locks.create name: "Item B"
+    item_c = list.item_without_advisory_locks.create name: "Item C"
+
+    2.times do
+      threads = 3.times.map do
+        Thread.new do
+          ActiveRecord::Base.connection_pool.with_connection do
+            list.with_lock do
+              item_c.update(position: {before: item_a})
+            end
+          end
+        end
+      end
+      threads.each(&:join)
+    end
+
+    assert_equal item_c.reload.position, 1
+    assert_equal item_a.reload.position, 2
+    assert_equal item_b.reload.position, 3
+
+    list.destroy
+  end
+
+  def test_no_duplicate_row_values_when_destroying_and_advisory_lock_is_disabled_and_parent_record_is_locked
+    ActiveRecord::Base.connection_handler.clear_all_connections!
+
+    list = List.create name: "List"
+    items = []
+    ["A", "B", "C", "D", "E", "F", "G", "H"].each do |name|
+      items << list.item_without_advisory_locks.create(name: name)
+    end
+
+    2.times do
+      threads = 3.times.map do
+        Thread.new do
+          ActiveRecord::Base.connection_pool.with_connection do
+            list.with_lock do
+              items.first.destroy
+              items.shift
+            end
+          end
+        end
+      end
+      threads.each(&:join)
+    end
+
+    items.each(&:reload)
+
+    assert_equal items.sort_by(&:position).pluck(:position, :name), [[1, "G"], [2, "H"]]
+    assert_equal list.item_without_advisory_locks.order(:position).pluck(:position, :name), [[1, "G"], [2, "H"]]
+
+    list.destroy
+  end
 end
 
 class TestPositioningMechanisms < Minitest::Test