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..d9ec86faa2be9e4a472a441161a3fea64b26577e 100644
--- a/google_apis/drive/drive_api_requests.cc
+++ b/google_apis/drive/drive_api_requests.cc
@@ -16,6 +16,7 @@
 #include "google_apis/drive/request_util.h"
 #include "google_apis/drive/time_util.h"
 #include "net/base/url_util.h"
+#include "net/http/http_response_headers.h"
 
 namespace google_apis {
 namespace drive {
@@ -36,6 +37,12 @@ const char kBatchUploadHeader[] = "X-Goog-Upload-Protocol: batch";
 // Content type of HTTP request.
 const char kHttpContentType[] = "application/http";
 
+// Break line in HTTP message.
+const char kHttpBr[] = "\r\n";
+
+// Mime type of multipart mixed.
+const char kMultipartMixedMimeTypePrefix[] = "multipart/mixed; boundary=";
+
 // 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,8 +133,142 @@ std::string CreateMultipartUploadMetadataJson(
   return json_string;
 }
 
+// Checks if the string between |begin| and |end| equals to |expected|. The
+// string between |begin| and |end| can include kHttpBr. |expected| must not
+// include kHttpBr.
+bool IsLineEqual(std::string::const_iterator begin,
+                 std::string::const_iterator end,
+                 const std::string& expected) {
+  size_t size = end - begin;
+  const std::string br(kHttpBr);
+  if (size >= br.size() && std::equal(br.begin(), br.end(), end - br.size())) {
+    size -= br.size();
+  }
+
+  if (size != expected.size())
+    return false;
+
+  return std::equal(expected.begin(), expected.end(), begin);
+}
+
+// Obtains substring of string between |begin| and |end| that follows |prefix|.
+// Returns false if |prefix| does not match.
+bool GetAfterPrefix(std::string::const_iterator begin,
+                    std::string::const_iterator end,
+                    const std::string& prefix,
+                    std::string* output) {
+  std::string::const_iterator output_begin = begin + prefix.size();

[kinaba, 2015/04/27 08:09:37]

strictly speaking this addition is not legal if output_begin > end. Maybe:

if (end - begin > prefix.size())
  return false;
if (!std::equal(prefix.begin(), prefix.end(), begin))
  return false;
output->assign(begin + prefix.size(), end);
return true;


Or see my comment below. I guess uses of this function can be replaced with
StringPiece and string_util.h.

[hirono, 2015/04/27 12:32:45]

I turned to use StringPiece and just removed this function.

+  if (output_begin > end)
+    return false;
+  if (std::equal(prefix.begin(), prefix.end(), begin)) {
+    *output = std::string(output_begin, end);
+    return true;
+  } else {
+    return false;
+  }
+}
+
 }  // namespace
 
