Fixing #446. I ended up largely not using porridge's patch,

but only because the code (and my coding style, to some extent)
has changed so much in the last few months.
Also, added specs.

3 files changed, 91 insertions, 21 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 6dbb730cc..16f758c3a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,10 @@
+    Relationship metaparams (:notify, :require, :subscribe, and
+    :before) now stack when they are collecting metaparam values
+    from their containers (#446).  For instance, if a resource
+    inside a definition has a value set for 'require', and you call
+    the definition with 'require', the resource gets both requires,
+    where before it would only retain its initial value.
+
     Changed the behavior of --debug to include Mongrel client
     debugging information.  Mongrel output will be written to
     the terminal only, not to the puppet debug log.  This should
diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb
index a8da6b054..ea0f93f57 100644
--- a/lib/puppet/parser/resource.rb
+++ b/lib/puppet/parser/resource.rb
@@ -15,6 +15,14 @@ class Puppet::Parser::Resource
 
     attr_writer :tags
 
+    # Determine whether the provided parameter name is a relationship parameter.
+    def self.relationship_parameter?(name)
+        unless defined?(@relationship_names)
+            @relationship_names = Puppet::Type.relationship_params.collect { |p| p.name }
+        end
+        @relationship_names.include?(name)
+    end
+
     # Proxy a few methods to our @ref object.
     [:builtin?, :type, :title].each do |method|
         define_method(method) do
@@ -337,13 +345,19 @@ class Puppet::Parser::Resource
     # from any parent scope, and there's currently no way to turn that off.
     def add_metaparams
         Puppet::Type.eachmetaparam do |name|
-            # Skip metaparams that we already have defined.
-            next if @params[name]
-            if val = scope.lookupvar(name.to_s, false)
-                unless val == :undefined
-                    set_parameter(name, val)
-                end
-            end
+            # Skip metaparams that we already have defined, unless they're relationship metaparams.
+            # LAK:NOTE Relationship metaparams get treated specially -- we stack them, instead of
+            # overriding.
+            next if @params[name] and not self.class.relationship_parameter?(name)
+
+            # Skip metaparams for which we get no value.
+            next unless val = scope.lookupvar(name.to_s, false) and val != :undefined
+
+            # The default case: just set the value
+            return set_parameter(name, val) unless @params[name]
+
+            # For relationship params, though, join the values (a la #446).
+            @params[name].value = [@params[name].value, val].flatten
         end
     end
 
@@ -364,21 +378,10 @@ class Puppet::Parser::Resource
     def override_parameter(param)
         # This can happen if the override is defining a new parameter, rather
         # than replacing an existing one.
-        unless current = @params[param.name]
-            @params[param.name] = param
-            return
-        end
+        (@params[param.name] = param and return) unless current = @params[param.name]
 
-        # The parameter is already set.  See if they're allowed to override it.
-        if param.source.child_of?(current.source)
-            if param.add
-                # Merge with previous value.
-                param.value = [ current.value, param.value ].flatten
-            end
-
-            # Replace it, keeping all of its info.
-            @params[param.name] = param
-        else
+        # The parameter is already set.  Fail if they're not allowed to override it.
+        unless param.source.child_of?(current.source)
             if Puppet[:trace]
                 puts caller
             end
@@ -396,6 +399,16 @@ class Puppet::Parser::Resource
             msg += "; cannot redefine"
             raise Puppet::ParseError.new(msg, param.line, param.file)
         end
+
+        # If we've gotten this far, we're allowed to override.
+
+        # Merge with previous value, if the parameter was generated with the +> syntax.
+        if param.add
+            current.value = [current.value, param.value].flatten
+        else
+            # Just replace the existing parameter with this new one.
+            @params[param.name] = param
+        end
     end
 
     # Verify that all passed parameters are valid.  This throws an error if
diff --git a/spec/unit/parser/resource.rb b/spec/unit/parser/resource.rb
index 354690711..3d048f7e6 100755
--- a/spec/unit/parser/resource.rb
+++ b/spec/unit/parser/resource.rb
@@ -37,3 +37,53 @@ describe Puppet::Parser::Resource, " when evaluating" do
         res.evaluate
     end
 end
+
+describe Puppet::Parser::Resource, " when finishing" do
+    before do
+        @parser = Puppet::Parser::Parser.new :Code => ""
+        @source = @parser.newclass ""
+        @definition = @parser.newdefine "mydefine"
+        @class = @parser.newclass "myclass"
+        @nodedef = @parser.newnode("mynode")[0]
+        @node = Puppet::Node.new("yaynode")
+        @compile = Puppet::Parser::Compile.new(@node, @parser)
+        @scope = @compile.topscope
+
+        @resource = Puppet::Parser::Resource.new(:type => "mydefine", :title => "whatever", :scope => @scope, :source => @source)
+    end
+
+    it "should copy metaparams from its scope" do
+        @scope.setvar("noop", "true")
+
+        @resource.class.publicize_methods(:add_metaparams)  { @resource.add_metaparams }
+
+        @resource["noop"].should == "true"
+    end
+
+    it "should not copy metaparams that it already has" do
+        @resource.class.publicize_methods(:set_parameter)  { @resource.set_parameter("noop", "false") }
+        @scope.setvar("noop", "true")
+
+        @resource.class.publicize_methods(:add_metaparams)  { @resource.add_metaparams }
+
+        @resource["noop"].should == "false"
+    end
+
+    it "should stack relationship metaparams from its container if it already has them" do
+        @resource.class.publicize_methods(:set_parameter)  { @resource.set_parameter("require", "resource") }
+        @scope.setvar("require", "container")
+
+        @resource.class.publicize_methods(:add_metaparams)  { @resource.add_metaparams }
+
+        @resource["require"].sort.should == %w{container resource}
+    end
+
+    it "should flatten the array resulting from stacking relationship metaparams" do
+        @resource.class.publicize_methods(:set_parameter)  { @resource.set_parameter("require", ["resource1", "resource2"]) }
+        @scope.setvar("require", %w{container1 container2})
+
+        @resource.class.publicize_methods(:add_metaparams)  { @resource.add_metaparams }
+
+        @resource["require"].sort.should == %w{container1 container2 resource1 resource2}
+    end
+end