From 65b0d1ffca44604b8f9ead2b2db15cec8eee23fb Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Tue, 30 Dec 2025 17:39:12 +0000 Subject: [PATCH 1/2] fix: improve path type detection heuristics in Downloader and PathHelpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses #1577 and #1584 by improving the reliability of path type detection: - Add check for trailing directory separator before falling back to Path.HasExtension() heuristic - paths ending with "/" or "\" are now reliably detected as directories - Add comprehensive XML documentation explaining the detection logic, order of precedence, and limitations - Document that directory names with dots (e.g., "my.folder") may still be misidentified as files when using the extension heuristic - Add helper method EndsWithDirectorySeparator() for consistent directory separator checking The extension heuristic remains as a fallback since it handles the common case correctly, but users can now explicitly indicate directory intent by appending a directory separator. Fixes #1577 Fixes #1584 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- src/ModularPipelines/Context/Downloader.cs | 51 ++++++++++++++++++- src/ModularPipelines/Helpers/PathHelpers.cs | 55 ++++++++++++++++++++- 2 files changed, 103 insertions(+), 3 deletions(-) diff --git a/src/ModularPipelines/Context/Downloader.cs b/src/ModularPipelines/Context/Downloader.cs index b6f1f70905..ac46f01851 100644 --- a/src/ModularPipelines/Context/Downloader.cs +++ b/src/ModularPipelines/Context/Downloader.cs @@ -66,6 +66,27 @@ public async Task DownloadResponseAsync(DownloadOptions opt return response.EnsureSuccessStatusCode(); } + /// + /// Determines the file path where the downloaded content should be saved. + /// + /// + /// + /// The save location is determined using the following logic: + /// + /// + /// If SavePath is null/empty, generates a temp file path with a GUID name and extension from the download URI. + /// If SavePath ends with a directory separator (e.g., "/" or "\"), treats it as a directory and generates a filename. + /// If SavePath has a file extension, treats it as a complete file path. + /// Otherwise, treats SavePath as a directory and generates a filename within it. + /// + /// + /// Note: The extension heuristic (step 3) may not be reliable for directory names + /// containing dots (e.g., "my.folder"). To ensure a path is treated as a directory, append a + /// directory separator character to the path. + /// + /// + /// The download options containing the save path. + /// The determined file path for saving the download. private string GetSaveLocation(DownloadFileOptions options) { if (string.IsNullOrWhiteSpace(options.SavePath)) @@ -73,6 +94,16 @@ private string GetSaveLocation(DownloadFileOptions options) return Path.Combine(Path.GetTempPath(), Guid.NewGuid() + GetExtension(options.DownloadUri.AbsoluteUri)); } + // Check if the path explicitly ends with a directory separator + // This is a reliable indicator that the user intends this to be a directory + if (EndsWithDirectorySeparator(options.SavePath)) + { + Directory.CreateDirectory(options.SavePath); + return Path.Combine(options.SavePath, Guid.NewGuid() + GetExtension(options.DownloadUri.AbsoluteUri)); + } + + // Use extension heuristic as a fallback + // Note: This can be unreliable for directory names containing dots (e.g., "my.folder") if (Path.HasExtension(options.SavePath)) { Directory.CreateDirectory(new FileInfo(options.SavePath).Directory!.FullName); @@ -83,7 +114,23 @@ private string GetSaveLocation(DownloadFileOptions options) return Path.Combine(options.SavePath, Guid.NewGuid() + GetExtension(options.DownloadUri.AbsoluteUri)); } - private string GetExtension(string downloadUri) + /// + /// Determines whether the path ends with a directory separator character. + /// + /// The path to check. + /// true if the path ends with a directory separator; otherwise, false. + private static bool EndsWithDirectorySeparator(string path) + { + if (string.IsNullOrEmpty(path)) + { + return false; + } + + var lastChar = path[path.Length - 1]; + return lastChar == Path.DirectorySeparatorChar || lastChar == Path.AltDirectorySeparatorChar; + } + + private static string GetExtension(string downloadUri) { if (Path.HasExtension(downloadUri)) { @@ -92,4 +139,4 @@ private string GetExtension(string downloadUri) return string.Empty; } -} \ No newline at end of file +} diff --git a/src/ModularPipelines/Helpers/PathHelpers.cs b/src/ModularPipelines/Helpers/PathHelpers.cs index c87c2b7b4b..063e45a70e 100644 --- a/src/ModularPipelines/Helpers/PathHelpers.cs +++ b/src/ModularPipelines/Helpers/PathHelpers.cs @@ -2,6 +2,29 @@ namespace ModularPipelines.Helpers; internal static class PathHelpers { + /// + /// Determines whether a path represents a file or directory. + /// + /// + /// + /// The detection logic follows this order of precedence: + /// + /// + /// If the path exists as a file on disk, returns . + /// If the path exists as a directory on disk, returns . + /// If the path ends with a directory separator character (e.g., "/" or "\"), returns . + /// If the path has a file extension (e.g., ".txt", ".cs"), returns . + /// Otherwise, defaults to . + /// + /// + /// Limitations: When the path does not exist on disk, this method uses heuristics + /// that may not always be accurate. For example, a directory named "my.folder" would be incorrectly + /// identified as a file due to the extension heuristic. Use explicit directory separators when + /// working with non-existent directory paths that contain dots. + /// + /// + /// The path to analyze. + /// The determined . public static PathType GetPathType(this string path) { if (File.Exists(path)) @@ -14,6 +37,15 @@ public static PathType GetPathType(this string path) return PathType.Directory; } + // Check if the path explicitly ends with a directory separator + // This is a reliable indicator that the user intends this to be a directory + if (EndsWithDirectorySeparator(path)) + { + return PathType.Directory; + } + + // Use extension heuristic as a fallback + // Note: This can be unreliable for directory names containing dots (e.g., "my.folder") if (Path.HasExtension(path)) { return PathType.File; @@ -22,8 +54,29 @@ public static PathType GetPathType(this string path) return PathType.Directory; } + /// + /// Gets the directory portion of a path. + /// + /// The path to extract the directory from. + /// The directory path, or null if it cannot be determined. public static string? GetDirectory(this string path) { return Path.GetDirectoryName(path) ?? new FileInfo(path).Directory?.FullName; } -} \ No newline at end of file + + /// + /// Determines whether the path ends with a directory separator character. + /// + /// The path to check. + /// true if the path ends with a directory separator; otherwise, false. + private static bool EndsWithDirectorySeparator(string path) + { + if (string.IsNullOrEmpty(path)) + { + return false; + } + + var lastChar = path[path.Length - 1]; + return lastChar == Path.DirectorySeparatorChar || lastChar == Path.AltDirectorySeparatorChar; + } +} From e484d961034a16bbb15d811c5e3d0ffa59650d81 Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Tue, 30 Dec 2025 18:01:07 +0000 Subject: [PATCH 2/2] fix: Address review feedback - eliminate DRY violation and add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Make EndsWithDirectorySeparator internal in PathHelpers - Remove duplicate method from Downloader.cs, now uses PathHelpers - Add comprehensive tests for trailing separator behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- src/ModularPipelines/Context/Downloader.cs | 19 +------- src/ModularPipelines/Helpers/PathHelpers.cs | 2 +- .../PathHelpersTests.cs | 48 +++++++++++++++++++ 3 files changed, 51 insertions(+), 18 deletions(-) diff --git a/src/ModularPipelines/Context/Downloader.cs b/src/ModularPipelines/Context/Downloader.cs index ac46f01851..12d799dd4d 100644 --- a/src/ModularPipelines/Context/Downloader.cs +++ b/src/ModularPipelines/Context/Downloader.cs @@ -1,4 +1,5 @@ using Microsoft.Extensions.Logging; +using ModularPipelines.Helpers; using ModularPipelines.Http; using ModularPipelines.Logging; using ModularPipelines.Options; @@ -96,7 +97,7 @@ private string GetSaveLocation(DownloadFileOptions options) // Check if the path explicitly ends with a directory separator // This is a reliable indicator that the user intends this to be a directory - if (EndsWithDirectorySeparator(options.SavePath)) + if (PathHelpers.EndsWithDirectorySeparator(options.SavePath)) { Directory.CreateDirectory(options.SavePath); return Path.Combine(options.SavePath, Guid.NewGuid() + GetExtension(options.DownloadUri.AbsoluteUri)); @@ -114,22 +115,6 @@ private string GetSaveLocation(DownloadFileOptions options) return Path.Combine(options.SavePath, Guid.NewGuid() + GetExtension(options.DownloadUri.AbsoluteUri)); } - /// - /// Determines whether the path ends with a directory separator character. - /// - /// The path to check. - /// true if the path ends with a directory separator; otherwise, false. - private static bool EndsWithDirectorySeparator(string path) - { - if (string.IsNullOrEmpty(path)) - { - return false; - } - - var lastChar = path[path.Length - 1]; - return lastChar == Path.DirectorySeparatorChar || lastChar == Path.AltDirectorySeparatorChar; - } - private static string GetExtension(string downloadUri) { if (Path.HasExtension(downloadUri)) diff --git a/src/ModularPipelines/Helpers/PathHelpers.cs b/src/ModularPipelines/Helpers/PathHelpers.cs index 063e45a70e..e96a520814 100644 --- a/src/ModularPipelines/Helpers/PathHelpers.cs +++ b/src/ModularPipelines/Helpers/PathHelpers.cs @@ -69,7 +69,7 @@ public static PathType GetPathType(this string path) /// /// The path to check. /// true if the path ends with a directory separator; otherwise, false. - private static bool EndsWithDirectorySeparator(string path) + internal static bool EndsWithDirectorySeparator(string path) { if (string.IsNullOrEmpty(path)) { diff --git a/test/ModularPipelines.UnitTests/PathHelpersTests.cs b/test/ModularPipelines.UnitTests/PathHelpersTests.cs index de47cb31a4..ba36709764 100644 --- a/test/ModularPipelines.UnitTests/PathHelpersTests.cs +++ b/test/ModularPipelines.UnitTests/PathHelpersTests.cs @@ -44,4 +44,52 @@ public async Task Directory_Path_Type2() var path = Path.Combine(TestContext.WorkingDirectory, "Blah", "Foo", "Bar", "Foo"); await Assert.That(path.GetPathType()).IsEqualTo(PathType.Directory); } + + [Test] + public async Task Directory_Path_Type_With_Trailing_Separator() + { + var path = Path.Combine(TestContext.WorkingDirectory, "Blah", "Foo") + Path.DirectorySeparatorChar; + await Assert.That(path.GetPathType()).IsEqualTo(PathType.Directory); + } + + [Test] + public async Task Directory_Path_Type_With_Dots_And_Trailing_Separator() + { + // A directory with dots in the name should be detected as directory when it has a trailing separator + var path = Path.Combine(TestContext.WorkingDirectory, "my.folder") + Path.DirectorySeparatorChar; + await Assert.That(path.GetPathType()).IsEqualTo(PathType.Directory); + } + + [Test] + public async Task EndsWithDirectorySeparator_Returns_True_For_Trailing_Separator() + { + var path = Path.Combine(TestContext.WorkingDirectory, "foo") + Path.DirectorySeparatorChar; + await Assert.That(PathHelpers.EndsWithDirectorySeparator(path)).IsTrue(); + } + + [Test] + public async Task EndsWithDirectorySeparator_Returns_True_For_Alt_Separator() + { + var path = Path.Combine(TestContext.WorkingDirectory, "foo") + Path.AltDirectorySeparatorChar; + await Assert.That(PathHelpers.EndsWithDirectorySeparator(path)).IsTrue(); + } + + [Test] + public async Task EndsWithDirectorySeparator_Returns_False_For_No_Separator() + { + var path = Path.Combine(TestContext.WorkingDirectory, "foo"); + await Assert.That(PathHelpers.EndsWithDirectorySeparator(path)).IsFalse(); + } + + [Test] + public async Task EndsWithDirectorySeparator_Returns_False_For_Empty_String() + { + await Assert.That(PathHelpers.EndsWithDirectorySeparator(string.Empty)).IsFalse(); + } + + [Test] + public async Task EndsWithDirectorySeparator_Returns_False_For_Null() + { + await Assert.That(PathHelpers.EndsWithDirectorySeparator(null!)).IsFalse(); + } } \ No newline at end of file