-
Notifications
You must be signed in to change notification settings - Fork 295
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve truncation error message #258
Conversation
Ouch 😞 CI isn't public |
@Suchiman we'll help you out with logs till then! Public CI is WIP ✌️ |
src/Microsoft.Data.SqlClient/netcore/src/Resources/SR.Designer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs
Outdated
Show resolved
Hide resolved
Just noticed that the netfx build is a distinct copy.
|
src/Microsoft.Data.SqlClient/netfx/src/Resources/Strings.de.resx
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs
Show resolved
Hide resolved
If always encrypted is used this may potentially leak an unencrypted input value of the field which exceeds the length into exception log files. To avoid that the value print would need to be optional and default to off. |
Good point @Wraith2 . @Suchiman Could you verify if data at this stage is unencrypted/raw when AE is enabled and if yes, then we'll have to take a special handle to that scenario. We should respect client's encryption mode and should not change error message in this case. From server side, string is Encrypted when thrown in error, so that turned acceptable. From our "mocked" error message, we need to be careful. |
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlUtil.cs
Outdated
Show resolved
Hide resolved
The change to the error message to contain the column and length is still very valuable but the truncated value would have to be omitted. |
So in cases where encryption is enabled, should i add a second message without |
Setting it to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Brings the error message to parity with SQL Server 2016+ (trace flag) / 2019 (default).
fixes #256