Skip to content

Fix incorrect "illegal XML comment chars" check & add regression tests - #74787

Merged
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom
GrabYourPitchforks:xml
Aug 30, 2022
Merged

Fix incorrect "illegal XML comment chars" check & add regression tests#74787
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom
GrabYourPitchforks:xml

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Aug 29, 2022

Copy link
Copy Markdown
Member

In PR #69038, we accidentally changed an "ends with" check to a "starts with" check. This fixes that error and improves our regression test coverage. From scanning through the original PR, there were two locations total where it appears we introduced errors into the logic. #71752 (already in the 7.0 RC1 branch) addressed one of them; this PR addresses the remaining location.

Since this is a regression from 6.0 and affects customer scenarios, I'll kick off the backporting process once this PR is merged.

Resolves#74752.

@ghost

ghost commented Aug 29, 2022

Copy link
Copy Markdown

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

Issue Details

In PR #69038, we accidentally changed an "ends with" check to a "starts with" check. This fixes that error and improves our regression test coverage. From scanning through the original PR, there were two locations total where it appears we introduced errors into the logic. #71752 (already in the 7.0 RC1 branch) addressed one of them; this PR addresses the remaining location.

Since this is a regression from 6.0 and affects customer scenarios, I'll kick off the backporting process once this PR is merged.

Resolves #74752.

Author:GrabYourPitchforks
Assignees:-
Labels:

area-System.Xml

Milestone:-

@GrabYourPitchforks

GrabYourPitchforks commented Aug 29, 2022

Copy link
Copy Markdown
MemberAuthor

Side observation, not really realted to this PR: the workhorse method behind WriteRaw is really just a glorified "is the input data well-formed UTF-16?" check. That's something we already have internal within the framework, and making it public would allow considerable simplification of these sorts of methods.

danmoseley
danmoseley reviewed Aug 30, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.Xml/tests/XmlWriter/System.Xml.RW.XmlWriter.Tests.csproj
<Compile Include="WriteWithEncoding.cs" />
<Compile Include="WriteWithEncodingWithFallback.cs" />
<Compile Include="WriteWithInvalidSurrogate.cs" />
<Compile Include="XmlTextWriterTests.cs" />

@danmoseleydanmoseleyAug 30, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this caught my eye :/ .. as far as I can see our test coverage of XmlTextWriter is minimal. Our tests test various other *Xml*Writers. XmlWriter.Create() doesn't seem to create one either. Not an urgent problem, but I suppose it led to this bug.

danmoseley
danmoseley reviewed Aug 30, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.Xml/tests/XmlWriter/XmlTextWriterTests.cs
@eiriktsarpalis

eiriktsarpalis commented Aug 30, 2022

Copy link
Copy Markdown
Member

Since this is a regression from 6.0

Was #69038 backported to 6?

@GrabYourPitchforks

GrabYourPitchforks commented Aug 30, 2022

Copy link
Copy Markdown
MemberAuthor

Was #69038 backported to 6?

@eiriktsarpalis No, this only affects 7.0.

@GrabYourPitchforks

GrabYourPitchforks commented Aug 30, 2022

Copy link
Copy Markdown
MemberAuthor

Per suggestion at #74787 (comment), will rerun the tests tomorrow morning against netfx481 just to validate the behavior. Hold off merging until then.

@GrabYourPitchforksGrabYourPitchforks added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 30, 2022
krwq
krwq reviewed Aug 30, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.Xml/tests/XmlWriter/XmlTextWriterTests.cs
@danmoseley

danmoseley commented Aug 30, 2022

Copy link
Copy Markdown
Contributor

That's something we already have internal within the framework, and making it public would allow considerable simplification of these sorts of methods.

Seems like a reasonable API proposal..

@GrabYourPitchforks

GrabYourPitchforks commented Aug 30, 2022

Copy link
Copy Markdown
MemberAuthor

Tested against 4.8, 3.1, 6.0, and [unpatched] 7.0. Tests succeed on all platforms except the platform where the regression was introduced. So I think we're good for merge!

image

@GrabYourPitchforks

GrabYourPitchforks commented Aug 30, 2022

Copy link
Copy Markdown
MemberAuthor

Test failure is tracked by #74795.

@GrabYourPitchforks
GrabYourPitchforks merged commit f825814 into dotnet:mainAug 30, 2022
@GrabYourPitchforks

GrabYourPitchforks commented Aug 30, 2022

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0

@github-actions

github-actionsBot commented Aug 30, 2022

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2957973010

@github-actionsgithub-actionsBot mentioned this pull request Aug 30, 2022
Merged
@eiriktsarpaliseiriktsarpalis removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 30, 2022
@ghostghost locked as resolved and limited conversation to collaborators Sep 29, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@krwqkrwqkrwq approved these changes
@eiriktsarpaliseiriktsarpaliseiriktsarpalis approved these changes
+1 more reviewer
@danmoseleydanmoseleydanmoseley approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

area-System.Xml

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Regression in the XmlBaseWriter.cs

4 participants

@GrabYourPitchforks@eiriktsarpalis@danmoseley@krwq