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

Show file tree
Hide file tree
Changes from all commits
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
4 changes: 3 additions & 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,9 @@ 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::TimeoutError => e
Copy link
Member

Choose a reason for hiding this comment

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

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

raise e
rescue *faraday_error_classes
Copy link
Member

Choose a reason for hiding this comment

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

See my comment below on faraday_error_classes

raise Diplomat::QueryAlreadyExists
end

Expand Down
12 changes: 11 additions & 1 deletion lib/diplomat/rest_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,9 @@ def send_get_request(connection, url, options, custom_params = nil)
rest_options[:headers].map { |k, v| req.headers[k.to_sym] = v } unless rest_options[:headers].nil?
req.options.timeout = options[:timeout] if options[:timeout]
end
rescue Faraday::ClientError, Faraday::ServerError => e
rescue Faraday::TimeoutError => e
raise e
rescue *faraday_error_classes => e
Copy link
Member

Choose a reason for hiding this comment

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

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

resp = e.response
if resp
raise Diplomat::AclNotFound, e if resp[:status] == 403 && resp[:body] == 'ACL not found'
Expand Down Expand Up @@ -304,6 +306,14 @@ def send_delete_request(connection, url, options, custom_params = nil)
end
end

# This method returns the correct Exception Classes depending on the Faraday version being installed
# see https://github.com/WeAreFarmGeek/diplomat/issues/227
#
# @return [Array<Class>] Faraday error classes that should be caught
def faraday_error_classes
Faraday.const_defined?(:ServerError) ? [Faraday::ClientError, Faraday::ServerError] : [Faraday::ClientError]
Copy link
Member

Choose a reason for hiding this comment

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

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

end
Comment on lines +313 to +315
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.


# Mapping for valid key/value store transaction verbs and required parameters
#
# @return [Hash] valid key/store transaction verbs and required parameters
Expand Down