Drive: Add response handling to BatchUploadRequst class.

google_apis/drive/drive_api_requests.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "google_apis/drive/drive_api_requests.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/json/json_writer.h"
 #include "base/location.h"
@@ skipping 18 matching lines @@
     "Content-Type: %s\n"
     "\n"
     "%s";
 
 // Request header for specifying batch upload.
 const char kBatchUploadHeader[] = "X-Goog-Upload-Protocol: batch";
 
 // Content type of HTTP request.
 const char kHttpContentType[] = "application/http";
 
+// Break line in HTTP message.
+const std::string kHttpBr = "\r\n";
+
 // Parses the JSON value to FileResource instance and runs |callback| on the
 // UI thread once parsing is done.
 // This is customized version of ParseJsonAndRun defined above to adapt the
 // remaining response type.
 void ParseFileResourceWithUploadRangeAndRun(const UploadRangeCallback& callback,
                                             const UploadRangeResponse& response,
                                             scoped_ptr<base::Value> value) {
   DCHECK(!callback.is_null());
 
   scoped_ptr<FileResource> file_resource;
@@ skipping 70 matching lines @@
     root.SetString("lastViewedByMeDate", google_apis::util::FormatTimeAsString(
                                              last_viewed_by_me_date));
   }
 
   AttachProperties(properties, &root);
   std::string json_string;
   base::JSONWriter::Write(&root, &json_string);
   return json_string;
 }
 
+// Checks if |str| at |pos| starts with |prefix|.
+bool StartWith(const std::string& str, size_t pos, const std::string& prefix) {
+  if (pos + prefix.size() > str.size())
+    return false;
+  return std::equal(prefix.begin(), prefix.end(), str.begin() + pos);
+}
+
+// Splits multipart |response| into |parts|. Returns true on succcess.

[kinaba, 2015/04/14 06:25:26]

Link to the spec (and preferably a short summary of it as the comment) of
multipart response will be very useful.

[kinaba, 2015/04/14 06:25:26]

I think these util functions are complex enough for adding specific unit tests
(not just as an integrated test within the response parsing.) Could you add such
tests?

[hirono, 2015/04/27 06:25:07]

Done.

+bool SplitMultipartResponse(const std::string& response,
+                            std::vector<std::string>* parts) {
+  const std::string kMultipartTerminator = "--";
+  std::string boundary;
+  {
+    const size_t pos = response.find(kHttpBr, 0);
+    if (pos == std::string::npos)
+      return false;
+    boundary = response.substr(0, pos);

[kinaba, 2015/04/14 06:25:25]

Can't you get the boundary from HTTP response header (Content-Type:
multipart/midex; boundary=HERE) instead of the first line? I thought there can
be a preamble before the first boundary line, so the current code may not be
valid in such case.

[hirono, 2015/04/27 06:25:07]

Done.

+  }
+  size_t pos = 0;
+  parts->clear();
+  while (true) {
+    pos += boundary.size();
+    if (StartWith(response, pos, kMultipartTerminator))
+      return true;
+    else if (StartWith(response, pos, kHttpBr))
+      pos += kHttpBr.size();
+    else
+      return false;
+
+    const size_t next_pos = response.find(boundary, pos);

[kinaba, 2015/04/14 06:25:26]

IIUC boundary may still appear in content body (while \r\n--boundary\r\n must
not), so you probably need additional care.

[hirono, 2015/04/27 06:25:07]

Done.

+    if (next_pos == std::string::npos)
+      return false;
+    parts->push_back(response.substr(pos, next_pos - pos));
+    pos = next_pos;
+  }
+  return true;
+}
+
+// Parses HTTP response in multipart entry and obtain |code| and |body|.
+// Returns true on succcess.
+bool ParseMultipartEntry(const std::string& entry,
+                         DriveApiErrorCode* code,
+                         std::string* body) {
+  const std::string kHttpStatusPrefix = "HTTP/1.1 ";
+
+  // Skip content type line of multipart.
+  size_t pos = entry.find(kHttpBr + kHttpBr, 0);
+  if (pos == std::string::npos)
+    return false;
+  pos += kHttpBr.size() * 2;

[kinaba, 2015/04/14 06:25:26]

Btw, I guess it is in general easy to split all the response first by kHttpBr
into vector<string>, and then apply the multipart related jobs in the
line-oriented manner.

If you feel anxious about performance on splitting and re-concatenating, you can
use vector<string::iterator> or maybe StringPiece.

[hirono, 2015/04/27 06:25:07]

Done.

+
+  // Read status line of HTTP.
+  if (!StartWith(entry, pos, kHttpStatusPrefix))
+    return false;
+  pos += kHttpStatusPrefix.size();
+
+  *code = static_cast<DriveApiErrorCode>(atoi(entry.c_str() + pos));
+  if (*code == 0)
+    return false;
+
+  // Skip headers of HTTP.
+  pos = entry.find(kHttpBr + kHttpBr, pos);
+  if (pos == std::string::npos)
+    return false;
+  pos += kHttpBr.size() * 2;
+
+  // Read body.
+  *body = entry.substr(pos);
+
+  return true;
+}
+
 }  // namespace
 
 Property::Property() : visibility_(VISIBILITY_PRIVATE) {
 }
 
 Property::~Property() {
 }
 
 //============================ DriveApiPartialFieldRequest ====================
 
