Skip to content

Commit

Permalink
let AR::FinderMethods#exists? return singletons in all cases [closes #…
Browse files Browse the repository at this point in the history
…11592]

This fixes a regression. The documentation said in its introduction
paragraph that the method returns truthy/falsy, but then below it
was said that if there were no arguments you'd get `true` or `false`.
Also when the argument is exactly `false` a singleton is documented
to be returned.

The method was not returning the singletons so it didn't conform to
those special cases.

The best solution here seems to be to just return singletons in all
cases. This solution is backwards compatible. Also, the contract
has been revised because it has no sense that the predicate varies
that way depending on the input. I bet the previous contract was just
an accident, not something mixed on purpose.

Conflicts:
	activerecord/lib/active_record/relation/finder_methods.rb
	activerecord/test/cases/finder_test.rb
  • Loading branch information
fxn committed Aug 19, 2013
1 parent 2fcef66 commit 565c367
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 34 deletions.
14 changes: 7 additions & 7 deletions activerecord/lib/active_record/relation/finder_methods.rb
Expand Up @@ -171,21 +171,21 @@ def last!
last or raise RecordNotFound last or raise RecordNotFound
end end


# Returns truthy if a record exists in the table that matches the +id+ or # Returns +true+ if a record exists in the table that matches the +id+ or
# conditions given, or falsy otherwise. The argument can take six forms: # conditions given, or +false+ otherwise. The argument can take six forms:
# #
# * Integer - Finds the record with this primary key. # * Integer - Finds the record with this primary key.
# * String - Finds the record with a primary key corresponding to this # * String - Finds the record with a primary key corresponding to this
# string (such as <tt>'5'</tt>). # string (such as <tt>'5'</tt>).
# * Array - Finds the record that matches these +find+-style conditions # * Array - Finds the record that matches these +find+-style conditions
# (such as <tt>['color = ?', 'red']</tt>). # (such as <tt>['name LIKE ?', "%#{query}%"]</tt>).
# * Hash - Finds the record that matches these +find+-style conditions # * Hash - Finds the record that matches these +find+-style conditions
# (such as <tt>{color: 'red'}</tt>). # (such as <tt>{name: 'David'}</tt>).
# * +false+ - Returns always +false+. # * +false+ - Returns always +false+.
# * No args - Returns +false+ if the table is empty, +true+ otherwise. # * No args - Returns +false+ if the table is empty, +true+ otherwise.
# #
# For more information about specifying conditions as a Hash or Array, # For more information about specifying conditions as a hash or array,
# see the Conditions section in the introduction to ActiveRecord::Base. # see the Conditions section in the introduction to <tt>ActiveRecord::Base</tt>.
# #
# Note: You can't pass in a condition as a string (like <tt>name = # Note: You can't pass in a condition as a string (like <tt>name =
# 'Jamie'</tt>), since it would be sanitized and then queried against # 'Jamie'</tt>), since it would be sanitized and then queried against
Expand Down Expand Up @@ -213,7 +213,7 @@ def exists?(conditions = :none)
relation = relation.where(table[primary_key].eq(conditions)) if conditions != :none relation = relation.where(table[primary_key].eq(conditions)) if conditions != :none
end end


connection.select_value(relation.arel, "#{name} Exists", relation.bind_values) connection.select_value(relation, "#{name} Exists", relation.bind_values) ? true : false
end end


# This method is called whenever no records are found with either a single # This method is called whenever no records are found with either a single
Expand Down
56 changes: 29 additions & 27 deletions activerecord/test/cases/finder_test.rb
Expand Up @@ -45,17 +45,18 @@ def test_find_with_string
end end


def test_exists def test_exists
assert Topic.exists?(1) assert_equal true, Topic.exists?(1)
assert Topic.exists?("1") assert_equal true, Topic.exists?("1")
assert Topic.exists?(title: "The First Topic") assert_equal true, Topic.exists?(title: "The First Topic")
assert Topic.exists?(heading: "The First Topic") assert_equal true, Topic.exists?(heading: "The First Topic")
assert Topic.exists?(:author_name => "Mary", :approved => true) assert_equal true, Topic.exists?(:author_name => "Mary", :approved => true)
assert Topic.exists?(["parent_id = ?", 1]) assert_equal true, Topic.exists?(["parent_id = ?", 1])
assert !Topic.exists?(45)
assert !Topic.exists?(Topic.new) assert_equal false, Topic.exists?(45)
assert_equal false, Topic.exists?(Topic.new)


