Refactoring how files in FileServing are named.

Previously, they retained some concept of the URI used to
find them, and this uri was the primary key
for the FileServing instances.  This key was unfortunately
completely useless, as evidenced by the fact that it was never
used except to test that it worked.

I've modified the FileServing instances (through modifying
the Base class) to use their local path as their key, and they
no longer care about the URI at all.

This commit is mostly about fixing the code that interacts with
the instances to use this new API.

Signed-off-by: Luke Kanies <luke@madstop.com>

16 files changed, 56 insertions, 102 deletions

diff --git a/lib/puppet/file_serving/base.rb b/lib/puppet/file_serving/base.rb
index 1cfd0bc4c..94e337b99 100644
--- a/lib/puppet/file_serving/base.rb
+++ b/lib/puppet/file_serving/base.rb
@@ -7,8 +7,6 @@ require 'puppet/file_serving'
 # The base class for Content and Metadata; provides common
 # functionality like the behaviour around links.
 class Puppet::FileServing::Base
-    attr_accessor :key
-
     # Does our file exist?
     def exist?
         begin
@@ -21,8 +19,6 @@ class Puppet::FileServing::Base
 
     # Return the full path to our file.  Fails if there's no path set.
     def full_path
-        raise(ArgumentError, "You must set a path to get a file's path") unless self.path
-
         if relative_path.nil? or relative_path == ""
             path
         else
@@ -30,8 +26,8 @@ class Puppet::FileServing::Base
         end
     end
 
-    def initialize(key, options = {})
-        @key = key
+    def initialize(path, options = {})
+        self.path = path
         @links = :manage
 
         options.each do |param, value|
diff --git a/lib/puppet/file_serving/content.rb b/lib/puppet/file_serving/content.rb
index 64f000eaa..f13fcaa72 100644
--- a/lib/puppet/file_serving/content.rb
+++ b/lib/puppet/file_serving/content.rb
@@ -25,7 +25,6 @@ class Puppet::FileServing::Content < Puppet::FileServing::Base
             # This stat can raise an exception, too.
             raise(ArgumentError, "Cannot read the contents of links unless following links") if stat().ftype == "symlink" 
 
-            p full_path
             @content = ::File.read(full_path())
         end
         @content
diff --git a/lib/puppet/file_serving/terminus_helper.rb b/lib/puppet/file_serving/terminus_helper.rb
index e5da0e29f..bde0bd389 100644
--- a/lib/puppet/file_serving/terminus_helper.rb
+++ b/lib/puppet/file_serving/terminus_helper.rb
@@ -11,7 +11,7 @@ module Puppet::FileServing::TerminusHelper
     def path2instances(request, path)
         args = [:links, :ignore, :recurse].inject({}) { |hash, param| hash[param] = request.options[param] if request.options[param]; hash }
         Puppet::FileServing::Fileset.new(path, args).files.collect do |file|
-            inst = model.new(File.join(request.key, file), :path => path, :relative_path => file)
+            inst = model.new(path, :relative_path => file)
             inst.links = request.options[:links] if request.options[:links]
             inst
         end
diff --git a/lib/puppet/indirector/direct_file_server.rb b/lib/puppet/indirector/direct_file_server.rb
index b3b4886f3..bcda92366 100644
--- a/lib/puppet/indirector/direct_file_server.rb
+++ b/lib/puppet/indirector/direct_file_server.rb
@@ -12,16 +12,14 @@ class Puppet::Indirector::DirectFileServer < Puppet::Indirector::Terminus
     include Puppet::FileServing::TerminusHelper
 
     def find(request)
-        uri = key2uri(request.key)
-        return nil unless FileTest.exists?(uri.path)
-        instance = model.new(request.key, :path => uri.path)
+        return nil unless FileTest.exists?(request.key)
+        instance = model.new(request.key)
         instance.links = request.options[:links] if request.options[:links]
         return instance
     end
 
     def search(request)
