Add option to ScalePrecisionValidator to check digits to left of decimal point

[KevinHicksSW]

System Details

• FluentValidation version: 8.1.2
• Web Framework version: ASP.NET Core 2.1

Issue Description

When using ScalePrecisionValidator to validate input that is going to be saved into MS SQL Server, the validation can pass, but then fail to save into SQL Server due to a difference in how SQL Server looks at precision and scale and how FluentValidation is using it.

If you look at the Decimal Type documentation for SQL Server. It says:

s (scale)
The number of decimal digits that will be stored to the right of the decimal point. This number is subtracted from p to determine the maximum number of digits to the left of the decimal point.

If you look at the code for ScalePrecisionValidator. It is just checking if (scale > Scale || precision > Precision). It doesn't care about how many digits are to the left of the decimal point.

It would be nice to add an option so the behavior of ScalePrecisionValidator could be set up to match what SQL Server expects. This could be something like how IgnoreTrailingZeros works so you opt-in to using this different behavior. I can put up a PR for this change, but wanted to double check if it would be a good approach to this since I saw in #151 that it was just recommended to use a custom validator for this. I think it would be useful for this to be built into ScalePrecisionValidator since SQL Server is a commonly used database and its not obvious what is going wrong when you first run into this difference between the two.

Here is an example of a validator that passes, but then fails when the input goes down to SQL Server.

using FluentValidation;

public class TestClass
{
    public decimal Value { get; set; }
}

public class TestValidator : AbstractValidator<TestClass>
{
    public TestValidator()
    {
        RuleFor(x => x.Value).SetValidator(new ScalePrecisionValidator(4, 9) {IgnoreTrailingZeros = true});
    }
}

var testClass = new TestClass{Value = 123456};
var validator = new TestValidator();

// This passes
var result = validator.Validate(testClass);

However if the SQL Server Value column is also set to a scale of 4 and a precision of 9, inserting the value 123456 throws System.Data.SqlClient.SqlException (0x80131904): Error converting data type numeric to decimal.

[JeremySkinner]

Hi, yes sure, if you’d like to submit a pull request for this then I’d happily review it.

Yes let’s have a property for opting into this (SqlServerCompatible maybe?), and if you could add appropriate tests too that’d be great.

Thanks for wanting to contribute.

[KevinHicksSW]

Hi @JeremySkinner ,

You will see in my PR I didn't do anything for the validation error messages for this yet. I'm actually stuck trying to figure out the best way to handle that. We almost need two different versions of the error message, one that only says how many decimals are allowed and another for SqlServerCompatible that says how many digits and decimals are allowed.

It looks like we can't use string interpolation to put a ternary operator in the language files themselves to change what is output. I also thought of putting the dynamic part right in the validator, but that wouldn't work for multiple languages.

The only thing I can think of at this point is to actually make a SqlServerCompatibleScalePrecisionValidator that extends ScalePrecisionValidator so it can have its own error message and translations. Do you have any other recommendations?

[JeremySkinner]

Thanks for doing this. Yes I think having a subclass is probably the way to go here.

[Bubleguy]

Hi,
I think that this behavior is not related to Sql Server only.
The same is true for :

• Oracle : "For example, number(7,2) is a number that has 5 digits before the decimal and 2 digits after the decimal."
  https://www.techonthenet.com/oracle/datatypes.php

• Mysql : "A DECIMAL(20,6) column has fourteen integer digits and six fractional digits"
  https://dev.mysql.com/doc/refman/5.7/en/precision-math-decimal-characteristics.html

Untill now, i always assumed that the behavior described by @KevinHicksSW was the good one...
So @JeremySkinner can you precise me from which sources/docs/specs you based your initial algorithm

Thanks

[JeremySkinner]

I didn't write it - it was a contribution to the project made in 2012. I assume it was based on this stackoverflow answer from 2009, as the code is very similar, but I don't know for sure 🤷‍♂️

My understanding was the original intent of the validator was just to validate the scale value and the precision value - validating the number of digits clearly wasn't the original author's intent.

As I posted above, I am very open to this being changed if it would make it more useful but the implications of a breaking change have to be considered (both in terms of functionality, and also re-translation of error messsages), which is why I think @KevinHicksSW's idea of a subclass is better for now. This is currently waiting on @KevinHicksSW (or someone else) to provide an updated PR, at which point I can get it reviewed and merged.

I'm also open to making this the default behaviour in future, but as that's a breaking change it'd need to be for a major release (9.0 is the next, which I haven't started working on yet).

[Bubleguy]

