Skip to content

Fix | Enable transport of large decimal values with explicit Precision and Scale#4443

Open
edwardneal wants to merge 1 commit into
dotnet:mainfrom
edwardneal:fix/issue-1655
Open

Fix | Enable transport of large decimal values with explicit Precision and Scale#4443
edwardneal wants to merge 1 commit into
dotnet:mainfrom
edwardneal:fix/issue-1655

Conversation

@edwardneal

Copy link
Copy Markdown
Contributor

Description

#1655 indicates that it's impossible to send decimal.MaxValue (or any other large decimal value) as a parameter to SQL Server when that parameter's Precision and Scale are set. This is a challenge for Always Encrypted scenarios, which requires these to always be explicitly set.

This issue was originally marked as external to SqlClient, but upon review I don't believe this is the case. The root cause is that the .NET decimal type supports a precision of up to 29 significant digits while the SQL Server numeric type's precision can range up to 38. Any conversion from SqlDecimal (which aligns with SQL Server) back to decimal after having been rescaled will risk overflowing - while the value itself might be within the bounds of decimal, the precision might not be. The OverflowException is therefore indicative of a correctness problem within SqlClient, not .NET.

This conversion from SqlDecimal to decimal occurs in TdsParser.AdjustDecimalScale. When taken in the context of its call site in (TDSExecuteRPCAddParameter) we see that to adjust the scale of a decimal SqlClient:

  1. Calls AdjustDecimalScale
    1. Gets the current scale of the decimal value
    2. Checks whether the value needs to be rescaled (i.e. whether its current scale does not already match the desired scale). If so:
      1. Instantiates a new SqlDecimal instance.
      2. Calls SqlDecimal.AdjustScale, accounting for the AppContext switch.
      3. Returns SqlDecimal.Value to converts the scaled value back to a decimal. This is the location of the OverflowException.
  2. Instantiates a new SqlDecimal with the return value of the above method
  3. Verifies that the new SqlDecimal instance's precision is >= the desired precision.

Once we've rescaled a decimal value, it should never perform a blind round-trip from SqlDecimal to decimal without checks to ensure that it'll fit in the CLR type. This PR removes this round-trip. We now return SqlDecimal directly from AdjustDecimalScale, then force the rest of TDSExecuteRPCAddParameter to treat the value as a SQL type. This has the pleasant side-effect of being able to merge some of the verification logic.

Issues

Fixes #1655.
Upstream EF Core issues: dotnet/efcore#28240; dotnet/efcore#33109; dotnet/efcore#35921.

Testing

One new test added which exercises the code path. One Always Encrypted test modified to verify that there have been no regressions.

This is necessary because a normal System.Decimal value is limited to a precision of 29, while SQL Server's numeric type has a maximum precision of 38.
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@cheenamalhotra

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).

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

Labels

None yet

Projects

Status: To triage

Development

Successfully merging this pull request may close these issues.

Cannot write decimal.MaxValue if precision/scale are set on SqlParameter

2 participants