Fix SQLite decimal comparison with turkish culture #37429
Conversation
[release/10.0] Source code updates from dotnet/dotnet
[release/10.0] Source code updates from dotnet/dotnet
Updated decimal parsing in EF_DECIMAL collation to use CultureInfo.InvariantCulture for consistent ordering across cultures. Added a test to verify correct ordering of decimal values under Turkish culture.
Eliminated the unnecessary 'using System.Globalization;' directive from BuiltInDataTypesSqliteTest.cs to clean up the code.
There was a problem hiding this comment.
Pull request overview
This PR fixes a culture-specific bug in SQLite decimal comparison where using Turkish culture (or any culture with a different decimal separator) would cause incorrect sorting of decimal values.
- Updated the
EF_DECIMALcollation to parse decimal strings usingCultureInfo.InvariantCulture - Added comprehensive test coverage for decimal ordering under Turkish culture (tr-TR)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/EFCore.Sqlite.Core/Storage/Internal/SqliteRelationalConnection.cs |
Updated the EF_DECIMAL collation to use InvariantCulture when parsing decimal strings, fixing culture-sensitive comparison issues |
test/EFCore.Sqlite.FunctionalTests/BuiltInDataTypesSqliteTest.cs |
Added test with Turkish culture to verify decimal ordering works correctly regardless of application culture |
| decimal.Parse(x, NumberStyles.Any, CultureInfo.InvariantCulture), | ||
| decimal.Parse(y, NumberStyles.Any, CultureInfo.InvariantCulture))); |
There was a problem hiding this comment.
Using NumberStyles.Any for parsing decimal values from the database might be too permissive. NumberStyles.Any includes support for currency symbols, thousands separators, parentheses for negatives, and other formatting that shouldn't appear in SQLite's decimal storage format. Consider using a more restrictive NumberStyles value like NumberStyles.Number or NumberStyles.AllowDecimalPoint | NumberStyles.AllowLeadingSign to more accurately match the expected format of decimal values stored in SQLite.
| decimal.Parse(x, NumberStyles.Any, CultureInfo.InvariantCulture), | |
| decimal.Parse(y, NumberStyles.Any, CultureInfo.InvariantCulture))); | |
| decimal.Parse(x, NumberStyles.Number, CultureInfo.InvariantCulture), | |
| decimal.Parse(y, NumberStyles.Number, CultureInfo.InvariantCulture))); |
Adjusted the TestDecimal values for test entities in Can_query_OrderBy_decimal_with_Turkish_culture to ensure correct ordering and test coverage.
AndriySvyryd
requested review from
AndriySvyryd and
cincuranet
as code owners
Can you please open an issue? We try to always have an issue - especially when there's a bug - so that it's clear which release contains what, and when a bug was fixed, etc. |
|
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Thanks for submitting a fix! See below for some comments.
|
|
||
| <PropertyGroup Label="Version settings"> | ||
| <VersionPrefix>10.0.2</VersionPrefix> | ||
| <VersionPrefix>10.0.3</VersionPrefix> |
There was a problem hiding this comment.
Please revert, we handle version bumps separately.
| decimal.Parse(x, NumberStyles.Any, CultureInfo.InvariantCulture), | ||
| decimal.Parse(y, NumberStyles.Any, CultureInfo.InvariantCulture))); |
| } | ||
|
|
||
| [ConditionalFact, UseCulture("tr-TR")] | ||
| public virtual void Can_query_OrderBy_decimal_with_Turkish_culture() |
There was a problem hiding this comment.
Can you please reduce this test to the bare minimum, removing all the unneeded stuff? Check out the minimal repro posted here.
3 participants
Fixes #37432
Summary of the changes
Details
SqliteConnection.CreateCollationto useCultureInfo.InvariantCulturewhen parsing decimal stringsFixes
This addresses the issue where SQLite decimal comparison would fail when the application was running under a culture that uses a different decimal separator (e.g., Turkish using comma instead of period).