From 16a476e4f8f802774ae7c8dca2e59f4e672dc591 Mon Sep 17 00:00:00 2001
From: Ben Woosley <ben.woosley@gmail.com>
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.
---
activerecord/CHANGELOG.md | 5 +++++
.../lib/active_record/relation/finder_methods.rb | 24 ++++++++++++++--------
2 files changed, 21 insertions(+), 8 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 2551841aaf..5daec6fb34 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+* Deprecate sending the `offset` argument to `find_nth`. Please use the
+ `offset` method on relation instead.
+
+ *Ben Woosley*
+
## Rails 5.0.0.beta1 (December 18, 2015) ##
* Order the result of `find(ids)` to match the passed array, if the relation
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 <ben.woosley@gmail.com>
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`.
---
activerecord/CHANGELOG.md | 5 +++++
.../lib/active_record/relation/finder_methods.rb | 25 +++++++++++++++++-----
activerecord/test/cases/relations_test.rb | 17 ++++++++++++---
3 files changed, 39 insertions(+), 8 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 5daec6fb34..9144ab6695 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+* When calling `first` with a `limit` argument, return directly from the
+ `loaded?` records if available.
+
+ *Ben Woosley*
+
* Deprecate sending the `offset` argument to `find_nth`. Please use the
`offset` method on relation instead.
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
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 7149c7d072..df0c2f1f92 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -111,10 +111,21 @@ class RelationTest < ActiveRecord::TestCase
def test_loaded_first
topics = Topic.all.order('id ASC')
+ topics.to_a # force load
- assert_queries(1) do
- topics.to_a # force load
- 2.times { assert_equal "The First Topic", topics.first.title }
+ assert_no_queries do
+ assert_equal "The First Topic", topics.first.title
+ end
+
+ assert topics.loaded?
+ end
+
+ def test_loaded_first_with_limit
+ topics = Topic.all.order('id ASC')
+ topics.to_a # force load
+
+ assert_no_queries do
+ assert_equal "The First Topic", topics.first(2).first.title
end
assert topics.loaded?
--
cgit v1.2.3