diff options
Diffstat (limited to 'activerecord')
13 files changed, 108 insertions, 18 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 6f0b5bfce6..a661a44f1f 100644 --- a/activerecord/CHANGELOG.md +++ b/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. diff --git a/activerecord/Rakefile b/activerecord/Rakefile index 4090293b56..7feb0b75a0 100755..100644 --- a/activerecord/Rakefile +++ b/activerecord/Rakefile @@ -1,4 +1,3 @@ -#!/usr/bin/env rake require 'rake/testtask' require 'rake/packagetask' require 'rubygems/package_task' diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb index ad029d1101..50d16b16a9 100644 --- a/activerecord/lib/active_record/associations/collection_proxy.rb +++ b/activerecord/lib/active_record/associations/collection_proxy.rb @@ -81,7 +81,7 @@ module ActiveRecord def method_missing(method, *args, &block) match = DynamicFinderMatch.match(method) if match && match.instantiator? - send(:find_or_instantiator_by_attributes, match, match.attribute_names, *args) do |r| + scoped.send(:find_or_instantiator_by_attributes, match, match.attribute_names, *args) do |r| proxy_association.send :set_owner_attributes, r proxy_association.send :add_to_target, r yield(r) if block_given? @@ -101,7 +101,7 @@ module ActiveRecord end else - scoped.readonly(nil).send(method, *args, &block) + scoped.readonly(nil).public_send(method, *args, &block) end end diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb index 3546873550..f0b6ae2b7d 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -408,7 +408,7 @@ module ActiveRecord # t.remove(:qualification) # t.remove(:qualification, :experience) def remove(*column_names) - @base.remove_column(@table_name, column_names) + @base.remove_column(@table_name, *column_names) end # Removes the given index from the table. diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index 30a4f9aa35..e7a4f061fd 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -618,8 +618,6 @@ module ActiveRecord end def columns_for_remove(table_name, *column_names) - column_names = column_names.flatten - raise ArgumentError.new("You must specify at least one column name. Example: remove_column(:people, :first_name)") if column_names.blank? column_names.map {|column_name| quote_column_name(column_name) } end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 273c165084..68cf495025 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -1233,7 +1233,10 @@ module ActiveRecord # 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}" } diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 83f75e3505..44e407a561 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -446,7 +446,7 @@ module ActiveRecord def remove_column(table_name, *column_names) #:nodoc: raise ArgumentError.new("You must specify at least one column name. Example: remove_column(:people, :first_name)") if column_names.empty? - column_names.flatten.each do |column_name| + column_names.each do |column_name| alter_table(table_name) do |definition| definition.columns.delete(definition[column_name]) end diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb index 29b8b2fb73..4d8283bcff 100644 --- a/activerecord/lib/active_record/querying.rb +++ b/activerecord/lib/active_record/querying.rb @@ -3,7 +3,7 @@ require 'active_support/deprecation' 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 diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 1ceb1949a4..3c9c9c4e84 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -60,6 +60,28 @@ module ActiveRecord 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 @@ module ActiveRecord # 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 @@ module ActiveRecord 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 @@ module ActiveRecord # 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index a98e5b115c..f74fe42dc2 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -1703,4 +1703,9 @@ class HasManyAssociationsTest < ActiveRecord::TestCase ensure ActiveRecord::Base.dependent_restrict_raises = option_before end + + def test_collection_association_with_private_kernel_method + firm = companies(:first_firm) + assert_equal [accounts(:signals37)], firm.accounts.open + end end diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index 29469c42ed..54801bd101 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -143,6 +143,26 @@ class FinderTest < ActiveRecord::TestCase 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 @@ class FinderTest < ActiveRecord::TestCase 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 @@ class FinderTest < ActiveRecord::TestCase 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 @@ class FinderTest < ActiveRecord::TestCase 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 diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index e71b84a3a5..f788690b0d 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -588,14 +588,14 @@ class ChangeTableMigrationsTest < ActiveRecord::TestCase def test_remove_drops_single_column with_change_table do |t| - @connection.expects(:remove_column).with(:delete_me, [:bar]) + @connection.expects(:remove_column).with(:delete_me, :bar) t.remove :bar end end def test_remove_drops_multiple_columns with_change_table do |t| - @connection.expects(:remove_column).with(:delete_me, [:bar, :baz]) + @connection.expects(:remove_column).with(:delete_me, :bar, :baz) t.remove :bar, :baz end end diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb index fbdfaa2c29..7b993d5a2c 100644 --- a/activerecord/test/models/company.rb +++ b/activerecord/test/models/company.rb @@ -198,6 +198,11 @@ class Account < ActiveRecord::Base @destroyed_account_ids ||= Hash.new { |h,k| h[k] = [] } end + # Test private kernel method through collection proxy using has_many. + def self.open + where('firm_name = ?', '37signals') + end + before_destroy do |account| if account.firm Account.destroyed_account_ids[account.firm.id] << account.id |