diff --git a/README.md b/README.md index 4c077ab..7cd849a 100644 --- a/README.md +++ b/README.md @@ -319,6 +319,7 @@ When you include ```has_closure_tree``` in your model, you can provide a hash to * ```nil``` does nothing with descendant nodes * ```:name_column``` used by #```find_or_create_by_path```, #```find_by_path```, and ```ancestry_path``` instance methods. This is primarily useful if the model only has one required field (like a "tag"). * ```:order``` used to set up [deterministic ordering](#deterministic-ordering) +* ```:scope``` restricts root nodes and sibling ordering to specific columns. Can be a single symbol or an array of symbols. Example: ```scope: :user_id``` or ```scope: [:user_id, :group_id]```. This ensures that root nodes and siblings are scoped correctly when reordering. See [Ordering Roots](#ordering-roots) for more details. * ```:touch``` delegates to the `belongs_to` annotation for the parent, so `touch`ing cascades to all children (the performance of this for deep trees isn't currently optimal). ## Accessing Data @@ -491,9 +492,30 @@ table. So for instance if you have 5 nodes with no parent, they will be ordered If your model represents many separate trees and you have a lot of records, this can cause performance problems, and doesn't really make much sense. -You can disable this default behavior by passing `dont_order_roots: true` as an option to your delcaration: +You can scope root nodes and sibling ordering by passing the `scope` option: +```ruby +class Block < ApplicationRecord + has_closure_tree order: 'sort_order', numeric_order: true, scope: :user_id +end ``` + +This ensures that: +* Root nodes are scoped by the specified columns. You can filter roots like: ```Block.roots.where(user_id: 123)``` +* Sibling reordering only affects nodes with the same scope values +* Children reordering respects the parent's scope values + +You can also scope by multiple columns: + +```ruby +class Block < ApplicationRecord + has_closure_tree order: 'sort_order', numeric_order: true, scope: [:user_id, :group_id] +end +``` + +Alternatively, you can disable root ordering entirely by passing `dont_order_roots: true`: + +```ruby has_closure_tree order: 'sort_order', numeric_order: true, dont_order_roots: true ``` diff --git a/lib/closure_tree/has_closure_tree.rb b/lib/closure_tree/has_closure_tree.rb index 5550cc7..24dc138 100644 --- a/lib/closure_tree/has_closure_tree.rb +++ b/lib/closure_tree/has_closure_tree.rb @@ -14,7 +14,8 @@ def has_closure_tree(options = {}) :numeric_order, :touch, :with_advisory_lock, - :advisory_lock_name + :advisory_lock_name, + :scope ) class_attribute :_ct diff --git a/lib/closure_tree/model.rb b/lib/closure_tree/model.rb index 98ba50f..4f1f279 100644 --- a/lib/closure_tree/model.rb +++ b/lib/closure_tree/model.rb @@ -80,7 +80,9 @@ def descendant_ids end def self_and_siblings - _ct.scope_with_order(_ct.base_class.where(_ct.parent_column_sym => _ct_parent_id)) + scope = _ct.base_class.where(_ct.parent_column_sym => _ct_parent_id) + scope = _ct.apply_scope_conditions(scope, self) + _ct.scope_with_order(scope) end def siblings diff --git a/lib/closure_tree/numeric_deterministic_ordering.rb b/lib/closure_tree/numeric_deterministic_ordering.rb index 1ee4898..ab49f32 100644 --- a/lib/closure_tree/numeric_deterministic_ordering.rb +++ b/lib/closure_tree/numeric_deterministic_ordering.rb @@ -15,16 +15,19 @@ def _ct_reorder_prior_siblings_if_parent_changed return unless saved_change_to_attribute?(_ct.parent_column_name) && !@was_new_record was_parent_id = attribute_before_last_save(_ct.parent_column_name) - _ct.reorder_with_parent_id(was_parent_id) + scope_conditions = _ct.scope_values_from_instance(self) + _ct.reorder_with_parent_id(was_parent_id, nil, scope_conditions) end def _ct_reorder_siblings(minimum_sort_order_value = nil) - _ct.reorder_with_parent_id(_ct_parent_id, minimum_sort_order_value) + scope_conditions = _ct.scope_values_from_instance(self) + _ct.reorder_with_parent_id(_ct_parent_id, minimum_sort_order_value, scope_conditions) reload unless destroyed? end def _ct_reorder_children(minimum_sort_order_value = nil) - _ct.reorder_with_parent_id(_ct_id, minimum_sort_order_value) + scope_conditions = _ct.scope_values_from_instance(self) + _ct.reorder_with_parent_id(_ct_id, minimum_sort_order_value, scope_conditions) end def self_and_descendants_preordered diff --git a/lib/closure_tree/numeric_order_support.rb b/lib/closure_tree/numeric_order_support.rb index 46b4100..4565f5a 100644 --- a/lib/closure_tree/numeric_order_support.rb +++ b/lib/closure_tree/numeric_order_support.rb @@ -14,7 +14,7 @@ def self.adapter_for_connection(connection) end module MysqlAdapter - def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) + def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil, scope_conditions = {}) return if parent_id.nil? && dont_order_roots min_where = if minimum_sort_order_value @@ -22,18 +22,21 @@ def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) else '' end + + scope_where = build_scope_where_clause(scope_conditions) + connection.execute 'SET @i = 0' connection.execute <<-SQL.squish UPDATE #{quoted_table_name} SET #{quoted_order_column} = (@i := @i + 1) + #{minimum_sort_order_value.to_i - 1} - WHERE #{where_eq(parent_column_name, parent_id)} #{min_where} + WHERE #{where_eq(parent_column_name, parent_id)} #{min_where}#{scope_where} ORDER BY #{nulls_last_order_by} SQL end end module PostgreSQLAdapter - def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) + def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil, scope_conditions = {}) return if parent_id.nil? && dont_order_roots min_where = if minimum_sort_order_value @@ -41,13 +44,16 @@ def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) else '' end + + scope_where = build_scope_where_clause(scope_conditions) + connection.execute <<-SQL.squish UPDATE #{quoted_table_name} SET #{quoted_order_column(false)} = t.seq + #{minimum_sort_order_value.to_i - 1} FROM ( SELECT #{quoted_id_column_name} AS id, row_number() OVER(ORDER BY #{order_by}) AS seq FROM #{quoted_table_name} - WHERE #{where_eq(parent_column_name, parent_id)} #{min_where} + WHERE #{where_eq(parent_column_name, parent_id)} #{min_where}#{scope_where} ) AS t WHERE #{quoted_table_name}.#{quoted_id_column_name} = t.id and #{quoted_table_name}.#{quoted_order_column(false)} is distinct from t.seq + #{minimum_sort_order_value.to_i - 1} @@ -60,12 +66,13 @@ def rows_updated(result) end module GenericAdapter - def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) + def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil, scope_conditions = {}) return if parent_id.nil? && dont_order_roots scope = model_class .where(parent_column_sym => parent_id) .order(nulls_last_order_by) + scope = scope.where(scope_conditions) if scope_conditions.any? scope = scope.where("#{quoted_order_column} >= #{minimum_sort_order_value}") if minimum_sort_order_value scope.each_with_index do |ea, idx| ea.update_order_value(idx + minimum_sort_order_value.to_i) diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index 3a0a8ae..87a4687 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -23,6 +23,13 @@ def initialize(model_class, options) }.merge(options) raise ArgumentError, "name_column can't be 'path'" if options[:name_column] == 'path' + if options[:scope] + scope_option = options[:scope] + unless scope_option.is_a?(Symbol) || (scope_option.is_a?(Array) && scope_option.all? { |item| item.is_a?(Symbol) }) + raise ArgumentError, "scope option must be a Symbol or an Array of Symbols (e.g., :user_id or [:user_id, :group_id])" + end + end + return unless order_is_numeric? extend NumericOrderSupport.adapter_for_connection(connection) @@ -109,6 +116,22 @@ def where_eq(column_name, value) end end + # Builds SQL WHERE conditions for scope columns + # Returns a string that can be appended to a WHERE clause + def build_scope_where_clause(scope_conditions) + return '' unless scope_conditions.is_a?(Hash) && scope_conditions.any? + + conditions = scope_conditions.map do |column, value| + if value.nil? + "#{connection.quote_column_name(column.to_s)} IS NULL" + else + "#{connection.quote_column_name(column.to_s)} = #{quoted_value(value)}" + end + end + + " AND #{conditions.join(' AND ')}" + end + def with_advisory_lock(&block) if options[:with_advisory_lock] && connection.supports_advisory_locks? && model_class.respond_to?(:with_advisory_lock) model_class.with_advisory_lock(advisory_lock_name) do @@ -170,5 +193,49 @@ def create(model_class, attributes) def create!(model_class, attributes) create(model_class, attributes).tap(&:save!) end + + def scope_columns + return [] unless options[:scope] + + scope_option = options[:scope] + + case scope_option + when Symbol + [scope_option] + when Array + scope_option.select { |item| item.is_a?(Symbol) } + else + [] + end + end + + def scope_values_from_instance(instance) + return {} unless options[:scope] && instance + + scope_option = options[:scope] + scope_hash = {} + + case scope_option + when Symbol + value = instance.read_attribute(scope_option) + scope_hash[scope_option] = value unless value.nil? + when Array + scope_option.each do |item| + if item.is_a?(Symbol) + value = instance.read_attribute(item) + scope_hash[item] = value unless value.nil? + end + end + end + + scope_hash + end + + def apply_scope_conditions(scope, instance = nil) + return scope unless options[:scope] && instance + + scope_values = scope_values_from_instance(instance) + scope_values.any? ? scope.where(scope_values) : scope + end end end diff --git a/test/closure_tree/scope_test.rb b/test/closure_tree/scope_test.rb new file mode 100644 index 0000000..8101f61 --- /dev/null +++ b/test/closure_tree/scope_test.rb @@ -0,0 +1,179 @@ +# frozen_string_literal: true + +require 'test_helper' + +class ScopeTest < ActiveSupport::TestCase + def setup + ScopedItem.delete_all + ScopedItemHierarchy.delete_all + MultiScopedItem.delete_all + end + + def test_roots_with_single_scope + root1 = ScopedItem.create!(name: 'root1', user_id: 1) + root2 = ScopedItem.create!(name: 'root2', user_id: 1) + root3 = ScopedItem.create!(name: 'root3', user_id: 2) + + scoped_roots = ScopedItem.roots.where(user_id: 1) + assert_equal 2, scoped_roots.count + assert_includes scoped_roots, root1 + assert_includes scoped_roots, root2 + refute_includes scoped_roots, root3 + end + + def test_roots_with_multiple_scope + root1 = MultiScopedItem.create!(name: 'root1', user_id: 1, group_id: 10) + root2 = MultiScopedItem.create!(name: 'root2', user_id: 1, group_id: 10) + root3 = MultiScopedItem.create!(name: 'root3', user_id: 1, group_id: 20) + root4 = MultiScopedItem.create!(name: 'root4', user_id: 2, group_id: 10) + + scoped_roots = MultiScopedItem.roots.where(user_id: 1, group_id: 10) + assert_equal 2, scoped_roots.count + assert_includes scoped_roots, root1 + assert_includes scoped_roots, root2 + refute_includes scoped_roots, root3 + refute_includes scoped_roots, root4 + end + + def test_siblings_with_scope + parent = ScopedItem.create!(name: 'parent', user_id: 1) + child1 = parent.children.create!(name: 'child1', user_id: 1) + child2 = parent.children.create!(name: 'child2', user_id: 1) + child3 = parent.children.create!(name: 'child3', user_id: 2) + + siblings = child1.siblings + assert_equal 1, siblings.count + assert_includes siblings, child2 + refute_includes siblings, child3 + end + + def test_siblings_with_multiple_scope + parent = MultiScopedItem.create!(name: 'parent', user_id: 1, group_id: 10) + child1 = parent.children.create!(name: 'child1', user_id: 1, group_id: 10) + child2 = parent.children.create!(name: 'child2', user_id: 1, group_id: 10) + child3 = parent.children.create!(name: 'child3', user_id: 1, group_id: 20) + child4 = parent.children.create!(name: 'child4', user_id: 2, group_id: 10) + + siblings = child1.siblings + assert_equal 1, siblings.count + assert_includes siblings, child2 + refute_includes siblings, child3 + refute_includes siblings, child4 + end + + def test_reordering_siblings_with_scope + parent = ScopedItem.create!(name: 'parent', user_id: 1) + child1 = parent.children.create!(name: 'child1', user_id: 1) + child2 = parent.children.create!(name: 'child2', user_id: 1) + child3 = parent.children.create!(name: 'child3', user_id: 2) + + child1._ct_reorder_siblings + + child1.reload + child2.reload + child3.reload + + assert_equal 0, child1.order_value + assert_equal 1, child2.order_value + assert_equal 0, child3.order_value + end + + def test_reordering_siblings_with_multiple_scope + parent = MultiScopedItem.create!(name: 'parent', user_id: 1, group_id: 10) + child1 = parent.children.create!(name: 'child1', user_id: 1, group_id: 10) + child2 = parent.children.create!(name: 'child2', user_id: 1, group_id: 10) + child3 = parent.children.create!(name: 'child3', user_id: 1, group_id: 20) + child4 = parent.children.create!(name: 'child4', user_id: 2, group_id: 10) + + child1._ct_reorder_siblings + + child1.reload + child2.reload + child3.reload + child4.reload + + assert_equal 0, child1.order_value + assert_equal 1, child2.order_value + assert_equal 0, child3.order_value + assert_equal 0, child4.order_value + end + + def test_reordering_children_with_scope + parent1 = ScopedItem.create!(name: 'parent1', user_id: 1) + parent2 = ScopedItem.create!(name: 'parent2', user_id: 2) + + child1 = parent1.children.create!(name: 'child1', user_id: 1) + child2 = parent1.children.create!(name: 'child2', user_id: 1) + child3 = parent1.children.create!(name: 'child3', user_id: 1) + + parent1._ct_reorder_children + + child1.reload + child2.reload + child3.reload + + assert_equal 0, child1.order_value + assert_equal 1, child2.order_value + assert_equal 2, child3.order_value + end + + def test_reordering_children_with_multiple_scope + parent1 = MultiScopedItem.create!(name: 'parent1', user_id: 1, group_id: 10) + parent2 = MultiScopedItem.create!(name: 'parent2', user_id: 1, group_id: 20) + + child1 = parent1.children.create!(name: 'child1', user_id: 1, group_id: 10) + child2 = parent1.children.create!(name: 'child2', user_id: 1, group_id: 10) + child3 = parent1.children.create!(name: 'child3', user_id: 1, group_id: 20) + child4 = parent1.children.create!(name: 'child4', user_id: 2, group_id: 10) + + parent1._ct_reorder_children + + child1.reload + child2.reload + child3.reload + child4.reload + + assert_equal 0, child1.order_value + assert_equal 1, child2.order_value + assert_equal 0, child3.order_value + assert_equal 0, child4.order_value + end + + def test_reordering_children_excludes_different_scope + parent1 = ScopedItem.create!(name: 'parent1', user_id: 1) + + child1 = parent1.children.create!(name: 'child1', user_id: 1) + child2 = parent1.children.create!(name: 'child2', user_id: 1) + child3 = parent1.children.create!(name: 'child3', user_id: 2) + + initial_order = child3.order_value + + parent1._ct_reorder_children + + child1.reload + child2.reload + child3.reload + + assert_equal 0, child1.order_value + assert_equal 1, child2.order_value + assert_equal initial_order, child3.order_value, 'child3 with different scope should not be reordered' + end + + def test_scope_values_from_instance + instance = ScopedItem.new(user_id: 123) + scope_values = instance._ct.scope_values_from_instance(instance) + assert_equal({ user_id: 123 }, scope_values) + end + + def test_scope_values_from_instance_multiple_columns + instance = MultiScopedItem.new(user_id: 123, group_id: 456) + scope_values = instance._ct.scope_values_from_instance(instance) + assert_equal({ user_id: 123, group_id: 456 }, scope_values) + end + + def test_scope_columns_method + assert_equal [:user_id], ScopedItem._ct.scope_columns + assert_equal [:user_id, :group_id], MultiScopedItem._ct.scope_columns + end +end + diff --git a/test/dummy/app/models/multi_scoped_item.rb b/test/dummy/app/models/multi_scoped_item.rb new file mode 100644 index 0000000..8959efb --- /dev/null +++ b/test/dummy/app/models/multi_scoped_item.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class MultiScopedItem < ApplicationRecord + self.table_name = 'scoped_items' + has_closure_tree order: :sort_order, numeric_order: true, scope: [:user_id, :group_id] +end + diff --git a/test/dummy/app/models/scoped_item.rb b/test/dummy/app/models/scoped_item.rb new file mode 100644 index 0000000..93f05bf --- /dev/null +++ b/test/dummy/app/models/scoped_item.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +class ScopedItem < ApplicationRecord + has_closure_tree order: :sort_order, numeric_order: true, scope: :user_id +end + diff --git a/test/dummy/db/schema.rb b/test/dummy/db/schema.rb index 8884f4f..f5a27bd 100644 --- a/test/dummy/db/schema.rb +++ b/test/dummy/db/schema.rb @@ -201,4 +201,23 @@ add_index 'sqlite_tag_hierarchies', %i[ancestor_id descendant_id generations], unique: true, name: 'sqlite_tag_anc_desc_idx' add_index 'sqlite_tag_hierarchies', [:descendant_id], name: 'sqlite_tag_desc_idx' + + create_table 'scoped_items' do |t| + t.string 'name' + t.references 'parent' + t.integer 'sort_order' + t.integer 'user_id' + t.integer 'group_id' + t.timestamps null: false + end + + create_table 'scoped_item_hierarchies', id: false do |t| + t.references 'ancestor', null: false + t.references 'descendant', null: false + t.integer 'generations', null: false + end + + add_index 'scoped_item_hierarchies', %i[ancestor_id descendant_id generations], unique: true, + name: 'scoped_item_anc_desc_idx' + add_index 'scoped_item_hierarchies', [:descendant_id], name: 'scoped_item_desc_idx' end