From 9249a3c84863f3fab836ebef51811c3adfdacd33 Mon Sep 17 00:00:00 2001 From: Joao Marcos Date: Fri, 28 Jun 2024 13:58:18 -0300 Subject: [PATCH 1/4] Support opting out of Advisory Lock --- README.md | 22 ++++++++++++++++++++ lib/positioning.rb | 18 +++++++++------- test/models/item_without_advisory_lock.rb | 5 +++++ test/models/list.rb | 1 + test/support/active_record.rb | 8 ++++++++ test/test_positioning.rb | 25 +++++++++++++++++++++++ 6 files changed, 72 insertions(+), 7 deletions(-) create mode 100644 test/models/item_without_advisory_lock.rb diff --git a/README.md b/README.md index 792c6b1..d49f3da 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,10 @@ positioned on: :type belongs_to :list belongs_to :category positioned on: [:list, :category, :enabled] + +# If Advisory Lock does not fit with your environment, you can disable it entirely by passing the 'use_advisory_lock' flag as false +belongs_to :list +positioned on: :list, use_advisory_lock: false ``` ### Manipulating Positioning @@ -243,6 +247,24 @@ 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 + +You can opt out of Advisory Lock by setting `use_advisory_lock` as `false` when declaring positioning: + +```ruby +belongs_to :list +positioned on: :list, use_advisory_lock: false +``` + +When disabling Advisory Lock, you still want to avoid race conditions. There are multiple ways to achieve that, one of them is locking the parent record manually: + +```ruby +list = List.create(name: "My list") +list.with_lock do + list.items.create(name: "My list item") +end +``` + ## 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. diff --git a/lib/positioning.rb b/lib/positioning.rb index 32860cb..b486f61 100644 --- a/lib/positioning.rb +++ b/lib/positioning.rb @@ -18,7 +18,7 @@ def positioning_columns @positioning_columns ||= {} end - def positioned(on: [], column: :position) + def positioned(on: [], column: :position, use_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 use_advisory_lock + advisory_lock = AdvisoryLock.new(base_class, column) - before_create advisory_lock - before_update advisory_lock - before_destroy advisory_lock + before_create advisory_lock + before_update advisory_lock + before_destroy advisory_lock + 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 use_advisory_lock + after_commit advisory_lock + after_rollback advisory_lock + end end end end diff --git a/test/models/item_without_advisory_lock.rb b/test/models/item_without_advisory_lock.rb new file mode 100644 index 0000000..b7f4b5c --- /dev/null +++ b/test/models/item_without_advisory_lock.rb @@ -0,0 +1,5 @@ +class ItemWithoutAdvisoryLock < ActiveRecord::Base + belongs_to :list + + positioned on: :list, use_advisory_lock: false +end diff --git a/test/models/list.rb b/test/models/list.rb index 8f01ee7..118efda 100644 --- a/test/models/list.rb +++ b/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 diff --git a/test/support/active_record.rb b/test/support/active_record.rb index 41d09d6..6648e10 100644 --- a/test/support/active_record.rb +++ b/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 diff --git a/test/test_positioning.rb b/test/test_positioning.rb index b535fea..7dda040 100644 --- a/test/test_positioning.rb +++ b/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,30 @@ 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 = [] + + 10.times do + threads = 20.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 end class TestPositioningMechanisms < Minitest::Test From dd3b1e40ef9bf2fb4e15e6f924eafd620a2af0d9 Mon Sep 17 00:00:00 2001 From: Joao Marcos Date: Fri, 5 Jul 2024 00:09:23 -0300 Subject: [PATCH 2/4] Update README --- README.md | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index d49f3da..c9b4f0d 100644 --- a/README.md +++ b/README.md @@ -67,9 +67,9 @@ belongs_to :list belongs_to :category positioned on: [:list, :category, :enabled] -# If Advisory Lock does not fit with your environment, you can disable it entirely by passing the 'use_advisory_lock' flag as false +# 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, use_advisory_lock: false +positioned on: :list, advisory_lock: false ``` ### Manipulating Positioning @@ -249,20 +249,29 @@ If you have any concerns or improvements please file a GitHub issue. ### Opting out of Advisory Lock -You can opt out of Advisory Lock by setting `use_advisory_lock` as `false` when declaring positioning: +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 -belongs_to :list -positioned on: :list, use_advisory_lock: false +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 ``` -When disabling Advisory Lock, you still want to avoid race conditions. There are multiple ways to achieve that, one of them is locking the parent record manually: +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 -list = List.create(name: "My list") -list.with_lock do - list.items.create(name: "My list item") -end +belongs_to :list +positioned on: :list, advisory_lock: false ``` ## Development From e27d85361d02073bd029064e4c34315cf7f29598 Mon Sep 17 00:00:00 2001 From: Joao Marcos Date: Fri, 5 Jul 2024 00:14:00 -0300 Subject: [PATCH 3/4] Rename option --- lib/positioning.rb | 18 +++++++++--------- test/models/item_without_advisory_lock.rb | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/positioning.rb b/lib/positioning.rb index b486f61..90bf598 100644 --- a/lib/positioning.rb +++ b/lib/positioning.rb @@ -18,7 +18,7 @@ def positioning_columns @positioning_columns ||= {} end - def positioned(on: [], column: :position, use_advisory_lock: true) + 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,21 +43,21 @@ def positioned(on: [], column: :position, use_advisory_lock: true) super(position) end - if use_advisory_lock - 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 } - if use_advisory_lock - after_commit advisory_lock - after_rollback advisory_lock + if advisory_lock + after_commit advisory_lock_callback + after_rollback advisory_lock_callback end end end diff --git a/test/models/item_without_advisory_lock.rb b/test/models/item_without_advisory_lock.rb index b7f4b5c..d57e70f 100644 --- a/test/models/item_without_advisory_lock.rb +++ b/test/models/item_without_advisory_lock.rb @@ -1,5 +1,5 @@ class ItemWithoutAdvisoryLock < ActiveRecord::Base belongs_to :list - positioned on: :list, use_advisory_lock: false + positioned on: :list, advisory_lock: false end From 3fc4b1b7af1220b4b125a7e6484d0c73775d3c53 Mon Sep 17 00:00:00 2001 From: Joao Marcos Date: Fri, 5 Jul 2024 00:15:37 -0300 Subject: [PATCH 4/4] Add test cases for update and destroy --- test/test_positioning.rb | 63 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 61 insertions(+), 2 deletions(-) diff --git a/test/test_positioning.rb b/test/test_positioning.rb index 7dda040..9ae097c 100644 --- a/test/test_positioning.rb +++ b/test/test_positioning.rb @@ -55,8 +55,8 @@ def test_no_duplicate_row_values_when_advisory_lock_is_disabled_and_parent_recor list = List.create name: "List" items = [] - 10.times do - threads = 20.times.map do + 2.times do + threads = 3.times.map do Thread.new do ActiveRecord::Base.connection_pool.with_connection do list.with_lock do @@ -72,6 +72,65 @@ def test_no_duplicate_row_values_when_advisory_lock_is_disabled_and_parent_recor 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