Did a short-cut on the graphing, since we currently only support one type of subscription. This solution still will not scale to all that many edges, but it works, although it will fail if we need to support different types of subcriptions.

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

5 files changed, 134 insertions, 21 deletions

diff --git a/lib/puppet/metatype/metaparams.rb b/lib/puppet/metatype/metaparams.rb
index c9c3a694d..93fffc04c 100644
--- a/lib/puppet/metatype/metaparams.rb
+++ b/lib/puppet/metatype/metaparams.rb
@@ -288,15 +288,15 @@ class Puppet::Type
                     target = object
                 end
 
-                # ok, both sides of the connection store some information
-                # we store the method to call when a given subscription is 
-                # triggered, but the source object decides whether 
-                subargs = {
-                    :event => self.class.events
-                }
-
                 if method = self.class.callback
-                    subargs[:callback] = method
+                    subargs = {
+                        :event => self.class.events,
+                        :callback => method
+                    }
+                else
+                    # If there's no callback, there's no point in even adding
+                    # a label.
+                    subargs = nil
                 end
                 rel = Puppet::Relationship.new(source, target, subargs)
             end
diff --git a/lib/puppet/pgraph.rb b/lib/puppet/pgraph.rb
index de89e0a71..64f411a40 100644
--- a/lib/puppet/pgraph.rb
+++ b/lib/puppet/pgraph.rb
@@ -33,6 +33,23 @@ class Puppet::PGraph < GRATR::Digraph
         end
     end
 
+    # Make sure whichever edge has a label keeps the label
+    def copy_label(source, target, label)
+        # 'require' relationships will not have a label,
+        # and all 'subscribe' relationships have the same
+        # label, at least for now.
+
+        # Labels default to {}, so we can't just test for nil.
+        newlabel = label || {}
+        oldlabel = edge_label(source, target) || {}
+        if ! newlabel.empty? and oldlabel.empty?
+            edge_label_set(source, target, label)
+            # We should probably check to see if the labels both exist
+            # and don't match, but we'd just throw an error which the user
+            # couldn't do anyting about.
+        end
+    end
+
     # Which resources a given resource depends upon.
     def dependents(resource)
         tree_from_vertex2(resource).keys
@@ -128,6 +145,7 @@ class Puppet::PGraph < GRATR::Digraph
                                 s = child
                                 t = neighbor
                             end
+
                             if s.is_a?(type)
                                 raise "Source %s is still a container" % s
                             end
@@ -135,9 +153,16 @@ class Puppet::PGraph < GRATR::Digraph
                                 raise "Target %s is still a container" % t
                             end
 
-                            # It *appears* that we're having a problem
-                            # with multigraphs.
-                            next if edge?(s, t)
+                            # We don't want to add multiple copies of the
+                            # same edge, but we *do* want to make sure we
+                            # keep labels around.
+                            # XXX This will *not* work when we support multiple
+                            # types of labels, and only works now because
+                            # you can only do simple subscriptions.
+                            if edge?(s, t)
+                                copy_label(s, t, edge.label)
+                                next
+                            end
                             add_edge!(s, t, edge.label)
                             if cyclic?
                                 raise ArgumentError,
diff --git a/lib/puppet/relationship.rb b/lib/puppet/relationship.rb
index 879beeab7..15f67b384 100644
--- a/lib/puppet/relationship.rb
+++ b/lib/puppet/relationship.rb
@@ -46,9 +46,9 @@ class Puppet::Relationship < GRATR::Edge
     # Does the passed event match our event?  This is where the meaning
     # of :NONE comes from. 
     def match?(event)
-        if event == :NONE or self.event == :NONE
+        if self.event.nil? or event == :NONE or self.event == :NONE
             return false
-        elsif self.event == :ALL_EVENTS or event == :ALL_EVENTS or event == self.event
+        elsif self.event == :ALL_EVENTS or event == self.event
             return true
         else
             return false
diff --git a/test/other/pgraph.rb b/test/other/pgraph.rb
index c8a04cb8d..2763142ea 100755
--- a/test/other/pgraph.rb
+++ b/test/other/pgraph.rb
@@ -91,11 +91,8 @@ class TestPGraph < Test::Unit::TestCase
         # We have to specify a relationship to our empty container, else it
         # never makes it into the dep graph in the first place.
         #{one => two, three => [middle, two, "c"], "f" => "c", "h" => middle, "c" => empty}.each do |source, targets|
-        {one => two, two => three, middle => three, "f" => "c", "h" => middle, "c" => [three, empty]}.each do |source, targets|
-            targets = [targets] unless targets.is_a?(Array)
-            targets.each do |target|
-                deps.add_edge!(source, target, :callback => :refresh)
-            end
+        {one => two, "f" => "c", "h" => middle, "c" => empty}.each do |source, target|
+            deps.add_edge!(source, target, :callback => :refresh)
         end
 
         #num = 6
@@ -103,7 +100,7 @@ class TestPGraph < Test::Unit::TestCase
         #contgraph.reversal.to_jpg(File.expand_path("~/tmp/graphs"), "reversal#{num}")
         #deps.to_jpg(File.expand_path("~/tmp/graphs"), "relationships#{num}")
         
