From 93839837a5ca5ee74cc7fb81ec980871f7d23f00 Mon Sep 17 00:00:00 2001 From: Raul Gracia Date: Wed, 26 Mar 2014 12:39:13 +0000 Subject: [PATCH] Fixing #32 for incorrect handling http 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, and if it was a 200 then it will try to parse the body. --- lib/active_rest_client/request.rb | 25 ++++++++++++------------- spec/lib/request_spec.rb | 23 +++++++++-------------- 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/lib/active_rest_client/request.rb b/lib/active_rest_client/request.rb index 6847f2b..44d622d 100644 --- a/lib/active_rest_client/request.rb +++ b/lib/active_rest_client/request.rb @@ -268,10 +268,22 @@ def handle_cached_response(cached) end def handle_response(response) + response.status ||= 200 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 + elsif response.status == 401 + raise HTTPUnauthorisedClientException.new(status:response.status, url:@url) + elsif response.status == 403 + raise HTTPForbiddenClientException.new(status:response.status, url:@url) + elsif response.status == 404 + raise HTTPNotFoundClientException.new(status:response.status, url:@url) + elsif (400..499).include? response.status + raise HTTPClientException.new(status:response.status, url:@url) + elsif (500..599).include? response.status + raise HTTPServerException.new(status:response.status, url:@url) 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 @@ -308,19 +320,6 @@ def handle_response(response) 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) diff --git a/spec/lib/request_spec.rb b/spec/lib/request_spec.rb index 0a12b19..ab81ddb 100644 --- a/spec/lib/request_spec.rb +++ b/spec/lib/request_spec.rb @@ -248,12 +248,11 @@ class FilteredBodyExampleClient < ExampleClient object = ExampleClient.new(first_name:"John", should_disappear:true) begin object.create - rescue ActiveRestClient::HTTPUnauthorisedClientException => e + rescue => e e end expect(e).to be_instance_of(ActiveRestClient::HTTPUnauthorisedClientException) expect(e.status).to eq(401) - expect(e.result.first_name).to eq("John") end it "should raise a forbidden client exception for 403 errors" do @@ -265,12 +264,11 @@ class FilteredBodyExampleClient < ExampleClient object = ExampleClient.new(first_name:"John", should_disappear:true) begin object.create - rescue ActiveRestClient::HTTPForbiddenClientException => e + rescue => e e end expect(e).to be_instance_of(ActiveRestClient::HTTPForbiddenClientException) expect(e.status).to eq(403) - expect(e.result.first_name).to eq("John") end it "should raise a not found client exception for 404 errors" do @@ -282,12 +280,11 @@ class FilteredBodyExampleClient < ExampleClient object = ExampleClient.new(first_name:"John", should_disappear:true) begin object.create - rescue ActiveRestClient::HTTPNotFoundClientException => e + rescue => e e end expect(e).to be_instance_of(ActiveRestClient::HTTPNotFoundClientException) expect(e.status).to eq(404) - expect(e.result.first_name).to eq("John") end it "should raise a client exceptions for 4xx errors" do @@ -299,12 +296,11 @@ class FilteredBodyExampleClient < ExampleClient object = ExampleClient.new(first_name:"John", should_disappear:true) begin object.create - rescue ActiveRestClient::HTTPClientException => e + rescue => e e end expect(e).to be_instance_of(ActiveRestClient::HTTPClientException) expect(e.status).to eq(409) - expect(e.result.first_name).to eq("John") end it "should raise a server exception for 5xx errors" do @@ -316,29 +312,28 @@ class FilteredBodyExampleClient < ExampleClient object = ExampleClient.new(first_name:"John", should_disappear:true) begin object.create - rescue ActiveRestClient::HTTPServerException => e + rescue => e e end expect(e).to be_instance_of(ActiveRestClient::HTTPServerException) expect(e.status).to eq(500) - expect(e.result.first_name).to eq("John") end it "should raise a parse exception for invalid JSON returns" do - error_content = "

500 Server Error

" + error_content = "

invalid json content

" 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:{}, status:500)) + and_return(OpenStruct.new(body:error_content, headers:{}, status:200)) 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.status).to eq(500) + expect(e.status).to eq(200) expect(e.body).to eq(error_content) end