From edd2ae69fffb2adc33a4a6b6fb27b91a9d1087fd Mon Sep 17 00:00:00 2001 From: Felipe Oliveira Carvalho Date: Thu, 15 Feb 2024 09:13:29 -0300 Subject: [PATCH] GH-40085: [C++][FS][Azure] Validate containers in AzureFileSystem::Impl::MovePaths() (#40086) ### Rationale for this change Cross container moves aren't supported yet (and might never be). ### What changes are included in this PR? - Check that containers are the same before calling a `Rename` that assumes `src` and `dest` are on the same container ### Are these changes tested? Yes, new tests were added. * Closes: #40085 Authored-by: Felipe Oliveira Carvalho Signed-off-by: Felipe Oliveira Carvalho --- cpp/src/arrow/filesystem/azurefs.cc | 5 ++++ cpp/src/arrow/filesystem/azurefs_test.cc | 32 +++++++++++++----------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc index 11750591932e9..23af67a33d688 100644 --- a/cpp/src/arrow/filesystem/azurefs.cc +++ b/cpp/src/arrow/filesystem/azurefs.cc @@ -2297,6 +2297,11 @@ class AzureFileSystem::Impl { } } + // Now that src and dest are validated, make sure they are on the same filesystem. + if (src.container != dest.container) { + return CrossContainerMoveNotImplemented(src, dest); + } + try { // NOTE: The Azure SDK provides a RenameDirectory() function, but the // implementation is the same as RenameFile() with the only difference being diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc index 42f38f1ed6ac7..e6bd80d1d2508 100644 --- a/cpp/src/arrow/filesystem/azurefs_test.cc +++ b/cpp/src/arrow/filesystem/azurefs_test.cc @@ -1234,30 +1234,32 @@ class TestAzureFileSystem : public ::testing::Test { void TestMovePath() { Status st; auto data = SetUpPreexistingData(); + auto another_container = PreexistingData::RandomContainerName(rng_); + CreateContainer(another_container); // When source doesn't exist. ASSERT_MOVE("missing-container/src-path", data.ContainerPath("dest-path"), ENOENT); auto missing_path1 = data.RandomDirectoryPath(rng_); ASSERT_MOVE(missing_path1, "missing-container/path", ENOENT); // But when source exists... - if (!WithHierarchicalNamespace()) { - // ...and containers are different, we get an error message telling cross-container - // moves are not implemented. - EXPECT_RAISES_WITH_MESSAGE_THAT( - NotImplemented, - HasCrossContainerNotImplementedMessage(data.ObjectPath(), - "missing-container/path"), - fs()->Move(data.ObjectPath(), "missing-container/path")); - GTEST_SKIP() << "The rest of TestMovePath is not implemented for non-HNS scenarios"; - } - auto adlfs_client = - datalake_service_client_->GetFileSystemClient(data.container_name); - // ...and dest.container doesn't exist. + // ...and containers are different, we get an error message telling cross-container + // moves are not implemented. EXPECT_RAISES_WITH_MESSAGE_THAT( - IOError, HasMissingParentDirMessage("missing-container/path"), + NotImplemented, + HasCrossContainerNotImplementedMessage(data.ObjectPath(), + "missing-container/path"), fs()->Move(data.ObjectPath(), "missing-container/path")); + EXPECT_RAISES_WITH_MESSAGE_THAT( + NotImplemented, + HasCrossContainerNotImplementedMessage( + data.ObjectPath(), ConcatAbstractPath(another_container, "path")), + fs()->Move(data.ObjectPath(), ConcatAbstractPath(another_container, "path"))); AssertFileInfo(fs(), data.ObjectPath(), FileType::File); + if (!WithHierarchicalNamespace()) { + GTEST_SKIP() << "The rest of TestMovePath is not implemented for non-HNS scenarios"; + } + EXPECT_RAISES_WITH_MESSAGE_THAT( IOError, HasMissingParentDirMessage(data.Path("missing-subdir/file")), fs()->Move(data.ObjectPath(), data.Path("missing-subdir/file"))); @@ -1271,6 +1273,8 @@ class TestAzureFileSystem : public ::testing::Test { // "file0" exists // src is a file and dest exists (as a file) + auto adlfs_client = + datalake_service_client_->GetFileSystemClient(data.container_name); CreateFile(adlfs_client, PreexistingData::kObjectName, PreexistingData::kLoremIpsum); CreateFile(adlfs_client, "file1", PreexistingData::kLoremIpsum); ASSERT_MOVE_OK(data.ObjectPath(), data.Path("file0"));