Least kludgy patch for #2675

This makes parameters responsible for the canonicalization of their values and
provides a default (passthrough) implementation.  It changes munge to pre-
canonicalize the value and resource references to builtin types to canonicalize
titles (which map to resorce namevars) with the corresponding parameter's
classes's canonicalization.

It adds a canonicalization routine to file paths that normalizes the behaviour
(trailing slashes are ignored) and DRYs up the related code.

Signed-off-by: Markus Roberts <Markus@reality.com>

6 files changed, 49 insertions, 41 deletions

diff --git a/lib/puppet/parameter.rb b/lib/puppet/parameter.rb
index f4086671d..58a91477a 100644
--- a/lib/puppet/parameter.rb
+++ b/lib/puppet/parameter.rb
@@ -293,6 +293,13 @@ class Puppet::Parameter
             define_method(:unmunge, &block)
         end
 
+        # Optionaly convert the value to a canonical form so that it will
+        # be found in hashes, etc.  Mostly useful for namevars.
+        def to_canonicalize(&block)
+            metaclass = (class << self; self; end)
+            metaclass.send(:define_method,:canonicalize,&block)
+        end
+
         # Mark whether we're the namevar.
         def isnamevar
             @isnamevar = true
@@ -464,10 +471,19 @@ class Puppet::Parameter
         value
     end
 
+    # Assume the value is already in canonical form by default
+    def self.canonicalize(value)
+        value
+    end
+
+    def canonicalize(value)
+        self.class.canonicalize(value)
+    end
+
     # A wrapper around our munging that makes sure we raise useful exceptions.
     def munge(value)
         begin
-            ret = unsafe_munge(value)
+            ret = unsafe_munge(canonicalize(value))
         rescue Puppet::Error => detail
             Puppet.debug "Reraising %s" % detail
             raise
diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
index 09c099418..f21c820a0 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -78,7 +78,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
             @resource_table[ref] = resource
 
             # If the name and title differ, set up an alias
-            #self.alias(resource, resource.name) if resource.respond_to?(:name) and resource.respond_to?(:title) and resource.name != resource.title
+
             if resource.respond_to?(:name) and resource.respond_to?(:title) and resource.name != resource.title
                 self.alias(resource, resource.name) if resource.isomorphic?
             end
diff --git a/lib/puppet/resource/reference.rb b/lib/puppet/resource/reference.rb
index 37522ffe9..dce4449de 100644
--- a/lib/puppet/resource/reference.rb
+++ b/lib/puppet/resource/reference.rb
@@ -20,24 +20,12 @@ class Puppet::Resource::Reference
     end
 
     def initialize(argtype, argtitle = nil)
-        if argtitle.nil?
-            if argtype.is_a?(Puppet::Type)
-                self.title = argtype.title
-                self.type = argtype.class.name
-            else
-                self.title = argtype
-                if self.title == argtype
-                    raise ArgumentError, "No title provided and title '%s' is not a valid resource reference" % argtype.inspect
-                end
-            end
-        else
-            # This will set @type if it looks like a resource reference.
-            self.title = argtitle
-
-            # Don't override whatever was done by setting the title.
-            self.type ||= argtype
-        end
-
+        self.type,self.title = 
+	    if    (argtitle || argtype) =~ /^([^\[\]]+)\[(.+)\]$/m then [ $1,                 $2            ]
+	    elsif argtitle                                         then [ argtype,            argtitle      ]
+	    elsif argtype.is_a?(Puppet::Type)                      then [ argtype.class.name, argtype.title ]
+	    else raise ArgumentError, "No title provided and #{argtype.inspect} is not a valid resource reference"
+	    end
         @builtin_type = nil
     end
 
@@ -47,15 +35,11 @@ class Puppet::Resource::Reference
         return nil
     end
 
-    # If the title has square brackets, treat it like a reference and
-    # set things appropriately; else, just set it.
     def title=(value)
-        if value =~ /^([^\[\]]+)\[(.+)\]$/m
-            self.type = $1
-            @title = $2
-        else
-            @title = value
+        if @type and klass = Puppet::Type.type(@type.to_s.downcase)
+            value = klass.canonicalize_ref(value)
         end
+        @title = value
     end
 
     # Canonize the type so we know it's always consistent.
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index ee87c2680..2f7b57afc 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -210,8 +210,8 @@ class Type
     end
 
     # Find the namevar
-    def self.namevar
-        unless defined? @namevar
+    def self.namevar_parameter
+        @namevar_parameter ||= (
             params = @parameters.find_all { |param|
                 param.isnamevar? or param.name == :name
             }
@@ -219,12 +219,19 @@ class Type
             if params.length > 1
                 raise Puppet::DevError, "Found multiple namevars for %s" % self.name
             elsif params.length == 1
-                @namevar = params[0].name
+                params.first
             else
                 raise Puppet::DevError, "No namevar for %s" % self.name
             end
-        end
-        @namevar
+        )
+    end
+
+    def self.namevar
+        @namevar ||= namevar_parameter.name
+    end
+
+    def self.canonicalize_ref(s)
+        namevar_parameter.canonicalize(s)
     end
 
     # Create a new parameter.  Requires a block and a name, stores it in the
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 48fe3ea38..f9205255a 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -46,6 +46,12 @@ module Puppet
             unmunge do |value|
                 File.join( Puppet::FileCollection.collection.path(value[:index]), value[:name] )
             end
+ 
+            to_canonicalize do |s|
+                # Get rid of any duplicate slashes, and remove any trailing slashes unless 
+                # the title is just a slash, in which case leave it.
+                s.gsub(/\/+/, "/").sub(/(.)\/$/,'\1')
+            end
         end
 
         newparam(:backup) do
@@ -399,11 +405,7 @@ module Puppet
 
             super
 
-            # Get rid of any duplicate slashes, and remove any trailing slashes.
-            @title = @title.gsub(/\/+/, "/")
-
-            @title.sub!(/\/$/, "") unless @title == "/"
-
+            @title = self.class.canonicalize_ref(@title)
             @stat = nil
         end
 
diff --git a/lib/puppet/util/methodhelper.rb b/lib/puppet/util/methodhelper.rb
index 32fca1877..ecc9d537f 100644
--- a/lib/puppet/util/methodhelper.rb
+++ b/lib/puppet/util/methodhelper.rb
@@ -12,11 +12,10 @@ module Puppet::Util::MethodHelper
     def set_options(options)
         options.each do |param,value|
             method = param.to_s + "="
-            begin
+            if respond_to? method
                 self.send(method, value)
-            rescue NoMethodError
-                raise ArgumentError, "Invalid parameter %s to object class %s" %
-                        [param,self.class.to_s]
+            else
+                raise ArgumentError, "Invalid parameter #{param} to object class #{self.class}"
             end
         end
     end