Merge pull request #1183 from onddo/CHEF-4762

CHEF-4762: http_request with action :head does not behave correctly in 1.8.0

3 files changed, 74 insertions, 3 deletions

diff --git a/lib/chef/provider/http_request.rb b/lib/chef/provider/http_request.rb
index 1e0aa8b4a0..2bbd20aa2c 100644
--- a/lib/chef/provider/http_request.rb
+++ b/lib/chef/provider/http_request.rb
@@ -36,7 +36,8 @@ class Chef
       # Send a HEAD request to @new_resource.url, with ?message=@new_resource.message
       def action_head
         message = check_message(@new_resource.message)
-        # returns true from Chef::REST if returns 2XX (Net::HTTPSuccess)
+        # CHEF-4762: we expect a nil return value from Chef::HTTP for a "200 Success" response
+        # and false for a "304 Not Modified" response
         modified = @http.head(
           "#{@new_resource.url}?message=#{message}",
           @new_resource.headers
@@ -44,7 +45,7 @@ class Chef
         Chef::Log.info("#{@new_resource} HEAD to #{@new_resource.url} successful")
         Chef::Log.debug("#{@new_resource} HEAD request response: #{modified}")
         # :head is usually used to trigger notifications, which converge_by now does
-        if modified
+        if modified != false
           converge_by("#{@new_resource} HEAD to #{@new_resource.url} returned modified, trigger notifications") {}
         end
       end
diff --git a/spec/unit/http_spec.rb b/spec/unit/http_spec.rb
new file mode 100644
index 0000000000..d747482260
--- /dev/null
+++ b/spec/unit/http_spec.rb
@@ -0,0 +1,48 @@
+#
+# Author:: Xabier de Zuazo (xabier@onddo.com)
+# Copyright:: Copyright (c) 2014 Onddo Labs, SL.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'spec_helper'
+
+require 'chef/http'
+require 'chef/http/basic_client'
+
+describe Chef::HTTP do
+
+  describe "head" do
+
+    it 'should return nil for a "200 Success" response (CHEF-4762)' do
+      resp = Net::HTTPOK.new("1.1", 200, "OK")
+      resp.should_receive(:read_body).and_return(nil)
+      http = Chef::HTTP.new("")
+      Chef::HTTP::BasicClient.any_instance.should_receive(:request).and_return(["request", resp])
+
+      http.head("http://www.getchef.com/").should eql(nil)
+    end
+
+    it 'should return false for a "304 Not Modified" response (CHEF-4762)' do
+      resp = Net::HTTPNotModified.new("1.1", 304, "Not Modified")
+      resp.should_receive(:read_body).and_return(nil)
+      http = Chef::HTTP.new("")
+      Chef::HTTP::BasicClient.any_instance.should_receive(:request).and_return(["request", resp])
+
+      http.head("http://www.getchef.com/").should eql(false)
+    end
+
+  end # head
+
+end
diff --git a/spec/unit/provider/http_request_spec.rb b/spec/unit/provider/http_request_spec.rb
index e8ce5f9ae3..3efdd6c5fd 100644
--- a/spec/unit/provider/http_request_spec.rb
+++ b/spec/unit/provider/http_request_spec.rb
@@ -103,6 +103,8 @@ describe Chef::Provider::HttpRequest do
       end
     end
 
+    # CHEF-4762: we expect a nil return value for a "200 Success" response
+    # and false for a "304 Not Modified" response
     describe "action_head" do
       before do
         @provider.http = @http
@@ -110,17 +112,37 @@ describe Chef::Provider::HttpRequest do
 
       it "should inflate a message block at runtime" do
         @new_resource.message { "return" }
-        @http.should_receive(:head).with("http://www.opscode.com/?message=return", {}).and_return("")
+        @http.should_receive(:head).with("http://www.opscode.com/?message=return", {}).and_return(nil)
         @provider.run_action(:head)
         @new_resource.should be_updated
       end
 
       it "should run a HEAD request" do
+        @http.should_receive(:head).with("http://www.opscode.com/?message=is cool", {}).and_return(nil)
+        @provider.run_action(:head)
+        @new_resource.should be_updated
+      end
+
+      it "should update a HEAD request with empty string response body (CHEF-4762)" do
         @http.should_receive(:head).with("http://www.opscode.com/?message=is cool", {}).and_return("")
         @provider.run_action(:head)
         @new_resource.should be_updated
       end
 
+      it "should update a HEAD request with nil response body (CHEF-4762)" do
+        @http.should_receive(:head).with("http://www.opscode.com/?message=is cool", {}).and_return(nil)
+        @provider.run_action(:head)
+        @new_resource.should be_updated
+      end
+
+      it "should not update a HEAD request if a not modified response (CHEF-4762)" do
+        if_modified_since = File.mtime(__FILE__).httpdate
+        @new_resource.headers "If-Modified-Since" => if_modified_since
+        @http.should_receive(:head).with("http://www.opscode.com/?message=is cool", {"If-Modified-Since" => if_modified_since}).and_return(false)
+        @provider.run_action(:head)
+        @new_resource.should_not be_updated
+      end
+
       it "should run a HEAD request with If-Modified-Since header" do
         @new_resource.headers "If-Modified-Since" => File.mtime(__FILE__).httpdate
         @http.should_receive(:head).with("http://www.opscode.com/?message=is cool", @new_resource.headers)