Port#3399 to release/5.2 #3407
Open
Port#3399 to release/5.2 #3407
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR ports the BOM-free UTF-8 fix (#3399) into both .NET Framework and .NET Core parsers, and adds a manual test to validate UTF-8 bulk copy behavior.
- Introduces
s_utf8EncodingWithoutBominTdsParserfor netfx and netcore and replaces allEncoding.UTF8uses. - Adds
TestBulkCopyWithUTF8manual test and includes it in the test project. - Introduces
DataTestUtility.CreateTablehelper for table setup in tests.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/.../SqlBulkCopyTest/TestBulkCopyWithUTF8.cs | New manual test for UTF-8 behavior |
| src/.../ManualTesting.Tests.csproj | Included the new test in the project file |
| src/.../DataCommon/DataTestUtility.cs | Added CreateTable helper |
| src/.../netfx/src/.../TdsParser.cs | Introduced non-BOM UTF8 encoding and replaced uses |
| src/.../netcore/src/.../TdsParser.cs | Introduced non-BOM UTF8 encoding and replaced uses |
Comments suppressed due to low confidence (4)
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/TestBulkCopyWithUTF8.cs:12
- [nitpick] Class name casing is inconsistent with the file name (
TestBulkCopyWithUTF8.cs). Consider renaming toTestBulkCopyWithUTF8for clarity and consistency.
public sealed class TestBulkCopyWithUtf8 : IDisposable
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/TestBulkCopyWithUTF8.cs:112
- xUnit does not provide
Assert.Fail. To explicitly fail, useAssert.True(false, message)or throw aXunitExceptionwith the failure message.
Assert.Fail($"Bulk copy failed: {ex.Message}");
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/TestBulkCopyWithUTF8.cs:176
- xUnit does not provide
Assert.Fail. To explicitly fail, useAssert.True(false, message)or throw aXunitExceptionwith the failure message.
Assert.Fail($"Bulk copy failed: {ex.Message}");
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs:593
- Building SQL with raw
tableNameconcatenation can risk SQL injection or invalid identifiers. Consider wrappingtableNameinQUOTENAMEor otherwise quoting it safely.
public static void CreateTable(SqlConnection sqlConnection, string tableName, string createBody)
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs
Show resolved
Hide resolved
apoorvdeshmukh
force-pushed
the
dev/ad/port3399
branch
from
949fda2 to
c808346
Compare
apoorvdeshmukh
changed the title
apoorvdeshmukh
force-pushed
the
dev/ad/port3399
branch
from
c808346 to
334a636
Compare
apoorvdeshmukh
changed the title
benrr101
approved these changes
Jun 11, 2025
paulmedynski
requested changes
Jul 4, 2025
| @@ -0,0 +1,196 @@ | |||
| using System; | |||
There was a problem hiding this comment.
Missing license preamble.
| DataTestUtility.CreateTable(connection, destinationTable, columnDefinition); | ||
| using SqlCommand insertCommand = connection.CreateCommand(); | ||
| insertCommand.CommandText = insertQuery; | ||
| Helpers.TryExecute(insertCommand, insertQuery); |
There was a problem hiding this comment.
CI says Helpers is not found for some builds.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a port of #3399 for release/5.2
Issues
#3397
Testing
This port also contains the testcase to validate the fix.
Additional Notes
For local validation on 5.2,
Have net 6.0 installed. Build all targets (BuildAllConfigurations, buildtestsnetcore) with -p:TF=net60 so that ManualTesting.dll is correctly generated.