Skip to content

Fix precision of rescaled zero decimal parameters - #4721

Merged
saurabh500 merged 4 commits into
mainfrom
dev/saurabh/issue-4715-argumentexception-after-updating-microso-f33eb7
Sep 22, 2026
Merged

saurabh500 merged 4 commits into
mainfrom
dev/saurabh/issue-4715-argumentexception-after-updating-microso-f33eb7

Conversation

@saurabh500

@saurabh500 saurabh500 commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Fix the 7.1.0 regression where CLR 0m fails with ArgumentException when sent as a decimal(p,p) parameter, including decimal(3,3) and decimal(2,2).

PR #4443 removed the SqlDecimal -> decimal -> SqlDecimal round-trip to support large decimal values. That round-trip also recomputed precision: increasing the scale of scale-zero zero retains an unnecessary integer digit in SqlDecimal.AdjustScale metadata.

Following review, handle zero in the shared TDSExecuteRPCAddParameter precision check after scale adjustment, covering both CLR decimal and SqlDecimal inputs. Zero fits any valid decimal precision/scale even when its precision metadata is larger. Compare magnitudes to recognize negative zero, retain the existing precision validation for nonzero values, and preserve the large-decimal support from #4443 without restoring a CLR decimal round-trip. This also covers the pre-existing rejection of SqlDecimal zero with excess precision metadata.

No public API changes.

Suggested release note: Fixed an ArgumentException when sending zero-valued decimal parameters whose precision equals their scale.

Issues

Fixes #4715

Regression introduced by 45f4759 (#4443). Preserves support added for #1655.

Backport after merge: release/7.1, targeting 7.1.1. The Hotfix 7.1.1 label schedules the repository's automatic cherry-pick PR workflow after this main-branch PR merges. This PR retains milestone 8.0.0-preview1 to match its target branch.

Testing

  • Added RPC-level unit coverage for CLR and SQL positive/negative zero, input scales 0-28, parameter scales 1-38, and both rounding modes. Added excess SQL precision, values rounded/truncated to zero, nonzero precision/rounding, and decimal.MinValue/MaxValue guards. Four SqlDecimal zero cases failed before moving the fix to shared validation.

  • Added SQL insertion coverage for decimal(p,p), with p = 1, 2, 3, 28, 29, and 38. Inputs explicitly include 0m, 0.000m, their SqlDecimal equivalents, signed zero, and SQL zero with excess precision. Following review, the scale matrix runs synchronously, with one representative decimal(3,3) async round-trip. Retained sync/async insufficient-precision rejection for both input types and large-decimal round-trip coverage.

  • Passed all 18 selected parser unit cases on both .NET 8 and .NET 9.

  • Passed all 11 selected integration cases after the review changes on .NET 8 against an isolated SQL Server 2022 container, with no skipped cases.

  • Used installed SDK 10.0.102 because the repository-pinned SDK 10.0.401 is unavailable. Integration builds used GenerateDocumentationFile=false to avoid the unrelated nested SDK invocation for XML documentation trimming.

  • SQL Server 2025 integration could not run because the local image segfaulted during startup. Windows/.NET Framework, Always Encrypted, and the original macOS/EF Core sample were not run.

  • Tests added or updated

  • Public API changes documented (not applicable; no API changes)

  • Verified against reduced customer repro at the driver serialization layer and direct SQL inserts

  • Ensure no breaking changes introduced: existing nonzero validation and large-decimal support retained

Normalize zero precision without restoring the CLR decimal round-trip, preserving large-decimal support. Add unit, sync/async integration coverage, and a precision documentation example for #4715.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings September 22, 2026 01:47
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Sep 22, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The focused fix preserves existing behavior and is supported by thorough unit and integration coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes decimal parameter serialization so rescaled CLR zero values fit decimal(p,p) while preserving large-decimal support.

Changes:

  • Normalizes rescaled zero precision using SqlDecimal.ConvertToPrecScale.
  • Adds comprehensive unit and sync/async integration coverage.
  • Documents zero-valued decimal(3,3) parameters.
File Description
TdsParser.cs Corrects precision metadata for rescaled CLR zero.
TdsParserDecimalTests.cs Tests zero, nonzero, rounding, and large values.
ParametersTest.cs Adds database-level sync/async regression coverage.
SqlParameter.xml Documents zero with equal precision and scale.
SqlParameter_Precision.cs Adds the referenced zero-parameter sample.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread doc/snippets/Microsoft.Data.SqlClient/SqlParameter.xml Outdated
@saurabh500 saurabh500 added this to the 8.0.0-preview1 milestone Sep 22, 2026
Keep a representative async zero round-trip and async exception propagation while testing scale boundaries synchronously, addressing PR review feedback.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@saurabh500 saurabh500 added the Hotfix 7.1.1 PRs targeting main that should be backported to release/7.1 for 7.1.1. label Sep 22, 2026
Copilot AI review requested due to automatic review settings September 22, 2026 05:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The focused fix preserves existing behavior and is backed by thorough regression coverage.

Review effort: Balanced
Findings: None

@edwardneal edwardneal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, and apologies for the regression. Just one comment about handling the SqlDecimal path - I'd been thinking about handling the 0M case in the check in TDSExecuteRPCAddParameter (line ~10436), after the values have been converted to their final value.

It might also be worth seeing if 0 and 0.000 behave differently in test cases (since one decimal will supply a scale and one won't.)

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs Outdated
Address review feedback by covering both decimal and SqlDecimal input paths after scale adjustment. Retain nonzero overflow checks and large-decimal support, with RPC-level and SQL integration coverage for signed and scaled zero values.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 22, 2026 06:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The focused fix preserves nonzero validation and is supported by thorough boundary, regression, and integration tests.

Review effort: Balanced
Findings: None

@saurabh500
saurabh500 marked this pull request as ready for review September 22, 2026 12:30
@saurabh500
saurabh500 requested a review from a team as a code owner September 22, 2026 12:30
@codecov

codecov Bot commented Sep 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.11%. Comparing base (671d010) to head (39242a4).

❗ There is a different number of reports uploaded between BASE (671d010) and HEAD (39242a4). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (671d010) HEAD (39242a4)
CI-SqlClient 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4721      +/-   ##
==========================================
- Coverage   71.96%   64.11%   -7.85%     
==========================================
  Files         291      285       -6     
  Lines       45110    68082   +22972     
==========================================
+ Hits        32462    43654   +11192     
- Misses      12648    24428   +11780     
Flag Coverage Δ
CI-SqlClient ?
PR-SqlClient-Project 64.11% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@saurabh500
saurabh500 enabled auto-merge (squash) September 22, 2026 15:45
mdaigle
mdaigle previously approved these changes Sep 22, 2026

@mdaigle mdaigle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Would be nice to factor out the logic into something that's more easily unit-testable, but I won't block on it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 22, 2026 18:47
This was referenced Sep 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Hotfix 7.1.1 PRs targeting main that should be backported to release/7.1 for 7.1.1.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

ArgumentException after updating Microsoft.Data.SqlClient to 7.1.0

6 participants