From dcf305b3f30dc405aefd8e13ad2c883438b4e4d8 Mon Sep 17 00:00:00 2001 From: Jacob Lauzon Date: Fri, 5 Sep 2025 15:47:55 -0700 Subject: [PATCH 1/3] Fixed potential incorrect resource names for upload transfers --- .../CHANGELOG.md | 1 + .../CHANGELOG.md | 1 + .../Azure.Storage.DataMovement/CHANGELOG.md | 1 + .../LocalDirectoryStorageResourceContainer.cs | 1 + .../LocalDirectoryStorageResourceTests.cs | 22 +++++++++---------- 5 files changed, 15 insertions(+), 11 deletions(-) diff --git a/sdk/storage/Azure.Storage.DataMovement.Blobs/CHANGELOG.md b/sdk/storage/Azure.Storage.DataMovement.Blobs/CHANGELOG.md index 42383859b6f2..a03c20ff50b1 100644 --- a/sdk/storage/Azure.Storage.DataMovement.Blobs/CHANGELOG.md +++ b/sdk/storage/Azure.Storage.DataMovement.Blobs/CHANGELOG.md @@ -7,6 +7,7 @@ ### Breaking Changes ### Bugs Fixed +- Fixed an issue on upload transfers where file/directory names on the destination may be incorrect. The issue could occur if the path passed to `LocalFilesStorageResourceProvider.FromDirectory` contained a trailing slash. ### Other Changes diff --git a/sdk/storage/Azure.Storage.DataMovement.Files.Shares/CHANGELOG.md b/sdk/storage/Azure.Storage.DataMovement.Files.Shares/CHANGELOG.md index e855d65a41a3..0b8b07d5eae1 100644 --- a/sdk/storage/Azure.Storage.DataMovement.Files.Shares/CHANGELOG.md +++ b/sdk/storage/Azure.Storage.DataMovement.Files.Shares/CHANGELOG.md @@ -7,6 +7,7 @@ ### Breaking Changes ### Bugs Fixed +- Fixed an issue on upload transfers where file/directory names on the destination may be incorrect. The issue could occur if the path passed to `LocalFilesStorageResourceProvider.FromDirectory` contained a trailing slash. ### Other Changes diff --git a/sdk/storage/Azure.Storage.DataMovement/CHANGELOG.md b/sdk/storage/Azure.Storage.DataMovement/CHANGELOG.md index db100e9225c1..ebcbe13961ba 100644 --- a/sdk/storage/Azure.Storage.DataMovement/CHANGELOG.md +++ b/sdk/storage/Azure.Storage.DataMovement/CHANGELOG.md @@ -7,6 +7,7 @@ ### Breaking Changes ### Bugs Fixed +- Fixed an issue on upload transfers where file/directory names on the destination may be incorrect. The issue could occur if the path passed to `LocalFilesStorageResourceProvider.FromDirectory` contained a trailing slash. ### Other Changes diff --git a/sdk/storage/Azure.Storage.DataMovement/src/LocalDirectoryStorageResourceContainer.cs b/sdk/storage/Azure.Storage.DataMovement/src/LocalDirectoryStorageResourceContainer.cs index 0d8cb6d3311e..fe9b5e3181a4 100644 --- a/sdk/storage/Azure.Storage.DataMovement/src/LocalDirectoryStorageResourceContainer.cs +++ b/sdk/storage/Azure.Storage.DataMovement/src/LocalDirectoryStorageResourceContainer.cs @@ -29,6 +29,7 @@ internal class LocalDirectoryStorageResourceContainer : StorageResourceContainer public LocalDirectoryStorageResourceContainer(string path) { Argument.AssertNotNullOrWhiteSpace(path, nameof(path)); + path = path.TrimEnd(Path.DirectorySeparatorChar); _uri = PathScanner.GetEncodedUriFromPath(path); } diff --git a/sdk/storage/Azure.Storage.DataMovement/tests/LocalDirectoryStorageResourceTests.cs b/sdk/storage/Azure.Storage.DataMovement/tests/LocalDirectoryStorageResourceTests.cs index 8b5e2b6c6cc6..36991ff07647 100644 --- a/sdk/storage/Azure.Storage.DataMovement/tests/LocalDirectoryStorageResourceTests.cs +++ b/sdk/storage/Azure.Storage.DataMovement/tests/LocalDirectoryStorageResourceTests.cs @@ -17,16 +17,14 @@ public LocalDirectoryStorageResourceTests(bool async) : base(async, null /* TestMode.Record /* to re-record */) { } - private string[] fileNames => new[] - { - "C:\\Users\\user1\\Documents\\directory", - "C:\\Users\\user1\\Documents\\directory1\\", - "/user1/Documents/directory", - }; - [Test] public void Ctor_string() { + string[] fileNames = + { + "C:\\Users\\user1\\Documents\\directory", + "/user1/Documents/directory", + }; foreach (string path in fileNames) { // Arrange @@ -43,12 +41,13 @@ public void Ctor_string() [TestCase("C:\\test\\path=true@&#%", "C:/test/path%3Dtrue%40%26%23%25")] [TestCase("C:\\test\\path%3Dtest%26", "C:/test/path%253Dtest%2526")] [TestCase("C:\\test\\folder with spaces", "C:/test/folder%20with%20spaces")] + [TestCase("X:\\testing\\test\\", "X:/testing/test")] public void Ctor_String_Encoding_Windows(string path, string absolutePath) { LocalDirectoryStorageResourceContainer storageResource = new(path); Assert.That(storageResource.Uri.AbsolutePath, Is.EqualTo(absolutePath)); - // LocalPath should equal original path - Assert.That(storageResource.Uri.LocalPath, Is.EqualTo(path)); + // LocalPath should equal original path (trimmed) + Assert.That(storageResource.Uri.LocalPath, Is.EqualTo(path.TrimEnd('\\'))); } [Test] @@ -56,12 +55,13 @@ public void Ctor_String_Encoding_Windows(string path, string absolutePath) [TestCase("/test/path=true@&#%", "/test/path%3Dtrue%40%26%23%25")] [TestCase("/test/path%3Dtest%26", "/test/path%253Dtest%2526")] [TestCase("/test/folder with spaces", "/test/folder%20with%20spaces")] + [TestCase("/testing/test/", "/testing/test")] public void Ctor_String_Encoding_Unix(string path, string absolutePath) { LocalDirectoryStorageResourceContainer storageResource = new(path); Assert.That(storageResource.Uri.AbsolutePath, Is.EqualTo(absolutePath)); - // LocalPath should equal original path - Assert.That(storageResource.Uri.LocalPath, Is.EqualTo(path)); + // LocalPath should equal original path (trimmed) + Assert.That(storageResource.Uri.LocalPath, Is.EqualTo(path.TrimEnd('/'))); } [Test] From 548945f8aab38da01c24f9b1fca429937f175dd3 Mon Sep 17 00:00:00 2001 From: Jacob Lauzon Date: Tue, 9 Sep 2025 13:06:04 -0700 Subject: [PATCH 2/3] Fixed and added tests --- .../assets.json | 2 +- .../BlobContainerClientExtensionsTests.cs | 4 +-- .../assets.json | 2 +- .../LocalDirectoryStorageResourceTests.cs | 2 ++ .../StartTransferDirectoryDownloadTestBase.cs | 27 +++++++++++++++---- .../StartTransferUploadDirectoryTestBase.cs | 27 +++++++++++++++++++ .../tests/Shared/TransferValidator.Local.cs | 1 + 7 files changed, 56 insertions(+), 9 deletions(-) diff --git a/sdk/storage/Azure.Storage.DataMovement.Blobs/assets.json b/sdk/storage/Azure.Storage.DataMovement.Blobs/assets.json index 2bee441be7b6..0bfa287c1b98 100644 --- a/sdk/storage/Azure.Storage.DataMovement.Blobs/assets.json +++ b/sdk/storage/Azure.Storage.DataMovement.Blobs/assets.json @@ -2,5 +2,5 @@ "AssetsRepo": "Azure/azure-sdk-assets", "AssetsRepoPrefixPath": "net", "TagPrefix": "net/storage/Azure.Storage.DataMovement.Blobs", - "Tag": "net/storage/Azure.Storage.DataMovement.Blobs_f1a4120258" + "Tag": "net/storage/Azure.Storage.DataMovement.Blobs_2f44fdf50b" } diff --git a/sdk/storage/Azure.Storage.DataMovement.Blobs/tests/BlobContainerClientExtensionsTests.cs b/sdk/storage/Azure.Storage.DataMovement.Blobs/tests/BlobContainerClientExtensionsTests.cs index f68308b24ed1..922eca28cb7a 100644 --- a/sdk/storage/Azure.Storage.DataMovement.Blobs/tests/BlobContainerClientExtensionsTests.cs +++ b/sdk/storage/Azure.Storage.DataMovement.Blobs/tests/BlobContainerClientExtensionsTests.cs @@ -46,7 +46,7 @@ public async Task VerifyStartUploadDirectoryAsync([Values] bool addBlobDirectory var blobUri = new Uri(accountUrl + (addBlobDirectoryPath ? containerName + "/" + blobDirectoryPrefix : containerName)); - var directoryPath = Path.GetTempPath(); + var directoryPath = Path.GetTempPath().TrimEnd(Path.DirectorySeparatorChar); var options = addTransferOptions ? new TransferOptions() : (TransferOptions)null; @@ -91,7 +91,7 @@ public async Task VerifyStartDownloadToDirectoryAsync([Values] bool addBlobDirec var blobUri = new Uri(accountUrl + (addBlobDirectoryPath ? containerName + "/" + blobDirectoryPrefix : containerName)); - var directoryPath = Path.GetTempPath(); + var directoryPath = Path.GetTempPath().TrimEnd(Path.DirectorySeparatorChar); var options = addTransferOptions ? new TransferOptions() : (TransferOptions)null; diff --git a/sdk/storage/Azure.Storage.DataMovement.Files.Shares/assets.json b/sdk/storage/Azure.Storage.DataMovement.Files.Shares/assets.json index 3809b4e9b038..a1767d6ab3f1 100644 --- a/sdk/storage/Azure.Storage.DataMovement.Files.Shares/assets.json +++ b/sdk/storage/Azure.Storage.DataMovement.Files.Shares/assets.json @@ -2,5 +2,5 @@ "AssetsRepo": "Azure/azure-sdk-assets", "AssetsRepoPrefixPath": "net", "TagPrefix": "net/storage/Azure.Storage.DataMovement.Files.Shares", - "Tag": "net/storage/Azure.Storage.DataMovement.Files.Shares_0f05f57f67" + "Tag": "net/storage/Azure.Storage.DataMovement.Files.Shares_df34cc0ab7" } diff --git a/sdk/storage/Azure.Storage.DataMovement/tests/LocalDirectoryStorageResourceTests.cs b/sdk/storage/Azure.Storage.DataMovement/tests/LocalDirectoryStorageResourceTests.cs index 36991ff07647..49b80da0405b 100644 --- a/sdk/storage/Azure.Storage.DataMovement/tests/LocalDirectoryStorageResourceTests.cs +++ b/sdk/storage/Azure.Storage.DataMovement/tests/LocalDirectoryStorageResourceTests.cs @@ -42,6 +42,7 @@ public void Ctor_string() [TestCase("C:\\test\\path%3Dtest%26", "C:/test/path%253Dtest%2526")] [TestCase("C:\\test\\folder with spaces", "C:/test/folder%20with%20spaces")] [TestCase("X:\\testing\\test\\", "X:/testing/test")] + [TestCase("X:\\testing\\test\\\\", "X:/testing/test")] public void Ctor_String_Encoding_Windows(string path, string absolutePath) { LocalDirectoryStorageResourceContainer storageResource = new(path); @@ -56,6 +57,7 @@ public void Ctor_String_Encoding_Windows(string path, string absolutePath) [TestCase("/test/path%3Dtest%26", "/test/path%253Dtest%2526")] [TestCase("/test/folder with spaces", "/test/folder%20with%20spaces")] [TestCase("/testing/test/", "/testing/test")] + [TestCase("/testing/test//", "/testing/test")] public void Ctor_String_Encoding_Unix(string path, string absolutePath) { LocalDirectoryStorageResourceContainer storageResource = new(path); diff --git a/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferDirectoryDownloadTestBase.cs b/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferDirectoryDownloadTestBase.cs index 92b7926a8d10..ecab1a16ca2a 100644 --- a/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferDirectoryDownloadTestBase.cs +++ b/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferDirectoryDownloadTestBase.cs @@ -10,6 +10,7 @@ using Azure.Core; using Azure.Core.TestFramework; using Azure.Storage.Common; +using Azure.Storage.Test; using Azure.Storage.Test.Shared; using NUnit.Framework; @@ -141,7 +142,8 @@ private async Task DownloadDirectoryAndVerifyAsync( string directoryName = default, TransferManagerOptions transferManagerOptions = default, TransferOptions options = default, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + bool trailingSlash = false) { await SetupSourceDirectoryAsync(sourceContainer, sourcePrefix, itemSizes, cancellationToken); @@ -157,7 +159,8 @@ private async Task DownloadDirectoryAndVerifyAsync( }; StorageResourceContainer sourceResource = GetStorageResourceContainer(sourceContainer, sourcePrefix); - StorageResourceContainer destinationResource = LocalFilesStorageResourceProvider.FromDirectory(disposingLocalDirectory.DirectoryPath); + StorageResourceContainer destinationResource = LocalFilesStorageResourceProvider.FromDirectory( + disposingLocalDirectory.DirectoryPath + (trailingSlash ? Path.DirectorySeparatorChar : string.Empty)); await new TransferValidator().TransferAndVerifyAsync( sourceResource, @@ -407,14 +410,28 @@ public async Task DownloadDirectoryAsync_SpecialChars(string prefix) string.Join("/", prefix, "space folder", "space file"), ]; - CancellationTokenSource cts = new(); - cts.CancelAfter(TimeSpan.FromSeconds(30)); + CancellationToken cancellationToken = TestHelper.GetTimeoutToken(30); await DownloadDirectoryAndVerifyAsync( test.Container, prefix, itemNames.Select(name => (name, Constants.KB)).ToList(), directoryName: directoryName, - cancellationToken: cts.Token).ConfigureAwait(false); + cancellationToken: cancellationToken); + } + + [Test] + public async Task DownloadDirectoryAsync_TrailingSlash() + { + await using IDisposingContainer test = await GetDisposingContainerAsync(); + + string[] items = { "file1", "file2", "dir1/file1" }; + + CancellationToken cancellationToken = TestHelper.GetTimeoutToken(30); + await DownloadDirectoryAndVerifyAsync( + test.Container, + string.Empty, + items.Select(name => (name, Constants.KB)).ToList(), + cancellationToken: cancellationToken); } #endregion DirectoryDownloadTests diff --git a/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferUploadDirectoryTestBase.cs b/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferUploadDirectoryTestBase.cs index 3de233227cc8..b2e3694e2b31 100644 --- a/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferUploadDirectoryTestBase.cs +++ b/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferUploadDirectoryTestBase.cs @@ -523,5 +523,32 @@ await UploadDirectoryAndVerifyAsync( expectedTransfers: files.Count, cancellationToken: cancellationToken); } + + [RecordedTest] + public async Task Upload_TrailingSlash() + { + using DisposingLocalDirectory disposingLocalDirectory = DisposingLocalDirectory.GetTestDirectory(); + await using IDisposingContainer test = await GetDisposingContainerAsync(); + + List files = + [ + GetNewObjectName(), + GetNewObjectName(), + ]; + + CancellationToken cancellationToken = TestHelper.GetTimeoutToken(30); + await SetupDirectoryAsync( + disposingLocalDirectory.DirectoryPath, + files.Select(path => (path, (long)Constants.KB)).ToList(), + cancellationToken); + + // Intentionally append trailing slash + string sourcePath = disposingLocalDirectory.DirectoryPath + Path.DirectorySeparatorChar; + await UploadDirectoryAndVerifyAsync( + sourcePath, + test.Container, + expectedTransfers: files.Count, + cancellationToken: cancellationToken); + } } } diff --git a/sdk/storage/Azure.Storage.DataMovement/tests/Shared/TransferValidator.Local.cs b/sdk/storage/Azure.Storage.DataMovement/tests/Shared/TransferValidator.Local.cs index 457b7f65431d..c1898f2ca279 100644 --- a/sdk/storage/Azure.Storage.DataMovement/tests/Shared/TransferValidator.Local.cs +++ b/sdk/storage/Azure.Storage.DataMovement/tests/Shared/TransferValidator.Local.cs @@ -30,6 +30,7 @@ public Task OpenReadAsync(CancellationToken cancellationToken) public static ListFilesAsync GetLocalFileLister(string directoryPath) { + directoryPath = directoryPath.TrimEnd(Path.DirectorySeparatorChar); Task> ListFiles(CancellationToken cancellationToken) { List result = new(); From 01228bda7ee54aa9ca237cb702c429f6be3a489d Mon Sep 17 00:00:00 2001 From: Jacob Lauzon Date: Tue, 9 Sep 2025 14:41:45 -0700 Subject: [PATCH 3/3] Added extra fix, improved upload test --- .../assets.json | 2 +- .../assets.json | 2 +- .../src/TransferJobInternal.cs | 24 ++++++++++--------- .../StartTransferUploadDirectoryTestBase.cs | 6 +---- 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/sdk/storage/Azure.Storage.DataMovement.Blobs/assets.json b/sdk/storage/Azure.Storage.DataMovement.Blobs/assets.json index 0bfa287c1b98..fa5d9300e453 100644 --- a/sdk/storage/Azure.Storage.DataMovement.Blobs/assets.json +++ b/sdk/storage/Azure.Storage.DataMovement.Blobs/assets.json @@ -2,5 +2,5 @@ "AssetsRepo": "Azure/azure-sdk-assets", "AssetsRepoPrefixPath": "net", "TagPrefix": "net/storage/Azure.Storage.DataMovement.Blobs", - "Tag": "net/storage/Azure.Storage.DataMovement.Blobs_2f44fdf50b" + "Tag": "net/storage/Azure.Storage.DataMovement.Blobs_c2f1f2fc3f" } diff --git a/sdk/storage/Azure.Storage.DataMovement.Files.Shares/assets.json b/sdk/storage/Azure.Storage.DataMovement.Files.Shares/assets.json index a1767d6ab3f1..66d91f836dda 100644 --- a/sdk/storage/Azure.Storage.DataMovement.Files.Shares/assets.json +++ b/sdk/storage/Azure.Storage.DataMovement.Files.Shares/assets.json @@ -2,5 +2,5 @@ "AssetsRepo": "Azure/azure-sdk-assets", "AssetsRepoPrefixPath": "net", "TagPrefix": "net/storage/Azure.Storage.DataMovement.Files.Shares", - "Tag": "net/storage/Azure.Storage.DataMovement.Files.Shares_df34cc0ab7" + "Tag": "net/storage/Azure.Storage.DataMovement.Files.Shares_c847746677" } diff --git a/sdk/storage/Azure.Storage.DataMovement/src/TransferJobInternal.cs b/sdk/storage/Azure.Storage.DataMovement/src/TransferJobInternal.cs index 9258a8b82a52..03b4671a6fa6 100644 --- a/sdk/storage/Azure.Storage.DataMovement/src/TransferJobInternal.cs +++ b/sdk/storage/Azure.Storage.DataMovement/src/TransferJobInternal.cs @@ -329,13 +329,7 @@ private async IAsyncEnumerable EnumerateAndCreateJobPartsAsync( if (current.IsContainer) { - // Create sub-container - string containerUriPath = _sourceResourceContainer.Uri.GetPath(); - string subContainerPath = string.IsNullOrEmpty(containerUriPath) - ? current.Uri.GetPath() - : current.Uri.GetPath().Substring(containerUriPath.Length + 1); - // Decode the container name as it was pulled from encoded Uri and will be re-encoded on destination. - subContainerPath = Uri.UnescapeDataString(subContainerPath); + string subContainerPath = GetChildResourcePath(_sourceResourceContainer, current); StorageResourceContainer subContainer = _destinationResourceContainer.GetChildStorageResourceContainer(subContainerPath); @@ -370,10 +364,7 @@ private async IAsyncEnumerable EnumerateAndCreateJobPartsAsync( // Real container trasnfer else { - string containerUriPath = _sourceResourceContainer.Uri.GetPath(); - sourceName = current.Uri.GetPath().Substring(containerUriPath.Length + 1); - // Decode the resource name as it was pulled from encoded Uri and will be re-encoded on destination. - sourceName = Uri.UnescapeDataString(sourceName); + sourceName = GetChildResourcePath(_sourceResourceContainer, current); } StorageResourceItem sourceItem = (StorageResourceItem)current; @@ -654,5 +645,16 @@ internal async ValueTask IncrementJobParts() { await _progressTracker.IncrementQueuedFilesAsync(_cancellationToken).ConfigureAwait(false); } + + private static string GetChildResourcePath(StorageResourceContainer parent, StorageResource child) + { + string parentPath = parent.Uri.GetPath(); + string childPath = child.Uri.GetPath().Substring(parentPath.Length); + // If container path does not contain a '/' (normal case), then childPath will have one after substring. + // Safe to use / here as we are using AbsolutePath which normalizes to /. + childPath = childPath.TrimStart('/'); + // Decode the resource name as it was pulled from encoded Uri and will be re-encoded on destination. + return Uri.UnescapeDataString(childPath); + } } } diff --git a/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferUploadDirectoryTestBase.cs b/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferUploadDirectoryTestBase.cs index b2e3694e2b31..7f6c89b41fba 100644 --- a/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferUploadDirectoryTestBase.cs +++ b/sdk/storage/Azure.Storage.DataMovement/tests/Shared/StartTransferUploadDirectoryTestBase.cs @@ -530,11 +530,7 @@ public async Task Upload_TrailingSlash() using DisposingLocalDirectory disposingLocalDirectory = DisposingLocalDirectory.GetTestDirectory(); await using IDisposingContainer test = await GetDisposingContainerAsync(); - List files = - [ - GetNewObjectName(), - GetNewObjectName(), - ]; + List files = [ "file1", "file2", "dir1/file1" ]; CancellationToken cancellationToken = TestHelper.GetTimeoutToken(30); await SetupDirectoryAsync(