-        uri = key2uri(request.key)
-        return nil unless FileTest.exists?(uri.path)
-        path2instances(request, uri.path)
+        return nil unless FileTest.exists?(request.key)
+        path2instances(request, request.key)
     end
 end
diff --git a/lib/puppet/indirector/file_server.rb b/lib/puppet/indirector/file_server.rb
index b0df7ff5d..476fc5b23 100644
--- a/lib/puppet/indirector/file_server.rb
+++ b/lib/puppet/indirector/file_server.rb
@@ -25,7 +25,7 @@ class Puppet::Indirector::FileServer < Puppet::Indirector::Terminus
     # Find our key using the fileserver.
     def find(request)
         return nil unless path = find_path(request)
-        result =  model.new(request.key, :path => path)
+        result =  model.new(path)
         result.links = request.options[:links] if request.options[:links]
         return result
     end
diff --git a/lib/puppet/indirector/module_files.rb b/lib/puppet/indirector/module_files.rb
index 40dd06941..7c5cf278f 100644
--- a/lib/puppet/indirector/module_files.rb
+++ b/lib/puppet/indirector/module_files.rb
@@ -30,7 +30,7 @@ class Puppet::Indirector::ModuleFiles < Puppet::Indirector::Terminus
     def find(request)
         return nil unless path = find_path(request)
 
-        result = model.new(request.key, :path => path)
+        result = model.new(path)
         result.links = request.options[:links] if request.options[:links]
         return result
     end
diff --git a/spec/integration/file_serving/metadata.rb b/spec/integration/file_serving/metadata.rb
index 067cb566a..af3e16324 100755
--- a/spec/integration/file_serving/metadata.rb
+++ b/spec/integration/file_serving/metadata.rb
@@ -15,4 +15,6 @@ describe Puppet::FileServing::Metadata, " when finding files" do
         @test_class = Puppet::FileServing::Metadata
         @indirection = Puppet::FileServing::Metadata.indirection
     end
+
+    after { Puppet::Util::Cacher.invalidate }
 end
diff --git a/spec/integration/indirector/direct_file_server.rb b/spec/integration/indirector/direct_file_server.rb
index 40b753a6c..6f3da5169 100755
--- a/spec/integration/indirector/direct_file_server.rb
+++ b/spec/integration/indirector/direct_file_server.rb
@@ -68,12 +68,12 @@ describe Puppet::Indirector::DirectFileServer, " when interacting with FileServi
         end
 
         @terminus.search(@request).each do |instance|
-            case instance.key
+            case instance.full_path
             when /one/: instance.content.should == "one content"
             when /two/: instance.content.should == "two content"
             when /\.$/: 
             else
-                raise "No valid key for %s" % instance.key.inspect
+                raise "No valid key for %s" % instance.path.inspect
             end
         end
     end
diff --git a/spec/integration/indirector/module_files.rb b/spec/integration/indirector/module_files.rb
index ae14aa5a7..6cbbd3dbd 100755
--- a/spec/integration/indirector/module_files.rb
+++ b/spec/integration/indirector/module_files.rb
@@ -20,7 +20,7 @@ describe Puppet::Indirector::ModuleFiles, " when interacting with Puppet::Module
 
         FileTest.expects(:exists?).with(filepath).returns(true)
 
-        @request = Puppet::Indirector::Request.new(:content, :find, "puppetmounts://host/modules/mymod/myfile")
+        @request = Puppet::Indirector::Request.new(:content, :find, "puppet://host/modules/mymod/myfile")
 
         @terminus.find(@request).should be_instance_of(Puppet::FileServing::Content)
     end
@@ -47,7 +47,7 @@ describe Puppet::Indirector::ModuleFiles, " when interacting with FileServing::F
 
         Dir.expects(:entries).with(filepath).returns(%w{one two})
 
