path: root/0001-4922-Don-t-truncate-remotely-sourced-files-on-404.patch

From 743e03930758d17ed35fc6b73f7c2c68d8212137 Mon Sep 17 00:00:00 2001
From: Nick Lewis <nick@puppetlabs.com>
Date: Mon, 28 Feb 2011 13:40:18 -0800
Subject: [PATCH/puppet] (#4922) Don't truncate remotely-sourced files on 404

We were 'handling' 404's on remote file content retrieval by returning nil
rather than raising an exception. This caused no content to be written to the
temporary file, but still appeared successful, so the destination file was
overwritten, instead of preserved.  Now we just handle 404 like any other
error.

Note that the root cause of these 404s seems to have been #4319, which has been
fixed. However, in the event we do happen to get a 404 here, it's better not to
have code to specifically handle it incorrectly.

Paired-With: Max Martin
Reviewed-By: Matt Robinson
---
 lib/puppet/type/file/content.rb     |    1 -
 spec/unit/type/file/content_spec.rb |  175 ++++++++---------------------------
 2 files changed, 38 insertions(+), 138 deletions(-)

diff --git a/lib/puppet/type/file/content.rb b/lib/puppet/type/file/content.rb
index 63c0aaf..1b36acb 100755
--- a/lib/puppet/type/file/content.rb
+++ b/lib/puppet/type/file/content.rb
@@ -194,7 +194,6 @@ module Puppet
       connection = Puppet::Network::HttpPool.http_instance(source_or_content.server, source_or_content.port)
       connection.request_get(indirection2uri(request), add_accept_encoding({"Accept" => "raw"})) do |response|
         case response.code
-        when "404"; nil
         when /^2/;  uncompress(response) { |uncompressor| response.read_body { |chunk| yield uncompressor.uncompress(chunk) } }
         else
           # Raise the http error if we didn't get a 'success' of some kind.
diff --git a/spec/unit/type/file/content_spec.rb b/spec/unit/type/file/content_spec.rb
index 9178c94..7d23399 100755
--- a/spec/unit/type/file/content_spec.rb
+++ b/spec/unit/type/file/content_spec.rb
@@ -4,15 +4,14 @@ Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f
 
 content = Puppet::Type.type(:file).attrclass(:content)
 describe content do
+  include PuppetSpec::Files
   before do
-    @resource = Puppet::Type.type(:file).new :path => "/foo/bar"
+    @filename = tmpfile('testfile')
+    @resource = Puppet::Type.type(:file).new :path => @filename
+    File.open(@filename, 'w') {|f| f.write "initial file content"}
     content.stubs(:standalone?).returns(false)
   end
 
-  it "should be a subclass of Property" 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
       @resource[:source] = "/foo"
@@ -249,10 +248,10 @@ describe content do
   describe "when writing" do
     before do
       @content = content.new(:resource => @resource)
-      @fh = stub_everything
     end
 
     it "should attempt to read from the filebucket if no actual content nor source exists" do
+      @fh = File.open(@filename, 'w')
       @content.should = "{md5}foo"
       @content.resource.bucket.class.any_instance.stubs(:getfile).returns "foo"
       @content.write(@fh)
@@ -302,166 +301,68 @@ describe content do
 
     describe "from local source" do
       before(:each) do
-        @content.stubs(:actual_content).returns(nil)
-        @source = stub_everything 'source', :local? => true, :full_path => "/path/to/source"
-        @resource.stubs(:parameter).with(:source).returns @source
-
-        @sum = stub_everything 'sum'
-        @resource.stubs(:parameter).with(:checksum).returns(@sum)
-
-        @digest = stub_everything 'digest'
-        @sum.stubs(:sum_stream).yields(@digest)
-
-        @file = stub_everything 'file'
-        File.stubs(:open).yields(@file)
-        @file.stubs(:read).with(8192).returns("chunk1").then.returns("chunk2").then.returns(nil)
-      end
-
-      it "should open the local file" do
-        File.expects(:open).with("/path/to/source", "r")
-        @content.write(@fh)
-      end
+        @resource = Puppet::Type.type(:file).new :path => @filename, :backup => false
+        @sourcename = tmpfile('source')
+        @source_content = "source file content"*10000
+        @sourcefile = File.open(@sourcename, 'w') {|f| f.write @source_content}
 
-      it "should read the local file by chunks" do
-        @file.expects(:read).with(8192).returns("chunk1").then.returns(nil)
-        @content.write(@fh)
+        @content = @resource.newattr(:content)
+        @source = @resource.newattr(:source)
+        @source.stubs(:metadata).returns stub_everything('metadata', :source => @sourcename, :ftype => 'file')
       end
 
-      it "should write each chunk to the file" do
-        @fh.expects(:print).with("chunk1").then.with("chunk2")
-        @content.write(@fh)
-      end
-
-      it "should pass each chunk to the current sum stream" do
-        @digest.expects(:<<).with("chunk1").then.with("chunk2")
-        @content.write(@fh)
+      it "should copy content from the source to the file" do
+        @resource.write(@source)
+        File.read(@filename).should == @source_content
       end
 
       it "should return the checksum computed" do
-        @sum.stubs(:sum_stream).yields(@digest).returns("checksum")
-        @content.write(@fh).should == "checksum"
+        File.open(@filename, 'w') do |file|
+          @content.write(file).should == "{md5}#{Digest::MD5.hexdigest(@source_content)}"
+        end
       end
     end
 
     describe "from remote source" do
       before(:each) do
-        @response = stub_everything 'mock response', :code => "404"
+        @resource = Puppet::Type.type(:file).new :path => @filename, :backup => false
+        @response = stub_everything 'response', :code => "200"
+        @source_content = "source file content"*10000
+        @response.stubs(:read_body).multiple_yields(*(["source file content"]*10000))
+
         @conn = stub_everything 'connection'
         @conn.stubs(:request_get).yields(@response)
         Puppet::Network::HttpPool.stubs(:http_instance).returns @conn
 
-        @content.stubs(:actual_content).returns(nil)
-        @source = stub_everything 'source', :local? => false, :full_path => "/path/to/source", :server => "server", :port => 1234
-        @resource.stubs(:parameter).with(:source).returns @source
-
-        @sum = stub_everything 'sum'
-        @resource.stubs(:parameter).with(:checksum).returns(@sum)
-
-        @digest = stub_everything 'digest'
-        @sum.stubs(:sum_stream).yields(@digest)
-      end
-
-      it "should open a network connection to source server and port" do
-        Puppet::Network::HttpPool.expects(:http_instance).with("server", 1234).returns @conn
-        @content.write(@fh)
-      end
-
-      it "should send the correct indirection uri" do
-        @conn.expects(:request_get).with { |uri,headers| uri == "/production/file_content/path/to/source" }.yields(@response)
-        @content.write(@fh)
+        @content = @resource.newattr(:content)
+        @sourcename = "puppet:///test/foo"
+        @source = @resource.newattr(:source)
+        @source.stubs(:metadata).returns stub_everything('metadata', :source => @sourcename, :ftype => 'file')
       end
 
-      it "should return nil if source is not found" do
-        @response.expects(:code).returns("404")
-        @content.write(@fh).should == nil
+      it "should write the contents to the file" do
+        @resource.write(@source)
+        File.read(@filename).should == @source_content
       end
 
       it "should not write anything if source is not found" do
-        @response.expects(:code).returns("404")
-        @fh.expects(:print).never
-        @content.write(@fh).should == nil
+        @response.stubs(:code).returns("404")
+        lambda {@resource.write(@source)}.should raise_error(Net::HTTPError) { |e| e.message =~ /404/ }
+        File.read(@filename).should == "initial file content"
       end
 
       it "should raise an HTTP error in case of server error" do
-        @response.expects(:code).returns("500")
-        lambda { @content.write(@fh) }.should raise_error
-      end
-
-      it "should write content by chunks" do
-        @response.expects(:code).returns("200")
-        @response.expects(:read_body).multiple_yields("chunk1","chunk2")
-        @fh.expects(:print).with("chunk1").then.with("chunk2")
-        @content.write(@fh)
-      end
-
-      it "should pass each chunk to the current sum stream" do
-        @response.expects(:code).returns("200")
-        @response.expects(:read_body).multiple_yields("chunk1","chunk2")
-        @digest.expects(:<<).with("chunk1").then.with("chunk2")
-        @content.write(@fh)
+        @response.stubs(:code).returns("500")
+        lambda { @content.write(@fh) }.should raise_error { |e| e.message.include? @source_content }
       end
 
       it "should return the checksum computed" do
-        @response.expects(:code).returns("200")
-        @response.expects(:read_body).multiple_yields("chunk1","chunk2")
-        @sum.expects(:sum_stream).yields(@digest).returns("checksum")
-        @content.write(@fh).should == "checksum"
-      end
-
-      it "should get the current accept encoding header value" do
-        @content.expects(:add_accept_encoding)
-        @content.write(@fh)
-      end
-
-      it "should uncompress body on error" do
-        @response.expects(:code).returns("500")
-        @response.expects(:body).returns("compressed body")
-        @content.expects(:uncompress_body).with(@response).returns("uncompressed")
-        lambda { @content.write(@fh) }.should raise_error { |e| e.message =~ /uncompressed/ }
-      end
-
-      it "should uncompress chunk by chunk" do
-        uncompressor = stub_everything 'uncompressor'
-        @content.expects(:uncompress).with(@response).yields(uncompressor)
-        @response.expects(:code).returns("200")
-        @response.expects(:read_body).multiple_yields("chunk1","chunk2")
-
-        uncompressor.expects(:uncompress).with("chunk1").then.with("chunk2")
-        @content.write(@fh)
-      end
-
-      it "should write uncompressed chunks to the file" do
-        uncompressor = stub_everything 'uncompressor'
-        @content.expects(:uncompress).with(@response).yields(uncompressor)
-        @response.expects(:code).returns("200")
-        @response.expects(:read_body).multiple_yields("chunk1","chunk2")
-
-        uncompressor.expects(:uncompress).with("chunk1").returns("uncompressed1")
-        uncompressor.expects(:uncompress).with("chunk2").returns("uncompressed2")
-
-        @fh.expects(:print).with("uncompressed1")
-        @fh.expects(:print).with("uncompressed2")
-
-        @content.write(@fh)
-      end
-
-      it "should pass each uncompressed chunk to the current sum stream" do
-        uncompressor = stub_everything 'uncompressor'
-        @content.expects(:uncompress).with(@response).yields(uncompressor)
-        @response.expects(:code).returns("200")
-        @response.expects(:read_body).multiple_yields("chunk1","chunk2")
-
-        uncompressor.expects(:uncompress).with("chunk1").returns("uncompressed1")
-        uncompressor.expects(:uncompress).with("chunk2").returns("uncompressed2")
-
-        @digest.expects(:<<).with("uncompressed1").then.with("uncompressed2")
-        @content.write(@fh)
+        File.open(@filename, 'w') do |file|
+          @content.write(file).should == "{md5}#{Digest::MD5.hexdigest(@source_content)}"
+        end
       end
     end
 
-    describe "from a filebucket" do
-    end
-
     # These are testing the implementation rather than the desired behaviour; while that bites, there are a whole
     # pile of other methods in the File type that depend on intimate details of this implementation and vice-versa.
     # If these blow up, you are gonna have to review the callers to make sure they don't explode! --daniel 2011-02-01
-- 
1.7.4.1