Hi,
I read the link you provided. If i am correct, the algorithm allow us to know the precision, scale, effective scale, ... for any decimal. Well that's a cool tool.

Then, what i assume is :

• since the developper explicited precision AND scale (ScalePrecisionValidator) then he expects a decimal with X precision with Y scale and X-Y digits according to definition of precision and scale (wikipedia, oracle specs, ...)
• if the developper wants to check only the precision no matter what the scale is, then a specific validator should be existing (ex: PrecisionValidator)
• if the developper wants to check only the scale ... same idea

Concerning breaking change :

• I am a bit suprised that the problem didn't came up earlier
• I agree that retro-compatibility with a subclass should be considered but i think :
  • naming the subclass "SqlServer" is a bit too retrictive
  • having 2 class to validate a precision ans scale can be confusing for someone who take the nugget package for the first time. The documentation should be clear to help him.

Let me know if i can help you for something. Cheers

-- EDIT --
I see that you edited the previous post while i was typing my answer so don't take in count obselete information provided by me ;)

[JeremySkinner]

Thanks for the suggestions (and yes I edited it to include info about making this the default for 9.0 :) )

I am a bit suprised that the problem didn't came up earlier

Very few people seem to use this validator (in fact, until recently it only had a class and didn't have an extension method on RuleFor).

naming the subclass "SqlServer" is a bit too retrictive

Definitely open to suggestions for alternative names.

having 2 class to validate a precision ans scale can be confusing for someone who take the nugget package for the first time. The documentation should be clear to help him.

From a user's point of view, they wouldn't need to know there are 2 classes. The PR (#1014) adds a parameter to the ScalePrecision method, allowing you to opt-in to the new behaviour:

RuleFor(x => x.Discount).ScalePrecision(2, 5, ignoreTrailingZeros: true, sqlServerCompatible: true);

(Again I'm happy to rename the parameter).

If the new behaviour is going to be the default for 9.0 then ideally I'd like to see this implemented in a way which preserves the existing behaviour for 8.x, but marks it as Obsolete (thereby giving a warning). We'd then remove the current/obsolete behaviour from 9.x, leaving only the new approach (the current PR doesn't do this, it just adds the opt-in behaviour)

Language translations are more of an issue, as adding a new placeholder to an error message is a massive undertaking to get everything re-translated. Maybe we just update the english translation and not bother to re-translate other languages unless someone contributes them.

[JeremySkinner]

This didn't make it into the 8.2 release as it's still awaiting an updated PR. There are no more planned 8.x releases (barring any emergency hotfixes), so this will not be in 8.x. Retargetting for 9.0.

[Bubleguy]

ok thanks for the info.
I am busy at job at the moment. I will try to dig into the subject in the incoming weeks.
Cheers
-- EDIT --
Maybe we should close this issue, and open a new one since the pullrequest has been accepted

[JeremySkinner]

@Bubleguy the pull request hasn't been accepted - it's still open and marked as WIP, so will keep this open until the issues are resolved.

[nhowell]

Ran into this issue today where ScalePrecision doesn't work like SQL Server's decimal/numeric type. My understanding is that the implementation of FluentValidation's ScalePrecision is actually wrong, and the "sqlServerCompatible" version should become the only/default way that ScalePrecision works.

I would be willing to implement the fix and submit a PR for this, if you can get it into version 9.0. Thoughts @JeremySkinner?

[JeremySkinner]

Hi, thanks for offering to help. Yes I'm happy for this to become the default for 9.0, but the main problem is re-translating all of the error messages to include the new placeholder, which is a lot of work.

[JeremySkinner]

I've also been considering just dropping this validator completely, as I don't consider it a "core" validator, and instead open it up to the community to support it through an external package as it's not a validator that I'm keen on supporting.

[nhowell]

Thanks for the quick reply! I believe the existing error message placeholders are correct and wouldn't need changing (source). Notice that "Digits" already subtracts scale from precision.

context.MessageFormatter
	.AppendArgument("ExpectedPrecision", Precision)
	.AppendArgument("ExpectedScale", Scale)
	.AppendArgument("Digits", precision - scale)
	.AppendArgument("ActualScale", scale);

Since I'm obviously utilizing this validator I'm biased, but I would like to see this validator kept in the "core" :)

[JeremySkinner]

Ok, sounds good. If you'd like to get this updated then that'd be appreciated and I'll happily give it a review.

[nhowell]

Great! @mauricioz and myself will work on this and get a PR in for your review.

[JeremySkinner]

#1356 has been merged