begin begin
assert !Topic.exists?("foo") assert_equal false, Topic.exists?("foo")
rescue ActiveRecord::StatementInvalid rescue ActiveRecord::StatementInvalid
# PostgreSQL complains about string comparison with integer field # PostgreSQL complains about string comparison with integer field
rescue Exception rescue Exception
Expand All @@ -72,61 +73,62 @@ def test_exists_does_not_select_columns_without_alias
end end


def test_exists_returns_true_with_one_record_and_no_args def test_exists_returns_true_with_one_record_and_no_args
assert Topic.exists? assert_equal true, Topic.exists?
end end


def test_exists_returns_false_with_false_arg def test_exists_returns_false_with_false_arg
assert !Topic.exists?(false) assert_equal false, Topic.exists?(false)
end end


# exists? should handle nil for id's that come from URLs and always return false # exists? should handle nil for id's that come from URLs and always return false
# (example: Topic.exists?(params[:id])) where params[:id] is nil # (example: Topic.exists?(params[:id])) where params[:id] is nil
def test_exists_with_nil_arg def test_exists_with_nil_arg
assert !Topic.exists?(nil) assert_equal false, Topic.exists?(nil)
assert Topic.exists? assert_equal true, Topic.exists?
assert !Topic.first.replies.exists?(nil)
assert Topic.first.replies.exists? assert_equal false, Topic.first.replies.exists?(nil)
assert_equal true, Topic.first.replies.exists?
end end


# ensures +exists?+ runs valid SQL by excluding order value # ensures +exists?+ runs valid SQL by excluding order value
def test_exists_with_order def test_exists_with_order
assert Topic.order(:id).distinct.exists? assert_equal true, Topic.order(:id).distinct.exists?
end end


def test_exists_with_includes_limit_and_empty_result def test_exists_with_includes_limit_and_empty_result
assert !Topic.includes(:replies).limit(0).exists? assert_equal false, Topic.includes(:replies).limit(0).exists?
assert !Topic.includes(:replies).limit(1).where('0 = 1').exists? assert_equal false, Topic.includes(:replies).limit(1).where('0 = 1').exists?
end end


def test_exists_with_distinct_association_includes_and_limit def test_exists_with_distinct_association_includes_and_limit
author = Author.first author = Author.first
assert !author.unique_categorized_posts.includes(:special_comments).limit(0).exists? assert_equal false, author.unique_categorized_posts.includes(:special_comments).limit(0).exists?
assert author.unique_categorized_posts.includes(:special_comments).limit(1).exists? assert_equal true, author.unique_categorized_posts.includes(:special_comments).limit(1).exists?
end end


def test_exists_with_distinct_association_includes_limit_and_order def test_exists_with_distinct_association_includes_limit_and_order
author = Author.first author = Author.first
assert !author.unique_categorized_posts.includes(:special_comments).order('comments.taggings_count DESC').limit(0).exists? assert_equal false, author.unique_categorized_posts.includes(:special_comments).order('comments.taggings_count DESC').limit(0).exists?
assert author.unique_categorized_posts.includes(:special_comments).order('comments.taggings_count DESC').limit(1).exists? assert_equal true, author.unique_categorized_posts.includes(:special_comments).order('comments.taggings_count DESC').limit(1).exists?
end end


def test_exists_with_empty_table_and_no_args_given def test_exists_with_empty_table_and_no_args_given
Topic.delete_all Topic.delete_all
assert !Topic.exists? assert_equal false, Topic.exists?
end end


def test_exists_with_aggregate_having_three_mappings def test_exists_with_aggregate_having_three_mappings
existing_address = customers(:david).address existing_address = customers(:david).address
assert Customer.exists?(:address => existing_address) assert_equal true, Customer.exists?(:address => existing_address)
end end


def test_exists_with_aggregate_having_three_mappings_with_one_difference def test_exists_with_aggregate_having_three_mappings_with_one_difference
existing_address = customers(:david).address existing_address = customers(:david).address
assert !Customer.exists?(:address => assert_equal false, Customer.exists?(:address =>
Address.new(existing_address.street, existing_address.city, existing_address.country + "1")) Address.new(existing_address.street, existing_address.city, existing_address.country + "1"))
assert !Customer.exists?(:address => assert_equal false, Customer.exists?(:address =>
Address.new(existing_address.street, existing_address.city + "1", existing_address.country)) Address.new(existing_address.street, existing_address.city + "1", existing_address.country))
assert !Customer.exists?(:address => assert_equal false, Customer.exists?(:address =>
Address.new(existing_address.street + "1", existing_address.city, existing_address.country)) Address.new(existing_address.street + "1", existing_address.city, existing_address.country))
end end


Expand Down

0 comments on commit 565c367

Please sign in to comment.