Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

fix exists? to return false if passed nil (which may come from a missing #2485

Merged
merged 1 commit into from

4 participants

@akaspick

fixes exists? on ActiveRecord classes to return false if passed nil (which may come from a missing URL param)

Before:
Topic.exists?(nil) => true

After:
Topic.exists?(nil) => false

This is the behaviour that Rails 2.x had and should be how Rails 3.x works as well.

Please backport to Rails 3-0 stable.

@kalbasit

+1 please pull

@waseem waseem commented on the diff
...verecord/lib/active_record/relation/finder_methods.rb
@@ -180,7 +180,9 @@ module ActiveRecord
# Person.exists?(:name => "David")
# Person.exists?(['name LIKE ?', "%#{query}%"])
# Person.exists?
- def exists?(id = nil)
+ def exists?(id = false)
+ return false if id.nil?
@waseem
waseem added a note

I wonder if return false if id.blank? would be much better.

No, because false.blank? => true, which would make Topic.exists? => false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton jonleighton merged commit db8d54e into rails:master
@jonleighton
Collaborator

If you would like this to be backported to 3-1-stable, please could you submit a new pull request for that, and include in the pull request a CHANGELOG entry?

@akaspick

Opened a new PR for 3-0-stable at #2919

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 10, 2011
  1. @akaspick
This page is out of date. Refresh to see the latest.
View
4 activerecord/lib/active_record/relation/finder_methods.rb
@@ -180,7 +180,9 @@ def all(*args)
# Person.exists?(:name => "David")
# Person.exists?(['name LIKE ?', "%#{query}%"])
# Person.exists?
- def exists?(id = nil)
+ def exists?(id = false)
+ return false if id.nil?
@waseem
waseem added a note

I wonder if return false if id.blank? would be much better.

No, because false.blank? => true, which would make Topic.exists? => false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
id = id.id if ActiveRecord::Base === id
join_dependency = construct_join_dependency_for_association_find
View
9 activerecord/test/cases/finder_test.rb
@@ -48,6 +48,15 @@ def test_exists_returns_true_with_one_record_and_no_args
assert Topic.exists?
end
+ # 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
+ def test_exists_with_nil_arg
+ assert !Topic.exists?(nil)
+ assert Topic.exists?
+ assert !Topic.first.replies.exists?(nil)
+ assert Topic.first.replies.exists?
+ end
+
def test_does_not_exist_with_empty_table_and_no_args_given
Topic.delete_all
assert !Topic.exists?
Something went wrong with that request. Please try again.