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

Allow Faraday init params to be passed to HTTP adaptor #288

Merged
merged 5 commits into from
Mar 14, 2017
Merged

Allow Faraday init params to be passed to HTTP adaptor #288

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
62ad69c
Allow Faraday init params to be passed to HTTP adaptor
heavydawson Mar 11, 2017
0b42d54
Update http.rb
heavydawson Mar 13, 2017
424843a
Fix failing tests
heavydawson Mar 13, 2017
817261e
Fix failing rubocop issues
heavydawson Mar 13, 2017
2b8c741
Fix silly C&P error
heavydawson Mar 13, 2017
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
13 changes: 7 additions & 6 deletions lib/neo4j/core/cypher_session/adaptors/http.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def initialize(url, options = {})
end

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] = {})
Copy link
Contributor

Choose a reason for hiding this comment

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

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, {})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. That was some post-implementation cleanup I thought was correct, so thanks for spotting

end

ROW_REST = %w(row REST)
Expand Down Expand Up @@ -101,13 +101,13 @@ class Requestor
include Adaptors::HasUri
default_url('http://neo4:neo4j@localhost:7474')
validate_uri { |uri| uri.is_a?(URI::HTTP) }

def initialize(url, user_agent_string, instrument_proc)
def initialize(url, user_agent_string, instrument_proc, params = {})
self.url = url
@user = user
@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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (apart from rails-related config changes)

@faraday = wrap_connection_failed! { faraday_connection(init_params) }
@instrument_proc = instrument_proc
end

Expand Down Expand Up @@ -144,12 +144,13 @@ def get(path, body = '', options = {})

private

def faraday_connection
def faraday_connection(params = {})
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

require 'faraday'
require 'faraday_middleware/multi_json'

Faraday.new(url) do |c|
Faraday.new(url,params) do |c|
c.request :basic_auth, user, password
c.request :basic_auth, params[:basic_auth][:username], params[:basic_auth][:password] if params[:basic_auth]
c.request :multi_json

c.response :multi_json, symbolize_keys: true, content_type: 'application/json'
Expand Down