Drive: Add response handling to BatchUploadRequst class.

google_apis/drive/drive_api_requests.cc

Index: google_apis/drive/drive_api_requests.cc
diff --git a/google_apis/drive/drive_api_requests.cc b/google_apis/drive/drive_api_requests.cc
index 8c16a03b1895843c435045a7a0f041977bc89a22..7b3cee4313e5787e8c16be8475fdac77798ff75a 100644
--- a/google_apis/drive/drive_api_requests.cc
+++ b/google_apis/drive/drive_api_requests.cc
@@ -36,6 +36,9 @@ 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
@@ -126,6 +129,78 @@ std::string CreateMultipartUploadMetadataJson(
   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) {
@@ -1014,14 +1089,12 @@ void BatchUploadRequest::AddRequest(BatchableRequestBase* 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());
@@ -1061,8 +1134,8 @@ void BatchUploadRequest::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;
@@ -1104,11 +1177,8 @@ void BatchUploadRequest::MayCompletePrepare() {
     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());
   }
 
@@ -1149,11 +1219,25 @@ void BatchUploadRequest::ProcessURLFetchResults(const net::URLFetcher* source) {
     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();