Skip to content

GH-40052: [C++][FS][Azure] Fix CreateDir and DeleteDir trailing slash issues on hierarchical namespace accounts#40054

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.

Sign up for GitHub

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

Merged
felipecrv merged 4 commits into from
Feb 13, 2024
Merged

GH-40052: [C++][FS][Azure] Fix CreateDir and DeleteDir trailing slash issues on hierarchical namespace accounts #40054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9fa8dce
Add tests
Tom-Newton Feb 12, 2024
1e0a31c
Remove trailing slashes where required
Tom-Newton Feb 12, 2024
6cfc277
Lint
Tom-Newton Feb 12, 2024
ed301ea
Rename test
Tom-Newton Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions cpp/src/arrow/filesystem/azurefs.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
Expand DownExpand Up@@ -1860,7 +1861,8 @@ class AzureFileSystem::Impl {
Azure::Nullable<std::string> 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 {
Expand Down
64 changes: 64 additions & 0 deletions cpp/src/arrow/filesystem/azurefs_test.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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_);
Expand DownExpand Up@@ -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_);
Expand DownExpand Up@@ -844,6 +861,21 @@ class TestAzureFileSystem : public ::testing::Test {
AssertFileInfo(fs(), blob_path, FileType::NotFound);
}

void TestNonEmptyDirWithTrailingSlash() {
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;
Expand DownExpand Up@@ -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;
Expand DownExpand Up@@ -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.
Expand DownExpand Up@@ -1496,6 +1546,11 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, CreateDirOnExistingContainer) {
this->TestCreateDirOnExistingContainer();
}

TYPED_TEST(TestAzureFileSystemOnAllScenarios,
CreateDirOnExistingContainerWithTrailingSlash) {
this->TestCreateDirOnExistingContainerWithTrailingSlash();
}

TYPED_TEST(TestAzureFileSystemOnAllScenarios, CreateDirOnMissingContainer) {
this->TestCreateDirOnMissingContainer();
}
Expand All@@ -1512,6 +1567,10 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirSuccessHaveBlob) {
this->TestDeleteDirSuccessHaveBlob();
}

TYPED_TEST(TestAzureFileSystemOnAllScenarios, NonEmptyDirWithTrailingSlash) {
this->TestNonEmptyDirWithTrailingSlash();
}

TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirSuccessHaveDirectory) {
this->TestDeleteDirSuccessHaveDirectory();
}
Expand All@@ -1520,6 +1579,11 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirContentsSuccessExist) {
this->TestDeleteDirContentsSuccessExist();
}

TYPED_TEST(TestAzureFileSystemOnAllScenarios,
DeleteDirContentsSuccessExistWithTrailingSlash) {
this->TestDeleteDirContentsSuccessExistWithTrailingSlash();
}

TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirContentsSuccessNonexistent) {
this->TestDeleteDirContentsSuccessNonexistent();
}
Expand Down