This project is archived and is in readonly mode.
Valid number pretending to be a invalid number in ActiveModel tests
Reported by Rohit Arondekar | May 17th, 2010 @ 05:09 AM | in 3.0.2
Running the ActiveModel testsuite in Rails master on ruby 1.9.2dev (2010-05-08 trunk 27665) [x86_64-linux] gives 3 failures.
2 failures are because of a number that's supposed to be bad but is actually good. I've made a patch that makes the number bad again, and will attach it in a comment.
The stacktrace:
1) Failure:
test_default_validates_numericality_of(NumericalityValidationTest)
[/home/rohit/remote-repos/rails_patches/working1/activemodel/test/cases/validations/numericality_validation_test.rb:166]:
"0xdeadbeef" not rejected as a number
2) Failure:
test_validates_numericality_of_with_nil_allowed(NumericalityValidationTest)
[/home/rohit/remote-repos/rails_patches/working1/activemodel/test/cases/validations/numericality_validation_test.rb:166]:
"0xdeadbeef" not rejected as a number
3) Failure: test_should_serialize_yaml(XmlSerializationTest)
[/home/rohit/remote-repos/rails_patches/working1/activemodel/test/cases/serializeration/xml_serialization_test.rb:107]:
Expected /\n\n aaron stack\n
303 tests, 865 assertions, 3 failures, 0 errors, 0 skips
Test run options: --seed 11594
rake aborted!
Command failed with status (1):
[/home/rohit/.rvm/rubies/ruby-1.9.2-head/bi...]
Failure 1 and 2 are because of the good number "0xdeadbeef" in the JUNK array in numericality_validation_test.rb Changing it to an invalid number like "0xdeadbeet" makes the test pass as expected. Patch will be provided in a comment.
P.S I don't know how to fix the 3rd failure.
Comments and changes to this ticket
-
Rohit Arondekar May 17th, 2010 @ 05:12 AM
I've attached a patch.
P.S rubydiamond from #railsbridge has confirmed that the tests pass in 1.8.7, so it might be something that needs to be looked into more carefully.
-
Anil Wadghule May 17th, 2010 @ 09:59 AM
+1
Verified patch on ruby-1.8.7-p249 and ruby-1.9.2-head.
Still a question I have is, why 1.9.2 considers 0xdeadbeef as valid number but 1.8.7 does not.
When inspected in irb for both rubies, it shows
ruby-1.8.7-p249 > 0xdeadbeef.class => Bignum ruby-1.8.7-p249 > 0xdeadbeef => 3735928559
ruby-1.9.2-head > 0xdeadbeef.class => Bignum ruby-1.9.2-head > 0xdeadbeef => 3735928559
Need to know reason for putting this hex number 0xdeadbeef as not valid numerical test. Thoughts?
-
Anil Wadghule May 17th, 2010 @ 10:10 AM
Looks like 1.9.2 is converting hex number to float but 1.8.7 don't
ruby-1.8.7-p249 > Kernel.Float("0xdeadbeef") ArgumentError: invalid value for Float(): "0xdeadbeef" from (irb):1:in `Float' from (irb):1
ruby-1.9.2-head > Kernel.Float("0xdeadbeef") => 3735928559.0
+1 for the patch
-
Rohit Arondekar May 17th, 2010 @ 11:28 AM
I think I've got it! Take a look at the following code from activemodel/lib/active_model/validations/numericality.rb
def parse_raw_value_as_a_number(raw_value) begin Kernel.Float(raw_value) rescue ArgumentError, TypeError nil end end def parse_raw_value_as_an_integer(raw_value) raw_value.to_i if raw_value.to_s =~ /\A[+-]?\d+\Z/ end
Note how a raw integer value is parsed. Using a regex, most probably because Kernel.Integer honors radix indicators like 0x.
Whereas a raw float is parsed using Kernel.Float which until 1.9.2 (or maybe 1.9.1) didn't honor radix indicators. But now that it does on 1.9.2, bam! it accepts a number which was not meant to be accepted. That's my understanding so far about this issue.
-
Rohit Arondekar May 17th, 2010 @ 11:52 AM
I can confirm that ruby 1.9.1p378 (2010-01-10 revision 26273) [x86_64-linux] behaves like 1.8.7 but 1.9.2 doesn't. I believe I've found the changeset that did this => http://redmine.ruby-lang.org/repositories/revision/ruby-19?rev=26965 and it's associated feature => http://redmine.ruby-lang.org/issues/show/2969 but the discussions are in Japanese :(
I can't make any more progress on this issue as I think I've reached a dead-end, I don't plan on digging in the Ruby source code :P. Hopefully a Core member or somebody who knows how the validator should behave can shed some light.
-
Rizwan Reza May 17th, 2010 @ 12:08 PM
- Milestone cleared.
- Tag changed from rails 3.0 activemodel activerecord tests, bugmash to bugmash-review
- State changed from new to verified
-
José Valim May 17th, 2010 @ 12:33 PM
- Assigned user changed from Pratik to José Valim
Great work debugging guys!
The fix is not changing the test to use "0xdeadbeet". I doubt our applications should allow hexadecimals entries. IMHO, we should simply change the code to check if the number does not start with "0x". If it does, it should be marked as not a number.
-
Anil Wadghule May 17th, 2010 @ 02:05 PM
I've attached a patch which fixes the broken numericality tests for ruby-1.9.2-head. Tests pass on ruby-1.8.7-p249 too.
-
Repository May 17th, 2010 @ 03:59 PM
- State changed from verified to resolved
(from [5371242384171dc0255716e31e9257ddeec17d10]) Valid hex strings aren't valid float column values, to match the integer restriction. [#4622 state:resolved] http://github.com/rails/rails/commit/5371242384171dc0255716e31e9257...
-
Rizwan Reza May 17th, 2010 @ 04:17 PM
- Tag cleared.
-
Rohit Arondekar May 18th, 2010 @ 01:42 AM
- Tag set to bugmash
I've attached a patch. First only a failing test to show that the commit doesn't block hex numbers of the form 0X22 and another patch to fix the code (with the test).
-
Ryan Bigg May 18th, 2010 @ 01:54 AM
- Tag changed from bugmash to bugmash, bugmash-review
- State changed from resolved to open
-
Repository May 18th, 2010 @ 02:19 AM
(from [05e3fb45eeacd20f2b8b5691f17d6fdf2fb4582b]) Add a valid hex that shouldn't be valid to ActiveModel numericality tests [#4622 state:commited]
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/05e3fb45eeacd20f2b8b5691f17d6f... -
Repository May 18th, 2010 @ 02:19 AM
(from [8e3c3b06dc8ff8842f6390efc58eaf4bb1a23060]) Fixed numericality validator in ActiveModel to reject hex numbers for floats completely [#4622 state:commited]
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/8e3c3b06dc8ff8842f6390efc58eaf... -
Ryan Bigg May 19th, 2010 @ 10:21 AM
- State changed from open to committed
-
Rizwan Reza June 6th, 2010 @ 09:29 AM
- Tag changed from bugmash, bugmash-review to bugmash
-
Rizwan Reza June 6th, 2010 @ 09:30 AM
- Tag cleared.
-
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to 3.0.2
- Importance changed from to Low
-
teiddy November 30th, 2010 @ 05:54 AM
Instructions: download and untar to /sites/all/modules
also - Download and install 'Rules' module to /sites/all/modules
Run /update.php
Goto Admin>Build>Modules, enable "OA Single Group Login Redirect" module and allow Rules module to be activated when prompted.
Log-out as admin and log-in as user with 1 group - page redirects as expected.
Greyside Thank-you!
Thank you for this information,I like it very much,Would you lik a pair of
Pachuco Suits
pack linen clothes
pack suit into suit
pant length
pant length for menpaul smith mens suits
Welcome to our store,We have the best service team!
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
Attachments
Referenced by
- 4622 Valid number pretending to be a invalid number in ActiveModel tests (from [5371242384171dc0255716e31e9257ddeec17d10]) Valid h...
- 4622 Valid number pretending to be a invalid number in ActiveModel tests (from [05e3fb45eeacd20f2b8b5691f17d6fdf2fb4582b]) Add a v...
- 4622 Valid number pretending to be a invalid number in ActiveModel tests (from [8e3c3b06dc8ff8842f6390efc58eaf4bb1a23060]) Fixed n...