Allow Faraday init params to be passed to HTTP adaptor #288
Conversation
|
|
||
| def connect | ||
| @requestor = Requestor.new(@url, USER_AGENT_STRING, self.class.method(:instrument_request)) | ||
| @requestor = Requestor.new(@url, USER_AGENT_STRING, self.class.method(:instrument_request), @options[:faraday_opts] = {}) |
There was a problem hiding this comment.
Nitpick: faraday_options would be better, I think
Also, did you mean to do @options[:faraday_opts] ||= {} here? Otherwise it's going to always set / pass through an empty Hash. You could also do @options.fetch(:faraday_options, {})
There was a problem hiding this comment.
Nice catch. That was some post-implementation cleanup I thought was correct, so thanks for spotting
| @password = password | ||
| @user_agent_string = user_agent_string | ||
| @faraday = wrap_connection_failed! { faraday_connection } | ||
| init_params = params[:initialize] && params.delete(:initialize) if params.key?(:initialize) |
There was a problem hiding this comment.
I know this is what the previous code did, but I don't like the delete because it modifies the Hash that we were given. I'd be happy with just the following if it works:
@faraday = wrap_connection_failed! { faraday_connection(params.fetch(:initialize, {})) }Honestly since we're passing in a hash with the key of :faraday_options above, I don't know why we need the extra :initialize key underneath. I would actually probably just rename params to faraday_options and pass it through.
Separately we're going to need to make the Rails integration like this work:
config.neo4j.session.options = {initialize: { ssl: { verify: true }}}But I think that we could simply pass down the values of initialize from the railtie in the neo4j gem into here as well as also supporting:
config.neo4j.session.options = {faraday_options: { ssl: { verify: true }}}And probably raising a deprecation warning about how initialize should be replaced with faraday_options
There was a problem hiding this comment.
Done (apart from rails-related config changes)
| private | ||
|
|
||
| def faraday_connection | ||
| def faraday_connection(params = {}) |
There was a problem hiding this comment.
Since we're in the place where we're creating the Faraday connection, I would probably just call this variable options to match with the naming of faraday_options. Also, I've seen a general pattern of using options as the hash of optional arguments at the end of a Ruby function. When I see params I think of Rails' controller parameters (that may just be my preference, but I think it's a trend)
6 similar comments
* Allow Faraday init params to be passed to HTTP adaptor * Update http.rb * Fix failing tests * Fix failing rubocop issues * Fix silly C&P error (cherry picked from commit b492670)
|
Thanks so much @heavydawson ! I merged and then made some changes of my own, mainly refactoring. The biggest functional change that I made was to pass the options directly through to Faraday without the https://github.com/neo4jrb/neo4j-core/blob/7.0.x/lib/neo4j/core/cypher_session/adaptors/http.rb Could you give it a final try to make sure it works for you since I removed the gem 'neo4j-core', github: 'neo4jrb/neo4j-core', branch: '7.0.x'If that works I'll release a patch version. Thanks again! |
|
Also, I decided since there was no ability to configure Faraday in 8.0.x, that it wasn't a big deal to not deprecate |
|
Hey @heavydawson ! I actually changed this from http://neo4jrb.readthedocs.io/en/8.0.x/Setup.html#configuring-faraday The change is available starting with version 7.0.8 of |
3 participants
This pull introduces/changes:
A new key called
:faraday_optsis available for use in the options array passed to the HTTP adaptorExample usage:
adaptor ||= Neo4j::Core::CypherSession::Adaptors::HTTP.new("http://"
"#{@user}:"
"#{@password}@"
"#{@host}:"
"#{@http_port}",
faraday_opts: {initialize: { request: { open_timeout: 2, timeout: 600 }} })
Pings:
@cheerfulstoic
@subvertallchris