[PATCH conductor] RM 3628 - ensure that all params in a deployable can be resolved

jprovazn at redhat.com jprovazn at redhat.com
Tue Aug 7 08:11:26 UTC 2012


From: Jan Provazník <jan.provaznik at gmail.com>

Adds validation for cyclic and not existing references in deployable XML.
Ii also resolves https://bugzilla.redhat.com/show_bug.cgi?id=841028
---
 src/app/models/deployable.rb       |   44 +++++++++++++-
 src/app/util/deployable_xml.rb     |   94 ++++++++++++++++++++++++++++++
 src/config/locales/en.yml          |    4 ++
 src/spec/factories/deployable.rb   |  111 ++++++++++++++++++++++++++++++++++++
 src/spec/models/deployable_spec.rb |   71 ++++++++++++++++++++++-
 5 files changed, 321 insertions(+), 3 deletions(-)

diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
index e1d80d5..374b4e3 100644
--- a/src/app/models/deployable.rb
+++ b/src/app/models/deployable.rb
@@ -29,7 +29,8 @@ class Deployable < ActiveRecord::Base
   validates_length_of :name, :maximum => 1024
 
   validates_presence_of :xml
-  validate :valid_deployable_xml?, :if => Proc.new { |deployable| !deployable.xml.blank? }
+  validate :validate_deployable_xml, :if => Proc.new {|deployable|
+                                              !deployable.xml.blank? }
 
   has_many :permissions, :as => :permission_object, :dependent => :destroy,
            :include => [:role],
@@ -57,7 +58,7 @@ class Deployable < ActiveRecord::Base
     super + catalogs + catalogs.collect{|c| c.pool}.uniq + [pool_family]
   end
 
-  def valid_deployable_xml?
+  def validate_deployable_xml
     begin
       deployable_xml = DeployableXML.new(xml)
       if !deployable_xml.validate!
@@ -65,11 +66,50 @@ class Deployable < ActiveRecord::Base
       elsif !deployable_xml.unique_assembly_names?
         errors.add(:xml, I18n.t('catalog_entries.flash.warning.not_valid_duplicate_assembly_names'))
       end
+      validate_cycles_in_deployable_xml(deployable_xml)
+      validate_references_in_deployable_xml(deployable_xml)
     rescue Nokogiri::XML::SyntaxError => e
       errors.add(:base, I18n.t("deployments.errors.not_valid_deployable_xml", :msg => "#{e.message}"))
     end
   end
 
+  def validate_cycles_in_deployable_xml(deployable_xml)
+    deployable_xml.dependency_graph.cycles.each do |cycle|
+      cycle_str = cycle.map do |node|
+        str = node[:assembly].to_s
+        str << "[#{node[:service]}]" if node[:service].present?
+        str
+      end.join(' -> ')
+      errors.add(:xml,
+                 I18n.t('catalog_entries.flash.warning.not_valid_cyclic_reference',
+                        :reference => cycle_str))
+    end
+  end
+
+  def validate_references_in_deployable_xml(deployable_xml)
+    deployable_xml.dependency_graph.not_existing_references.each do |reference|
+      locale_params = {:from_assembly => reference[:assembly],
+                       :from_service => reference[:service],
+                       :from_param => reference[:reference][:from_param],
+                       :to_param => reference[:reference][:param],
+                       :to_assembly => reference[:reference][:assembly],
+                       :to_service => reference[:reference][:service]}
+      if reference[:no_return_param]
+        errors.add(:xml,
+                   I18n.t('catalog_entries.flash.warning.not_valid_not_existing_param',
+                          locale_params))
+      elsif reference[:to_service]
+        errors.add(:xml,
+                   I18n.t('catalog_entries.flash.warning.not_valid_not_existing_service_reference',
+                          locale_params))
+      else
+        errors.add(:xml,
+                   I18n.t('catalog_entries.flash.warning.not_valid_not_existing_assembly_reference',
+                          locale_params))
+      end
+    end
+  end
+
   # Fetch the deployable contained at :url
   def fetch_deployable
     begin
