Skip to content

Commit

Permalink
GH-39718: [C++][FS][Azure] Remove StatusFromErrorResponse as it's not…
Browse files Browse the repository at this point in the history
… necessary (#39719)

### Rationale for this change

Only the "*IfExists" functions from the Azure SDK ever set `response.Value.Deleted` to `false` to indicate that a resource wasn't found and the request succeeded without deleting anything. 

It's better that we use the `Delete()` versions of these functions instead of `DeleteIfExists` and check the `ErrorCode` ourselves to return an appropriate `Status` instead of something generic.

### What changes are included in this PR?

 - Removing `StatusFromErrorResponse`
 - Comments explaining the error handling decisions
 - Addition of a boolean parameter to `DeleteDirOnFileSystem` that controls how it fails when the directory being deleted doesn't exist
 
### Are these changes tested?

 - Yes, by the existing tests in `azurefs_test.cc`.
* Closes: #39718

Authored-by: Felipe Oliveira Carvalho <[email protected]>
Signed-off-by: Felipe Oliveira Carvalho <[email protected]>
  • Loading branch information
felipecrv authored Jan 22, 2024
1 parent 315e06a commit 4fc2a70
Showing 1 changed file with 22 additions and 40 deletions.
62 changes: 22 additions & 40 deletions cpp/src/arrow/filesystem/azurefs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -305,31 +305,9 @@ Status ValidateFileLocation(const AzureLocation& location) {
return internal::AssertNoTrailingSlash(location.path);
}

std::string_view BodyTextView(const Http::RawResponse& raw_response) {
const auto& body = raw_response.GetBody();
#ifndef NDEBUG
auto& headers = raw_response.GetHeaders();
auto content_type = headers.find("Content-Type");
if (content_type != headers.end()) {
DCHECK_EQ(std::string_view{content_type->second}.substr(5), "text/");
}
#endif
return std::string_view{reinterpret_cast<const char*>(body.data()), body.size()};
}

Status StatusFromErrorResponse(const std::string& url,
const Http::RawResponse& raw_response,
const std::string& context) {
// There isn't an Azure specification that response body on error
// doesn't contain any binary data but we assume it. We hope that
// error response body has useful information for the error.
auto body_text = BodyTextView(raw_response);
return Status::IOError(context, ": ", url, ": ", raw_response.GetReasonPhrase(), " (",
static_cast<int>(raw_response.GetStatusCode()),
"): ", body_text);
}

bool IsContainerNotFound(const Storage::StorageException& e) {
// In some situations, only the ReasonPhrase is set and the
// ErrorCode is empty, so we check both.
if (e.ErrorCode == "ContainerNotFound" ||
e.ReasonPhrase == "The specified container does not exist." ||
e.ReasonPhrase == "The specified filesystem does not exist.") {
Expand Down Expand Up @@ -1515,13 +1493,9 @@ class AzureFileSystem::Impl {
DCHECK(location.path.empty());
try {
auto response = container_client.Delete();
if (response.Value.Deleted) {
return Status::OK();
} else {
return StatusFromErrorResponse(
container_client.GetUrl(), *response.RawResponse,
"Failed to delete a container: " + location.container);
}
// Only the "*IfExists" functions ever set Deleted to false.
// All the others either succeed or throw an exception.
DCHECK(response.Value.Deleted);
} catch (const Storage::StorageException& exception) {
if (IsContainerNotFound(exception)) {
return PathNotFound(location);
Expand All @@ -1530,6 +1504,7 @@ class AzureFileSystem::Impl {
"Failed to delete a container: ", location.container, ": ",
container_client.GetUrl());
}
return Status::OK();
}

/// Deletes contents of a directory and possibly the directory itself
Expand Down Expand Up @@ -1649,23 +1624,29 @@ class AzureFileSystem::Impl {
/// \pre location.container is not empty.
/// \pre location.path is not empty.
Status DeleteDirOnFileSystem(const DataLake::DataLakeFileSystemClient& adlfs_client,
const AzureLocation& location) {
const AzureLocation& location, bool recursive,
bool require_dir_to_exist) {
DCHECK(!location.container.empty());
DCHECK(!location.path.empty());
auto directory_client = adlfs_client.GetDirectoryClient(location.path);
// XXX: should "directory not found" be considered an error?
try {
auto response = directory_client.DeleteRecursive();
if (response.Value.Deleted) {
auto response =
recursive ? directory_client.DeleteRecursive() : directory_client.DeleteEmpty();
// Only the "*IfExists" functions ever set Deleted to false.
// All the others either succeed or throw an exception.
DCHECK(response.Value.Deleted);
} catch (const Storage::StorageException& exception) {
if (exception.ErrorCode == "FilesystemNotFound" ||
exception.ErrorCode == "PathNotFound") {
if (require_dir_to_exist) {
return PathNotFound(location);
}
return Status::OK();
} else {
return StatusFromErrorResponse(directory_client.GetUrl(), *response.RawResponse,
"Failed to delete a directory: " + location.path);
}
} catch (const Storage::StorageException& exception) {
return ExceptionToStatus(exception, "Failed to delete a directory: ", location.path,
": ", directory_client.GetUrl());
}
return Status::OK();
}

/// \pre location.container is not empty.
Expand Down Expand Up @@ -1855,7 +1836,8 @@ Status AzureFileSystem::DeleteDir(const std::string& path) {
return PathNotFound(location);
}
if (hns_support == HNSSupport::kEnabled) {
return impl_->DeleteDirOnFileSystem(adlfs_client, location);
return impl_->DeleteDirOnFileSystem(adlfs_client, location, /*recursive=*/true,
/*require_dir_to_exist=*/true);
}
DCHECK_EQ(hns_support, HNSSupport::kDisabled);
auto container_client = impl_->GetBlobContainerClient(location.container);
Expand Down

0 comments on commit 4fc2a70

Please sign in to comment.