From 80079aab6c7d2154f320752cb1316b47c24a0f0a Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Thu, 4 Apr 2019 09:46:21 -0400 Subject: [PATCH] Do not throw exceptions in WriteObject(). 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.) --- google/cloud/storage/client.cc | 14 ++++ google/cloud/storage/client.h | 7 +- .../cloud/storage/client_write_object_test.cc | 49 +++--------- .../storage/internal/curl_client_test.cc | 5 -- .../tests/object_media_integration_test.cc | 79 ++++++++++++++++++- ...object_resumable_write_integration_test.cc | 15 ++-- 6 files changed, 115 insertions(+), 54 deletions(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index 5d0d5ca76f943..ffe55f7e40768 100644 --- 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 index 841c420bd872f..d75fe6a2cd4f2 100644 --- 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)...); - 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)...); - 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 diff --git a/google/cloud/storage/client_write_object_test.cc b/google/cloud/storage/client_write_object_test.cc index 45c222b8d1080..c35e20891b7ce 100644 --- 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 result(mock_result); return make_status_or(std::move(result)); })); @@ -102,28 +104,16 @@ TEST_F(WriteObjectTest, WriteObjectTooManyFailures) { return StatusOr>( 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) { @@ -131,25 +121,12 @@ TEST_F(WriteObjectTest, WriteObjectPermanentFailure) { return StatusOr>( 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 index 091ee8616aa57..949132078df6f 100644 --- 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 index 488a0cc37a2a5..92da1e8df9499 100644 --- 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 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 index 2b54d597d6d77..c5d5c3ba93597 100644 --- 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) {