MiqAeNamespace should use ancestry instead of acts_as_tree

Fixes ManageIQ/manageiq-automation_engine#409

app/models/miq_ae_class.rb

@@ -29,7 +29,7 @@ def self.lookup_by_namespace_and_name(name_space, name, _args = {})
     name_space = MiqAeNamespace.lookup_by_fqname(name_space)
     return nil if name_space.nil?
 
-    name_space.ae_classes.detect { |c| name.casecmp(c.name).zero? }
+    where(:namespace_id => name_space.id).where(arel_table[:name].lower.eq(name.downcase)).first
   end
 
   singleton_class.send(:alias_method, :find_by_namespace_and_name, :lookup_by_namespace_and_name)

app/models/miq_ae_namespace.rb

@@ -1,5 +1,8 @@
+require 'ancestry'
+require 'ancestry_patch'
+
 class MiqAeNamespace < ApplicationRecord
-  acts_as_tree
+  has_ancestry
   include MiqAeSetUserInfoMixin
   include MiqAeYamlImportExportMixin
 
@@ -8,24 +11,24 @@ class MiqAeNamespace < ApplicationRecord
                          /^commit_time/, /^commit_sha/, /^ref$/, /^ref_type$/,
                          /^last_import_on/, /^source/, /^top_level_namespace/].freeze
 
-  belongs_to :parent,        :class_name => "MiqAeNamespace",  :foreign_key => :parent_id
-  has_many   :ae_namespaces, :class_name => "MiqAeNamespace",  :foreign_key => :parent_id,    :dependent => :destroy
-  has_many   :ae_classes, -> { includes([:ae_methods, :ae_fields, :ae_instances]) },    :class_name => "MiqAeClass",      :foreign_key => :namespace_id, :dependent => :destroy
+  has_many :ae_classes, -> { includes([:ae_methods, :ae_fields, :ae_instances]) }, :class_name => "MiqAeClass", :foreign_key => :namespace_id, :dependent => :destroy
+
+  validates_format_of :name, :with    => /\A[\w\.\-\$]+\z/i,
+                             :message => N_("may contain only alphanumeric and _ . - $ characters")
+  validates :name, :presence => true, :uniqueness => {:scope => :ancestry, :case_sensitive => false}
 
-  validates_presence_of   :name
-  validates_format_of     :name, :with    => /\A[\w\.\-\$]+\z/i,
-                                 :message => N_("may contain only alphanumeric and _ . - $ characters")
-  validates_uniqueness_of :name, :scope => :parent_id, :case_sensitive => false
+  alias ae_namespaces children
+  virtual_has_many :ae_namespaces
 
   def self.lookup_by_fqname(fqname, include_classes = true)
     return nil if fqname.blank?
 
-    fqname   = fqname[0] == '/' ? fqname : "/#{fqname}"
-    fqname   = fqname.downcase
-    last     = fqname.split('/').last
+    fqname = fqname[0] == '/' ? fqname : "/#{fqname}"
+    fqname = fqname.downcase
+    last = fqname.split('/').last
     low_name = arel_table[:name].lower
-    query    = includes(:parent)
-    query    = query.includes(:ae_classes) if include_classes
+
+    query = include_classes ? includes(:ae_classes) : all
     query.where(low_name.eq(last)).detect { |namespace| namespace.fqname.downcase == fqname }
   end
 
@@ -35,21 +38,21 @@ def self.lookup_by_fqname(fqname, include_classes = true)
   def self.find_or_create_by_fqname(fqname, include_classes = true)
     return nil if fqname.blank?
 
-    fqname   = fqname[1..-1] if fqname[0] == '/'
+    fqname = fqname[1..-1] if fqname[0] == '/'
     found = lookup_by_fqname(fqname, include_classes)
     return found unless found.nil?
 
     parts = fqname.split('/')
     new_parts = [parts.pop]
     loop do
+      break if parts.empty?
       found = lookup_by_fqname(parts.join('/'), include_classes)
       break unless found.nil?
       new_parts.unshift(parts.pop)
-      break if parts.empty?
     end
 
     new_parts.each do |p|
-      found = create(:name => p, :parent_id => found.try(:id))
+      found = found ? create(:name => p, :parent => found) : create(:name => p)
     end
 
     found
@@ -82,7 +85,7 @@ def self.find_tree(find_options = {})
   end
 
   def fqname
-    @fqname ||= "/#{ancestors.collect(&:name).reverse.push(name).join('/')}"
+    @fqname ||= "/#{path.pluck(:name).join('/')}"
   end
 
   def editable?(user = User.current_user)
@@ -106,15 +109,11 @@ def domain_name
   end
 
   def domain
-    if domain?
-      self
-    elsif (ns = ancestors.last) && ns.domain?
-      ns
-    end
+    root if root.domain?
   end
 
   def domain?
