Fixing handling of http reponse errors

This commit fix the error of rising a wrong HTTPParseException when
trying to parse the body for non 200 response status code

Now we first doublecheck the status codes for the response, then we
check if content-type header is json and rise the parse execption if not

We also ensure that if status code is not 200 we don't pollute the
attributes for the current object.

lib/active_rest_client/request.rb

@@ -268,62 +268,49 @@ def handle_cached_response(cached)
     end
 
     def handle_response(response)
-      if response.status == 304
-        ActiveRestClient::Logger.debug "  \033[1;4;32m#{ActiveRestClient::NAME}\033[0m #{@instrumentation_name} - Etag copy is the same as the server"
-        return :not_modified
-      end
-      if response.respond_to?(:proxied) && response.proxied
-        ActiveRestClient::Logger.debug "  \033[1;4;32m#{ActiveRestClient::NAME}\033[0m #{@instrumentation_name} - Response was proxied, unable to determine size"
-      else
-        ActiveRestClient::Logger.debug "  \033[1;4;32m#{ActiveRestClient::NAME}\033[0m #{@instrumentation_name} - Response received #{response.body.size} bytes"
-      end
-
-      if @method[:options][:plain]
-        return @response = response.body
-      end
-
       @response = response
+      @response.status ||= 200
 
-      if response.body.is_a?(Array) || response.body.is_a?(Hash)
-        body = response.body
-      else
-        body = MultiJson.load(response.body) || {}
-      end
-      body = begin
-        @method[:name].nil? ? body : translator.send(@method[:name], body)
-      rescue NoMethodError
-        body
-      end
-      if body.is_a? Array
-        result = ActiveRestClient::ResultIterator.new(response.status)
-        body.each do |json_object|
-          result << new_object(json_object, @overriden_name)
+      if (200..399).include? @response.status
+        if response.status == 304
+          ActiveRestClient::Logger.debug "  \033[1;4;32m#{ActiveRestClient::NAME}\033[0m #{@instrumentation_name} - Etag copy is the same as the server"
+          return :not_modified
+        end
+        if @method[:options][:plain]
+          return @response = response.body
+        elsif is_json_response?
+          if @response.respond_to?(:proxied) && @response.proxied
+            ActiveRestClient::Logger.debug "  \033[1;4;32m#{ActiveRestClient::NAME}\033[0m #{@instrumentation_name} - Response was proxied, unable to determine size"
+          else
+            ActiveRestClient::Logger.debug "  \033[1;4;32m#{ActiveRestClient::NAME}\033[0m #{@instrumentation_name} - Response received #{@response.body.size} bytes"
+          end
+          result = generate_new_object
+        else
+          raise ResponseParseException.new(status:@response.status, body:@response.body)
         end
       else
-        result = new_object(body, @overriden_name)
-        result._status = response.status
-        unless object_is_class?
-          @object._copy_from(result)
-          result = @object
+        if is_json_response?
+          error_response = generate_new_object(mutable: false)
+        else
+          error_response = @response.body
+        end
+        if @response.status == 400
+          raise HTTPBadRequestClientException.new(status:@response.status, result:error_response, url:@url)
+        elsif @response.status == 401
+          raise HTTPUnauthorisedClientException.new(status:@response.status, result:error_response, url:@url)
+        elsif @response.status == 403
+          raise HTTPForbiddenClientException.new(status:@response.status, result:error_response, url:@url)
+        elsif @response.status == 404
+          raise HTTPNotFoundClientException.new(status:@response.status, result:error_response, url:@url)
+        elsif (400..499).include? @response.status
+          raise HTTPClientException.new(status:@response.status, result:error_response, url:@url)
+        elsif (500..599).include? @response.status
+          raise HTTPServerException.new(status:@response.status, result:error_response, url:@url)
         end
       end
 
-      response.status ||= 200
-      if response.status == 401
-        raise HTTPUnauthorisedClientException.new(status:response.status, result:result, url:@url)
-      elsif response.status == 403
-        raise HTTPForbiddenClientException.new(status:response.status, result:result, url:@url)
-      elsif response.status == 404
-        raise HTTPNotFoundClientException.new(status:response.status, result:result, url:@url)
-      elsif (400..499).include? response.status
-        raise HTTPClientException.new(status:response.status, result:result, url:@url)
-      elsif (500..599).include? response.status
-        raise HTTPServerException.new(status:response.status, result:result, url:@url)
-      end
 
       result
-    rescue MultiJson::ParseError
-      raise ResponseParseException.new(status:response.status, body:response.body)
     end
 
     def new_object(attributes, name = nil)
@@ -418,6 +405,39 @@ def handle_hal_links_embedded(object, attributes)
 
       attributes
     end
