aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorRyuta Kamizono <kamipo@gmail.com>2017-07-24 09:50:11 +0900
committerRyuta Kamizono <kamipo@gmail.com>2019-01-18 16:01:07 +0900
commit31ffbf8d5056137717da3f11d28c4fbd7fbc8f07 (patch)
tree3b8d6a2d9329a91178d358f0e45c7ec6b13362ee
parent5b6daff5b6d5439e07c058718069f54b34970f93 (diff)
downloadrails-31ffbf8d5056137717da3f11d28c4fbd7fbc8f07.tar.gz
rails-31ffbf8d5056137717da3f11d28c4fbd7fbc8f07.tar.bz2
rails-31ffbf8d5056137717da3f11d28c4fbd7fbc8f07.zip
All of queries should return correct result even if including large number
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.
-rw-r--r--activerecord/lib/active_record/relation/finder_methods.rb10
-rw-r--r--activerecord/lib/arel/predications.rb24
-rw-r--r--activerecord/lib/arel/visitors/to_sql.rb4
-rw-r--r--activerecord/test/cases/calculations_test.rb4
-rw-r--r--activerecord/test/cases/finder_test.rb14
-rw-r--r--activerecord/test/cases/relation/or_test.rb5
-rw-r--r--activerecord/test/cases/relation/where_test.rb10
7 files changed, 53 insertions, 18 deletions
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index fd84f9c46b..8f1065c1e7 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -79,17 +79,12 @@ module ActiveRecord
# 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
@@ -322,8 +317,6 @@ module ActiveRecord
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
@@ -434,9 +427,6 @@ module ActiveRecord
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)
diff --git a/activerecord/lib/arel/predications.rb b/activerecord/lib/arel/predications.rb
index 2a62c53aa3..7dafde4952 100644
--- a/activerecord/lib/arel/predications.rb
+++ b/activerecord/lib/arel/predications.rb
@@ -35,15 +35,17 @@ module Arel # :nodoc: all
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))
@@ -81,15 +83,17 @@ Passing a range to `#in` is deprecated. Call `#between`, instead.
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)
@@ -241,5 +245,13 @@ Passing a range to `#not_in` is deprecated. Call `#not_between`, instead.
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
diff --git a/activerecord/lib/arel/visitors/to_sql.rb b/activerecord/lib/arel/visitors/to_sql.rb
index c08403eea9..d0dec63860 100644
--- a/activerecord/lib/arel/visitors/to_sql.rb
+++ b/activerecord/lib/arel/visitors/to_sql.rb
@@ -629,6 +629,8 @@ module Arel # :nodoc: all
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?
@@ -662,6 +664,8 @@ module Arel # :nodoc: all
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?
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index 7af26d8ff5..850bc49676 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -838,13 +838,13 @@ class CalculationsTest < ActiveRecord::TestCase
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
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index 4e8f779951..32cf0fb9e7 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -282,6 +282,17 @@ class FinderTest < ActiveRecord::TestCase
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
@@ -383,16 +394,19 @@ class FinderTest < ActiveRecord::TestCase
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
diff --git a/activerecord/test/cases/relation/or_test.rb b/activerecord/test/cases/relation/or_test.rb
index 065819e0f1..8623867864 100644
--- a/activerecord/test/cases/relation/or_test.rb
+++ b/activerecord/test/cases/relation/or_test.rb
@@ -30,6 +30,11 @@ module ActiveRecord
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
diff --git a/activerecord/test/cases/relation/where_test.rb b/activerecord/test/cases/relation/where_test.rb
index 99797528b2..d49ed092b2 100644
--- a/activerecord/test/cases/relation/where_test.rb
+++ b/activerecord/test/cases/relation/where_test.rb
@@ -359,6 +359,16 @@ module ActiveRecord
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