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.

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

[release/6.0] Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests (#81878) #82062

Conversation

jeffhandley
Copy link
Member

Backport of #81878 to release/6.0

Customer Impact

As reported in #74852, a regression was introduced in .NET 6 with #43379 that prevents SqlXml.CreateReader() from processing SQL Binary XML content (see MS-BINXML).

The regression is caused by seeking beyond the end of the current token during token validation. This is due to a simple mistake of using _end instead of _pos when _end represents the end of the entire buffer and _pos represents the position 1 byte beyond the end of the current token.

We currently have 2 customers reporting that their migrations to .NET 6 are blocked by this regression. While we have not yet obtained their validation that the .NET 8 builds work for them, our collective confidence in the fix is high. We will get their validation as soon as possible, but we wanted to get this fix queued up for the March servicing releases rather than waiting for that validation.

Testing

While the root cause and fix for this regression were well understood since @WaynePaiMS submitted #74852, this issue surfaced a substantial test hole. We did not have any tests validating that SQL Binary XML content could be processed. Such tests existed in .NET Framework, but they had not been ported to .NET Core+.

The fix includes reintroduction of the same collection of tests that were used in .NET Framework, including regression tests that had been added over time. The test cases include collections of SQL Binary and Text XML files. A new unit test processes the test files in both forms, validating that the processed XML matches the results of processing the text form directly through XmlTextReader (without SqlXml.CreateReader()). The tests were reviewed for thoroughness and named to match the scenarios they cover.

The details of those test cases can be seen here:
https://github.com/dotnet/runtime-assets/tree/main/src/System.Data.Common.TestData

Regression

Yes. The regression categorically prevents SQL XML Binary data from being processed, and this is currently blocking customers from migrating from .NET Core 3.1 and .NET 5.0 to .NET 6 or .NET 7.

Risk

Low. SQL Binary XML processing is categorically broken in .NET 6 and .NET 7. The root cause and fix are understood, and the fix introduces a body of tests to ensure SqlXml.CreateReader() produces the same result for SQL Binary XML files as it does for Text XML files, and those tests also ensure that it's processing of Text XML files matches the results of using XmlTextReader directly.

…#81878)

* Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests

* Revert solution file changes made by VS

* Use runtime-assets test files for Text and SQL Binary XML tests

* Remove System.Data.Common.TestData package reference

* Reference the System.Data.Common.TestData package for SqlXml tests

* Add System.Common.Data.TestData to Version.Details.xml for automated dependency flow
@jeffhandley jeffhandley added Servicing-consider Issue for next servicing release review area-System.Xml labels Feb 14, 2023
@jeffhandley jeffhandley added this to the 6.0.x milestone Feb 14, 2023
@jeffhandley jeffhandley self-assigned this Feb 14, 2023
@ghost
Copy link

ghost commented Feb 14, 2023

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #81878 to release/6.0

Customer Impact

As reported in #74852, a regression was introduced in .NET 6 with #43379 that prevents SqlXml.CreateReader() from processing SQL Binary XML content (see MS-BINXML).

The regression is caused by seeking beyond the end of the current token during token validation. This is due to a simple mistake of using _end instead of _pos when _end represents the end of the entire buffer and _pos represents the position 1 byte beyond the end of the current token.

We currently have 2 customers reporting that their migrations to .NET 6 are blocked by this regression. While we have not yet obtained their validation that the .NET 8 builds work for them, our collective confidence in the fix is high. We will get their validation as soon as possible, but we wanted to get this fix queued up for the March servicing releases rather than waiting for that validation.

Testing

While the root cause and fix for this regression were well understood since @WaynePaiMS submitted #74852, this issue surfaced a substantial test hole. We did not have any tests validating that SQL Binary XML content could be processed. Such tests existed in .NET Framework, but they had not been ported to .NET Core+.

The fix includes reintroduction of the same collection of tests that were used in .NET Framework, including regression tests that had been added over time. The test cases include collections of SQL Binary and Text XML files. A new unit test processes the test files in both forms, validating that the processed XML matches the results of processing the text form directly through XmlTextReader (without SqlXml.CreateReader()). The tests were reviewed for thoroughness and named to match the scenarios they cover.

The details of those test cases can be seen here:
https://github.com/dotnet/runtime-assets/tree/main/src/System.Data.Common.TestData

Regression

Yes. The regression categorically prevents SQL XML Binary data from being processed, and this is currently blocking customers from migrating from .NET Core 3.1 and .NET 5.0 to .NET 6 or .NET 7.

Risk

Low. SQL Binary XML processing is categorically broken in .NET 6 and .NET 7. The root cause and fix are understood, and the fix introduces a body of tests to ensure SqlXml.CreateReader() produces the same result for SQL Binary XML files as it does for Text XML files, and those tests also ensure that it's processing of Text XML files matches the results of using XmlTextReader directly.

Author: jeffhandley
Assignees: jeffhandley
Labels:

Servicing-consider, area-System.Xml

Milestone: 6.0.x

@carlossanlop carlossanlop modified the milestones: 6.0.x, 6.0.15 Feb 14, 2023
@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 14, 2023
@carlossanlop
Copy link
Member

Approved by Tactics via email for 6.0.15.
Signed-off by area owners.
No OOB changes needed.
CI failures are known, pre-existing and unrelated: #81914, #81383, #81907, #82097.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit c5b9524 into dotnet:release/6.0 Feb 14, 2023
@jeffhandley jeffhandley deleted the jeffhandley/6.0-xmlsqlbinaryreader branch February 18, 2023 01:32
@jeffhandley
Copy link
Member Author

Update: We received customer validation confirmation today. They tested against net8.0 bits using a daily build from main and confirmed their scenarios are unblocked.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Xml Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants