Merge pull request #597 from kshnurov/materialized_path2_fix

Fix materialized_path2
stefankroes · Feb 15, 2023 · f66c906 · f66c906
2 parents 8a46ea1 + 73468ba
commit f66c906
Show file tree

Hide file tree

Showing 15 changed files with 191 additions and 110 deletions.
diff --git a/.github/workflows/run_test_suite.yml b/.github/workflows/run_test_suite.yml
@@ -78,7 +78,7 @@ jobs:
         run: |
           # for this database, run all the forms of our tests
           bundle exec rake
-          STRATEGY=materialized_path2 bundle exec rake
+          FORMAT=materialized_path2 bundle exec rake
 
       - name: run pg tests
         env:
@@ -95,8 +95,8 @@ jobs:
           # db container is currently creating the database
           # psql -c 'create database ancestry_test;' || echo 'db exists'
           bundle exec rake
+          FORMAT=materialized_path2 bundle exec rake
 
-          STRATEGY=materialized_path2 bundle exec rake
       - name: run mysql tests
         env:
           DB: mysql2
@@ -111,5 +111,4 @@ jobs:
           # db container is currently creating the database
           # mysql --host $MYSQL_HOST --port 3306 -u $MYSQL_USER  -e 'CREATE SCHEMA IF NOT EXISTS 'ancestry_test';'
           bundle exec rake
