Detect Unix paths on Unix and Dos&Unc paths on Windows #15925

tmds · 2017-02-07T20:56:51Z

Fixes https://github.com/dotnet/corefx/issues/1745

TODO:

Equals test HashCode check 4a7ce12

tmds · 2017-02-07T21:02:24Z

src/System.Runtime/tests/System/Uri.MethodsTests.cs

+            {
+                // Implicit file
+                yield return new object[] { new Uri("/sharepath/path/file"), new Uri("/sharepath/path/file"), true };
+                yield return new object[] { new Uri("/sharepath/path/file"), new Uri("/sharepath/path/File"), false };


@stephentoub
These two inputs are failing the test (note: file vs File).

yield return new object[] { new Uri("file:///sharepath/path/file"), new Uri("file:///sharepath/path/File"), false }; yield return new object[] { new Uri("/sharepath/path/file"), new Uri("/sharepath/path/File"), false };

The Uri's are not Equal (great!), but the test also expects the HashCode to be the same. Change the test?

…eature/uri-unix-path

stephentoub · 2017-02-08T16:37:36Z

src/Common/tests/Tests/System/Security/IdentityHelperTests.cs

@@ -36,13 +36,23 @@ public void GetNormalizedStrongNameHash()
        }

        [Fact]
-        public void GetNormalizedUrlHash()
+        [PlatformSpecific(TestPlatforms.Windows)]


Nit: @sepidehms just went through all of our platform-specific tests and added comments next to them to indicate why they're platform-specific (#15932). Could you add such comments for these?

stephentoub · 2017-02-08T16:39:28Z

src/Common/tests/Tests/System/Security/IdentityHelperTests.cs

