[#4472]+[#4483] Moved type-name resolution out of Puppet::Parser::TypeLoader.

Moved type-name resolution out of Puppet::Parser::TypeLoader, and into
its primary client, Puppet::Resource::TypeCollection.  TypeCollection
now always passes fully qualified type names to TypeLoader.

This avoids duplicate type-name resolution logic between TypeLoader
and TypeCollection.  That in turn fixes bug 4472, which resulted
from flaws in the type-name resolution logic in TypeLoader.

In addition, it fixes bug 4483, which resulted from improper
interleaving between looking up names using the TypeCollection and the
TypeLoader.

6 files changed, 166 insertions, 179 deletions

diff --git a/lib/puppet/parser/parser_support.rb b/lib/puppet/parser/parser_support.rb
index c0fd37178..97d985cfb 100644
--- a/lib/puppet/parser/parser_support.rb
+++ b/lib/puppet/parser/parser_support.rb
@@ -103,11 +103,11 @@ class Puppet::Parser::Parser
   end
 
   def find_hostclass(namespace, name)
-    known_resource_types.find_or_load(namespace, name, :hostclass)
+    known_resource_types.find_hostclass(namespace, name)
   end
 
   def find_definition(namespace, name)
-    known_resource_types.find_or_load(namespace, name, :definition)
+    known_resource_types.find_definition(namespace, name)
   end
 
   def import(file)
diff --git a/lib/puppet/parser/type_loader.rb b/lib/puppet/parser/type_loader.rb
index 09aa636e1..516c1e32b 100644
--- a/lib/puppet/parser/type_loader.rb
+++ b/lib/puppet/parser/type_loader.rb
@@ -76,50 +76,29 @@ class Puppet::Parser::TypeLoader
     @imported = {}
   end
 
-  def load_until(namespaces, name)
-    return nil if name == "" # special-case main.
-    name2files(namespaces, name).each do |filename|
-      modname = begin
-        import_if_possible(filename)
-      rescue Puppet::ImportError => detail
-        # We couldn't load the item
-        # I'm not convienced we should just drop these errors, but this
-        # preserves existing behaviours.
-        nil
-      end
-      if result = yield(filename)
-        Puppet.debug "Automatically imported #{name} from #{filename} into #{environment}"
-        result.module_name = modname if modname and result.respond_to?(:module_name=)
-        return result
+  # Try to load the object with the given fully qualified name.  For
+  # each file that was actually loaded, yield(filename, modname).
+  def try_load_fqname(fqname)
+    return nil if fqname == "" # special-case main.
+    name2files(fqname).each do |filename|
+      if not loaded?(filename)
+        modname = begin
+          import_if_possible(filename)
+        rescue Puppet::ImportError => detail
+          # We couldn't load the item
+          # I'm not convienced we should just drop these errors, but this
+          # preserves existing behaviours.
+          nil
+        end
+        yield(filename, modname)
       end
     end
-    nil
   end
 
   def loaded?(name)
     @loaded.include?(name)
   end
 
-  def name2files(namespaces, name)
-    return [name.sub(/^::/, '').gsub("::", File::SEPARATOR)] if name =~ /^::/
-
-    result = namespaces.inject([]) do |names_to_try, namespace|
-      fullname = (namespace + "::#{name}").sub(/^::/, '')
-
-      # Try to load the module init file if we're a qualified name
-      names_to_try << fullname.split("::")[0] if fullname.include?("::")
-
-      # Then the fully qualified name
-      names_to_try << fullname
-    end
-
-    # Otherwise try to load the bare name on its own.  This
-    # is appropriate if the class we're looking for is in a
-    # module that's different from our namespace.
-    result << name
-    result.uniq.collect { |f| f.gsub("::", File::SEPARATOR) }
-  end
-
   def parse_file(file)
     Puppet.debug("importing '#{file}' in environment #{environment}")
     parser = Puppet::Parser::Parser.new(environment)
@@ -143,4 +122,19 @@ class Puppet::Parser::TypeLoader
       @loading.done_with(file)
     end
   end
+
+  private
+
+  # Return a list of all file basenames that should be tried in order
+  # to load the object with the given fully qualified name.
+  def name2files(fqname)
+    result = []
+    ary = fqname.split("::")
+    while ary.length > 0
+      result << ary.join(File::SEPARATOR)
+      ary.pop
+    end
+    return result
+  end
+
 end
diff --git a/lib/puppet/resource/type_collection.rb b/lib/puppet/resource/type_collection.rb
index 63d110395..3a327e264 100644
--- a/lib/puppet/resource/type_collection.rb
+++ b/lib/puppet/resource/type_collection.rb
@@ -92,50 +92,8 @@ class Puppet::Resource::TypeCollection
     @definitions[munge_name(name)]
   end
 
-  def find(namespaces, name, type)
-    #Array("") == [] for some reason
-    namespaces = [namespaces] unless namespaces.is_a?(Array)
-
-    if name =~ /^::/
-      return send(type, name.sub(/^::/, ''))
-    end
-
-    namespaces.each do |namespace|
-      ary = namespace.split("::")
-
-      while ary.length > 0
-        tmp_namespace = ary.join("::")
-        if r = find_partially_qualified(tmp_namespace, name, type)
-          return r
-        end
-
-        # Delete the second to last object, which reduces our namespace by one.
-        ary.pop
-      end
-
-      if result = send(type, name)
-        return result
-      end
-    end
-    nil
-  end
-
-  def find_or_load(namespaces, name, type)
-    name      = name.downcase
-    namespaces = [namespaces] unless namespaces.is_a?(Array)
-    namespaces = namespaces.collect { |ns| ns.downcase }
-
-    # This could be done in the load_until, but the knowledge seems to
-    # belong here.
-    if r = find(namespaces, name, type)
-      return r
-    end
-
-    loader.load_until(namespaces, name) { find(namespaces, name, type) }
-  end
-
   def find_node(namespaces, name)
-    find("", name, :node)
+    @nodes[munge_name(name)]
   end
 
   def find_hostclass(namespaces, name)
@@ -198,8 +156,59 @@ class Puppet::Resource::TypeCollection
 
   private
 
-  def find_partially_qualified(namespace, name, type)
-    send(type, [namespace, name].join("::"))
+  # Return a list of all possible fully-qualified names that might be
+  # meant by the given name, in the context of namespaces.
+  def resolve_namespaces(namespaces, name)
+    name      = name.downcase
+    if name =~ /^::/
+      # name is explicitly fully qualified, so just return it, sans
+      # initial "::".
+      return [name.sub(/^::/, '')]
+    end
+    if name == ""
+      # The name "" has special meaning--it always refers to a "main"
+      # hostclass which contains all toplevel resources.
+      return [""]
+    end
+
+    namespaces = [namespaces] unless namespaces.is_a?(Array)
+    namespaces = namespaces.collect { |ns| ns.downcase }
+
+    result = []
+    namespaces.each do |namespace|
+      ary = namespace.split("::")
+
+      # Search each namespace nesting in innermost-to-outermost order.
+      while ary.length > 0
+        result << "#{ary.join("::")}::#{name}"
+        ary.pop
+      end
+
+      # Finally, search the toplevel namespace.
+      result << name
+    end
+
+    return result.uniq
+  end
+
+  # Resolve namespaces and find the given object.  Autoload it if
+  # necessary.
+  def find_or_load(namespaces, name, type)
+    resolve_namespaces(namespaces, name).each do |fqname|
+      if result = send(type, fqname)
+        return result
+      end
+      loader.try_load_fqname(fqname) do |filename, modname|
+        if result = send(type, fqname)
+          Puppet.debug "Automatically imported #{name} from #{filename} into #{environment}"
+          result.module_name = modname if modname and result.respond_to?(:module_name=)
+          return result
+        end
+      end
+    end
+
+    # Nothing found.
+    return nil
   end
 
   def munge_name(name)
diff --git a/spec/unit/parser/type_loader_spec.rb b/spec/unit/parser/type_loader_spec.rb
index 8f005d551..b7e174753 100644
--- a/spec/unit/parser/type_loader_spec.rb
+++ b/spec/unit/parser/type_loader_spec.rb
@@ -28,86 +28,28 @@ describe Puppet::Parser::TypeLoader do
   describe "when loading names from namespaces" do
     it "should do nothing if the name to import is an empty string" do
       @loader.expects(:name2files).never
-      @loader.load_until(["foo"], "") { |f| false }.should be_nil
-    end
-
-    it "should turn the provided namespaces and name into a list of files" do
-      @loader.expects(:name2files).with(["foo"], "bar").returns []
-      @loader.load_until(["foo"], "bar") { |f| false }
+      @loader.try_load_fqname("") { |filename, modname| raise :should_not_occur }.should be_nil
     end
 
     it "should attempt to import each generated name" do
-      @loader.expects(:name2files).returns %w{foo bar}
+      @loader.expects(:import).with("foo/bar",nil)
       @loader.expects(:import).with("foo",nil)
-      @loader.expects(:import).with("bar",nil)
-      @loader.load_until(["foo"], "bar") { |f| false }
+      @loader.try_load_fqname("foo::bar") { |f| false }
     end
 
     it "should yield after each import" do
       yielded = []
-      @loader.expects(:name2files).returns %w{foo bar}
-      @loader.expects(:import).with("foo",nil)
-      @loader.expects(:import).with("bar",nil)
-      @loader.load_until(["foo"], "bar") { |f| yielded << f; false }
-      yielded.should == %w{foo bar}
-    end
-
-    it "should stop importing when the yielded block returns true" do
-      yielded = []
-      @loader.expects(:name2files).returns %w{foo bar baz}
-      @loader.expects(:import).with("foo",nil)
-      @loader.expects(:import).with("bar",nil)
-      @loader.expects(:import).with("baz",nil).never
-      @loader.load_until(["foo"], "bar") { |f| true if f == "bar" }
-    end
-
-    it "should return the result of the block" do
-      yielded = []
-      @loader.expects(:name2files).returns %w{foo bar baz}
+      @loader.expects(:import).with("foo/bar",nil)
       @loader.expects(:import).with("foo",nil)
-      @loader.expects(:import).with("bar",nil)
-      @loader.expects(:import).with("baz",nil).never
-      @loader.load_until(["foo"], "bar") { |f| 10 if f == "bar" }.should == 10
-    end
-
-    it "should return nil if the block never returns true" do
-      @loader.expects(:name2files).returns %w{foo bar}
-      @loader.expects(:import).with("foo",nil)
-      @loader.expects(:import).with("bar",nil)
-      @loader.load_until(["foo"], "bar") { |f| false }.should be_nil
+      @loader.try_load_fqname("foo::bar") { |filename, modname| yielded << [filename, modname]; false }
+      yielded.should == [["foo/bar", nil], ["foo", nil]]
     end
 
     it "should know when a given name has been loaded" do
-      @loader.expects(:name2files).returns %w{file}
       @loader.expects(:import).with("file",nil)
-      @loader.load_until(["foo"], "bar") { |f| true }
+      @loader.try_load_fqname("file") { |f| true }
       @loader.should be_loaded("file")
     end
-
-    it "should set the module name on any created resource types" do
-      type = Puppet::Resource::Type.new(:hostclass, "mytype")
-
-      Puppet::Parser::Files.expects(:find_manifests).returns ["modname", %w{one}]
-      @loader.stubs(:parse_file)
-      @loader.load_until(["foo"], "one") { |f| type }
-
-      type.module_name.should == "modname"
-    end
-  end
-
-  describe "when mapping names to files" do
-    {
-      [["foo"], "::bar::baz"] => %w{bar/baz},
-      [[""], "foo::bar"]      => %w{foo foo/bar},
-      [%w{foo}, "bar"]        => %w{foo foo/bar bar},
-      [%w{a b}, "bar"]        => %w{a a/bar b b/bar bar},
-      [%w{a::b::c}, "bar"]    => %w{a a/b/c/bar bar},
-      [%w{a::b}, "foo::bar"]  => %w{a a/b/foo/bar foo/bar}
-    }.each do |inputs, outputs|
-      it "should produce #{outputs.inspect} from the #{inputs[0].inspect} namespace and #{inputs[1]} name" do
-        @loader.name2files(*inputs).should == outputs
-      end
-    end
   end
 
   describe "when importing" do
@@ -198,4 +140,15 @@ describe Puppet::Parser::TypeLoader do
 
     @loader.known_resource_types.hostclass("foo").should be_instance_of(Puppet::Resource::Type)
   end
+
+  describe "when deciding where to look for files" do
+    { 'foo' => ['foo'],
+      'foo::bar' => ['foo/bar', 'foo'],
+      'foo::bar::baz' => ['foo/bar/baz', 'foo/bar', 'foo']
+    }.each do |fqname, expected_paths|
+      it "should look for #{fqname.inspect} in #{expected_paths.inspect}" do
+        @loader.instance_eval { name2files(fqname) }.should == expected_paths
+      end
+    end
+  end
 end
diff --git a/spec/unit/resource/type_collection_spec.rb b/spec/unit/resource/type_collection_spec.rb
index 577aea42b..4bad29116 100644
--- a/spec/unit/resource/type_collection_spec.rb
+++ b/spec/unit/resource/type_collection_spec.rb
@@ -89,6 +89,34 @@ describe Puppet::Resource::TypeCollection do
     loader.node("node").should be_nil
   end
 
+  describe "when resolving namespaces" do
+    [ ['',               '::foo', ['foo']],
+      ['a',              '::foo', ['foo']],
+      ['a::b',           '::foo', ['foo']],
+      [['a::b'],         '::foo', ['foo']],
+      [['a::b', 'c'],    '::foo', ['foo']],
+      [['A::B', 'C'],    '::Foo', ['foo']],
+      ['',               '',      ['']],
+      ['a',              '',      ['']],
+      ['a::b',           '',      ['']],
+      [['a::b'],         '',      ['']],
+      [['a::b', 'c'],    '',      ['']],
+      [['A::B', 'C'],    '',      ['']],
+      ['',               'foo',   ['foo']],
+      ['a',              'foo',   ['a::foo', 'foo']],
+      ['a::b',           'foo',   ['a::b::foo', 'a::foo', 'foo']],
+      ['A::B',           'Foo',   ['a::b::foo', 'a::foo', 'foo']],
+      [['a::b'],         'foo',   ['a::b::foo', 'a::foo', 'foo']],
+      [['a', 'b'],       'foo',   ['a::foo', 'foo', 'b::foo']],
+      [['a::b', 'c::d'], 'foo',   ['a::b::foo', 'a::foo', 'foo', 'c::d::foo', 'c::foo']],
+      [['a::b', 'a::c'], 'foo',   ['a::b::foo', 'a::foo', 'foo', 'a::c::foo']],
+    ].each do |namespaces, name, expected_result|
+      it "should resolve #{name.inspect} in namespaces #{namespaces.inspect} correctly" do
+        @code.instance_eval { resolve_namespaces(namespaces, name) }.should == expected_result
+      end
+    end
+  end
+
   describe "when looking up names" do
     before do
       @type = Puppet::Resource::Type.new(:hostclass, "ns::klass")
@@ -107,29 +135,32 @@ describe Puppet::Resource::TypeCollection do
 
     describe "that need to be loaded" do
       it "should use the loader to load the files" do
-        @code.loader.expects(:load_until).with(["ns"], "klass")
-        @code.find_or_load(["ns"], "klass", :hostclass)
+        @code.loader.expects(:try_load_fqname).with("ns::klass")
+        @code.loader.expects(:try_load_fqname).with("klass")
+        @code.find_hostclass(["ns"], "klass")
       end
 
       it "should downcase the name and downcase and array-fy the namespaces before passing to the loader" do
-        @code.loader.expects(:load_until).with(["ns"], "klass")
-        @code.find_or_load("Ns", "Klass", :hostclass)
+        @code.loader.expects(:try_load_fqname).with("ns::klass")
+        @code.loader.expects(:try_load_fqname).with("klass")
+        @code.find_hostclass("Ns", "Klass")
       end
 
       it "should attempt to find the type when the loader yields" do
-        @code.loader.expects(:load_until).yields
-        @code.expects(:find).with(["ns"], "klass", :hostclass).times(2).returns(false).then.returns(true)
-        @code.find_or_load("ns", "klass", :hostclass)
+        @code.loader.expects(:try_load_fqname).yields
+        @code.expects(:hostclass).with("ns::klass").times(2).returns(false).then.returns(true)
+        @code.find_hostclass("ns", "klass")
       end
 
-      it "should return the result of 'load_until'" do
-        @code.loader.expects(:load_until).returns "foo"
-        @code.find_or_load("Ns", "Klass", :hostclass).should == "foo"
+      it "should return nil if the name isn't found" do
+        @code.stubs(:try_load_fqname).returns(nil)
+        @code.find_hostclass("Ns", "Klass").should be_nil
       end
 
-      it "should return nil if the name isn't found" do
-        @code.stubs(:load_until).returns(nil)
-        @code.find_or_load("Ns", "Klass", :hostclass).should be_nil
+      it "already-loaded names at broader scopes should not shadow autoloaded names" do
+        @code.add Puppet::Resource::Type.new(:hostclass, "bar")
+        @code.loader.expects(:try_load_fqname).with("foo::bar")
+        @code.find_hostclass("foo", "bar")
       end
     end
   end
@@ -195,68 +226,68 @@ describe Puppet::Resource::TypeCollection do
       loader = Puppet::Resource::TypeCollection.new("env")
       instance = Puppet::Resource::Type.new(:hostclass, "foo::bar")
       loader.add instance
-      loader.find("namespace", "::foo::bar", :hostclass).should equal(instance)
+      loader.find_hostclass("namespace", "::foo::bar").should equal(instance)
     end
 
     it "should return nil if the instance name is fully qualified and no such instance exists" do
       loader = Puppet::Resource::TypeCollection.new("env")
-      loader.find("namespace", "::foo::bar", :hostclass).should be_nil
+      loader.find_hostclass("namespace", "::foo::bar").should be_nil
     end
 
     it "should be able to find classes in the base namespace" do
       loader = Puppet::Resource::TypeCollection.new("env")
       instance = Puppet::Resource::Type.new(:hostclass, "foo")
       loader.add instance
-      loader.find("", "foo", :hostclass).should equal(instance)
+      loader.find_hostclass("", "foo").should equal(instance)
     end
 
     it "should return the partially qualified object if it exists in a provided namespace" do
       loader = Puppet::Resource::TypeCollection.new("env")
       instance = Puppet::Resource::Type.new(:hostclass, "foo::bar::baz")
       loader.add instance
-      loader.find("foo", "bar::baz", :hostclass).should equal(instance)
+      loader.find_hostclass("foo", "bar::baz").should equal(instance)
     end
 
     it "should be able to find partially qualified objects in any of the provided namespaces" do
       loader = Puppet::Resource::TypeCollection.new("env")
       instance = Puppet::Resource::Type.new(:hostclass, "foo::bar::baz")
       loader.add instance
-      loader.find(["nons", "foo", "otherns"], "bar::baz", :hostclass).should equal(instance)
+      loader.find_hostclass(["nons", "foo", "otherns"], "bar::baz").should equal(instance)
     end
 
     it "should return the unqualified object if it exists in a provided namespace" do
       loader = Puppet::Resource::TypeCollection.new("env")
       instance = Puppet::Resource::Type.new(:hostclass, "foo::bar")
       loader.add instance
-      loader.find("foo", "bar", :hostclass).should equal(instance)
+      loader.find_hostclass("foo", "bar").should equal(instance)
     end
 
     it "should return the unqualified object if it exists in the parent namespace" do
       loader = Puppet::Resource::TypeCollection.new("env")
       instance = Puppet::Resource::Type.new(:hostclass, "foo::bar")
       loader.add instance
-      loader.find("foo::bar::baz", "bar", :hostclass).should equal(instance)
+      loader.find_hostclass("foo::bar::baz", "bar").should equal(instance)
     end
 
     it "should should return the partially qualified object if it exists in the parent namespace" do
       loader = Puppet::Resource::TypeCollection.new("env")
       instance = Puppet::Resource::Type.new(:hostclass, "foo::bar::baz")
       loader.add instance
-      loader.find("foo::bar", "bar::baz", :hostclass).should equal(instance)
+      loader.find_hostclass("foo::bar", "bar::baz").should equal(instance)
     end
 
     it "should return the qualified object if it exists in the root namespace" do
       loader = Puppet::Resource::TypeCollection.new("env")
       instance = Puppet::Resource::Type.new(:hostclass, "foo::bar::baz")
       loader.add instance
-      loader.find("foo::bar", "foo::bar::baz", :hostclass).should equal(instance)
+      loader.find_hostclass("foo::bar", "foo::bar::baz").should equal(instance)
     end
 
     it "should return nil if the object cannot be found" do
       loader = Puppet::Resource::TypeCollection.new("env")
       instance = Puppet::Resource::Type.new(:hostclass, "foo::bar::baz")
       loader.add instance
-      loader.find("foo::bar", "eh", :hostclass).should be_nil
+      loader.find_hostclass("foo::bar", "eh").should be_nil
     end
 
     describe "when topscope has a class that has the same name as a local class" do
@@ -268,11 +299,11 @@ describe Puppet::Resource::TypeCollection do
       end
 
       it "should favor the local class, if the name is unqualified" do
-        @loader.find("foo", "bar",   :hostclass).name.should == 'foo::bar'
+        @loader.find_hostclass("foo", "bar").name.should == 'foo::bar'
       end
 
       it "should only look in the topclass, if the name is qualified" do
-        @loader.find("foo", "::bar", :hostclass).name.should == 'bar'
+        @loader.find_hostclass("foo", "::bar").name.should == 'bar'
       end
 
     end
@@ -281,15 +312,16 @@ describe Puppet::Resource::TypeCollection do
         @loader = Puppet::Resource::TypeCollection.new("env")
         @loader.add Puppet::Resource::Type.new(:hostclass, "foo::bar")
 
-        @loader.find("foo", "::bar", :hostclass).should == nil
+        @loader.find_hostclass("foo", "::bar").should == nil
     end
 
   end
 
-  it "should use the generic 'find' method with an empty namespace to find nodes" do
+  it "should be able to find nodes" do
+    node = Puppet::Resource::Type.new(:node, "bar")
     loader = Puppet::Resource::TypeCollection.new("env")
-    loader.expects(:find).with("", "bar", :node)
-    loader.find_node(stub("ignored"), "bar")
+    loader.add(node)
+    loader.find_node(stub("ignored"), "bar").should == node
   end
 
   it "should use the 'find_or_load' method to find hostclasses" do
diff --git a/spec/unit/resource/type_spec.rb b/spec/unit/resource/type_spec.rb
index 4d3942c5c..ac9d3811b 100755
--- a/spec/unit/resource/type_spec.rb
+++ b/spec/unit/resource/type_spec.rb
@@ -514,8 +514,7 @@ describe Puppet::Resource::Type do
         @compiler.add_resource @scope, @parent_resource
 
         @type.resource_type_collection = @scope.known_resource_types
-        @type.resource_type_collection.stubs(:node).with("parent").returns(@parent_type)
-        @type.resource_type_collection.stubs(:node).with("Parent").returns(@parent_type)
+        @type.resource_type_collection.add(@parent_type)
       end
 
       it "should evaluate the parent's resource" do