(PUP-10730) Separate abstract response and net http adapter

Move behavior specific to Net::HTTPResponse into an adapter, and leave the
common behaviors in the abstract Puppet::HTTP::Response class. It also makes it
possible to create a response in tests given just the URL, code and status.

lib/puppet/http.rb

@@ -24,6 +24,7 @@ module HTTP
     require 'puppet/http/pool'
     require 'puppet/http/dns'
     require 'puppet/http/response'
+    require 'puppet/http/response_net_http'
     require 'puppet/http/service'
     require 'puppet/http/service/ca'
     require 'puppet/http/service/compiler'

lib/puppet/http/client.rb

@@ -350,7 +350,7 @@ def execute_streaming(request, options: {}, &block)
 
         # don't call return within the `request` block
         http.request(request) do |nethttp|
-          response = Puppet::HTTP::Response.new(nethttp, request.uri)
+          response = Puppet::HTTP::ResponseNetHTTP.new(request.uri, nethttp)
           begin
             Puppet.debug("HTTP #{request.method.upcase} #{request.uri} returned #{response.code} #{response.reason}")
 

lib/puppet/http/external_client.rb

@@ -22,7 +22,7 @@ def get(url, headers: {}, params: {}, options: {}, &block)
     options[:use_ssl] = url.scheme == 'https'
 
     client = @http_client_class.new(url.host, url.port, options)
-    response = Puppet::HTTP::Response.new(client.get(url.request_uri, headers, options), url)
+    response = Puppet::HTTP::ResponseNetHTTP.new(url, client.get(url.request_uri, headers, options))
 
     if block_given?
       yield response
@@ -44,7 +44,7 @@ def post(url, body, headers: {}, params: {}, options: {}, &block)
     options[:use_ssl] = url.scheme == 'https'
 
     client = @http_client_class.new(url.host, url.port, options)
-    response = Puppet::HTTP::Response.new(client.post(url.request_uri, body, headers, options), url)
+    response = Puppet::HTTP::ResponseNetHTTP.new(url, client.post(url.request_uri, body, headers, options))
 
     if block_given?
       yield response

lib/puppet/http/response.rb

@@ -1,39 +1,42 @@
 # Represents the response returned from the server from an HTTP request.
 #
+# @api abstract
 # @api public
 class Puppet::HTTP::Response
   # @api private
   # @return [Net::HTTP] the Net::HTTP response
   attr_reader :nethttp
 
-  # @return [URI] the response uri
+  # @return [URI] the response url
   attr_reader :url
 
-  # Object to represent the response returned from an HTTP request.
+  # Create a response associated with the URL.
   #
-  # @param [Net::HTTP] nethttp the request response
   # @param [URI] url
-  def initialize(nethttp, url)
-    @nethttp = nethttp
+  # @param [Integer] HTTP status
+  # @param [String] HTTP reason
+  def initialize(url, code, reason)
     @url = url
+    @code = code
+    @reason = reason
   end
 
-  # Extract the response code.
+  # Return the response code.
   #
   # @return [Integer] Response code for the request
   #
   # @api public
   def code
-    @nethttp.code.to_i
+    @code
   end
 
-  # Extract the response message.
+  # Return the response message.
   #
   # @return [String] Response message for the request
   #
   # @api public
   def reason
-    @nethttp.message
+    @reason
   end
 
   # Returns the entire response body. Can be used instead of
@@ -44,7 +47,7 @@ def reason
   #
   # @api public
   def body
-    @nethttp.body
+    raise NotImplementedError
   end
 
   # Streams the response body to the caller in chunks. Can be used instead of
@@ -57,9 +60,7 @@ def body
   #
   # @api public
   def read_body(&block)
-    raise ArgumentError, "A block is required" unless block_given?
-
-    @nethttp.read_body(&block)
+    raise NotImplementedError
   end
 
   # Check if the request received a response of success (HTTP 2xx).
