Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Faraday::Error to get consistent response information #1135

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 15 additions & 2 deletions lib/faraday/error.rb
Expand Up @@ -6,10 +6,11 @@ module Faraday
class Error < StandardError
attr_reader :response, :wrapped_exception

def initialize(exc, response = nil)
def initialize(exc, deprecated_response = nil, response = nil)
Copy link
Author

@qsona qsona Mar 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me assume that in faraday v2 this signature should be changed to def initialize(exc = nil, response:) (using keyword arg, as I mentioned in the PR description). Then, first I thought to change the signature at this time to such like

def initialize(exc, deperecated_response = nil, response: nil)

but I couldn't to do that because the old response object is hash and it conflicts with the keyword argument.

@wrapped_exception = nil unless defined?(@wrapped_exception)
@response = nil unless defined?(@response)
super(exc_msg_and_response!(exc, response))
@v2_response = response
super(exc_msg_and_response!(exc, deprecated_response))
end

def backtrace
Expand All @@ -28,6 +29,18 @@ def inspect
%(#<#{self.class}#{inner}>)
end

def response_status
@v2_response&.status
end

def response_body
@v2_response&.body
end

def response_headers
@v2_response&.headers
end

protected

# Pulls out potential parent exception and response hash, storing them in
Expand Down
27 changes: 18 additions & 9 deletions lib/faraday/response/raise_error.rb
Expand Up @@ -13,25 +13,34 @@ class RaiseError < Middleware
def on_complete(env)
case env[:status]
when 400
raise Faraday::BadRequestError, response_values(env)
raise Faraday::BadRequestError
.new(response_values(env), nil, env.response)
when 401
raise Faraday::UnauthorizedError, response_values(env)
raise Faraday::UnauthorizedError
.new(response_values(env), nil, env.response)
when 403
raise Faraday::ForbiddenError, response_values(env)
raise Faraday::ForbiddenError
.new(response_values(env), nil, env.response)
when 404
raise Faraday::ResourceNotFound, response_values(env)
raise Faraday::ResourceNotFound
.new(response_values(env), nil, env.response)
when 407
# mimic the behavior that we get with proxy requests with HTTPS
msg = %(407 "Proxy Authentication Required")
raise Faraday::ProxyAuthError.new(msg, response_values(env))
raise Faraday::ProxyAuthError
.new(msg, response_values(env), env.response)
when 409
raise Faraday::ConflictError, response_values(env)
raise Faraday::ConflictError
.new(response_values(env), nil, env.response)
when 422
raise Faraday::UnprocessableEntityError, response_values(env)
raise Faraday::UnprocessableEntityError
.new(response_values(env), nil, env.response)
when ClientErrorStatuses
raise Faraday::ClientError, response_values(env)
raise Faraday::ClientError
.new(response_values(env), nil, env.response)
when ServerErrorStatuses
raise Faraday::ServerError, response_values(env)
raise Faraday::ServerError
.new(response_values(env), nil, env.response)
when nil
raise Faraday::NilStatusError, response_values(env)
end
Expand Down
32 changes: 30 additions & 2 deletions spec/faraday/error_spec.rb
Expand Up @@ -2,7 +2,8 @@

RSpec.describe Faraday::ClientError do
describe '.initialize' do
subject { described_class.new(exception, response) }
subject { described_class.new(exception, deprecated_response, response) }
let(:deprecated_response) { nil }
let(:response) { nil }

context 'with exception only' do
Expand All @@ -15,7 +16,7 @@
it { expect(subject.inspect).to eq('#<Faraday::ClientError wrapped=#<RuntimeError: test>>') }
end

context 'with response hash' do
context 'with deprecated_response hash' do
let(:exception) { { status: 400 } }

it { expect(subject.wrapped_exception).to be_nil }
Expand All @@ -41,5 +42,32 @@
it { expect(subject.message).to eq('["error1", "error2"]') }
it { expect(subject.inspect).to eq('#<Faraday::ClientError #<Faraday::ClientError: ["error1", "error2"]>>') }
end

context 'with exception string and deprecated_response hash' do
let(:exception) { 'custom message' }
let(:deprecated_response) { { status: 400 } }

it { expect(subject.wrapped_exception).to be_nil }
it { expect(subject.response).to eq(deprecated_response) }
it { expect(subject.message).to eq('custom message') }
it { expect(subject.inspect).to eq('#<Faraday::ClientError response={:status=>400}>') }
end

context 'with deprecated_response hash and response' do
let(:exception) { { status: 400 } }
let(:env) do
Faraday::Env.from(status: 400, body: 'yikes',
response_headers: { 'Content-Type' => 'text/plain' })
end
let(:response) { Faraday::Response.new(env) }

it { expect(subject.wrapped_exception).to be_nil }
it { expect(subject.response).to eq(exception) }
it { expect(subject.message).to eq('the server responded with status 400') }
it { expect(subject.inspect).to eq('#<Faraday::ClientError response={:status=>400}>') }
it { expect(subject.response_status).to eq(400) }
it { expect(subject.response_body).to eq('yikes') }
it { expect(subject.response_headers).to eq({ 'Content-Type' => 'text/plain' }) }
end
end
end
27 changes: 27 additions & 0 deletions spec/faraday/response/raise_error_spec.rb
Expand Up @@ -29,6 +29,9 @@
expect(ex.message).to eq('the server responded with status 400')
expect(ex.response[:headers]['X-Reason']).to eq('because')
expect(ex.response[:status]).to eq(400)
expect(ex.response_status).to eq(400)
expect(ex.response_body).to eq('keep looking')
expect(ex.response_headers['X-Reason']).to eq('because')
end
end

Expand All @@ -37,6 +40,9 @@
expect(ex.message).to eq('the server responded with status 401')
expect(ex.response[:headers]['X-Reason']).to eq('because')
expect(ex.response[:status]).to eq(401)
expect(ex.response_status).to eq(401)
expect(ex.response_body).to eq('keep looking')
expect(ex.response_headers['X-Reason']).to eq('because')
end
end

Expand All @@ -45,6 +51,9 @@
expect(ex.message).to eq('the server responded with status 403')
expect(ex.response[:headers]['X-Reason']).to eq('because')
expect(ex.response[:status]).to eq(403)
expect(ex.response_status).to eq(403)
expect(ex.response_body).to eq('keep looking')
expect(ex.response_headers['X-Reason']).to eq('because')
end
end

Expand All @@ -53,6 +62,9 @@
expect(ex.message).to eq('the server responded with status 404')
expect(ex.response[:headers]['X-Reason']).to eq('because')
expect(ex.response[:status]).to eq(404)
expect(ex.response_status).to eq(404)
expect(ex.response_body).to eq('keep looking')
expect(ex.response_headers['X-Reason']).to eq('because')
end
end

Expand All @@ -61,6 +73,9 @@
expect(ex.message).to eq('407 "Proxy Authentication Required"')
expect(ex.response[:headers]['X-Reason']).to eq('because')
expect(ex.response[:status]).to eq(407)
expect(ex.response_status).to eq(407)
expect(ex.response_body).to eq('keep looking')
expect(ex.response_headers['X-Reason']).to eq('because')
end
end

Expand All @@ -69,6 +84,9 @@
expect(ex.message).to eq('the server responded with status 409')
expect(ex.response[:headers]['X-Reason']).to eq('because')
expect(ex.response[:status]).to eq(409)
expect(ex.response_status).to eq(409)
expect(ex.response_body).to eq('keep looking')
expect(ex.response_headers['X-Reason']).to eq('because')
end
end

Expand All @@ -77,6 +95,9 @@
expect(ex.message).to eq('the server responded with status 422')
expect(ex.response[:headers]['X-Reason']).to eq('because')
expect(ex.response[:status]).to eq(422)
expect(ex.response_status).to eq(422)
expect(ex.response_body).to eq('keep looking')
expect(ex.response_headers['X-Reason']).to eq('because')
end
end

Expand All @@ -93,6 +114,9 @@
expect(ex.message).to eq('the server responded with status 499')
expect(ex.response[:headers]['X-Reason']).to eq('because')
expect(ex.response[:status]).to eq(499)
expect(ex.response_status).to eq(499)
expect(ex.response_body).to eq('keep looking')
expect(ex.response_headers['X-Reason']).to eq('because')
end
end

Expand All @@ -101,6 +125,9 @@
expect(ex.message).to eq('the server responded with status 500')
expect(ex.response[:headers]['X-Error']).to eq('bailout')
expect(ex.response[:status]).to eq(500)
expect(ex.response_status).to eq(500)
expect(ex.response_body).to eq('fail')
expect(ex.response_headers['X-Error']).to eq('bailout')
end
end
end