apacheGH-40085: [C++][FS][Azure] Validate containers in AzureFileSyst…

…em::Impl::MovePaths() (apache#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: apache#40085 Authored-by: Felipe Oliveira Carvalho <[email protected]> Signed-off-by: Felipe Oliveira Carvalho <[email protected]>
thisisnic · Mar 8, 2024 · edd2ae6 · edd2ae6
1 parent 26d4fc1
commit edd2ae6
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 14 deletions.
diff --git 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
@@ -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"));