[Offline pages] Implementation of RQStore.UpdateRequests with StoreUpdateResult

components/offline_pages/background/request_queue_store_sql.cc

 // Copyright 2016 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 "components/offline_pages/background/request_queue_store_sql.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/sequenced_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/offline_pages/background/save_page_request.h"
+#include "components/offline_pages/offline_store_types_impl.h"
 #include "sql/connection.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
 
 namespace offline_pages {
 
+template class StoreUpdateResult<SavePageRequest>;
+
 namespace {
 
 using UpdateStatus = RequestQueueStore::UpdateStatus;
 
 // This is a macro instead of a const so that
 // it can be used inline in other SQL statements below.
 #define REQUEST_QUEUE_TABLE_NAME "request_queue_v1"
 const bool kUserRequested = true;
 
 bool CreateRequestQueueTable(sql::Connection* db) {
@@ skipping 207 matching lines @@
   statement.BindString(8, request.client_id().name_space);
   statement.BindString(9, request.client_id().id);
 
   if (!statement.Run())
     return ItemActionStatus::STORE_ERROR;
   if (db->GetLastChangeCount() == 0)
     return ItemActionStatus::ALREADY_EXISTS;
   return ItemActionStatus::SUCCESS;
 }
 
-RequestQueueStore::UpdateStatus InsertOrReplace(
-    sql::Connection* db,
-    const SavePageRequest& request) {
-  // In order to use the enums in the Bind* methods, keep the order of fields
-  // the same as in the definition/select query.
-  const char kInsertSql[] =
-      "INSERT OR REPLACE INTO " REQUEST_QUEUE_TABLE_NAME
-      " (request_id, creation_time, activation_time, last_attempt_time, "
-      " started_attempt_count, completed_attempt_count, state, url, "
-      " client_namespace, client_id) "
-      " VALUES "
-      " (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
+ItemActionStatus Update(sql::Connection* db, const SavePageRequest& request) {
+  const char kSql[] =
+      "UPDATE OR IGNORE " REQUEST_QUEUE_TABLE_NAME
+      " SET creation_time = ?, activation_time = ?, last_attempt_time = ?,"

[Pete Williamson, 2016/09/22 00:12:50]

Why are these in a different order than before? (I'm presuming it is because
once you use the parameterized form that order no longer has to match column
order, but I wanted to double check.)

[fgorski, 2016/09/22 15:47:53]

You are correct that it does not matter.
If you look below however you will notice that except for request_id, which was
moved to the WHERE clause at the end, they are in exactly the same order.

+      " started_attempt_count = ?, completed_attempt_count = ?, state = ?,"
+      " url = ?, client_namespace = ?, client_id = ?"
+      " WHERE request_id = ?";
 
-  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kInsertSql));
-  statement.BindInt64(0, request.request_id());
-  statement.BindInt64(1, request.creation_time().ToInternalValue());
-  statement.BindInt64(2, request.activation_time().ToInternalValue());
-  statement.BindInt64(3, request.last_attempt_time().ToInternalValue());
-  statement.BindInt64(4, request.started_attempt_count());
-  statement.BindInt64(5, request.completed_attempt_count());
-  statement.BindInt64(6, static_cast<int64_t>(request.request_state()));
-  statement.BindString(7, request.url().spec());
-  statement.BindString(8, request.client_id().name_space);
-  statement.BindString(9, request.client_id().id);
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt64(0, request.creation_time().ToInternalValue());
+  statement.BindInt64(1, request.activation_time().ToInternalValue());
+  statement.BindInt64(2, request.last_attempt_time().ToInternalValue());
+  statement.BindInt64(3, request.started_attempt_count());
+  statement.BindInt64(4, request.completed_attempt_count());
+  statement.BindInt64(5, static_cast<int64_t>(request.request_state()));
+  statement.BindString(6, request.url().spec());
+  statement.BindString(7, request.client_id().name_space);
+  statement.BindString(8, request.client_id().id);
+  statement.BindInt64(9, request.request_id());
 
-  // TODO(fgorski): Replace the UpdateStatus with boolean in the
-  // RequestQueueStore interface and update this code.
-  return statement.Run() ? RequestQueueStore::UpdateStatus::UPDATED
-                         : RequestQueueStore::UpdateStatus::FAILED;
+  if (!statement.Run())
+    return ItemActionStatus::STORE_ERROR;
+  if (db->GetLastChangeCount() == 0)
+    return ItemActionStatus::NOT_FOUND;
+  return ItemActionStatus::SUCCESS;
 }
 
 bool InitDatabase(sql::Connection* db, const base::FilePath& path) {
   db->set_page_size(4096);
   db->set_cache_size(500);
   db->set_histogram_tag("BackgroundRequestQueue");
   db->set_exclusive_locking();
 
   base::File::Error err;
   if (!base::CreateDirectoryAndGetError(path.DirName(), &err))
@@ skipping 26 matching lines @@
 
 void AddRequestSync(sql::Connection* db,
                     scoped_refptr<base::SingleThreadTaskRunner> runner,
                     const SavePageRequest& request,
                     const RequestQueueStore::AddCallback& callback) {
   // TODO(fgorski): add UMA metrics here.
   ItemActionStatus status = Insert(db, request);
   runner->PostTask(FROM_HERE, base::Bind(callback, status));
 }
 
-void AddOrUpdateRequestSync(sql::Connection* db,
-                            scoped_refptr<base::SingleThreadTaskRunner> runner,
-                            const SavePageRequest& request,
-                            const RequestQueueStore::UpdateCallback& callback) {
+void UpdateRequestsSync(sql::Connection* db,
+                        scoped_refptr<base::SingleThreadTaskRunner> runner,
+                        const std::vector<SavePageRequest>& requests,
+                        const RequestQueueStore::UpdateCallback& callback) {
   // TODO(fgorski): add UMA metrics here.
-  RequestQueueStore::UpdateStatus status = InsertOrReplace(db, request);
-  runner->PostTask(FROM_HERE, base::Bind(callback, status));
+  std::unique_ptr<UpdateRequestsResult> result(
+      new UpdateRequestsResult(StoreState::LOADED));
+
+  sql::Transaction transaction(db);
+  if (!transaction.Begin()) {
+    // post error for all items.
+    return;
+  }
+
+  for (const auto& request : requests) {
+    ItemActionStatus status = Update(db, request);
+    result->item_statuses.push_back(
+        std::make_pair(request.request_id(), status));
+    if (status == ItemActionStatus::SUCCESS)
+      result->updated_items.push_back(request);
+  }
+
+  if (!transaction.Commit()) {
+    // post error for all items.
+    return;
+  }
+
+  runner->PostTask(FROM_HERE, base::Bind(callback, base::Passed(&result)));
 }
 
 void RemoveRequestsSync(sql::Connection* db,
                         scoped_refptr<base::SingleThreadTaskRunner> runner,
                         const std::vector<int64_t>& request_ids,
                         const RequestQueueStore::RemoveCallback& callback) {
   RequestQueue::UpdateMultipleRequestResults results;
   std::vector<std::unique_ptr<SavePageRequest>> requests;
   // TODO(fgorski): add UMA metrics here.
   DeleteRequestsByIds(db, request_ids, results, &requests);
@@ skipping 79 matching lines @@
                                       const AddCallback& callback) {
   if (!CheckDb(base::Bind(callback, ItemActionStatus::STORE_ERROR)))
     return;
 
   background_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&AddRequestSync, db_.get(),
                  base::ThreadTaskRunnerHandle::Get(), request, callback));
 }
 
-void RequestQueueStoreSQL::AddOrUpdateRequest(const SavePageRequest& request,
-                                              const UpdateCallback& callback) {
-  DCHECK(db_.get());
-  if (!CheckDb(base::Bind(callback, UpdateStatus::FAILED)))
+void RequestQueueStoreSQL::UpdateRequests(
+    const std::vector<SavePageRequest>& requests,
+    const UpdateCallback& callback) {
+  if (!db_.get()) {
+    // port store error for all.
     return;
+  }
 
   background_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&AddOrUpdateRequestSync, db_.get(),
-                 base::ThreadTaskRunnerHandle::Get(), request, callback));
+      base::Bind(&UpdateRequestsSync, db_.get(),
+                 base::ThreadTaskRunnerHandle::Get(), requests, callback));
 }
 
 void RequestQueueStoreSQL::RemoveRequests(
     const std::vector<int64_t>& request_ids,
     const RemoveCallback& callback) {
   // Set up a failed set of results in case we fail the DB check.
   RequestQueue::UpdateMultipleRequestResults results;
   for (int64_t request_id : request_ids) {
     results.push_back(std::make_pair(
         request_id, RequestQueue::UpdateRequestResult::STORE_FAILURE));
@@ skipping 60 matching lines @@
 }
 
 void RequestQueueStoreSQL::OnResetDone(const ResetCallback& callback,
                                        bool success) {
   // Complete connection initialization post reset.
   OnOpenConnectionDone(success);
   callback.Run(success);
 }
 
 }  // namespace offline_pages