[#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.

3 files changed, 80 insertions, 96 deletions

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