@@ skipping 868 matching lines @@
 void BatchUploadRequest::SetBoundaryForTesting(const std::string& boundary) {
   boundary_ = boundary;
 }
 
 void BatchUploadRequest::AddRequest(BatchableRequestBase* request) {
   DCHECK(CalledOnValidThread());
   DCHECK(request);
   DCHECK(GetChildEntry(request) == child_requests_.end());
   DCHECK(!committed_);
   child_requests_.push_back(BatchUploadChildEntry(request));
-  request->Prepare(
-      base::Bind(&BatchUploadRequest::OnChildRequestPrepared,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 request));
+  request->Prepare(base::Bind(&BatchUploadRequest::OnChildRequestPrepared,
+                              weak_ptr_factory_.GetWeakPtr(), request));
 }
 
-void BatchUploadRequest::OnChildRequestPrepared(
-    RequestID request_id, DriveApiErrorCode result) {
+void BatchUploadRequest::OnChildRequestPrepared(RequestID request_id,
+                                                DriveApiErrorCode result) {
   DCHECK(CalledOnValidThread());
   auto const child = GetChildEntry(request_id);
   DCHECK(child != child_requests_.end());
   if (IsSuccessfulDriveApiErrorCode(result)) {
     child->prepared = true;
   } else {
     child->request->RunCallbackOnPrematureFailure(result);
     sender_->RequestFinished(child->request);
     child_requests_.erase(child);
   }
@@ skipping 19 matching lines @@
   for (auto& child : child_requests_) {
     // Request cancel should delete the request instance.
     child.request->Cancel();
   }
   child_requests_.clear();
   UrlFetchRequestBase::Cancel();
 }
 
 // Obtains corresponding child entry of |request_id|. Returns NULL if the
 // entry is not found.
-std::vector<BatchUploadChildEntry>::iterator
-BatchUploadRequest::GetChildEntry(RequestID request_id) {
+std::vector<BatchUploadChildEntry>::iterator BatchUploadRequest::GetChildEntry(
+    RequestID request_id) {
   for (auto it = child_requests_.begin(); it != child_requests_.end(); ++it) {
     if (it->request == request_id)
       return it;
   }
   return child_requests_.end();
 }
 
 void BatchUploadRequest::MayCompletePrepare() {
   if (!committed_ || prepare_callback_.is_null())
     return;
@@ skipping 21 matching lines @@
         method = "PUT";
         break;
       default:
         NOTREACHED();
         break;
     }
 
     parts.push_back(ContentTypeAndData());
     parts.back().type = kHttpContentType;
     parts.back().data = base::StringPrintf(
-        kBatchUploadRequestFormat,
-        method.c_str(),
-        url.path().c_str(),
-        url_generator_.GetBatchUploadUrl().host().c_str(),
-        type.c_str(),
+        kBatchUploadRequestFormat, method.c_str(), url.path().c_str(),
+        url_generator_.GetBatchUploadUrl().host().c_str(), type.c_str(),
         data.c_str());
   }
 
   GenerateMultipartBody(MULTIPART_MIXED, boundary_, parts, &upload_content_);
   prepare_callback_.Run(HTTP_SUCCESS);
 }
 
 bool BatchUploadRequest::GetContentData(std::string* upload_content_type,
                                         std::string* upload_content_data) {
   upload_content_type->assign(upload_content_.type);
@@ skipping 20 matching lines @@
   return headers;
 }
 
 void BatchUploadRequest::ProcessURLFetchResults(const net::URLFetcher* source) {
   if (!IsSuccessfulDriveApiErrorCode(GetErrorCode())) {
     RunCallbackOnPrematureFailure(GetErrorCode());
     sender_->RequestFinished(this);
     return;
   }
 
-  for (auto& child : child_requests_) {
-    // TODO(hirono): Split the mutlipart result and return the correct code and
-    // body.
-    child.request->ProcessURLFetchResults(HTTP_SERVICE_UNAVAILABLE, "");
-    // Request deletes itself after processing.
+  std::vector<std::string> parts;
+  if (!SplitMultipartResponse(response_writer()->data(), &parts) ||
+      child_requests_.size() != parts.size()) {
+    RunCallbackOnPrematureFailure(DRIVE_PARSE_ERROR);
+    sender_->RequestFinished(this);
+    return;
+  }
+
+  for (size_t i = 0; i < parts.size(); ++i) {
+    DriveApiErrorCode code;
+    std::string body;
+    if (ParseMultipartEntry(parts[i], &code, &body)) {
+      child_requests_[i].request->ProcessURLFetchResults(code, body);
+      // Request deletes itself after processing.
+    } else {
+      child_requests_[i].request->RunCallbackOnPrematureFailure(
+          DRIVE_PARSE_ERROR);
+      sender_->RequestFinished(child_requests_[i].request);
+    }
   }
 
   child_requests_.clear();
 }

[kinaba, 2015/04/14 06:25:25]

Maybe I should have asked this in the previous CL?
Where is |this| deleted in this successful branch?

[hirono, 2015/04/27 06:25:07]

Done.

 
 void BatchUploadRequest::RunCallbackOnPrematureFailure(DriveApiErrorCode code) {
   for (auto child : child_requests_) {
     child.request->RunCallbackOnPrematureFailure(code);
     sender_->RequestFinished(child.request);
   }
   child_requests_.clear();
 }
 
 }  // namespace drive
 }  // namespace google_apis