From 7176d19715f21b9069227e64f76660b0c2aa75f7 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 22 Oct 2024 18:20:01 -0400 Subject: [PATCH 1/4] extract ancestry_depth_sql to module methods want to call these from tests --- lib/ancestry/materialized_path.rb | 13 +++++++------ lib/ancestry/materialized_path2.rb | 14 ++++++++------ test/concerns/db_test.rb | 2 ++ 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/lib/ancestry/materialized_path.rb b/lib/ancestry/materialized_path.rb index 41eb84d3..d53854af 100644 --- a/lib/ancestry/materialized_path.rb +++ b/lib/ancestry/materialized_path.rb @@ -101,12 +101,7 @@ def child_ancestry_sql end def ancestry_depth_sql - @ancestry_depth_sql ||= - begin - tmp = %{(LENGTH(#{table_name}.#{ancestry_column}) - LENGTH(REPLACE(#{table_name}.#{ancestry_column},'#{ancestry_delimiter}','')))} - tmp = tmp + "/#{ancestry_delimiter.size}" if ancestry_delimiter.size > 1 - "(CASE WHEN #{table_name}.#{ancestry_column} IS NULL THEN 0 ELSE 1 + #{tmp} END)" - end + @ancestry_depth_sql ||= MaterializedPath.construct_depth_sql(table_name, ancestry_column, ancestry_delimiter) end def generate_ancestry(ancestor_ids) @@ -135,6 +130,12 @@ def concat(*args) end end + def self.construct_depth_sql(table_name, ancestry_column, ancestry_delimiter) + tmp = %{(LENGTH(#{table_name}.#{ancestry_column}) - LENGTH(REPLACE(#{table_name}.#{ancestry_column},'#{ancestry_delimiter}','')))} + tmp = tmp + "/#{ancestry_delimiter.size}" if ancestry_delimiter.size > 1 + "(CASE WHEN #{table_name}.#{ancestry_column} IS NULL THEN 0 ELSE 1 + #{tmp} END)" + end + private def ancestry_validation_options(ancestry_primary_key_format) diff --git a/lib/ancestry/materialized_path2.rb b/lib/ancestry/materialized_path2.rb index 47d520f8..46887852 100644 --- a/lib/ancestry/materialized_path2.rb +++ b/lib/ancestry/materialized_path2.rb @@ -33,12 +33,7 @@ def child_ancestry_sql end def ancestry_depth_sql - @ancestry_depth_sql ||= - begin - tmp = %{(LENGTH(#{table_name}.#{ancestry_column}) - LENGTH(REPLACE(#{table_name}.#{ancestry_column},'#{ancestry_delimiter}','')))} - tmp = tmp + "/#{ancestry_delimiter.size}" if ancestry_delimiter.size > 1 - "(#{tmp} -1)" - end + @ancestry_depth_sql ||= MaterializedPath2.construct_depth_sql(table_name, ancestry_column, ancestry_delimiter) end def generate_ancestry(ancestor_ids) @@ -49,6 +44,13 @@ def generate_ancestry(ancestor_ids) end end + # module method + def self.construct_depth_sql(table_name, ancestry_column, ancestry_delimiter) + tmp = %{(LENGTH(#{table_name}.#{ancestry_column}) - LENGTH(REPLACE(#{table_name}.#{ancestry_column},'#{ancestry_delimiter}','')))} + tmp = tmp + "/#{ancestry_delimiter.size}" if ancestry_delimiter.size > 1 + "(#{tmp} -1)" + end + private def ancestry_nil_allowed? diff --git a/test/concerns/db_test.rb b/test/concerns/db_test.rb index eb794056..fe2001c8 100644 --- a/test/concerns/db_test.rb +++ b/test/concerns/db_test.rb @@ -3,6 +3,8 @@ class DbTest < ActiveSupport::TestCase def test_does_not_load_database c = Class.new(ActiveRecord::Base) do + self.table_name = "table" + def self.connection raise "Oh No - tried to connect to database" end From fe8c2ce207c8d1d5b5df8e81448bae1ba426f4e3 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 22 Oct 2024 18:50:23 -0400 Subject: [PATCH 2/4] Extract format module extract --- lib/ancestry/has_ancestry.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/ancestry/has_ancestry.rb b/lib/ancestry/has_ancestry.rb index 55daed94..12306f93 100644 --- a/lib/ancestry/has_ancestry.rb +++ b/lib/ancestry/has_ancestry.rb @@ -39,12 +39,7 @@ def has_ancestry options = {} # Include dynamic class methods extend Ancestry::ClassMethods - - if ancestry_format == :materialized_path2 - extend Ancestry::MaterializedPath2 - else - extend Ancestry::MaterializedPath - end + extend Ancestry::HasAncestry.ancestry_format_module(ancestry_format) attribute self.ancestry_column, default: self.ancestry_root @@ -124,6 +119,15 @@ def acts_as_tree(*args) return super if defined?(super) has_ancestry(*args) end + + def self.ancestry_format_module(ancestry_format) + ancestry_format ||= Ancestry.default_ancestry_format + if ancestry_format == :materialized_path2 + Ancestry::MaterializedPath2 + else + Ancestry::MaterializedPath + end + end end end From 4dda866e0bb17ff9cd42fd34f3b74cf58d5caaa0 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 26 Sep 2023 11:50:03 -0400 Subject: [PATCH 3/4] remove duplication for before_depth scope and friends --- lib/ancestry/has_ancestry.rb | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/lib/ancestry/has_ancestry.rb b/lib/ancestry/has_ancestry.rb index 12306f93..91362925 100644 --- a/lib/ancestry/has_ancestry.rb +++ b/lib/ancestry/has_ancestry.rb @@ -84,20 +84,18 @@ def has_ancestry options = {} # Validate depth column validates_numericality_of depth_cache_column, :greater_than_or_equal_to => 0, :only_integer => true, :allow_nil => false - scope :before_depth, lambda { |depth| where("#{depth_cache_column} < ?", depth) } - scope :to_depth, lambda { |depth| where("#{depth_cache_column} <= ?", depth) } - scope :at_depth, lambda { |depth| where("#{depth_cache_column} = ?", depth) } - scope :from_depth, lambda { |depth| where("#{depth_cache_column} >= ?", depth) } - scope :after_depth, lambda { |depth| where("#{depth_cache_column} > ?", depth) } + depth_cache_sql = depth_cache_column else # this is not efficient, but it works - scope :before_depth, lambda { |depth| where("#{ancestry_depth_sql} < ?", depth) } - scope :to_depth, lambda { |depth| where("#{ancestry_depth_sql} <= ?", depth) } - scope :at_depth, lambda { |depth| where("#{ancestry_depth_sql} = ?", depth) } - scope :from_depth, lambda { |depth| where("#{ancestry_depth_sql} >= ?", depth) } - scope :after_depth, lambda { |depth| where("#{ancestry_depth_sql} > ?", depth) } + depth_cache_sql = ancestry_depth_sql end + scope :before_depth, lambda { |depth| where("#{depth_cache_sql} < ?", depth) } + scope :to_depth, lambda { |depth| where("#{depth_cache_sql} <= ?", depth) } + scope :at_depth, lambda { |depth| where("#{depth_cache_sql} = ?", depth) } + scope :from_depth, lambda { |depth| where("#{depth_cache_sql} >= ?", depth) } + scope :after_depth, lambda { |depth| where("#{depth_cache_sql} > ?", depth) } + # Create counter cache column accessor and set to option or default if options[:counter_cache] cattr_accessor :counter_cache_column From 541c167938c6a19c4e1c9c22f55e609b27c90205 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 22 Oct 2024 18:34:53 -0400 Subject: [PATCH 4/4] Add support for virtual depth column still need to consider whether to keep depth_cache_column as deprecated --- lib/ancestry/has_ancestry.rb | 6 +- test/concerns/depth_virtual_test.rb | 104 ++++++++++++++++++++++++++++ test/environment.rb | 15 +++- 3 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 test/concerns/depth_virtual_test.rb diff --git a/lib/ancestry/has_ancestry.rb b/lib/ancestry/has_ancestry.rb index 91362925..8806cfce 100644 --- a/lib/ancestry/has_ancestry.rb +++ b/lib/ancestry/has_ancestry.rb @@ -64,7 +64,11 @@ def has_ancestry options = {} end # Create ancestry column accessor and set to option or default - if options[:cache_depth] + + if options[:cache_depth] == :virtual + # NOTE: not setting self.depth_cache_column so the code does not try to update the column + depth_cache_sql = options[:depth_cache_column]&.to_s || 'ancestry_depth' + elsif options[:cache_depth] # Create accessor for column name and set to option or default self.cattr_accessor :depth_cache_column self.depth_cache_column = diff --git a/test/concerns/depth_virtual_test.rb b/test/concerns/depth_virtual_test.rb new file mode 100644 index 00000000..fe79aae4 --- /dev/null +++ b/test/concerns/depth_virtual_test.rb @@ -0,0 +1,104 @@ +require_relative '../environment' + +# These are only valid for postgres +class DepthVirtualTest < ActiveSupport::TestCase + def test_depth_caching + return unless test_virtual_column? + + AncestryTestDatabase.with_model :depth => 3, :width => 3, :cache_depth => :virtual do |_model, roots| + roots.each do |lvl0_node, lvl0_children| + assert_equal 0, lvl0_node.depth + lvl0_children.each do |lvl1_node, lvl1_children| + assert_equal 1, lvl1_node.depth + lvl1_children.each do |lvl2_node, _lvl2_children| + assert_equal 2, lvl2_node.depth + end + end + end + end + end + + def test_depth_caching_after_subtree_movement + return unless test_virtual_column? + + AncestryTestDatabase.with_model :depth => 6, :width => 1, :cache_depth => :virtual do |model, _roots| + node = model.at_depth(3).first + node.update(:parent => model.roots.first) + assert_equal(1, node.depth) + node.children.each do |child| + assert_equal(2, child.depth) + child.children.each do |gchild| + assert_equal(3, gchild.depth) + end + end + end + end + + def test_depth_scopes + return unless test_virtual_column? + + AncestryTestDatabase.with_model :depth => 4, :width => 2, :cache_depth => true do |model, _roots| + model.before_depth(2).all? { |node| assert node.depth < 2 } + model.to_depth(2).all? { |node| assert node.depth <= 2 } + model.at_depth(2).all? { |node| assert node.depth == 2 } + model.from_depth(2).all? { |node| assert node.depth >= 2 } + model.after_depth(2).all? { |node| assert node.depth > 2 } + end + end + + def test_depth_scopes_without_depth_cache + return unless test_virtual_column? + + AncestryTestDatabase.with_model :depth => 4, :width => 2 do |model, _roots| + model.before_depth(2).all? { |node| assert node.depth < 2 } + model.to_depth(2).all? { |node| assert node.depth <= 2 } + model.at_depth(2).all? { |node| assert node.depth == 2 } + model.from_depth(2).all? { |node| assert node.depth >= 2 } + model.after_depth(2).all? { |node| assert node.depth > 2 } + end + end + + def test_exception_when_rebuilding_depth_cache_for_model_without_depth_caching + return unless test_virtual_column? + + AncestryTestDatabase.with_model do |model| + assert_raise Ancestry::AncestryException do + model.rebuild_depth_cache! + end + end + end + + def test_exception_on_unknown_depth_column + return unless test_virtual_column? + + AncestryTestDatabase.with_model :cache_depth => true do |model| + assert_raise Ancestry::AncestryException do + model.create!.subtree(:this_is_not_a_valid_depth_option => 42) + end + end + end + + # we are already testing generate and parse against static values + # this assumes those are methods are tested and working + def test_ancestry_depth_change + return unless test_virtual_column? + + AncestryTestDatabase.with_model do |model| + { + [[], [1]] => +1, + [[1], []] => -1, + [[1], [2]] => 0, + [[1], [1, 2, 3]] => +2, + [[1, 2, 3], [1]] => -2 + }.each do |(before, after), diff| + a_before = model.generate_ancestry(before) + a_after = model.generate_ancestry(after) + assert_equal(diff, model.ancestry_depth_change(a_before, a_after)) + end + end + end + + def test_virtual_column? + AncestryTestDatabase.postgres? && ActiveRecord.version.to_s >= "7.0" + end +end diff --git a/test/environment.rb b/test/environment.rb index a87f40fc..dc3e848e 100644 --- a/test/environment.rb +++ b/test/environment.rb @@ -126,7 +126,20 @@ def self.with_model options = {} ActiveRecord::Base.connection.create_table 'test_nodes', **table_options do |table| table.send(column_type, options[:ancestry_column], **column_options(force_allow_nil: skip_ancestry)) - table.integer options[:cache_depth] == true ? :ancestry_depth : options[:cache_depth] if options[:cache_depth] + case options[:cache_depth] + when true + table.integer :ancestry_depth + when :virtual + # sorry, this duplicates has_ancestry a little + path_module = Ancestry::HasAncestry.ancestry_format_module(options[:ancestry_format]) + ancestry_depth_sql = path_module.construct_depth_sql("test_nodes", options[:ancestry_column], '/') + + table.virtual :ancestry_depth, type: :integer, as: ancestry_depth_sql, stored: true + when nil, false + # no column + else + table.integer options[:cache_depth] + end if options[:counter_cache] counter_cache_column = options[:counter_cache] == true ? :children_count : options[:counter_cache] table.integer counter_cache_column, default: 0, null: false