ashkan18 / graphlient

Ruby GraphQL Client
MIT License
251 stars 44 forks source link

Faraday Timeouts no longer coming through as instance of Graphlient::Errors::FaradayServerError #70

Open BenDrozdoff opened 4 years ago

BenDrozdoff commented 4 years ago

Starting in Faraday 0.17.1, the TimeoutError is no longer an instance of Faraday::ClientError, which is caught here as a Graphlient::Errors::FaradayServerError

See this line and this line as the breaking change.

We have some extra error handling logic based on Graphlient::Errors::FaradayServerErrors, so for now we've had to shoehorn in Timeout catching, whereas before they came through in Graphlient.

I haven't been able to dive in enough to figure out exactly what the right solution is, but I'm happy to be part of the discussion

BenDrozdoff commented 4 years ago

Possibly also relevant, we internally altered the constructor a bit due to the structure of Faraday::TimeoutError

module Graphlient
  module Errors
    class FaradayServerError < ServerError
      # Overriding this constructor. Timeout errors do not have a `response` member, thus we get NoMethodError on [].
      def initialize(inner_exception)
        super(inner_exception.message, inner_exception)
        @inner_exception = inner_exception
        return unless inner_exception.response.present?

        @response = inner_exception.response[:body]
        @status_code = inner_exception.response[:status]
      end
    end
  end
end
ashkan18 commented 4 years ago

Thanks @BenDrozdoff for reporting this 💜. Feel free to open a PR with your change or I'll try to get to this during holidays.

BenDrozdoff commented 4 years ago

NP, looks like the error I was seeing was pretty similar to #68 just with a different Faraday Error class.

dblock commented 4 years ago

https://github.com/lostisland/faraday/issues/1090

dblock commented 4 years ago

https://github.com/slack-ruby/slack-ruby-client/pull/309

iMacTia commented 4 years ago

Hi guys, sorry if this caused some pain, that was an involuntary change. I'm fixing this so that the change in hierarchy will be reverted.

Looking at your code, it doesn't seem like you'll need to change anything, but I'd suggest you keep the changes you've done since the changes on our side will come back in Faraday 1.0. This way you'll be one step closer to upgrading when the time comes 😄