On Tue, 2012-08-07 at 10:11 +0200, [email protected] wrote: > From: Jan Provazník <[email protected]> > > 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.
