ScalePrecision validation error message is misleading

[dropik]

FluentValidation version

11.2.2

ASP.NET version

.NET 6

Summary

It appears that ScalePrecisionValidator produces an error message which does not quite reflect what is actually being validated.

I have a decimal property in my model class, which I want to be of precision 10 and scale 2. So in theory a value like 123456789 should be fine, since by not providing any zeros after decimal point the total precision of this number is 9. And it seems that ScalePrecisionValidator calculates precision correctly, but it still gives a validation error and a message like this:

'{PropertyName}' must not be more than 10 digits in total, with allowance for 2 decimals. 9 digits and 0 decimals were found.

Which sounds like a nonsense. After a bit of digging the FluentValidation source code it turned out what is actually being validated by ScalePrecisionValidator:

public override bool IsValid(ValidationContext<T> context, decimal decimalValue) {
    var scale = GetScale(decimalValue);
    var precision = GetPrecision(decimalValue);
    var actualIntegerDigits = precision - scale;
    var expectedIntegerDigits = Precision - Scale;
    if (scale > Scale || actualIntegerDigits > expectedIntegerDigits) {
        context.MessageFormatter
	    .AppendArgument("ExpectedPrecision", Precision)
	    .AppendArgument("ExpectedScale", Scale)
	    .AppendArgument("Digits", actualIntegerDigits < 0 ? 0 : actualIntegerDigits)
	    .AppendArgument("ActualScale", scale);

	return false;
    }
    return true;
}

So it does not actually validate precision at all. What is being validated is that the integer part of the value must not overflow expected amount of digits, i.e. Precision - Scale amount of digits, which in my case was 8 digits. And now it makes sense why validation does not pass, because indeed my number has 9 digits in the integer part. But what is being communicated back to the user as validation error message and what is being validated are two different things. Without knowing this implementational detail this might be quite hard to the user to make out why validation does not pass.

This issue would actually relate to the abandoned issue #1200, but the proposal stated there would not resolve the problem I think, since in this case, for example, giving the total precision and giving amount of integer digits would result in the same number 9. So I would say that it might be better to communicate 'ExpectedIntegerDigits' as something that was expected from a decimal value to have, and what is indeed validated. And the error message would sound something like:

'{PropertyName}' must have at most 8 integer digits and 2 decimals. 9 integer digits and 0 decimals were found.

In this manner the error message would make sense.

But also as stated in #1200, it is true that such a validation logic does not correspond with what is actually defined by scale and precision. But such a thing is discutable, and the error message problem is on the other hand more specific.

Steps to Reproduce

This can be quite easily reproduced. Assume we have a class A with a decimal property Property:

public class A
{
    public decimal Property { get; set; }
}

And we want this property to be of precision 10 and scale 2:

public class AValidator : AbstractValidator<A>
{
    public AValidator()
    {
        RuleFor(x => x.Property).ScalePrecision(2, 10);
    }
}

So we try to give it a number like 123456789 and to execute validation on it:

public class Program
{
    public static void Main(string[] args)
    {
        var a = new A()
        {
            Property = 123456789,
        };

        var validationResult = new AValidator().Validate(a);
        if (!validationResult.IsValid())
        {
            foreach (var error in validationResult.Errors)
            {
                Console.WriteLine(error.ErrorMessage);
            }
        }
        else
        {
            Console.WriteLine("A is ok!");
        }
    }
}

And instead of not having any validation error at all or at least having an error message which would seem reasonable the result would be:

'Property' must not be more than 10 digits in total, with allowance for 2 decimals. 9 digits and 0 decimals were found.

[JeremySkinner]

hi, thanks for the feedback. I'm not keen on changing the error message at this point because of the huge amount of work required in updating all the translations, however it sounds like it might be worth at least updating the documentation to provide additional clarification - if you'd like to open a PR for the docs then I'd be happy to review it.

[dropik]

Hi, thank you for the response!
I see... In such a case may be it would be also reasonable to communicate precision of the value instead of the integer part, but "right-padded" to amount of expected decimals? So that in the same example as above the right-padded number would be 123456789.00, because we expect the number to have 2 decimals. And the error message would state ... 11 digits and 2 decimals were found.. Basically actual precision and scale communicated in error message would be calculated as s = max(actualScale, expectedScale) and p = actualIntegerPart + s.

I think that it can be borrowed here how sql defines what range of numbers can be stored as DECIMAL(M,N). It states that for example for DECIMAL(3,1) it can store values between -99.9 to 99.9 (and may be this statement can be reflected in docs). Which makes it crystal clear why's the integer part restriction and makes think about sort of a mask to restrict decimals with. Which brings to the zeros padding. The thing here is that by communicating back actual precision and scale in this manner, yes, the user should make assumptions on how validator interprets input values, but at least error message becomes non self contradictory.

Note that this change would not conflict with ignoreTrailingZeros feature, since this makes importance only when decimal part somehow overflows expected decimals amount. In other words if scale >= expectedScale nothing changes.

[JeremySkinner]

I've pushed out 11.9.2 with this change

[dropik]

Awesome, thank you!