- Notifications
You must be signed in to change notification settings - Fork311
Change TryReadString to use TryReadBytesWithContinue#3422
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
cheenamalhotra left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Small change to remove old cleanup query, rest LGTM!
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.
Low stakes comments on the test more than anything
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
6f66457
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
…est/DataReaderTest.cs
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
codecovbot commentedJun 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #3422 +/- ##==========================================- Coverage 68.43% 62.70% -5.73%========================================== Files 299 285 -14 Lines 65418 63321 -2097 ==========================================- Hits 44766 39706 -5060- Misses 20652 23615 +2963
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
44762d9
intodotnet:mainUh oh!
There was an error while loading.Please reload this page.
Fixes#3421
Digging into the replication provided I found that if the data type of the string column were changed from
ntext
tonvarchar(max)
everything worked as expected. The reason for this is that the two types are handled differently internally despite being value equivalent to the user. nvarchar() is a plp type and ntext is not. non-plp strings pass through the TryReadString function and it was tripping up when hitting the continue point because it had not been altered to deal with it.TryReadString was using TryReadByteArray and in previous work I have written a version of this which encapsulates continue capable logic, TryReadByteArrayWithContinue. I moved this method down from TdsParser to TdsParserStateObject so it sits at the same level as the original method and then called it from TryReadString. This resolves the problem with minimal changes and no new code.
/cc@ErikEJ@Kobuntu