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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0a0ee1
Also catch faraday server errors
tlbraams Aug 22, 2022
b68e95d
handle ServerError not existing for faraday < 1
tlbraams Aug 23, 2022
e3bb98b
ensure timeout errors are re-raised
tlbraams Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/diplomat/query.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ 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 Faraday::ClientError
rescue Faraday::ClientError, Faraday::ServerError
Copy link
Member

Choose a reason for hiding this comment

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

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
      ...

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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)

raise Diplomat::QueryAlreadyExists
end

Expand Down