fix(rest): return complete payloads for errors

google/cloud/internal/curl_impl.cc

@@ -181,7 +181,6 @@ void CurlImpl::ApplyOptions(Options const& options) {
   transfer_stall_minimum_rate_ = options.get<TransferStallMinimumRateOption>();
   download_stall_timeout_ = options.get<DownloadStallTimeoutOption>();
   transfer_stall_minimum_rate_ = options.get<DownloadStallMinimumRateOption>();
-  ignored_http_error_codes_ = options.get<IgnoredHttpErrorCodes>();
 }
 
 CurlImpl::CurlImpl(CurlHandle handle,
@@ -654,16 +653,7 @@ StatusOr<std::size_t> CurlImpl::ReadImpl(absl::Span<char> output) {
 
   if (curl_closed_) {
     OnTransferDone();
-    status = google::cloud::rest_internal::AsStatus(
-        static_cast<HttpStatusCode>(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());

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<std::int32_t> 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

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<UnifiedCredentialsOption>(MakeInsecureCredentials());
-  auto client =
-      MakeDefaultRestClient(url_, Options{}.set<IgnoredHttpErrorCodes>({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<HttpPayload> payload = std::move(*response).ExtractPayload();
-  auto body = ReadAll(std::move(payload));
-  EXPECT_STATUS_OK(body);
-}
-
-TEST_F(RestClientIntegrationTest, GetWithNonIgnoredErrorCode) {
-  options_.set<UnifiedCredentialsOption>(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<UnifiedCredentialsOption>(MakeInsecureCredentials());
   options_.set<UserIpOption>("127.0.0.1");

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<std::int32_t>;
-};
-
 /// 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

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) {

google/cloud/storage/client_options.cc

@@ -241,32 +241,25 @@ Options DefaultOptions(std::shared_ptr<oauth2::Credentials> 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<rest::DownloadStallTimeoutOption>(
-              o.get<DownloadStallTimeoutOption>())
-          .set<rest::DownloadStallMinimumRateOption>(
-              o.get<DownloadStallMinimumRateOption>())
-          .set<rest::TransferStallTimeoutOption>(
-              o.get<TransferStallTimeoutOption>())
-          .set<rest::TransferStallMinimumRateOption>(
-              o.get<TransferStallMinimumRateOption>())
-          .set<rest::MaximumCurlSocketRecvSizeOption>(
-              o.get<MaximumCurlSocketRecvSizeOption>())
-          .set<rest::MaximumCurlSocketSendSizeOption>(
-              o.get<MaximumCurlSocketSendSizeOption>())
-          .set<rest::ConnectionPoolSizeOption>(
-              o.get<ConnectionPoolSizeOption>())
-          .set<rest::EnableCurlSslLockingOption>(
-              o.get<EnableCurlSslLockingOption>())
-          .set<rest::EnableCurlSigpipeHandlerOption>(
-              o.get<EnableCurlSigpipeHandlerOption>())
-          // 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::IgnoredHttpErrorCodes>(
-              {rest::HttpStatusCode::kResumeIncomplete,
-               rest::HttpStatusCode::kClientClosedRequest});
+  auto rest_defaults = Options{}
+                           .set<rest::DownloadStallTimeoutOption>(
+                               o.get<DownloadStallTimeoutOption>())
+                           .set<rest::DownloadStallMinimumRateOption>(
+                               o.get<DownloadStallMinimumRateOption>())
+                           .set<rest::TransferStallTimeoutOption>(
+                               o.get<TransferStallTimeoutOption>())
+                           .set<rest::TransferStallMinimumRateOption>(
+                               o.get<TransferStallMinimumRateOption>())
+                           .set<rest::MaximumCurlSocketRecvSizeOption>(
+                               o.get<MaximumCurlSocketRecvSizeOption>())
+                           .set<rest::MaximumCurlSocketSendSizeOption>(
+                               o.get<MaximumCurlSocketSendSizeOption>())
+                           .set<rest::ConnectionPoolSizeOption>(
+                               o.get<ConnectionPoolSizeOption>())
+                           .set<rest::EnableCurlSslLockingOption>(
+                               o.get<EnableCurlSslLockingOption>())
+                           .set<rest::EnableCurlSigpipeHandlerOption>(
+                               o.get<EnableCurlSigpipeHandlerOption>());
 
   // 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::`.

google/cloud/storage/client_options_test.cc

@@ -386,10 +386,6 @@ TEST_F(ClientOptionsTest, DefaultOptions) {
   EXPECT_EQ(o.get<rest::EnableCurlSigpipeHandlerOption>(),
             o.get<EnableCurlSigpipeHandlerOption>());
 
-  EXPECT_THAT(o.get<rest::IgnoredHttpErrorCodes>(),
-              UnorderedElementsAre(rest::kResumeIncomplete,
-                                   rest::kClientClosedRequest));
-
   EXPECT_FALSE(o.has<rest::HttpVersionOption>());
   EXPECT_FALSE(o.has<rest::CAPathOption>());
 }

google/cloud/storage/internal/rest_client.cc

@@ -634,7 +634,9 @@ StatusOr<std::unique_ptr<ObjectReadSource>> 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<ObjectReadSource>(
       new RestObjectReadSource(*std::move(response)));
 }
@@ -667,6 +669,9 @@ StatusOr<std::unique_ptr<ObjectReadSource>> 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<ObjectReadSource>(
       new RestObjectReadSource(*std::move(response)));

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());