cleanup(GCS+gRPC): add public DeleteObjectRequest (#12523)

Author: coryan

google/cloud/storage/async_client.h

@@ -241,7 +241,7 @@ class AsyncClient {
    *
    * @param bucket_name the name of the bucket that contains the object.
    * @param object_name the name of the object to be deleted.
-   * @param options a list of optional query parameters and/or request headers.
+   * @param args a list of optional query parameters and/or request headers.
    *     Valid types for this operation include `Generation`,
    *     `IfGenerationMatch`, `IfGenerationNotMatch`, `IfMetagenerationMatch`,
    *     `IfMetagenerationNotMatch`, and `UserProject`.
@@ -255,15 +255,14 @@ class AsyncClient {
    * @par Example
    * @snippet storage_async_samples.cc delete-object
    */
-  template <typename... RequestOptions>
-  future<Status> DeleteObject(std::string const& bucket_name,
-                              std::string const& object_name,
-                              RequestOptions&&... options) {
-    google::cloud::internal::OptionsSpan const span(
-        SpanOptions(std::forward<RequestOptions>(options)...));
-    storage::internal::DeleteObjectRequest request(bucket_name, object_name);
-    request.set_multiple_options(std::forward<RequestOptions>(options)...);
-    return connection_->AsyncDeleteObject(std::move(request));
+  template <typename... Args>
+  future<Status> DeleteObject(std::string bucket_name, std::string object_name,
+                              Args&&... args) {
+    auto options = SpanOptions(std::forward<Args>(args)...);
+    return connection_->AsyncDeleteObject(
+        {DeleteObjectRequest(std::move(bucket_name), std::move(object_name))
+             .set_multiple_options(std::forward<Args>(args)...),
+         /*.options=*/std::move(options)});
   }
 
   /**

google/cloud/storage/async_object_requests.h

@@ -15,6 +15,7 @@
 #ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_ASYNC_OBJECT_REQUESTS_H
 #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_ASYNC_OBJECT_REQUESTS_H
 
+#include "google/cloud/storage/internal/async/connection_fwd.h"
 #include "google/cloud/storage/internal/async/write_payload_fwd.h"
 #include "google/cloud/storage/internal/object_requests.h"
 #include "google/cloud/storage/version.h"
@@ -110,6 +111,45 @@ class InsertObjectRequest {
   Impl impl_;
 };
 
+/**
+ * A request to delete an object.
+ *
+ * This class can hold all the mandatory and optional parameters to delete an
+ * object. This class is the public API because it is required for mocking.
+ */
+class DeleteObjectRequest {
+ public:
+  DeleteObjectRequest() = default;
+  DeleteObjectRequest(std::string bucket_name, std::string object_name)
+      : impl_(std::move(bucket_name), std::move(object_name)) {}
+
+  std::string const& bucket_name() const { return impl_.bucket_name(); }
+  std::string const& object_name() const { return impl_.object_name(); }
+
+  template <typename... O>
+  DeleteObjectRequest& set_multiple_options(O&&... o) & {
+    impl_.set_multiple_options(std::forward<O>(o)...);
+    return *this;
+  }
+  template <typename... O>
+  DeleteObjectRequest&& set_multiple_options(O&&... o) && {
+    return std::move(set_multiple_options(std::forward<O>(o)...));
+  }
+
+  template <typename O>
+  bool HasOption() const {
+    return impl_.HasOption<O>();
+  }
+  template <typename O>
+  O GetOption() const {
+    return impl_.GetOption<O>();
+  }
+
+ protected:
+  friend class storage_internal::AsyncConnectionImpl;
+  storage::internal::DeleteObjectRequest impl_;
+};
+
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace storage_experimental
 }  // namespace cloud

google/cloud/storage/examples/storage_async_samples.cc

@@ -147,9 +147,9 @@ void AsyncDeleteObject(google::cloud::storage_experimental::AsyncClient& client,
   //! [delete-object]
   namespace g = google::cloud;
   namespace gcs_ex = google::cloud::storage_experimental;
-  [](gcs_ex::AsyncClient& client, std::string const& bucket_name,
-     std::string const& object_name) {
-    client.DeleteObject(bucket_name, object_name)
+  [](gcs_ex::AsyncClient& client, std::string bucket_name,
+     std::string object_name) {
+    client.DeleteObject(std::move(bucket_name), std::move(object_name))
         .then([](auto f) {
           auto status = f.get();
           if (!status.ok()) throw g::Status(std::move(status));

google/cloud/storage/google_cloud_cpp_storage_grpc.bzl

@@ -24,6 +24,7 @@ google_cloud_cpp_storage_grpc_hdrs = [
     "grpc_plugin.h",
     "internal/async/accumulate_read_object.h",
     "internal/async/connection.h",
+    "internal/async/connection_fwd.h",
     "internal/async/connection_impl.h",
     "internal/async/insert_object.h",
     "internal/async/token_impl.h",

google/cloud/storage/google_cloud_cpp_storage_grpc.cmake

@@ -50,6 +50,7 @@ else ()
         internal/async/accumulate_read_object.cc
         internal/async/accumulate_read_object.h
         internal/async/connection.h
+        internal/async/connection_fwd.h
         internal/async/connection_impl.cc
         internal/async/connection_impl.h
         internal/async/insert_object.cc

google/cloud/storage/internal/async/connection.h

@@ -65,6 +65,20 @@ class AsyncConnection {
     Options options;
   };
 
+  /**
+   * A thin wrapper around the `DeleteObject()` parameters.
+   *
+   * We use a single struct as the input parameter for this function to
+   * prevent breaking any mocks when additional parameters are needed.
+   */
+  struct DeleteObjectParams {
+    /// The metadata attributes to create the object.
+    storage_experimental::DeleteObjectRequest request;
+    /// Any options modifying the RPC behavior, including per-client and
+    /// per-connection options.
+    Options options;
+  };
+
   /// Insert a new object.
   virtual future<StatusOr<storage::ObjectMetadata>> AsyncInsertObject(
       InsertObjectParams p) = 0;
@@ -75,8 +89,8 @@ class AsyncConnection {
   virtual future<StatusOr<storage::ObjectMetadata>> AsyncComposeObject(
       storage::internal::ComposeObjectRequest request) = 0;
 
-  virtual future<Status> AsyncDeleteObject(
-      storage::internal::DeleteObjectRequest request) = 0;
+  // Delete an object.
+  virtual future<Status> AsyncDeleteObject(DeleteObjectParams p) = 0;
 
   virtual future<StatusOr<std::string>> AsyncStartResumableWrite(
       storage::internal::ResumableUploadRequest request) = 0;

google/cloud/storage/internal/async/connection_fwd.h

@@ -0,0 +1,32 @@
+// Copyright 2023 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_ASYNC_CONNECTION_FWD_H
+#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_ASYNC_CONNECTION_FWD_H
+
+#include "google/cloud/version.h"
+
+namespace google {
+namespace cloud {
+namespace storage_internal {
+GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
+
+class AsyncConnectionImpl;
+
+GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
+}  // namespace storage_internal
+}  // namespace cloud
+}  // namespace google
+
+#endif  // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_ASYNC_CONNECTION_FWD_H

google/cloud/storage/internal/async/connection_impl.cc

@@ -157,19 +157,21 @@ AsyncConnectionImpl::AsyncComposeObject(
       });
 }
 
-future<Status> AsyncConnectionImpl::AsyncDeleteObject(
-    storage::internal::DeleteObjectRequest request) {
-  auto current = google::cloud::internal::SaveCurrentOptions();
-  auto proto = ToProto(request);
-  auto const idempotency = idempotency_policy(*current)->IsIdempotent(request)
-                               ? Idempotency::kIdempotent
-                               : Idempotency::kNonIdempotent;
+future<Status> AsyncConnectionImpl::AsyncDeleteObject(DeleteObjectParams p) {
+  auto current = internal::MakeImmutableOptions(std::move(p.options));
+  auto proto = ToProto(p.request.impl_);
+  auto const idempotency =
+      idempotency_policy(*current)->IsIdempotent(p.request.impl_)
+          ? Idempotency::kIdempotent
+          : Idempotency::kNonIdempotent;
   return google::cloud::internal::AsyncRetryLoop(
       retry_policy(*current), backoff_policy(*current), idempotency, cq_,
-      [stub = stub_, current, request = std::move(request)](
+      [stub = stub_, current, request = std::move(p.request)](
           CompletionQueue& cq, std::shared_ptr<grpc::ClientContext> context,
           google::storage::v2::DeleteObjectRequest const& proto) {
         ApplyQueryParameters(*context, *current, request);
+        // TODO(#12359) - pass the `options` parameter
+        google::cloud::internal::OptionsSpan span(current);
         return stub->AsyncDeleteObject(cq, std::move(context), proto);
       },
       proto, __func__);

google/cloud/storage/internal/async/connection_impl.h

@@ -56,8 +56,7 @@ class AsyncConnectionImpl : public AsyncConnection {
   future<StatusOr<storage::ObjectMetadata>> AsyncComposeObject(
       storage::internal::ComposeObjectRequest request) override;
 
-  future<Status> AsyncDeleteObject(
-      storage::internal::DeleteObjectRequest request) override;
+  future<Status> AsyncDeleteObject(DeleteObjectParams p) override;
 
   future<StatusOr<std::string>> AsyncStartResumableWrite(
       storage::internal::ResumableUploadRequest request) override;

google/cloud/storage/internal/async/connection_impl_test.cc

@@ -33,7 +33,6 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 namespace {
 
 using ::google::cloud::internal::CurrentOptions;
-using ::google::cloud::internal::OptionsSpan;
 using ::google::cloud::storage::testing::MockAsyncInsertStream;
 using ::google::cloud::storage::testing::MockStorageStub;
 using ::google::cloud::storage::testing::canonical_errors::PermanentError;
@@ -238,10 +237,15 @@ TEST_F(AsyncConnectionImplTest, AsyncInsertObjectTooManyTransients) {
 }
 
 TEST_F(AsyncConnectionImplTest, AsyncDeleteObject) {
+  AsyncSequencer<bool> sequencer;
   auto mock = std::make_shared<storage::testing::MockStorageStub>();
   EXPECT_CALL(*mock, AsyncDeleteObject)
-      .WillOnce([this](
-                    CompletionQueue&, auto context,
+      .WillOnce([&] {
+        return sequencer.PushBack("DeleteObject(1)").then([](auto) {
+          return TransientError();
+        });
+      })
+      .WillOnce([&](CompletionQueue&, auto context,
                     google::storage::v2::DeleteObjectRequest const& request) {
         // Verify at least one option is initialized with the correct
         // values.
@@ -252,24 +256,77 @@ TEST_F(AsyncConnectionImplTest, AsyncDeleteObject) {
                                   Pair("x-goog-fieldmask", "field1,field2")));
         EXPECT_THAT(request.bucket(), "projects/_/buckets/test-bucket");
         EXPECT_THAT(request.object(), "test-object");
-        return make_ready_future(PermanentError());
+        return sequencer.PushBack("DeleteObject(2)").then([](auto) {
+          return Status{};
+        });
       });
-  CompletionQueue cq;
-  auto connection = MakeTestConnection(cq, mock);
-  // Simulate the option span created by the `*Client` class. The
-  // `AuthorityOption` value is used by the `*Stub` classes. We want to verify
-  // it is initialized properly by the time it makes it to the mocked call.
-  OptionsSpan span(connection->options());
-  auto response =
-      connection
-          ->AsyncDeleteObject(
-              storage::internal::DeleteObjectRequest("test-bucket",
-                                                     "test-object")
-                  .set_multiple_options(storage::Fields("field1,field2"),
-                                        storage::QuotaUser("test-quota-user")))
-          .get();
-  EXPECT_THAT(response, StatusIs(PermanentError().code(),
-                                 HasSubstr(PermanentError().message())));
+
+  internal::AutomaticallyCreatedBackgroundThreads pool(1);
+  auto connection = MakeTestConnection(pool.cq(), mock);
+  auto pending = connection->AsyncDeleteObject(
+      {storage_experimental::DeleteObjectRequest("test-bucket", "test-object")
+           .set_multiple_options(storage::Fields("field1,field2"),
+                                 storage::QuotaUser("test-quota-user")),
+       connection->options()});
+
+  auto next = sequencer.PopFrontWithName();
+  EXPECT_EQ(next.second, "DeleteObject(1)");
+  next.first.set_value(false);
+
+  next = sequencer.PopFrontWithName();
+  EXPECT_EQ(next.second, "DeleteObject(2)");
+  next.first.set_value(true);
+
+  auto response = pending.get();
+  ASSERT_STATUS_OK(response);
+}
+
+TEST_F(AsyncConnectionImplTest, AsyncDeleteObjectPermanentError) {
+  AsyncSequencer<bool> sequencer;
+  auto mock = std::make_shared<storage::testing::MockStorageStub>();
+  EXPECT_CALL(*mock, AsyncDeleteObject).WillOnce([&] {
+    return sequencer.PushBack("DeleteObject").then([](auto) {
+      return PermanentError();
+    });
+  });
+
+  internal::AutomaticallyCreatedBackgroundThreads pool(1);
+  auto connection = MakeTestConnection(pool.cq(), mock);
+  auto pending = connection->AsyncDeleteObject(
+      {storage_experimental::DeleteObjectRequest("test-bucket", "test-object"),
+       connection->options()});
+
+  auto next = sequencer.PopFrontWithName();
+  EXPECT_EQ(next.second, "DeleteObject");
+  next.first.set_value(false);
+
+  auto response = pending.get();
+  EXPECT_THAT(response, StatusIs(PermanentError().code()));
+}
+
+TEST_F(AsyncConnectionImplTest, AsyncDeleteObjectTooManyTransients) {
+  AsyncSequencer<bool> sequencer;
+  auto mock = std::make_shared<storage::testing::MockStorageStub>();
+  EXPECT_CALL(*mock, AsyncDeleteObject).Times(3).WillRepeatedly([&] {
+    return sequencer.PushBack("DeleteObject").then([](auto) {
+      return TransientError();
+    });
+  });
+
+  internal::AutomaticallyCreatedBackgroundThreads pool(1);
+  auto connection = MakeTestConnection(pool.cq(), mock);
+  auto pending = connection->AsyncDeleteObject(
+      {storage_experimental::DeleteObjectRequest("test-bucket", "test-object"),
+       connection->options()});
+
+  for (int i = 0; i != 3; ++i) {
+    auto next = sequencer.PopFrontWithName();
+    EXPECT_EQ(next.second, "DeleteObject");
+    next.first.set_value(false);
+  }
+
+  auto response = pending.get();
+  EXPECT_THAT(response, StatusIs(TransientError().code()));
 }
 
 }  // namespace