Merge pull request #312 from sassafrastech/no_reorder_with_parent_id_nil

Added dont_order_roots option

Author: mceachen

README.md

@@ -494,6 +494,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.

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

lib/closure_tree/has_closure_tree_root.rb

@@ -1,5 +1,6 @@
 module ClosureTree
   class MultipleRootError < StandardError; end
+  class RootOrderingDisabledError < StandardError; end
 
   module HasClosureTreeRoot
 

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
 

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)

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

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

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