One third done refactoring file[:source] -- retrieve() is done.

It now uses the FileServing::Metadata indirection and
is about 100x cleaner to boot.

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

2 files changed, 238 insertions, 112 deletions

diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index 2514d3d1e..e8db13cfa 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -1,3 +1,7 @@
+
+require 'puppet/file_serving/content'
+require 'puppet/file_serving/metadata'
+
 module Puppet
     # Copy files from a local or remote source.  This state *only* does any work
     # when the remote file is an actual file; in that case, this state copies
@@ -77,61 +81,48 @@ module Puppet
         end
         
         def change_to_s(currentvalue, newvalue)
-            # newvalue = "{md5}" + @stats[:checksum]
+            # newvalue = "{md5}" + @metadata.checksum
             if @resource.property(:ensure).retrieve == :absent
-                return "creating from source %s with contents %s" % [@source, @stats[:checksum]]
+                return "creating from source %s with contents %s" % [@source, @metadata.checksum]
             else
-                return "replacing from source %s with contents %s" % [@source, @stats[:checksum]]
+                return "replacing from source %s with contents %s" % [@source, @metadata.checksum]
             end
         end
         
         def checksum
-            if defined?(@stats)
-                @stats[:checksum]
+            if defined?(@metadata)
+                @metadata.checksum
             else
                 nil
             end
         end
 
-        # Ask the file server to describe our file.
-        def describe(source)
-            sourceobj, path = @resource.uri2obj(source)
-            server = sourceobj.server
+        # Copy the values from the source to the resource.  Yay.
+        def copy_source_values
+            devfail "Somehow got asked to copy source values without any metadata" unless metadata
 
-            begin
-                desc = server.describe(path, @resource[:links])
-            rescue Puppet::Network::XMLRPCClientError => detail
-                fail detail, "Could not describe %s: %s" % [path, detail]
+            # Take each of the stats and set them as states on the local file
+            # if a value has not already been provided.
+            [:owner, :mode, :group].each do |param|
+                @resource[param] ||= metadata.send(param)
             end
 
-            return nil if desc == ""
-
-            # Collect everything except the checksum
-            values = desc.split("\t")
-            other = values.pop
-            args = {}
-            pinparams.zip(values).each { |param, value|
-                if value =~ /^[0-9]+$/
-                    value = value.to_i
-                end
-                unless value.nil?
-                    args[param] = value
-                end
-            }
+            unless @resource.deleting?
+                @resource[:ensure] = metadata.ftype
+            end
 
-            # Now decide whether we're doing checksums or symlinks
-            if args[:type] == "link"
-                args[:target] = other
-            else
-                args[:checksum] = other
+            if metadata.ftype == "link"
+                @resource[:target] = metadata.destination
             end
+        end
 
-            # we can't manage ownership unless we're root, so don't even try
-            unless Puppet::Util::SUIDManager.uid == 0
-                args.delete(:owner)
+        # Ask the file server to describe our file.
+        def describe(source)
+            begin
+                Puppet::FileServing::Metadata.find source
+            rescue => detail
+                fail detail, "Could not retrieve file metadata for %s: %s" % [path, detail]
             end
-            
-            return args
         end
         
         # Use the info we get from describe() to check if we're in sync.
@@ -142,7 +133,7 @@ module Puppet
             
             # the only thing this actual state can do is copy files around.  Therefore,
             # only pay attention if the remote is a file.
-            unless @stats[:type] == "file" 
+            unless @metadata.ftype == "file" 
                 return true
             end
             
@@ -153,15 +144,15 @@ module Puppet
             end
             # Now, we just check to see if the checksums are the same
             parentchecksum = @resource.property(:checksum).retrieve
-            result = (!parentchecksum.nil? and (parentchecksum == @stats[:checksum]))
+            result = (!parentchecksum.nil? and (parentchecksum == @metadata.checksum))
 
             # Diff the contents if they ask it.  This is quite annoying -- we need to do this in
             # 'insync?' because they might be in noop mode, but we don't want to do the file
             # retrieval twice, so we cache the value.
-            if ! result and Puppet[:show_diff] and File.exists?(@resource[:path]) and ! @stats[:_diffed]
-                @stats[:_remote_content] = get_remote_content
-                string_file_diff(@resource[:path], @stats[:_remote_content])
-                @stats[:_diffed] = true
+            if ! result and Puppet[:show_diff] and File.exists?(@resource[:path]) and ! @metadata._diffed
+                @metadata._remote_content = get_remote_content
+                string_file_diff(@resource[:path], @metadata._remote_content)
+                @metadata._diffed = true
             end
             return result
         end
@@ -171,55 +162,34 @@ module Puppet
         end
 
         def found?
