Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Merge pull request #5153 from mhfs/first_finder_fix

Made `first` finder consistent among database engines by adding a default order
  • Loading branch information...
commit 260104243b74546e6d0b9b161d37039782988083 2 parents 5603050 + 878db1f
@tenderlove tenderlove authored
View
6 activerecord/CHANGELOG.md
@@ -1,5 +1,11 @@
## Rails 4.0.0 (unreleased) ##
+* Added default order to `first` to assure consistent results among
+ diferent database engines. Introduced `take` as a replacement to
+ the old behavior of `first`.
+
+ *Marcelo Silveira*
+
* Added an :index option to automatically create indexes for references
and belongs_to statements in migrations.
View
5 activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -1233,7 +1233,10 @@ def distinct(columns, orders) #:nodoc:
# Construct a clean list of column names from the ORDER BY clause, removing
# any ASC/DESC modifiers
- order_columns = orders.collect { |s| s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '') }
+ order_columns = orders.collect do |s|
+ s = s.to_sql unless s.is_a?(String)
+ s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '')
+ end
order_columns.delete_if { |c| c.blank? }
order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
View
2  activerecord/lib/active_record/querying.rb
@@ -3,7 +3,7 @@
module ActiveRecord
module Querying
- delegate :find, :first, :first!, :last, :last!, :all, :exists?, :any?, :many?, :to => :scoped
+ delegate :find, :take, :take!, :first, :first!, :last, :last!, :all, :exists?, :any?, :many?, :to => :scoped
delegate :first_or_create, :first_or_create!, :first_or_initialize, :to => :scoped
delegate :find_by, :find_by!, :to => :scoped
delegate :destroy, :destroy_all, :delete, :delete_all, :update, :update_all, :to => :scoped
View
56 activerecord/lib/active_record/relation/finder_methods.rb
@@ -60,6 +60,28 @@ def find_by!(*args)
where(*args).first!
end
+ # Gives a record (or N records if a parameter is supplied) without any implied
+ # order. The order will depend on the database implementation.
+ # If an order is supplied it will be respected.
+ #
+ # Examples:
+ #
+ # Person.take # returns an object fetched by SELECT * FROM people
+ # Person.take(5) # returns 5 objects fetched by SELECT * FROM people LIMIT 5
+ # Person.where(["name LIKE '%?'", name]).take
+ def take(limit = nil)
+ limit ? limit(limit).to_a : find_take
+ end
+
+ # Same as +take+ but raises <tt>ActiveRecord::RecordNotFound</tt> if no record
+ # is found. Note that <tt>take!</tt> accepts no arguments.
+ def take!
+ take or raise RecordNotFound
+ end
+
+ # Find the first record (or first N records if a parameter is supplied).
+ # If no order is defined it will order by primary key.
+ #
# Examples:
#
# Person.first # returns the first object fetched by SELECT * FROM people
@@ -67,7 +89,15 @@ def find_by!(*args)
# Person.where(["user_name = :u", { :u => user_name }]).first
# Person.order("created_on DESC").offset(5).first
def first(limit = nil)
- limit ? limit(limit).to_a : find_first
+ if limit
+ if order_values.empty? && primary_key
+ order(arel_table[primary_key].asc).limit(limit).to_a
+ else
+ limit(limit).to_a
+ end
+ else
+ find_first
+ end
end
# Same as +first+ but raises <tt>ActiveRecord::RecordNotFound</tt> if no record
@@ -76,6 +106,9 @@ def first!
first or raise RecordNotFound
end
+ # Find the last record (or last N records if a parameter is supplied).
+ # If no order is defined it will order by primary key.
+ #
# Examples:
#
# Person.last # returns the last object fetched by SELECT * FROM people
@@ -83,8 +116,8 @@ def first!
# Person.order("created_on DESC").offset(5).last
def last(limit = nil)
if limit
- if order_values.empty?
- order("#{primary_key} DESC").limit(limit).reverse
+ if order_values.empty? && primary_key
+ order(arel_table[primary_key].desc).limit(limit).reverse
else
to_a.last(limit)
end
@@ -315,11 +348,24 @@ def find_some(ids)
end
end
+ def find_take
+ if loaded?
+ @records.take(1).first
+ else
+ @take ||= limit(1).to_a.first
+ end
+ end
+
def find_first
if loaded?
@records.first
else
- @first ||= limit(1).to_a[0]
+ @first ||=
+ if order_values.empty? && primary_key
+ order(arel_table[primary_key].asc).limit(1).to_a.first
+ else
+ limit(1).to_a.first
+ end
end
end
@@ -331,7 +377,7 @@ def find_last
if offset_value || limit_value
to_a.last
else
- reverse_order.limit(1).to_a[0]
+ reverse_order.limit(1).to_a.first
end
end
end
View
32 activerecord/test/cases/finder_test.rb
@@ -143,6 +143,26 @@ def test_find_by_sql_with_sti_on_joined_table
assert_equal [Account], accounts.collect(&:class).uniq
end
+ def test_take
+ assert_equal topics(:first), Topic.take
+ end
+
+ def test_take_failing
+ assert_nil Topic.where("title = 'This title does not exist'").take
+ end
+
+ def test_take_bang_present
+ assert_nothing_raised do
+ assert_equal topics(:second), Topic.where("title = 'The Second Topic of the day'").take!
+ end
+ end
+
+ def test_take_bang_missing
+ assert_raises ActiveRecord::RecordNotFound do
+ Topic.where("title = 'This title does not exist'").take!
+ end
+ end
+
def test_first
assert_equal topics(:second).title, Topic.where("title = 'The Second Topic of the day'").first.title
end
@@ -163,6 +183,12 @@ def test_first_bang_missing
end
end
+ def test_first_have_primary_key_order_by_default
+ expected = topics(:first)
+ expected.touch # PostgreSQL changes the default order if no order clause is used
+ assert_equal expected, Topic.first
+ end
+
def test_model_class_responds_to_first_bang
assert Topic.first!
Topic.delete_all
@@ -191,7 +217,8 @@ def test_model_class_responds_to_last_bang
end
end
- def test_first_and_last_with_integer_should_use_sql_limit
+ def test_take_and_first_and_last_with_integer_should_use_sql_limit
+ assert_sql(/LIMIT 3|ROWNUM <= 3/) { Topic.take(3).entries }
assert_sql(/LIMIT 2|ROWNUM <= 2/) { Topic.first(2).entries }
assert_sql(/LIMIT 5|ROWNUM <= 5/) { Topic.last(5).entries }
end
@@ -212,7 +239,8 @@ def test_last_with_integer_and_reorder_should_not_use_sql_limit
assert_no_match(/LIMIT/, query.first)
end
- def test_first_and_last_with_integer_should_return_an_array
+ def test_take_and_first_and_last_with_integer_should_return_an_array
+ assert_kind_of Array, Topic.take(5)
assert_kind_of Array, Topic.first(5)
assert_kind_of Array, Topic.last(5)
end

0 comments on commit 2601042

Please sign in to comment.
Something went wrong with that request. Please try again.