Use streamreader instead of string to load xml #1004

martinscholz83 · 2016-09-08T08:06:59Z

fixes #985

If you have URL characters in your project path like C:\Project\Project%20\proj.csproj XMLTextReader.Read() method try to unescape these characters. In result the build is failed because it doesn't find the path. Workaround is to load xml from StreamReader instead of string.

cdmihai · 2016-09-09T00:13:35Z

I wonder if the tests fail because the stream is not disposed, and thus the file handle not released. Can you try adding the stream in the using statement?

rainersigwald · 2016-09-12T15:02:53Z

So . . . was the old way doing encoding detection based on XML magic, and the new way now isn't? Or is there another explanation for those test failures?

martinscholz83 · 2016-09-15T08:59:58Z

@rainersigwald, for now we use XmlTextReader to import the xml. XmlTextReader has the ability to read the encoding from string. But since we now read from a stream it can't read the encoding and use a default one. This is why these tests are failing.

martinscholz83 · 2016-09-15T09:02:19Z

I have added a commit to change from XmlTextReader to XmlReader. I've tested with url characters like %20 and all works fine

Because we now read from a stream we should use XmlReader instead of XmlTextReader and get encoding with `GetAttribute()`.

cdmihai · 2016-09-15T18:24:14Z

@dotnet-bot test this please

rainersigwald · 2016-09-15T18:48:02Z

Ah, nice detective work @maddin2016.

Also

Starting with the .NET Framework 2.0, we recommend that you use the System.Xml.XmlReader class instead.

😆 Well, that class was deprecated so recently!

martinscholz83 · 2016-09-15T18:52:26Z

👍 😉

In XMLReader `DtdProcessing` property is readonly and had to be set in constructor

enable BOM for StreamReader and use this as default encoding instead of `Default.Encoding`

change unit tests that they work with the new XMLReader

rainersigwald · 2016-09-16T15:45:39Z

src/XMakeBuildEngine/Construction/ProjectRootElement.cs

+                    XmlReaderSettings dtdSettings = new XmlReaderSettings();
+                    dtdSettings.DtdProcessing = DtdProcessing.Ignore;
+
+                    using (var stream = new StreamReader(fullPath, true))


@AndyGerlicher the bool here is for BOM detection, which may be relevant in the future make-assumptions-about-content-without-explicit-setting stuff interesting. I don't think it's wrong, but I do hope we can just get to the UTF-8 everywhere model.

I stumbled upon this here where encoding is added with BOM and we should be able to read this encoding instead of using a default one.

rainersigwald · 2016-09-16T16:07:05Z

src/XMakeBuildEngine/UnitTests/Construction/ElementLocation_Tests.cs