diff --git a/src/app/util/deployable_xml.rb b/src/app/util/deployable_xml.rb
index cdbbc39..cb01cf5 100644
--- a/src/app/util/deployable_xml.rb
+++ b/src/app/util/deployable_xml.rb
@@ -18,6 +18,8 @@
 XML Wrapper objects for the deployable XML format
 =end
 
+require 'tsort'
+
 class ValidationError < RuntimeError; end
 
 class ParameterXML
@@ -96,6 +98,16 @@ class ServiceXML
       ParameterXML.new(param_node)
     end
   end
+
+  def references
+    refs = parameters.map do |param|
+      next unless param.reference?
+      {:assembly => param.reference_assembly,
+       :service => param.reference_service,
+       :param => param.reference_parameter,
+       :from_param => param.name}
+    end.compact
+  end
 end
 
 class AssemblyXML
@@ -166,6 +178,20 @@ class AssemblyXML
     @root.to_s
   end
 
+  def dependency_nodes
+    nodes = services.map do |service|
+      {:assembly => name,
+       :service => service.name,
+       :references => service.references}
+    end
+    # whole assembly is added as a node too because a inter-assembly references
+    # reference whole assembly, not particular service
+    nodes << {:assembly => name,
+              :service => nil,
+              :references => nodes.map {|n|
+                               n[:references]}.flatten}
+  end
+
   private
   def collect_services
     # collect the service level tooling
@@ -270,8 +296,76 @@ class DeployableXML
     end
   end
 
+  def dependency_graph
+    @dependency_graph ||= DeployableDependencyGraph.new(assemblies)
+  end
+
   private
+
   def relax
     @relax ||= File.open(@relax_file) {|f| Nokogiri::XML::RelaxNG(f)}
   end
 end
+
+class DeployableDependencyGraph
+  include TSort
+
+  def initialize(assemblies)
+    @assemblies = assemblies
+  end
+
+  def cycles
+    strongly_connected_components.find_all {|c| c.length > 1}
+  end
+
+  def dependency_nodes
+    @nodes ||= @assemblies.map {|assembly| assembly.dependency_nodes}.flatten
+  end
+
+  def tsort_each_node(&block)
+    dependency_nodes.each(&block)
+  end
+
+  def tsort_each_child(node, &block)
+    node[:references].map do |ref|
+      ref_node = dependency_nodes.find do |n|
+        n[:assembly] == ref[:assembly] && n[:service] == ref[:service]
+      end
+      ref[:not_existing_ref] = true unless ref_node
+      ref_node
+    end.compact.each(&block)
+  end
+
+  def not_existing_references
+    # not_existing references are detected when doing tsort
+    strongly_connected_components
+
+    invalid_refs = []
+    dependency_nodes.each do |node|
+      # skip "whole assembly" nodes
+      next unless node[:service]
+
+      node[:references].each do |ref|
+        if ref[:not_existing_ref]
+          # in this case, the referenced assembly/service doesn't exist
+          # at all
+          invalid_refs << {:assembly => node[:assembly],
+                           :service => node[:service],
+                           :reference => ref}
+        else
+          # in this case, the referenced assembly/service exists, but the
+          # referenced parameter is not listed in <returns> tag of the assembly
+          assembly = @assemblies.find {|a| a.name == ref[:assembly]}
+          next unless assembly # this shouldn't be needed
+          unless assembly.output_parameters.include?(ref[:param])
+            invalid_refs << {:assembly => node[:assembly],
+                             :service => node[:service],
+                             :no_return_param => true,
+                             :reference => ref}
+          end
+        end
+      end
+    end
+    invalid_refs
+  end
+end
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index 9ae82e4..8cadf41 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -826,6 +826,10 @@ en:
         no_url_provided: "No URL provided for the deployable XML file."
         not_valid: "Deployable XML file doesn't resolve valid XML"
         not_valid_duplicate_assembly_names: "Deployable XML must contain unique assembly names"
