Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Use UTF8 instance that doesn't emit BOM#3399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
cheenamalhotra merged 8 commits intomainfromdev/ad/3397
Jun 10, 2025
Merged

Conversation

apoorvdeshmukh
Copy link
Contributor

@apoorvdeshmukhapoorvdeshmukh commentedJun 6, 2025•
edited
Loading

Description

Fixes#3397. Used UTF8Encoding(false) instance that doesn't emit BOM instead of
using System.Encoding.UTF8 which adds BOM while transmitting the data.
Added test case based on the available repro which fails without the fix.

Issues

#3397

Testing

Added sync and async SqlBulkCopy testcase that does bulkcopy from UTF8 source table to another table with UTF8 collation
with MARS = true/false, EnableStraming = true/false.
Without the fix, when the destination table is read after bulkcopy, contains BOM in the data.

mahara reacted with thumbs up emoji
@CopilotCopilotAI review requested due to automatic review settingsJune 6, 2025 20:20
@apoorvdeshmukhapoorvdeshmukh requested a review froma team as acode ownerJune 6, 2025 20:20
@apoorvdeshmukhapoorvdeshmukh added this to the6.1-preview2 milestoneJun 6, 2025
Copy link
Contributor

@CopilotCopilotAI 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 replaces uses ofEncoding.UTF8 (which emits a BOM) with a dedicatedUTF8Encoding(false) instance that suppresses the BOM, and adds a manual test to verify bulk copy behavior with UTF8 data.

  • Introduceds_utf8EncodingWithoutBom and updated allEncoding.UTF8 references in TdsParser (netfx & netcore)
  • AddedTestBulkCopyWithUTF8.cs manual test for streaming bulk copy without BOM
  • Updated test project and solution to include the new test file

Reviewed Changes

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

Show a summary per file
FileDescription
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.csAdded BOM-less UTF8 static field and replacedEncoding.UTF8 usages
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.csSame BOM-less UTF8 changes for .NET Core
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/TestBulkCopyWithUTF8.csNew manual test to verify UTF8 bulk copy without BOM
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csprojIncluded the new test file
src/Microsoft.Data.SqlClient.slnAddedCommon project entries
Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/TestBulkCopyWithUTF8.cs:12

  • [nitpick] The class name casing (TestBulkCopyWithUtf8) doesn’t match the file name (TestBulkCopyWithUTF8.cs). Consider renaming the class toTestBulkCopyWithUTF8 for consistency.
public class TestBulkCopyWithUtf8

src/Microsoft.Data.SqlClient.sln:307

  • Entries for aCommon project were added to the solution, but the PR description and related changes don't reference this project. If unintentional, consider removing it or splitting it into a separate PR to keep scope focused.
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Common", "Microsoft.Data.SqlClient\tests\Common\Common.csproj", "{67128EC0-30F5-6A98-448B-55F88A1DE707}"

Copy link
Contributor

@saurabh500saurabh500 left a comment

Choose a reason for hiding this comment

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

Approach looks good.
Left some comments on the test approach

@edwardneal
Copy link
Contributor

It might be a good idea to make sure that we have test coverage for scenarios where v6.1 tries to read UTF8 data which starts with a BOM, to handle cases where v6.1+ has to read data which was copied by v6.0...

@saurabh500
Copy link
Contributor

saurabh500 commentedJun 6, 2025•
edited
Loading

@edwardneal The scenario you're describing is not something we intend to support. Adding a BOM (Byte Order Mark) can make the data unusable in many cases.

We're operating under the assumption that no one has relied on this buggy behavior for bulk copy operations. As such, ensuring interoperability between the old and corrected behavior is not within the scope of this fix.

edwardneal and mahara reacted with thumbs up emoji

@saurabh500saurabh500 self-requested a reviewJune 7, 2025 06:52
@apoorvdeshmukhapoorvdeshmukh requested a review froma teamJune 7, 2025 12:39
saurabh500
saurabh500 previously approved these changesJun 7, 2025
@codecovCodecov
Copy link

codecovbot commentedJun 8, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is9.09091% with10 lines in your changes missing coverage. Please review.

Project coverage is 65.57%. Comparing base(0444198) to head(41cea80).

Files with missing linesPatch %Lines
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs0.00%5 Missing⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs0.00%5 Missing⚠️
Additional details and impacted files
@@            Coverage Diff             @@##             main    #3399      +/-   ##==========================================- Coverage   68.04%   65.57%   -2.48%==========================================  Files         301      301                Lines       65625    65631       +6     ==========================================- Hits        44654    43036    -1618- Misses      20971    22595    +1624
FlagCoverage Δ
addons92.58% <ø> (ø)
netcore68.73% <16.66%> (-3.67%)⬇️
netfx66.96% <16.66%> (+0.19%)⬆️

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.

@apoorvdeshmukhapoorvdeshmukh added the Triage Needed 🆕For new issues, not triaged yet. labelJun 8, 2025
Copy link
Contributor

@paulmedynskipaulmedynski left a comment

Choose a reason for hiding this comment

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

One suggestion to harden the tests.

paulmedynski
paulmedynski previously approved these changesJun 9, 2025
saurabh500
saurabh500 previously approved these changesJun 9, 2025
@apoorvdeshmukhapoorvdeshmukh added Bug! 🐛Issues that are bugs in the drivers we maintain. and removed Triage Needed 🆕For new issues, not triaged yet. labelsJun 9, 2025
Copy link
Member

@cheenamalhotracheenamalhotra left a comment

Choose a reason for hiding this comment

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

Please fix tests to cleanup tables as well.

Copy link
Contributor

@paulmedynskipaulmedynski left a comment

Choose a reason for hiding this comment

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

Some changes requested.

Copy link
Contributor

@paulmedynskipaulmedynski left a comment

Choose a reason for hiding this comment

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

So close! :)

apoorvdeshmukh reacted with laugh emoji
@cheenamalhotracheenamalhotra merged commite649a7c intomainJun 10, 2025
248 of 250 checks passed
@cheenamalhotracheenamalhotra deleted the dev/ad/3397 branchJune 10, 2025 16:52
apoorvdeshmukh added a commit that referenced this pull requestJun 11, 2025
apoorvdeshmukh added a commit that referenced this pull requestJun 11, 2025
apoorvdeshmukh added a commit that referenced this pull requestJun 11, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

Copilot code reviewCopilotCopilot left review comments

@benrr101benrr101benrr101 approved these changes

@cheenamalhotracheenamalhotracheenamalhotra approved these changes

@paulmedynskipaulmedynskipaulmedynski approved these changes

@saurabh500saurabh500saurabh500 left review comments

Assignees

@apoorvdeshmukhapoorvdeshmukh

Labels
Bug! 🐛Issues that are bugs in the drivers we maintain.
Projects
None yet
Milestone
6.1-preview2
Development

Successfully merging this pull request may close these issues.

SqlBulkCopy streaming ends up prefixing BOM to UTF8 source column with VARCHAR(MAX) type
6 participants
@apoorvdeshmukh@edwardneal@saurabh500@benrr101@cheenamalhotra@paulmedynski
[8]ページ先頭
©2009-2025 Movatter.jp