+        public void GetNormalizedUrlHash_Unix()
+        {
+            // Validating that we match the exact hash the desktop IsolatedStorage implementation would create.
+            Uri uri = new Uri(@"file:///Users/jerem/Documents/Visual Studio 2015/Projects/LongPath/LongPath/bin/Debug/TestAssembly.EXE");


This path can be used in the Uri ctor on Windows as well, no?

Right. Will use same test for both.

stephentoub · 2017-02-08T16:40:22Z

src/System.IO.Packaging/tests/Tests.cs

@@ -20,6 +20,8 @@ public class Tests
        private const string Mime_MediaTypeNames_Image_Jpeg = "image/jpeg"; // System.Net.Mime.MediaTypeNames.Image.Jpeg
        private const string s_DocumentXml = @"<Hello>Test</Hello>";
        private const string s_ResourceXml = @"<Resource>Test</Resource>";
+        private static readonly bool s_IsWindowsSystem = Path.DirectorySeparatorChar == '\\';
+        private static readonly string s_LocalFile = s_IsWindowsSystem ? @"c:/resources/image1.jpg" : "/resources/image1.jpg";


Nit: s_LocalFile => s_localFile (or maybe s_fullPathToLocalFile, or something like that)

stephentoub · 2017-02-08T16:41:07Z

src/System.Net.Requests/tests/FileWebRequestTest.cs

@@ -80,7 +80,7 @@ public void GetRequestResponseAfterAbort_Throws()
        [Fact]
        public void NotImplementedMembers_Throws()
        {
-            WebRequest request = WebRequest.Create("file://anything");
+            WebRequest request = WebRequest.Create("file:///anything");


These paths previously worked on both Windows and Unix without the third slash. Is that changing?

This changed. On Unix, Unix paths must start with a '/' on Windows the behavior is unchanged.
00a1a71#diff-06f80dbdb7963930a25fb8a935d08e0dR2193

Why does that have to change? I'm concerned about this being a breaking change. Are there no valid situations someone could have been using this today without the extra '/'?

Why does that have to change? I'm concerned about this being a breaking change. Are there no valid situations someone could have been using this today without the extra '/'?

I wrote the requirement more strict at first. The path could not be empty and had to start with a slash '/'. At that point, I updated the tests. I then relaxed it to, if there is a path it must start with '/'.

file://anything -> host=anything, path=/ file:///anything -> host=, path=/anything

So the tests will now also work with 'file://anything'.

So there are tests for these cases as well? The below are also valid on Unix..

file:// -> host=, path=, AbsoluteUri should be file:///
file:/// -> host=, path=/, AbsoluteUri should be file:///

@Priya91 these are tested here: https://github.com/dotnet/corefx/pull/15925/files#diff-37ac644755d2501e671ac255288654a3R715

+ // Explicit with empty host and empty path + yield return new object[] { "file://", "/", "", "" }; + // Explicit with empty host and non-empty path + yield return new object[] { "file:///", "/", "", "" }; + yield return new object[] { "file:////", "//", "", "" };

I'll revert the change of unix paths having to start with '/'. (For implicit paths, this is the case still.) But for file:// uris, a backslash will do to. The Uri class will convert these backslashes to forwardslashes.
e.g.:

/one\two -> path = /one\two file://\one\two -> path = /one/two file:///one%5Ctwo -> path = /one\two

stephentoub · 2017-02-08T16:45:33Z

src/System.Private.Uri/src/System/Uri.cs

+                            _flags |= Flags.UnixPath;
+                        }
+                        //Windows: UNC path?
+                        else if (IsWindowsSystem && _syntax.InFact(UriSyntaxFlags.FileLikeUri) && (i - idx >= 2 && i - idx != 3 &&


I believe Roslyn will effectively trim away the dead branches based on it seeing that IsWindowsSystem is hardcoded to be true or false. Can you double-check the IL just to confirm? If it doesn't, we may want to rethink this.

Checked. Roslyn trims away the dead branches.

Great, thanks.

stephentoub · 2017-02-08T16:50:19Z

src/System.Private.Uri/src/System/Uri.cs

@@ -2177,6 +2190,12 @@ private unsafe ParsingError PrivateParseMinimal()
                    if (err != ParsingError.None)
                        return err;

+                    // Unix paths must start with '/'
+                    if (InFact(Flags.UnixPath) && idx < (ushort)length && pUriString[idx] != '/')


InFact(Flags.UnixPath) will only ever return true if we're on a unix system, right (since otherwise it wouldn't have been recognized)? If so, we may want to preface this check with !IsWindowsSystem &&... that way, assuming my previous comment about Roslyn is correct, this whole if statement would end up being compiled away on Windows and would only be in the Unix binary.

(Same comment applies to other similar situations)

stephentoub · 2017-02-08T16:54:18Z

src/System.Private.Uri/src/System/Uri.cs

@@ -3648,8 +3667,8 @@ private unsafe void ParseRemaining()
                }
            }

-            //NB: A string must have at least 3 characters and at least 1 before ':'
-            if (idx + 2 >= length || end == idx)
+            //NB: A string must have at least 3(Windows)/1(Unix) characters and at least 1 before ':'


Nit: I assume the 3/1 chars being referred to here are:

On Unix the opening slash

On Windows the opening drive letter, colon, and slash?

If so, might be worth clarifying that further in the comment. If not, I'm obviously not understanding, so it'd be worth spelling it out :)

stephentoub · 2017-02-08T16:58:51Z

src/System.Private.Uri/tests/FunctionalTests/IriTest.cs

