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.

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

Fix on Ruby 3 #283

Merged
merged 3 commits into from
Jan 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:
ruby: 3.0
- gemfile: ar52
ruby: ruby-head
continue-on-error: ${{ matrix.ruby == 3.0 || matrix.ruby == 'jruby' || matrix.ruby == 'ruby-head' || matrix.gemfile == 'ar-head' }}
continue-on-error: ${{ matrix.ruby == 'jruby' || matrix.ruby == 'ruby-head' || matrix.gemfile == 'ar-head' }}
services:
postgres:
image: postgis/postgis:12-3.1
Expand Down
34 changes: 19 additions & 15 deletions lib/makara/connection_wrapper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,11 @@ def execute(*args)

# we want to forward all private methods, since we could have kicked out from a private scenario
def method_missing(m, *args, &block)
if _makara_connection.respond_to?(m)
_makara_connection.public_send(m, *args, &block)
else # probably private method
_makara_connection.__send__(m, *args, &block)
end
_makara_connection.send(m, *args, &block)
end

ruby2_keywords :method_missing if Module.private_method_defined?(:ruby2_keywords)

def respond_to_missing?(m, include_private = false)
_makara_connection.respond_to?(m, true)
end
Expand All @@ -120,7 +118,7 @@ def respond_to_missing?(m, include_private = false)
# all extra functionality is in the format of _makara*
def _makara_decorate_connection(con)

extension = %Q{
extension = <<~RUBY
# the proxy object controlling this connection
def _makara
@_makara
Expand All @@ -144,38 +142,44 @@ def _makara_hijack
def _makara_name
#{@config[:name].inspect}
end
}
RUBY

args = RUBY_VERSION >= "3.0.0" ? "..." : "*args, &block"

# Each method the Makara::Proxy needs to hijack should be redefined in the underlying connection.
# The new definition should allow for the proxy to intercept the invocation if required.
@proxy.class.hijack_methods.each do |meth|
extension << %Q{
def #{meth}(*args, &block)
method_call = RUBY_VERSION >= "3.0.0" ? "public_send(#{meth.inspect}, ...)" : "#{meth}(*args, &block)"
Copy link
Member Author

Choose a reason for hiding this comment

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

We need public_send here because foo.bar=(...) is not valid syntax in Ruby 3.0 (even though foo.bar(...) is)


extension << <<~RUBY
def #{meth}(#{args})
_makara_hijack do |proxy|
if proxy
proxy.#{meth}(*args, &block)
proxy.#{method_call}
else
super
end
end
end
}
RUBY
end

# Control methods must always be passed to the
# Makara::Proxy control object for handling (typically
# related to ActiveRecord connection pool management)
@proxy.class.control_methods.each do |meth|
extension << %Q{
def #{meth}(*args, &block)
method_call = RUBY_VERSION >= "3.0.0" ? "public_send(#{meth.inspect}, ...)" : "#{meth}(*args=args, block)"

extension << <<~RUBY
def #{meth}(#{args})
proxy = _makara
if proxy
proxy.control.#{meth}(*args=args, block)
proxy.control.#{method_call}
else
super # Only if we are not wrapped any longer
end
end
}
RUBY
end

# extend the instance
Expand Down
33 changes: 20 additions & 13 deletions lib/makara/proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,23 @@ def hijack_method(*method_names)
self.hijack_methods |= method_names

method_names.each do |method_name|
define_method method_name do |*args, &block|
define_method(method_name) do |*args, &block|
appropriate_connection(method_name, args) do |con|
con.send(method_name, *args, &block)
end
end

ruby2_keywords method_name if Module.private_method_defined?(:ruby2_keywords)
end
end

def send_to_all(*method_names)
method_names.each do |method_name|
define_method method_name do |*args|
send_to_all method_name, *args
define_method(method_name) do |*args|
send_to_all(method_name, *args)
end

ruby2_keywords method_name if Module.private_method_defined?(:ruby2_keywords)
end
end

Expand All @@ -45,9 +49,11 @@ def control_method(*method_names)
self.control_methods |= method_names

method_names.each do |method_name|
define_method method_name do |*args, &block|
define_method(method_name) do |*args, &block|
control&.send(method_name, *args, &block)
end

ruby2_keywords method_name if Module.private_method_defined?(:ruby2_keywords)
end
end
end
Expand Down Expand Up @@ -118,20 +124,20 @@ def strategy_class_for(strategy_name)

