diff --git a/google/cloud/internal/curl_impl.cc b/google/cloud/internal/curl_impl.cc index 078922543e543..82acc006f71e8 100644 --- a/google/cloud/internal/curl_impl.cc +++ b/google/cloud/internal/curl_impl.cc @@ -181,7 +181,6 @@ void CurlImpl::ApplyOptions(Options const& options) { transfer_stall_minimum_rate_ = options.get(); download_stall_timeout_ = options.get(); transfer_stall_minimum_rate_ = options.get(); - ignored_http_error_codes_ = options.get(); } CurlImpl::CurlImpl(CurlHandle handle, @@ -654,16 +653,7 @@ StatusOr CurlImpl::ReadImpl(absl::Span output) { if (curl_closed_) { OnTransferDone(); - status = google::cloud::rest_internal::AsStatus( - static_cast(http_code_), {}); - TRACE_STATE() << ", status=" << status << ", http code=" << http_code_ - << "\n"; - - if (status.ok() || - internal::Contains(ignored_http_error_codes_, http_code_)) { - return bytes_read; - } - return status; + return bytes_read; } TRACE_STATE() << ", http code=" << http_code_ << "\n"; received_headers_.emplace(":curl-peer", handle_.GetPeer()); diff --git a/google/cloud/internal/curl_impl.h b/google/cloud/internal/curl_impl.h index 84e4ac963c98d..e7f2defc47260 100644 --- a/google/cloud/internal/curl_impl.h +++ b/google/cloud/internal/curl_impl.h @@ -132,7 +132,6 @@ class CurlImpl { std::uint32_t download_stall_minimum_rate_ = 1; std::string http_version_; std::int32_t http_code_; - std::set ignored_http_error_codes_; // Explicitly closing the handle happens in two steps. // 1. This class needs to notify libcurl that the transfer is terminated by diff --git a/google/cloud/internal/curl_rest_client_integration_test.cc b/google/cloud/internal/curl_rest_client_integration_test.cc index 3a3ddabb1c060..48b912017a783 100644 --- a/google/cloud/internal/curl_rest_client_integration_test.cc +++ b/google/cloud/internal/curl_rest_client_integration_test.cc @@ -28,7 +28,6 @@ namespace rest_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN namespace { -using ::google::cloud::testing_util::IsOk; using ::testing::Contains; using ::testing::Eq; using ::testing::HasSubstr; @@ -148,31 +147,6 @@ TEST_F(RestClientIntegrationTest, Get) { EXPECT_GT(body->size(), 0); } -TEST_F(RestClientIntegrationTest, GetWithIgnoredErrorCode) { - options_.set(MakeInsecureCredentials()); - auto client = - MakeDefaultRestClient(url_, Options{}.set({308})); - RestRequest request; - request.SetPath("status/308"); - auto response_status = RetryRestRequest([&] { return client->Get(request); }); - ASSERT_STATUS_OK(response_status); - auto response = std::move(response_status.value()); - EXPECT_THAT(response->StatusCode(), Eq(HttpStatusCode::kResumeIncomplete)); - EXPECT_GT(response->Headers().size(), 0); - std::unique_ptr payload = std::move(*response).ExtractPayload(); - auto body = ReadAll(std::move(payload)); - EXPECT_STATUS_OK(body); -} - -TEST_F(RestClientIntegrationTest, GetWithNonIgnoredErrorCode) { - options_.set(MakeInsecureCredentials()); - auto client = MakeDefaultRestClient(url_, {}); - RestRequest request; - request.SetPath("status/308"); - auto response_status = RetryRestRequest([&] { return client->Get(request); }); - EXPECT_THAT(response_status, Not(IsOk())); -} - TEST_F(RestClientIntegrationTest, Delete) { options_.set(MakeInsecureCredentials()); options_.set("127.0.0.1"); diff --git a/google/cloud/internal/rest_options.h b/google/cloud/internal/rest_options.h index f6df271cf0549..f1fd4f23e622e 100644 --- a/google/cloud/internal/rest_options.h +++ b/google/cloud/internal/rest_options.h @@ -95,20 +95,10 @@ struct DownloadStallMinimumRateOption { using Type = std::int32_t; }; -/** - * Some services appropriate Http error codes for their own use. If any such - * error codes need to be treated as non-failures, this option can indicate - * which codes. - */ -struct IgnoredHttpErrorCodes { - using Type = std::set; -}; - /// The complete list of options accepted by `CurlRestClient` using RestOptionList = ::google::cloud::OptionList< UserIpOption, TransferStallTimeoutOption, TransferStallMinimumRateOption, - DownloadStallTimeoutOption, DownloadStallMinimumRateOption, - IgnoredHttpErrorCodes>; + DownloadStallTimeoutOption, DownloadStallMinimumRateOption>; GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace rest_internal diff --git a/google/cloud/internal/rest_response.cc b/google/cloud/internal/rest_response.cc index 83f64bb14cd2e..b30f06ff536a3 100644 --- a/google/cloud/internal/rest_response.cc +++ b/google/cloud/internal/rest_response.cc @@ -49,7 +49,7 @@ StatusCode MapHttpCodeToStatus3xx(std::int32_t code) { // is a success status. // // This level of complexity / detail is something that the caller should - // handle, that is what `IgnoredHttpErrorCodes` are for. + // handle. return StatusCode::kFailedPrecondition; } if (code == HttpStatusCode::kNotModified) { diff --git a/google/cloud/storage/client_options.cc b/google/cloud/storage/client_options.cc index c2b89b9a5bc86..b86c393527631 100644 --- a/google/cloud/storage/client_options.cc +++ b/google/cloud/storage/client_options.cc @@ -241,32 +241,25 @@ Options DefaultOptions(std::shared_ptr credentials, // use the low-level initialization code in // google/cloud/internal/curl_wrappers.cc, these are always needed. namespace rest = ::google::cloud::rest_internal; - auto rest_defaults = - Options{} - .set( - o.get()) - .set( - o.get()) - .set( - o.get()) - .set( - o.get()) - .set( - o.get()) - .set( - o.get()) - .set( - o.get()) - .set( - o.get()) - .set( - o.get()) - // This prevents the RestClient from treating these codes as errors. - // Instead, it will allow them to propagate back to the calling code - // where it can determine if they are indeed errors or not. - .set( - {rest::HttpStatusCode::kResumeIncomplete, - rest::HttpStatusCode::kClientClosedRequest}); + auto rest_defaults = Options{} + .set( + o.get()) + .set( + o.get()) + .set( + o.get()) + .set( + o.get()) + .set( + o.get()) + .set( + o.get()) + .set( + o.get()) + .set( + o.get()) + .set( + o.get()); // These two are not always present, but if they are, and only if they are, we // need to map their value to the corresponding option in `rest_internal::`. diff --git a/google/cloud/storage/client_options_test.cc b/google/cloud/storage/client_options_test.cc index b323608f0bc26..9596c575c48e8 100644 --- a/google/cloud/storage/client_options_test.cc +++ b/google/cloud/storage/client_options_test.cc @@ -386,10 +386,6 @@ TEST_F(ClientOptionsTest, DefaultOptions) { EXPECT_EQ(o.get(), o.get()); - EXPECT_THAT(o.get(), - UnorderedElementsAre(rest::kResumeIncomplete, - rest::kClientClosedRequest)); - EXPECT_FALSE(o.has()); EXPECT_FALSE(o.has()); } diff --git a/google/cloud/storage/internal/rest_client.cc b/google/cloud/storage/internal/rest_client.cc index 0f9a3e9fb48e9..38767984a1a96 100644 --- a/google/cloud/storage/internal/rest_client.cc +++ b/google/cloud/storage/internal/rest_client.cc @@ -634,7 +634,9 @@ StatusOr> RestClient::ReadObjectXml( auto response = storage_rest_client_->Get(std::move(builder).BuildRequest()); if (!response.ok()) return std::move(response).status(); - + if (IsHttpError((*response)->StatusCode())) { + return rest::AsStatus(std::move(**response)); + } return std::unique_ptr( new RestObjectReadSource(*std::move(response))); } @@ -667,6 +669,9 @@ StatusOr> RestClient::ReadObject( auto response = storage_rest_client_->Get(std::move(builder).BuildRequest()); if (!response.ok()) return response.status(); + if (IsHttpError((*response)->StatusCode())) { + return rest::AsStatus(std::move(**response)); + } return std::unique_ptr( new RestObjectReadSource(*std::move(response))); 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 117f80905e08e..ce8e9d88863f3 100644 --- a/google/cloud/storage/tests/object_resumable_write_integration_test.cc +++ b/google/cloud/storage/tests/object_resumable_write_integration_test.cc @@ -28,6 +28,7 @@ namespace { using ::testing::AnyOf; using ::testing::Eq; using ::testing::HasSubstr; +using ::testing::IsEmpty; using ::testing::Not; class ObjectResumableWriteIntegrationTest @@ -311,6 +312,13 @@ TEST_F(ObjectResumableWriteIntegrationTest, StreamingWriteFailure) { AnyOf(Eq(StatusCode::kFailedPrecondition), Eq(StatusCode::kAborted))) << " status=" << os.metadata().status(); + if (os.metadata().status().code() == StatusCode::kFailedPrecondition && + !UsingEmulator() && !UsingGrpc()) { + EXPECT_THAT(os.metadata().status().message(), Not(IsEmpty())); + EXPECT_EQ(os.metadata().status().error_info().domain(), "global"); + EXPECT_EQ(os.metadata().status().error_info().reason(), "conditionNotMet"); + } + auto status = client->DeleteObject(bucket_name_, object_name); EXPECT_STATUS_OK(status); } @@ -433,8 +441,8 @@ TEST_F(ObjectResumableWriteIntegrationTest, WithInvalidXUploadContentLength) { offset += n; } - // This operation should fail because the x-upload-content-length header does - // not match the amount of data sent in the upload. + // This operation should fail because the x-upload-content-length header + // does not match the amount of data sent in the upload. os.Close(); EXPECT_TRUE(os.bad()); EXPECT_FALSE(os.metadata().ok());