diff --git a/README.md b/README.md index 30d36a5d..7909d0c2 100644 --- a/README.md +++ b/README.md @@ -489,6 +489,25 @@ root.reload.children.pluck(:name) => ["b", "c", "a"] ``` +### Ordering Roots + +With numeric ordering, root nodes are, by default, assigned order values globally across the whole database +table. So for instance if you have 5 nodes with no parent, they will be ordered 0 through 4 by default. +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: + +``` +has_closure_tree order: 'sort_order', numeric_order: true, dont_order_roots: true +``` + +In this case, calling `prepend_sibling` and `append_sibling` on a root node or calling +`roots_and_descendants_preordered` on the model will raise a `RootOrderingDisabledError`. + +The `dont_order_roots` option will be ignored unless `numeric_order` is set to true. + + ## Concurrency Several methods, especially ```#rebuild``` and ```#find_or_create_by_path```, cannot run concurrently correctly. diff --git a/lib/closure_tree/has_closure_tree.rb b/lib/closure_tree/has_closure_tree.rb index 7e08a835..b0bc5b1a 100644 --- a/lib/closure_tree/has_closure_tree.rb +++ b/lib/closure_tree/has_closure_tree.rb @@ -8,6 +8,7 @@ def has_closure_tree(options = {}) :hierarchy_table_name, :name_column, :order, + :dont_order_roots, :numeric_order, :touch, :with_advisory_lock diff --git a/lib/closure_tree/has_closure_tree_root.rb b/lib/closure_tree/has_closure_tree_root.rb index 5aa1e275..8e9faeec 100644 --- a/lib/closure_tree/has_closure_tree_root.rb +++ b/lib/closure_tree/has_closure_tree_root.rb @@ -1,5 +1,6 @@ module ClosureTree class MultipleRootError < StandardError; end + class RootOrderingDisabledError < StandardError; end module HasClosureTreeRoot diff --git a/lib/closure_tree/numeric_deterministic_ordering.rb b/lib/closure_tree/numeric_deterministic_ordering.rb index 5ed360eb..c65732c0 100644 --- a/lib/closure_tree/numeric_deterministic_ordering.rb +++ b/lib/closure_tree/numeric_deterministic_ordering.rb @@ -65,10 +65,15 @@ def _ct_sum_order_by(node = nil) node_score = "(1 + anc.#{_ct.quoted_order_column(false)}) * " + "power(#{h['total_descendants']}, #{h['max_depth'].to_i + 1} - #{depth_column})" - Arel.sql("SUM(#{node_score})") + # We want the NULLs to be first in case we are not ordering roots and they have NULL order. + Arel.sql("SUM(#{node_score}) IS NULL DESC, SUM(#{node_score})") end def roots_and_descendants_preordered + if _ct.dont_order_roots + raise ClosureTree::RootOrderingDisabledError.new("Root ordering is disabled on this model") + end + join_sql = <<-SQL.strip_heredoc JOIN #{_ct.quoted_hierarchy_table_name} anc_hier ON anc_hier.descendant_id = #{_ct.quoted_table_name}.#{_ct.quoted_id_column_name} @@ -113,6 +118,10 @@ def prepend_sibling(sibling_node) def add_sibling(sibling, add_after = true) fail "can't add self as sibling" if self == sibling + if _ct.dont_order_roots && parent.nil? + raise ClosureTree::RootOrderingDisabledError.new("Root ordering is disabled on this model") + end + # Make sure self isn't dirty, because we're going to call reload: save diff --git a/lib/closure_tree/numeric_order_support.rb b/lib/closure_tree/numeric_order_support.rb index 46909670..606f5371 100644 --- a/lib/closure_tree/numeric_order_support.rb +++ b/lib/closure_tree/numeric_order_support.rb @@ -14,6 +14,7 @@ def self.adapter_for_connection(connection) module MysqlAdapter def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) + return if parent_id.nil? && dont_order_roots min_where = if minimum_sort_order_value "AND #{quoted_order_column} >= #{minimum_sort_order_value}" else @@ -31,6 +32,7 @@ def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) module PostgreSQLAdapter def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) + return if parent_id.nil? && dont_order_roots min_where = if minimum_sort_order_value "AND #{quoted_order_column} >= #{minimum_sort_order_value}" else @@ -56,6 +58,7 @@ def rows_updated(result) module GenericAdapter def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) + return if parent_id.nil? && dont_order_roots scope = model_class. where(parent_column_sym => parent_id). order(nulls_last_order_by) diff --git a/lib/closure_tree/support_attributes.rb b/lib/closure_tree/support_attributes.rb index cd95b944..d924ecde 100644 --- a/lib/closure_tree/support_attributes.rb +++ b/lib/closure_tree/support_attributes.rb @@ -75,6 +75,10 @@ def order_by options[:order] end + def dont_order_roots + options[:dont_order_roots] || false + end + def nulls_last_order_by "-#{quoted_order_column} #{order_by_order(reverse = true)}" end diff --git a/spec/db/database.yml b/spec/db/database.yml index 023e7971..12e144d9 100644 --- a/spec/db/database.yml +++ b/spec/db/database.yml @@ -13,7 +13,7 @@ sqlite: postgresql: <<: *common adapter: postgresql - username: postgres + username: tomsmyth mysql: <<: *common diff --git a/spec/db/models.rb b/spec/db/models.rb index 939c13dc..9927af41 100644 --- a/spec/db/models.rb +++ b/spec/db/models.rb @@ -99,6 +99,21 @@ class DateLabel < Label class DirectoryLabel < Label end +class LabelWithoutRootOrdering < ActiveRecord::Base + # make sure order doesn't matter + acts_as_tree :order => :column_whereby_ordering_is_inferred, # <- symbol, and not "sort_order" + :numeric_order => true, + :dont_order_roots => true, + :parent_column_name => "mother_id", + :hierarchy_table_name => "label_hierarchies" + + self.table_name = "#{table_name_prefix}labels#{table_name_suffix}" + + def to_s + "#{self.class}: #{name}" + end +end + class CuisineType < ActiveRecord::Base acts_as_tree end diff --git a/spec/label_spec.rb b/spec/label_spec.rb index cd99f47f..6c824c34 100644 --- a/spec/label_spec.rb +++ b/spec/label_spec.rb @@ -289,6 +289,50 @@ def roots_name_and_order end end + context "doesn't order roots when requested" do + before :each do + @root1 = LabelWithoutRootOrdering.create!(:name => 'root1') + @root2 = LabelWithoutRootOrdering.create!(:name => 'root2') + @a, @b, @c, @d, @e = ('a'..'e').map { |ea| LabelWithoutRootOrdering.new(:name => ea) } + @root1.children << @a + @root1.append_child(@c) + @root1.prepend_child(@d) + + # Reload is needed here and below because order values may have been adjusted in the DB during + # prepend_child, append_sibling, etc. + [@a, @c, @d].each(&:reload) + + @a.append_sibling(@b) + [@a, @c, @d, @b].each(&:reload) + @d.prepend_sibling(@e) + end + + it 'order_values properly' do + expect(@root1.reload.order_value).to be_nil + orders_and_names = @root1.children.reload.map { |ea| [ea.name, ea.order_value] } + expect(orders_and_names).to eq([['e', 0], ['d', 1], ['a', 2], ['b', 3], ['c', 4]]) + end + + it 'raises on prepending and appending to root' do + expect { @root1.prepend_sibling(@f) }.to raise_error(ClosureTree::RootOrderingDisabledError) + expect { @root1.append_sibling(@f) }.to raise_error(ClosureTree::RootOrderingDisabledError) + end + + it 'returns empty array for siblings_before and after' do + expect(@root1.siblings_before).to eq([]) + expect(@root1.siblings_after).to eq([]) + end + + it 'returns expected result for self_and_descendants_preordered' do + expect(@root1.self_and_descendants_preordered.to_a).to eq([@root1, @e, @d, @a, @b, @c]) + end unless sqlite? # sqlite doesn't have a power function. + + it 'raises on roots_and_descendants_preordered' do + expect { LabelWithoutRootOrdering.roots_and_descendants_preordered }.to raise_error( + ClosureTree::RootOrderingDisabledError) + end + end + describe 'code in the readme' do it 'creates STI label hierarchies' do child = Label.find_or_create_by_path([ @@ -341,17 +385,7 @@ def roots_name_and_order root = Label.create(:name => "root") a = Label.create(:name => "a", :parent => root) b = Label.create(:name => "b", :parent => root) - expect(a.order_value).to eq(0) - expect(b.order_value).to eq(1) - #c = Label.create(:name => "c") - # should the order_value for roots be set? - expect(root.order_value).not_to be_nil - expect(root.order_value).to eq(0) - - # order_value should never be nil on a child. - expect(a.order_value).not_to be_nil - expect(a.order_value).to eq(0) # Add a child to root at end of children. root.children << b expect(b.parent).to eq(root) @@ -395,28 +429,41 @@ def roots_name_and_order end context "order_value must be set" do + shared_examples_for "correct order_value" do + before do + @root = model.create(name: 'root') + @a, @b, @c = %w(a b c).map { |n| @root.children.create(name: n) } + end - before do - @root = Label.create(name: 'root') - @a, @b, @c = %w(a b c).map { |n| @root.children.create(name: n) } - end + it 'should set order_value on roots' do + expect(@root.order_value).to eq(expected_root_order_value) + end - it 'should set order_value on roots' do - expect(@root.order_value).to eq(0) + it 'should set order_value with siblings' do + expect(@a.order_value).to eq(0) + expect(@b.order_value).to eq(1) + expect(@c.order_value).to eq(2) + end + + it 'should reset order_value when a node is moved to another location' do + root2 = model.create(name: 'root2') + root2.add_child @b + expect(@a.order_value).to eq(0) + expect(@b.order_value).to eq(0) + expect(@c.reload.order_value).to eq(1) + end end - it 'should set order_value with siblings' do - expect(@a.order_value).to eq(0) - expect(@b.order_value).to eq(1) - expect(@c.order_value).to eq(2) + context "with normal model" do + let(:model) { Label } + let(:expected_root_order_value) { 0 } + it_behaves_like "correct order_value" end - it 'should reset order_value when a node is moved to another location' do - root2 = Label.create(name: 'root2') - root2.add_child @b - expect(@a.order_value).to eq(0) - expect(@b.order_value).to eq(0) - expect(@c.reload.order_value).to eq(1) + context "without root ordering" do + let(:model) { LabelWithoutRootOrdering } + let(:expected_root_order_value) { nil } + it_behaves_like "correct order_value" end end