diff --git a/cpp/src/arrow/filesystem/test_util.cc b/cpp/src/arrow/filesystem/test_util.cc index bbff33f4d32..be9d99d72b8 100644 --- a/cpp/src/arrow/filesystem/test_util.cc +++ b/cpp/src/arrow/filesystem/test_util.cc @@ -208,6 +208,9 @@ void GenericFileSystemTest::TestCreateDir(FileSystem* fs) { ASSERT_RAISES(IOError, fs->CreateDir("AB/def/EF/GH", true /* recursive */)); ASSERT_RAISES(IOError, fs->CreateDir("AB/def/EF", false /* recursive */)); + // Cannot create a directory when there is already a file with the same name + ASSERT_RAISES(IOError, fs->CreateDir("AB/def")); + AssertAllDirs(fs, {"AB", "AB/CD", "AB/CD/EF", "AB/GH", "AB/GH/IJ", "XY"}); AssertAllFiles(fs, {"AB/def"}); } diff --git a/cpp/src/arrow/util/io_util.cc b/cpp/src/arrow/util/io_util.cc index 5e6da2fb9d6..552417e5a13 100644 --- a/cpp/src/arrow/util/io_util.cc +++ b/cpp/src/arrow/util/io_util.cc @@ -472,11 +472,18 @@ namespace { Result DoCreateDir(const PlatformFilename& dir_path, bool create_parents) { #ifdef _WIN32 - if (CreateDirectoryW(dir_path.ToNative().c_str(), nullptr)) { + const auto s = dir_path.ToNative().c_str(); + if (CreateDirectoryW(s, nullptr)) { return true; } int errnum = GetLastError(); if (errnum == ERROR_ALREADY_EXISTS) { + const auto attrs = GetFileAttributesW(s); + if (attrs == INVALID_FILE_ATTRIBUTES || !(attrs & FILE_ATTRIBUTE_DIRECTORY)) { + // Note we propagate the original error, not the GetFileAttributesW() error + return IOErrorFromWinError(ERROR_ALREADY_EXISTS, "Cannot create directory '", + dir_path.ToString(), "': non-directory entry exists"); + } return false; } if (create_parents && errnum == ERROR_PATH_NOT_FOUND) { @@ -489,10 +496,17 @@ Result DoCreateDir(const PlatformFilename& dir_path, bool create_parents) return IOErrorFromWinError(GetLastError(), "Cannot create directory '", dir_path.ToString(), "'"); #else - if (mkdir(dir_path.ToNative().c_str(), S_IRWXU | S_IRWXG | S_IRWXO) == 0) { + const auto s = dir_path.ToNative().c_str(); + if (mkdir(s, S_IRWXU | S_IRWXG | S_IRWXO) == 0) { return true; } if (errno == EEXIST) { + struct stat st; + if (stat(s, &st) || !S_ISDIR(st.st_mode)) { + // Note we propagate the original errno, not the stat() errno + return IOErrorFromErrno(EEXIST, "Cannot create directory '", dir_path.ToString(), + "': non-directory entry exists"); + } return false; } if (create_parents && errno == ENOENT) { diff --git a/cpp/src/arrow/util/io_util_test.cc b/cpp/src/arrow/util/io_util_test.cc index a423ecd0152..c09e4b974dd 100644 --- a/cpp/src/arrow/util/io_util_test.cc +++ b/cpp/src/arrow/util/io_util_test.cc @@ -29,6 +29,7 @@ #include #endif +#include #include #include "arrow/testing/gtest_util.h" @@ -53,6 +54,12 @@ void AssertNotExists(const PlatformFilename& path) { ASSERT_FALSE(exists) << "Path '" << path.ToString() << "' exists"; } +void TouchFile(const PlatformFilename& path) { + int fd = -1; + ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(path)); + ASSERT_OK(FileClose(fd)); +} + TEST(ErrnoFromStatus, Basics) { Status st; st = Status::OK(); @@ -370,7 +377,7 @@ TEST(CreateDirDeleteDir, Basics) { const std::string BASE = temp_dir->path().Join("xxx-io-util-test-dir2").ValueOrDie().ToString(); bool created, deleted; - PlatformFilename parent, child; + PlatformFilename parent, child, child_file; ASSERT_OK_AND_ASSIGN(parent, PlatformFilename::FromString(BASE)); ASSERT_EQ(parent.ToString(), BASE); @@ -392,6 +399,11 @@ TEST(CreateDirDeleteDir, Basics) { ASSERT_TRUE(created); AssertExists(child); + ASSERT_OK_AND_ASSIGN(child_file, PlatformFilename::FromString(BASE + "/some-file")); + TouchFile(child_file); + EXPECT_RAISES_WITH_MESSAGE_THAT( + IOError, ::testing::HasSubstr("non-directory entry exists"), CreateDir(child_file)); + ASSERT_OK_AND_ASSIGN(deleted, DeleteDirTree(parent)); ASSERT_TRUE(deleted); AssertNotExists(parent); @@ -436,9 +448,7 @@ TEST(DeleteDirContents, Basics) { ASSERT_OK_AND_ASSIGN(child2, PlatformFilename::FromString(BASE + "/child-file")); ASSERT_OK_AND_ASSIGN(created, CreateDir(child1)); ASSERT_TRUE(created); - int fd = -1; - ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(child2)); - ASSERT_OK(FileClose(fd)); + TouchFile(child2); AssertExists(child1); AssertExists(child2); @@ -522,6 +532,14 @@ TEST(CreateDirTree, Basics) { ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("EF")); ASSERT_OK_AND_ASSIGN(created, CreateDirTree(fn)); ASSERT_TRUE(created); + + ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("AB/file")); + TouchFile(fn); + EXPECT_RAISES_WITH_MESSAGE_THAT( + IOError, ::testing::HasSubstr("non-directory entry exists"), CreateDirTree(fn)); + + ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("AB/file/sub")); + ASSERT_RAISES(IOError, CreateDirTree(fn)); } TEST(ListDir, Basics) { @@ -546,9 +564,7 @@ TEST(ListDir, Basics) { ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("AB/EF/GH")); ASSERT_OK(CreateDirTree(fn)); ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("AB/ghi.txt")); - int fd = -1; - ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(fn)); - ASSERT_OK(FileClose(fd)); + TouchFile(fn); ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("AB")); ASSERT_OK_AND_ASSIGN(entries, ListDir(fn)); @@ -568,15 +584,13 @@ TEST(ListDir, Basics) { TEST(DeleteFile, Basics) { std::unique_ptr temp_dir; PlatformFilename fn; - int fd; bool deleted; ASSERT_OK_AND_ASSIGN(temp_dir, TemporaryDir::Make("io-util-test-")); ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("test-file")); AssertNotExists(fn); - ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(fn)); - ASSERT_OK(FileClose(fd)); + TouchFile(fn); AssertExists(fn); ASSERT_OK_AND_ASSIGN(deleted, DeleteFile(fn)); ASSERT_TRUE(deleted); @@ -638,8 +652,7 @@ TEST(FileUtils, LongPaths) { AssertExists(long_path); ASSERT_OK_AND_ASSIGN(long_filename, PlatformFilename::FromString(fs.str() + "/file.txt")); - ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(long_filename)); - ASSERT_OK(FileClose(fd)); + TouchFile(long_filename); AssertExists(long_filename); fd = -1; ASSERT_OK_AND_ASSIGN(fd, FileOpenReadable(long_filename));