Converted File[checksum] to a parameter not property

At the same time I removed all of the code in checksum
that managed tracking changes to the checksum over time.
I'll add it back in as I fix the fact that changes aren't
being tracked like the should at the moment.

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

8 files changed, 119 insertions, 333 deletions

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 895c0b207..efac1c835 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -722,12 +722,6 @@ Puppet::Type.newtype(:file) do
     # Write out the file.  Requires the content to be written,
     # the property name for logging, and the checksum for validation.
     def write(content, property, checksum = nil)
-        if validate = validate_checksum?
-            # Use the appropriate checksum type -- md5, md5lite, etc.
-            sumtype = property(:checksum).checktype
-            checksum ||= "{#{sumtype}}" + property(:checksum).send(sumtype, content)
-        end
-
         remove_existing(:file)
 
         use_temporary_file = (content.length != 0)
@@ -750,7 +744,7 @@ Puppet::Type.newtype(:file) do
         # And put our new file in place
         if use_temporary_file # This is only not true when our file is empty.
             begin
-                fail_if_checksum_is_wrong(path, checksum) if validate
+                fail_if_checksum_is_wrong(path, content) if validate
                 File.rename(path, self[:path])
             rescue => detail
                 fail "Could not rename temporary file %s to %s : %s" % [path, self[:path], detail]
@@ -763,32 +757,17 @@ Puppet::Type.newtype(:file) do
         # make sure all of the modes are actually correct
         property_fix
 
-        # And then update our checksum, so the next run doesn't find it.
-        self.setchecksum(checksum)
-    end
-
-    # Should we validate the checksum of the file we're writing?
-    def validate_checksum?
-        if sumparam = @parameters[:checksum]
-            return sumparam.checktype.to_s !~ /time/
-        else
-            return false
-        end
     end
 
     private
 
     # Make sure the file we wrote out is what we think it is.
     def fail_if_checksum_is_wrong(path, checksum)
-        if checksum =~ /^\{(\w+)\}.+/
-            sumtype = $1
-        else
-            # This shouldn't happen, but if it happens to, it's nicer
-            # to just use a default sumtype than fail.
-            sumtype = "md5"
-        end
-        newsum = property(:checksum).getsum(sumtype, path)
-        return if newsum == checksum
+        # Use the appropriate checksum type -- md5, md5lite, etc.
+        checksum = parameter(:checksum).sum(content)
+
+        newsum = parameter(:checksum).sum_file(path)
+        return if [:absent, nil, checksum].include?(newsum)
 
         self.fail "File written to disk did not match checksum; discarding changes (%s vs %s)" % [checksum, newsum]
     end
diff --git a/lib/puppet/type/file/checksum.rb b/lib/puppet/type/file/checksum.rb
index 0c45aad32..d4c24886c 100755
--- a/lib/puppet/type/file/checksum.rb
+++ b/lib/puppet/type/file/checksum.rb
@@ -1,250 +1,26 @@
 require 'puppet/util/checksums'
 
-# Keep a copy of the file checksums, and notify when they change.  This
-# property never actually modifies the system, it only notices when the system
-# changes on its own.
-Puppet::Type.type(:file).newproperty(:checksum) do
+# Specify which checksum algorithm to use when checksumming
+# files.
+Puppet::Type.type(:file).newparam(:checksum) do
     include Puppet::Util::Checksums
 
