Fixing #437. Transactions now check whether graphs are cyclic, with a somewhat-useful error message if they are.

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@2079 980ebf18-57e1-0310-9a29-db15c13687c0

4 files changed, 54 insertions, 0 deletions

diff --git a/lib/puppet/pgraph.rb b/lib/puppet/pgraph.rb
index 393f88449..894a8fe03 100644
--- a/lib/puppet/pgraph.rb
+++ b/lib/puppet/pgraph.rb
@@ -50,6 +50,21 @@ class Puppet::PGraph < GRATR::Digraph
         end
     end
 
+    # Fail in a somewhat informative way if the graph has become cyclic.
+    def check_cycle
+        sorted = topsort()
+        return true if sorted.size == size()
+
+        bad = []
+        vertices.each do |v|
+            bad << v unless sorted.include?(v)
+        end
+
+        raise Puppet::Error, "Found cycle involving %s" % bad.collect do |v|
+            v.to_s
+        end.join(", ")
+    end
+
     # Which resources a given resource depends upon.
     def dependents(resource)
         tree_from_vertex2(resource).keys
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index b4d4c8512..33874f83e 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -38,6 +38,7 @@ class Transaction
     # Apply all changes for a resource, returning a list of the events
     # generated.
     def apply(resource)
+        resource.info "Applying"
         begin
             changes = resource.evaluate
         rescue => detail
@@ -458,6 +459,8 @@ class Transaction
         # Then splice in the container information
         graph.splice!(@resources, Puppet::Type::Component)
 
+        graph.check_cycle
+
         graph(graph, :expanded_relationships)
         
         return graph
diff --git a/test/other/pgraph.rb b/test/other/pgraph.rb
index f3999a459..e28decebb 100755
--- a/test/other/pgraph.rb
+++ b/test/other/pgraph.rb
@@ -192,6 +192,29 @@ class TestPGraph < Test::Unit::TestCase
         assert_equal({:callback => :yay},
             graph.edge_label(:a, :b), "lost label")
     end
+
+    def test_check_cycle
+        {
+            {:a => :b, :b => :a} => true,
+            {:a => :b, :b => :c, :c => :a} => true,
+            {:a => :b, :b => :c} => false,
+        }.each do |hash, result|
+            graph = Puppet::PGraph.new
+            hash.each do |a,b|
+                graph.add_edge!(a, b)
+            end
+
+            if result
+                assert_raise(Puppet::Error, "%s did not fail" % hash.inspect) do
+                    graph.check_cycle
+                end
+            else
+                assert_nothing_raised("%s failed" % hash.inspect) do
+                    graph.check_cycle
+                end
+            end
+        end
+    end
 end
 
 # $Id$
diff --git a/test/other/transactions.rb b/test/other/transactions.rb
index d1ea0ef68..0a0bb5fd4 100755
--- a/test/other/transactions.rb
+++ b/test/other/transactions.rb
@@ -972,6 +972,19 @@ class TestTransactions < Test::Unit::TestCase
         
         assert(! $called.include?(:refresh), "Called refresh when it wasn't set as a method")
     end
+
+    # Testing #437 - cyclic graphs should throw failures.
+    def test_fail_on_cycle
+        one = Puppet::Type.type(:exec).create(:name => "/bin/echo one")
+        two = Puppet::Type.type(:exec).create(:name => "/bin/echo two")
+        one[:require] = two
+        two[:require] = one
+
+        trans = newcomp(one, two).evaluate
+        assert_raise(Puppet::Error) do
+            trans.relationship_graph
+        end
+    end
 end
 
 # $Id$