Use non-throwing JSON parser for ObjectMetadata. (#1745)

This is the last PR in the series to use the non-throwing version of
nl::json::parse(). It fixes #1685.

Author: coryan

google/cloud/storage/client_object_copy_test.cc

@@ -60,10 +60,8 @@ class ObjectCopyTest : public ::testing::Test {
 };
 
 TEST_F(ObjectCopyTest, CopyObject) {
-  std::string text = R"""({
-      "name": "test-bucket-name/test-object-name/1"
-})""";
-  auto expected = storage::ObjectMetadata::ParseFromString(text);
+  std::string text = R"""({"name": "test-bucket-name/test-object-name/1"})""";
+  auto expected = storage::ObjectMetadata::ParseFromString(text).value();
 
   EXPECT_CALL(*mock, CopyObject(_))
       .WillOnce(Invoke([&expected](internal::CopyObjectRequest const& request) {
@@ -76,9 +74,9 @@ TEST_F(ObjectCopyTest, CopyObject) {
   Client client{std::shared_ptr<internal::RawClient>(mock),
                 LimitedErrorCountRetryPolicy(2)};
 
-  ObjectMetadata actual = client.CopyObject(
-      "source-bucket-name", "source-object-name", "test-bucket-name",
-      "test-object-name");
+  ObjectMetadata actual =
+      client.CopyObject("source-bucket-name", "source-object-name",
+                        "test-bucket-name", "test-object-name");
   EXPECT_EQ(expected, actual);
 }
 
@@ -130,8 +128,8 @@ TEST_F(ObjectCopyTest, ComposeObject) {
       "timeStorageClassUpdated": "2018-05-19T19:31:34Z",
       "updated": "2018-05-19T19:31:24Z",
       "componentCount": 2
-})""";
-  auto expected = ObjectMetadata::ParseFromString(response);
+  })""";
+  auto expected = ObjectMetadata::ParseFromString(response).value();
 
   EXPECT_CALL(*mock, ComposeObject(_))
       .WillOnce(Return(StatusOr<ObjectMetadata>(TransientError())))
@@ -149,9 +147,8 @@ TEST_F(ObjectCopyTest, ComposeObject) {
   Client client{std::shared_ptr<internal::RawClient>(mock),
                 LimitedErrorCountRetryPolicy(2)};
 
-  auto actual =
-      client.ComposeObject("test-bucket-name", {{"object1"}, {"object2"}},
-                           "test-object-name");
+  auto actual = client.ComposeObject(
+      "test-bucket-name", {{"object1"}, {"object2"}}, "test-object-name");
   EXPECT_EQ(expected, actual);
 }
 
@@ -181,8 +178,8 @@ TEST_F(ObjectCopyTest, ComposeObjectPermanentFailure) {
 
 TEST_F(ObjectCopyTest, RewriteObject) {
   EXPECT_CALL(*mock, RewriteObject(_))
-      .WillOnce(Return(
-          StatusOr<internal::RewriteObjectResponse>(TransientError())))
+      .WillOnce(
+          Return(StatusOr<internal::RewriteObjectResponse>(TransientError())))
       .WillOnce(Invoke([](internal::RewriteObjectRequest const& r) {
         EXPECT_EQ("test-source-bucket-name", r.source_bucket());
         EXPECT_EQ("test-source-object-name", r.source_object());
@@ -197,8 +194,8 @@ TEST_F(ObjectCopyTest, RewriteObject) {
             "done": false,
             "rewriteToken": "abcd-test-token-0"
         })""";
-        return make_status_or(internal::RewriteObjectResponse::FromHttpResponse(
-                                  internal::HttpResponse{200, response, {}}));
+        return internal::RewriteObjectResponse::FromHttpResponse(
+            internal::HttpResponse{200, response, {}});
       }))
       .WillOnce(Invoke([](internal::RewriteObjectRequest const& r) {
         EXPECT_EQ("test-source-bucket-name", r.source_bucket());
@@ -214,8 +211,8 @@ TEST_F(ObjectCopyTest, RewriteObject) {
             "done": false,
             "rewriteToken": "abcd-test-token-2"
         })""";
-        return make_status_or(internal::RewriteObjectResponse::FromHttpResponse(
-                                  internal::HttpResponse{200, response, {}}));
+        return internal::RewriteObjectResponse::FromHttpResponse(
+            internal::HttpResponse{200, response, {}});
       }))
       .WillOnce(Invoke([](internal::RewriteObjectRequest const& r) {
         EXPECT_EQ("test-source-bucket-name", r.source_bucket());
@@ -235,8 +232,8 @@ TEST_F(ObjectCopyTest, RewriteObject) {
                "name": "test-destination-object-name"
             }
         })""";
-        return make_status_or(internal::RewriteObjectResponse::FromHttpResponse(
-                                  internal::HttpResponse{200, response, {}}));
+        return internal::RewriteObjectResponse::FromHttpResponse(
+            internal::HttpResponse{200, response, {}});
       }));
   Client client{std::shared_ptr<internal::RawClient>(mock),
                 LimitedErrorCountRetryPolicy(2)};
@@ -278,8 +275,7 @@ TEST_F(ObjectCopyTest, RewriteObjectTooManyFailures) {
       [](Client& client) {
         client.RewriteObjectBlocking(
             "test-source-bucket-name", "test-source-object",
-            "test-dest-bucket-name", "test-dest-object",
-            IfGenerationMatch(7));
+            "test-dest-bucket-name", "test-dest-object", IfGenerationMatch(7));
       },
       "RewriteObject");
 }

google/cloud/storage/client_write_object_test.cc

@@ -71,7 +71,7 @@ TEST_F(WriteObjectTest, WriteObject) {
   std::string text = R"""({
       "name": "test-bucket-name/test-object-name/1"
 })""";
-  auto expected = ObjectMetadata::ParseFromString(text);
+  auto expected = ObjectMetadata::ParseFromString(text).value();
 
   EXPECT_CALL(*mock, WriteObject(_))
       .WillOnce(Invoke(

google/cloud/storage/internal/curl_client.cc

@@ -215,14 +215,17 @@ CurlClient::CreateResumableSessionGeneric(RequestType const& request) {
   }
   auto response =
       ResumableUploadResponse::FromHttpResponse(*std::move(http_response));
-  if (response.upload_session_url.empty()) {
+  if (not response.ok()) {
+    return std::move(response).status();
+  }
+  if (response->upload_session_url.empty()) {
     std::ostringstream os;
-    os << __func__ << " - invalid server response, parsed to " << response;
+    os << __func__ << " - invalid server response, parsed to " << *response;
     return Status(StatusCode::INTERNAL, std::move(os).str());
   }
   return std::unique_ptr<ResumableUploadSession>(
       google::cloud::internal::make_unique<CurlResumableUploadSession>(
-          shared_from_this(), std::move(response.upload_session_url)));
+          shared_from_this(), std::move(response->upload_session_url)));
 }
 
 CurlClient::CurlClient(ClientOptions options)

google/cloud/storage/internal/hash_validator_test.cc

@@ -259,7 +259,7 @@ TEST(CompositeHashValidator, ProcessMetadata) {
   auto object_metadata = ObjectMetadata::ParseFromJson(internal::nl::json{
       {"crc32c", QUICK_FOX_CRC32C_CHECKSUM},
       {"md5Hash", QUICK_FOX_MD5_HASH},
-  });
+  }).value();
   validator.ProcessMetadata(object_metadata);
   auto result = std::move(validator).Finish();
   EXPECT_EQ(

google/cloud/storage/internal/logging_client_test.cc

@@ -119,7 +119,7 @@ TEST_F(LoggingClientTest, InsertObjectMedia) {
 
   auto mock = std::make_shared<testing::MockClient>();
   EXPECT_CALL(*mock, InsertObjectMedia(_))
-      .WillOnce(Return(ObjectMetadata::ParseFromString(text)));
+      .WillOnce(Return(ObjectMetadata::ParseFromString(text).value()));
 
   // We want to test that the key elements are logged, but do not want a
   // "change detection test", so this is intentionally not exhaustive.
@@ -145,8 +145,10 @@ TEST_F(LoggingClientTest, InsertObjectMedia) {
 
 TEST_F(LoggingClientTest, ListObjects) {
   std::vector<ObjectMetadata> items = {
-      ObjectMetadata::ParseFromString(R""({"name": "response-object-o1"})""),
-      ObjectMetadata::ParseFromString(R""({"name": "response-object-o2"})""),
+      ObjectMetadata::ParseFromString(R""({"name": "response-object-o1"})"")
+          .value(),
+      ObjectMetadata::ParseFromString(R""({"name": "response-object-o2"})"")
+          .value(),
   };
   auto mock = std::make_shared<testing::MockClient>();
   EXPECT_CALL(*mock, ListObjects(_))

google/cloud/storage/internal/object_requests.cc

@@ -30,15 +30,23 @@ std::ostream& operator<<(std::ostream& os, ListObjectsRequest const& r) {
   return os << "}";
 }
 
-ListObjectsResponse ListObjectsResponse::FromHttpResponse(
+StatusOr<ListObjectsResponse> ListObjectsResponse::FromHttpResponse(
     HttpResponse&& response) {
-  auto json = storage::internal::nl::json::parse(response.payload);
+  auto json =
+      storage::internal::nl::json::parse(response.payload, nullptr, false);
+  if (not json.is_object()) {
+    return Status(StatusCode::INVALID_ARGUMENT, __func__);
+  }
 
   ListObjectsResponse result;
   result.next_page_token = json.value("nextPageToken", "");
 
   for (auto const& kv : json["items"].items()) {
-    result.items.emplace_back(ObjectMetadata::ParseFromJson(kv.value()));
+    auto parsed = ObjectMetadata::ParseFromJson(kv.value());
+    if (not parsed.ok()) {
+      return std::move(parsed).status();
+    }
+    result.items.emplace_back(std::move(*parsed));
   }
 
   return result;
@@ -170,20 +178,20 @@ std::ostream& operator<<(std::ostream& os, UpdateObjectRequest const& r) {
 }
 
 ComposeObjectRequest::ComposeObjectRequest(
-    std::string bucket_name,
-    std::vector<ComposeSourceObject> source_objects,
+    std::string bucket_name, std::vector<ComposeSourceObject> source_objects,
     std::string destination_object_name)
     : GenericObjectRequest(std::move(bucket_name),
                            std::move(destination_object_name)),
-                           source_objects_(std::move(source_objects)) {}
+      source_objects_(std::move(source_objects)) {}
 
 std::string ComposeObjectRequest::JsonPayload() const {
   using internal::nl::json;
   json compose_object_payload_json;
   compose_object_payload_json["kind"] = "storage#composeRequest";
   json destination_metadata_payload;
   if (HasOption<WithObjectMetadata>()) {
-    destination_metadata_payload = GetOption<WithObjectMetadata>().value().JsonPayloadForCompose();
+    destination_metadata_payload =
+        GetOption<WithObjectMetadata>().value().JsonPayloadForCompose();
   }
   if (!destination_metadata_payload.is_null()) {
     compose_object_payload_json["destination"] = destination_metadata_payload;
@@ -305,17 +313,25 @@ std::ostream& operator<<(std::ostream& os, RewriteObjectRequest const& r) {
   return os << "}";
 }
 
-RewriteObjectResponse RewriteObjectResponse::FromHttpResponse(
+StatusOr<RewriteObjectResponse> RewriteObjectResponse::FromHttpResponse(
     HttpResponse const& response) {
-  nl::json object = nl::json::parse(response.payload);
+  nl::json object = nl::json::parse(response.payload, nullptr, false);
+  if (not object.is_object()) {
+    return Status(StatusCode::INVALID_ARGUMENT, __func__);
+  }
+
   RewriteObjectResponse result;
   result.total_bytes_rewritten =
       ParseUnsignedLongField(object, "totalBytesRewritten");
   result.object_size = ParseUnsignedLongField(object, "objectSize");
   result.done = object.value("done", false);
   result.rewrite_token = object.value("rewriteToken", "");
   if (object.count("resource") != 0U) {
-    result.resource = ObjectMetadata::ParseFromJson(object["resource"]);
+    auto parsed = ObjectMetadata::ParseFromJson(object["resource"]);
+    if (not parsed.ok()) {
+      return std::move(parsed).status();
+    }
+    result.resource = std::move(*parsed);
   }
   return result;
 }
@@ -373,7 +389,7 @@ std::ostream& operator<<(std::ostream& os,
   return os << "}";
 }
 
-ResumableUploadResponse ResumableUploadResponse::FromHttpResponse(
+StatusOr<ResumableUploadResponse> ResumableUploadResponse::FromHttpResponse(
     HttpResponse&& response) {
   ResumableUploadResponse result;
   result.last_committed_byte = 0;

google/cloud/storage/internal/object_requests.h

@@ -53,7 +53,8 @@ class ListObjectsRequest
 std::ostream& operator<<(std::ostream& os, ListObjectsRequest const& r);
 
 struct ListObjectsResponse {
-  static ListObjectsResponse FromHttpResponse(HttpResponse&& response);
+  static StatusOr<ListObjectsResponse> FromHttpResponse(
+      HttpResponse&& response);
 
   std::string next_page_token;
   std::vector<ObjectMetadata> items;
@@ -344,7 +345,8 @@ std::ostream& operator<<(std::ostream& os, RewriteObjectRequest const& r);
 
 /// Holds an `Objects: rewrite` response.
 struct RewriteObjectResponse {
-  static RewriteObjectResponse FromHttpResponse(HttpResponse const& response);
+  static StatusOr<RewriteObjectResponse> FromHttpResponse(
+      HttpResponse const& response);
 
   std::uint64_t total_bytes_rewritten;
   std::uint64_t object_size;
@@ -447,7 +449,8 @@ std::ostream& operator<<(std::ostream& os,
                          QueryResumableUploadRequest const& r);
 
 struct ResumableUploadResponse {
-  static ResumableUploadResponse FromHttpResponse(HttpResponse&& response);
+  static StatusOr<ResumableUploadResponse> FromHttpResponse(
+      HttpResponse&& response);
 
   std::string upload_session_url;
   std::uint64_t last_committed_byte;