Skip to content

Conversation

@apoorvdeshmukh
Copy link
Contributor

Description

Ports #3399 to release/5.1

Issues

#3397

Testing

Port contains the testcase to validate the fix

Copilot AI review requested due to automatic review settings September 13, 2025 11:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ports a UTF-8 encoding fix from the main branch to release/5.1. The fix addresses an issue where UTF-8 encoding operations were including a Byte Order Mark (BOM) when they shouldn't, which could cause data corruption or incorrect encoding behavior in SQL Server bulk copy operations.

Key changes:

  • Replaces direct use of Encoding.UTF8 with a custom UTF-8 encoder that doesn't emit BOM
  • Adds comprehensive test coverage for UTF-8 bulk copy scenarios
  • Updates both .NET Framework and .NET Core implementations consistently

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
TestBulkCopyWithUTF8.cs New test file that validates UTF-8 bulk copy operations preserve byte sequences correctly
Microsoft.Data.SqlClient.ManualTesting.Tests.csproj Adds the new test files to the project compilation
DataTestUtility.cs Adds helper method for creating test tables with specific column definitions
netfx/src/Microsoft/Data/SqlClient/TdsParser.cs Replaces UTF-8 encoding instances with BOM-less version in .NET Framework implementation
netcore/src/Microsoft/Data/SqlClient/TdsParser.cs Replaces UTF-8 encoding instances with BOM-less version in .NET Core implementation

@apoorvdeshmukh apoorvdeshmukh requested a review from a team September 13, 2025 11:31
@codecov
Copy link

codecov bot commented Sep 13, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.34%. Comparing base (11c1f15) to head (d8546dc).
⚠️ Report is 1 commits behind head on release/5.1.

Files with missing lines Patch % Lines
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 80.00% 1 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           release/5.1    #3617      +/-   ##
===============================================
+ Coverage        72.12%   72.34%   +0.21%     
===============================================
  Files              293      293              
  Lines            61610    61612       +2     
===============================================
+ Hits             44437    44571     +134     
+ Misses           17173    17041     -132     
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 76.41% <80.00%> (+0.16%) ⬆️
netfx 69.89% <80.00%> (+0.17%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

@paulmedynski paulmedynski self-assigned this Sep 16, 2025
paulmedynski
paulmedynski previously approved these changes Sep 16, 2025
@benrr101
Copy link
Contributor

Afaik, this will need to have the branch merged into it to pick up CI changes once they go through.

@mdaigle mdaigle added this to the 5.1.8 milestone Oct 23, 2025
@apoorvdeshmukh apoorvdeshmukh merged commit 499b095 into release/5.1 Nov 5, 2025
80 checks passed
@apoorvdeshmukh apoorvdeshmukh deleted the dev/ad/port3399_to_51 branch November 5, 2025 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants