Inconsistent response attribute for Faraday::RetriableResponse raised on Retry loop

[Alan-M-Thomaz]

Basic Info

• Faraday Version: 0.15.4
• Ruby Version: 2.5.1p57

Issue description

The response on the exception Faraday::RetriableResponse raised by retry loop is a instance of Faraday::Response, while errors raised by the middleware RaiseError are simple hash.

that is kinda bad for checking body, headers, and status, because in one they are attributes, and in the other they are keys

Steps to reproduce

Using webmock , and rspec. i removed some unnecessary code, you probably can also use just faraday stubs instead of webmock.

let(:stub_send_request_unavaible_server) do
  i = 0
  stub_request(:post, "https://www.test.com/send").with(
    body: "{}",
    headers: {
      'Accept'=>'*/*',
      'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3',
      'User-Agent'=>'Faraday v0.15.4'
  }).to_return do |stub|
    body = {}
    status = ( i == 0) ? 501 : 200
    body[:result] = "error" if i == 1
    i += 1
    {status: status, headers: { "Retry-After": "1"}, body: body.to_json}
  end
end

before(:each) do
  stub_send_request_unavaible_server
end

it 'should retry 2 times' do
  retry_if_func = lambda do |env, exception|
    p exception.response
    case (exception.response[:status] || exception.response.status)
    when (500..599)
      true
    when (400..499)
      false
    when 200
      body = JSON.parse(exception.response.body)
      body["result"] == "error"
    end
  end
  retryable_exceptions =  Faraday::Request::Retry::DEFAULT_EXCEPTIONS + [ Faraday::ClientError]
  connection = ::Faraday.new(:url => "https://www.test.com") do |faraday|
    faraday.request :retry, max: 5, interval: 0.1, interval_randomness: 0.5, backoff_factor: 2,
                      exceptions: retryable_exceptions, methods: [], retry_statuses: [200],
                      retry_if: retry_if_func
    faraday.response :raise_error
    faraday.adapter Faraday.default_adapter
  end
  connection.post('/send', "{}")
  stub_send_request_unavaible_server.should have_been_made.times(3)
end

[technoweenie]

I agree wholeheartedly that this inconsistency is a problem. I would like to move away from wide open hashes, towards objects with documented attributes. Ideally, our users should avoid code like that case statement in your example, and Faraday should not make that necessary.

We're currently in a push to release Faraday v1 (#903), so we can't simply fix this by returning a Response object in RaiseError. I would be open to some helpers like #response_status though. After a quick glance at Faraday::Response, I think at least we'd want the following:

• response_status
• response_headers
• response_body
• response_finished?
• response_success?

I've made a note of this on my Faraday v2 wishlist (#953) to fix properly.

[pboling]

For those stuck on Faraday v1, this may be useful:

      case response
        when Hash
          # Sometimes response is a hash, and other times it is a response object.
          # Downstream from here it is expected to always be a response object.
          # See: https://github.com/lostisland/faraday/issues/956#issuecomment-478072433
          Faraday::Response.new(response.transform_keys({headers: :response_headers}))
        else
          response
      end

It rebuilds the response object from the Hash.