@@ -68,7 +69,7 @@ def read_body(&block)
   #
   # @api public
   def success?
-    @nethttp.is_a?(Net::HTTPSuccess)
+    200 <= @code && @code < 300
   end
 
   # Get a header case-insensitively.
@@ -78,7 +79,7 @@ def success?
   #
   # @api public
   def [](name)
-    @nethttp[name]
+    raise NotImplementedError
   end
 
   # Yield each header name and value. Returns an enumerator if no block is given.
@@ -88,7 +89,7 @@ def [](name)
   #
   # @api public
   def each_header(&block)
-    @nethttp.each_header(&block)
+    raise NotImplementedError
   end
 
   # Ensure the response body is fully read so that the server is not blocked

lib/puppet/http/response_net_http.rb

@@ -0,0 +1,42 @@
+# Adapts Net::HTTPResponse to Puppet::HTTP::Response
+#
+# @api public
+class Puppet::HTTP::ResponseNetHTTP < Puppet::HTTP::Response
+
+  # Create a response associated with the URL.
+  #
+  # @param [URI] url
+  # @param [Net::HTTPResponse] nethttp The response
+  def initialize(url, nethttp)
+    super(url, nethttp.code.to_i, nethttp.message)
+
+    @nethttp = nethttp
+  end
+
+  # (see Puppet::HTTP::Response#body)
+  def body
+    @nethttp.body
+  end
+
+  # (see Puppet::HTTP::Response#read_body)
+  def read_body(&block)
+    raise ArgumentError, "A block is required" unless block_given?
+
+    @nethttp.read_body(&block)
+  end
+
+  # (see Puppet::HTTP::Response#success?)
+  def success?
+    @nethttp.is_a?(Net::HTTPSuccess)
+  end
+
+  # (see Puppet::HTTP::Response#[])
+  def [](name)
+    @nethttp[name]
+  end
+
+  # (see Puppet::HTTP::Response#each_header)
+  def each_header(&block)
+    @nethttp.each_header(&block)
+  end
+end

spec/unit/http/client_spec.rb

@@ -154,7 +154,7 @@ def expect_http_error(cause, expected_message)
       stub_request(:get, uri)
 
       response = client.get(uri)
-      expect(response).to be_an_instance_of(Puppet::HTTP::Response)
+      expect(response).to be_a(Puppet::HTTP::Response)
       expect(response).to be_success
       expect(response.code).to eq(200)
     end
@@ -224,7 +224,7 @@ def expect_http_error(cause, expected_message)
       stub_request(:head, uri)
 
       response = client.head(uri)
-      expect(response).to be_an_instance_of(Puppet::HTTP::Response)
+      expect(response).to be_a(Puppet::HTTP::Response)
       expect(response).to be_success
       expect(response.code).to eq(200)
     end
@@ -284,7 +284,7 @@ def expect_http_error(cause, expected_message)
       stub_request(:put, uri)
 
       response = client.put(uri, "", headers: {'Content-Type' => 'text/plain'})
-      expect(response).to be_an_instance_of(Puppet::HTTP::Response)
+      expect(response).to be_a(Puppet::HTTP::Response)
       expect(response).to be_success
       expect(response.code).to eq(200)
     end
@@ -353,7 +353,7 @@ def expect_http_error(cause, expected_message)
       stub_request(:post, uri)
 
       response = client.post(uri, "", headers: {'Content-Type' => 'text/plain'})
-      expect(response).to be_an_instance_of(Puppet::HTTP::Response)
+      expect(response).to be_a(Puppet::HTTP::Response)
       expect(response).to be_success
       expect(response.code).to eq(200)
     end
@@ -435,7 +435,7 @@ def expect_http_error(cause, expected_message)
       stub_request(:delete, uri)
 
       response = client.delete(uri)
-      expect(response).to be_an_instance_of(Puppet::HTTP::Response)
+      expect(response).to be_a(Puppet::HTTP::Response)
       expect(response).to be_success
       expect(response.code).to eq(200)
     end

