Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RangeAttribute double parsing issue with hungarian culture #109672

Open
zgabi opened this issue Nov 9, 2024 · 4 comments
Open

RangeAttribute double parsing issue with hungarian culture #109672

zgabi opened this issue Nov 9, 2024 · 4 comments

Comments

@zgabi
Copy link
Contributor

zgabi commented Nov 9, 2024

Description

The RangeAttribute fails to parse the "2,0" double value, which is a valid hungarian format of the 2.0 number even when the ConvertValueInInvariantCulture is set to false.

Reproduction Steps

            void Check(RangeAttribute ra, object o)
            {
                try
                {
                    Console.WriteLine("result: " + ra.IsValid(o));
                }
                catch (Exception ex)
                {
                    Console.WriteLine("exception: " + ex.Message);
                }
            }

            Thread.CurrentThread.CurrentCulture = new CultureInfo("hu-HU");
            Thread.CurrentThread.CurrentUICulture = new CultureInfo("hu-HU");

            // in the hungarian language the decimal separator is the comma (",")
            // the group separator is the space (" ")

            RangeAttribute r;
            r = new RangeAttribute(1.1, 9.5) { ConvertValueInInvariantCulture = true };
            Check(r, 2.0); // True => OK
            Check(r, "2.0"); // True => OK
            Check(r, "2,0"); // Converts to 20, False => OK

            r = new RangeAttribute(typeof(double), "1.1", "9.5")
            {
                ParseLimitsInInvariantCulture = true,
                ConvertValueInInvariantCulture = true
            };
            Check(r, 2.0); // True => OK
            Check(r, "2.0"); // True => OK
            Check(r, "2,0"); // Exception, why? It should be converted to 20 => False
            // however this is not a big issue, since "2,0" looks a strange string format for 20
            // but double.Parse and Convert.ToDouble returns 20

            r = new RangeAttribute(1.1, 9.5) { ConvertValueInInvariantCulture = false };
            Check(r, 2.0); // True => OK
            Check(r, "2.0"); // True, why? It should throw exception
            Check(r, "2,0"); // False, why? It should convert to 2 which is in the range
            // This is the biggest issue!

Expected behavior

See the comments in the prevous section.

Actual behavior

See the comments in the prevous section.

Regression?

No response

Known Workarounds

            // When I specify the min and max values as string it seems to work:
            r = new RangeAttribute(typeof(double), "1.1", "9.5")
            {
                ParseLimitsInInvariantCulture = true,
                ConvertValueInInvariantCulture = false
            };
            Check(r, 2.0); // True => OK
            Check(r, "2.0"); // Exception => OK
            Check(r, "2,0"); // True => OK

Configuration

No response

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 9, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Nov 9, 2024
@jeffhandley jeffhandley added area-System.ComponentModel.DataAnnotations needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners untriaged New issue has not been triaged by the area owner labels Nov 9, 2024
@jeffhandley jeffhandley added this to the Future milestone Nov 9, 2024
@tarekgh
Copy link
Member

tarekgh commented Nov 9, 2024

@check could you share your Check method code to know what you are doing there? Are you just calling RangeAttribute.IsValid? and what is the value of CultureInfo.CurrentCulture in your environment?

@tarekgh tarekgh added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Nov 9, 2024
@zgabi
Copy link
Contributor Author

zgabi commented Nov 9, 2024

Yes, simply calling IsValid in a try catch. Added the method.
hu-HU is the culture in my system.

@dotnet-policy-service dotnet-policy-service bot added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Nov 9, 2024
@tarekgh tarekgh removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Nov 9, 2024
@tarekgh
Copy link
Member

tarekgh commented Nov 10, 2024

Here are answers for the questions but not necessary I am saying this should be the correct behavior. So, it looks like this type needs some fixes. Before I answer, just for the record, this type introduced long ago in the PR dotnet/corefx#501.

        Check(r, "2,0"); // Exception, why? It should be converted to 20 => False

When creating the RangeAttribute using signature like r = new RangeAttribute(typeof(double), "1.1", "9.5"), this internally will use DoubleConverted.FromString(..) which will call double.Parse(value, NumberStyles.Float, formatInfo);. Using NumberStyles.Float means the parse will not handle the group separator. While using Invariant culture and parsing 2,0, this will throw exception as , is not recognized because it is a group separator.
Note, if you create the RangeAttribute using signature r = new RangeAttribute(1.1, 9.5), this will cause using Convert.ToDouble which eventually use DoubleConverter.FromString(string value, int radix) which eventually will parse the string including the group separator and the parse will succeed at that time.

        Check(r, "2.0"); // True, why? It should throw exception
       Check(r, "2,0"); // False, why? It should convert to 2 which is in the range

When creating RangeAttribute using signature r = new RangeAttribute(1.1, 9.5) even if specifying ConvertValueInInvariantCulture = false, the Invariant culture will still be used in parsing this is because of the code

.

In summary, it looks to me these two issues are bugs in the code. Your workaround looks good to me which creating the RangeAttribute using the signature RangeAttribute(typeof(double), "1.1", "9.5") and setting ConvertValueInInvariantCulture = false.

@tarekgh
Copy link
Member

tarekgh commented Nov 10, 2024

I discover the behavior of the second issue is already documented RangeAttribute.ConvertValueInInvariantCulture Remarks.

Remarks
This property has no effect on the constructors with Int32 or Double parameters, which always use the invariant culture for any conversions of the validated value.

This will leave this issue with one problem which is keep the double parsing consistent regardless of the used constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants