From 9fa8dcea5ff26f530cb37ee5f2eef3abeb54aef3 Mon Sep 17 00:00:00 2001 From: Thomas Newton Date: Mon, 12 Feb 2024 20:14:06 +0000 Subject: [PATCH 1/4] Add tests --- cpp/src/arrow/filesystem/azurefs_test.cc | 64 ++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc index c39a5b7d22b..cfe55c3e62b 100644 --- a/cpp/src/arrow/filesystem/azurefs_test.cc +++ b/cpp/src/arrow/filesystem/azurefs_test.cc @@ -698,6 +698,14 @@ class TestAzureFileSystem : public ::testing::Test { AssertFileInfo(fs(), dir1, FileType::Directory); } + void TestCreateDirOnRootWithTrailingSlash() { + auto dir1 = PreexistingData::RandomContainerName(rng_) + "/"; + + AssertFileInfo(fs(), dir1, FileType::NotFound); + ASSERT_OK(fs()->CreateDir(dir1, false)); + AssertFileInfo(fs(), dir1, FileType::Directory); + } + void TestCreateDirOnExistingContainer() { auto data = SetUpPreexistingData(); auto dir1 = data.RandomDirectoryPath(rng_); @@ -758,6 +766,15 @@ class TestAzureFileSystem : public ::testing::Test { AssertFileInfo(fs(), subdir5, FileType::Directory); } + void TestCreateDirOnExistingContainerWithTrailingSlash() { + auto data = SetUpPreexistingData(); + auto dir1 = data.RandomDirectoryPath(rng_) + "/"; + + AssertFileInfo(fs(), dir1, FileType::NotFound); + ASSERT_OK(fs()->CreateDir(dir1, /*recursive=*/false)); + AssertFileInfo(fs(), dir1, FileType::Directory); + }; + void TestCreateDirOnMissingContainer() { auto container1 = PreexistingData::RandomContainerName(rng_); auto container2 = PreexistingData::RandomContainerName(rng_); @@ -844,6 +861,21 @@ class TestAzureFileSystem : public ::testing::Test { AssertFileInfo(fs(), blob_path, FileType::NotFound); } + void TestDeleteDirSuccessHaveBlobWithTrailingSlash() { + if (HasSubmitBatchBug()) { + GTEST_SKIP() << kSubmitBatchBugMessage; + } + auto data = SetUpPreexistingData(); + const auto directory_path = data.RandomDirectoryPath(rng_); + const auto blob_path = ConcatAbstractPath(directory_path, "hello.txt"); + ASSERT_OK_AND_ASSIGN(auto output, fs()->OpenOutputStream(blob_path)); + ASSERT_OK(output->Write("hello")); + ASSERT_OK(output->Close()); + AssertFileInfo(fs(), blob_path, FileType::File); + ASSERT_OK(fs()->DeleteDir(directory_path + "/")); + AssertFileInfo(fs(), blob_path, FileType::NotFound); + } + void TestDeleteDirSuccessHaveDirectory() { if (HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; @@ -873,6 +905,20 @@ class TestAzureFileSystem : public ::testing::Test { } } + void TestDeleteDirContentsSuccessExistWithTrailingSlash() { + if (HasSubmitBatchBug()) { + GTEST_SKIP() << kSubmitBatchBugMessage; + } + auto preexisting_data = SetUpPreexistingData(); + HierarchicalPaths paths; + CreateHierarchicalData(&paths); + ASSERT_OK(fs()->DeleteDirContents(paths.directory + "/")); + AssertFileInfo(fs(), paths.directory, FileType::Directory); + for (const auto& sub_path : paths.sub_paths) { + AssertFileInfo(fs(), sub_path, FileType::NotFound); + } + } + void TestDeleteDirContentsSuccessNonexistent() { if (HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; @@ -1466,6 +1512,10 @@ TYPED_TEST(TestAzureFileSystemOnAllEnvs, CreateDirWithEmptyPath) { TYPED_TEST(TestAzureFileSystemOnAllEnvs, CreateDirOnRoot) { this->TestCreateDirOnRoot(); } +TYPED_TEST(TestAzureFileSystemOnAllEnvs, CreateDirOnRootWithTrailingSlash) { + this->TestCreateDirOnRootWithTrailingSlash(); +} + // Tests using all the 3 environments (Azurite, Azure w/o HNS (flat), Azure w/ HNS) // combined with the two scenarios for AzureFileSystem::cached_hns_support_ -- unknown and // known according to the environment. @@ -1496,6 +1546,11 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, CreateDirOnExistingContainer) { this->TestCreateDirOnExistingContainer(); } +TYPED_TEST(TestAzureFileSystemOnAllScenarios, + CreateDirOnExistingContainerWithTrailingSlash) { + this->TestCreateDirOnExistingContainerWithTrailingSlash(); +} + TYPED_TEST(TestAzureFileSystemOnAllScenarios, CreateDirOnMissingContainer) { this->TestCreateDirOnMissingContainer(); } @@ -1512,6 +1567,10 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirSuccessHaveBlob) { this->TestDeleteDirSuccessHaveBlob(); } +TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirSuccessHaveBlobWithTrailingSlash) { + this->TestDeleteDirSuccessHaveBlobWithTrailingSlash(); +} + TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirSuccessHaveDirectory) { this->TestDeleteDirSuccessHaveDirectory(); } @@ -1520,6 +1579,11 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirContentsSuccessExist) { this->TestDeleteDirContentsSuccessExist(); } +TYPED_TEST(TestAzureFileSystemOnAllScenarios, + DeleteDirContentsSuccessExistWithTrailingSlash) { + this->TestDeleteDirContentsSuccessExistWithTrailingSlash(); +} + TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirContentsSuccessNonexistent) { this->TestDeleteDirContentsSuccessNonexistent(); } From 1e0a31cf9d4ef21e8f93f47f0b780ef3dc9c5277 Mon Sep 17 00:00:00 2001 From: Thomas Newton Date: Mon, 12 Feb 2024 20:48:07 +0000 Subject: [PATCH 2/4] Remove trailing slashes where required --- cpp/src/arrow/filesystem/azurefs.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc index d4bb4457014..11750591932 100644 --- a/cpp/src/arrow/filesystem/azurefs.cc +++ b/cpp/src/arrow/filesystem/azurefs.cc @@ -1635,7 +1635,8 @@ class AzureFileSystem::Impl { return CreateDirTemplate( adlfs_client, [](const auto& adlfs_client, const auto& location) { - auto directory_client = adlfs_client.GetDirectoryClient(location.path); + auto directory_client = adlfs_client.GetDirectoryClient( + std::string(internal::RemoveTrailingSlash(location.path))); directory_client.CreateIfNotExists(); }, location, recursive); @@ -1860,7 +1861,8 @@ class AzureFileSystem::Impl { Azure::Nullable lease_id = {}) { DCHECK(!location.container.empty()); DCHECK(!location.path.empty()); - auto directory_client = adlfs_client.GetDirectoryClient(location.path); + auto directory_client = adlfs_client.GetDirectoryClient( + std::string(internal::RemoveTrailingSlash(location.path))); DataLake::DeleteDirectoryOptions options; options.AccessConditions.LeaseId = std::move(lease_id); try { From 6cfc2772404c92723aa0e1908fa04920c929c291 Mon Sep 17 00:00:00 2001 From: Thomas Newton Date: Mon, 12 Feb 2024 22:16:49 +0000 Subject: [PATCH 3/4] Lint --- cpp/src/arrow/filesystem/azurefs_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc index cfe55c3e62b..97b45564d84 100644 --- a/cpp/src/arrow/filesystem/azurefs_test.cc +++ b/cpp/src/arrow/filesystem/azurefs_test.cc @@ -773,7 +773,7 @@ class TestAzureFileSystem : public ::testing::Test { AssertFileInfo(fs(), dir1, FileType::NotFound); ASSERT_OK(fs()->CreateDir(dir1, /*recursive=*/false)); AssertFileInfo(fs(), dir1, FileType::Directory); - }; + } void TestCreateDirOnMissingContainer() { auto container1 = PreexistingData::RandomContainerName(rng_); From ed301ea88f46a23e8375a211e5f48b3f57f96de2 Mon Sep 17 00:00:00 2001 From: Thomas Newton Date: Tue, 13 Feb 2024 14:33:03 +0000 Subject: [PATCH 4/4] Rename test --- cpp/src/arrow/filesystem/azurefs_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc index 97b45564d84..42f38f1ed6a 100644 --- a/cpp/src/arrow/filesystem/azurefs_test.cc +++ b/cpp/src/arrow/filesystem/azurefs_test.cc @@ -861,7 +861,7 @@ class TestAzureFileSystem : public ::testing::Test { AssertFileInfo(fs(), blob_path, FileType::NotFound); } - void TestDeleteDirSuccessHaveBlobWithTrailingSlash() { + void TestNonEmptyDirWithTrailingSlash() { if (HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } @@ -1567,8 +1567,8 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirSuccessHaveBlob) { this->TestDeleteDirSuccessHaveBlob(); } -TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirSuccessHaveBlobWithTrailingSlash) { - this->TestDeleteDirSuccessHaveBlobWithTrailingSlash(); +TYPED_TEST(TestAzureFileSystemOnAllScenarios, NonEmptyDirWithTrailingSlash) { + this->TestNonEmptyDirWithTrailingSlash(); } TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirSuccessHaveDirectory) {