+        not_valid_cyclic_reference: "Contains cyclic reference between following assemblies or services: %{reference}"
+        not_valid_not_existing_assembly_reference: "Assembly %{from_assembly}, service %{from_service}, parameter %{from_param} references not existing assembly %{to_assembly}"
+        not_valid_not_existing_service_reference: "Assembly %{from_assembly}, service %{from_service}, parameter %{from_param} references not existing assembly %{to_assembly}, service %{to_service}"
+        not_valid_not_existing_param: "Assembly %{from_assembly}, service %{from_service}, parameter %{from_param} references param %{to_param} which is not returned by assembly %{to_assembly}"
         failed: Deployable was not created.
       notice:
         added: Deployable added to catalog %{catalog}.
diff --git a/src/spec/factories/deployable.rb b/src/spec/factories/deployable.rb
index efbfc6e..8a4f3ba 100644
--- a/src/spec/factories/deployable.rb
+++ b/src/spec/factories/deployable.rb
@@ -72,4 +72,115 @@ FactoryGirl.define do
                 </assemblies>
             </deployable>"
   end
+
+  factory :deployable_with_cyclic_assembly_references, :parent => :deployable do
+    xml "<?xml version=\"1.0\"?>
+            <deployable version='1.0' name='deployable'>
+                <assemblies>
+                  <assembly name='assembly1' hwp='front_hwp1'>
+                    <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+                    <services>
+                      <service name='service1'>
+                        <executable url='executable_url'/>
+                        <parameters>
+                          <parameter name='param1'>
+                            <reference assembly='assembly2' parameter='hostname'/>
+                          </parameter>
+                        </parameters>
+                      </service>
+                    </services>
+                  </assembly>
+                  <assembly name='assembly2' hwp='front_hwp1'>
+                    <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+                    <services>
+                      <service name='service1'>
+                        <executable url='executable_url'/>
+                        <parameters>
+                          <parameter name='param1'>
+                            <reference assembly='assembly3' parameter='hostname'/>
+                          </parameter>
+                        </parameters>
+                      </service>
+                    </services>
+                  </assembly>
+                  <assembly name='assembly3' hwp='front_hwp1'>
+                    <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+                    <services>
+                      <service name='service1'>
+                        <executable url='executable_url'/>
+                        <parameters>
+                          <parameter name='param1'>
+                            <reference assembly='assembly1' parameter='hostname'/>
+                          </parameter>
+                        </parameters>
+                      </service>
+                    </services>
+                  </assembly>
+                </assemblies>
+            </deployable>"
+  end
+
+
+  factory :deployable_with_cyclic_service_references, :parent => :deployable do
+    xml "<?xml version=\"1.0\"?>
+            <deployable version='1.0' name='deployable'>
+                <assemblies>
+                  <assembly name='assembly1' hwp='front_hwp1'>
+                    <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+                    <services>
+                      <service name='service1'>
+                        <executable url='executable_url'/>
+                        <parameters>
+                          <parameter name='param1'>
+                            <reference assembly='assembly1' service='service2'/>
+                          </parameter>
+                        </parameters>
+                      </service>
+                      <service name='service2'>
+                        <executable url='executable_url'/>
+                        <parameters>
+                          <parameter name='param1'>
+                            <reference assembly='assembly1' service='service1'/>
+                          </parameter>
+                        </parameters>
+                      </service>
+                    </services>
+                  </assembly>
+                </assemblies>
+            </deployable>"
+  end
+
+  factory :deployable_with_not_existing_references, :parent => :deployable do
+    xml "<?xml version=\"1.0\"?>
+            <deployable version='1.0' name='deployable'>
+                <assemblies>
+                  <assembly name='assembly1' hwp='front_hwp1'>
+                    <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+                    <services>
+                      <service name='service1'>
+                        <executable url='executable_url'/>
+                        <parameters>
+                          <parameter name='param1'>
+                            <reference assembly='assembly2' service='service2'/>
+                          </parameter>
+                        </parameters>
+                      </service>
+                    </services>
+                  </assembly>
+                  <assembly name='assembly2' hwp='front_hwp1'>
+                    <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+                    <services>
+                      <service name='service1'>
+                        <executable url='executable_url'/>
+                        <parameters>
+                          <parameter name='param1'>
+                            <reference assembly='assembly3' service='service1'/>
+                          </parameter>
+                        </parameters>
+                      </service>
+                    </services>
+                  </assembly>
+                </assemblies>
+            </deployable>"
+  end
 end
