ActiveRecord::Relation select problem #7221

Closed
wants to merge 1 commit into
from

Projects

None yet

7 participants

@kot-begemot

All klass.connection and @klass.connection are now just connection, which is delegated to klass. Tests are passing. find_by_sql and count_by_sql are AR::Relation methods.

This should fully fix #6331.

This patch will also give an opportunity to implement something like:

Post.connection(external_database).all

@kot-begemot

After this fix will be merged that will allow me to finish my gem

https://github.com/kot-begemot/undestroyable

Currently, it is made with a weird hack.

@steveklabnik
Member

This will need a rebase.

@kot-begemot

Not sure what you mean?

@steveklabnik
Member

It does not merge cleanly with current master. You'll need to rebase your changes off of it. Here's how. I'm assuming that your rails remote is called upstream:

$ git fetch upstream
$ git checkout master
$ git rebase upstream/master

You'll probably get a conflict, resolve it as you would with any other rebase.

$ git push origin master -f

Since you rebased, you have to force push.

@kot-begemot

Well, it is rebased and tested.

@steveklabnik
Member

Awesome, looks good.

@kot-begemot

May be it looks good, but i am mot really sure it will ever be merged =)
Well at least it doesn't look like it will happen any soon.

@steveklabnik
Member

These things can take time.

@frodsan frodsan commented on an outdated diff Oct 26, 2012
activerecord/lib/active_record/query_delegation.rb
@@ -0,0 +1,16 @@
+module ActiveRecord
+ module QueryDelegation
+ delegate :find, :take, :take!, :first, :first!, :last, :last!, :exists?, :any?, :many?, :to => :all
@frodsan
frodsan Oct 26, 2012

Please use 1.9 hash syntax where you have made changes or add new code. Thanks.

@frodsan frodsan commented on an outdated diff Oct 26, 2012
activerecord/lib/active_record/relation.rb
@@ -559,6 +557,25 @@ def inspect
"#<#{self.class.name} [#{entries.join(', ')}]>"
end
+ # Redirect the request to different connection.
+ #
+ # Note: +connection+ can be defined only once, hence it is a good practice to define
+ # the connection in the first place. By default, +connection+ will be extracted from
+ # the class.
+ #
+ # ==== Examples
@frodsan
frodsan Oct 26, 2012

Remove this header please. Thanks.

@kot-begemot

New commit includes requested changes.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Oct 30, 2012
activerecord/lib/active_record/relation.rb
@@ -559,6 +557,12 @@ def inspect
"#<#{self.class.name} [#{entries.join(', ')}]>"
end
+ def with_connection new_connection
@carlosantoniodasilva
carlosantoniodasilva Oct 30, 2012 Ruby on Rails member

Need () here.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Oct 30, 2012
activerecord/test/cases/relation_test.rb
@@ -176,6 +176,21 @@ def test_references_values_dont_duplicate
relation.merge!(where: ['foo = ?', 'bar'])
assert_equal ['foo = bar'], relation.where_values
end
+
+ test 'extracting connection from klass' do
+ relation = Relation.new Post, Post.arel_table
+
+ assert relation.send(:connection).eql?(Post.connection)
+ end
+
+ test 'extracting connection from klass' do
@carlosantoniodasilva
carlosantoniodasilva Oct 30, 2012 Ruby on Rails member

Both tests have the same description.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Oct 30, 2012
activerecord/lib/active_record/relation.rb
@@ -559,6 +557,12 @@ def inspect
"#<#{self.class.name} [#{entries.join(', ')}]>"
end
+ def with_connection new_connection
+ @connection ||= new_connection
+ self
+ end
+ alias_method :with_conn, :with_connection
@carlosantoniodasilva
carlosantoniodasilva Oct 30, 2012 Ruby on Rails member

Well, I don't see with_connection being used anywhere, besides, it will only override connection if it hasn't been called yet, is it that what you want, or should it override if called?

@kot-begemot
kot-begemot Oct 30, 2012

Idea is that you can query records from remote database, using different connection. Once the Relation was loaded (hence, connection is defined), connection variable should not be overwritten.

@carlosantoniodasilva
carlosantoniodasilva Oct 30, 2012 Ruby on Rails member

I think we could leave it to a separate pull request, since it's adding a new feature rather than fixing a bug (which is all the other code related to, right?)

@kot-begemot
kot-begemot Oct 30, 2012

ok, i will provide separate request for it then.

@carlosantoniodasilva

I believe a changelog entry would be necessary as well. Thanks!

@kot-begemot

How shall the changelog be provided then?

@carlosantoniodasilva

@kot-begemot you can briefly explain the issue this is fixing, and add some code example if it applies. You can also link to the issue you're closing. Check the current AR changelog for some examples.