-            ! (@stats.nil? or @stats[:type].nil?)
+            ! (@metadata.nil? or @metadata.ftype.nil?)
         end
 
-        # This basically calls describe() on our file, and then sets all
-        # of the local states appropriately.  If the remote file is a normal
-        # file then we set it to copy; if it's a directory, then we just mark
-        # that the local directory should be created.
-        def retrieve(remote = true)
-            sum = nil
-            @source = nil
-
-            # This is set to false by the File#retrieve function on the second
-            # retrieve, so that we do not do two describes.
-            if remote
-                # Find the first source that exists.  @shouldorig contains
-                # the sources as specified by the user.
-                @should.each { |source|
-                    if @stats = self.describe(source)
-                        @source = source
-                        break
+        # Provide, and retrieve if necessary, the metadata for this file.  Fail
+        # if we can't find data about this host, and fail if there are any
+        # problems in our query.
+        attr_writer :metadata
+        def metadata
+            unless defined?(@metadata) and @metadata
+                return @metadata = nil unless should
+                should.each do |source|
+                    begin
+                        if data = Puppet::FileServing::Metadata.find(source)
+                            @metadata = data
+                            @metadata.source = source
+                            break
+                        end
+                    rescue => detail
+                        fail detail, "Could not retrieve file metadata for %s: %s" % [source, detail]
                     end
-                }
-            end
-
-            if !found?
-                raise Puppet::Error, "No specified source was found from" + @should.inject("") { |s, source| s + " #{source},"}.gsub(/,$/,"")
-            end
-            
-            case @stats[:type]
-            when "directory", "file", "link":
-                @resource[:ensure] = @stats[:type] unless @resource.deleting?
-            else
-                self.info @stats.inspect
-                self.err "Cannot use files of type %s as sources" % @stats[:type]
-                return :nocopy
+                end
+                fail "Could not retrieve information from source(s) %s" % @should.join(", ") unless @metadata
             end
+            return @metadata
+        end
 
-            # Take each of the stats and set them as states on the local file
-            # if a value has not already been provided.
-            @stats.each { |stat, value|
-                next if stat == :checksum
-                next if stat == :type
-
-                # was the stat already specified, or should the value
-                # be inherited from the source?
-                @resource[stat] = value unless @resource.argument?(stat)
-            }
-
-            return @stats[:checksum]
+        def retrieve
+            copy_source_values
         end
         
         def should
@@ -238,11 +208,13 @@ module Puppet
         end
 
         def sync
-            contents = @stats[:_remote_content] || get_remote_content()
-
             exists = File.exists?(@resource[:path])
 
-            @resource.write(contents, :source, @stats[:checksum])
+            if content = Puppet::FileServing::Content.find(@metadata.source)
+                @resource.write(content.content, :source, @metadata.checksum)
+            else
+                raise "Could not retrieve content"
+            end
 
             if exists
                 return :file_changed
@@ -250,27 +222,5 @@ module Puppet
                 return :file_created
             end
         end
-
-        private
-
-        def get_remote_content
-            raise Puppet::DevError, "Got told to copy non-file %s" % @resource[:path] unless @stats[:type] == "file"
-
-            sourceobj, path = @resource.uri2obj(@source)
-
-            begin
-                contents = sourceobj.server.retrieve(path, @resource[:links])
-            rescue => detail
-                self.fail "Could not retrieve %s: %s" % [path, detail]
-            end
-
-            contents = CGI.unescape(contents) unless sourceobj.server.local
-
-            if contents == ""
-                self.notice "Could not retrieve contents for %s" % @source
-            end
-
-            return contents
-        end
     end
 end
