Do not throw exceptions in WriteObject(). (#2400)

coryan · web-flow · commit 01cb07e5f370 · 2019-04-04T14:10:01.000-04:00
We were still throwing an exception in WriteObject() for some types of
errors. This fixes that problem and adds integration tests to verify we
do not throw exceptions in other media operations (downloads, uploads,
etc.)
diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc
@@ -59,6 +59,20 @@ ObjectReadStream Client::ReadObjectImpl(
   return ObjectReadStream(*std::move(streambuf));
 }
 
+ObjectWriteStream Client::WriteObjectImpl(
+    internal::InsertObjectStreamingRequest const& request) {
+  auto streambuf = raw_client_->WriteObject(request);
+  if (!streambuf) {
+    ObjectWriteStream error_stream(google::cloud::internal::make_unique<
+                                   internal::ObjectWriteErrorStreambuf>(
+        std::move(streambuf).status()));
+    error_stream.setstate(std::ios::badbit | std::ios::eofbit);
+    error_stream.Close();
+    return error_stream;
+  }
+  return ObjectWriteStream(*std::move(streambuf));
+}
+
 bool Client::UseSimpleUpload(std::string const& file_name) const {
   auto status = google::cloud::internal::status(file_name);
   if (!is_regular(status)) {
diff --git a/google/cloud/storage/client.h b/google/cloud/storage/client.h
@@ -892,7 +892,7 @@ class Client {
                               Options&&... options) {
     internal::ReadObjectRangeRequest request(bucket_name, object_name);
     request.set_multiple_options(std::forward<Options>(options)...);
-    return ReadObjectImpl(std::move(request));
+    return ReadObjectImpl(request);
   }
 
   /**
@@ -958,7 +958,7 @@ class Client {
                                 Options&&... options) {
     internal::InsertObjectStreamingRequest request(bucket_name, object_name);
     request.set_multiple_options(std::forward<Options>(options)...);
-    return ObjectWriteStream(raw_client_->WriteObject(request).value());
+    return WriteObjectImpl(request);
   }
 
   /**
@@ -2786,6 +2786,9 @@ class Client {
   ObjectReadStream ReadObjectImpl(
       internal::ReadObjectRangeRequest const& request);
 
+  ObjectWriteStream WriteObjectImpl(
+      internal::InsertObjectStreamingRequest const& request);
+
   // The version of UploadFile() where UseResumableUploadSession is one of the
   // options. Note how this does not use InsertObjectMedia at all.
   template <typename... Options>
diff --git a/google/cloud/storage/client_write_object_test.cc b/google/cloud/storage/client_write_object_test.cc
@@ -83,6 +83,8 @@ TEST_F(WriteObjectTest, WriteObject) {
             EXPECT_CALL(*mock_result, DoClose())
                 .WillRepeatedly(Return(internal::HttpResponse{200, text, {}}));
             EXPECT_CALL(*mock_result, IsOpen()).WillRepeatedly(Return(true));
+            EXPECT_CALL(*mock_result, ValidateHash(_))
+                .WillRepeatedly(Return(true));
             std::unique_ptr<internal::ObjectWriteStreambuf> result(mock_result);
             return make_status_or(std::move(result));
           }));
@@ -102,54 +104,29 @@ TEST_F(WriteObjectTest, WriteObjectTooManyFailures) {
     return StatusOr<std::unique_ptr<internal::ObjectWriteStreambuf>>(
         TransientError());
   };
-#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
   EXPECT_CALL(*mock, WriteObject(_))
       .WillOnce(Invoke(returner))
       .WillOnce(Invoke(returner))
       .WillOnce(Invoke(returner));
-  EXPECT_THROW(
-      try {
-        client.WriteObject("test-bucket-name", "test-object-name").Close();
-      } catch (std::runtime_error const& ex) {
-        EXPECT_THAT(ex.what(), HasSubstr("Retry policy exhausted"));
-        EXPECT_THAT(ex.what(), HasSubstr("WriteObject"));
-        throw;
-      },
-      std::runtime_error);
-#else
-  // With EXPECT_DEATH*() the mocking framework cannot detect how many times the
-  // operation is called.
-  EXPECT_CALL(*mock, WriteObject(_)).WillRepeatedly(Invoke(returner));
-  EXPECT_DEATH_IF_SUPPORTED(
-      client.WriteObject("test-bucket-name", "test-object-name").Close(),
-      "exceptions are disabled");
-#endif  // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
+
+  auto stream = client.WriteObject("test-bucket-name", "test-object-name");
+  EXPECT_TRUE(stream.bad());
+  EXPECT_FALSE(stream.metadata().status().ok());
+  EXPECT_EQ(TransientError().code(), stream.metadata().status().code())
+      << ", status=" << stream.metadata().status();
 }
 
 TEST_F(WriteObjectTest, WriteObjectPermanentFailure) {
   auto returner = [](internal::InsertObjectStreamingRequest const&) {
     return StatusOr<std::unique_ptr<internal::ObjectWriteStreambuf>>(
         PermanentError());
   };
-#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
   EXPECT_CALL(*mock, WriteObject(_)).WillOnce(Invoke(returner));
-  EXPECT_THROW(
-      try {
-        client->WriteObject("test-bucket-name", "test-object-name").Close();
-      } catch (std::runtime_error const& ex) {
-        EXPECT_THAT(ex.what(), HasSubstr("Permanent error"));
-        EXPECT_THAT(ex.what(), HasSubstr("WriteObject"));
-        throw;
-      },
-      std::runtime_error);
-#else
-  // With EXPECT_DEATH*() the mocking framework cannot detect how many times the
-  // operation is called.
-  EXPECT_CALL(*mock, WriteObject(_)).WillRepeatedly(Invoke(returner));
-  EXPECT_DEATH_IF_SUPPORTED(
-      client->WriteObject("test-bucket-name", "test-object-name").Close(),
-      "exceptions are disabled");
-#endif  // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
+  auto stream = client->WriteObject("test-bucket-name", "test-object-name");
+  EXPECT_TRUE(stream.bad());
+  EXPECT_FALSE(stream.metadata().status().ok());
+  EXPECT_EQ(PermanentError().code(), stream.metadata().status().code())
+      << ", status=" << stream.metadata().status();
 }
 
 }  // namespace
diff --git a/google/cloud/storage/internal/curl_client_test.cc b/google/cloud/storage/internal/curl_client_test.cc
@@ -195,11 +195,6 @@ TEST_P(CurlClientTest, InsertObjectMediaSimple) {
 }
 
 TEST_P(CurlClientTest, InsertObjectMediaMultipart) {
-  std::string const error_type = GetParam();
-  if (error_type != "credentials-failure") {
-    // TODO(#1735) - enable this test when ObjectWriteStream uses StatusOr.
-    return;
-  }
   auto actual = client_
                     ->InsertObjectMedia(
                         InsertObjectMediaRequest("bkt", "obj", "contents"))
diff --git a/google/cloud/storage/tests/object_media_integration_test.cc b/google/cloud/storage/tests/object_media_integration_test.cc
@@ -692,7 +692,7 @@ TEST_F(ObjectMediaIntegrationTest, ReadRangeXml) {
   EXPECT_STATUS_OK(status);
 }
 
-TEST_F(ObjectMediaIntegrationTest, ConnectionFailureJSON) {
+TEST_F(ObjectMediaIntegrationTest, ConnectionFailureReadJSON) {
   Client client{ClientOptions(oauth2::CreateAnonymousCredentials())
                     .set_endpoint("http://localhost:0"),
                 LimitedErrorCountRetryPolicy(2)};
@@ -713,7 +713,7 @@ TEST_F(ObjectMediaIntegrationTest, ConnectionFailureJSON) {
       << ", status=" << stream.status();
 }
 
-TEST_F(ObjectMediaIntegrationTest, ConnectionFailureXML) {
+TEST_F(ObjectMediaIntegrationTest, ConnectionFailureReadXML) {
   google::cloud::internal::SetEnv("CLOUD_STORAGE_TESTBENCH_ENDPOINT",
                                   "http://localhost:0");
   Client client{ClientOptions(oauth2::CreateAnonymousCredentials())
@@ -732,6 +732,81 @@ TEST_F(ObjectMediaIntegrationTest, ConnectionFailureXML) {
       << ", status=" << stream.status();
 }
 
+TEST_F(ObjectMediaIntegrationTest, ConnectionFailureWriteJSON) {
+  Client client{ClientOptions(oauth2::CreateAnonymousCredentials())
+                    .set_endpoint("http://localhost:0"),
+                LimitedErrorCountRetryPolicy(2)};
+
+  std::string bucket_name = flag_bucket_name;
+  auto object_name = MakeRandomObjectName();
+
+  // We force the library to use the JSON API by adding the
+  // `IfGenerationNotMatch()` parameter, both JSON and XML use the same code to
+  // download, but controlling the endpoint for JSON is easier.
+  auto stream = client.WriteObject(
+      bucket_name, object_name, IfGenerationMatch(0), IfGenerationNotMatch(7));
+  EXPECT_TRUE(stream.bad());
+  EXPECT_FALSE(stream.metadata().status().ok());
+  EXPECT_EQ(StatusCode::kUnavailable, stream.metadata().status().code())
+      << ", status=" << stream.metadata().status();
+}
+
+TEST_F(ObjectMediaIntegrationTest, ConnectionFailureWriteXML) {
+  google::cloud::internal::SetEnv("CLOUD_STORAGE_TESTBENCH_ENDPOINT",
+                                  "http://localhost:0");
+  Client client{ClientOptions(oauth2::CreateAnonymousCredentials())
+                    .set_endpoint("http://localhost:0"),
+                LimitedErrorCountRetryPolicy(2)};
+
+  std::string bucket_name = flag_bucket_name;
+  auto object_name = MakeRandomObjectName();
+
+  auto stream = client.WriteObject(
+      bucket_name, object_name, IfGenerationMatch(0), IfGenerationNotMatch(7));
+  EXPECT_TRUE(stream.bad());
+  EXPECT_FALSE(stream.metadata().status().ok());
+  EXPECT_EQ(StatusCode::kUnavailable, stream.metadata().status().code())
+      << ", status=" << stream.metadata().status();
+}
+
+TEST_F(ObjectMediaIntegrationTest, ConnectionFailureDownloadFile) {
+  google::cloud::internal::SetEnv("CLOUD_STORAGE_TESTBENCH_ENDPOINT",
+                                  "http://localhost:0");
+  Client client{ClientOptions(oauth2::CreateAnonymousCredentials())
+                    .set_endpoint("http://localhost:0"),
+                LimitedErrorCountRetryPolicy(2)};
+
+  std::string bucket_name = flag_bucket_name;
+  auto object_name = MakeRandomObjectName();
+  auto file_name = MakeRandomObjectName();
+
+  Status status = client.DownloadToFile(bucket_name, object_name, file_name);
+  EXPECT_FALSE(status.ok());
+  EXPECT_EQ(StatusCode::kUnavailable, status.code()) << ", status=" << status;
+}
+
+TEST_F(ObjectMediaIntegrationTest, ConnectionFailureUploadFile) {
+  google::cloud::internal::SetEnv("CLOUD_STORAGE_TESTBENCH_ENDPOINT",
+                                  "http://localhost:0");
+  Client client{ClientOptions(oauth2::CreateAnonymousCredentials())
+                    .set_endpoint("http://localhost:0"),
+                LimitedErrorCountRetryPolicy(2)};
+
+  std::string bucket_name = flag_bucket_name;
+  auto object_name = MakeRandomObjectName();
+  auto file_name = MakeRandomObjectName();
+
+  std::ofstream(file_name) << LoremIpsum();
+
+  StatusOr<ObjectMetadata> meta =
+      client.UploadFile(file_name, bucket_name, object_name);
+  EXPECT_FALSE(meta.ok()) << "value=" << meta.value();
+  EXPECT_EQ(StatusCode::kUnavailable, meta.status().code())
+      << ", status=" << meta.status();
+
+  EXPECT_EQ(0, std::remove(file_name.c_str()));
+}
+
 }  // anonymous namespace
 }  // namespace STORAGE_CLIENT_NS
 }  // namespace storage
diff --git a/google/cloud/storage/tests/object_resumable_write_integration_test.cc b/google/cloud/storage/tests/object_resumable_write_integration_test.cc
@@ -81,15 +81,12 @@ TEST_F(ObjectResumableWriteIntegrationTest, WriteWithContentTypeFailure) {
   std::ostringstream expected;
 
   // Create the object, but only if it does not exist already.
-  TestPermanentFailure([&] {
-    auto os = client->WriteObject(
-        bucket_name, object_name, IfGenerationMatch(0),
-        WithObjectMetadata(ObjectMetadata().set_content_type("text/plain")));
-    os.exceptions(std::ios_base::failbit);
-    os << LoremIpsum();
-    os.Close();
-    ObjectMetadata meta = os.metadata().value();
-  });
+  auto os = client->WriteObject(
+      bucket_name, object_name, IfGenerationMatch(0),
+      WithObjectMetadata(ObjectMetadata().set_content_type("text/plain")));
+  EXPECT_TRUE(os.bad());
+  EXPECT_FALSE(os.metadata().status().ok())
+      << ", status=" << os.metadata().status();
 }
 
 TEST_F(ObjectResumableWriteIntegrationTest, WriteWithUseResumable) {