@@ -184,7 +184,7 @@ public void Iri_IncompleteEscapedCharacter()
        [Fact]
        public void Iri_ReservedCharacters()
        {
-            string input = "/?#??#%[]";
+            string input = "?/#??#%[]";


This changed because the original input would trigger recognition as a unix path? Is there any concern about such things in the wild?

This would actually cause an IndexOutOfRange exception to be thrown. I investigated it further now, and the cause was newHost being an empty string instead of null.
Reverted the IriTest and fixed here: tmds@c13d66a.

stephentoub · 2017-02-08T16:59:54Z

src/System.Private.Uri/tests/FunctionalTests/UriTests.cs


 namespace System.PrivateUri.Tests
 {
    public static class UriTests
    {
+        private static readonly bool s_IsWindowsSystem = Path.DirectorySeparatorChar == '\\';


In other test suites, we've used RuntimeInformation for this instead of adding such statics, e.g. RuntimeInformation.IsOSPlatform(OSPlatform.Windows)

You can also use PlatformDetection.IsWindows, include it as a linked filed from Common\test\System location.

stephentoub · 2017-02-08T17:01:43Z

src/System.Private.Uri/tests/FunctionalTests/UriTests.cs

+                uri = new Uri(@"file://c:/directory/filename");
+                b = uri.IsWellFormedOriginalString();
+                Assert.False(b);
+            }


Can/should we validate the inverse on Unix, that these aren't recognized as file paths?

Though related to some of my previous comments, I also wonder whether we can/should try to make the implementation recognize, on unix, unix file paths in addition to rather than instead of windows ones.

The tests verify these are invalid Uris on Unix (https://github.com/dotnet/corefx/pull/15925/files#diff-37ac644755d2501e671ac255288654a3R1295) The constructor throws for these strings.

+ // Implicit Windows drive + yield return new object[] { @"c:\", UriKind.Absolute }; + yield return new object[] { @"c:\path\file", UriKind.Absolute }; + + // Implicit UNC + yield return new object[] { @"\\unchost", UriKind.Absolute }; + yield return new object[] { @"\\unchost\", UriKind.Absolute }; + yield return new object[] { @"\\unchost\path\file", UriKind.Absolute };

I also wonder whether we can/should try to make the implementation recognize, on unix, unix file paths in addition to rather than instead of windows ones.

Technically it is possible. I doubt whether it will be used.

Technically it is possible. I doubt whether it will be used.

It may not be, or at least not on purpose. But since such code would have worked on desktop and on .NET Core 1.x on Unix, if we can support it, it could be good, too. What does Mono do with such paths on Unix?

stephentoub · 2017-02-08T17:02:25Z

src/System.Private.Xml/tests/XmlSchema/XmlSchemaValidatorApi/PropertiesTests.cs

+            else
+            {
+                yield return new object[] { "/some/path" };
+            }


Nit: could be:

yield return new object[] { s_IsWindowsSystem ? "\\\\wddata\\some\\path" : "/some/path" };

stephentoub · 2017-02-18T15:35:03Z

let me know if you if I should add support for Dos paths? and Unc paths starting with '\' and '/'?

Let's try to make this fix as additive-only as possible, i.e. remove as little existing behavior as possible to add support for recognizing Unix file paths. Especially since Mono supports those things, I would like us not to drop such support unless it's absolutely necessary (and it sounds like it's not).

…eature/uri-unix-path

…path Conflicts: src/System.Runtime/tests/System/Uri.CreateStringTests.cs

Priya91 · 2017-02-21T19:23:52Z

@tmds There are some merge commits in the PR history, can you please clean them up.

stephentoub · 2017-02-21T19:25:22Z

There are some merge commits in the PR history, can you please clean them up.

Once we're all happy with the PR, we can merge it via "squash and merge" rather than "create a merge commit" to get rid of them.

tmds · 2017-02-22T11:54:37Z

Making it additive and doing cleanup made the PR much more digestible.
@stephentoub @Priya91 @CIPop @davidsh ptal

stephentoub · 2017-02-25T14:20:08Z

@dotnet-bot test this please

stephentoub · 2017-02-25T16:25:29Z

src/System.Private.Uri/tests/FunctionalTests/IriRelativeFileResolutionTest.cs

@@ -81,6 +83,7 @@ public void IriRelativeResolution_CompareImplcitAndExplicitUncWithNoUnicode_AllP
        }

        [Fact]
+        [PlatformSpecific(TestPlatforms.Windows)] // Unc paths must start with '\' on Unix


// Unc paths must start with \ on Unix

Is this how Mono does it as well?

Yes. fyi, the check is here: https://github.com/mono/mono/blob/master/mcs/class/referencesource/System/net/System/URI.cs#L3686

stephentoub

Looks great. Thanks for working on this, @tmds.

stephentoub · 2017-02-25T16:41:52Z

@dotnet-bot test Outerloop Windows_NT Debug please
@dotnet-bot test Outerloop Ubuntu14.04 Debug please
@dotnet-bot test Outerloop OSX Debug please

tmds · 2017-02-25T18:25:14Z

Looks great. Thanks for working on this, @tmds.

Thanks. Making it additive and some refactoring really improved the PR. Will someone else do a review also?

stephentoub · 2017-02-25T23:34:54Z

@geoffkizer, @CIPop, @JeremyKuhne, can one or more of you review as well?

geoffkizer · 2017-02-28T07:45:31Z

src/System.Private.Uri/src/System/Uri.cs

@@ -95,6 +95,7 @@ private enum Flags : ulong
                                                    // and Host values in case of a custom user Parser
            DosPath = 0x08000000,
            UncPath = 0x10000000,
+            UnixPath = 0x100000000000,


This should be listed in order.

geoffkizer · 2017-02-28T07:53:16Z

src/System.Private.Uri/tests/FunctionalTests/IriRelativeFileResolutionTest.cs

@@ -45,8 +47,8 @@ public void IriRelativeResolution_CompareImplcitAndExplicitFileWithReservedChar_
        [Fact]
        public void IriRelativeResolution_CompareImplcitAndExplicitFileWithUnicodeIriOn_AllPropertiesTheSame()
        {
-            string unicodeImplicitTestFile = @"c:\path\\u30AF\path3\\u30EB\u30DE.text";
-            string nonUnicodeImplicitFileBase = @"c:\path\file.txt";
+            string unicodeImplicitTestFile = s_isWindowsSystem ? @"c:\path\\u30AF\path3\\u30EB\u30DE.text" : "/path//u30AF/path3//u30EB/u30DE.text";


I have to say, I'm confused by this test. What's the point of the \u30AF etc stuff if it's not meant to be a Unicode char?

Probably it should be Unicode. Shall I change it?

I'm not sure. What's the intent of the test? Is there history that shows that this was intended to test Unicode chars? Maybe @CIPop or @davidsh understand this better?

stephentoub · 2017-03-03T00:33:50Z

@geoffkizer, @CIPop, @JeremyKuhne, are you guys good with these changes?

stephentoub · 2017-03-04T21:37:03Z

Ok, given the lack of further responses in the last week, I'm going to go ahead with this.

@tmds, thanks for working on this!

…#15925) * Uri: detect Unix path on Unix, detect Dos&Unc path on Windows * Update System.IO.Packaging/tests * Update System.Xml.XmlSchema.XmlSchemaValidatorApi.Tests * Update Common.Tests * Update System.Private.Uri.Functional.Tests * Update System.Private.Uri.ExtendedFunctional.Tests * Update System.Runtime/tests * Uri.MethodsTests.cs: comment out failing inputs * Restore uri minimal length of 3 * Require unix paths to start with a / * Allow Unix path file uri to start with '\' * FileWebRequestTest: revert extra slashes added (file:/// back to file://) * Uri: fix empty newHost bug and revert workaround in IriTest * Annotate PlatformSpecific tests * IdentityHelperTests: use file path that works on Windows and Linux * System.IO.Packaging/tests: rename s_LocalFile to s_fullPathToLocalFile * Uri: update comments on minimal Uri length check, don't check length twice on Windows * Use PlatformDetection.IsWindows * Ensure Roslyn gets rid of UnixPath checks on Windows by adding IsWindowsSystem (const bool) checks * Uri: Fix uapaot build by including Uri.Windows.cs * IdentityHelperTests: rename GetNormalizedUrlHash_Unix -> GetNormalizedUrlHash * Uri.MethodsTests: don't require GetHashCode to return different values when Uris only differ in case * s_IsWindowsSystem -> s_isWindowsSystem * Detect Dos and Unc paths on Unix * Update tests * Clean-up * Clean-up II * Clean-up III * Clean-up IV * Clean-up V * Move Flags.UnixPath to the end of the Flags enum Commit migrated from dotnet/corefx@7df6b74

tmds added 7 commits February 3, 2017 14:23

Uri: detect Unix path on Unix, detect Dos&Unc path on Windows

3c0ab8a

Update System.IO.Packaging/tests

36bf57b

Update System.Xml.XmlSchema.XmlSchemaValidatorApi.Tests

ee379cc

Update Common.Tests

33150f0

Update System.Private.Uri.Functional.Tests

6289145

Update System.Private.Uri.ExtendedFunctional.Tests

8d966b8

Update System.Runtime/tests

71f06b2

dnfclas added the cla-already-signed label Feb 7, 2017

tmds commented Feb 7, 2017

View reviewed changes

Uri.MethodsTests.cs: comment out failing inputs

4a7ce12

danmoseley added the area-System.Net label Feb 7, 2017

tmds added 3 commits February 8, 2017 08:17

Restore uri minimal length of 3

7897d8a

Merge branch 'feature/uri-unix-path' of github.com:tmds/corefx into f…

015755d

…eature/uri-unix-path

Require unix paths to start with a /

00a1a71

karelz assigned tmds, davidsh, Priya91, CIPop and geoffkizer Feb 8, 2017

stephentoub reviewed Feb 8, 2017

View reviewed changes

tmds added 8 commits February 20, 2017 08:55

s_IsWindowsSystem -> s_isWindowsSystem

bdd7917

Detect Dos and Unc paths on Unix

add4468

Update tests

c562425

Merge branch 'feature/uri-unix-path' of github.com:tmds/corefx into f…

6a7a3d3

…eature/uri-unix-path

Merge remote-tracking branch 'upstream/master' into feature/uri-unix-…

d3bc5dd

…path Conflicts: src/System.Runtime/tests/System/Uri.CreateStringTests.cs

Clean-up

e3c9fe6

Clean-up II

67b562f

Clean-up III

a8f2072

tmds added 2 commits February 21, 2017 22:46

Clean-up IV

0cd747f

Clean-up V

c641db8

stephentoub reviewed Feb 25, 2017

View reviewed changes

stephentoub approved these changes Feb 25, 2017

View reviewed changes

geoffkizer reviewed Feb 28, 2017

View reviewed changes

Move Flags.UnixPath to the end of the Flags enum

c0c6c5c

stephentoub merged commit 7df6b74 into dotnet:master Mar 4, 2017

karelz modified the milestone: 2.0.0 Mar 7, 2017

tmds mentioned this pull request Jun 2, 2017

XmlResolver: remove workaround for lack of Unix path support in Uri #20564

Closed

Detect Unix paths on Unix and Dos&Unc paths on Windows #15925

Detect Unix paths on Unix and Dos&Unc paths on Windows #15925

Conversation

tmds commented Feb 7, 2017 • edited

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

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

stephentoub Feb 8, 2017 • edited

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

stephentoub Feb 8, 2017 • edited

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

Choose a reason for hiding this comment

stephentoub commented Feb 18, 2017

Priya91 commented Feb 21, 2017

stephentoub commented Feb 21, 2017

tmds commented Feb 22, 2017

stephentoub commented Feb 25, 2017

stephentoub Feb 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

stephentoub commented Feb 25, 2017

tmds commented Feb 25, 2017

stephentoub commented Feb 25, 2017

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

stephentoub commented Mar 3, 2017

stephentoub commented Mar 4, 2017

tmds commented Feb 7, 2017 •

edited

stephentoub Feb 8, 2017 •

edited

stephentoub Feb 8, 2017 •

edited

stephentoub Feb 25, 2017 •

edited