Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Improved Sql bulk copy error message #437

Merged
merged 12 commits into from
Feb 29, 2020
Merged

Improved Sql bulk copy error message #437

merged 12 commits into from
Feb 29, 2020

Conversation

DavoudEshtehari
Copy link
Contributor

@DavoudEshtehari DavoudEshtehari commented Feb 20, 2020 •
edited
Loading

Improved the error message for wrong data type conversion with adding more detail about the source of error in the sqlBulkCopy.
In this change, the error message of converting a string to a numerical column is handled and the complementary information about the column name and the row number of the data source is added.
We brought this additional information for DataTable and DataRow[]. For the rest of the datatype supported by SqlBulkCopy, it will add in the next improvements.

@dnfclas
Copy link

dnfclas commented Feb 20, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@DavoudEshtehari
Improved sqlBulkCopy data conversion's error message
339c7e6 
support RowID for DataTable and DataRow[]
@DavoudEshtehari DavoudEshtehari force-pushed the bulkCopyError branch 2 times, most recently from f0af312 to d8253f5 Compare February 22, 2020 02:37
@cheenamalhotra cheenamalhotra marked this pull request as ready for review February 26, 2020 01:09
@DavoudEshtehari DavoudEshtehari changed the title Bulk copy error Improved Sql bulk copy error message Feb 26, 2020
cheenamalhotra
cheenamalhotra reviewed Feb 26, 2020
View reviewed changes
Copy link
Member

@cheenamalhotra cheenamalhotra left a comment

Choose a reason for hiding this comment

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

Overall looks good!
Would really like to see same for SqlDataReader and IDataReader if possible as they're widely used.

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/BulkCopyAEErrorMessage.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/BulkCopyAEErrorMessage.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/BulkCopyAEErrorMessage.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/BulkCopyAEErrorMessage.cs Outdated Show resolved Hide resolved
...osoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/DataConversionErrorMessageTest.cs Outdated Show resolved Hide resolved
@cheenamalhotra cheenamalhotra added this to the 2.0.0-preview2 milestone Feb 26, 2020
@DavoudEshtehari DavoudEshtehari force-pushed the bulkCopyError branch 2 times, most recently from 71d6900 to f9de9e2 Compare February 27, 2020 16:54
@DavoudEshtehari DavoudEshtehari force-pushed the bulkCopyError branch 4 times, most recently from f04f5f0 to fa22b29 Compare February 28, 2020 01:33
@ErikEJ
Copy link
Contributor

ErikEJ commented Feb 28, 2020

Add an .editorconfig file?

@cheenamalhotra
Copy link
Member

cheenamalhotra commented Feb 28, 2020 •
edited
Loading

Thanks @ErikEJ
We have file in root, but there is no such setting that enforces use of braces in if-else.

Ref: Editorconfig formatting conventions

@DavoudEshtehari
Improved bulk copy unit tests
928d023 
plus support short date issue at Linux
@ErikEJ
Copy link
Contributor

ErikEJ commented Feb 28, 2020

It is enforced for us - maybe via a StyleCop rule then?

@DavoudEshtehari DavoudEshtehari merged this pull request into dotnet:master Feb 29, 2020
cheenamalhotra pushed a commit to cheenamalhotra/SqlClient that referenced this pull request Mar 3, 2020
@DavoudEshtehari
Improved Sql bulk copy error message (dotnet#437)
0dc60ad
@DavoudEshtehari DavoudEshtehari deleted the bulkCopyError branch March 4, 2020 20:05
@cheenamalhotra cheenamalhotra mentioned this pull request Apr 30, 2020
Closed
@Berreek Berreek mentioned this pull request Mar 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JRahnama JRahnama JRahnama left review comments

@David-Engel David-Engel David-Engel approved these changes

@cheenamalhotra cheenamalhotra cheenamalhotra approved these changes

@karinazhou karinazhou Awaiting requested review from karinazhou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0.0-preview2
Development

Successfully merging this pull request may close these issues.
6 participants
@DavoudEshtehari @dnfclas @ErikEJ @cheenamalhotra @David-Engel @JRahnama