def method_missing(m, *args, &block)
if METHOD_MISSING_SKIP.include?(m)
return super(m, *args, &block)
return super
end

any_connection do |con|
if con.respond_to?(m)
con.public_send(m, *args, &block)
elsif con.respond_to?(m, true)
con.__send__(m, *args, &block)
if con.respond_to?(m, true)
con.send(m, *args, &block)
Copy link
Contributor

Choose a reason for hiding this comment

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

Subtle regression here. We'll now call a private #foo method on the ConnectionWrapper instead of calling the public #foo method on the wrapped ConnectionAdapter. That's because using #send will dispatch the private method immediately whereas #public_send will fall back to method_missing, which is where the Makara ConnectionWrapper talks to the AR ConnectionWrapper.

Example

c.send(:select, "select 1")
*** TypeError Exception: wrong argument type String (expected Array)

c.public_send(:select, "select 1")
   (2.1ms)  select 1
#<ActiveRecord::Result:0x00007f8784b4e7f0 @columns=["1"], @rows=[[1]], @hash_rows=nil, @column_types={}>

This is the cause of #326

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this. TIL about #send not hitting #method_missing.

If you can send a PR and maybe add a spec to prevent a regression I'll get it merged ASAP.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure thing. Thanks @mlarraz.

else
super(m, *args, &block)
super
end
end
end

ruby2_keywords :method_missing if Module.private_method_defined?(:ruby2_keywords)

def respond_to_missing?(m, include_private = false)
any_connection do |con|
con._makara_connection.respond_to?(m, true)
Expand All @@ -157,15 +163,16 @@ def disconnect!

protected


def send_to_all(method_name, *args)
# replica pool must run first to allow for replica --> master failover without running operations on master twice.
handling_an_all_execution(method_name) do
@replica_pool.send_to_all method_name, *args
@master_pool.send_to_all method_name, *args
@replica_pool.send_to_all(method_name, *args)
@master_pool.send_to_all(method_name, *args)
end
end

ruby2_keywords :send_to_all if Module.private_method_defined?(:ruby2_keywords)

def any_connection
if @master_pool.disabled
@replica_pool.provide do |con|
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,12 @@
Test::User.exists? # flush other (schema) things that need to happen

con = connection.replica_pool.connections.first
expect(con).to receive(:exec_query).with(/SELECT\s+1\s*(AS one)?\s+FROM .?users.?\s+LIMIT\s+.?1/, any_args).once.and_call_original
expect(con).to receive(:exec_query) do |query|
expect(query).to match(/SELECT\s+1\s*(AS one)?\s+FROM .?users.?\s+LIMIT\s+.?1/)
end.once.
# and_call_original # Switch back to this once https://github.com/rspec/rspec-mocks/pull/1385 is released
and_wrap_original { |m, *args| m.call(*args.first(3)) }

Test::User.exists?
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,13 @@
Test::User.exists? # flush other (schema) things that need to happen

con = connection.replica_pool.connections.first
expect(con).to receive(:exec_query).with(/SELECT\s+1\s*(AS one)?\s+FROM .?users.?\s+LIMIT\s+.?1/, any_args).once.and_call_original

expect(con).to receive(:exec_query) do |query|

Choose a reason for hiding this comment

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

Seems like this change is causing a failure for this matrix run: https://github.com/instacart/makara/pull/283/checks?check_run_id=1730065278

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this spec flaps on jruby for some reason. It's the same on master now.

Copy link
Member Author

Choose a reason for hiding this comment

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

expect(query).to match(/SELECT\s+1\s*(AS one)?\s+FROM .?users.?\s+LIMIT\s+.?1/)
end.once.
# and_call_original # Switch back to this once https://github.com/rspec/rspec-mocks/pull/1385 is released
and_wrap_original { |m, *args| m.call(*args.first(3)) }

Test::User.exists?
end

Expand Down
11 changes: 11 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,17 @@
rescue LoadError
end

if RUBY_VERSION >= "2.7.0"
Warning[:deprecated] = true
end

# Delete once Timecop fixes Ruby 3.1 support
Time.class_eval do
class << self
ruby2_keywords :new if Module.private_method_defined?(:ruby2_keywords)
end
end

RSpec.configure do |config|
config.run_all_when_everything_filtered = true
config.filter_run :focus
Expand Down