-
Notifications
You must be signed in to change notification settings - Fork 80
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.
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
Http driver config #273
base: master
Are you sure you want to change the base?
Http driver config #273
Changes from 18 commits
685628b
b011c46
662fed4
f270f53
7dbcf0a
f9c2f31
f376dc9
eee66cc
ca48b34
074ca27
de0a304
b035039
d66d47c
99e3b3f
9db897e
aea75e0
bb32ab4
e6e1844
cfa8973
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
require 'uri' | ||
require 'neo4j/core/cypher_session/adaptors/faraday_helpers' | ||
|
||
module Neo4j | ||
module Server | ||
|
@@ -7,7 +8,22 @@ module Server | |
end | ||
|
||
class CypherSession < Neo4j::Session | ||
module PrivateMethods | ||
private | ||
|
||
def extract_faraday_options(params, defaults = {}) | ||
verify_faraday_options(params.delete(:faraday_options) || params.delete('faraday_options') || {}, defaults) | ||
end | ||
|
||
def extract_basic_auth(url, params) | ||
return unless url && URI(url).userinfo | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a huge deal, but you could perhaps create a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call |
||
params[:basic_auth] = {username: URI(url).user, password: URI(url).password} | ||
end | ||
end | ||
|
||
include Resource | ||
extend Neo4j::Core::CypherSession::Adaptors::FaradayHelpers | ||
extend Neo4j::Server::CypherSession::PrivateMethods | ||
|
||
alias super_query query | ||
attr_reader :connection | ||
|
@@ -24,15 +40,16 @@ def initialize(data_url, connection) | |
# @see https://github.com/lostisland/faraday | ||
def self.create_connection(params, url = nil) | ||
init_params = params[:initialize] && params.delete(:initialize) | ||
|
||
request = [:multi_json] | ||
request.unshift([:basic_auth, params[:basic_auth][:username], params[:basic_auth][:password]]) if params[:basic_auth] | ||
|
||
faraday_options = extract_faraday_options( | ||
params, request: request, response: [:multi_json, symbolize_keys: true, content_type: 'application/json']) | ||
conn = Faraday.new(url, init_params) do |b| | ||
b.request :basic_auth, params[:basic_auth][:username], params[:basic_auth][:password] if params[:basic_auth] | ||
b.request :multi_json | ||
# b.response :logger, ::Logger.new(STDOUT), bodies: true | ||
|
||
b.response :multi_json, symbolize_keys: true, content_type: 'application/json' | ||
# b.use Faraday::Response::RaiseError | ||
b.use Faraday::Adapter::NetHttpPersistent | ||
# b.adapter Faraday.default_adapter | ||
set_faraday_middleware b, faraday_options | ||
end | ||
conn.headers = {'Content-Type' => 'application/json', 'User-Agent' => ::Neo4j::Session.user_agent_string} | ||
conn | ||
|
@@ -58,13 +75,6 @@ def self.establish_session(root_data, connection) | |
CypherSession.new(data_url, connection) | ||
end | ||
|
||
def self.extract_basic_auth(url, params) | ||
return unless url && URI(url).userinfo | ||
params[:basic_auth] = {username: URI(url).user, password: URI(url).password} | ||
end | ||
|
||
private_class_method :extract_basic_auth | ||
|
||
def db_type | ||
:server_db | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
module Neo4j | ||
module Core | ||
class CypherSession | ||
module Adaptors | ||
module FaradayHelpers | ||
private | ||
|
||
def verify_faraday_options(faraday_options = {}, defaults = {}) | ||
faraday_options.symbolize_keys!.reverse_merge!(defaults) | ||
faraday_options[:adapter] ||= :net_http_persistent | ||
require 'typhoeus/adapters/faraday' if faraday_options[:adapter].to_sym == :typhoeus | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the user There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that is true... I was just bitten by typhoeus/typhoeus#226 (comment) which was a little hard to track down the solution, so I figured it would be a little easier to just handle that for them instead (plus it is only needed here, not in their project generally). Happy to take it out if you'd rather doc it / force folks to figure it out, but it seems like an easy win that doesn't really cost much here. in the I can imagine if there is any issues with the adapter gem(s), it requires a little work to maintain the tests, but again, it seems like an easy thing we can stay on top of here vs just letting people get bitten and have to figure it out themselves..? I'm of the understanding that only the |
||
faraday_options | ||
end | ||
|
||
def set_faraday_middleware(faraday, options = {adapter: :net_http_persistent}) | ||
adapter = options.delete(:adapter) | ||
send_all_faraday_options(faraday, options) | ||
faraday.adapter adapter | ||
end | ||
|
||
def send_all_faraday_options(faraday, options) | ||
options.each do |key, value| | ||
next unless faraday.respond_to? key | ||
if value.is_a? Array | ||
if value.none? { |arg| arg.is_a? Array } | ||
faraday.send key, *value | ||
else | ||
value.each do |args| | ||
arg_safe_send faraday, key, args | ||
end | ||
end | ||
else | ||
faraday.send key, value | ||
end | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not really sure I get why all of this is necessary. Doesn't the second argument of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't look too hard, but I took these comments to mean no since I don't see anything mentioning :response as a key, for instance. But yeah, if that's true that makes life much easier - I'll give it a try |
||
|
||
def arg_safe_send(object, msg, args) | ||
if args.is_a? Array | ||
object.send(msg, *args) | ||
else | ||
object.send(msg, args) | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
require 'neo4j/core/cypher_session/adaptors' | ||
require 'neo4j/core/cypher_session/adaptors/has_uri' | ||
require 'neo4j/core/cypher_session/adaptors/faraday_helpers' | ||
require 'neo4j/core/cypher_session/responses/http' | ||
|
||
# TODO: Work with `Query` objects | ||
|
@@ -16,7 +17,8 @@ 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_options] || @options['faraday_options'] || {}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that you should just be able to do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not unless I stringify_keys! since coders will likely use :faraday_options but when config'ed via neo4j.yml (ie in Rails), the keys are strings. There is an explicit test for this. |
||
rescue Faraday::ConnectionFailed => e | ||
raise CypherSession::ConnectionFailedError, "#{e.class}: #{e.message}" | ||
end | ||
|
@@ -91,15 +93,16 @@ def connected? | |
# - Sets headers, including user agent string | ||
class Requestor | ||
include Adaptors::HasUri | ||
include FaradayHelpers | ||
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, faraday_options) | ||
self.url = url | ||
@user = user | ||
@password = password | ||
@user_agent_string = user_agent_string | ||
@faraday = faraday_connection | ||
@faraday = faraday_connection(faraday_options) | ||
@instrument_proc = instrument_proc | ||
end | ||
|
||
|
@@ -134,22 +137,23 @@ def get(path, body = '', options = {}) | |
|
||
private | ||
|
||
def faraday_connection | ||
def faraday_connection(faraday_options = {}) | ||
verify_faraday_options(faraday_options, | ||
request: [ | ||
[:basic_auth, user, password], | ||
:multi_json | ||
], | ||
response: [:multi_json, symbolize_keys: true, content_type: 'application/json'] | ||
) | ||
require 'faraday' | ||
require 'faraday_middleware/multi_json' | ||
|
||
Faraday.new(url) do |c| | ||
c.request :basic_auth, user, password | ||
c.request :multi_json | ||
|
||
c.response :multi_json, symbolize_keys: true, content_type: 'application/json' | ||
c.use Faraday::Adapter::NetHttpPersistent | ||
|
||
conn = Faraday.new(url) do |c| | ||
# c.response :logger, ::Logger.new(STDOUT), bodies: true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just verifying, if you leave these lines in, are they no longer defaults but rather they override the options? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is my understanding - all the middleware that gets called during initialization gets mounted in Faraday and these would then always get mounted no matter if people wanted them or not, and there would be no way to get rid of them. |
||
|
||
c.headers['Content-Type'] = 'application/json' | ||
c.headers['User-Agent'] = @user_agent_string | ||
set_faraday_middleware c, faraday_options | ||
end | ||
conn.headers = {'Content-Type' => 'application/json', 'User-Agent' => @user_agent_string} | ||
conn | ||
end | ||
|
||
def request_body(body) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
RSpec.shared_examples 'Neo4j::Server::CypherSession::Adaptor' do | ||
describe 'faraday_options' do | ||
describe 'a faraday connection type adapter option' do | ||
it 'can use a user supplied faraday connection for a new session' do | ||
connection = Faraday.new do |faraday| | ||
faraday.request :basic_auth, basic_auth_hash[:username], basic_auth_hash[:password] | ||
|
||
faraday.request :multi_json | ||
faraday.response :multi_json, symbolize_keys: true, content_type: 'application/json' | ||
faraday.adapter Faraday.default_adapter | ||
end | ||
connection.headers = {'Content-Type' => 'application/json'} | ||
|
||
expect(connection).to receive(:get).at_least(:once).and_call_original | ||
create_server_session(connection: connection) | ||
end | ||
|
||
it 'will pass through a symbol key' do | ||
expect_any_instance_of(Faraday::Connection).to receive(:adapter).with(:typhoeus).and_call_original | ||
create_server_session(faraday_options: {adapter: :typhoeus}) | ||
end | ||
|
||
it 'will pass through a string key' do | ||
expect_any_instance_of(Faraday::Connection).to receive(:adapter).with(:typhoeus).and_call_original | ||
create_server_session('faraday_options' => {'adapter' => :typhoeus}) | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did these counts go up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my guess was that no one had run
rubocop --auto-gen-config
in a while (since 2016-03-28 in fact). A lot of metrics changed that were unrelated to what I did here, I just needed to update the metrics because I was getting caught byAbcSize
.Since the rest of those rules were already being muted via this file, it just updated the counts of existing violations since the previous run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, the offense count increase in fine, but the "Max" counts increasing is worrying. We try not to go backwards with our Rubocop todo values (but anything that we set in
.rubocop.yml
is our official agreed rule / max). If you're having trouble getting the code to fit in the metrics we can help (or increase the values if it really is too cumbersome)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I had kinda been hoping to not take on a refactor job as well, for two reasons:
Anyways, I just reverted the
rubocop_todo.yml
and made some changes that fixes theRubocop
issues, but to me the code looks worse. You can be the judge though.