diff --git a/src/spec/models/deployable_spec.rb b/src/spec/models/deployable_spec.rb
index 6bc8ffd..4aec099 100644
--- a/src/spec/models/deployable_spec.rb
+++ b/src/spec/models/deployable_spec.rb
@@ -17,6 +17,16 @@
 require 'spec_helper'
 
 describe Deployable do
+  shared_examples_for "deployable XML with cyclic reference" do
+    it "should not be valid" do
+      @deployable.valid?.should be_false
+    end
+
+    it "should detect one cycle" do
+      @cycles.length.should == 1
+    end
+  end
+
   it "should generate xml when set from image" do
     image = mock(Aeolus::Image::Warehouse::Image, :id => '3c58e0d6-d11a-4e68-8b12-233783e56d35', :name => 'image1', :uuid => '3c58e0d6-d11a-4e68-8b12-233783e56d35')
     Aeolus::Image::Warehouse::Image.stub(:find).and_return(image)
@@ -40,7 +50,7 @@ describe Deployable do
 
   it "should not be valid if xml has multiple assemblies with the same name" do
     deployable = FactoryGirl.build(:deployable_unique_name_violation)
-    deployable.valid_deployable_xml?
+    deployable.validate_deployable_xml
     deployable.errors[:xml].should == [I18n.t('catalog_entries.flash.warning.not_valid_duplicate_assembly_names')]
   end
 
@@ -72,6 +82,65 @@ describe Deployable do
     deployable.should_not be_valid
   end
 
+  describe "validation of deployable_xml with cyclic service references" do
+    before :each do
+      @deployable = FactoryGirl.build(:deployable_with_cyclic_service_references)
+      @cycles = DeployableXML.new(@deployable.xml).dependency_graph.cycles
+    end
+
+    it_behaves_like "deployable XML with cyclic reference"
+
+    it "should detect cycle between services of same assembly" do
+      cycle = @cycles.first
+      cycle.length.should == 2
+      cycle.find {|n| n[:assembly] == 'assembly1' && n[:service] == 'service1'}
+      cycle.find {|n| n[:assembly] == 'assembly1' && n[:service] == 'service2'}
+    end
+  end
+
+  describe "validation of deployable_xml with cyclic assembly references" do
+    before :each do
+      @deployable = FactoryGirl.build(:deployable_with_cyclic_assembly_references)
+      @cycles = DeployableXML.new(@deployable.xml).dependency_graph.cycles
+    end
+
+    it_behaves_like "deployable XML with cyclic reference"
+
+    it "should detect cycle between multiple assmebliess" do
+      cycle = @cycles.first
+      cycle.length.should == 3
+      cycle.find {|n| n[:assembly] == 'assembly1' && n[:service] == 'service1'}
+      cycle.find {|n| n[:assembly] == 'assembly2' && n[:service] == 'service1'}
+      cycle.find {|n| n[:assembly] == 'assembly3' && n[:service] == 'service1'}
+    end
+  end
+
+  describe "validation of deployable_xml with not existing references" do
+    before :each do
+      @deployable = FactoryGirl.build(:deployable_with_not_existing_references)
+      @invalid_refs = DeployableXML.new(@deployable.xml).
+                        dependency_graph.not_existing_references
+    end
+
+    it "should not be valid" do
+      @deployable.should_not be_valid
+    end
+
+    it "should detect reference to not existing assembly" do
+      @invalid_refs.find {|r| r[:assembly] == 'assembly2' &&
+        r[:service] == 'service1' &&
+        r[:reference][:assembly] ==  'assembly3' &&
+        r[:reference][:service] ==  'service1'}.should_not be_nil
+    end
+
+    it "should detect reference to not existing service" do
+      @invalid_refs.find {|r| r[:assembly] == 'assembly1' &&
+        r[:service] == 'service1' &&
+        r[:reference][:assembly] ==  'assembly2' &&
+        r[:reference][:service] ==  'service2'}.should_not be_nil
+    end
+  end
+
   it "should manage permissions on catalog changes" do
     pool1 = FactoryGirl.create :pool
     pool2 = FactoryGirl.create :pool
-- 
1.7.10.4




More information about the aeolus-devel mailing list