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

Revert FileSystemEventArgs/RenamedEventArgs FullPath/OldFullPath changes #68883

Merged

Conversation

jeffhandley
Copy link
Member

After fixing #68566 with #68582, which was addressing a regression caused by #63051, which fixed #62606, I've reconsidered the original issue further. This PR reverts both #68582 and #63051 and returns FileSystemEventArgs and RenamedEventArgs back to their previous behavior for FullPath and OldFullPath that dates back to .NET Framework.

The regression 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 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 the changes we had made.

I've confirmed that with these reverts, FileSystemEventArgs and RenamedEventArgs are now functionally equivalent to what existed in .NET Core 1.0 and .NET Framework, with only these exceptions:

  1. Nullability annotations
  2. Allowing for UNIX directory separators
  3. Throwing an ArgumentNullException instead of NullReferenceException when directory is null

I will follow up with a documentation issue that clarifies that FullPath and OldFullPath are not rooted, but they are mere combinations of the specified directory and file name, and when directory is an empty string, the values will begin with a directory separator character.

@ghost
Copy link

ghost commented May 5, 2022

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

Issue Details

After fixing #68566 with #68582, which was addressing a regression caused by #63051, which fixed #62606, I've reconsidered the original issue further. This PR reverts both #68582 and #63051 and returns FileSystemEventArgs and RenamedEventArgs back to their previous behavior for FullPath and OldFullPath that dates back to .NET Framework.

The regression 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 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 the changes we had made.

I've confirmed that with these reverts, FileSystemEventArgs and RenamedEventArgs are now functionally equivalent to what existed in .NET Core 1.0 and .NET Framework, with only these exceptions:

  1. Nullability annotations
  2. Allowing for UNIX directory separators
  3. Throwing an ArgumentNullException instead of NullReferenceException when directory is null

I will follow up with a documentation issue that clarifies that FullPath and OldFullPath are not rooted, but they are mere combinations of the specified directory and file name, and when directory is an empty string, the values will begin with a directory separator character.

Author: jeffhandley
Assignees: -
Labels:

area-System.IO

Milestone: -

@jeffhandley jeffhandley requested a review from jozkee May 5, 2022 03:00
@jeffhandley jeffhandley added this to the 7.0.0 milestone May 5, 2022
@jeffhandley
Copy link
Member Author

Thanks, @stephentoub. I was so focused on reverting the changes/behavior, I didn't look for the modernization opportunities there.

@jeffhandley jeffhandley merged commit 45f15a5 into dotnet:main May 5, 2022
@jeffhandley jeffhandley deleted the jeffhandley/filesystemeventargs branch May 5, 2022 23:54
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

Thanks, Jeff.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FileSystemEventArgs.FullPath does not return a fully qualified path
3 participants