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

FileSystemEventArgs.FullPath does not return a fully qualified path #62606

Closed
Tracked by #64596
UnityAlex opened this issue Dec 9, 2021 · 7 comments · Fixed by #63051 or #68883
Closed
Tracked by #64596

FileSystemEventArgs.FullPath does not return a fully qualified path #62606

UnityAlex opened this issue Dec 9, 2021 · 7 comments · Fixed by #63051 or #68883
Assignees
Labels
area-System.IO breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@UnityAlex
Copy link
Contributor

Description

As per the documentation https://docs.microsoft.com/en-us/dotnet/api/system.io.filesystemeventargs.fullpath?view=net-6.0 the FullPath property should return a fully qualified path. However it appears the constructor for FileSystemEventArgs assumes the directory passed in is fully qualified: https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemEventArgs.cs#L22

When used in conjunction with FileSystemWatcher it is possible for the path to be relative: https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs#L260

Reproduction Steps

Repro code:

using System;
using System.IO;

namespace MyNamespace
{
    class Program
    {
        static void Main()
        {
            using var watcher = new FileSystemWatcher(@"bar");

            watcher.NotifyFilter = NotifyFilters.LastWrite;

            watcher.Changed += OnChanged;

            watcher.Filter = "*.txt";
            watcher.IncludeSubdirectories = false;
            watcher.EnableRaisingEvents = true;

            Console.WriteLine("Press enter to exit.");
            Console.ReadLine();
        }

        private static void OnChanged(object sender, FileSystemEventArgs e)
        {
            if (e.ChangeType != WatcherChangeTypes.Changed)
            {
                return;
            }
            Console.WriteLine($"Changed: {e.FullPath}");
        }
    }
}
  1. I created the following dir structure on my machine:
    C:\foo\bar\footest.txt

  2. Set the working directory to be C:\foo

  3. Run the program and then modify footest.txt to trigger a changed event.

Expected behavior

Output should be:
Changed: C:\foo\bar\footest.txt

Actual behavior

Current output:
Changed: bar\footest.txt

Regression?

No response

Known Workarounds

Using Path.GetFullPath to fetch the fully qualified path

Console.WriteLine($"Changed: {Path.GetFullPath(e.FullPath)}");

Configuration

.Net 6.0
Windows 10 x64
After looking at the source I believe that this is not unique to Windows

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO untriaged New issue has not been triaged by the area owner labels Dec 9, 2021
@ghost
Copy link

ghost commented Dec 9, 2021

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

Issue Details

Description

As per the documentation https://docs.microsoft.com/en-us/dotnet/api/system.io.filesystemeventargs.fullpath?view=net-6.0 the FullPath property should return a fully qualified path. However it appears the constructor for FileSystemEventArgs assumes the directory passed in is fully qualified: https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemEventArgs.cs#L22

When used in conjunction with FileSystemWatcher it is possible for the path to be relative: https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs#L260

Reproduction Steps

Repro code:

using System;
using System.IO;

namespace MyNamespace
{
    class Program
    {
        static void Main()
        {
            using var watcher = new FileSystemWatcher(@"bar");

            watcher.NotifyFilter = NotifyFilters.LastWrite;

            watcher.Changed += OnChanged;

            watcher.Filter = "*.txt";
            watcher.IncludeSubdirectories = false;
            watcher.EnableRaisingEvents = true;

            Console.WriteLine("Press enter to exit.");
            Console.ReadLine();
        }

        private static void OnChanged(object sender, FileSystemEventArgs e)
        {
            if (e.ChangeType != WatcherChangeTypes.Changed)
            {
                return;
            }
            Console.WriteLine($"Changed: {e.FullPath}");
        }
    }
}
  1. I created the following dir structure on my machine:
    C:\foo\bar\footest.txt

  2. Set the working directory to be C:\foo

  3. Run the program and then modify footest.txt to trigger a changed event.

Expected behavior

Output should be:
Changed: C:\foo\bar\footest.txt

Actual behavior

Current output:
Changed: bar\footest.txt

Regression?

No response

Known Workarounds

Using Path.GetFullPath to fetch the fully qualified path

Console.WriteLine($"Changed: {Path.GetFullPath(e.FullPath)}");

Configuration

.Net 6.0
Windows 10 x64
After looking at the source I believe that this is not unique to Windows

Other information

No response

Author: UnityAlex
Assignees: -
Labels:

area-System.IO, untriaged

Milestone: -

@adamsitnik
Copy link
Member

cc @jozkee

@jozkee
Copy link
Member

jozkee commented Dec 17, 2021

This would require a breaking change on FileSystemEventArgs.FullPath which I'm inclined to accept given the name of the API.

cc @ericstj @jeffhandley

@jozkee jozkee added this to the 7.0.0 milestone Dec 17, 2021
@jozkee jozkee added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Dec 17, 2021
@slask
Copy link
Contributor

slask commented Dec 18, 2021

Hi @jozkee. Can I take this issue?

@jozkee
Copy link
Member

jozkee commented Dec 18, 2021

@slask Yes, let me assign you. Thanks!

slask added a commit to slask/runtime that referenced this issue Dec 21, 2021
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 dotnet#62606
slask added a commit to slask/runtime that referenced this issue Dec 21, 2021
To be consistent with new behavior of FullPath from FileSystemEventArgs
same change was applied for RenamedEventArgs including updated tests.

Fix dotnet#62606
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 21, 2021
slask added a commit to slask/runtime that referenced this issue Dec 25, 2021
In the context of using an actual full path
which can vary on Unix vs Windows

Fix dotnet#62606
slask added a commit to slask/runtime that referenced this issue Dec 29, 2021
Adjusted unit tests to match the new logic.

Fix dotnet#62606
slask added a commit to slask/runtime that referenced this issue Dec 30, 2021
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 dotnet#62606
slask added a commit to slask/runtime that referenced this issue Jan 6, 2022
slask added a commit to slask/runtime that referenced this issue Jan 17, 2022
Implemented most of the code review suggestions.

Fix dotnet#62606
slask added a commit to slask/runtime that referenced this issue Jan 17, 2022
Also made unit tests more focused by
removing needless asserts from the tests

Fix dotnet#62606
slask added a commit to slask/runtime that referenced this issue Jan 18, 2022
Reverted the changes to PhysicalFilesWatcher
and added back the trailing separator behavior
as per code review suggestion

Fix dotnet#62606
@slask
Copy link
Contributor

slask commented Jan 21, 2022

Linking docs issue dotnet/docs#27916

slask added a commit to slask/runtime that referenced this issue Feb 7, 2022
slask added a commit to slask/runtime that referenced this issue Feb 7, 2022
Pipeline retrigger. Some pipelines timed out, most probably transient issues.

Fix dotnet#62606
jozkee pushed a commit that referenced this issue Feb 7, 2022
* 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
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 10, 2022
@jeffhandley
Copy link
Member

After fixing #68566 with #68582, which was addressing a regression caused by #63051, which fixed this issue, I've reconsidered this original issue further. I think we should revert both #68582 and #63051 and return FileSystemEventArgs and RenamedEventArgs back to their previous behavior and instead address this as a documentation issue.

The regression that was reported involved using an empty directory name, and it was relying on FullPath being relative to the process's directory. From that regression report, it's easy to imagine scenarios where the non-rooted path nature of FullPath is utilized and perhaps even leveraged to combine that full path to a different root. I remember such code I wrote myself back in .NET Framework days that would be broken by this change.

I'm going to submit a PR that reverts both #63051 and #68582, but leaves the additional tests in place, adapted to match the previous behavior.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
5 participants