-
-          STRATEGY=materialized_path2 bundle exec rake
+          FORMAT=materialized_path2 bundle exec rake
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,10 @@
 Doing our best at supporting [SemVer](http://semver.org/) with
 a nice looking [Changelog](http://keepachangelog.com).
 
+## Version [4.3.0] <sub><sup>2023-01-05</sub></sup>
+* Fix: materialized_path2 strategy was completely broken [#597](https://github.com/stefankroes/ancestry/pull/597) (thx @kshnurov)
+* Fix: descendants ancestry should be updated in after_update callbacks [#589](https://github.com/stefankroes/ancestry/pull/589) (thx @kshnurov)
+
 ## Version [4.2.0] <sub><sup>2022-06-09</sub></sup>
 
 * added strategy: materialized_path2 [#571](https://github.com/stefankroes/ancestry/pull/571)
@@ -270,7 +274,8 @@ Missed 2 commits (which are feature adds)
 * Validations
 
 
-[HEAD]: https://github.com/stefankroes/ancestry/compare/v4.2.0...HEAD
+[HEAD]: https://github.com/stefankroes/ancestry/compare/v4.3.0...HEAD
+[4.3.0]: https://github.com/stefankroes/ancestry/compare/v4.2.0...v4.3.0
 [4.2.0]: https://github.com/stefankroes/ancestry/compare/v4.1.0...v4.2.0
 [4.1.0]: https://github.com/stefankroes/ancestry/compare/v4.0.0...v4.1.0
 [4.0.0]: https://github.com/stefankroes/ancestry/compare/v3.2.1...v4.0.0

diff --git a/README.md b/README.md
@@ -134,6 +134,9 @@ The yellow nodes are those returned by the method.
 The has_ancestry method supports the following options:
 
     :ancestry_column       Pass in a symbol to store ancestry in a different column
+    :ancestry_format       Ancestry format (see Formats below):
+                           :materialized_path (default) 1/2/3, root nodes ancestry=nil
+                           :materialized_path2  /1/2/3/, root nodes ancestry=/
     :orphan_strategy       Instruct Ancestry what to do with children of a node that is destroyed:
                            :destroy   All children are destroyed as well (default)
                            :rootify   The children of the destroyed node become root nodes
@@ -149,14 +152,14 @@ The has_ancestry method supports the following options:
                            By default, primary keys only match integers ([0-9]+)
     :touch                 Instruct Ancestry to touch the ancestors of a node when it changes, to
                            invalidate nested key-based caches. (default: false)
-    :counter_cache         Boolean whether to create counter cache column accessor. 
-                           Default column name is `children_count`. 
+    :counter_cache         Boolean whether to create counter cache column accessor.
+                           Default column name is `children_count`.
                            Pass symbol to use different column name (default: false)
     :update_strategy       Choose the strategy to update descendants nodes (default: `ruby`)
                            :ruby     All descendants are updated using the ruby algorithm. This strategy
                                      triggers update callbacks for each descendants nodes
                            :sql      All descendants are updated using a single SQL statement. This strategy
-                                     does not trigger update callbacks for the descendants. This strategy is 
+                                     does not trigger update callbacks for the descendants. This strategy is
                                      available only for PostgreSql implementations
 
 # (Named) Scopes
@@ -264,6 +267,29 @@ to sort an array of nodes as if traversing in preorder. (Note that since materia
 trees do not support ordering within a rank, the order of siblings is
 dependant upon their original array order.)
 
+# Formats
+
+You can choose from 2 ancestry formats:
+
+```
+:materialized_path (default) 1/2/3, root nodes ancestry=nil
+    descendants: ancestry LIKE '1/2/3/%' OR ancestry = '1/2/3'
+:materialized_path2  /1/2/3/, root nodes ancestry=/
+    descendants: ancestry LIKE '/1/2/3/%'
+```
+
+`materialized_path2` has easier sorting and faster descendants queries since
+there's no NULL values and no OR condition.
+
+Migrating from `materialized_path` to `materialized_path2`:
+```
+klass = YourModel
+klass.where.not(klass.arel_table[klass.ancestry_column].eq(nil)).update_all("#{klass.ancestry_column} = CONCAT('#{klass.ancestry_delimiter}', #{klass.ancestry_column}, '#{klass.ancestry_delimiter}')")
+klass.where(klass.arel_table[klass.ancestry_column].eq(nil)).update_all("#{klass.ancestry_column} = '#{klass.ancestry_root}'")
+
+change_column_null klass.table_name, klass.ancestry_column, false
+```
+
 # Migrating from plugin that uses parent_id column
 
 Most current tree plugins use a parent_id column (has_ancestry,

diff --git a/lib/ancestry.rb b/lib/ancestry.rb
@@ -4,6 +4,7 @@
 require_relative 'ancestry/exceptions'
 require_relative 'ancestry/has_ancestry'
 require_relative 'ancestry/materialized_path'
+require_relative 'ancestry/materialized_path2'
 require_relative 'ancestry/materialized_path_pg'
 
 I18n.load_path += Dir[File.join(File.expand_path(File.dirname(__FILE__)),

diff --git a/lib/ancestry/has_ancestry.rb b/lib/ancestry/has_ancestry.rb
@@ -4,15 +4,25 @@ def has_ancestry options = {}
       # Check options
       raise Ancestry::AncestryException.new(I18n.t("ancestry.option_must_be_hash")) unless options.is_a? Hash
       options.each do |key, value|
-        unless [:ancestry_column, :orphan_strategy, :cache_depth, :depth_cache_column, :touch, :counter_cache, :primary_key_format, :update_strategy, :strategy].include? key
+        unless [:ancestry_column, :orphan_strategy, :cache_depth, :depth_cache_column, :touch, :counter_cache, :primary_key_format, :update_strategy, :ancestry_format].include? key
           raise Ancestry::AncestryException.new(I18n.t("ancestry.unknown_option", key: key.inspect, value: value.inspect))
         end
       end
 
+      if options[:ancestry_format].present? && ![:materialized_path, :materialized_path2].include?( options[:ancestry_format] )
+        raise Ancestry::AncestryException.new(I18n.t("ancestry.unknown_format", value: options[:ancestry_format]))
+      end
+
       # Create ancestry column accessor and set to option or default
       cattr_accessor :ancestry_column
       self.ancestry_column = options[:ancestry_column] || :ancestry
 
+      cattr_accessor :ancestry_primary_key_format
+      self.ancestry_primary_key_format = options[:primary_key_format].presence || '[0-9]+'
+
+      cattr_accessor :ancestry_delimiter
+      self.ancestry_delimiter = '/'
+
       # Save self as base class (for STI)
       cattr_accessor :ancestry_base_class
       self.ancestry_base_class = self
@@ -27,14 +37,19 @@ def has_ancestry options = {}
       # Include dynamic class methods
       extend Ancestry::ClassMethods
 
-      if options[:strategy] == :materialized_path2
-        validates_format_of self.ancestry_column, :with => derive_materialized2_pattern(options[:primary_key_format]), :allow_nil => false
+      cattr_accessor :ancestry_format
+      self.ancestry_format = options[:ancestry_format] || :materialized_path
+
+      if options[:ancestry_format] == :materialized_path2
         extend Ancestry::MaterializedPath2
       else
-        validates_format_of self.ancestry_column, :with => derive_materialized_pattern(options[:primary_key_format]), :allow_nil => true
         extend Ancestry::MaterializedPath
       end
 
+      attribute self.ancestry_column, default: self.ancestry_root
+
+      validates self.ancestry_column, ancestry_validation_options
+
       update_strategy = options[:update_strategy] || Ancestry.default_update_strategy
       include Ancestry::MaterializedPathPg if update_strategy == :sql
 
@@ -99,28 +114,6 @@ def acts_as_tree(*args)
       return super if defined?(super)
       has_ancestry(*args)
     end
-
-    private
-
-    def derive_materialized_pattern(primary_key_format, delimiter = '/')
-      primary_key_format ||= '[0-9]+'
-
-      if primary_key_format.to_s.include?('\A')
-        primary_key_format
-      else
-        /\A#{primary_key_format}(#{delimiter}#{primary_key_format})*\Z/
-      end
-    end
-
-    def derive_materialized2_pattern(primary_key_format, delimiter = '/')
-      primary_key_format ||= '[0-9]+'
-
-      if primary_key_format.to_s.include?('\A')
-        primary_key_format
-      else
-        /\A#{delimiter}(#{primary_key_format}#{delimiter})*\Z/
-      end
-    end
   end
 end
 

diff --git a/lib/ancestry/locales/en.yml b/lib/ancestry/locales/en.yml
@@ -9,6 +9,7 @@ en:
 
     option_must_be_hash: "Options for has_ancestry must be in a hash."
     unknown_option: "Unknown option for has_ancestry: %{key} => %{value}."
+    unknown_format: "Unknown ancestry format: %{value}."
     named_scope_depth_cache: "Named scope '%{scope_name}' is only available when depth caching is enabled."
 
     exclude_self: "%{class_name} cannot be a descendant of itself."

diff --git a/lib/ancestry/materialized_path.rb b/lib/ancestry/materialized_path.rb
@@ -3,11 +3,6 @@ module Ancestry
   # root a=nil,id=1   children=id,id/%      == 1, 1/%
   # 3: a=1/2,id=3     children=a/id,a/id/%  == 1/2/3, 1/2/3/%
   module MaterializedPath
-    BEFORE_LAST_SAVE_SUFFIX = '_before_last_save'.freeze
-    IN_DATABASE_SUFFIX = '_in_database'.freeze
-    ANCESTRY_DELIMITER='/'.freeze
-    ROOT=nil
-
     def self.extended(base)
       base.send(:include, InstanceMethods)
     end
@@ -17,7 +12,7 @@ def path_of(object)
     end
 
     def roots
-      where(arel_table[ancestry_column].eq(ROOT))
+      where(arel_table[ancestry_column].eq(ancestry_root))
     end
 
     def ancestors_of(object)
@@ -42,17 +37,16 @@ def children_of(object)
     def indirects_of(object)
       t = arel_table
       node = to_node(object)
-      where(t[ancestry_column].matches("#{node.child_ancestry}#{ANCESTRY_DELIMITER}%", nil, true))
+      where(t[ancestry_column].matches("#{node.child_ancestry}#{ancestry_delimiter}%", nil, true))
     end
 
     def descendants_of(object)
-      node = to_node(object)
-      indirects_of(node).or(children_of(node))
+      where(descendant_conditions(object))
     end
 
     def descendants_by_ancestry(ancestry)
       t = arel_table
-      t[ancestry_column].matches("#{ancestry}/%", nil, true).or(t[ancestry_column].eq(ancestry))
+      t[ancestry_column].matches("#{ancestry}#{ancestry_delimiter}%", nil, true).or(t[ancestry_column].eq(ancestry))
     end
 
     def descendant_conditions(object)
@@ -94,31 +88,48 @@ def ordered_by_ancestry_and(order)
       ordered_by_ancestry(order)
     end
 
+    def ancestry_root
+      nil
+    end
+
+    private
+
+    def ancestry_validation_options
+      {
+        format: { with: ancestry_format_regexp },
+        allow_nil: ancestry_nil_allowed?
+      }
+    end
+
+    def ancestry_nil_allowed?
+      true
+    end
+
+    def ancestry_format_regexp
+      /\A#{ancestry_primary_key_format}(#{Regexp.escape(ancestry_delimiter)}#{ancestry_primary_key_format})*\z/.freeze
+    end
+
     module InstanceMethods
       # optimization - better to go directly to column and avoid parsing
       def ancestors?
-        read_attribute(self.ancestry_base_class.ancestry_column) != ROOT
+        read_attribute(self.ancestry_base_class.ancestry_column) != self.ancestry_base_class.ancestry_root
       end
       alias :has_parent? :ancestors?
 
       def ancestor_ids=(value)
-        col = self.ancestry_base_class.ancestry_column
-        value.present? ? write_attribute(col, generate_ancestry(value)) : write_attribute(col, ROOT)
+        write_attribute(self.ancestry_base_class.ancestry_column, generate_ancestry(value))
       end
 
       def ancestor_ids
         parse_ancestry_column(read_attribute(self.ancestry_base_class.ancestry_column))
       end
 
       def ancestor_ids_before_last_save
-        parse_ancestry_column(send("#{self.ancestry_base_class.ancestry_column}#{BEFORE_LAST_SAVE_SUFFIX}"))
+        parse_ancestry_column(attribute_before_last_save(self.ancestry_base_class.ancestry_column))
       end
 
       def parent_id_before_last_save
-        ancestry_was = send("#{self.ancestry_base_class.ancestry_column}#{BEFORE_LAST_SAVE_SUFFIX}")
-        return if ancestry_was == ROOT
-
-        parse_ancestry_column(ancestry_was).last
+        parse_ancestry_column(attribute_before_last_save(self.ancestry_base_class.ancestry_column)).last
       end
 
       # optimization - better to go directly to column and avoid parsing
@@ -132,25 +143,27 @@ def sibling_of?(node)
       def child_ancestry
         # New records cannot have children
         raise Ancestry::AncestryException.new(I18n.t("ancestry.no_child_for_new_record")) if new_record?
-        path_was = self.send("#{self.ancestry_base_class.ancestry_column}#{IN_DATABASE_SUFFIX}")
-        path_was.blank? ? id.to_s : "#{path_was}#{ANCESTRY_DELIMITER}#{id}"
+        [attribute_in_database(self.ancestry_base_class.ancestry_column), id].compact.join(self.ancestry_base_class.ancestry_delimiter)
       end
 
       def child_ancestry_before_save
         # New records cannot have children
         raise Ancestry::AncestryException.new(I18n.t("ancestry.no_child_for_new_record")) if new_record?
-        path_was = self.send("#{self.ancestry_base_class.ancestry_column}#{BEFORE_LAST_SAVE_SUFFIX}")
-        path_was.blank? ? id.to_s : "#{path_was}#{ANCESTRY_DELIMITER}#{id}"
+        [attribute_before_last_save(self.ancestry_base_class.ancestry_column), id].compact.join(self.ancestry_base_class.ancestry_delimiter)
       end
 
       def parse_ancestry_column(obj)
-        return [] if obj == ROOT
-        obj_ids = obj.split(ANCESTRY_DELIMITER)
+        return [] if obj.nil? || obj == self.ancestry_base_class.ancestry_root
+        obj_ids = obj.split(self.ancestry_base_class.ancestry_delimiter).delete_if(&:blank?)
         self.class.primary_key_is_an_integer? ? obj_ids.map!(&:to_i) : obj_ids
       end
 
       def generate_ancestry(ancestor_ids)
-        ancestor_ids.join(ANCESTRY_DELIMITER)
+        if ancestor_ids.present? && ancestor_ids.any?
+          ancestor_ids.join(self.ancestry_base_class.ancestry_delimiter)
+        else
+          self.ancestry_base_class.ancestry_root
+        end
       end
     end
   end