From c367ecdae6f80a12bc5921f4f7bc0ac6c5b23b98 Mon Sep 17 00:00:00 2001 From: Ryan Bates Date: Mon, 19 May 2008 10:04:41 -0700 Subject: [PATCH] calling AssociationProxy#first now uses SQL when possible, also fixing a couple broken tests which relied on .first to load the association target --- .../associations/association_collection.rb | 6 ++++++ .../has_and_belongs_to_many_associations_test.rb | 2 ++ .../associations/has_many_associations_test.rb | 8 ++++++++ .../test/cases/associations/join_model_test.rb | 4 +++- activerecord/test/cases/associations_test.rb | 4 ++-- 5 files changed, 21 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 7e47bf7..8f583f5 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -48,6 +48,12 @@ module ActiveRecord end end + # fetch first using SQL if target hasn't been loaded yet + def first(*args) + load_target if @owner.new_record? || @reflection.options[:finder_sql] + loaded? ? @target.first : find(:first, *args) + end + def to_ary load_target @target.to_ary diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb index 6456514..119df8a 100644 --- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb @@ -401,6 +401,8 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase def test_include_uses_array_include_after_loaded project = projects(:active_record) + project.developers.class # force load target + developer = project.developers.first assert_no_queries do diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 9e26e2a..3165b79 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -818,6 +818,8 @@ class HasManyAssociationsTest < ActiveRecord::TestCase def test_include_uses_array_include_after_loaded firm = companies(:first_firm) + firm.clients.class # force load target + client = firm.clients.first assert_no_queries do @@ -856,5 +858,11 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert ! firm.clients.loaded? assert ! firm.clients.include?(client) end + + def test_calling_first_on_association_should_not_load_entire_association + firm = companies(:first_firm) + firm.clients.first + assert ! firm.clients.loaded? + end end diff --git a/activerecord/test/cases/associations/join_model_test.rb b/activerecord/test/cases/associations/join_model_test.rb index 952ea63..60d6e13 100644 --- a/activerecord/test/cases/associations/join_model_test.rb +++ b/activerecord/test/cases/associations/join_model_test.rb @@ -664,8 +664,10 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase def test_has_many_through_include_uses_array_include_after_loaded david = authors(:david) + david.categories.class # force load target + category = david.categories.first - + assert_no_queries do assert david.categories.loaded? assert david.categories.include?(category) diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb index d8fe98b..3ad8c60 100755 --- a/activerecord/test/cases/associations_test.rb +++ b/activerecord/test/cases/associations_test.rb @@ -99,12 +99,12 @@ class AssociationProxyTest < ActiveRecord::TestCase david = authors(:david) assert_equal david, david.posts.proxy_owner assert_equal david.class.reflect_on_association(:posts), david.posts.proxy_reflection - david.posts.first # force load target + david.posts.class # force load target assert_equal david.posts, david.posts.proxy_target assert_equal david, david.posts_with_extension.testing_proxy_owner assert_equal david.class.reflect_on_association(:posts_with_extension), david.posts_with_extension.testing_proxy_reflection - david.posts_with_extension.first # force load target + david.posts_with_extension.class # force load target assert_equal david.posts_with_extension, david.posts_with_extension.testing_proxy_target end -- 1.5.4 From 91b9aa87cc389f9b8d5b09320422a7ded658a33f Mon Sep 17 00:00:00 2001 From: Ryan Bates Date: Mon, 19 May 2008 10:28:15 -0700 Subject: [PATCH] adding AssociationProxy#last method so it fetches using SQL when possible --- .../associations/association_collection.rb | 6 ++++ .../associations/has_many_associations_test.rb | 28 +++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 8f583f5..869c178 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -54,6 +54,12 @@ module ActiveRecord loaded? ? @target.first : find(:first, *args) end + # fetch last using SQL if target hasn't been loaded yet + def last(*args) + load_target if @owner.new_record? || @reflection.options[:finder_sql] || !@target.blank? + loaded? ? @target.last : find(:last, *args) + end + def to_ary load_target @target.to_ary diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 3165b79..2e6b217 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -864,5 +864,31 @@ class HasManyAssociationsTest < ActiveRecord::TestCase firm.clients.first assert ! firm.clients.loaded? end - + + def test_calling_first_on_loaded_association_should_not_fetch_with_query + firm = companies(:first_firm) + firm.clients.class # force load target + + assert_no_queries do + assert firm.clients.loaded? + firm.clients.first + end + end + + def test_calling_last_on_association_should_not_load_entire_association + firm = companies(:first_firm) + firm.clients.last + assert ! firm.clients.loaded? + end + + def test_calling_last_on_loaded_association_should_not_fetch_with_query + firm = companies(:first_firm) + firm.clients.class # force load target + + assert_no_queries do + assert firm.clients.loaded? + firm.clients.last + end + end + end -- 1.5.4 From 24c7867c21eb46e6e298dd0863b1c4203b8cb2a7 Mon Sep 17 00:00:00 2001 From: Ryan Bates Date: Mon, 19 May 2008 10:30:08 -0700 Subject: [PATCH] adding AssociationProxy#last method so it fetches using SQL when possible --- .../associations/has_many_associations_test.rb | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-) diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 2e6b217..08ec6a1 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -891,4 +891,15 @@ class HasManyAssociationsTest < ActiveRecord::TestCase end end + def test_calling_first_or_last_with_options_on_loaded_association_should_fetch_with_query + firm = companies(:first_firm) + firm.clients.class # force load target + + assert_queries 2 do + assert firm.clients.loaded? + firm.clients.first(:order => 'name') + firm.clients.last(:order => 'name') + end + end + end -- 1.5.4 From eb3088c417944ed337feca2b52f73d91fd4e6456 Mon Sep 17 00:00:00 2001 From: Ryan Bates Date: Mon, 19 May 2008 11:09:50 -0700 Subject: [PATCH] fixing AssociationProxy#first and #last so it really works --- .../associations/association_collection.rb | 26 ++++++++--- .../associations/has_many_associations_test.rb | 45 ++++++++++++++----- 2 files changed, 52 insertions(+), 19 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 869c178..108688e 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -48,16 +48,24 @@ module ActiveRecord end end - # fetch first using SQL if target hasn't been loaded yet + # fetch first using SQL if possible def first(*args) - load_target if @owner.new_record? || @reflection.options[:finder_sql] - loaded? ? @target.first : find(:first, *args) + if fetch_first_or_last_using_find? args + find(:first, *args) + else + load_target unless loaded? + @target.first(*args) + end end - # fetch last using SQL if target hasn't been loaded yet + # fetch last using SQL if possible def last(*args) - load_target if @owner.new_record? || @reflection.options[:finder_sql] || !@target.blank? - loaded? ? @target.last : find(:last, *args) + if fetch_first_or_last_using_find? args + find(:last, *args) + else + load_target unless loaded? + @target.last(*args) + end end def to_ary @@ -342,7 +350,11 @@ module ActiveRecord raise ActiveRecord::RecordNotSaved, "You cannot call create unless the parent is saved" end end - + + def fetch_first_or_last_using_find?(args) + args.first.kind_of?(Hash) || !(loaded? || @owner.new_record? || @reflection.options[:finder_sql] || !@target.blank? || args.first.kind_of?(Numeric)) + end + end end end diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 08ec6a1..de26327 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -859,39 +859,49 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert ! firm.clients.include?(client) end - def test_calling_first_on_association_should_not_load_entire_association + def test_calling_first_or_last_on_association_should_not_load_association firm = companies(:first_firm) firm.clients.first - assert ! firm.clients.loaded? + firm.clients.last + assert !firm.clients.loaded? end - def test_calling_first_on_loaded_association_should_not_fetch_with_query + def test_calling_first_or_last_on_loaded_association_should_not_fetch_with_query firm = companies(:first_firm) firm.clients.class # force load target + assert firm.clients.loaded? assert_no_queries do - assert firm.clients.loaded? firm.clients.first + assert_equal 2, firm.clients.first(2).size + firm.clients.last + assert_equal 2, firm.clients.last(2).size end end - def test_calling_last_on_association_should_not_load_entire_association + def test_calling_first_or_last_on_existing_record_with_build_should_load_association firm = companies(:first_firm) - firm.clients.last - assert ! firm.clients.loaded? + firm.clients.build(:name => 'Foo') + assert !firm.clients.loaded? + + assert_queries 1 do + firm.clients.first + firm.clients.last + end + + assert firm.clients.loaded? end - def test_calling_last_on_loaded_association_should_not_fetch_with_query - firm = companies(:first_firm) - firm.clients.class # force load target + def test_calling_first_or_last_on_new_record_should_not_run_queries + firm = Firm.new assert_no_queries do - assert firm.clients.loaded? + firm.clients.first firm.clients.last end end - def test_calling_first_or_last_with_options_on_loaded_association_should_fetch_with_query + def test_calling_first_or_last_with_find_options_on_loaded_association_should_fetch_with_query firm = companies(:first_firm) firm.clients.class # force load target @@ -902,4 +912,15 @@ class HasManyAssociationsTest < ActiveRecord::TestCase end end + def test_calling_first_or_last_with_integer_on_association_should_load_association + firm = companies(:first_firm) + + assert_queries 1 do + firm.clients.first(2) + firm.clients.last(2) + end + + assert firm.clients.loaded? + end + end -- 1.5.4 From f957b313891dca8e22a2610e55deef3f3858040b Mon Sep 17 00:00:00 2001 From: Ryan Bates Date: Mon, 19 May 2008 13:44:48 -0700 Subject: [PATCH] adding named_scope first/last methods --- .../associations/association_collection.rb | 2 +- activerecord/lib/active_record/named_scope.rb | 18 ++++++++++++- activerecord/test/cases/named_scope_test.rb | 28 ++++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 108688e..14d226e 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -352,7 +352,7 @@ module ActiveRecord end def fetch_first_or_last_using_find?(args) - args.first.kind_of?(Hash) || !(loaded? || @owner.new_record? || @reflection.options[:finder_sql] || !@target.blank? || args.first.kind_of?(Numeric)) + args.first.kind_of?(Hash) || !(loaded? || @owner.new_record? || @reflection.options[:finder_sql] || !@target.blank? || args.first.kind_of?(Integer)) end end diff --git a/activerecord/lib/active_record/named_scope.rb b/activerecord/lib/active_record/named_scope.rb index d43ebef..db52966 100644 --- a/activerecord/lib/active_record/named_scope.rb +++ b/activerecord/lib/active_record/named_scope.rb @@ -102,7 +102,7 @@ module ActiveRecord class Scope attr_reader :proxy_scope, :proxy_options - [].methods.each { |m| delegate m, :to => :proxy_found unless m =~ /(^__|^nil\?|^send|class|extend|find|count|sum|average|maximum|minimum|paginate)/ } + [].methods.each { |m| delegate m, :to => :proxy_found unless m =~ /(^__|^nil\?|^send|class|extend|find|count|sum|average|maximum|minimum|paginate|first|last)/ } delegate :scopes, :with_scope, :to => :proxy_scope def initialize(proxy_scope, options, &block) @@ -114,6 +114,22 @@ module ActiveRecord def reload load_found; self end + + def first(*args) + if args.first.kind_of?(Integer) || (@found && !args.first.kind_of?(Hash)) + proxy_found.first(*args) + else + find(:first, *args) + end + end + + def last(*args) + if args.first.kind_of?(Integer) || (@found && !args.first.kind_of?(Hash)) + proxy_found.last(*args) + else + find(:last, *args) + end + end protected def proxy_found diff --git a/activerecord/test/cases/named_scope_test.rb b/activerecord/test/cases/named_scope_test.rb index 9730f93..b833f33 100644 --- a/activerecord/test/cases/named_scope_test.rb +++ b/activerecord/test/cases/named_scope_test.rb @@ -117,5 +117,33 @@ class NamedScopeTest < ActiveRecord::TestCase expected_proxy_options = { :conditions => { :approved => true } } assert_equal expected_proxy_options, Topic.approved.proxy_options end + + def test_first_and_last_should_support_find_options + assert_equal Topic.base.first(:order => 'title'), Topic.base.find(:first, :order => 'title') + assert_equal Topic.base.last(:order => 'title'), Topic.base.find(:last, :order => 'title') + end + + def test_first_and_last_should_allow_integers_for_limit + assert_equal Topic.base.first(2), Topic.base.to_a.first(2) + assert_equal Topic.base.last(2), Topic.base.to_a.last(2) + end + + def test_first_and_last_should_not_use_query_when_results_are_loaded + topics = Topic.base + topics.reload # force load + assert_no_queries do + topics.first + topics.last + end + end + + def test_first_and_last_find_options_should_use_query_when_results_are_loaded + topics = Topic.base + topics.reload # force load + assert_queries(2) do + topics.first(:order => 'title') + topics.last(:order => 'title') + end + end end -- 1.5.4