Passing checksums around instead of file contents

This switches the file's 'content' parameter to always
use checksums, rather than always using content but switching
to checksums whenever necessary.

This greatly simplifies all the logging requirements (so
that content doesn't show up in logs), but also simplifies
insync comparisons, and much more.

In the process, I found that the code was pulling down file content
more often than was necessary, and fixing that cut 40% off of the time
of a very small transaction.

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

4 files changed, 158 insertions, 73 deletions

diff --git a/lib/puppet/type/file/checksum.rb b/lib/puppet/type/file/checksum.rb
index 3b748631a..76e27e55d 100755
--- a/lib/puppet/type/file/checksum.rb
+++ b/lib/puppet/type/file/checksum.rb
@@ -204,10 +204,7 @@ Puppet::Type.type(:file).newproperty(:checksum) do
         if currentvalue == :absent
             # if they're copying, then we won't worry about the file
             # not existing yet
-            unless @resource.property(:source)
-                self.warning("File %s does not exist -- cannot checksum" % @resource[:path])
-            end
-            return nil
+            return nil unless @resource.property(:source)
         end
         
         # If the sums are different, then return an event.
diff --git a/lib/puppet/type/file/content.rb b/lib/puppet/type/file/content.rb
index 385a86357..a5fe9920a 100755
--- a/lib/puppet/type/file/content.rb
+++ b/lib/puppet/type/file/content.rb
@@ -25,17 +25,44 @@ module Puppet
             This attribute is especially useful when used with
             `PuppetTemplating templating`:trac:."
 
-        def string_as_checksum(string)
-            return "absent" if string == :absent
-            "{md5}" + Digest::MD5.hexdigest(string)
+        # Store a checksum as the value, rather than the actual content.
+        # Simplifies everything.
+        munge do |value|
+            if value == :absent
+                value
+            else
+                @actual_content = value
+                "{#{checksum_type}}" + send(self.checksum_type, value)
+            end
         end
 
-        def should_to_s(should)
-            string_as_checksum(should)
+        def checksum_type
+            if source = resource.parameter(:source)
+                source.checksum =~ /^\{(\w+)\}.+/
+                return $1.to_sym
+            elsif checksum = resource.parameter(:checksum)
+                result = checksum.checktype
+                if result =~ /^\{(\w+)\}.+/
+                    return $1.to_sym
+                else
+                    return result
+                end
+            else
+                return :md5
+            end
         end
 
-        def is_to_s(is)
-            string_as_checksum(is)
+        # If content was specified, return that; else try to return the source content;
+        # else, return nil.
+        def actual_content
+            if defined?(@actual_content) and @actual_content
+                return @actual_content
+            end
+
+            if s = resource.parameter(:source)
+                return s.content
+            end
+            return nil
         end
         
         def content
@@ -58,12 +85,7 @@ module Puppet
                 return super
             elsif source = resource.parameter(:source)
                 fail "Got a remote source with no checksum" unless source.checksum
-                unless sum_method = sumtype(source.checksum)
-                    fail "Could not extract checksum type from source checksum '%s'" % source.checksum
-                end
-
-                newsum = "{%s}" % sum_method + send(sum_method, is)
-                result = (newsum == source.checksum)
+                result = (is == source.checksum)
             else
                 # We've got no content specified, and no source from which to
                 # get content.
@@ -71,7 +93,7 @@ module Puppet
             end
 
             if ! result and Puppet[:show_diff]
-                string_file_diff(@resource[:path], content)
+                string_file_diff(@resource[:path], actual_content)
             end
             return result
         end
@@ -83,7 +105,7 @@ module Puppet
             return nil if stat.ftype == "directory"
 
             begin
-                return File.read(@resource[:path])
+                return "{#{checksum_type}}" + send(checksum_type.to_s + "_file", resource[:path])
             rescue => detail
                 raise Puppet::Error, "Could not read %s: %s" % [@resource.title, detail]
             end
@@ -91,8 +113,8 @@ module Puppet
 
         # Make sure we're also managing the checksum property.
         def should=(value)
-            super
             @resource.newattr(:checksum) unless @resource.parameter(:checksum)
+            super
         end
 
         # Just write our content out to disk.
@@ -102,8 +124,7 @@ module Puppet
             # We're safe not testing for the 'source' if there's no 'should'
             # because we wouldn't have gotten this far if there weren't at least
             # one valid value somewhere.
-            content = self.should || resource.parameter(:source).content
-            @resource.write(content, :content)
+            @resource.write(actual_content, :content)
 
             return return_event
         end
diff --git a/lib/puppet/type/file/ensure.rb b/lib/puppet/type/file/ensure.rb
index 7466c5e3a..5c4d98d4b 100755
--- a/lib/puppet/type/file/ensure.rb
+++ b/lib/puppet/type/file/ensure.rb
@@ -110,11 +110,19 @@ module Puppet
         end
 
         def change_to_s(currentvalue, newvalue)
-            if property = @resource.property(:content) and content = property.retrieve and ! property.insync?(content)
-                return property.change_to_s(content, property.should)
+            return super unless newvalue.to_s == "file"
+
+            return super unless property = @resource.property(:content)
+
+            # We know that content is out of sync if we're here, because
+            # it's essentially equivalent to 'ensure' in the transaction.
+            if source = @resource.parameter(:source)
+                should = source.checksum
             else
-                super(currentvalue, newvalue)
+                should = property.should
             end
+
+            return property.change_to_s(property.retrieve, should)
         end
 
         # Check that we can actually create anything
diff --git a/spec/unit/type/file/content.rb b/spec/unit/type/file/content.rb
index 212bb2fdb..fd225fa17 100755
--- a/spec/unit/type/file/content.rb
+++ b/spec/unit/type/file/content.rb
@@ -13,6 +13,95 @@ describe content do
         content.superclass.must == Puppet::Property
     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
+        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
+
+            @content = content.new(:resource => @resource)
+            @content.checksum_type.should == :litemd5
+        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 md5 if neither a source nor a checksum parameter is available" do
+            @content = content.new(:resource => @resource)
+            @content.checksum_type.should == :md5
+        end
+    end
+
+    describe "when determining the actual content to write" do
+        it "should use the set content if available" do
+            @content = content.new(:resource => @resource)
+            @content.should = "ehness"
+            @content.actual_content.should == "ehness"
+        end
+
+        it "should use the content from the source if the source is set" do
+            source = mock 'source'
+            source.expects(:content).returns "scont"
+
+            @resource.expects(:parameter).with(:source).returns source
+
+            @content = content.new(:resource => @resource)
+            @content.actual_content.should == "scont"
+        end
+
+        it "should return nil if no source is available and no content is set" do
+            @content = content.new(:resource => @resource)
+            @content.actual_content.should be_nil
+        end
+    end
+
+    describe "when setting the desired content" do
+        it "should make the actual content available via an attribute" do
+            @content = content.new(:resource => @resource)
+            @content.stubs(:checksum_type).returns "md5"
+            @content.should = "this is some content"
+
+            @content.actual_content.should == "this is some content"
+        end
+
+        it "should store the checksum as the desired content" do
+            @content = content.new(:resource => @resource)
+            digest = Digest::MD5.hexdigest("this is some content")
+
+            @content.stubs(:checksum_type).returns "md5"
+            @content.should = "this is some content"
+
+            @content.should.must == "{md5}#{digest}"
+        end
+
+        it "should not checksum 'absent'" do
+            @content = content.new(:resource => @resource)
+            @content.should = :absent
+
+            @content.should.must == :absent
+        end
+    end
+
     describe "when retrieving the current content" do
         it "should return :absent if the file does not exist" do
             @content = content.new(:resource => @resource)
@@ -21,9 +110,7 @@ describe content do
             @content.retrieve.should == :absent
         end
 
-        it "should not manage content on non-files" do
-            pending "Haven't decided how this should behave"
-
+        it "should not manage content on directories" do
             @content = content.new(:resource => @resource)
 
             stat = mock 'stat', :ftype => "directory"
@@ -32,16 +119,18 @@ describe content do
             @content.retrieve.should be_nil
         end
 
-        it "should return the current content of the file if it exists and is a normal file" do
+        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"
-            File.expects(:read).with("/my/file").returns "some content"
 
-            @content.retrieve.should == "some content"
+            @content.expects(:md5_file).with("/my/file").returns "mysum"
+
+            @content.retrieve.should == "{md5}mysum"
         end
     end
 
@@ -51,7 +140,7 @@ describe content do
             @resource.stubs(:replace?).returns true
             @resource.stubs(:should_be_file?).returns true
             @content = content.new(:resource => @resource)
-            @content.should = "something"
+            @content.stubs(:checksum_type).returns "md5"
         end
 
         it "should return true if the resource shouldn't be a regular file" do
@@ -79,9 +168,9 @@ describe content do
                 @content.should_not be_insync("other content")
             end
 
-            it "should return true if the current contents are the same as the desired content" do
+            it "should return true if the sum for the current contents is the same as the sum for the desired content" do
                 @content.should = "some content"
-                @content.must be_insync("some content")
+                @content.must be_insync("{md5}" + Digest::MD5.hexdigest("some content"))
             end
 
             describe "and the content is specified via a remote source" do
@@ -89,15 +178,12 @@ describe content do
                     @metadata = stub 'metadata'
                     @source = stub 'source', :metadata => @metadata
                     @resource.stubs(:parameter).with(:source).returns @source
-
-                    @content.should = nil
                 end
 
                 it "should use checksums to compare remote content, rather than downloading the content" do
-                    @content.expects(:md5).with("some content").returns "whatever"
                     @source.stubs(:checksum).returns "{md5}whatever"
 
-                    @content.insync?("some content")
+                    @content.insync?("{md5}eh")
                 end
 
                 it "should return false if the current content is different from the remote content" do
@@ -107,10 +193,9 @@ describe content do
                 end
 
                 it "should return true if the current content is the same as the remote content" do
-                    sum = @content.md5("some content")
-                    @source.stubs(:checksum).returns("{md5}%s" % sum)
+                    @source.stubs(:checksum).returns("{md5}something")
 
-                    @content.must be_insync("some content")
+                    @content.must be_insync("{md5}something")
                 end
             end
         end
@@ -141,54 +226,28 @@ describe content do
     describe "when changing the content" do
         before do
             @content = content.new(:resource => @resource)
+            @content.should = "some content"
 
             @resource.stubs(:[]).with(:path).returns "/boo"
+            @resource.stubs(:stat).returns "eh"
         end
 
         it "should use the file's :write method to write the content" do
-            pending "not switched from :source yet"
-            @resource.expects(:write).with("foobar", :content, 123)
+            @resource.expects(:write).with("some content", :content)
 
             @content.sync
         end
 
         it "should return :file_changed if the file already existed" do
-            pending "not switched from :source yet"
+            @resource.expects(:stat).returns "something"
             @resource.stubs(:write)
-            FileTest.expects(:exist?).with("/boo").returns true
             @content.sync.should == :file_changed
         end
 
-        it "should return :file_created if the file already existed" do
-            pending "not switched from :source yet"
+        it "should return :file_created if the file did not exist" do
+            @resource.expects(:stat).returns nil
             @resource.stubs(:write)
-            FileTest.expects(:exist?).with("/boo").returns false
             @content.sync.should == :file_created
         end
     end
-
-    describe "when logging changes" do
-        before do
-            @resource = stub 'resource', :line => "foo", :file => "bar", :replace? => true
-            @resource.stubs(:[]).returns "foo"
-            @resource.stubs(:[]).with(:path).returns "/my/file"
-            @content = content.new :resource => @resource
-        end
-
-        it "should not include current contents" do
-            @content.change_to_s("current_content", "desired").should_not be_include("current_content")
-        end
-
-        it "should not include desired contents" do
-            @content.change_to_s("current", "desired_content").should_not be_include("desired_content")
-        end
-
-        it "should not include the content when converting current content to a string" do
-            @content.is_to_s("my_content").should_not be_include("my_content")
-        end
-
-        it "should not include the content when converting desired content to a string" do
-            @content.should_to_s("my_content").should_not be_include("my_content")
-        end
-    end
 end