Handle Faraday::ServerError in Diplomat::Query#create

[tlbraams]

Fixes #227

Builds on #203

To support Faraday 1.0, Faraday::ServerError handling was added to the Diplomat::RestClient#send_get_request method. The Diplomat::Server#create` method should also be updated.

Change passed rake with 1.10.1 and 0.17.3

[pierresouchay]

Hello @tlbraams ,

Thanks a lot for you contribution, that's great!

However, this fix is currently causing issues with Faraday 0.9.x -> so would break those installations.

I made 2 suggestions to avoid that, feel free to fix and I'll be glad to merge your changes

[pierresouchay]

This code is now using the same path for both Faraday::ClientError and Faraday::ServerError

The distinction between both started in version 1.0 as you noted and has been introduced with this commit:
https://github.com/lostisland/faraday/pull/841/files#diff-c5f031a4be2bbda4d7befb7c1604ffee77d3bc86038f280aca9b14f1fc00350e

Both Faraday::ClientError and Faraday::ServerError inherit the same Faraday::Error class and this is the case before and after this change.

The problem with your fix is that rescuing both will create a NameError on versions of Faraday lower than 1.x (assuming that some users are using it, but it is definitely the case from my experience), if you look at dependencies in GemFile, 0.9 is still supported.

I would suggest 2 possible solutions here:

• either simply do a rescue Faraday::Error => would work for both ServerError and ClientError for Faraday 1+ but would also work on Faraday 0.9.x
• either check that if Faraday::ServerError is defined, catch it, otherwise, catch ClientError

The second solution is probably better, because the message would be more accurate with Faraday 1.x because a timeout would never trigger a "QuertAlreadyExists" Exception which is misleading

This could be done by something like this:

# This method returns the correct Exception Class for ServerError depending of Faraday version being installed
# see https://github.com/WeAreFarmGeek/diplomat/issues/227
def serverExceptionToCatch()
  Faraday.const_defined?(:ServerError) ? Faraday::ServerError : Faraday::Error
end

[...]
def create(definition, options = {})
      custom_params = options[:dc] ? use_named_parameter('dc', options[:dc]) : nil
      @raw = send_post_request(@conn, ['/v1/query'], options, definition, custom_params)
      parse_body
    rescue serverExceptionToCatch
      ...

[tlbraams]

I was actually wondering if that might cause issues. I did see that in the PR I linked, the same change was made:

diplomat/lib/diplomat/rest_client.rb

Line 259 in 1165891

rescue Faraday::ClientError, Faraday::ServerError => e

This caused me to believe this change should be fine. Is it actually broken in the rest client aswell then?

[pierresouchay]

Yes, indeed, it has not been reported, but pointing to a non existing class will actually fail.

Example:

irb
 :001 > require 'faraday'
 => true
 :002 > begin
 :003 >   1/0
 :004 > rescue Faraday::ClientError, Faraday::ServerError, Faraday::WeirdError, ZeroDivisionError
 :005 >   puts "bye"
 :006 > end
Traceback (most recent call last):
        5: from /Users/b188lf/.rvm/rubies/ruby-2.7.2/bin/irb:23:in `<main>'
        4: from /Users/b188lf/.rvm/rubies/ruby-2.7.2/bin/irb:23:in `load'
        3: from /Users/b188lf/.rvm/rubies/ruby-2.7.2/lib/ruby/gems/2.7.0/gems/irb-1.2.6/exe/irb:11:in `<top (required)>'
        2: from (irb):2
        1: from (irb):4:in `rescue in irb_binding'
NameError (uninitialized constant Faraday::WeirdError)

[tlbraams]

Not entirely sure about the name/placement of this method, but is this like what you had in mind?

[pierresouchay]

Actually, i think it is bad to catch both ClientError and ServerError when ServerError exists, because it would thow a QueryAlreadyExists when a timeout occurs.

So I would use your code, but only return ServerError in such case, not ClientError, to avoid code calling this that this query exist while it is just that the agent did timeout for instance.

[tlbraams]

Hmm, Faraday::TimeoutError is a client error in faraday < 1 and a server error in >= 1, even though the documentation claims it's a unified client error. As such, I decided to explicitly catch and reraise timeout errors.

[pierresouchay]

Sorry, I was not clear last time (confusing about timeout), the best is probably to just catch Faraday::Error everywhere and not try to be too clever

[pierresouchay]

I would not put this, simply let the exception Faraday::Timeout error raise by itself... my example with timeout was a bad example, I was talking on ClientError on Faraday 1+

So, remove the :

    rescue Faraday::TimeoutError => e
        raise e

[pierresouchay]

This part of existing code sucks because it raises a Diplomat::PathNotFound in most cases, but users might rely on it... just rescue Faraday::Error and don't catch Faraday::TimeoutError

[pierresouchay]

Hum, we probably misunderstood each other:

My point was:

• Either we just want to catch all, in such case, we just catch Faraday::Error and this method is not useful => which is exactly what you do (because in Faraday 1+: Faraday::Error := Faraday::ClientError + Faraday::ServerError while in 0.9, Faraday::Error := Faraday::ClientError which includes also all errors of Faraday::ServerError )
• either you create this method, but in such case, it should only return Faraday::ServerError when defined and Faraday::ClientError on Faraday 0.9

[pierresouchay]

See my comment below on faraday_error_classes

[pierresouchay]

Hello @tlbraams,

Sorry, this MR takes some time, I hope you are not upset or afraid of contributing :-)

If you get lost or need help, feel free to ask

Regards