-        deps.splice!(contgraph, Container)
+        assert_nothing_raised { deps.splice!(contgraph, Container) }
         #deps.to_jpg(File.expand_path("~/tmp/graphs"), "after_relationships#{num}")
         
         assert(! deps.cyclic?, "Created a cyclic graph")
@@ -128,9 +125,74 @@ class TestPGraph < Test::Unit::TestCase
         
         deps.edges.each do |edge|
             assert_equal({:callback => :refresh}, edge.label,
-                "Label was not copied on splice")
+                "Label was not copied for %s => %s" % [edge.source, edge.target])
+        end
+
+        # Now add some relationships to three, but only add labels to one of
+        # the relationships.
+        Puppet.err :yay
+
+        # Add a simple, label-less relationship
+        deps.add_edge!(two, three)
+        assert_nothing_raised { deps.splice!(contgraph, Container) }
+
+        # And make sure it stuck, with no labels.
+        assert_equal({}, deps.edge_label("c", "i"),
+            "label was created for c => i")
+
+        # Now add some edges with labels, in a way that should overwrite
+        deps.add_edge!("c", three, {:callback => :refresh})
+        assert_nothing_raised { deps.splice!(contgraph, Container) }
+
+        # And make sure the label got copied.
+        assert_equal({:callback => :refresh}, deps.edge_label("c", "i"),
+            "label was not copied for c => i")
+
+        # Lastly, add some new label-less edges and make sure the label stays.
+        deps.add_edge!(middle, three)
+        assert_nothing_raised { deps.splice!(contgraph, Container) }
+        assert_equal({:callback => :refresh}, deps.edge_label("c", "i"),
+            "label was lost for c => i")
+        
+        # Now make sure the 'three' edges all have the label we've used.
+        # Note that this will not work when we support more than one type of
+        # subscription.
+        three.each do |child|
+            edge = deps.edge_class.new("c", child)
+            assert(deps.edge?(edge), "no c => %s edge" % child)
+            assert_equal({:callback => :refresh}, deps[edge],
+                "label was not retained for c => %s" % child)
         end
     end
+
+    def test_copy_label
+        graph = Puppet::PGraph.new
+
+        # First make an edge with no label
+        graph.add_edge!(:a, :b)
+        assert_nil(graph.edge_label(:a, :b), "Created a label")
+
+        # Now try to copy an empty label in.
+        graph.copy_label(:a, :b, {})
+
+        # It should just do nothing, since we copied an empty label.
+        assert_nil(graph.edge_label(:a, :b), "Created a label")
+
+        # Now copy in a real label.
+        graph.copy_label(:a, :b, {:callback => :yay})
+        assert_equal({:callback => :yay},
+            graph.edge_label(:a, :b), "Did not copy label")
+
+        # Now copy in a nil label
+        graph.copy_label(:a, :b, nil)
+        assert_equal({:callback => :yay},
+            graph.edge_label(:a, :b), "lost label")
+
+        # And an empty one.
+        graph.copy_label(:a, :b, {})
+        assert_equal({:callback => :yay},
+            graph.edge_label(:a, :b), "lost label")
+    end
 end
 
 # $Id$
diff --git a/test/other/relationship.rb b/test/other/relationship.rb
index c6f8eb763..0a4d725ae 100755
--- a/test/other/relationship.rb
+++ b/test/other/relationship.rb
@@ -43,6 +43,32 @@ class TestRelationship < Test::Unit::TestCase
         assert_nil(i.event, "event was not nil")
         assert_nil(i.callback, "callback was not nil")
     end
+
+    def test_match
+        edge = Puppet::Relationship.new(:a, :b)
+
+        assert(! edge.match?(:NONE), "nil event matched :NONE")
+        assert(! edge.match?(:ALL_EVENT), "nil event matched :ALL_EVENTS")
+        assert(! edge.match?(:random), "nil event matched random")
+
+        # Now set the edge to none
+        edge.label = {:event => :NONE}
+        assert(! edge.match?(:NONE), ":NONE event matched :NONE")
+        assert(! edge.match?(:ALL_EVENT), ":NONE event matched :ALL_EVENTS")
+        assert(! edge.match?(:random), ":NONE event matched random")
+
+        # Now set it to :ALL
+        edge.label = {:event => :ALL_EVENTS}
+        assert(! edge.match?(:NONE), ":ALL_EVENTS event matched :NONE")
+        assert(edge.match?(:ALL_EVENTS), ":ALL_EVENTS did not match :ALL_EVENTS")
+        assert(edge.match?(:random), ":ALL_EVENTS did not match random")
+
+        # And finally, :random
+        edge.label = {:event => :random}
+        assert(! edge.match?(:NONE), ":random event matched :NONE")
+        assert(! edge.match?(:ALL_EVENTS), ":random matched :ALL_EVENTS")
+        assert(edge.match?(:random), ":random did not match random")
+    end
 end
 
-# $Id$
\ No newline at end of file
+# $Id$