spec/unit/http/external_client_spec.rb

@@ -61,7 +61,7 @@ def do_request(request, options)
       stub_request(:get, uri)
 
       response = client.get(uri)
-      expect(response).to be_an_instance_of(Puppet::HTTP::Response)
+      expect(response).to be_a(Puppet::HTTP::Response)
       expect(response).to be_success
       expect(response.code).to eq(200)
     end
@@ -113,7 +113,7 @@ def do_request(request, options)
       stub_request(:post, uri)
 
       response = client.post(uri, "", headers: {'Content-Type' => 'text/plain'})
-      expect(response).to be_an_instance_of(Puppet::HTTP::Response)
+      expect(response).to be_a(Puppet::HTTP::Response)
       expect(response).to be_success
       expect(response.code).to eq(200)
     end

spec/unit/http/session_spec.rb

@@ -222,6 +222,8 @@ def resolve(session, name, ssl_context: nil, canceled_handler: nil)
   end
 
   context 'when retrieving capabilities' do
+    let(:response) { Puppet::HTTP::Response.new(uri, 200, 'OK') }
+
     let(:session) do
       resolver = DummyResolver.new(good_service)
       described_class.new(client, [resolver])
@@ -242,7 +244,7 @@ def resolve(session, name, ssl_context: nil, canceled_handler: nil)
       end
 
       it "supports locales if the cached service's version is 5.3.4 or greater" do
-        response = Puppet::HTTP::Response.new({'X-Puppet-Version' => '5.3.4'}, uri)
+        allow(response).to receive(:[]).with('X-Puppet-Version').and_return('5.3.4')
 
         session.route_to(:puppet)
         session.process_response(response)
@@ -251,7 +253,7 @@ def resolve(session, name, ssl_context: nil, canceled_handler: nil)
       end
 
       it "does not support locales if the cached service's version is 5.3.3" do
-        response = Puppet::HTTP::Response.new({'X-Puppet-Version' => '5.3.3'}, uri)
+        allow(response).to receive(:[]).with('X-Puppet-Version').and_return('5.3.3')
 
         session.route_to(:puppet)
         session.process_response(response)
@@ -260,7 +262,7 @@ def resolve(session, name, ssl_context: nil, canceled_handler: nil)
       end
 
       it "does not support locales if the cached service's version is missing" do
-        response = Puppet::HTTP::Response.new({}, uri)
+        allow(response).to receive(:[]).with('X-Puppet-Version').and_return(nil)
 
         session.route_to(:puppet)
         session.process_response(response)
@@ -277,7 +279,7 @@ def resolve(session, name, ssl_context: nil, canceled_handler: nil)
       end
 
       it "supports json if the cached service's version is 5 or greater" do
-        response = Puppet::HTTP::Response.new({'X-Puppet-Version' => '5.5.12'}, uri)
+        allow(response).to receive(:[]).with('X-Puppet-Version').and_return('5.5.12')
 
         session.route_to(:puppet)
         session.process_response(response)
@@ -286,7 +288,7 @@ def resolve(session, name, ssl_context: nil, canceled_handler: nil)
       end
 
       it "does not support json if the cached service's version is less than 5.0" do
-        response = Puppet::HTTP::Response.new({'X-Puppet-Version' => '4.10.1'}, uri)
+        allow(response).to receive(:[]).with('X-Puppet-Version').and_return('4.10.1')
 
         session.route_to(:puppet)
         session.process_response(response)
@@ -295,7 +297,7 @@ def resolve(session, name, ssl_context: nil, canceled_handler: nil)
       end
 
       it "supports json if the cached service's version is missing" do
-        response = Puppet::HTTP::Response.new({}, uri)
+        allow(response).to receive(:[]).with('X-Puppet-Version').and_return(nil)
 
         session.route_to(:puppet)
         session.process_response(response)