-        @request = Puppet::Indirector::Request.new(:content, :search, "puppetmounts://host/modules/mymod/myfile", :recurse => true)
+        @request = Puppet::Indirector::Request.new(:content, :search, "puppet://host/modules/mymod/myfile", :recurse => true)
 
         result = @terminus.search(@request)
         result.should be_instance_of(Array)
diff --git a/spec/unit/file_serving/base.rb b/spec/unit/file_serving/base.rb
index 9d900e30a..7eccb3a52 100755
--- a/spec/unit/file_serving/base.rb
+++ b/spec/unit/file_serving/base.rb
@@ -5,64 +5,57 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/file_serving/base'
 
 describe Puppet::FileServing::Base do
-    it "should accept a key in the form of a URI" do
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file").key.should == "puppet://host/module/dir/file"
+    it "should accept a path" do
+        Puppet::FileServing::Base.new("/module/dir/file").path.should == "/module/dir/file"
+    end
+
+    it "should require that paths be fully qualified" do
+        lambda { Puppet::FileServing::Base.new("module/dir/file") }.should raise_error(ArgumentError)
     end
 
     it "should allow specification of whether links should be managed" do
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :links => :manage).links.should == :manage
+        Puppet::FileServing::Base.new("/module/dir/file", :links => :manage).links.should == :manage
     end
 
     it "should consider :ignore links equivalent to :manage links" do
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :links => :ignore).links.should == :manage
+        Puppet::FileServing::Base.new("/module/dir/file", :links => :ignore).links.should == :manage
     end
 
     it "should fail if :links is set to anything other than :manage, :follow, or :ignore" do
-        proc { Puppet::FileServing::Base.new("puppet://host/module/dir/file", :links => :else) }.should raise_error(ArgumentError)
+        proc { Puppet::FileServing::Base.new("/module/dir/file", :links => :else) }.should raise_error(ArgumentError)
     end
 
     it "should default to :manage for :links" do
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file").links.should == :manage
+        Puppet::FileServing::Base.new("/module/dir/file").links.should == :manage
     end
 
     it "should allow specification of a path" do
         FileTest.stubs(:exists?).returns(true)
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :path => "/my/file").path.should == "/my/file"
+        Puppet::FileServing::Base.new("/module/dir/file", :path => "/my/file").path.should == "/my/file"
     end
 
     it "should allow specification of a relative path" do
         FileTest.stubs(:exists?).returns(true)
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :relative_path => "my/file").relative_path.should == "my/file"
+        Puppet::FileServing::Base.new("/module/dir/file", :relative_path => "my/file").relative_path.should == "my/file"
     end
 
     it "should have a means of determining if the file exists" do
-        Puppet::FileServing::Base.new("blah").should respond_to(:exist?)
+        Puppet::FileServing::Base.new("/blah").should respond_to(:exist?)
     end
 
     it "should correctly indicate if the file is present" do
         File.expects(:lstat).with("/my/file").returns(mock("stat"))
-        Puppet::FileServing::Base.new("blah", :path => "/my/file").exist?.should be_true
+        Puppet::FileServing::Base.new("/my/file").exist?.should be_true
     end
 
-    it "should correctly indicate if the file is asbsent" do
+    it "should correctly indicate if the file is absent" do
         File.expects(:lstat).with("/my/file").raises RuntimeError
-        Puppet::FileServing::Base.new("blah", :path => "/my/file").exist?.should be_false
-    end
-
-    describe "when setting the base path" do
-        before do
-            @file = Puppet::FileServing::Base.new("puppet://host/module/dir/file")
-        end
-
-        it "should require that the base path be fully qualified" do
-            FileTest.stubs(:exists?).returns(true)
-            proc { @file.path = "unqualified/file" }.should raise_error(ArgumentError)
-        end
+        Puppet::FileServing::Base.new("/my/file").exist?.should be_false
     end
 
     describe "when setting the relative path" do
         it "should require that the relative path be unqualified" do
