From a6c5b9e3fba17822a7bafe728381d7d74c2dae82 Mon Sep 17 00:00:00 2001 From: Lucy Fu Date: Fri, 27 Mar 2020 19:21:58 -0400 Subject: [PATCH] MiqAeNamespace should use ancestry instead of acts_as_tree Fixes ManageIQ/manageiq-automation_engine#409 --- app/models/miq_ae_class.rb | 2 +- app/models/miq_ae_namespace.rb | 48 +++++++++++++++------------- spec/models/miq_ae_class_spec.rb | 11 ++++--- spec/models/miq_ae_field_spec.rb | 3 +- spec/models/miq_ae_instance_spec.rb | 17 +++++----- spec/models/miq_ae_namespace_spec.rb | 14 ++++---- 6 files changed, 50 insertions(+), 45 deletions(-) diff --git a/app/models/miq_ae_class.rb b/app/models/miq_ae_class.rb index dc5520b001a..3f504ed41d3 100644 --- a/app/models/miq_ae_class.rb +++ b/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? } + find_by(:namespace_id => name_space.id).where(arel_table[:name].lower.eq(name.downcase)) end singleton_class.send(:alias_method, :find_by_namespace_and_name, :lookup_by_namespace_and_name) diff --git a/app/models/miq_ae_namespace.rb b/app/models/miq_ae_namespace.rb index cf2d0ae0028..4a07052efbf 100644 --- a/app/models/miq_ae_namespace.rb +++ b/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,26 @@ 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, :inverse_of => false + + validates :name, + :format => {:with => /\A[\w\.\-\$]+\z/i, :message => N_("may contain only alphanumeric and _ . - $ characters")}, + :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 +40,22 @@ 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 +88,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 +112,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) diff --git a/spec/models/miq_ae_class_spec.rb b/spec/models/miq_ae_class_spec.rb index 2eb9ee50040..c67c31e2944 100644 --- a/spec/models/miq_ae_class_spec.rb +++ b/spec/models/miq_ae_class_spec.rb @@ -39,6 +39,7 @@ 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 @@ -46,20 +47,20 @@ 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 diff --git a/spec/models/miq_ae_field_spec.rb b/spec/models/miq_ae_field_spec.rb index f80b3244e63..12b742dcc4c 100644 --- a/spec/models/miq_ae_field_spec.rb +++ b/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 diff --git a/spec/models/miq_ae_instance_spec.rb b/spec/models/miq_ae_instance_spec.rb index 3cdf30bd66d..d4a103a7113 100644 --- a/spec/models/miq_ae_instance_spec.rb +++ b/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") } diff --git a/spec/models/miq_ae_namespace_spec.rb b/spec/models/miq_ae_namespace_spec.rb index 71160228449..f29e1b9f3cf 100644 --- a/spec/models/miq_ae_namespace_spec.rb +++ b/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