From 16a476e4f8f802774ae7c8dca2e59f4e672dc591 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Fri, 23 Oct 2015 14:59:31 -0500 Subject: Deprecate passing `offset` to `find_nth` All uses of the `offset` are passing `offset_index`. Better to push down the `offset` consideration into `find_nth`. This also works toward enabling `find_nth_with_limit` to take advantage of the `loaded?` state of the relation. --- .../lib/active_record/relation/finder_methods.rb | 24 ++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 19244bcf95..4a0867289e 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -119,7 +119,7 @@ module ActiveRecord if limit find_nth_with_limit(offset_index, limit) else - find_nth(0, offset_index) + find_nth 0 end end @@ -169,7 +169,7 @@ module ActiveRecord # Person.offset(3).second # returns the second object from OFFSET 3 (which is OFFSET 4) # Person.where(["user_name = :u", { u: user_name }]).second def second - find_nth(1, offset_index) + find_nth 1 end # Same as #second but raises ActiveRecord::RecordNotFound if no record @@ -185,7 +185,7 @@ module ActiveRecord # Person.offset(3).third # returns the third object from OFFSET 3 (which is OFFSET 5) # Person.where(["user_name = :u", { u: user_name }]).third def third - find_nth(2, offset_index) + find_nth 2 end # Same as #third but raises ActiveRecord::RecordNotFound if no record @@ -201,7 +201,7 @@ module ActiveRecord # Person.offset(3).fourth # returns the fourth object from OFFSET 3 (which is OFFSET 6) # Person.where(["user_name = :u", { u: user_name }]).fourth def fourth - find_nth(3, offset_index) + find_nth 3 end # Same as #fourth but raises ActiveRecord::RecordNotFound if no record @@ -217,7 +217,7 @@ module ActiveRecord # Person.offset(3).fifth # returns the fifth object from OFFSET 3 (which is OFFSET 7) # Person.where(["user_name = :u", { u: user_name }]).fifth def fifth - find_nth(4, offset_index) + find_nth 4 end # Same as #fifth but raises ActiveRecord::RecordNotFound if no record @@ -233,7 +233,7 @@ module ActiveRecord # Person.offset(3).forty_two # returns the forty-second object from OFFSET 3 (which is OFFSET 44) # Person.where(["user_name = :u", { u: user_name }]).forty_two def forty_two - find_nth(41, offset_index) + find_nth 41 end # Same as #forty_two but raises ActiveRecord::RecordNotFound if no record @@ -488,17 +488,25 @@ module ActiveRecord end end - def find_nth(index, offset) + def find_nth(index, offset = nil) if loaded? @records[index] else + if offset + ActiveSupport::Deprecation.warn(<<-MSG.squish) + Passing an offset argument to find_nth is deprecated, + please use Relation#offset instead. + MSG + else + offset = offset_index + end offset += index @offsets[offset] ||= find_nth_with_limit(offset, 1).first end end def find_nth!(index) - find_nth(index, offset_index) or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql(@klass.arel_engine)}]") + find_nth(index) or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql(@klass.arel_engine)}]") end def find_nth_with_limit(offset, limit) -- cgit v1.2.3 From b42c3255bf22e54f459751d5370e8befc33e84ea Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Fri, 23 Oct 2015 16:13:36 -0500 Subject: Fix `first(limit)` to take advantage of `loaded?` records if available I realized that `first(2)`, etc. was unnecessarily querying for the records when they were already preloaded. This was because `find_nth_with_limit` can not know which `@records` to return because it conflates the `offset` and `index` into a single variable, while the `@records` only needs the `index` itself to select the proper record. Because `find_nth` and `find_nth_with_limit` are public methods, I instead introduced a private method `find_nth_with_limit_and_offset` which is called internally and handles the `loaded?` checking. Once the `offset` argument is removed from `find_nth`, `find_nth_with_limit_and_offset` can be collapsed into `find_nth_with_limit`, with `offset` always equal to `offset_index`. --- .../lib/active_record/relation/finder_methods.rb | 25 +++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 4a0867289e..3cbb12a09d 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -117,7 +117,7 @@ module ActiveRecord # def first(limit = nil) if limit - find_nth_with_limit(offset_index, limit) + find_nth_with_limit_and_offset(0, limit, offset: offset_index) else find_nth 0 end @@ -492,6 +492,9 @@ module ActiveRecord if loaded? @records[index] else + # TODO: once the offset argument is removed we rely on offset_index + # within find_nth_with_limit, rather than pass it in via + # find_nth_with_limit_and_offset if offset ActiveSupport::Deprecation.warn(<<-MSG.squish) Passing an offset argument to find_nth is deprecated, @@ -500,8 +503,7 @@ module ActiveRecord else offset = offset_index end - offset += index - @offsets[offset] ||= find_nth_with_limit(offset, 1).first + @offsets[offset + index] ||= find_nth_with_limit_and_offset(index, 1, offset: offset).first end end @@ -509,14 +511,16 @@ module ActiveRecord find_nth(index) or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql(@klass.arel_engine)}]") end - def find_nth_with_limit(offset, limit) + def find_nth_with_limit(index, limit) + # TODO: once the offset argument is removed from find_nth, + # find_nth_with_limit_and_offset can be merged into this method relation = if order_values.empty? && primary_key order(arel_table[primary_key].asc) else self end - relation = relation.offset(offset) unless offset.zero? + relation = relation.offset(index) unless index.zero? relation.limit(limit).to_a end @@ -532,5 +536,16 @@ module ActiveRecord end end end + + private + + def find_nth_with_limit_and_offset(index, limit, offset:) # :nodoc: + if loaded? + @records[index, limit] + else + index += offset + find_nth_with_limit(index, limit) + end + end end end -- cgit v1.2.3