Repository navigation
Convert unsigned enums to decimal through their underlying type - #39228
Conversation
EnumToNumberConverter<TEnum, decimal> converted the enum through long, so values of a ulong-backed enum above long.MaxValue were written as negative decimals (ulong.MaxValue became -1) and reading them back threw an OverflowException, even though the default mapping hints ask for decimal(20, 0) precisely to hold the full ulong range. Convert through the enum's underlying type instead.
…tored as -1
JsonTypesRelationalTestBase overrode the ulong enum JSON tests to expect -1
for EnumU64.Max ("because ulong is converted to long on SQL Server"), which
was the EnumToNumberConverter<TEnum, decimal> bug fixed in the previous commit.
SQL Server now writes 18446744073709551615 like the other providers, so the
overrides are removed, along with the SQLite overrides that only restored the
base expectations.
|
The SQL Server failures came from JsonTypesRelationalTestBase, which overrode the ulong enum JSON tests to expect -1 for EnumU64.Max (“because ulong is converted to long on SQL Server”). That expectation encoded the bug this PR fixes. I removed those overrides (and the SQLite ones that only restored the base expectations), so SQL Server now uses the same 18446744073709551615 expectation as the other providers. The full SQL Server functional suite passes locally. The Cosmos (ubuntu-24.04) failure looks unrelated: the same job is failing on other recent PRs too. |
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review issues remain, and the changes include appropriate boundary coverage.
0 open findings
What changed in this PR
Fixes enum-to-decimal conversion for unsigned underlying types, preserving values above long.MaxValue.
Changes:
- Convert through the enum’s underlying type.
- Add
ulongboundary conversion tests. - Remove obsolete provider-specific JSON overrides.
| File | Description |
|---|---|
test/EFCore.Tests/Storage/ValueConversion/EnumToNumberConverterTest.cs |
Adds unsigned decimal conversion boundary tests. |
test/EFCore.Sqlite.FunctionalTests/JsonTypesSqliteTest.cs |
Removes obsolete overrides. |
test/EFCore.Relational.Specification.Tests/JsonTypesRelationalTestBase.cs |
Removes obsolete negative-value expectations. |
src/EFCore/Storage/ValueConversion/EnumToNumberConverter.cs |
Converts through enum underlying types. |
🧠 Review effort: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
AndriySvyryd
left a comment
AndriySvyryd
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
EnumToNumberConverter<TEnum, decimal> converted the enum through long, so values of a ulong-backed enum above long.MaxValue were written as negative decimals (ulong.MaxValue became -1) and reading them back threw an OverflowException, even though the default mapping hints ask for decimal(20, 0) precisely to hold the full ulong range. Convert through the enum's underlying type instead.