-    parent_id.nil? && name != '$'
+    root? && name != '$'
   end
 
   def self.display_name(number = 1)

spec/models/miq_ae_class_spec.rb

@@ -39,27 +39,28 @@
 
   before do
     @user = FactoryBot.create(:user_with_group)
+    @ns = FactoryBot.create(:miq_ae_namespace, :name => "TEST", :parent => FactoryBot.create(:miq_ae_domain))
   end
 
   it "should not create class without namespace" do
     expect { MiqAeClass.new(:name => "TEST").save! }.to raise_error(ActiveRecord::RecordInvalid)
   end
 
   it "should not create class without name" do
-    expect { MiqAeClass.new(:namespace => "TEST").save! }.to raise_error(ActiveRecord::RecordInvalid)
+    expect { MiqAeClass.new(:namespace_id => @ns.id).save! }.to raise_error(ActiveRecord::RecordInvalid)
   end
 
   it "should set the updated_by field on save" do
-    c1 = MiqAeClass.create(:namespace => "TEST", :name => "oleg")
+    c1 = MiqAeClass.create(:namespace_id => @ns.id, :name => "oleg")
     expect(c1.updated_by).to eq('system')
   end
 
   it "should not create classes with the same name in the same namespace" do
-    c1 = MiqAeClass.new(:namespace => "TEST", :name => "oleg")
+    c1 = MiqAeClass.new(:namespace_id => @ns.id, :name => "oleg")
     expect(c1).not_to be_nil
     expect(c1.save!).to be_truthy
-    expect { MiqAeClass.new(:namespace => "TEST", :name => "OLEG").save! }.to raise_error(ActiveRecord::RecordInvalid)
-    c2 = MiqAeClass.new(:namespace => "PROD", :name => "oleg")
+    expect { MiqAeClass.new(:namespace_id => @ns.id, :name => "OLEG").save! }.to raise_error(ActiveRecord::RecordInvalid)
+    c2 = MiqAeClass.new(:namespace_id => FactoryBot.create(:miq_ae_namespace).id, :name => "oleg")
     expect(c2).not_to be_nil
     expect(c2.save!).to be_truthy
   end

spec/models/miq_ae_field_spec.rb

@@ -41,7 +41,8 @@
 
   context "legacy tests" do
     before do
-      @c1 = MiqAeClass.create(:namespace => "TEST", :name => "fields_test")
+      @ns = FactoryBot.create(:miq_ae_namespace, :name => "TEST", :parent => FactoryBot.create(:miq_ae_domain))
+      @c1 = MiqAeClass.create(:namespace_id => @ns.id, :name => "fields_test")
       @user = FactoryBot.create(:user_with_group)
     end
 

spec/models/miq_ae_instance_spec.rb

@@ -2,7 +2,8 @@
   context "legacy tests" do
     before do
       @user = FactoryBot.create(:user_with_group)
-      @c1 = MiqAeClass.create(:namespace => "TEST", :name => "instance_test")
+      @ns = FactoryBot.create(:miq_ae_namespace, :name => "TEST", :parent => FactoryBot.create(:miq_ae_domain))
+      @c1 = MiqAeClass.create(:namespace_id => @ns.id, :name => "instance_test")
       @fname1 = "field1"
       @f1 = @c1.ae_fields.create(:name => @fname1)
     end
@@ -149,7 +150,7 @@
 
     it "should return editable as false if the parent namespace/class is not editable" do
       d1 = FactoryBot.create(:miq_ae_system_domain, :tenant => User.current_tenant)
-      n1 = FactoryBot.create(:miq_ae_namespace, :parent_id => d1.id)
+      n1 = FactoryBot.create(:miq_ae_namespace, :parent => d1)
       c1 = FactoryBot.create(:miq_ae_class, :namespace_id => n1.id, :name => "foo")
       i1 = FactoryBot.create(:miq_ae_instance, :class_id => c1.id, :name => "foo_instance")
       expect(i1.editable?(@user)).to be_falsey
@@ -158,7 +159,7 @@
     it "should return editable as true if the parent namespace/class is editable" do
       User.current_user = @user
       d1 = FactoryBot.create(:miq_ae_domain, :tenant => User.current_tenant)
-      n1 = FactoryBot.create(:miq_ae_namespace, :parent_id => d1.id)
+      n1 = FactoryBot.create(:miq_ae_namespace, :parent => d1)
       c1 = FactoryBot.create(:miq_ae_class, :namespace_id => n1.id, :name => "foo")
       i1 = FactoryBot.create(:miq_ae_instance, :class_id => c1.id, :name => "foo_instance")
       expect(i1.editable?(@user)).to be_truthy
@@ -204,14 +205,14 @@
 
   context "#copy" do
     before do
-      @d1 = FactoryBot.create(:miq_ae_namespace, :name => "domain1", :parent_id => nil, :priority => 1)
-      @ns1 = FactoryBot.create(:miq_ae_namespace, :name => "ns1", :parent_id => @d1.id)
+      @d1 = FactoryBot.create(:miq_ae_namespace, :name => "domain1", :priority => 1)
+      @ns1 = FactoryBot.create(:miq_ae_namespace, :name => "ns1", :parent => @d1)
       @cls1 = FactoryBot.create(:miq_ae_class, :name => "cls1", :namespace_id => @ns1.id)
       @i1 = FactoryBot.create(:miq_ae_instance, :class_id => @cls1.id, :name => "foo_instance1")
       @i2 = FactoryBot.create(:miq_ae_instance, :class_id => @cls1.id, :name => "foo_instance2")
 
       @d2 = FactoryBot.create(:miq_ae_domain, :name => "domain2", :priority => 2)
-      @ns2 = FactoryBot.create(:miq_ae_namespace, :name => "ns2", :parent_id => @d2.id)
+      @ns2 = FactoryBot.create(:miq_ae_namespace, :name => "ns2", :parent => @d2)
     end
 
     it "copies instances under specified namespace" do
@@ -261,8 +262,8 @@
     let(:u1) { FactoryBot.create(:user_with_group, :name => 'user1') }
     let(:d1) { FactoryBot.create(:miq_ae_domain, :name => 'dom1', :priority => 1) }
     let(:d2) { FactoryBot.create(:miq_ae_domain, :name => 'dom2', :priority => 2) }
-    let(:n1) { FactoryBot.create(:miq_ae_namespace, :parent_id => d1.id, :name => "namespace") }
-    let(:n2) { FactoryBot.create(:miq_ae_namespace, :parent_id => d2.id, :name => "namespace") }
+    let(:n1) { FactoryBot.create(:miq_ae_namespace, :parent => d1, :name => "namespace") }
+    let(:n2) { FactoryBot.create(:miq_ae_namespace, :parent => d2, :name => "namespace") }
     let(:c1) { FactoryBot.create(:miq_ae_class, :namespace_id => n1.id, :name => "class") }
     let(:c2) { FactoryBot.create(:miq_ae_class, :namespace_id => n2.id, :name => "class") }
     let!(:i1) { FactoryBot.create(:miq_ae_instance, :class_id => c1.id, :name => "instance") }

spec/models/miq_ae_namespace_spec.rb

@@ -34,17 +34,17 @@
 
     context "with a duplicite names" do
       let(:domain) { FactoryBot.create(:miq_ae_domain) }
-      let(:ns1)    { FactoryBot.create(:miq_ae_namespace, :name => 'ns1', :parent_id => domain.id) }
+      let(:ns1)    { FactoryBot.create(:miq_ae_namespace, :name => 'ns1', :parent => domain) }
 
       before do
-        FactoryBot.create(:miq_ae_namespace, :name => 'namespace', :parent_id => ns1.id)
+        FactoryBot.create(:miq_ae_namespace, :name => 'namespace', :parent => ns1)
       end
 
       it "with a distinct path is allowed" do
         # domain/ns1/namespace
         # domain/ns2/namespace
-        ns2 = FactoryBot.create(:miq_ae_namespace, :name => 'ns2', :parent_id => domain.id)
-        dup_namespace = FactoryBot.create(:miq_ae_namespace, :name => 'namespace', :parent_id => ns2.id)
+        ns2 = FactoryBot.create(:miq_ae_namespace, :name => 'ns2', :parent => domain)
+        dup_namespace = FactoryBot.create(:miq_ae_namespace, :name => 'namespace', :parent => ns2)
 
         expect(ns2.valid?).to be_truthy
         expect(dup_namespace.valid?).to be_truthy
@@ -54,7 +54,7 @@
         # domain/ns1/namespace
         # domain/ns1/NAMESPACE
         expect do
-          FactoryBot.create(:miq_ae_namespace, :name => 'NAMESPACE', :parent_id => ns1.id)
+          FactoryBot.create(:miq_ae_namespace, :name => 'NAMESPACE', :parent => ns1)
         end.to raise_error("Validation failed: MiqAeNamespace: Name has already been taken")
       end
     end
@@ -92,9 +92,9 @@
     n1 = FactoryBot.create(:miq_ae_system_domain, :tenant => @user.current_tenant)
     expect(n1.editable?(@user)).to be_falsey
 
-    n2 = MiqAeNamespace.create!(:name => 'ns2', :parent_id => n1.id)
+    n2 = MiqAeNamespace.create!(:name => 'ns2', :parent => n1)
 
-    n3 = MiqAeNamespace.create!(:name => 'ns3', :parent_id => n2.id)
+    n3 = MiqAeNamespace.create!(:name => 'ns3', :parent => n2)
     expect(n3.editable?(@user)).to be_falsey
   end