[jscarle]

AI Disclaimer

I have also encountered this issue and previously confirmed it in #2035. AI (Codex) helped me prepare this PR, building on #4824 and taking its review feedback into account. The implementation and tests are AI-assisted and need maintainer review. The testing section below describes what was actually validated for this PR.

Description

ANSI parameter values can fit within 8,000 characters while exceeding the 8,000-byte limit in the connection's encoding. For example, 4,001 \u00e9 characters occupy 8,002 UTF-8 bytes. Selecting non-MAX metadata from the character count produces an invalid VARCHAR declaration or RPC payload.

This continues the work in #4824, retaining Jens Theisen's original commit and addressing the gaps identified in the requested-changes review:

• Count ANSI bytes with the negotiated connection encoding when selecting MAX metadata and building SQL parameter declarations.
• Require the parser argument at every ValidateTypeLengths call, including the SMI and Always Encrypted type_info paths. Preserve the MAX length indicator when byte-based promotion occurs and use encoded lengths for bounded ANSI metadata.
• Limit ANSI serialization to the characters remaining after Offset, including inferred Size and an explicit size larger than the remaining value. Invalidate the cached payload length when Offset changes.

SqlParameter.Size retains its character-based semantics, and there are no public API changes. Byte counting is shared by the declaration and metadata paths so they use the same encoding and transmitted slice.

Issues

Fixes #2035.

Related original PR: #4824.

Testing

Added 33 unit regression cases covering UTF-8 byte boundaries, ASCII and single-byte encodings, explicit/inferred/MAX sizes, truncation, offsets, changed offsets, nulls, SqlString, SqlChars, and the metadata entry point used by Always Encrypted. This includes checking that metadata remains MAX after declaration building has already promoted a parameter.

Added 16 SQL Server round-trip cases covering synchronous/asynchronous text, stored-procedure, and prepared commands, command reuse across the byte boundary, and offsets with inferred or explicit sizes. The tests use unique database names and clean up their resources.

Validation performed locally:

• git diff --check passed.
• Roslyn syntax parsing passed for all five changed C# files with both modern .NET and .NET Framework preprocessor configurations. This is a syntax check, not a successful compilation.
• The repository build and focused unit-test command were attempted with remote package sources cleared. Both stopped during cache-only dependency resolution: required packages and .NET Framework reference assemblies are missing, including Microsoft.NETFramework.ReferenceAssemblies.net462 and System.Configuration.ConfigurationManager. No build-dependent tests, SQL Server round trips, or customer-repro execution completed.
• Rider inspections were attempted with the installed command-line tool, with builds and updates disabled. Missing references prevented complete semantic analysis; the report contains unresolved dependency symbols, so a clean Rider result is not claimed.

No SDKs, tools, packages, or containers were installed or downloaded. The test results reported on #4824 belong to that PR and are not claimed as validation of this revision.

• Tests added or updated
• Public API unchanged
• Unit and SQL Server tests executed successfully
• Verified against the original customer repro

Suggested release note: Fixed ANSI parameter metadata for multibyte values that exceed the 8,000-byte limit, including encrypted parameter metadata and values sent with an offset.

[azure-pipelines]

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

[jscarle]

@priyankatiwari08 Can you please review this pull request to see if we can finally fix this long standing issue? Thanks!

[github-actions]

@jscarle This pull request has been marked as Author attention needed.

When you have addressed the reviewer feedback and are ready for another review, please post a comment with /ready to remove the label and re-engage reviewers.

[jscarle]

Not sure what my next action is? The github action mentions that I need to address feedback, but I see none?

[paulmedynski]

Hi @jscarle - it looks like there are some conflicts that need to be resolved. We can't run our workflows or trigger CI pipelines until those are fixed.

Also, should this PR be stacked on #4824? I can't tell at a glance if this PR depends on that one, or is just related and can be taken on its own.

[jscarle]

@paulmedynski I resolved the conflicts and recommitted as a single commit to make the fact that this does not depend on the prior pull request clearer.

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.

[jscarle]

@paulmedynski looks like the milestone needs to be updated from 8.0.0 to 8.0.0-preview1. I also fixed an error in tests.

[jscarle]

@mdaigle Can you please review this so we can finally put this long standing issue to bed?

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.

[paulmedynski]

/triage

[Copilot]

🟡 Changes recommended

A critical prepared-command reuse issue can truncate UTF-8 parameter values when byte-length promotion requires MAX, so declaration changes must trigger reprepare and a boundary-crossing regression test is needed.

1 open issue

• Reprepare when byte-based parameter promotion changes declaration · New

What changed in this PR

This PR addresses #2035 by using encoded byte lengths when sizing ANSI parameters, aligning SQL declarations, RPC metadata, and Always Encrypted metadata while preserving character-based SqlParameter.Size semantics.

Changes:

• Share connection-aware byte counting and promote oversized ANSI values to MAX.
• Account for Offset when calculating transmitted lengths and invalidate cached lengths when it changes.
• Add unit and synchronous/asynchronous SQL Server regression coverage. Build-dependent tests and round trips have not completed successfully.

