From 7b6af89509e8439fe2474e623ee97e4db67ab011 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Tue, 7 Jul 2026 02:46:37 +0200 Subject: Replace awesome_nested_set with a plain parent_id-based NestedTree concern Root-caused this session: appending a child to any node never widened that parent's own rgt boundary, on the pinned revision (Gemfile tracked main directly, chasing a too-conservative gemspec constraint - not, as first assumed, a deliberate pin to avoid a known bug). Reproduced cleanly on a single ordinary create with no concurrency and no bulk operation involved, confirmed via the gem's own SetValidator, then confirmed as the root cause of nodes_controller_test.rb's 3 long-standing "pre-existing" failures - not three separate mysteries, one bug. admin_controller's sitemap needed its own real conversion, not just a drop-in: awesome_nested_set's lft column implicitly provided correct depth-first tree order for free, which the old code combined with a separate class-level each_with_level iterator. Both replaced by one method, self_and_descendants_ordered_with_level, computing an ordered [node, level] list in a single query-then-walk pass - checked against the actual view template first (admin/index.html.erb) rather than assumed, since it relies on list order alone to render correct visual nesting. lft/rgt/depth columns intentionally left in schema, unused - dropping them is a separate, deliberately deferred migration once this is proven running for a while, not bundled with the behavior change. --- app/controllers/admin_controller.rb | 8 ++-- app/models/concerns/nested_tree.rb | 86 +++++++++++++++++++++++++++++++++++++ app/models/node.rb | 25 +++-------- 3 files changed, 95 insertions(+), 24 deletions(-) create mode 100644 app/models/concerns/nested_tree.rb (limited to 'app') diff --git a/app/controllers/admin_controller.rb b/app/controllers/admin_controller.rb index bfc6cd8..3fa0519 100644 --- a/app/controllers/admin_controller.rb +++ b/app/controllers/admin_controller.rb @@ -15,12 +15,10 @@ class AdminController < ApplicationController Time.now, Time.now - 14.days ).limit(50).order("updated_at desc") - all_nodes = Node.root.self_and_descendants + ordered_with_level = Node.root.self_and_descendants_ordered_with_level @sitemap_depth = {} - Node.each_with_level(all_nodes) do |node, level| - @sitemap_depth[node.id] = level - end - @sitemap = all_nodes.to_a.sort! { |node1,node2| node1.lft <=> node2.lft }.delete_if { |node| node.update? } + ordered_with_level.each { |node, level| @sitemap_depth[node.id] = level } + @sitemap = ordered_with_level.map(&:first).reject(&:update?) @mypages = Page.where("user_id = ? or editor_id = ?", @current_user, @current_user) diff --git a/app/models/concerns/nested_tree.rb b/app/models/concerns/nested_tree.rb new file mode 100644 index 0000000..9a2eb0e --- /dev/null +++ b/app/models/concerns/nested_tree.rb @@ -0,0 +1,86 @@ +# app/models/concerns/nested_tree.rb +# +# Minimal parent_id-based replacement for the tree-traversal subset of +# awesome_nested_set actually used by this app (descendants, ancestors, +# level, root, move_to_child_of) +module NestedTree + extend ActiveSupport::Concern + + included do + belongs_to :parent, class_name: name, foreign_key: :parent_id, optional: true, inverse_of: :children + has_many :children, -> { order(:id) }, class_name: name, foreign_key: :parent_id, inverse_of: :parent + + before_destroy :delete_descendants + end + + class_methods do + def root + roots.first + end + + def roots + where(parent_id: nil) + end + + def valid? + # Every non-root row's parent_id must point at a real, existing row. + where.not(parent_id: nil).where.missing(:parent).none? + end + end + + def root? + parent_id.nil? + end + + def ancestors + result = [] + current = parent + while current + result << current + current = current.parent + end + result + end + + def level + ancestors.size + end + + def descendants + ids = [] + queue = self.class.where(parent_id: id).pluck(:id) + until queue.empty? + ids.concat(queue) + queue = self.class.where(parent_id: queue).pluck(:id) + end + self.class.where(id: ids) + end + + def self_and_descendants + self.class.where(id: [id] + descendants.pluck(:id)) + end + + def self_and_descendants_ordered_with_level + nodes = [self] + descendants.to_a + children_by_parent = nodes.group_by(&:parent_id) + children_by_parent.each_value { |list| list.sort_by!(&:id) } + + result = [] + visit = ->(node, level) do + result << [node, level] + (children_by_parent[node.id] || []).each { |child| visit.call(child, level + 1) } + end + visit.call(self, 0) + result + end + + def move_to_child_of(new_parent) + update!(parent_id: new_parent.id) + end + + private + + def delete_descendants + self.class.where(id: descendants.pluck(:id)).delete_all + end +end diff --git a/app/models/node.rb b/app/models/node.rb index 2177f15..fc23dc1 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -1,6 +1,6 @@ class Node < ApplicationRecord # Mixins and Plugins - acts_as_nested_set + include NestedTree # Associations has_many :pages, -> { order("revision ASC") }, :dependent => :destroy @@ -21,16 +21,6 @@ class Node < ApplicationRecord validates_uniqueness_of :slug, :scope => :parent_id, :unless => -> { parent_id.nil? } validates_presence_of :parent_id, :unless => -> { Node.root.nil? } - validate :borders # This should never ever happen. - - # Index for Fulltext Search - # define_index do - # indexes head.translations.title - # indexes slug - # indexes unique_name - # indexes head.translations.body - # end - # Class methods # Returns a page for a given node. If no revision is supplied, it returns @@ -254,10 +244,13 @@ class Node < ApplicationRecord # Watch out recursion ahead! update_unique_name itself triggers this # after_save callback which invokes update_unique_name on its children. # Hopefully until no childrens occur + # + # Queries parent_id directly rather than the NestedTree#children + # association out of habit from the old awesome_nested_set-avoidance + # workaround - no longer strictly necessary now that children is + # equally safe, but left as-is since it already works correctly. def update_unique_names_of_children unless root? - # Use parent_id-based traversal instead of lft/rgt descendants - # due to awesome_nested_set not refreshing parent lft/rgt in memory Node.where(:parent_id => self.id).each do |child| child.reload child.update_unique_name @@ -265,10 +258,4 @@ class Node < ApplicationRecord end end end - - def borders - if lft && rgt && (lft > rgt) - errors.add("Fuck!. lft should never be smaller than rgt") - end - end end -- cgit v1.3