-    desc "How to check whether a file has changed.  This state is used internally
-        for file copying, but it can also be used to monitor files somewhat
-        like Tripwire without managing the file contents in any way.  You can
-        specify that a file's checksum should be monitored and then subscribe to
-        the file from another object and receive events to signify
-        checksum changes, for instance.
-
-        There are a number of checksum types available including MD5 hashing (and
-        an md5lite variation that only hashes the first 500 characters of the
-        file.
+    desc "The checksum type to use when checksumming a file.
 
         The default checksum parameter, if checksums are enabled, is md5."
 
-    @event = :file_changed
-
-    @unmanaged = true
-
-    @validtypes = %w{md5 md5lite timestamp mtime time none}
-
-    def self.validtype?(type)
-        @validtypes.include?(type)
-    end
-
-    @validtypes.each do |ctype|
-        newvalue(ctype) do
-            handlesum()
-        end
-    end
-
-    str = @validtypes.join("|")
+    newvalues "md5", "md5lite", "timestamp", "mtime", "time", "none"
 
-    # This is here because Puppet sets this internally, using
-    # {md5}......
-    newvalue(/^\{#{str}\}/) do
-        handlesum()
-    end
+    defaultto :md5
 
-    # If they pass us a sum type, behave normally, but if they pass
-    # us a sum type + sum, stick the sum in the cache.
-    munge do |value|
-        if value =~ /^\{(\w+)\}(.+)$/
-            type = symbolize($1)
-            sum = $2
-            cache(type, sum)
-            return type
-        else
-            return :none if value.nil? or value.to_s == "" or value.to_s == "none"
-            if FileTest.directory?(@resource[:path])
-                return :time
-            elsif @resource[:source] and value.to_s != "md5"
-                 self.warning("Files with source set must use md5 as checksum. Forcing to md5 from %s for %s" % [ value, @resource[:path] ])
-                return :md5
-            else
-                return symbolize(value)
-            end
-        end
+    def sum(content)
+        type = value || :md5 # because this might be called before defaults are set
+        "{#{type}}" + send(type, content)
     end
 
-    # Store the checksum in the data cache, or retrieve it if only the
-    # sum type is provided.
-    def cache(type, sum = nil)
-        return unless c = resource.catalog and c.host_config?
-        unless type
-            raise ArgumentError, "A type must be specified to cache a checksum"
-        end
-        type = symbolize(type)
-        type = :mtime if type == :timestamp
-        type = :ctime if type == :time
-
-        unless state = @resource.cached(:checksums)
-            self.debug "Initializing checksum hash"
-            state = {}
-            @resource.cache(:checksums, state)
-        end
-
-        if sum
-            unless sum =~ /\{\w+\}/
-                sum = "{%s}%s" % [type, sum]
-            end
-            state[type] = sum
-        else
-            return state[type]
-        end
-    end
-
-    # Because source and content and whomever else need to set the checksum
-    # and do the updating, we provide a simple mechanism for doing so.
-    def checksum=(value)
-        munge(@should)
-        self.updatesum(value)
-    end
-
-    def checktype
-        self.should || :md5
-    end
-
-    # Checksums need to invert how changes are printed.
-    def change_to_s(currentvalue, newvalue)
-        begin
-            if currentvalue == :absent
-                return "defined '%s' as '%s'" %
-                    [self.name, self.currentsum]
-            elsif newvalue == :absent
-                return "undefined %s from '%s'" %
-                    [self.name, self.is_to_s(currentvalue)]
-            else
-                if defined? @cached and @cached
-                    return "%s changed '%s' to '%s'" %
-                        [self.name, @cached, self.is_to_s(currentvalue)]
-                else
-                    return "%s changed '%s' to '%s'" %
-                        [self.name, self.currentsum, self.is_to_s(currentvalue)]
-                end
-            end
-        rescue Puppet::Error, Puppet::DevError
-            raise
-        rescue => detail
-            raise Puppet::DevError, "Could not convert change %s to string: %s" %
-                [self.name, detail]
-        end
-    end
-
-    def currentsum
-        cache(checktype())
-    end
-
-    # Calculate the sum from disk.
-    def getsum(checktype, file = nil)
-        sum = ""
-
-        checktype = :mtime if checktype == :timestamp
-        checktype = :ctime if checktype == :time
-        self.should = checktype = :md5 if @resource.property(:source)
-
-        file ||= @resource[:path]
-
-        return nil unless FileTest.exist?(file)
-
-        if ! FileTest.file?(file)
-            checktype = :mtime
-        end
-        method = checktype.to_s + "_file"
-
-        self.fail("Invalid checksum type %s" % checktype) unless respond_to?(method)
-
-        return "{%s}%s" % [checktype, send(method, file)]
-    end
-
-    # At this point, we don't actually modify the system, we modify
-    # the stored state to reflect the current state, and then kick
-    # off an event to mark any changes.
-    def handlesum
-        currentvalue = self.retrieve
-        if currentvalue.nil?
-            raise Puppet::Error, "Checksum state for %s is somehow nil" %
-                @resource.title
-        end
-
-        if self.insync?(currentvalue)
-            self.debug "Checksum is already in sync"
-            return nil
-        end
-        # If we still can't retrieve a checksum, it means that
-        # the file still doesn't exist
-        if currentvalue == :absent
-            # if they're copying, then we won't worry about the file
-            # not existing yet
-            return nil unless @resource.property(:source)
-        end
-
-        # If the sums are different, then return an event.
-        if self.updatesum(currentvalue)
-            return :file_changed
-        else
-            return nil
-        end
-    end
-
-    def insync?(currentvalue)
-        @should = [checktype()]
-        if cache(checktype())
-            return currentvalue == currentsum()
-        else
-            # If there's no cached sum, then we don't want to generate
-            # an event.
-            return true
-        end
-    end
-
-    # Even though they can specify multiple checksums, the insync?
-    # mechanism can really only test against one, so we'll just retrieve
-    # the first specified sum type.
-    def retrieve(usecache = false)
-        # When the 'source' is retrieving, it passes "true" here so
-        # that we aren't reading the file twice in quick succession, yo.
-        currentvalue = currentsum()
-        return currentvalue if usecache and currentvalue
-
-        stat = nil
-        return :absent unless stat = @resource.stat
-
-        if stat.ftype == "link" and @resource[:links] != :follow
-            self.debug "Not checksumming symlink"
-            # @resource.delete(:checksum)
-            return currentvalue
-        end
-
-        # Just use the first allowed check type
-        currentvalue = getsum(checktype())
-
-        # If there is no sum defined, then store the current value
-        # into the cache, so that we're not marked as being
-        # out of sync.  We don't want to generate an event the first
-        # time we get a sum.
-        self.updatesum(currentvalue) unless cache(checktype())
-
-        # @resource.debug "checksum state is %s" % self.is
-        return currentvalue
-    end
-
-    # Store the new sum to the state db.
-    def updatesum(newvalue)
-        return unless c = resource.catalog and c.host_config?
-        result = false
-
-        # if we're replacing, vs. updating
-        if sum = cache(checktype())
-            return false if newvalue == sum
-
-            self.debug "Replacing %s checksum %s with %s" % [@resource.title, sum, newvalue]
-            result = true
-        else
-            @resource.debug "Creating checksum %s" % newvalue
-            result = false
-        end
-
-        # Cache the sum so the log message can be right if possible.
-        @cached = sum
-        cache(checktype(), newvalue)
-        return result
+    def sum_file(path)
+        type = value || :md5 # because this might be called before defaults are set
+        method = type.to_s + "_file"
+        "{#{type}}" + send(method, path).to_s
     end
 end
diff --git a/lib/puppet/type/file/content.rb b/lib/puppet/type/file/content.rb
index 0f26a8561..a8a5e7c73 100755
--- a/lib/puppet/type/file/content.rb
+++ b/lib/puppet/type/file/content.rb
@@ -30,9 +30,13 @@ module Puppet
         munge do |value|
             if value == :absent
                 value
+            elsif checksum?(value)
+                # XXX This is potentially dangerous because it means users can't write a file whose
+                # entire contents are a plain checksum
+                value
             else
                 @actual_content = value
-                "{#{checksum_type}}" + send(self.checksum_type, value)
+                resource.parameter(:checksum).sum(value)
             end
         end
 
@@ -54,10 +58,8 @@ module Puppet
         def checksum_type
             if source = resource.parameter(:source)
                 result = source.checksum
-            elsif checksum = resource.parameter(:checksum)
-                result = checksum.checktype
-            else
-                return :md5
+            else checksum = resource.parameter(:checksum)
+                result = resource[:checksum]
             end
             if result =~ /^\{(\w+)\}.+/
                 return $1.to_sym
@@ -76,7 +78,7 @@ module Puppet
             if s = resource.parameter(:source)
                 return s.content
             end
-            return nil
+            fail "Could not find actual content from checksum"
         end
 
         def content
@@ -116,10 +118,10 @@ module Puppet
             return :absent unless stat = @resource.stat
             ftype = stat.ftype
             # Don't even try to manage the content on directories or links
-            return nil if ["directory","link"].include? ftype or checksum_type.nil?
+            return nil if ["directory","link"].include?(ftype)
 
             begin
-                "{#{checksum_type}}" + send(checksum_type.to_s + "_file", resource[:path]).to_s
+                resource.parameter(:checksum).sum_file(resource[:path])
             rescue => detail
                 raise Puppet::Error, "Could not read #{ftype} #{@resource.title}: #{detail}"
             end
diff --git a/lib/puppet/type/file/ensure.rb b/lib/puppet/type/file/ensure.rb
index 96369f6b3..fc4bbea3c 100755
--- a/lib/puppet/type/file/ensure.rb
+++ b/lib/puppet/type/file/ensure.rb
@@ -73,7 +73,6 @@ module Puppet
                 Dir.mkdir(@resource[:path])
             end
             @resource.send(:property_fix)
-            @resource.setchecksum
             return :directory_created
         end
 
diff --git a/spec/integration/type/file.rb b/spec/integration/type/file.rb
index d1e46a206..259c975ff 100755
--- a/spec/integration/type/file.rb
+++ b/spec/integration/type/file.rb
@@ -130,6 +130,19 @@ describe Puppet::Type.type(:file) do
             bucket.bucket.getfile(foomd5).should == "fooyay"
             bucket.bucket.getfile(barmd5).should == "baryay"
         end
+
+        it "should propagate failures encountered when renaming the temporary file" do
+            file = Puppet::Type.type(:file).new :path => tmpfile("fail_rename"), :content => "foo"
+            catalog = Puppet::Resource::Catalog.new
+            catalog.add_resource file
+
+            File.open(file[:path], "w") { |f| f.print "bar" }
+
+            File.expects(:rename).raises ArgumentError
+
+            lambda { file.write("something", :content) }.should raise_error(Puppet::Error)
+            File.read(file[:path]).should == "bar"
+        end
     end
 
     describe "when recursing" do
@@ -372,6 +385,7 @@ describe Puppet::Type.type(:file) do
 
             # Now change the file
             File.open(source, "w") { |f| f.print "bar" }
+            file.expire
             catalog.apply
 
             # And make sure it's changed
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index cedb1701d..64ac135f8 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -139,13 +139,13 @@ describe Puppet::Type.type(:file) do
     end
 
     describe "when validating attributes" do
-        %w{path backup recurse recurselimit source replace force ignore links purge sourceselect}.each do |attr|
+        %w{path checksum backup recurse recurselimit source replace force ignore links purge sourceselect}.each do |attr|
             it "should have a '#{attr}' parameter" do
                 Puppet::Type.type(:file).attrtype(attr.intern).should == :param
             end
         end
 
-        %w{checksum content target ensure owner group mode type}.each do |attr|
+        %w{content target ensure owner group mode type}.each do |attr|
             it "should have a '#{attr}' property" do
                 Puppet::Type.type(:file).attrtype(attr.intern).should == :property
             end
diff --git a/spec/unit/type/file/checksum.rb b/spec/unit/type/file/checksum.rb
index 5d715d15c..8f0f84290 100644
--- a/spec/unit/type/file/checksum.rb
+++ b/spec/unit/type/file/checksum.rb
@@ -5,24 +5,55 @@ Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f
 checksum = Puppet::Type.type(:file).attrclass(:checksum)
 describe checksum do
     before do
-        # Wow that's a messy interface to the resource.
-        @resource = stub 'resource', :[] => nil, :[]= => nil, :property => nil, :newattr => nil, :parameter => nil
+        @resource = Puppet::Type.type(:file).new :path => "/foo/bar"
+        @checksum = @resource.parameter(:checksum)
     end
 
-    it "should be a subclass of Property" do
-        checksum.superclass.must == Puppet::Property
+    it "should be a parameter" do
+        checksum.superclass.must == Puppet::Parameter
     end
 
-    it "should have default checksum of :md5" do
-        @checksum = checksum.new(:resource => @resource)
-        @checksum.checktype.should == :md5
+    it "should use its current value when asked to sum content" do
+        @checksum.value = :md5lite
+        @checksum.expects(:md5lite).with("foobar").returns "yay"
+        @checksum.sum("foobar")
     end
 
-    [:none, nil, ""].each do |ck|
-        it "should use a none checksum for #{ck.inspect}" do
-            @checksum = checksum.new(:resource => @resource)
-            @checksum.should = "none"
-            @checksum.checktype.should == :none
-        end
+    it "should use :md5 to sum when no value is set" do
+        @checksum.expects(:md5).with("foobar").returns "yay"
+        @checksum.sum("foobar")
+    end
+
+    it "should return the summed contents with a checksum label" do
+        sum = Digest::MD5.hexdigest("foobar")
+        @resource[:checksum] = :md5
+        @checksum.sum("foobar").should == "{md5}#{sum}"
+    end
+
+    it "should use :md5 as its default type" do
+        @checksum.default.should == :md5
+    end
+
+    it "should use its current value when asked to sum a file's content" do
+        @checksum.value = :md5lite
+        @checksum.expects(:md5lite_file).with("/foo/bar").returns "yay"
+        @checksum.sum_file("/foo/bar")
+    end
+
+    it "should use :md5 to sum a file when no value is set" do
+        @checksum.expects(:md5_file).with("/foo/bar").returns "yay"
+        @checksum.sum_file("/foo/bar")
+    end
+
+    it "should convert all sums to strings when summing files" do
+        @checksum.value = :mtime
+        @checksum.expects(:mtime_file).with("/foo/bar").returns Time.now
+        lambda { @checksum.sum_file("/foo/bar") }.should_not raise_error
+    end
+
+    it "should return the summed contents of a file with a checksum label" do
+        @resource[:checksum] = :md5
+        @checksum.expects(:md5_file).returns "mysum"
+        @checksum.sum_file("/foo/bar").should == "{md5}mysum"
     end
 end
diff --git a/spec/unit/type/file/content.rb b/spec/unit/type/file/content.rb
index 442de1309..1f2100981 100755
--- a/spec/unit/type/file/content.rb
+++ b/spec/unit/type/file/content.rb
@@ -5,8 +5,7 @@ Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f
 content = Puppet::Type.type(:file).attrclass(:content)
 describe content do
     before do
-        # Wow that's a messy interface to the resource.
-        @resource = stub 'resource', :[] => nil, :[]= => nil, :property => nil, :newattr => nil, :parameter => nil
+        @resource = Puppet::Type.type(:file).new :path => "/foo/bar"
     end
 
     it "should be a subclass of Property" do
@@ -14,45 +13,31 @@ describe content do
     end
 
     describe "when determining the checksum type" do
-        it "should use the type specified in the source checksum if a source is set" do
-            source = mock 'source'
-            source.expects(:checksum).returns "{litemd5}eh"
-
-            @resource.expects(:parameter).with(:source).returns source
-
-            @content = content.new(:resource => @resource)
-            @content.checksum_type.should == :litemd5
+        before do
+            @resource = Puppet::Type.type(:file).new :path => "/foo/bar"
         end
 
-        it "should use the type specified by the checksum parameter if no source is set" do
-            checksum = mock 'checksum'
-            checksum.expects(:checktype).returns :litemd5
-
-            @resource.expects(:parameter).with(:source).returns nil
-            @resource.expects(:parameter).with(:checksum).returns checksum
+        it "should use the type specified in the source checksum if a source is set" do
+            @resource[:source] = "/foo"
+            @resource.parameter(:source).expects(:checksum).returns "{md5lite}eh"
 
             @content = content.new(:resource => @resource)
-            @content.checksum_type.should == :litemd5
+            @content.checksum_type.should == :md5lite
         end
 
-        it "should only return the checksum type from the checksum parameter if the parameter returns a whole checksum" do
-            checksum = mock 'checksum'
-            checksum.expects(:checktype).returns "{md5}something"
-
-            @resource.expects(:parameter).with(:source).returns nil
-            @resource.expects(:parameter).with(:checksum).returns checksum
-
-            @content = content.new(:resource => @resource)
-            @content.checksum_type.should == :md5
-        end
+        it "should use the type specified by the checksum parameter if no source is set" do
+            @resource[:checksum] = :md5lite
 
-        it "should use md5 if neither a source nor a checksum parameter is available" do
             @content = content.new(:resource => @resource)
-            @content.checksum_type.should == :md5
+            @content.checksum_type.should == :md5lite
         end
     end
 
     describe "when determining the actual content to write" do
+        before do
+            @resource = Puppet::Type.type(:file).new :path => "/foo/bar"
+        end
+
         it "should use the set content if available" do
             @content = content.new(:resource => @resource)
             @content.should = "ehness"
@@ -69,9 +54,9 @@ describe content do
             @content.actual_content.should == "scont"
         end
 
-        it "should return nil if no source is available and no content is set" do
+        it "should fail if no source is available and no content is set" do
             @content = content.new(:resource => @resource)
-            @content.actual_content.should be_nil
+            lambda { @content.actual_content }.should raise_error(Puppet::Error)
         end
     end
 
@@ -100,6 +85,16 @@ describe content do
 
             @content.should.must == :absent
         end
+
+        it "should accept a checksum as the desired content" do
+            @content = content.new(:resource => @resource)
+            digest = Digest::MD5.hexdigest("this is some content")
+
+            string = "{md5}#{digest}"
+            @content.should = string
+
+            @content.should.must == string
+        end
     end
 
     describe "when retrieving the current content" do
@@ -130,29 +125,22 @@ describe content do
 
         it "should always return the checksum as a string" do
             @content = content.new(:resource => @resource)
-            @content.stubs(:checksum_type).returns "mtime"
+            @resource[:checksum] = :mtime
 
             stat = mock 'stat', :ftype => "file"
             @resource.expects(:stat).returns stat
 
-            @resource.expects(:[]).with(:path).returns "/my/file"
-
             time = Time.now
-            @content.expects(:mtime_file).with("/my/file").returns time
+            @resource.parameter(:checksum).expects(:mtime_file).with(@resource[:path]).returns time
 
             @content.retrieve.should == "{mtime}%s" % time
         end
 
         it "should return the checksum of the file if it exists and is a normal file" do
             @content = content.new(:resource => @resource)
-            @content.stubs(:checksum_type).returns "md5"
-
             stat = mock 'stat', :ftype => "file"
             @resource.expects(:stat).returns stat
-
-            @resource.expects(:[]).with(:path).returns "/my/file"
-
-            @content.expects(:md5_file).with("/my/file").returns "mysum"
+            @resource.parameter(:checksum).expects(:md5_file).with(@resource[:path]).returns "mysum"
 
             @content.retrieve.should == "{md5}mysum"
         end
@@ -160,11 +148,8 @@ describe content do
 
     describe "when testing whether the content is in sync" do
         before do
-            @resource.stubs(:[]).with(:ensure).returns :file
-            @resource.stubs(:replace?).returns true
-            @resource.stubs(:should_be_file?).returns true
+            @resource[:ensure] = :file
             @content = content.new(:resource => @resource)
-            @content.stubs(:checksum_type).returns "md5"
         end
 
         it "should return true if the resource shouldn't be a regular file" do