@kot-begemot

Changelog is there now. Commit now have different description.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Oct 30, 2012
activerecord/lib/active_record/query_delegation.rb
@@ -0,0 +1,16 @@
+module ActiveRecord
+ module QueryDelegation
+ delegate :find, :take, :take!, :first, :first!, :last, :last!, :exists?, :any?, :many?, to: :all
+ delegate :first_or_create, :first_or_create!, :first_or_initialize, to: :all
+ delegate :find_or_create_by, :find_or_create_by!, :find_or_initialize_by, to: :all
+ delegate :find_by, :find_by!, to: :all
+ delegate :destroy, :destroy_all, :delete, :delete_all, :update, :update_all, to: :all
+ delegate :find_each, :find_in_batches, to: :all
+ delegate :select, :group, :order, :except, :reorder, :limit, :offset, :joins,
+ :where, :preload, :eager_load, :includes, :from, :lock, :readonly,
+ :having, :create_with, :uniq, :references, :none, to: :all
+ delegate :count, :average, :minimum, :maximum, :sum, :calculate, :pluck, :ids, to: :all
+ delegate :find_by_sql, :count_by_sql, to: :all
+ delegate :with_connection, :with_conn, to: :all
@carlosantoniodasilva
carlosantoniodasilva Oct 30, 2012 Ruby on Rails member

This is related to the other change, right?

@carlosantoniodasilva
carlosantoniodasilva Oct 30, 2012 Ruby on Rails member

Can we revert these changes that add QueryDelegation, to concentrate on the connection ones? This one can be also sent in a different pull request if you want. Thanks.

@kot-begemot
kot-begemot Oct 30, 2012

Idea was to move find_by_sql and count_by_sql methods into AR::Relation. They were in same module, that was included into AR::Base. Hence there was a need to separate them.

@kot-begemot
kot-begemot Oct 30, 2012

May be the naming is bad, but this is one of the key points of this commit.

@carlosantoniodasilva

Looks fine to me, I'll ping @jonleighton and/or @tenderlove to do a review later. Thanks!

@tenderlove tenderlove and 1 other commented on an outdated diff Oct 30, 2012
activerecord/test/cases/relation/connection_test.rb
@@ -0,0 +1,15 @@
+require "cases/helper"
+require 'models/post'
+
+module ActiveRecord
+ class ConnectionTest < ActiveRecord::TestCase
+ fixtures :posts
+
+ def test_connection_overwriting
+ posts = Post.all
+ posts.instance_variable_set :@connection, Post.connection.dup
+
+ assert_not_equal posts.send(:connection), Post.connection
@tenderlove
tenderlove Oct 30, 2012 Ruby on Rails member

What is the point of this test? I don't like it because setting the instance variable makes assumptions about how the connection is stored and retrieved from the class.

Can you tell us what the goal of this test is? Maybe we can come up with a different way to test.

@kot-begemot
kot-begemot Oct 30, 2012

The goal was to check that the once the connection was overwritten, it will not reaper. I think this is out-of-date test btw.
So, the general idea behind this commit is to provide the possibility to simply overwrite the connection for AR::Relation instance, and provide the way to communicate with remote database on a short term.

@tenderlove tenderlove and 1 other commented on an outdated diff Oct 30, 2012
activerecord/lib/active_record/relation.rb
@@ -585,6 +583,10 @@ def exec_queries
@records
end
+ def connection
+ @connection ||= klass.connection
+ end
+
@tenderlove
tenderlove Oct 30, 2012 Ruby on Rails member

Why are we caching the return value of klass.connection? The connection should be cached on klass, and double caching doesn't buy us anything except code complexity and headache.

Can you change it to just:

def connection
  klass.connection
end

Or is there a reason we're caching the connection here?

@kot-begemot
kot-begemot Oct 30, 2012

Initial idea was to provide a way to overwrite the the connection for a request.

Post.with_connection(@remote_db).all
@kot-begemot
kot-begemot Oct 31, 2012

I will changed this then, so it will cache the connection in klass, and propose this functionality with different pull request, after that one will be mearged

@frodsan frodsan commented on an outdated diff Oct 30, 2012
activerecord/CHANGELOG.md
@@ -237,6 +237,11 @@
*Ernie Miller*
+* Centralized connection for AcriveRecord::Relation.
@frodsan
frodsan Oct 30, 2012

typo => ActiveRecord

@frodsan
frodsan Oct 30, 2012

Also, new CHANGELOG entries go to the top 🆙

@kot-begemot

All the problems are fixed. Commit is rebased to up-to-date HEAD.

@carlosantoniodasilva

The problem we can see with that is that we can't guarantee that the connection usage will always be managed by the instance method, someone somewhere will add a method that uses @klass.connection.

