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

Tomas Hrcka thrcka at redhat.com
Thu Aug 9 12:38:14 UTC 2012


On Tue, 2012-08-07 at 10:11 +0200, jprovazn at redhat.com wrote:
> 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

ACK.





More information about the aeolus-devel mailing list