62606 Return fully qualified path from FileSystemEventArgs (#63051)

slask · web-flow · commit 2a1b15d5c0bd · 2022-02-07T13:16:42.000-08:00
* FullPath property now returns fully qualified path Gets the fully qualified path after combining the directory and name part as per the thread discussion. Also added and updated tests for the new change. Fix #62606 * OldFullPath returns fully qualified path To be consistent with new behavior of FullPath from FileSystemEventArgs same change was applied for RenamedEventArgs including updated tests. Fix #62606 * Marked some test cases as Windows OS specific In the context of using an actual full path which can vary on Unix vs Windows Fix #62606 * Using Path.Join instead of the custom Combine Adjusted unit tests to match the new logic. Fix #62606 * Account for corner case when fullPath matches root The previous buggy implementation of RenamedEventArgs produced a convenient side-effect for PhysicalFilesWatcher. Without the trailing slash adde by RenamedEventArgs the root was longer than the fullPath by a trailing slash. Fix #62606 * Handle relative paths from CWD in given drive Fix #62606 * Clearly separated unix and windows test cases Implemented most of the code review suggestions. Fix #62606 * Use EnsureTrailingSeparator instead of custom code Also made unit tests more focused by removing needless asserts from the tests Fix #62606 * Keeping the old behavior with trailing separator Reverted the changes to PhysicalFilesWatcher and added back the trailing separator behavior as per code review suggestion Fix #62606 * Used Path.Combien vs PathInternalEnsureSeparator As per code review suggestions. Fix #62606 * Added blank line for readability Pipeline retrigger. Some pipelines timed out, most probably transient issues. Fix #62606
diff --git a/src/libraries/System.IO.FileSystem.Watcher/src/System.IO.FileSystem.Watcher.csproj b/src/libraries/System.IO.FileSystem.Watcher/src/System.IO.FileSystem.Watcher.csproj
@@ -25,10 +25,12 @@
     <Compile Include="System\IO\WaitForChangedResult.cs" />
     <Compile Include="$(CommonPath)System\IO\PathInternal.CaseSensitivity.cs"
              Link="Common\System\IO\PathInternal.CaseSensitivity.cs" />
-  </ItemGroup>
-  <ItemGroup Condition=" '$(TargetsWindows)' == 'true'">
+    <Compile Include="$(CommonPath)System\IO\PathInternal.cs"
+            Link="Common\System\IO\PathInternal.cs" />
     <Compile Include="$(CommonPath)System\Text\ValueStringBuilder.cs"
              Link="Common\System\Text\ValueStringBuilder.cs" />
+  </ItemGroup>
+  <ItemGroup Condition=" '$(TargetsWindows)' == 'true'">
     <Compile Include="$(CommonPath)System\IO\PathInternal.Windows.cs"
              Link="Common\System\IO\PathInternal.Windows.cs" />
     <Compile Include="$(CommonPath)Interop\Windows\Interop.Libraries.cs"
diff --git a/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemEventArgs.cs b/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemEventArgs.cs
@@ -19,29 +19,12 @@ public FileSystemEventArgs(WatcherChangeTypes changeType, string directory, stri
         {
             _changeType = changeType;
             _name = name;
-            _fullPath = Combine(directory, name);
-        }
+            _fullPath = Path.Join(Path.GetFullPath(directory), name);
 
-        /// <summary>Combines a directory path and a relative file name into a single path.</summary>
-        /// <param name="directoryPath">The directory path.</param>
-        /// <param name="name">The file name.</param>
-        /// <returns>The combined name.</returns>
-        /// <remarks>
-        /// This is like Path.Combine, except without argument validation,
-        /// and a separator is used even if the name argument is empty.
-        /// </remarks>
-        internal static string Combine(string directoryPath, string? name)
-        {
-            bool hasSeparator = false;
-            if (directoryPath.Length > 0)
+            if (string.IsNullOrWhiteSpace(name))
             {
-                char c = directoryPath[directoryPath.Length - 1];
-                hasSeparator = c == Path.DirectorySeparatorChar || c == Path.AltDirectorySeparatorChar;
+                _fullPath = PathInternal.EnsureTrailingSeparator(_fullPath);
             }
-
-            return hasSeparator ?
-                directoryPath + name :
-                directoryPath + Path.DirectorySeparatorChar + name;
         }
 
         /// <devdoc>
diff --git a/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/RenamedEventArgs.cs b/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/RenamedEventArgs.cs
@@ -18,7 +18,12 @@ public RenamedEventArgs(WatcherChangeTypes changeType, string directory, string?
             : base(changeType, directory, name)
         {
             _oldName = oldName;
-            _oldFullPath = Combine(directory, oldName);
+            _oldFullPath = Path.Join(Path.GetFullPath(directory), oldName);
+
+            if (string.IsNullOrWhiteSpace(oldName))
+            {
+                _oldFullPath = PathInternal.EnsureTrailingSeparator(_oldFullPath);
+            }
         }
 
         /// <devdoc>
diff --git a/src/libraries/System.IO.FileSystem.Watcher/tests/Args.FileSystemEventArgs.cs b/src/libraries/System.IO.FileSystem.Watcher/tests/Args.FileSystemEventArgs.cs
@@ -8,28 +8,89 @@ namespace System.IO.Tests
     public class FileSystemEventArgsTests
     {
         [Theory]
-        [InlineData(WatcherChangeTypes.Changed, "C:", "foo.txt")]
-        [InlineData(WatcherChangeTypes.All, "C:", "foo.txt")]
-        [InlineData((WatcherChangeTypes)0, "", "")]
-        [InlineData((WatcherChangeTypes)0, "", null)]
-        public static void FileSystemEventArgs_ctor(WatcherChangeTypes changeType, string directory, string name)
+        [InlineData(WatcherChangeTypes.Changed, "bar", "foo.txt")]
+        [InlineData(WatcherChangeTypes.All, "bar", "foo.txt")]
+        [InlineData((WatcherChangeTypes)0, "bar", "")]
+        [InlineData((WatcherChangeTypes)0, "bar", null)]
+        public static void FileSystemEventArgs_ctor_NonPathPropertiesAreSetCorrectly(WatcherChangeTypes changeType, string directory, string name)
         {
             FileSystemEventArgs args = new FileSystemEventArgs(changeType, directory, name);
 
-            if (!directory.EndsWith(Path.DirectorySeparatorChar.ToString(), StringComparison.Ordinal))
-            {
-                directory += Path.DirectorySeparatorChar;
-            }
-
             Assert.Equal(changeType, args.ChangeType);
-            Assert.Equal(directory + name, args.FullPath);
             Assert.Equal(name, args.Name);
         }
 
+        [Theory]
+        [PlatformSpecific(TestPlatforms.Windows)]
+        [InlineData("D:\\", "foo.txt", "D:\\foo.txt")]
+        [InlineData("E:\\bar", "foo.txt", "E:\\bar\\foo.txt")]
+        public static void FileSystemEventArgs_ctor_DirectoryIsAbsolutePath_Windows(string directory, string name, string expectedFullPath)
+        {
+            FileSystemEventArgs args = new FileSystemEventArgs(WatcherChangeTypes.All, directory, name);
+
+            Assert.Equal(expectedFullPath, args.FullPath);
+        }
+
+        [Theory]
+        [PlatformSpecific(TestPlatforms.AnyUnix)]
+        [InlineData("/", "foo.txt", "/foo.txt")]
+        [InlineData("/bar", "foo.txt", "/bar/foo.txt")]
+        public static void FileSystemEventArgs_ctor_DirectoryIsAbsolutePath_Unix(string directory, string name, string expectedFullPath)
+        {
+            FileSystemEventArgs args = new FileSystemEventArgs(WatcherChangeTypes.All, directory, name);
+
+            Assert.Equal(expectedFullPath, args.FullPath);
+        }
+
+        [Theory]
+        [PlatformSpecific(TestPlatforms.Windows)]
+        [InlineData("bar", "foo.txt")]
+        [InlineData("bar\\baz", "foo.txt")]
+        public static void FileSystemEventArgs_ctor_DirectoryIsRelativePath_Windows(string directory, string name)
+        {
+            FileSystemEventArgs args = new FileSystemEventArgs(WatcherChangeTypes.All, directory, name);
+
+            Assert.Equal(Path.Combine(Directory.GetCurrentDirectory(), directory, name), args.FullPath);
+        }
+
+        [Theory]
+        [PlatformSpecific(TestPlatforms.AnyUnix)]
+        [InlineData("bar", "foo.txt")]
+        [InlineData("bar/baz", "foo.txt")]
+        public static void FileSystemEventArgs_ctor_DirectoryIsRelativePath_Unix(string directory, string name)
+        {
+            FileSystemEventArgs args = new FileSystemEventArgs(WatcherChangeTypes.All, directory, name);
+
+            Assert.Equal(Path.Combine(Directory.GetCurrentDirectory(), directory, name), args.FullPath);
+        }
+
+        [Theory]
+        [PlatformSpecific(TestPlatforms.Windows)]
+        [InlineData("C:", "foo.txt")]
+        public static void FileSystemEventArgs_ctor_RelativePathFromCurrentDirectoryInGivenDrive(string directory, string name)
+        {
+            FileSystemEventArgs args = new FileSystemEventArgs(WatcherChangeTypes.All, directory, name);
+
+            Assert.Equal(Path.Combine(Directory.GetCurrentDirectory(), name), args.FullPath);
+        }
+
+        [Theory]
+        [InlineData("bar", "")]
+        [InlineData("bar", null)]
+        public static void FileSystemEventArgs_ctor_When_EmptyFileName_Then_FullPathReturnsTheDirectoryFullPath_WithTrailingSeparator(string directory, string name)
+        {
+            FileSystemEventArgs args = new FileSystemEventArgs(WatcherChangeTypes.All, directory, name);
+
+            directory = PathInternal.EnsureTrailingSeparator(directory);
+
+            Assert.Equal(PathInternal.EnsureTrailingSeparator(Directory.GetCurrentDirectory()) + directory, args.FullPath);
+        }
+
         [Fact]
         public static void FileSystemEventArgs_ctor_Invalid()
         {
-            Assert.Throws<NullReferenceException>(() => new FileSystemEventArgs((WatcherChangeTypes)0, null, string.Empty));
+            Assert.Throws<ArgumentNullException>(() => new FileSystemEventArgs((WatcherChangeTypes)0, null, "foo.txt"));
+            Assert.Throws<ArgumentException>(() => new FileSystemEventArgs((WatcherChangeTypes)0, "", "foo.txt"));
         }
     }
 }
diff --git a/src/libraries/System.IO.FileSystem.Watcher/tests/Args.RenamedEventArgs.cs b/src/libraries/System.IO.FileSystem.Watcher/tests/Args.RenamedEventArgs.cs
@@ -8,34 +8,93 @@ namespace System.IO.Tests
     public class RenamedEventArgsTests
     {
         [Theory]
-        [InlineData(WatcherChangeTypes.Changed, "C:", "foo.txt", "bar.txt")]
-        [InlineData(WatcherChangeTypes.All, "C:", "foo.txt", "bar.txt")]
-        [InlineData((WatcherChangeTypes)0, "", "", "")]
-        [InlineData((WatcherChangeTypes)0, "", null, null)]
-        public static void RenamedEventArgs_ctor(WatcherChangeTypes changeType, string directory, string name, string oldName)
+        [InlineData(WatcherChangeTypes.Changed, "bar", "foo.txt", "bar.txt")]
+        [InlineData(WatcherChangeTypes.All, "bar", "foo.txt", "bar.txt")]
+        [InlineData((WatcherChangeTypes)0, "bar", "", "")]
+        [InlineData((WatcherChangeTypes)0, "bar", null, null)]
+        public static void RenamedEventArgs_ctor_NonPathPropertiesAreSetCorrectly(WatcherChangeTypes changeType, string directory, string name, string oldName)
         {
             RenamedEventArgs args = new RenamedEventArgs(changeType, directory, name, oldName);
+
             Assert.Equal(changeType, args.ChangeType);
-            Assert.Equal(directory + Path.DirectorySeparatorChar + name, args.FullPath);
             Assert.Equal(name, args.Name);
             Assert.Equal(oldName, args.OldName);
+
+            // FullPath is tested as part of the base class FileSystemEventArgs tests
         }
 
         [Theory]
-        [InlineData(WatcherChangeTypes.Changed, "C:", "foo.txt", "bar.txt")]
-        [InlineData(WatcherChangeTypes.All, "C:", "foo.txt", "bar.txt")]
-        [InlineData((WatcherChangeTypes)0, "", "", "")]
-        [InlineData((WatcherChangeTypes)0, "", null, null)]
-        public static void RenamedEventArgs_ctor_OldFullPath(WatcherChangeTypes changeType, string directory, string name, string oldName)
+        [PlatformSpecific(TestPlatforms.Windows)]
+        [InlineData("D:\\", "foo.txt", "bar.txt", "D:\\bar.txt")]
+        [InlineData("E:\\bar", "foo.txt", "bar.txt", "E:\\bar\\bar.txt")]
+        public static void RenamedEventArgs_ctor_OldFullPath_DirectoryIsAnAbsolutePath_Windows(string directory, string name, string oldName, string expectedOldFullPath)
         {
-            RenamedEventArgs args = new RenamedEventArgs(changeType, directory, name, oldName);
-            Assert.Equal(directory + Path.DirectorySeparatorChar + oldName, args.OldFullPath);
+            RenamedEventArgs args = new RenamedEventArgs(WatcherChangeTypes.All, directory, name, oldName);
+
+            Assert.Equal(expectedOldFullPath, args.OldFullPath);
+        }
+
+        [Theory]
+        [PlatformSpecific(TestPlatforms.AnyUnix)]
+        [InlineData("/", "foo.txt", "bar.txt", "/bar.txt")]
+        [InlineData("/bar", "foo.txt", "bar.txt", "/bar/bar.txt")]
+        public static void RenamedEventArgs_ctor_OldFullPath_DirectoryIsAnAbsolutePath_Unix(string directory, string name, string oldName, string expectedOldFullPath)
+        {
+            RenamedEventArgs args = new RenamedEventArgs(WatcherChangeTypes.All, directory, name, oldName);
+
+            Assert.Equal(expectedOldFullPath, args.OldFullPath);
+        }
+
+        [Theory]
+        [PlatformSpecific(TestPlatforms.Windows)]
+        [InlineData("bar", "foo.txt", "bar.txt")]
+        [InlineData("bar\\baz", "foo.txt", "bar.txt")]
+        public static void RenamedEventArgs_ctor_OldFullPath_DirectoryIsRelativePath_Windows(string directory, string name, string oldName)
+        {
+            RenamedEventArgs args = new RenamedEventArgs(WatcherChangeTypes.All, directory, name, oldName);
+
+            Assert.Equal(Path.Combine(Directory.GetCurrentDirectory(), directory, oldName), args.OldFullPath);
+        }
+
+        [Theory]
+        [PlatformSpecific(TestPlatforms.AnyUnix)]
+        [InlineData("bar", "foo.txt", "bar.txt")]
+        [InlineData("bar/baz", "foo.txt", "bar.txt")]
+        public static void RenamedEventArgs_ctor_OldFullPath_DirectoryIsRelativePath_Unix(string directory, string name, string oldName)
+        {
+            RenamedEventArgs args = new RenamedEventArgs(WatcherChangeTypes.All, directory, name, oldName);
+
+            Assert.Equal(Path.Combine(Directory.GetCurrentDirectory(), directory, oldName), args.OldFullPath);
+        }
+
+        [Theory]
+        [PlatformSpecific(TestPlatforms.Windows)]
+        [InlineData("C:", "foo.txt", "bar.txt")]
+        public static void RenamedEventArgs_ctor_OldFullPath_DirectoryIsRelativePathFromCurrentDirectoryInGivenDrive(string directory, string name, string oldName)
+        {
+            RenamedEventArgs args = new RenamedEventArgs(WatcherChangeTypes.All, directory, name, oldName);
+
+            Assert.Equal(Path.Combine(Directory.GetCurrentDirectory(), oldName), args.OldFullPath);
+        }
+
+        [Theory]
+        [InlineData("bar", "", "")]
+        [InlineData("bar", null, null)]
+        [InlineData("bar", "foo.txt", null)]
+        public static void RenamedEventArgs_ctor_When_EmptyOldFileName_Then_OldFullPathReturnsTheDirectoryFullPath_WithTrailingSeparator(string directory, string name, string oldName)
+        {
+            RenamedEventArgs args = new RenamedEventArgs(WatcherChangeTypes.All, directory, name, oldName);
+
+            directory = PathInternal.EnsureTrailingSeparator(directory);
+
+            Assert.Equal(PathInternal.EnsureTrailingSeparator(Directory.GetCurrentDirectory()) + directory, args.OldFullPath);
         }
 
         [Fact]
         public static void RenamedEventArgs_ctor_Invalid()
         {
-            Assert.Throws<NullReferenceException>(() => new RenamedEventArgs((WatcherChangeTypes)0, null, string.Empty, string.Empty));
+            Assert.Throws<ArgumentException>(() => new RenamedEventArgs((WatcherChangeTypes)0, "", "foo.txt", "bar.txt"));
+            Assert.Throws<ArgumentNullException>(() => new RenamedEventArgs((WatcherChangeTypes)0, null, "foo.txt", "bar.txt"));
         }
     }
 }
diff --git a/src/libraries/System.IO.FileSystem.Watcher/tests/System.IO.FileSystem.Watcher.Tests.csproj b/src/libraries/System.IO.FileSystem.Watcher/tests/System.IO.FileSystem.Watcher.Tests.csproj
@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+﻿<Project Sdk="Microsoft.NET.Sdk">
   <PropertyGroup>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
     <IncludeRemoteExecutor>true</IncludeRemoteExecutor>
@@ -39,6 +39,18 @@
              Link="Common\System\IO\ReparsePointUtilities.cs" />
     <Compile Include="$(CommonTestPath)TestUtilities\System\DisableParallelization.cs"
              Link="Common\TestUtilities\System\DisableParallelization.cs" />
+    <Compile Include="$(CommonPath)System\IO\PathInternal.cs"
+             Link="Common\System\IO\PathInternal.cs" />
+    <Compile Include="$(CommonPath)System\Text\ValueStringBuilder.cs"
+             Link="Common\System\Text\ValueStringBuilder.cs" />
+  </ItemGroup>
+  <ItemGroup Condition=" '$(TargetsWindows)' == 'true'">
+    <Compile Include="$(CommonPath)System\IO\PathInternal.Windows.cs"
+             Link="Common\System\IO\PathInternal.Windows.cs" />
+  </ItemGroup>
+  <ItemGroup Condition="'$(TargetsUnix)' == 'true'">
+    <Compile Include="$(CommonPath)System\IO\PathInternal.Unix.cs"
+             Link="Common\System\IO\PathInternal.Unix.cs" />
   </ItemGroup>
   <ItemGroup Condition="'$(TargetsLinux)' == 'true' or '$(TargetsOSX)' == 'true' or '$(TargetsiOS)' == 'true' or '$(TargetstvOS)' == 'true'">
     <Compile Include="FileSystemWatcher.Unix.cs" />

Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,12 @@ public RenamedEventArgs(WatcherChangeTypes changeType, string directory, string?`
`18`	`18`	`: base(changeType, directory, name)`
`19`	`19`	`{`
`20`	`20`	`_oldName = oldName;`
`21`		`- _oldFullPath = Combine(directory, oldName);`
	`21`	`+ _oldFullPath = Path.Join(Path.GetFullPath(directory), oldName);`
	`22`	`+`
	`23`	`+ if (string.IsNullOrWhiteSpace(oldName))`
	`24`	`+ {`
	`25`	`+ _oldFullPath = PathInternal.EnsureTrailingSeparator(_oldFullPath);`
	`26`	`+ }`
`22`	`27`	`}`
`23`	`28`
`24`	`29`	`/// <devdoc>`