-            @file = Puppet::FileServing::Base.new("puppet://host/module/dir/file")
+            @file = Puppet::FileServing::Base.new("/module/dir/file")
             FileTest.stubs(:exists?).returns(true)
             proc { @file.relative_path = "/qualified/file" }.should raise_error(ArgumentError)
         end
@@ -70,7 +63,7 @@ describe Puppet::FileServing::Base do
 
     describe "when determining the full file path" do
         before do
-            @file = Puppet::FileServing::Base.new("mykey", :path => "/this/file")
+            @file = Puppet::FileServing::Base.new("/this/file")
         end
 
         it "should return the path if there is no relative path" do
@@ -86,16 +79,11 @@ describe Puppet::FileServing::Base do
             @file.relative_path = "not/qualified"
             @file.full_path.should == "/this/file/not/qualified"
         end
-
-        it "should should fail if there is no path set" do
-            @file = Puppet::FileServing::Base.new("not/qualified")
-            proc { @file.full_path }.should raise_error(ArgumentError)
-        end
     end
 
     describe "when stat'ing files" do
         before do
-            @file = Puppet::FileServing::Base.new("mykey", :path => "/this/file")
+            @file = Puppet::FileServing::Base.new("/this/file")
         end
 
         it "should stat the file's full path" do
diff --git a/spec/unit/file_serving/content.rb b/spec/unit/file_serving/content.rb
index 82353d23d..a63f7efab 100755
--- a/spec/unit/file_serving/content.rb
+++ b/spec/unit/file_serving/content.rb
@@ -18,15 +18,15 @@ describe Puppet::FileServing::Content do
     end
 
     it "should have a method for collecting its attributes" do
-        Puppet::FileServing::Content.new("sub/path", :path => "/base").should respond_to(:collect)
+        Puppet::FileServing::Content.new("/path").should respond_to(:collect)
     end
 
     it "should retrieve and store its contents when its attributes are collected" do
-        content = Puppet::FileServing::Content.new("sub/path", :path => "/base")
+        content = Puppet::FileServing::Content.new("/path")
 
         result = "foo"
         File.stubs(:lstat).returns(stub("stat", :ftype => "file"))
-        File.expects(:read).with("/base/sub/path").returns result
+        File.expects(:read).with("/path").returns result
         content.collect
         content.content.should equal(result)
     end
@@ -34,8 +34,8 @@ end
 
 describe Puppet::FileServing::Content, "when returning the contents" do
     before do
-        @path = "/my/base"
-        @content = Puppet::FileServing::Content.new("sub/path", :links => :follow, :path => @path)
+        @path = "/my/path"
+        @content = Puppet::FileServing::Content.new(@path, :links => :follow)
     end
 
     it "should fail if the file is a symlink and links are set to :manage" do
diff --git a/spec/unit/file_serving/metadata.rb b/spec/unit/file_serving/metadata.rb
index e76ceb3e8..f0e15873f 100755
--- a/spec/unit/file_serving/metadata.rb
+++ b/spec/unit/file_serving/metadata.rb
@@ -20,25 +20,22 @@ end
 
 describe Puppet::FileServing::Metadata, " when finding the file to use for setting attributes" do
     before do
-        @metadata = Puppet::FileServing::Metadata.new("my/path")
-
-        @full = "/base/path/my/path"
-
-        @metadata.path = @full
+        @path = "/my/path"
+        @metadata = Puppet::FileServing::Metadata.new(@path)
 
         # Use a link because it's easier to test -- no checksumming
         @stat = stub "stat", :uid => 10, :gid => 20, :mode => 0755, :ftype => "link"
     end
 
     it "should accept a base path path to which the file should be relative" do
-        File.expects(:lstat).with(@full).returns @stat
-        File.expects(:readlink).with(@full).returns "/what/ever"
+        File.expects(:lstat).with(@path).returns @stat
+        File.expects(:readlink).with(@path).returns "/what/ever"
         @metadata.collect_attributes
     end
 
     it "should use the set base path if one is not provided" do
