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

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

Open
BenDrozdoff opened this issue Dec 19, 2019 · 6 comments

Comments

@BenDrozdoff
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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

@dblock
Copy link
Collaborator

dblock commented Dec 24, 2019

lostisland/faraday#1090

@dblock
Copy link
Collaborator

dblock commented Dec 24, 2019

@iMacTia
Copy link

iMacTia commented Dec 27, 2019

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 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants