Make Model.find_by_* and Model.find_all_by_* use relations and remove dynamic method caching

4 files changed, 38 insertions, 124 deletions

diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index b6d73265a8..31737043eb 100755
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -671,20 +671,10 @@ module ActiveRecord #:nodoc:
         options = args.extract_options!
 
         if options.empty? && !scoped?(:find)
-          relation = arel_table
+          arel_table
         else
-          relation = construct_finder_arel(options)
-          include_associations = merge_includes(scope(:find, :include), options[:include])
-
-          if include_associations.any?
-            if references_eager_loaded_tables?(options)
-              relation.eager_load(include_associations)
-            else
-              relation.preload(include_associations)
-            end
-          end
+          construct_finder_arel_with_includes(options)
         end
-        relation
       end
 
       # Executes a custom SQL query against your database and returns all the results.  The results will
@@ -1687,6 +1677,8 @@ module ActiveRecord #:nodoc:
 
         def construct_finder_arel(options = {}, scope = scope(:find))
           # TODO add lock to Arel
+          validate_find_options(options)
+
           relation = arel_table(options[:from]).
             joins(construct_join(options[:joins], scope)).
             where(construct_conditions(options[:conditions], scope)).
@@ -1701,6 +1693,21 @@ module ActiveRecord #:nodoc:
           relation
         end
 
+        def construct_finder_arel_with_includes(options = {})
+          relation = construct_finder_arel(options)
+          include_associations = merge_includes(scope(:find, :include), options[:include])
+
+          if include_associations.any?
+            if references_eager_loaded_tables?(options)
+              relation = relation.eager_load(include_associations)
+            else
+              relation = relation.preload(include_associations)
+            end
+          end
+
+          relation
+        end
+
         def construct_finder_sql(options, scope = scope(:find))
           construct_finder_arel(options, scope).to_sql
         end
@@ -1853,48 +1860,9 @@ module ActiveRecord #:nodoc:
             attribute_names = match.attribute_names
             super unless all_attributes_exists?(attribute_names)
             if match.finder?
-              finder = match.finder
-              bang = match.bang?
-              # def self.find_by_login_and_activated(*args)
-              #   options = args.extract_options!
-              #   attributes = construct_attributes_from_arguments(
-              #     [:login,:activated],
-              #     args
-              #   )
-              #   finder_options = { :conditions => attributes }
-              #   validate_find_options(options)
-              #   set_readonly_option!(options)
-              #
-              #   if options[:conditions]
-              #     with_scope(:find => finder_options) do
-              #       find(:first, options)
-              #     end
-              #   else
-              #     find(:first, options.merge(finder_options))
-              #   end
-              # end
-              self.class_eval %{
-                def self.#{method_id}(*args)
-                  options = args.extract_options!
-                  attributes = construct_attributes_from_arguments(
-                    [:#{attribute_names.join(',:')}],
-                    args
-                  )
-                  finder_options = { :conditions => attributes }
-                  validate_find_options(options)
-                  set_readonly_option!(options)
-
-                  #{'result = ' if bang}if options[:conditions]
-                    with_scope(:find => finder_options) do
-                      find(:#{finder}, options)
-                    end
-                  else
-                    find(:#{finder}, options.merge(finder_options))
-                  end
-                  #{'result || raise(RecordNotFound, "Couldn\'t find #{name} with #{attributes.to_a.collect { |pair| pair.join(\' = \') }.join(\', \')}")' if bang}
-                end
-              }, __FILE__, __LINE__
-              send(method_id, *arguments)
+              options = arguments.extract_options!
+              relation = options.any? ? construct_finder_arel_with_includes(options) : scoped
+              relation.send :find_by_attributes, match, attribute_names, *arguments
             elsif match.instantiator?
               instantiator = match.instantiator
               # def self.find_or_create_by_user_id(*args)
diff --git a/activerecord/lib/active_record/named_scope.rb b/activerecord/lib/active_record/named_scope.rb
index 96d361093f..a6336e762a 100644
--- a/activerecord/lib/active_record/named_scope.rb
+++ b/activerecord/lib/active_record/named_scope.rb
@@ -26,23 +26,11 @@ module ActiveRecord
         if options.present?
           Scope.new(self, options, &block)
         else
-          if !scoped?(:find)
-            relation = arel_table
-            relation = relation.where(type_condition) if finder_needs_type_condition?
+          unless scoped?(:find)
+            finder_needs_type_condition? ? arel_table.where(type_condition) : arel_table
           else
-            relation = construct_finder_arel
-            include_associations = scope(:find, :include)
-
-            if include_associations.present?
-              if references_eager_loaded_tables?(options)
-                relation.eager_load(include_associations)
-              else
-                relation.preload(include_associations)
-              end
-            end
+            construct_finder_arel_with_includes
           end
-
-          relation
         end
       end
 
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index f9ca6cbbd2..bcae352cc6 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -129,7 +129,7 @@ module ActiveRecord
       self
     end
 
-    private
+    protected
 
     def method_missing(method, *args, &block)
       if @relation.respond_to?(method)
@@ -141,20 +141,24 @@ module ActiveRecord
         super unless @klass.send(:all_attributes_exists?, attributes)
 
         if match.finder?
-          conditions = attributes.inject({}) {|h, a| h[a] = args[attributes.index(a)]; h}
-          result = where(conditions).send(match.finder)
-
-          if match.bang? && result.blank?
-            raise RecordNotFound, "Couldn't find #{@klass.name} with #{conditions.to_a.collect {|p| p.join(' = ')}.join(', ')}"
-          else
-            result
-          end
+          find_by_attributes(match, attributes, *args)
         end
       else
         super
       end
     end
 
+    def find_by_attributes(match, attributes, *args)
+      conditions = attributes.inject({}) {|h, a| h[a] = args[attributes.index(a)]; h}
+      result = where(conditions).send(match.finder)
+
+      if match.bang? && result.blank?
+        raise RecordNotFound, "Couldn't find #{@klass.name} with #{conditions.to_a.collect {|p| p.join(' = ')}.join(', ')}"
+      else
+        result
+      end
+    end
+
     def create_new_relation(relation, readonly = @readonly, preload = @associations_to_preload, eager_load = @eager_load_associations)
       Relation.new(@klass, relation, readonly, preload, eager_load)
     end
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index efc850cffc..59c016d19c 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -571,21 +571,6 @@ class FinderTest < ActiveRecord::TestCase
     assert_equal(2, Entrant.count_by_sql(["SELECT COUNT(*) FROM entrants WHERE id > ?", 1]))
   end
 
-  def test_dynamic_finders_should_go_through_the_find_class_method
-    Topic.expects(:find).with(:first, :conditions => { :title => 'The First Topic!' })
-    Topic.find_by_title("The First Topic!")
-
-    Topic.expects(:find).with(:last, :conditions => { :title => 'The Last Topic!' })
-    Topic.find_last_by_title("The Last Topic!")
-
-    Topic.expects(:find).with(:all, :conditions => { :title => 'A Topic.' })
-    Topic.find_all_by_title("A Topic.")
-
-    Topic.expects(:find).with(:first, :conditions => { :title => 'Does not exist yet for sure!' }).times(2)
-    Topic.find_or_initialize_by_title('Does not exist yet for sure!')
-    Topic.find_or_create_by_title('Does not exist yet for sure!')
-  end
-
   def test_find_by_one_attribute
     assert_equal topics(:first), Topic.find_by_title("The First Topic")
     assert_nil Topic.find_by_title("The First Topic!")
@@ -596,21 +581,6 @@ class FinderTest < ActiveRecord::TestCase
     assert_raise(ActiveRecord::RecordNotFound) { Topic.find_by_title!("The First Topic!") }
   end
 
-  def test_find_by_one_attribute_caches_dynamic_finder
-    # ensure this test can run independently of order
-    class << Topic; self; end.send(:remove_method, :find_by_title) if Topic.public_methods.any? { |m| m.to_s == 'find_by_title' }
-    assert !Topic.public_methods.any? { |m| m.to_s == 'find_by_title' }
-    t = Topic.find_by_title("The First Topic")
-    assert Topic.public_methods.any? { |m| m.to_s == 'find_by_title' }
-  end
-
-  def test_dynamic_finder_returns_same_results_after_caching
-    # ensure this test can run independently of order
-    class << Topic; self; end.send(:remove_method, :find_by_title) if Topic.public_method_defined?(:find_by_title)
-    t = Topic.find_by_title("The First Topic")
-    assert_equal t, Topic.find_by_title("The First Topic") # find_by_title has been cached
-  end
-
   def test_find_by_one_attribute_with_order_option
     assert_equal accounts(:signals37), Account.find_by_credit_limit(50, :order => 'id')
     assert_equal accounts(:rails_core_account), Account.find_by_credit_limit(50, :order => 'id DESC')
@@ -654,14 +624,6 @@ class FinderTest < ActiveRecord::TestCase
     assert_equal customers(:david), found_customer
   end
 
-  def test_dynamic_finder_on_one_attribute_with_conditions_caches_method
-    # ensure this test can run independently of order
-    class << Account; self; end.send(:remove_method, :find_by_credit_limit) if Account.public_methods.any? { |m| m.to_s == 'find_by_credit_limit' }
-    assert !Account.public_methods.any? { |m| m.to_s == 'find_by_credit_limit' }
-    a = Account.find_by_credit_limit(50, :conditions => ['firm_id = ?', 6])
-    assert Account.public_methods.any? { |m| m.to_s == 'find_by_credit_limit' }
-  end
-
   def test_dynamic_finder_on_one_attribute_with_conditions_returns_same_results_after_caching
     # ensure this test can run independently of order
     class << Account; self; end.send(:remove_method, :find_by_credit_limit) if Account.public_methods.any? { |m| m.to_s == 'find_by_credit_limit' }
@@ -694,14 +656,6 @@ class FinderTest < ActiveRecord::TestCase
     assert_nil Topic.find_last_by_title("A title with no matches")
   end
 
-  def test_find_last_by_one_attribute_caches_dynamic_finder
-    # ensure this test can run independently of order
-    class << Topic; self; end.send(:remove_method, :find_last_by_title) if Topic.public_methods.any? { |m| m.to_s == 'find_last_by_title' }
-    assert !Topic.public_methods.any? { |m| m.to_s == 'find_last_by_title' }
-    t = Topic.find_last_by_title(Topic.last.title)
-    assert Topic.public_methods.any? { |m| m.to_s == 'find_last_by_title' }
-  end
-
   def test_find_last_by_invalid_method_syntax
     assert_raise(NoMethodError) { Topic.fail_to_find_last_by_title("The First Topic") }
     assert_raise(NoMethodError) { Topic.find_last_by_title?("The First Topic") }