Skip to content

Commit

Permalink
Move integer range validation to never raise on assignment
Browse files Browse the repository at this point in the history
Given that this was originally added to normalize an error that would
have otherwise come from the database (inconsistently), it's more
natural for us to raise in `type_cast_for_database`, rather than
`type_cast_from_user`. This way, things like numericality validators can
handle it instead if the user chooses to do so. It also fixes an issue
where assigning an out of range value would make it impossible to assign
a new value later.

This fixes several vague issues, none of which were ever directly
reported, so I have no issue number to give. Places it was mentioned
which I can remember:

- https://github.com/thoughtbot/shoulda-matchers/blob/9ba21381d7caf045053a81f32df7de2f49687820/lib/shoulda/matchers/active_model/allow_value_matcher.rb#L261-L263
- rails#18653 (comment)
  • Loading branch information
sgrif committed Jan 23, 2015
1 parent 96e504e commit 7c6f393
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 20 deletions.
8 changes: 8 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
* Integer types will no longer raise a `RangeError` when assigning an
attribute, but will instead raise when going to the database.

Fixes several vague issues which were never reported directly. See the
commit message from the commit which added this line for some examples.

*Sean Griffin*

* Values which would error while being sent to the database (such as an
ASCII-8BIT string with invalid UTF-8 bytes on Sqlite3), no longer error on
assignment. They will still error when sent to the database, but you are
Expand Down
14 changes: 9 additions & 5 deletions activerecord/lib/active_record/type/integer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,19 @@ def type
:integer
end

alias type_cast_for_database type_cast

def type_cast_from_database(value)
return if value.nil?
value.to_i
end

def type_cast_for_database(value)
result = type_cast(value)
if result
ensure_in_range(result)
end
result
end

protected

attr_reader :range
Expand All @@ -34,9 +40,7 @@ def cast_value(value)
when true then 1
when false then 0
else
result = value.to_i rescue nil
ensure_in_range(result) if result
result
value.to_i rescue nil
end
end

Expand Down
39 changes: 26 additions & 13 deletions activerecord/test/cases/type/integer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,55 +60,68 @@ class IntegerTest < ActiveRecord::TestCase

test "values below int min value are out of range" do
assert_raises(::RangeError) do
Integer.new.type_cast_from_user("-2147483649")
Integer.new.type_cast_for_database(-2147483649)
end
end

test "values above int max value are out of range" do
assert_raises(::RangeError) do
Integer.new.type_cast_from_user("2147483648")
Integer.new.type_cast_for_database(2147483648)
end
end

test "very small numbers are out of range" do
assert_raises(::RangeError) do
Integer.new.type_cast_from_user("-9999999999999999999999999999999")
Integer.new.type_cast_for_database(-9999999999999999999999999999999)
end
end

test "very large numbers are out of range" do
assert_raises(::RangeError) do
Integer.new.type_cast_from_user("9999999999999999999999999999999")
Integer.new.type_cast_for_database(9999999999999999999999999999999)
end
end

test "normal numbers are in range" do
type = Integer.new
assert_equal(0, type.type_cast_from_user("0"))
assert_equal(-1, type.type_cast_from_user("-1"))
assert_equal(1, type.type_cast_from_user("1"))
assert_equal(0, type.type_cast_for_database(0))
assert_equal(-1, type.type_cast_for_database(-1))
assert_equal(1, type.type_cast_for_database(1))
end

test "int max value is in range" do
assert_equal(2147483647, Integer.new.type_cast_from_user("2147483647"))
assert_equal(2147483647, Integer.new.type_cast_for_database(2147483647))
end

test "int min value is in range" do
assert_equal(-2147483648, Integer.new.type_cast_from_user("-2147483648"))
assert_equal(-2147483648, Integer.new.type_cast_for_database(-2147483648))
end

test "columns with a larger limit have larger ranges" do
type = Integer.new(limit: 8)

assert_equal(9223372036854775807, type.type_cast_from_user("9223372036854775807"))
assert_equal(-9223372036854775808, type.type_cast_from_user("-9223372036854775808"))
assert_equal(9223372036854775807, type.type_cast_for_database(9223372036854775807))
assert_equal(-9223372036854775808, type.type_cast_for_database(-9223372036854775808))
assert_raises(::RangeError) do
type.type_cast_from_user("-9999999999999999999999999999999")
type.type_cast_for_database(-9999999999999999999999999999999)
end
assert_raises(::RangeError) do
type.type_cast_from_user("9999999999999999999999999999999")
type.type_cast_for_database(9999999999999999999999999999999)
end
end

test "values which are out of range can be re-assigned" do
klass = Class.new(ActiveRecord::Base) do
self.table_name = 'posts'
attribute :foo, Type::Integer.new
end
model = klass.new

model.foo = 2147483648
model.foo = 1

assert_equal 1, model.foo
end
end
end
end
4 changes: 2 additions & 2 deletions activerecord/test/cases/type/unsigned_integer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ module ActiveRecord
module Type
class UnsignedIntegerTest < ActiveRecord::TestCase
test "unsigned int max value is in range" do
assert_equal(4294967295, UnsignedInteger.new.type_cast_from_user("4294967295"))
assert_equal(4294967295, UnsignedInteger.new.type_cast_for_database(4294967295))
end

test "minus value is out of range" do
assert_raises(::RangeError) do
UnsignedInteger.new.type_cast_from_user("-1")
UnsignedInteger.new.type_cast_for_database(-1)
end
end
end
Expand Down

0 comments on commit 7c6f393

Please sign in to comment.