From 1ec6cfead1a5a8d9bff07f0a8bf80a38bada217e Mon Sep 17 00:00:00 2001
From: Abhijat Malviya <abhijat@redpanda.com>
Date: Thu, 9 Nov 2023 12:18:05 +0530
Subject: [PATCH] cloud_storage_clients: Use get_optional to find error code

When searching for error codes in an XML tree, ptree::count does not
honor dot paths. The change uses get_optional and adds some tests for
the method.
---
 src/v/cloud_storage_clients/s3_client.cc      |  5 +-
 .../tests/s3_client_test.cc                   | 53 +++++++++++++++++++
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/src/v/cloud_storage_clients/s3_client.cc b/src/v/cloud_storage_clients/s3_client.cc
index 6532c1ab03eb..c6c1d7a2b499 100644
--- a/src/v/cloud_storage_clients/s3_client.cc
+++ b/src/v/cloud_storage_clients/s3_client.cc
@@ -861,12 +861,13 @@ ss::future<> s3_client::do_delete_object(
       });
 }
 
-static std::variant<client::delete_objects_result, rest_error_response>
+std::variant<client::delete_objects_result, rest_error_response>
 iobuf_to_delete_objects_result(iobuf&& buf) {
     auto root = util::iobuf_to_ptree(std::move(buf), s3_log);
     auto result = client::delete_objects_result{};
     try {
-        if (root.count("Error.Code") > 0) {
+        if (auto error_code = root.get_optional<ss::sstring>("Error.Code");
+            error_code) {
             // This is an error response. S3 can reply with 200 error code and
             // error response in the body.
             constexpr const char* empty = "";
diff --git a/src/v/cloud_storage_clients/tests/s3_client_test.cc b/src/v/cloud_storage_clients/tests/s3_client_test.cc
index 94e64ff05341..f8bd6a4c98ca 100644
--- a/src/v/cloud_storage_clients/tests/s3_client_test.cc
+++ b/src/v/cloud_storage_clients/tests/s3_client_test.cc
@@ -833,3 +833,56 @@ FIXTURE_TEST(test_client_pool_reconnect, client_pool_fixture) {
     auto count = std::count(result.begin(), result.end(), true);
     BOOST_REQUIRE(count == 20);
 }
+
+SEASTAR_THREAD_TEST_CASE(test_parse_delete_object_response_infra_error) {
+    const ss::sstring xml_response
+      = "<Error><Code>SlowDown</Code><Message>Please reduce your request "
+        "rate.</Message><RequestId>R123</RequestId><HostId>H123</HostId></"
+        "Error>";
+
+    iobuf b;
+    b.append(xml_response.data(), xml_response.size());
+    const auto result = cloud_storage_clients::iobuf_to_delete_objects_result(
+      std::move(b));
+    const auto* error = std::get_if<cloud_storage_clients::rest_error_response>(
+      &result);
+    BOOST_REQUIRE_NE(error, nullptr);
+    BOOST_REQUIRE_EQUAL(error->code_string(), "SlowDown");
+}
+
+SEASTAR_THREAD_TEST_CASE(test_parse_delete_object_response_key_error) {
+    const ss::sstring xml_response
+      = R"XML(<DeleteResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
+    <Error>
+        <Key>0</Key>
+        <Code>TestFailure</Code>
+    </Error>
+    </DeleteResult>)XML";
+
+    iobuf b;
+    b.append(xml_response.data(), xml_response.size());
+    const auto result = cloud_storage_clients::iobuf_to_delete_objects_result(
+      std::move(b));
+    const auto* response
+      = std::get_if<cloud_storage_clients::client::delete_objects_result>(
+        &result);
+    BOOST_REQUIRE_NE(response, nullptr);
+    BOOST_REQUIRE_EQUAL(response->undeleted_keys.size(), 1);
+    BOOST_REQUIRE_EQUAL(response->undeleted_keys.front().reason, "TestFailure");
+}
+
+SEASTAR_THREAD_TEST_CASE(test_parse_delete_object_response_no_error) {
+    const ss::sstring xml_response
+      = R"XML(<DeleteResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
+    </DeleteResult>)XML";
+
+    iobuf b;
+    b.append(xml_response.data(), xml_response.size());
+    const auto result = cloud_storage_clients::iobuf_to_delete_objects_result(
+      std::move(b));
+    const auto* response
+      = std::get_if<cloud_storage_clients::client::delete_objects_result>(
+        &result);
+    BOOST_REQUIRE_NE(response, nullptr);
+    BOOST_REQUIRE(response->undeleted_keys.empty());
+}