File	Description
src/​Microsoft.Data.SqlClient/​tests/​UnitTests/​SqlCommandAnsiParameterTests.cs	Adds byte-boundary, offset, encoding, and metadata regression cases.
src/​Microsoft.Data.SqlClient/​tests/​ManualTests/​SQL/​Utf8SupportTest/​Utf8ParameterTests.cs	Adds UTF-8 round trips across command modes and command reuse.
src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​SqlParameter.cs	Centralizes byte sizing, handles offsets, and preserves MAX metadata.
src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​SqlCommand.cs	Uses connection-aware sizing for declarations and RPC validation.
src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​Parser/​TdsParser.cs	Supplies parser context to encrypted and SMI metadata generation.

_{Review effort: Balanced}

[codecov]

Codecov Report

❌ Patch coverage is 97.14286% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 66.26%. Comparing base (7b1184b) to head (e8f7998).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...t/src/Microsoft/Data/SqlClient/Parser/TdsParser.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4906      +/-   ##
==========================================
- Coverage   67.94%   66.26%   -1.68%     
==========================================
  Files         311      305       -6     
  Lines       45399    68295   +22896     
==========================================
+ Hits        30845    45258   +14413     
- Misses      14554    23037    +8483     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	66.26% <97.14%> (?)

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.

[jscarle]

@paulmedynski I pushed a fix and additional unit tests and coverage to address the identified issues.

[mdaigle]

Thanks for picking this up @jscarle. Clearly, when size is inferred, we need to calculate based on byte count, not character count. That looks good to me.

But I want to push back a bit on the idea of automatic promotion to varchar(MAX) outside of the inferred case. At the end of the day, we need to convert a varchar SqlParameter to a SQL varchar(n). Reinforcing the SqlParameter.Size interpretation as character count rather than byte count breaks the, effectively, direct mapping we perform today. It seems easier to me to always reason about Size as byte count.

Consider inserting a SqlParameter(SqlDbType.VarChar, 8000) into a varchar(8000) column. Outwardly, this would seem to truncate the parameter value to match the capacity of the column and keep us protected. However, if we supply 4001 multibyte characters, we would now promote to varchar(max) and transmit 8002 bytes resulting in a truncation error on the server.

I'm thinking that if we reinforce SqlParameter.Size as byte count, it makes it very easy to know that what you're doing with a parameter applies directly to the server-side types. The only downside is that you now need to be aware of multibyte characters and be careful not to split them if you do want to preserve whole characters.

Do you agree with this, or are there any issues you can see in my thinking? I'm trying to dig through the existing code and understand it as best as possible, but there's some very tricky logic in here with all kinds of backwards compatibility traps...

[jscarle]

@mdaigle I agree that treating an explicitly specified Size as a byte limit is the clearer model for varchar, and your example is valid. The documentation supports that interpretation.

The reason I preserved it here is that the existing implementation already limits ANSI strings by character count and then expands the SQL declaration to accommodate the encoded bytes. For example, under UTF-8, Size = 4000 with 4,000 é characters can already produce varchar(8000) before this PR. Strictly enforcing a 4,000-byte limit would instead transmit 2,000 characters. (Character sizing, declaration sizing)

This PR addresses the mismatch between that selected value and the metadata used to transmit it, including reusing a prepared declaration that is too small. The new regression tests reproduce silent truncation on the previous code and verify that the corrected paths preserve the selected value.

Your varchar(8000) column example remains an important distinction: preserving all 8,002 bytes can produce a server-side truncation error. This PR does not establish an explicit Size as a reliable byte budget, and I agree that behavior deserves a consistent resolution.

I propose we take this as the metadata/prepared-reuse fix and track explicit Size enforcement in a separate issue and focused PR? That would let us assess the compatibility implications across UTF-8 and other multibyte encodings, define truncation at complete character boundaries, and verify consistent behavior across execution paths.

That follow-up would address the Size concern directly without requiring this bug fix to also redefine the existing truncation behavior.

[mdaigle]

@jscarle I can support that approach. We'll want to be very careful with any changes to Size interpretation, so I'm fine to decouple it from this.

[Copilot]

🟡 Changes recommended

The ANSI length adjustment causes SqlDbType.Variant parameters with an offset to advertise fewer bytes than the writer emits, producing malformed RPC payloads.

1 open issue

• Exclude SqlVariant from ANSI offset size adjustment · New

1 resolved since last review

• Reprepare when byte-based parameter promotion changes declaration

_{Review effort: Balanced}

[mdaigle]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.

[jscarle]

@mdaigle I handled the variant type parameter case that Copilot mentioned and added additional regression tests / coverage.

[Copilot]

🔵 Needs a closer look

RPC framing, Always Encrypted metadata, and prepared-handle lifecycle changes lack a successful build and executed SQL Server regressions, leaving wire compatibility and failure/reuse behavior unverified.

No open issues 🎉

1 resolved since last review

• Exclude SqlVariant from ANSI offset size adjustment

_{Review effort: Balanced}

[mdaigle]

Can you please add testing specific to InputOutput and Output type parameters? My latest review led me to discovering #4953 and I'd like to capture how these changes impact that issue.

[jscarle]

@mdaigle Added.

[mdaigle]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.

[Copilot]

🔵 Needs a closer look

RPC/Always Encrypted metadata and prepared-handle recovery changes lack successful build and SQL Server runtime validation, requiring maintainer verification of wire metadata and command lifecycle behavior.

No open issues 🎉

_{Review effort: Balanced}