From abac407e057ae104b05314cf21c707bb9afd00b0 Mon Sep 17 00:00:00 2001 From: zakky21 Date: Wed, 8 Apr 2026 16:47:22 +0900 Subject: [PATCH] feat: scope advisory lock names by scope column values When `scope:` option is configured (e.g., `scope: :company_id`), advisory lock names now include the scope values from the instance. This prevents unnecessary lock contention across different tenants in multi-tenant environments. Previously, all operations shared a single lock per model class (`ct_{CRC32(class_name)}`). Now, scoped models use per-scope locks (e.g., `ct_{CRC32(class_name)}_{company_id}`), allowing concurrent operations on different tenants. - Add `advisory_lock_name_for(instance)` to SupportAttributes - Update `with_advisory_lock` to accept an optional instance argument - Pass `self` at all instance-method call sites (5 locations) - Class-method call sites pass `nil` (fallback to model-wide lock) - Fully backward compatible: no change for unscoped models or existing callers without the instance argument --- lib/closure_tree/finders.rb | 2 +- lib/closure_tree/hierarchy_maintenance.rb | 6 +-- .../numeric_deterministic_ordering.rb | 2 +- lib/closure_tree/support.rb | 4 +- lib/closure_tree/support_attributes.rb | 11 +++++ .../closure_tree/scoped_advisory_lock_test.rb | 43 +++++++++++++++++++ 6 files changed, 61 insertions(+), 7 deletions(-) create mode 100644 test/closure_tree/scoped_advisory_lock_test.rb diff --git a/lib/closure_tree/finders.rb b/lib/closure_tree/finders.rb index eb7a6968..c9e57fea 100644 --- a/lib/closure_tree/finders.rb +++ b/lib/closure_tree/finders.rb @@ -20,7 +20,7 @@ def find_or_create_by_path(path, attributes = {}) return found if found attrs = subpath.shift - _ct.with_advisory_lock do + _ct.with_advisory_lock(self) do # shenanigans because children.create is bound to the superclass # (in the case of polymorphism): child = children.where(attrs).first || begin diff --git a/lib/closure_tree/hierarchy_maintenance.rb b/lib/closure_tree/hierarchy_maintenance.rb index 951fc141..caa43acf 100644 --- a/lib/closure_tree/hierarchy_maintenance.rb +++ b/lib/closure_tree/hierarchy_maintenance.rb @@ -64,7 +64,7 @@ def _ct_after_save end def _ct_before_destroy - _ct.with_advisory_lock do + _ct.with_advisory_lock(self) do _ct_adopt_children_to_grandparent if _ct.options[:dependent] == :adopt delete_hierarchy_references self.class.find(id).children.find_each(&:rebuild!) if _ct.options[:dependent] == :nullify @@ -86,7 +86,7 @@ def _ct_before_destroy end def rebuild!(called_by_rebuild = false) - _ct.with_advisory_lock do + _ct.with_advisory_lock(self) do delete_hierarchy_references unless (defined? @was_new_record) && @was_new_record hierarchy_class.create!(ancestor: self, descendant: self, generations: 0) unless root? @@ -112,7 +112,7 @@ def rebuild!(called_by_rebuild = false) end def delete_hierarchy_references - _ct.with_advisory_lock do + _ct.with_advisory_lock(self) do # The crazy double-wrapped sub-subselect works around MySQL's limitation of subselects on the same table that is being mutated. # It shouldn't affect performance of postgresql. # See http://dev.mysql.com/doc/refman/5.0/en/subquery-errors.html diff --git a/lib/closure_tree/numeric_deterministic_ordering.rb b/lib/closure_tree/numeric_deterministic_ordering.rb index e0e1644f..65737695 100644 --- a/lib/closure_tree/numeric_deterministic_ordering.rb +++ b/lib/closure_tree/numeric_deterministic_ordering.rb @@ -162,7 +162,7 @@ def add_sibling(sibling, add_after = true) # Make sure self isn't dirty, because we're going to call reload: save - _ct.with_advisory_lock do + _ct.with_advisory_lock(self) do prior_sibling_parent = sibling.parent sibling.order_value = order_value diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index b8fa91d2..ea74c88c 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -157,10 +157,10 @@ def build_scope_where_clause(scope_conditions) " AND #{conditions.join(' AND ')}" end - def with_advisory_lock(&block) + def with_advisory_lock(instance = nil, &block) lock_method = options[:advisory_lock_timeout_seconds].present? ? :with_advisory_lock! : :with_advisory_lock if options[:with_advisory_lock] && connection.supports_advisory_locks? && model_class.respond_to?(lock_method) - model_class.public_send(lock_method, advisory_lock_name, advisory_lock_options) do + model_class.public_send(lock_method, advisory_lock_name_for(instance), advisory_lock_options) do transaction(&block) end else diff --git a/lib/closure_tree/support_attributes.rb b/lib/closure_tree/support_attributes.rb index a40ed573..a44538e2 100644 --- a/lib/closure_tree/support_attributes.rb +++ b/lib/closure_tree/support_attributes.rb @@ -34,6 +34,17 @@ def advisory_lock_name end end + def advisory_lock_name_for(instance = nil) + base = advisory_lock_name + return base unless instance && options[:scope] + + scope_values = scope_values_from_instance(instance) + return base if scope_values.empty? + + suffix = scope_values.values.map(&:to_s).join('_') + "#{base}_#{suffix}" + end + def advisory_lock_options { timeout_seconds: options[:advisory_lock_timeout_seconds] }.compact end diff --git a/test/closure_tree/scoped_advisory_lock_test.rb b/test/closure_tree/scoped_advisory_lock_test.rb new file mode 100644 index 00000000..4eabbe6c --- /dev/null +++ b/test/closure_tree/scoped_advisory_lock_test.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'test_helper' + +# Test that advisory lock names include scope values when scope option is configured +class ScopedAdvisoryLockTest < ActiveSupport::TestCase + def test_advisory_lock_name_for_with_scope_includes_scope_value + item = ScopedItem.new(user_id: 42) + base = item._ct.advisory_lock_name + assert_equal "#{base}_42", item._ct.advisory_lock_name_for(item) + end + + def test_advisory_lock_name_for_different_scope_values_differ + item_a = ScopedItem.new(user_id: 1) + item_b = ScopedItem.new(user_id: 2) + assert_not_equal item_a._ct.advisory_lock_name_for(item_a), + item_b._ct.advisory_lock_name_for(item_b) + end + + def test_advisory_lock_name_for_same_scope_values_match + item_a = ScopedItem.new(user_id: 7) + item_b = ScopedItem.new(user_id: 7) + assert_equal item_a._ct.advisory_lock_name_for(item_a), + item_b._ct.advisory_lock_name_for(item_b) + end + + def test_advisory_lock_name_for_without_scope_returns_base + tag = Tag.new + base = tag._ct.advisory_lock_name + assert_equal base, tag._ct.advisory_lock_name_for(tag) + end + + def test_advisory_lock_name_for_nil_instance_returns_base + item = ScopedItem.new(user_id: 1) + assert_equal item._ct.advisory_lock_name, item._ct.advisory_lock_name_for(nil) + end + + def test_advisory_lock_name_for_multi_scope + item = MultiScopedItem.new(user_id: 10, group_id: 20) + base = item._ct.advisory_lock_name + assert_equal "#{base}_10_20", item._ct.advisory_lock_name_for(item) + end +end