-        File.expects(:lstat).with(@full).returns @stat
-        File.expects(:readlink).with(@full).returns "/what/ever"
+        File.expects(:lstat).with(@path).returns @stat
+        File.expects(:readlink).with(@path).returns "/what/ever"
         @metadata.collect_attributes()
     end
 
@@ -47,7 +44,7 @@ describe Puppet::FileServing::Metadata, " when finding the file to use for setti
     end
 
     it "should raise an exception if the file does not exist" do
-        File.expects(:lstat).with(@full).raises(Errno::ENOENT)
+        File.expects(:lstat).with(@path).raises(Errno::ENOENT)
         proc { @metadata.collect_attributes()}.should raise_error(Errno::ENOENT)
     end
 end
@@ -59,7 +56,7 @@ describe Puppet::FileServing::Metadata, " when collecting attributes" do
         @stat = stub 'stat', :uid => 10, :gid => 20, :mode => 33261, :ftype => "file"
         File.stubs(:lstat).returns(@stat)
         @checksum = Digest::MD5.hexdigest("some content\n")
-        @metadata = Puppet::FileServing::Metadata.new("file", :path => "/my/file")
+        @metadata = Puppet::FileServing::Metadata.new("/my/file")
         @metadata.stubs(:md5_file).returns(@checksum)
         @metadata.collect_attributes
     end
@@ -140,7 +137,7 @@ end
 
 describe Puppet::FileServing::Metadata, " when pointing to a link" do
     it "should store the destination of the link in :destination if links are :manage" do
-        file = Puppet::FileServing::Metadata.new("mykey", :links => :manage, :path => "/base/path/my/file")
+        file = Puppet::FileServing::Metadata.new("/base/path/my/file", :links => :manage)
 
         File.expects(:lstat).with("/base/path/my/file").returns stub("stat", :uid => 1, :gid => 2, :ftype => "link", :mode => 0755)
         File.expects(:readlink).with("/base/path/my/file").returns "/some/other/path"
@@ -150,7 +147,7 @@ describe Puppet::FileServing::Metadata, " when pointing to a link" do
     end
 
     it "should not collect the checksum" do
-        file = Puppet::FileServing::Metadata.new("my/file", :links => :manage, :path => "/base/path/my/file")
+        file = Puppet::FileServing::Metadata.new("/base/path/my/file", :links => :manage)
 
         File.expects(:lstat).with("/base/path/my/file").returns stub("stat", :uid => 1, :gid => 2, :ftype => "link", :mode => 0755)
         File.expects(:readlink).with("/base/path/my/file").returns "/some/other/path"
@@ -159,13 +156,3 @@ describe Puppet::FileServing::Metadata, " when pointing to a link" do
         file.checksum.should be_nil
     end
 end
-
-describe Puppet::FileServing::Metadata, " when converting from yaml" do
-    # LAK:FIXME This isn't in the right place, but we need some kind of
-    # control somewhere that requires that all REST connections only pull
-    # from the file-server, thus guaranteeing they go through our authorization
-    # hook.
-    it "should set the URI scheme to 'puppetmounts'" do
-        pending "We need to figure out where this should be"
-    end
-end
diff --git a/spec/unit/file_serving/terminus_helper.rb b/spec/unit/file_serving/terminus_helper.rb
index 763ce9eb0..aa9dca961 100755
--- a/spec/unit/file_serving/terminus_helper.rb
+++ b/spec/unit/file_serving/terminus_helper.rb
@@ -45,15 +45,9 @@ describe Puppet::FileServing::TerminusHelper do
             @helper.path2instances(@request, "/my/file").length.should == 2
         end
 