+MultipartHttpResponse::MultipartHttpResponse() : code(HTTP_SUCCESS) {
+}
+
+MultipartHttpResponse::~MultipartHttpResponse() {
+}
+
+bool ParseMultipartResponse(const std::string& boundary,
+                            const std::string& response,
+                            std::vector<MultipartHttpResponse>* parts) {
+  if (boundary.empty() || response.empty())
+    return false;
+  const size_t br_size = std::string(kHttpBr).size();
+  std::vector<std::string::const_iterator> lines;
+  std::string::const_iterator it = response.begin();
+  while (true) {
+    lines.push_back(it);
+    it = std::search(it, response.end(), kHttpBr, kHttpBr + br_size);
+    if (it == response.end()) {
+      lines.push_back(it);
+      break;
+    }
+    it += br_size;
+  }

[kinaba, 2015/04/27 08:09:37]

nit: this loop can be factored out as a function, something like 
SplitByHttpBr().

[hirono, 2015/04/27 12:32:45]

Done.

+
+  const std::string header = "--" + boundary;
+  const std::string terminator = "--" + boundary + "--";
+
+  enum {
+    STATE_START,
+    STATE_PART_HEADER,
+    STATE_PART_HTTP_STATUS_LINE,
+    STATE_PART_HTTP_HEADER,
+    STATE_PART_HTTP_BODY
+  } state = STATE_START;
+
+  const std::string kHttpStatusPrefix = "HTTP/1.1 ";
+  std::vector<MultipartHttpResponse> responses;
+  DriveApiErrorCode code;
+  std::string::const_iterator body_begin;
+  for (size_t i = 0; i < lines.size() - 1; ++i) {

[kinaba, 2015/04/27 08:09:37]

You can utilize base/strings/string_piece.h.

Let current_line = StringPiece(lines[i], line[i + 1]) with kHttpBr removed, then
IsLineEqual can be a simple comparison and GetAfterPrefix can be starts_with
followed by substr().

[hirono, 2015/04/27 12:32:45]

Done.

+    if (state == STATE_PART_HEADER && IsLineEqual(lines[i], lines[i + 1], "")) {
+      state = STATE_PART_HTTP_STATUS_LINE;
+      continue;
+    }
+
+    if (state == STATE_PART_HTTP_STATUS_LINE) {
+      std::string code_string;
+      if (GetAfterPrefix(lines[i], lines[i + 1], kHttpStatusPrefix,
+                         &code_string)) {
+        code = static_cast<DriveApiErrorCode>(atoi(code_string.c_str()));
+        if (code <= 0)
+          code = DRIVE_PARSE_ERROR;
+      } else {
+        code = DRIVE_PARSE_ERROR;
+      }
+      state = STATE_PART_HTTP_HEADER;
+      continue;
+    }
+
+    if (state == STATE_PART_HTTP_HEADER &&
+        IsLineEqual(lines[i], lines[i + 1], "")) {
+      state = STATE_PART_HTTP_BODY;
+      body_begin = lines[i + 1];
+      continue;
+    }
+
+    const bool is_new_part = IsLineEqual(lines[i], lines[i + 1], header);

[kinaba, 2015/04/27 08:09:37]

I'm not sure how much we need to be pedantic, but according to RFC, you need to
accept a line with header followed by spaces. ("transport-padding" rule in RFC)
Probably we can ignore such cases.

[hirono, 2015/04/27 12:32:45]

Let me handle this. Done

+    const bool was_last_part = IsLineEqual(lines[i], lines[i + 1], terminator);
+    if (is_new_part || was_last_part) {
+      switch (state) {
+        case STATE_START:
+          break;
+        case STATE_PART_HEADER:
+        case STATE_PART_HTTP_STATUS_LINE:
+          responses.push_back(MultipartHttpResponse());
+          responses.back().code = DRIVE_PARSE_ERROR;
+          break;
+        case STATE_PART_HTTP_HEADER:
+          responses.push_back(MultipartHttpResponse());
+          responses.back().code = code;
+          break;
+        case STATE_PART_HTTP_BODY:
+          responses.push_back(MultipartHttpResponse());
+          responses.back().code = code;
+          responses.back().body = std::string(body_begin, lines[i]);

[kinaba, 2015/04/27 08:09:37]

then body will include the last CRLF (i.e., two chars before lines[i]). Is it
ok? (I'm guessing not.)

[hirono, 2015/04/27 12:32:45]

Yes, it should not include CRLF. Thank you for catching it.

+          break;
+      }
+    }
+
+    if (is_new_part)
+      state = STATE_PART_HEADER;
+    if (was_last_part)
+      break;

[kinaba, 2015/04/27 08:09:37]

nit: I slightly prefer these 4 lines to be put inside if(...||...) block, which
makes clearer that something is done here only when (is_new_part ||
was_new_part) but is's up to your preference. 

[hirono, 2015/04/27 12:32:45]

Done.

+  }
+
+  parts->swap(responses);
+  return true;
+}
+
 Property::Property() : visibility_(VISIBILITY_PRIVATE) {
 }
 
@@ -1014,14 +1155,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 +1200,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 +1243,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,14 +1285,32 @@ 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::string content_type;
+  source->GetResponseHeaders()->EnumerateHeader(
+      /* need only first header */ NULL, "Content-Type", &content_type);
+  std::string boundary;
+  if (!GetAfterPrefix(content_type.begin(), content_type.end(),
+                      kMultipartMixedMimeTypePrefix, &boundary)) {

[kinaba, 2015/04/27 08:09:37]

According to the RFC boundary field can be quoted (search "WARNING TO
IMPLEMENTORS")

[hirono, 2015/04/27 12:32:45]

Done.

+    RunCallbackOnPrematureFailure(DRIVE_PARSE_ERROR);
+    sender_->RequestFinished(this);
+    return;
+  }
+
+  std::vector<MultipartHttpResponse> parts;
+  if (!ParseMultipartResponse(boundary, 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) {
+    child_requests_[i].request->ProcessURLFetchResults(parts[i].code,
+                                                       parts[i].body);
   }
 
   child_requests_.clear();
+  sender_->RequestFinished(this);
 }
 
 void BatchUploadRequest::RunCallbackOnPrematureFailure(DriveApiErrorCode code) {