Skip to content

Commit 0dbbd43

Browse files
authored
GH-40052: [C++][FS][Azure] Fix CreateDir and DeleteDir trailing slash issues on hierarchical namespace accounts (#40054)
### Rationale for this change There were the following failure cases ``` fs->CreateDir("directory/") ``` ``` fs->DeleteDir("directory/") ``` They fail with ``` Failed to delete a directory: directory/: https://tomtesthns.blob.core.windows.net/ea119933-c9d3-11ee-989a-71cec6336ac8/directory/ Azure Error: [InvalidUri] 400 The request URI is invalid. The request URI is invalid. RequestId:c9ad826a-101f-0005-5be0-5d0db4000000 Time:2024-02-12T18:24:12.9974541Z Request ID: c9ad826a-101f-0005-5be0-5d0db4000000 ``` ### What changes are included in this PR? Add tests to cover these cases. Remove trailing slashes to avoid these issues. ### Are these changes tested? Yes. I added new test cases to cover these cases. I was rather torn about whether to add precise tests like I've done or to duplicate every test case we had and run it a second time with trailing slashes. ### Are there any user-facing changes? Fixed bug on `CreateDir` and `DeleteDir`. * Closes: #40052 Authored-by: Thomas Newton <[email protected]> Signed-off-by: Felipe Oliveira Carvalho <[email protected]>
1 parent bbe59b3 commit 0dbbd43

File tree

2 files changed

+68
-2
lines changed

2 files changed

+68
-2
lines changed

cpp/src/arrow/filesystem/azurefs.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1635,7 +1635,8 @@ class AzureFileSystem::Impl {
16351635
return CreateDirTemplate(
16361636
adlfs_client,
16371637
[](const auto& adlfs_client, const auto& location) {
1638-
auto directory_client = adlfs_client.GetDirectoryClient(location.path);
1638+
auto directory_client = adlfs_client.GetDirectoryClient(
1639+
std::string(internal::RemoveTrailingSlash(location.path)));
16391640
directory_client.CreateIfNotExists();
16401641
},
16411642
location, recursive);
@@ -1860,7 +1861,8 @@ class AzureFileSystem::Impl {
18601861
Azure::Nullable<std::string> lease_id = {}) {
18611862
DCHECK(!location.container.empty());
18621863
DCHECK(!location.path.empty());
1863-
auto directory_client = adlfs_client.GetDirectoryClient(location.path);
1864+
auto directory_client = adlfs_client.GetDirectoryClient(
1865+
std::string(internal::RemoveTrailingSlash(location.path)));
18641866
DataLake::DeleteDirectoryOptions options;
18651867
options.AccessConditions.LeaseId = std::move(lease_id);
18661868
try {

cpp/src/arrow/filesystem/azurefs_test.cc

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -698,6 +698,14 @@ class TestAzureFileSystem : public ::testing::Test {
698698
AssertFileInfo(fs(), dir1, FileType::Directory);
699699
}
700700

701+
void TestCreateDirOnRootWithTrailingSlash() {
702+
auto dir1 = PreexistingData::RandomContainerName(rng_) + "/";
703+
704+
AssertFileInfo(fs(), dir1, FileType::NotFound);
705+
ASSERT_OK(fs()->CreateDir(dir1, false));
706+
AssertFileInfo(fs(), dir1, FileType::Directory);
707+
}
708+
701709
void TestCreateDirOnExistingContainer() {
702710
auto data = SetUpPreexistingData();
703711
auto dir1 = data.RandomDirectoryPath(rng_);
@@ -758,6 +766,15 @@ class TestAzureFileSystem : public ::testing::Test {
758766
AssertFileInfo(fs(), subdir5, FileType::Directory);
759767
}
760768

769+
void TestCreateDirOnExistingContainerWithTrailingSlash() {
770+
auto data = SetUpPreexistingData();
771+
auto dir1 = data.RandomDirectoryPath(rng_) + "/";
772+
773+
AssertFileInfo(fs(), dir1, FileType::NotFound);
774+
ASSERT_OK(fs()->CreateDir(dir1, /*recursive=*/false));
775+
AssertFileInfo(fs(), dir1, FileType::Directory);
776+
}
777+
761778
void TestCreateDirOnMissingContainer() {
762779
auto container1 = PreexistingData::RandomContainerName(rng_);
763780
auto container2 = PreexistingData::RandomContainerName(rng_);
@@ -844,6 +861,21 @@ class TestAzureFileSystem : public ::testing::Test {
844861
AssertFileInfo(fs(), blob_path, FileType::NotFound);
845862
}
846863

864+
void TestNonEmptyDirWithTrailingSlash() {
865+
if (HasSubmitBatchBug()) {
866+
GTEST_SKIP() << kSubmitBatchBugMessage;
867+
}
868+
auto data = SetUpPreexistingData();
869+
const auto directory_path = data.RandomDirectoryPath(rng_);
870+
const auto blob_path = ConcatAbstractPath(directory_path, "hello.txt");
871+
ASSERT_OK_AND_ASSIGN(auto output, fs()->OpenOutputStream(blob_path));
872+
ASSERT_OK(output->Write("hello"));
873+
ASSERT_OK(output->Close());
874+
AssertFileInfo(fs(), blob_path, FileType::File);
875+
ASSERT_OK(fs()->DeleteDir(directory_path + "/"));
876+
AssertFileInfo(fs(), blob_path, FileType::NotFound);
877+
}
878+
847879
void TestDeleteDirSuccessHaveDirectory() {
848880
if (HasSubmitBatchBug()) {
849881
GTEST_SKIP() << kSubmitBatchBugMessage;
@@ -873,6 +905,20 @@ class TestAzureFileSystem : public ::testing::Test {
873905
}
874906
}
875907

908+
void TestDeleteDirContentsSuccessExistWithTrailingSlash() {
909+
if (HasSubmitBatchBug()) {
910+
GTEST_SKIP() << kSubmitBatchBugMessage;
911+
}
912+
auto preexisting_data = SetUpPreexistingData();
913+
HierarchicalPaths paths;
914+
CreateHierarchicalData(&paths);
915+
ASSERT_OK(fs()->DeleteDirContents(paths.directory + "/"));
916+
AssertFileInfo(fs(), paths.directory, FileType::Directory);
917+
for (const auto& sub_path : paths.sub_paths) {
918+
AssertFileInfo(fs(), sub_path, FileType::NotFound);
919+
}
920+
}
921+
876922
void TestDeleteDirContentsSuccessNonexistent() {
877923
if (HasSubmitBatchBug()) {
878924
GTEST_SKIP() << kSubmitBatchBugMessage;
@@ -1466,6 +1512,10 @@ TYPED_TEST(TestAzureFileSystemOnAllEnvs, CreateDirWithEmptyPath) {
14661512

14671513
TYPED_TEST(TestAzureFileSystemOnAllEnvs, CreateDirOnRoot) { this->TestCreateDirOnRoot(); }
14681514

1515+
TYPED_TEST(TestAzureFileSystemOnAllEnvs, CreateDirOnRootWithTrailingSlash) {
1516+
this->TestCreateDirOnRootWithTrailingSlash();
1517+
}
1518+
14691519
// Tests using all the 3 environments (Azurite, Azure w/o HNS (flat), Azure w/ HNS)
14701520
// combined with the two scenarios for AzureFileSystem::cached_hns_support_ -- unknown and
14711521
// known according to the environment.
@@ -1496,6 +1546,11 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, CreateDirOnExistingContainer) {
14961546
this->TestCreateDirOnExistingContainer();
14971547
}
14981548

1549+
TYPED_TEST(TestAzureFileSystemOnAllScenarios,
1550+
CreateDirOnExistingContainerWithTrailingSlash) {
1551+
this->TestCreateDirOnExistingContainerWithTrailingSlash();
1552+
}
1553+
14991554
TYPED_TEST(TestAzureFileSystemOnAllScenarios, CreateDirOnMissingContainer) {
15001555
this->TestCreateDirOnMissingContainer();
15011556
}
@@ -1512,6 +1567,10 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirSuccessHaveBlob) {
15121567
this->TestDeleteDirSuccessHaveBlob();
15131568
}
15141569

1570+
TYPED_TEST(TestAzureFileSystemOnAllScenarios, NonEmptyDirWithTrailingSlash) {
1571+
this->TestNonEmptyDirWithTrailingSlash();
1572+
}
1573+
15151574
TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirSuccessHaveDirectory) {
15161575
this->TestDeleteDirSuccessHaveDirectory();
15171576
}
@@ -1520,6 +1579,11 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirContentsSuccessExist) {
15201579
this->TestDeleteDirContentsSuccessExist();
15211580
}
15221581

1582+
TYPED_TEST(TestAzureFileSystemOnAllScenarios,
1583+
DeleteDirContentsSuccessExistWithTrailingSlash) {
1584+
this->TestDeleteDirContentsSuccessExistWithTrailingSlash();
1585+
}
1586+
15231587
TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirContentsSuccessNonexistent) {
15241588
this->TestDeleteDirContentsSuccessNonexistent();
15251589
}

0 commit comments

Comments
 (0)