-        it "should set each instance's key to be the original key plus the file-specific path" do
-            @model.expects(:new).with { |key, options| key == @request.key + "/one" }.returns(:one)
-            @model.expects(:new).with { |key, options| key == @request.key + "/two" }.returns(:two)
-            @helper.path2instances(@request, "/my/file")
-        end
-
         it "should set each returned instance's path to the original path" do
-            @model.expects(:new).with { |key, options| options[:path] == "/my/file" }.returns(:one)
-            @model.expects(:new).with { |key, options| options[:path] == "/my/file" }.returns(:two)
+            @model.expects(:new).with { |key, options| key == "/my/file" }.returns(:one)
+            @model.expects(:new).with { |key, options| key == "/my/file" }.returns(:two)
             @helper.path2instances(@request, "/my/file")
         end
 
diff --git a/spec/unit/indirector/direct_file_server.rb b/spec/unit/indirector/direct_file_server.rb
index 0753f1bbe..b80519bbe 100755
--- a/spec/unit/indirector/direct_file_server.rb
+++ b/spec/unit/indirector/direct_file_server.rb
@@ -24,7 +24,7 @@ describe Puppet::Indirector::DirectFileServer do
 
         @uri = "file:///my/local"
 
-        @request = stub 'request', :key => @uri, :options => {}
+        @request = Puppet::Indirector::Request.new(:mytype, :find, @uri)
     end
 
     describe Puppet::Indirector::DirectFileServer, "when finding a single file" do
@@ -49,13 +49,8 @@ describe Puppet::Indirector::DirectFileServer do
             FileTest.expects(:exists?).with("/my/local").returns true
         end
 
-        it "should create the Content instance with the original key as the key" do
-            @model.expects(:new).with { |key, options| key == @uri }.returns(@data)
-            @server.find(@request)
-        end
-
         it "should pass the full path to the instance" do
-            @model.expects(:new).with { |key, options| options[:path] == "/my/local" }.returns(@data)
+            @model.expects(:new).with { |key, options| key == "/my/local" }.returns(@data)
             @server.find(@request)
         end
 
diff --git a/spec/unit/indirector/file_metadata/file.rb b/spec/unit/indirector/file_metadata/file.rb
index 9474620c7..b37c42c72 100755
--- a/spec/unit/indirector/file_metadata/file.rb
+++ b/spec/unit/indirector/file_metadata/file.rb
@@ -24,7 +24,7 @@ describe Puppet::Indirector::FileMetadata::File do
             @data.stubs(:collect_attributes)
             FileTest.expects(:exists?).with("/my/local").returns true
 
-            @request = stub 'request', :key => @uri, :options => {}
+            @request = Puppet::Indirector::Request.new(:file_metadata, :find, @uri)
         end
 
         it "should collect its attributes when a file is found" do
@@ -40,7 +40,7 @@ describe Puppet::Indirector::FileMetadata::File do
             @metadata = Puppet::Indirector::FileMetadata::File.new
             @uri = "file:///my/local"
 
-            @request = stub 'request', :key => @uri, :options => {}
+            @request = Puppet::Indirector::Request.new(:file_metadata, :find, @uri)
         end
 
         it "should collect the attributes of the instances returned" do
diff --git a/spec/unit/indirector/file_server.rb b/spec/unit/indirector/file_server.rb
index 544953932..90fb84d35 100755
--- a/spec/unit/indirector/file_server.rb
+++ b/spec/unit/indirector/file_server.rb
@@ -67,13 +67,8 @@ describe Puppet::Indirector::FileServer do
             @instance = mock 'instance'
         end
 
-        it "should create the instance with the key used to find the instance" do
-            @model.expects(:new).with { |key, *options| key == "my/local/file" }
-            @file_server.find(@request)
-        end
-
         it "should create the instance with the path at which the instance was found" do
-            @model.expects(:new).with { |key, options| options[:path] == "/some/file" }
+            @model.expects(:new).with { |key, options| key == "/some/file" }
             @file_server.find(@request)
         end