Skip to content

Pass **kwargs to the connection for LB defined methods [RUN ALL RSPEC] [RUN AS-IF-FOSS]

Aleksei Lipniagov requested to merge 336967-fix-kwargs-issues into master

What does this MR do?

Resolves #336967 (closed)

How to setup and validate locally (strongly suggested)

Showing the issue

Maybe there is an easier way to demonstrate the issue, but I did that:

  1. Added a new spec into the master. I didn't transfer this spec to this branch, because it would be self-documenting. Basically, you could modify an existing spec, we just need to pass kwargs into transaction:
    from spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb:116:
    end

    ### NEW TMP spec
    context 'with keyword arguments' do
      let(:replica) { double(:connection) }

      before do
        allow(session).to receive(:fallback_to_replicas_for_ambiguous_queries?).and_return(true)
        allow(session).to receive(:use_primary?).and_return(false)
        allow(replica).to receive(:select)
      end

      it 'passes them to the connection' do
        expect(proxy.load_balancer).to receive(:read).twice.and_yield(replica)
        expect(replica).to receive(:transaction).with(joinable: false).and_yield

        proxy.transaction(joinable: false) { proxy.select('true') }
      end
    end

    ### Existing code below...
    context 'session fallbacks ambiguous queries to replicas' do
      let(:replica) { double(:connection) }
  1. In lib/gitlab/database/load_balancing/connection_proxy.rb:94 I checked the arguments received:
        def read_using_load_balancer(name, args, &block)
          if current_session.use_primary? &&
             !current_session.use_replicas_for_read_queries?
            @load_balancer.read_write do |connection|
              connection.send(name, *args, &block)
            end
          else
            @load_balancer.read do |connection|
              p name
              p args
              p block
              p connection
              connection.send(name, *args, &block)
            end
  1. It was:
:transaction
[{:joinable=>false}]
#<Proc:0x00007fd9092f7aa8>
#<Double :connection>
  1. Then I checked that on Ruby 3.0, we fail to execute that on real connection, in rails c:
[3] pry(main)> name = :transaction
=> :transaction
[4] pry(main)> args = [{:joinable=>false}]
=> [{:joinable=>false}]
[6] pry(main)> block = Proc.new {}
=> #<Proc:0x00007fe7476798d8 (pry):6>
[7] pry(main)> connection = ActiveRecord::Base.connection
[8] pry(main)> connection.send(name, *args, &block)
ArgumentError: wrong number of arguments (given 1, expected 0)
from /Users/lipton/.asdf/installs/ruby/3.0.2/lib/ruby/gems/3.0.0/gems/activerecord-6.1.3.2/lib/active_record/connection_adapters/abstract/database_statements.rb:313:in `transaction'

^ ^ ^ 
an issue in Ruby3
^ ^ ^ 
----
Works fine if it would be kwargs (next two strings are the same):
----

[10] pry(main)> connection.send(name, **(args.first), &block)
=> nil
[13] pry(main)> connection.send(name, joinable: false, &block)
=> nil
  1. While on Ruby 2.7, all would be fine:
--------------------------------------------------------------------------------
 Ruby:         ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-darwin20]
 GitLab:       14.2.0-pre (742e2b297d7) EE
 GitLab Shell: 13.19.1
 PostgreSQL:   12.6
--------------------------------------------------------------------------------
Loading development environment (Rails 6.1.3.2)
Found plugin pry-shell, but could not require 'pry-shell'
cannot load such file -- pry-shell
[1] pry(main)> name = :transaction
=> :transaction
[2] pry(main)> args = [{:joinable=>false}]
=> [{:joinable=>false}]
[3] pry(main)> block = Proc.new {}
=> #<Proc:0x00007fc879026f68 (pry):3>
[4] pry(main)> connection = ActiveRecord::Base.connection; 0
=> 0
[5] pry(main)> connection.send(name, *args, &block)
=> nil
[6] pry(main)> connection.send(name, joinable: false, &block)
=> nil

Validating nothing is broken

I started the app locally with LB enabled. Any better ways to test it?

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

N/A

Related to #336967 (closed)

Edited by Matthias Käppler

Merge request reports

Loading