BigDecimal without scale, precision doesn't validate as numeric
Reported by Ana Nelson | July 16th, 2008 @ 02:48 PM
- FIXME: if precision and scale are not specified, can we assume that it is an integer?
Well, you might also assume that someone has forgotten to specify :scale. :-)
The validation error message when you have a decimal point which doesn't correspond to scale/precision settings is rather cryptic:
{:bad_value=>["Bad value must be a number"]}
Especially if you have forgotten to specify scale and precision and scale has defauled to 0.
Suggestion attached.
Comments and changes to this ticket
-
Ana Nelson July 16th, 2008 @ 02:50 PM
and here's an example...
oh...
if you just do
puts e.errors.inspect
then you don't see the error message but if you do
puts e.valid?
puts e.errors.inspect
then you do.
-
Mayo Jordanov July 17th, 2008 @ 02:43 AM
I don't think you can assume integer and/or force specifying scale and precision because floating point values don't really use scale and precision. (well, there is, but it's platform dependent. I guess precision = 15, scale = 14 would be reasonable)
The code (http://github.com/sam/dm-more/tr...) should either break Float out and deal with it separately , or assume some safe defaults for scale and precision just for the sake of validating.
Although, looking at the NumericValidator, it's not quite clear to me why precision and scale need to be specified ... line 31 (http://github.com/sam/dm-more/tr...) has a failover with the regexp on line 42 (http://github.com/sam/dm-more/tr...), which will return true for Floats. I'll investigate some more tomorrow.
-
Mayo Jordanov July 17th, 2008 @ 03:26 AM
When property is a Float and no precision and scale is specified, precision seems to default to 10. This then triggers condition on line 35 in numeric_validator (http://github.com/sam/dm-more/tr...) which basically checks for a floating point number ending in ".0".
Could both precision and scale be set to false for Float, which would fail line 31, triggering line 42, and resulting in the correct floating point detection?
Failing spec attached.
-
Ana Nelson July 17th, 2008 @ 03:40 AM
The original problem I had was because I had a BigDecimal without specifying
precision, scale so these defaulted to 10, 0 respectively. I would have had
the same problem had I actually specified :scale => 0 and then tried to save
a number with a decimal.
I think it's easy to forget what your scale/precision has been set to,
especially if it's set implicitly, but even if you set it yourself, so I
like the idea of a very explicit validation error which says "you are
violating scale X and precision y".
It's possible to add something like:
big_decimal = value.is_a?(BigDecimal)
(or kind_of? if you prefer that but I find is_a? more natural)
just before this line:
http://github.com/sam/dm-more/tr...
and then you can treat Float/BigDecimal differently further down. But, I
don't know whether they need to be treated differently.
2008/7/17 Lighthouse :
-
Mayo Jordanov July 17th, 2008 @ 03:45 AM
- Assigned user changed from “Sam Smoot” to “Dan Kubb (dkubb)”
re-assigned to Dan, thanks man!
The default of precision = 10 comes from DAO type map. I'm not sure why 10 was chosen (middle ground between 6 and 15??) From my experience picking precision of FP numbers is very tricky, as it totally depends on the platform and should be left for the underlying storage to deal with, IMHO.
-
Mayo Jordanov July 17th, 2008 @ 04:08 AM
Oops, sorry. I didn't mean to steal your ticket. I misread your example there, but I have to agree. Changing the error message like you suggest may be a very good idea. I'll start another ticket for my issue there.
Sorry about that.
-
Dan Kubb (dkubb) July 17th, 2008 @ 01:00 PM
The default of 10 for Float/BigDecimal was a rather arbitrary decision, similar to String being a length of 50. There's no right decision, what would work for you, may be too long or short for me.
Code in DM has to work the same regardless of the adapter used, so there has to be a default set because otherwise the same model (without precision/scale specified) with one backend will work and another will return errors because it isn't able to support that precision and/or scale.
The only alternative to picking a default would be to actually raise an exception if the precision and scale aren't specified for Float or BigDecimal properties.
-
Ana Nelson July 17th, 2008 @ 01:10 PM
I'm not saying that the defaults are bad or even that we should warn if no
default is given.
If you look at my 1-line patch, it just adds a more helpful error message in
case the validation error is likely to be due to your choice of :scale. Will
save a lot of debugging time without changing behaviour at all, other than
the wording of the error message.
I got burned by this in ActiveRecord a lot since I would forget to specify
scale and my data would be truncated by the database. I'm much happier to
get a validation error and not lose any information, I just think it should
be a verbose validation error since forgetting to specify :scale is pretty
common IMO and the current validation error doesn't lead you in the right
direction to fix the problem.
2008/7/17 Lighthouse :
-
Mayo Jordanov July 17th, 2008 @ 03:49 PM
Dan,
I get the reasoning behind the String as well as BigDecimal (there should be a default on this one), but I don't agree with those defaults on a Float. Enough about there on here though, I should open a new ticket for that issue.
I like Ana's fix to the error message that makes it much more clear to the developer what's wrong, but some may object it's not very friendly to users when you rely on the errors for user interface. One other solution may be to use logger.warn() to put a message about scale and precision into the logs.
-
Ana Nelson July 17th, 2008 @ 05:20 PM
Oh yeah. End users. :-) Well, I don't always have a logger running and
I'm not sure I would have looked there in this case anyway since it
wasn't a database issue.
Saying "value is not a number" when you have typed what looks like a
valid number to you is going to be confusing to end users as well as
developers, so I think an error message which is more specific, i.e.
"you have more decimal places in your input than %t was expecting"
would be useful all around.
On 17 Jul 2008, at 21:49, Lighthouse wrote:
-
-
Mayo Jordanov July 22nd, 2008 @ 11:51 AM
No I haven't changed the error message. I can push this one through shortly.
-
Mayo Jordanov July 22nd, 2008 @ 12:05 PM
Dan resolved this in http://github.com/sam/dm-more/co..., so this issue can be closed now.
Thanks.
-
Please Sign in or create a free account to add a new ticket.
With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.
Create new ticket
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
Dan Kubb (dkubb)
Rick