Skip to content

Commit

Permalink
All of queries should return correct result even if including large n…
Browse files Browse the repository at this point in the history
…umber

Currently several queries cannot return correct result due to incorrect
`RangeError` handling.

First example:

```ruby
assert_equal true, Topic.where(id: [1, 9223372036854775808]).exists?
assert_equal true, Topic.where.not(id: 9223372036854775808).exists?
```

The first example is obviously to be true, but currently it returns
false.

Second example:

```ruby
assert_equal topics(:first), Topic.where(id: 1..9223372036854775808).find(1)
```

The second example also should return the object, but currently it
raises `RecordNotFound`.

It can be seen from the examples, the queries including large number
assuming empty result is not always correct.

Therefore, This change handles `RangeError` to generate executable SQL
instead of raising `RangeError` to users to always return correct
result. By this change, it is no longer raised `RangeError` to users.
  • Loading branch information
kamipo committed Jan 18, 2019
1 parent 5b6daff commit 31ffbf8
Show file tree
Hide file tree
Showing 7 changed files with 53 additions and 18 deletions.
10 changes: 0 additions & 10 deletions activerecord/lib/active_record/relation/finder_methods.rb
Expand Up @@ -79,17 +79,12 @@ def find(*args)
# Post.find_by "published_at < ?", 2.weeks.ago
def find_by(arg, *args)
where(arg, *args).take
rescue ::RangeError
nil
end

# Like #find_by, except that if no record is found, raises
# an ActiveRecord::RecordNotFound error.
def find_by!(arg, *args)
where(arg, *args).take!
rescue ::RangeError
raise RecordNotFound.new("Couldn't find #{@klass.name} with an out of range value",
@klass.name, @klass.primary_key)
end

# Gives a record (or N records if a parameter is supplied) without any implied
Expand Down Expand Up @@ -322,8 +317,6 @@ def exists?(conditions = :none)
relation = construct_relation_for_exists(conditions)

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

# This method is called whenever no records are found with either a single
Expand Down Expand Up @@ -434,9 +427,6 @@ def find_with_ids(*ids)
else
find_some(ids)
end
rescue ::RangeError
error_message = "Couldn't find #{model_name} with an out of range ID"
raise RecordNotFound.new(error_message, model_name, primary_key, ids)
end

def find_one(id)
Expand Down
24 changes: 18 additions & 6 deletions activerecord/lib/arel/predications.rb
Expand Up @@ -35,15 +35,17 @@ def eq_all(others)
end

def between(other)
if infinity?(other.begin)
if other.end.nil? || infinity?(other.end)
if unboundable?(other.begin) == 1 || unboundable?(other.end) == -1
self.in([])
elsif open_ended?(other.begin)
if other.end.nil? || open_ended?(other.end)
not_in([])
elsif other.exclude_end?
lt(other.end)
else
lteq(other.end)
end
elsif other.end.nil? || infinity?(other.end)
elsif other.end.nil? || open_ended?(other.end)
gteq(other.begin)
elsif other.exclude_end?
gteq(other.begin).and(lt(other.end))
Expand Down Expand Up @@ -81,15 +83,17 @@ def in_all(others)
end

def not_between(other)
if infinity?(other.begin)
if other.end.nil? || infinity?(other.end)
if unboundable?(other.begin) == 1 || unboundable?(other.end) == -1
not_in([])
elsif open_ended?(other.begin)
if other.end.nil? || open_ended?(other.end)
self.in([])
elsif other.exclude_end?
gteq(other.end)
else
gt(other.end)
end
elsif other.end.nil? || infinity?(other.end)
elsif other.end.nil? || open_ended?(other.end)
lt(other.begin)
else
left = lt(other.begin)
Expand Down Expand Up @@ -241,5 +245,13 @@ def quoted_array(others)
def infinity?(value)
value.respond_to?(:infinite?) && value.infinite?
end

def unboundable?(value)
value.respond_to?(:unboundable?) && value.unboundable?
end

def open_ended?(value)
infinity?(value) || unboundable?(value)
end
end
end
4 changes: 4 additions & 0 deletions activerecord/lib/arel/visitors/to_sql.rb
Expand Up @@ -629,6 +629,8 @@ def visit_Arel_Nodes_Assignment(o, collector)
def visit_Arel_Nodes_Equality(o, collector)
right = o.right

return collector << "1=0" if unboundable?(right)

collector = visit o.left, collector

if right.nil?
Expand Down Expand Up @@ -662,6 +664,8 @@ def visit_Arel_Nodes_IsDistinctFrom(o, collector)
def visit_Arel_Nodes_NotEqual(o, collector)
right = o.right

return collector << "1=1" if unboundable?(right)

collector = visit o.left, collector

if right.nil?
Expand Down
4 changes: 2 additions & 2 deletions activerecord/test/cases/calculations_test.rb
Expand Up @@ -838,13 +838,13 @@ def test_pluck_loaded_relation_sql_fragment
def test_pick_one
assert_equal "The First Topic", Topic.order(:id).pick(:heading)
assert_nil Topic.none.pick(:heading)
assert_nil Topic.where("1=0").pick(:heading)
assert_nil Topic.where(id: 9999999999999999999).pick(:heading)
end

def test_pick_two
assert_equal ["David", "david@loudthinking.com"], Topic.order(:id).pick(:author_name, :author_email_address)
assert_nil Topic.none.pick(:author_name, :author_email_address)
assert_nil Topic.where("1=0").pick(:author_name, :author_email_address)
assert_nil Topic.where(id: 9999999999999999999).pick(:author_name, :author_email_address)
end

def test_pick_delegate_to_all
Expand Down
14 changes: 14 additions & 0 deletions activerecord/test/cases/finder_test.rb
Expand Up @@ -282,6 +282,17 @@ def test_exists_with_order
assert_equal true, Topic.order(Arel.sql("invalid sql here")).exists?
end

def test_exists_with_large_number
assert_equal true, Topic.where(id: [1, 9223372036854775808]).exists?
assert_equal true, Topic.where(id: 1..9223372036854775808).exists?
assert_equal true, Topic.where(id: -9223372036854775809..9223372036854775808).exists?
assert_equal false, Topic.where(id: 9223372036854775808..9223372036854775809).exists?
assert_equal false, Topic.where(id: -9223372036854775810..-9223372036854775809).exists?
assert_equal false, Topic.where(id: 9223372036854775808..1).exists?
assert_equal true, Topic.where(id: 1).or(Topic.where(id: 9223372036854775808)).exists?
assert_equal true, Topic.where.not(id: 9223372036854775808).exists?
end

def test_exists_with_joins
assert_equal true, Topic.joins(:replies).where(replies_topics: { approved: true }).order("replies_topics.created_at DESC").exists?
end
Expand Down Expand Up @@ -383,16 +394,19 @@ def test_find_on_relation_with_large_number
assert_raises(ActiveRecord::RecordNotFound) do
Topic.where("1=1").find(9999999999999999999999999999999)
end
assert_equal topics(:first), Topic.where(id: [1, 9999999999999999999999999999999]).find(1)
end

def test_find_by_on_relation_with_large_number
assert_nil Topic.where("1=1").find_by(id: 9999999999999999999999999999999)
assert_equal topics(:first), Topic.where(id: [1, 9999999999999999999999999999999]).find_by(id: 1)
end

def test_find_by_bang_on_relation_with_large_number
assert_raises(ActiveRecord::RecordNotFound) do
Topic.where("1=1").find_by!(id: 9999999999999999999999999999999)
end
assert_equal topics(:first), Topic.where(id: [1, 9999999999999999999999999999999]).find_by!(id: 1)
end

def test_find_an_empty_array
Expand Down
5 changes: 5 additions & 0 deletions activerecord/test/cases/relation/or_test.rb
Expand Up @@ -30,6 +30,11 @@ def test_or_with_null_right
assert_equal expected, Post.where("id = 1").or(Post.none).to_a
end

def test_or_with_large_number
expected = Post.where("id = 1 or id = 9223372036854775808").to_a
assert_equal expected, Post.where(id: 1).or(Post.where(id: 9223372036854775808)).to_a
end

def test_or_with_bind_params
assert_equal Post.find([1, 2]).sort_by(&:id), Post.where(id: 1).or(Post.where(id: 2)).sort_by(&:id)
end
Expand Down
10 changes: 10 additions & 0 deletions activerecord/test/cases/relation/where_test.rb
Expand Up @@ -359,6 +359,16 @@ def permit!
assert_equal author, Author.where(params.permit!).first
end

def test_where_with_large_number
assert_equal [authors(:bob)], Author.where(id: [3, 9223372036854775808])
assert_equal [authors(:bob)], Author.where(id: 3..9223372036854775808)
end

def test_to_sql_with_large_number
assert_equal [authors(:bob)], Author.find_by_sql(Author.where(id: [3, 9223372036854775808]).to_sql)
assert_equal [authors(:bob)], Author.find_by_sql(Author.where(id: 3..9223372036854775808).to_sql)
end

def test_where_with_unsupported_arguments
assert_raises(ArgumentError) { Author.where(42) }
end
Expand Down

0 comments on commit 31ffbf8

Please sign in to comment.