+
+    private
+
+    def is_json_response?
+      @response.headers['Content-Type'].nil? || @response.headers['Content-Type'].include?('json')
+    end
+
+    def generate_new_object(options={})
+      if @response.body.is_a?(Array) || @response.body.is_a?(Hash)
+        body = @response.body
+      else
+        body = MultiJson.load(@response.body) || {}
+      end
+      body = begin
+        @method[:name].nil? ? body : translator.send(@method[:name], body)
+      rescue NoMethodError
+        body
+      end
+      if body.is_a? Array
+        result = ActiveRestClient::ResultIterator.new(@response.status)
+        body.each do |json_object|
+          result << new_object(json_object, @overriden_name)
+        end
+      else
+        result = new_object(body, @overriden_name)
+        result._status = @response.status
+        if !object_is_class? && options[:mutable] != false
+          @object._copy_from(result)
+          result = @object
+        end
+      end
+      result
+    end
   end
 
   class RequestException < StandardError ; end
@@ -441,6 +461,7 @@ def initialize(options)
   end
   class HTTPClientException < HTTPException ; end
   class HTTPUnauthorisedClientException < HTTPClientException ; end
+  class HTTPBadRequestClientException < HTTPClientException ; end
   class HTTPForbiddenClientException < HTTPClientException ; end
   class HTTPNotFoundClientException < HTTPClientException ; end
   class HTTPServerException < HTTPException ; end

spec/lib/request_spec.rb

@@ -270,7 +270,7 @@ class FilteredBodyExampleClient < ExampleClient
     end
     expect(e).to be_instance_of(ActiveRestClient::HTTPForbiddenClientException)
     expect(e.status).to eq(403)
-  expect(e.result.first_name).to eq("John")
+    expect(e.result.first_name).to eq("John")
   end
 
   it "should raise a not found client exception for 404 errors" do
@@ -304,7 +304,7 @@ class FilteredBodyExampleClient < ExampleClient
     end
     expect(e).to be_instance_of(ActiveRestClient::HTTPClientException)
     expect(e.status).to eq(409)
-  expect(e.result.first_name).to eq("John")
+    expect(e.result.first_name).to eq("John")
   end
 
   it "should raise a server exception for 5xx errors" do
@@ -330,25 +330,79 @@ class FilteredBodyExampleClient < ExampleClient
       any_instance.
       should_receive(:post).
       with("/create", "first_name=John&should_disappear=true", an_instance_of(Hash)).
-      and_return(OpenStruct.new(body:error_content, headers:{}, status:500))
+      and_return(OpenStruct.new(body:error_content, headers:{'Content-Type' => 'text/html'}, status:500))
     object = ExampleClient.new(first_name:"John", should_disappear:true)
     begin
       object.create
-    rescue ActiveRestClient::ResponseParseException => e
+    rescue => e
       e
     end
-    expect(e).to be_instance_of(ActiveRestClient::ResponseParseException)
+    expect(e).to be_instance_of(ActiveRestClient::HTTPServerException)
     expect(e.status).to eq(500)
+    expect(e.result).to eq(error_content)
+  end
+
+  it "should raise a bad request exception for 400 response status" do
+    error_content = "<h1>400 Bad Request</h1>"
+    ActiveRestClient::Connection.
+      any_instance.
+      should_receive(:post).
+      with("/create", "first_name=John&should_disappear=true", an_instance_of(Hash)).
+      and_return(OpenStruct.new(body:error_content, headers:{'Content-Type' => 'text/html'}, status:400))
+    object = ExampleClient.new(first_name:"John", should_disappear:true)
+    begin
+      object.create
+    rescue => e
+      e
+    end
+    expect(e).to be_instance_of(ActiveRestClient::HTTPBadRequestClientException)
+    expect(e.status).to eq(400)
+    expect(e.result).to eq(error_content)
+  end
+
+  it "should raise response parse exception for 200 response status and non json content type" do
+    error_content = "<h1>malformed json</h1>"
+    ActiveRestClient::Connection.
+      any_instance.
+      should_receive(:post).
+      with("/create", "first_name=John&should_disappear=true", an_instance_of(Hash)).
+      and_return(OpenStruct.new(body:error_content, headers:{'Content-Type' => 'text/html'}, status:200))
+    object = ExampleClient.new(first_name:"John", should_disappear:true)
+    begin
+      object.create
+    rescue => e
+      e
+    end
+    expect(e).to be_instance_of(ActiveRestClient::ResponseParseException)
+    expect(e.status).to eq(200)
     expect(e.body).to eq(error_content)
   end
 
+  it "should not override the attributes of the existing object on error response status" do
+    ActiveRestClient::Connection.
+      any_instance.
+      should_receive(:post).
+      with("/create", "first_name=John&should_disappear=true", an_instance_of(Hash)).
+      and_return(OpenStruct.new(body:'{"errors": ["validation": "error in validation"]}', headers:{'Content-Type' => 'text/html'}, status:400))
+    object = ExampleClient.new(first_name:"John", should_disappear:true)
+    begin
+      object.create
+    rescue => e
+      e
+    end
+    expect(e).to be_instance_of(ActiveRestClient::HTTPBadRequestClientException)
+    expect(e.status).to eq(400)
+    expect(object.first_name).to eq 'John'
+    expect(object.errors).to be_nil
+  end
+
   it "should raise an exception if you try to pass in an unsupport method" do
     method = {:method => :wiggle, url:"/"}
     class RequestFakeObject
       def request_body_type
         :form_encoded
       end
-      
+
       def base_url
         "http://www.example.com/"
       end