@@ -112,7 +112,7 @@ public void TestLargeElementLocationUsedLargeColumn()
            }
            catch (InvalidProjectFileException ex)
            {
-                Assert.Equal(70012, ex.ColumnNumber);
+                Assert.Equal(1, ex.ColumnNumber);


These results are wrong, though, aren't they? In xplat we have them disabled for netcore.

Do you mean ex.ColumnNumber should be 70012?

It sure seems like it--we're deliberately inserting 70k spaces and no newline, so I'd expect that element to start around column 70k.

Ahh...ok, it wasn't clear to me what column exactly means.

So it's a bug?

Yeah, it seems like a bug to me. But I don't know where, exactly.

These tests were disabled because of same error like in dotnet#270

…-StreamReader-For-Load-XML

This just ignores purely whitespace text nodes which seem to be part of the parsing in .NET Core and newer versions of System.Xml Closes dotnet#270, fixes test failures in dotnet#1004

This just ignores purely whitespace text nodes. The XML reader only looks at the first 4k of text to determine if it should be ignored. Our tests are generating 70k of whitespace so the XML reader gives up after 4k and assumes its a text node. Closes #270 Fixes test failures in #1004 when this change is ported to master.

Manual port of 96e6a30 from xplat Fixes test issue in #1004

rainersigwald · 2016-10-12T18:42:52Z

@dotnet-bot test this please

(the tests should fail after #1189 made the location stuff work again)

jeffkl · 2016-10-12T18:47:24Z

Actually, I think the tests were disabled in 430a051. @maddin2016 can you undo your changes to the ElementLocation_Tests.cs file? The tests should be fixed now via #1189.

This reverts commit 430a051.

rainersigwald · 2016-10-12T19:03:02Z

@maddin2016 also revert decd351 please--the current state should fail since the column-number problem should now be fixed.

This reverts commit decd351.

rainersigwald

Looks good. Let's see those tests pass!

martinscholz83 · 2016-10-12T20:42:37Z

🙌 🎉

jeffkl · 2016-10-12T21:09:20Z

Thanks a lot for this contribution!

When dotnet#1004 moved the standard XML reading approach to be stream-based rather than file-based, the exceptions thrown on malformed XML changed--System.Xml no longer knows the path, so it isn't included in the XmlException. That caused MSBuild to fail to report the location of the XML error in a nice way as it had done before. Almost every case where we constructed a BuildEventFileInfo object already had access to the full path, so I added a constructor that accepted that as an argument and overrides the possibly-empty path returned from XmlException.SourceUri. Fixes dotnet#1286.

When #1004 moved the standard XML reading approach to be stream-based rather than file-based, the exceptions thrown on malformed XML changed--System.Xml no longer knows the path, so it isn't included in the XmlException. That caused MSBuild to fail to report the location of the XML error in a nice way as it had done before. Almost every case where we constructed a BuildEventFileInfo object already had access to the full path, so I added a constructor that accepted that as an argument and overrides the possibly-empty path returned from XmlException.SourceUri. Added a unit test to verify that the information is indeed preserved in the exception. Fixes #1286.

Background: * Previous implementation on Full Framework used XmlTextReader(path). This contained an issue with certain characters (dotnet#985) and was fixed by using streams (dotnet#1004). dotnet#1004 also changed from XmlTextReader to XmlReader. * XmlReader contains logic to normalize line endings. Internally, it sets the Normalize property to true and replaces (some? all?) \r\n with \n. This change switches implementation to use XmlTextReader. This class sets the internal Normalize to false and does not replace \r\n with \n. This fixes dotnet#1340. However, .NET Core does not ship with XmlTextReader, only XmlReader. dotnet#1340 still exists for .NET Core.

Background: * Previous implementation on Full Framework used XmlTextReader(path). This contained an issue with certain characters (#985) and was fixed by using streams (#1004). #1004 also changed from XmlTextReader to XmlReader. * XmlReader contains logic to normalize line endings. Internally, it sets the Normalize property to true and replaces (some? all?) \r\n with \n. This change switches implementation to use XmlTextReader. This class sets the internal Normalize to false and does not replace \r\n with \n. This fixes #1340. However, .NET Core does not ship with XmlTextReader, only XmlReader. #1340 still exists for .NET Core.

dnfclas added the cla-already-signed label Sep 8, 2016

Sarabeth-Jaffe-Microsoft added the Needs Review label Sep 8, 2016

martinscholz83 force-pushed the Use-StreamReader-For-Load-XML branch 3 times, most recently from b05d91a to b2c2a24 Compare September 9, 2016 08:23

martinscholz83 force-pushed the Use-StreamReader-For-Load-XML branch from b2c2a24 to 75afb0d Compare September 13, 2016 12:34

martinscholz83 added 4 commits September 15, 2016 11:33

Use streamreader instead of string to load xml

7790e70

add stream to using statement

305eb9a

add streamreader to ProjectRootElementCache

f18df5c

use XmlReader

24fd7e3

Because we now read from a stream we should use XmlReader instead of XmlTextReader and get encoding with `GetAttribute()`.

martinscholz83 force-pushed the Use-StreamReader-For-Load-XML branch from d266928 to 24fd7e3 Compare September 15, 2016 09:33

martinscholz83 added 3 commits September 16, 2016 09:50

add DtdProcessing to XMLReader Constructor

44aafc9

In XMLReader `DtdProcessing` property is readonly and had to be set in constructor

enable BOM for StreamReader

28bc2fe

enable BOM for StreamReader and use this as default encoding instead of `Default.Encoding`

change unit tests

decd351

change unit tests that they work with the new XMLReader

martinscholz83 force-pushed the Use-StreamReader-For-Load-XML branch from a813a36 to decd351 Compare September 16, 2016 10:35

rainersigwald mentioned this pull request Sep 16, 2016

Support preserving formatting when opening a project #1036

Merged

rainersigwald reviewed Sep 16, 2016

View reviewed changes

Disable LargeElemntLocation tests

430a051

These tests were disabled because of same error like in dotnet#270

martinscholz83 force-pushed the Use-StreamReader-For-Load-XML branch from 3549479 to 430a051 Compare September 19, 2016 06:27

Merge remote-tracking branch 'refs/remotes/Microsoft/master' into Use…

eed0ec4

…-StreamReader-For-Load-XML

jeffkl mentioned this pull request Oct 12, 2016

Ignore whitespace text nodes when parsing projects #1187

Merged

jeffkl added a commit that referenced this pull request Oct 12, 2016

Ignore whitespace text nodes when parsing projects (#1189)

27a6e05

Manual port of 96e6a30 from xplat Fixes test issue in #1004

Revert "Disable LargeElemntLocation tests"

89abb08

This reverts commit 430a051.

Revert "change unit tests"

7661ec6

This reverts commit decd351.

rainersigwald approved these changes Oct 12, 2016

View reviewed changes

jeffkl merged commit 33761a5 into dotnet:master Oct 12, 2016

Sarabeth-Jaffe-Microsoft removed the Needs Review label Oct 12, 2016

rainersigwald mentioned this pull request Oct 31, 2016

Malformed-XML error messages no longer have filename + location #1286

Closed

rainersigwald mentioned this pull request Oct 31, 2016

Keep path in invalid project exceptions #1287

Merged

AndyGerlicher mentioned this pull request Nov 22, 2016

Fix issue with xml file line ending normalization #1378

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use streamreader instead of string to load xml #1004

Use streamreader instead of string to load xml #1004

martinscholz83 commented Sep 8, 2016

cdmihai commented Sep 9, 2016 •

edited

Loading

rainersigwald commented Sep 12, 2016

martinscholz83 commented Sep 15, 2016

martinscholz83 commented Sep 15, 2016

cdmihai commented Sep 15, 2016

rainersigwald commented Sep 15, 2016

martinscholz83 commented Sep 15, 2016

rainersigwald Sep 16, 2016

martinscholz83 Sep 16, 2016

rainersigwald Sep 16, 2016

martinscholz83 Sep 16, 2016

rainersigwald Sep 16, 2016

martinscholz83 Sep 16, 2016

martinscholz83 Sep 16, 2016

rainersigwald Sep 16, 2016

rainersigwald commented Oct 12, 2016

jeffkl commented Oct 12, 2016

rainersigwald commented Oct 12, 2016

rainersigwald left a comment

martinscholz83 commented Oct 12, 2016

jeffkl commented Oct 12, 2016

Use streamreader instead of string to load xml #1004

Use streamreader instead of string to load xml #1004

Conversation

martinscholz83 commented Sep 8, 2016

cdmihai commented Sep 9, 2016 • edited Loading

rainersigwald commented Sep 12, 2016

martinscholz83 commented Sep 15, 2016

martinscholz83 commented Sep 15, 2016

cdmihai commented Sep 15, 2016

rainersigwald commented Sep 15, 2016

martinscholz83 commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald commented Oct 12, 2016

jeffkl commented Oct 12, 2016

rainersigwald commented Oct 12, 2016

rainersigwald left a comment

Choose a reason for hiding this comment

martinscholz83 commented Oct 12, 2016

jeffkl commented Oct 12, 2016

cdmihai commented Sep 9, 2016 •

edited

Loading