(arrow) branch main updated: GH-40085: [C++][FS][Azure] Validate containers in AzureFileSystem::Impl::MovePaths() (#40086)

[felipecrv]

This is an automated email from the ASF dual-hosted git repository.

felipecrv pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git


The following commit(s) were added to refs/heads/main by this push:
     new 621f707f62 GH-40085: [C++][FS][Azure] Validate containers in 
AzureFileSystem::Impl::MovePaths() (#40086)
621f707f62 is described below

commit 621f707f62bee8bde128eed0ef1e239abe5eb8c0
Author: Felipe Oliveira Carvalho <felipe...@gmail.com>
AuthorDate: Thu Feb 15 09:13:29 2024 -0300

    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 <felipe...@gmail.com>
    Signed-off-by: Felipe Oliveira Carvalho <felipe...@gmail.com>
---
 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 1175059193..23af67a33d 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 42f38f1ed6..e6bd80d1d2 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"));