diff --git a/spec/unit/type/file/source.rb b/spec/unit/type/file/source.rb
new file mode 100755
index 000000000..bb689ce76
--- /dev/null
+++ b/spec/unit/type/file/source.rb
@@ -0,0 +1,176 @@
+#!/usr/bin/env ruby
+
+Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/spec_helper.rb") }
+
+source = Puppet::Type.type(:file).attrclass(:source)
+describe Puppet::Type.type(:file).attrclass(:source) do
+    before do
+        # Wow that's a messy interface to the resource.
+        @resource = stub 'resource', :uri2obj => true, :[]= => nil, :property => nil
+    end
+
+    it "should be a subclass of Property" do
+        source.superclass.must == Puppet::Property
+    end
+
+    describe "when initializing" do
+        it "should fail if the 'should' values are not URLs" do
+            @resource.expects(:uri2obj).with("foo").returns false
+
+            lambda { source.new(:resource => @resource, :should => %w{foo}) }.must raise_error(Puppet::Error)
+        end
+    end
+
+    it "should have a method for retrieving its metadata" do
+        source.new(:resource => @resource).must respond_to(:metadata)
+    end
+
+    it "should have a method for setting its metadata" do
+        source.new(:resource => @resource).must respond_to(:metadata=)
+    end
+
+    describe "when returning the metadata" do
+        before do
+            @metadata = stub 'metadata', :source= => nil
+        end
+
+        it "should return already-available metadata" do
+            @source = source.new(:resource => @resource)
+            @source.metadata = "foo"
+            @source.metadata.should == "foo"
+        end
+
+        it "should return nil if no @should value is set and no metadata is available" do
+            @source = source.new(:resource => @resource)
+            @source.metadata.should be_nil
+        end
+
+        it "should collect its metadata using the Metadata class if it is not already set" do
+            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns @metadata
+            @source.metadata
+        end
+
+        it "should use the metadata from the first found source" do
+            metadata = stub 'metadata', :source= => nil
+            @source = source.new(:resource => @resource, :should => ["/foo/bar", "/fee/booz"])
+            Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns nil
+            Puppet::FileServing::Metadata.expects(:find).with("/fee/booz").returns metadata
+            @source.metadata.should equal(metadata)
+        end
+
+        it "should store the found source as the metadata's source" do
+            metadata = mock 'metadata'
+            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns metadata
+
+            metadata.expects(:source=).with("/foo/bar")
+            @source.metadata
+        end
+
+        it "should fail intelligently if an exception is encountered while querying for metadata" do
+            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").raises RuntimeError
+
+            @source.expects(:fail).raises ArgumentError
+            lambda { @source.metadata }.should raise_error(ArgumentError)
+        end
+
+        it "should fail if no specified sources can be found" do
+            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns nil
+
+            @source.expects(:fail).raises RuntimeError
+
+            lambda { @source.metadata }.should raise_error(RuntimeError)
+        end
+    end
+
+    it "should have a method for setting the desired values on the resource" do
+        source.new(:resource => @resource).must respond_to(:copy_source_values)
+    end
+
+    describe "when copying the source values" do
+        before do
+            @metadata = stub 'metadata', :owner => 100, :group => 200, :mode => 123
+
+            @source = source.new(:resource => @resource)
+            @source.metadata = @metadata
+
+            @resource.stubs(:deleting?).returns false
+        end
+
+        it "should fail if there is no metadata" do
+            @source.metadata = nil
+            @source.expects(:devfail).raises ArgumentError
+            lambda { @source.copy_source_values }.should raise_error(ArgumentError)
+        end
+
+        it "should set :ensure to the file type if the resource is not being deleted" do
+            @resource.expects(:deleting?).returns false
+            @resource.stubs(:[])
+            @resource.stubs(:[]=)
+            @metadata.stubs(:ftype).returns "foobar"
+
+            @resource.expects(:[]=).with(:ensure, "foobar")
+            @source.copy_source_values
+        end
+
+        it "should not set :ensure to the file type if the resource is being deleted" do
+            @resource.expects(:deleting?).returns true
+            @resource.stubs(:[])
+            @resource.stubs(:[]).returns "foo"
+            @metadata.expects(:ftype).returns "foobar"
+
+            @resource.expects(:[]=).with(:ensure, "foobar").never
+            @source.copy_source_values
+        end
+
+        describe "and the source is a file" do
+            before do
+                @metadata.stubs(:ftype).returns "file"
+            end
+
+            it "should copy the metadata's owner, group, and mode to the resource if they are not set on the resource" do
+                @resource.stubs(:[]).returns nil
+
+                @resource.expects(:[]=).with(:owner, 100)
+                @resource.expects(:[]=).with(:group, 200)
+                @resource.expects(:[]=).with(:mode, 123)
+
+                @source.copy_source_values
+            end
+
+            it "should not copy the metadata's owner to the resource if it is already set" do
+                @resource.stubs(:[]).returns "value"
+                @resource.expects(:[]).returns "value"
+
+                @resource.expects(:[]=).never
+
+                @source.copy_source_values
+            end
+        end
+
+        describe "and the source is a link" do
+            it "should set the target to the link destination" do
+                @metadata.stubs(:ftype).returns "link"
+                @resource.stubs(:[])
+                @resource.stubs(:[]=)
+
+                @metadata.expects(:destination).returns "/path/to/symlink"
+
+                @resource.expects(:[]=).with(:target, "/path/to/symlink")
+                @source.copy_source_values
+            end
+        end
+    end
+
+    describe "when retrieving the property state" do
+        it "should copy all metadata to the resource" do
+            @source = source.new(:resource => @resource)
+            @source.expects(:copy_source_values)
+
+            @source.retrieve
+        end
+    end
+end