Anyway, can you give again some explanation about what's the intent to extract this? Also, there are two different changes here to take into account, I believe the Querying extraction is a different one and should be sent separately. Thanks!

@kot-begemot

Ok, the initial reasoning was:

I am writing the gem called Undestroyable. This is a sort-delete gem that provides several ways of soft deleting records. For one strategy, I am facing the situation when I had to send the record to remote database. Plus, it should be selectable as well (otherwise there is no point for soft-delete). So the communication with remote database is required. Turns out that there is no simple way to communicate with remote database in AR. Nothing similar to Datamapper repository concept.

DataMapper.setup(:default, 'mysql://localhost/dm_core_test')
DataMapper.setup(:external, 'mysql://someother_host/dm_core_test')

Person.first 
DataMapper.repository(:external) { Person.first }

I was checking the code of AR::Relation, thought may be there is a way to implement it with simple redefinition of connection that is used. Turns out that the connection is obtained uniquely in each case, and there is no easy way to implement to communication, except for cloning the whole class, and redefine the connection for cloned one, which is weird hack.

So, I spend some time investigating the issue, and end up with this pull request. Now, connection method< can be redefined, and necessary communication with remote DB can happen through AR::Relation.

@carlosantoniodasilva

@kot-begemot alright, got it, thanks for the clarification. The main issue that I can see is that we cannot guarantee, as I said before, that anyone else will use the connection method from the instance 😄.

And now I'm thinking, can't you make use of establish_connection for that to work?

@kot-begemot

What you mean?

@carlosantoniodasilva

I mean, instead of overriding the connection method, you should be able to use establish_connection to connect to the database you need at the class level?

@kot-begemot

yes, but after that you need to send the data to that connection. There is no obvious description of how to send it.

In fact, that is what i wanted in the gem. Define the connection, and using the AR::Relation instance, gem will be able to easily communicate with remote database.

@carlosantoniodasilva

@kot-begemot I see what you mean. How about creating a relation not with the current class, but with an inherited class with an overriden connection? Something similar to:

def undest_relation
  @undest_relation ||= ::ActiveRecord::Relation.new(undest_klass, undest_arel_table)
end

def undest_klass
  @undest_klass ||= Class.new(self) do
    def self.connection; undest_connection; end
  end
end

def undest_connection
  @undest_connection ||= ::Undestroyable::Orm::ActiveRecord::Connection.get_connection(self)
end

This will ensure that klass.connection will point to your undest_connection everywhere, without the need of ensuring that in the Rails side someone possibly forgets to use the instance connection in the relation. Just an idea, not sure it'll work 100% fine, wdyt?

@kot-begemot

I have something very similar in the master branch already

https://github.com/kot-begemot/undestroyable/blob/master/lib/undestroyable/orm/active_record/database.rb

You can see i use there something like

def undest_mirror
  @undest_mirror ||= begin
    copy = self.dup
    copy.table_name = undestr_config[:full_table_name]
    def copy.name; undestr_config[:full_table_name].to_s.classify; end
      copy.establish_connection undestr_config[:connection]
      copy
    end
  end
end

Point is, i am not sure how much memory that consumes + it will have all the functionality attached to it (like methods and callbacks). So i would say there will be a lot of least code, especially in the situation once the model is big. Preferably, i want to avoid that as much as possible. On top of it, it looks quite hackish to me. That is why i wanted to have something more general and light weighted.

@carlosantoniodasilva

I believe that by using .dup, you might be spending more resources than creating a new class. A simple benchmark:

require 'benchmark/ips'

Benchmark.ips do |x|
  x.report("Class.new") {
    Class.new(Object)
  }

  x.report("Class.dup") {
    Object.dup
  }
end

Will give this output:

Calculating -------------------------------------
           Class.new     28606 i/100ms
           Class.dup      1917 i/100ms
-------------------------------------------------
           Class.new   457899.4 (±4.4%) i/s -    2288480 in   5.007871s
           Class.dup    23071.1 (±5.9%) i/s -     116937 in   5.083192s

Which shows that the Class.new approach is way faster than duping the class - that of course considering Object, you may want to try out with AR::Base.

Having the functionality attached is not a problem I think, specially when inheriting, because you will not be using anything of it, you'll just using stuff related to the database, ie connection, columns_hash, and so on.

@lukaszx0
Member

It's been a while. @kot-begemot any developments? I can offer my help if needed.

@lukaszx0
Member

@carlosantoniodasilva is it even considered to be merged?

@carlosantoniodasilva

Connection as an instance method has been deprecated in 992d87d, so I believe we're good with that for now. Closing here, please feel free to ping back if you have any more comments regarding this. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment