[EoS] Fix test so that duplicate catalog imports are not required.

Also document GetCatalogTask and update the version checking logic to
be more accurate and clearer.

Bug: 889104
Change-Id: I2467dd767c7a19e5454d4b31412061ea884b2b7e
Reviewed-on: https://chromium-review.googlesource.com/1254741Reviewed-by: Peter Williamson <petewil@chromium.org>
Commit-Queue: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595518}

chrome/browser/android/explore_sites/explore_sites_bridge.cc

@@ -31,8 +31,16 @@ namespace {
 
 void CatalogReady(ScopedJavaGlobalRef<jobject>(j_result_obj),
                   ScopedJavaGlobalRef<jobject>(j_callback_obj),
+                  GetCatalogStatus status,
                   std::unique_ptr<std::vector<ExploreSitesCategory>> result) {
   JNIEnv* env = base::android::AttachCurrentThread();
+  // TODO(dewittj): Pass along the status to Java land.
+  if (status == GetCatalogStatus::kNoCatalog) {
+    // Don't fill in the result, just return empty.
+    base::android::RunObjectCallbackAndroid(j_callback_obj, j_result_obj);
+    return;
+  }
+
   if (!result) {
     DLOG(ERROR) << "Unable to fetch the ExploreSites catalog!";
     base::android::RunObjectCallbackAndroid(j_callback_obj, nullptr);

chrome/browser/android/explore_sites/explore_sites_service_impl_unittest.cc

@@ -42,7 +42,9 @@ class ExploreSitesServiceImplTest : public testing::Test {
 
   void UpdateCatalogDoneCallback(bool success) { success_ = success; }
   void CatalogCallback(
+      GetCatalogStatus status,
       std::unique_ptr<std::vector<ExploreSitesCategory>> categories) {
+    database_status_ = status;
     if (categories != nullptr) {
       database_categories_ = std::move(categories);
     }
@@ -50,6 +52,7 @@ class ExploreSitesServiceImplTest : public testing::Test {
 
   bool success() { return success_; }
 
+  GetCatalogStatus database_status() { return database_status_; }
   std::vector<ExploreSitesCategory>* database_categories() {
     return database_categories_.get();
   }
@@ -71,6 +74,7 @@ class ExploreSitesServiceImplTest : public testing::Test {
 
   std::unique_ptr<explore_sites::ExploreSitesServiceImpl> service_;
   bool success_;
+  GetCatalogStatus database_status_;
   std::unique_ptr<std::vector<ExploreSitesCategory>> database_categories_;
   std::string test_data_;
   network::TestURLLoaderFactory test_url_loader_factory_;
@@ -117,6 +121,7 @@ ExploreSitesServiceImplTest::GetPendingRequest(size_t index) {
 std::string ExploreSitesServiceImplTest::CreateTestDataProto() {
   std::string serialized_protobuf;
   explore_sites::GetCatalogResponse catalog_response;
+  catalog_response.set_version_token("abcd");
   explore_sites::Catalog* catalog = catalog_response.mutable_catalog();
   explore_sites::Category* category = catalog->add_categories();
   explore_sites::Site* site1 = category->add_sites();
@@ -167,13 +172,11 @@ TEST_F(ExploreSitesServiceImplTest, UpdateCatalogFromNetwork) {
   // just start the update process.  For our test, we've already put data into
   // the catalog, but GetCatalog doesn't know that.
   // TODO(petewil): Fix get catalog so it always returns data if it has some.
-  service()->GetCatalog(base::BindOnce(
-      &ExploreSitesServiceImplTest::CatalogCallback, base::Unretained(this)));
-  // Second call is to get the actual catalog sata into the update callback.
   service()->GetCatalog(base::BindOnce(
       &ExploreSitesServiceImplTest::CatalogCallback, base::Unretained(this)));
   PumpLoop();
 
+  EXPECT_EQ(GetCatalogStatus::kSuccess, database_status());
   EXPECT_NE(nullptr, database_categories());
   EXPECT_EQ(1U, database_categories()->size());
 

chrome/browser/android/explore_sites/explore_sites_types.h

@@ -54,8 +54,10 @@ struct ExploreSitesCategory {
   DISALLOW_COPY_AND_ASSIGN(ExploreSitesCategory);
 };
 
-using CatalogCallback = base::OnceCallback<void(
-    std::unique_ptr<std::vector<ExploreSitesCategory>>)>;
+enum class GetCatalogStatus { kFailed, kNoCatalog, kSuccess };
+
+using CatalogCallback = base::OnceCallback<
+    void(GetCatalogStatus, std::unique_ptr<std::vector<ExploreSitesCategory>>)>;
 using BooleanCallback = base::OnceCallback<void(bool)>;
 using EncodedImageBytes = std::vector<uint8_t>;
 using EncodedImageList = std::vector<std::unique_ptr<EncodedImageBytes>>;

chrome/browser/android/explore_sites/get_catalog_task.cc

@@ -31,16 +31,25 @@ const char kDeleteCategorySql[] =
 
 }  // namespace
 
+// |current_version_token| represents the version in "current_catalog" key of
+// the meta table.  We check whether there exists a "downloading_catalog", and
+// if there doesn't, just return the |current_version_token|.  If there is, set
+// the current == the downloading catalog and return the downloading (aka the
+// new current.);
 std::string UpdateCurrentCatalogIfNewer(sql::MetaTable* meta_table,
                                         std::string current_version_token) {
   DCHECK(meta_table);
   std::string downloading_version_token;
+  // See if there is a downloading catalog.
   if (!meta_table->GetValue("downloading_catalog",
                             &downloading_version_token)) {
+    // No downloading catalog means no change required.
     return current_version_token;
   }
 
-  if (!meta_table->SetValue("current_catalog", downloading_version_token))
+  // Update the current version.
+  current_version_token = downloading_version_token;
+  if (!meta_table->SetValue("current_catalog", current_version_token))
     return "";
   meta_table->DeleteKey("downloading_catalog");
 
@@ -49,6 +58,8 @@ std::string UpdateCurrentCatalogIfNewer(sql::MetaTable* meta_table,
 
 void RemoveOutdatedCatalogEntries(sql::Database* db,
                                   std::string version_token) {
+  // Deletes sites and categories with a version that doesn't match
+  // |version_token|.
   sql::Statement delete_sites(
       db->GetCachedStatement(SQL_FROM_HERE, kDeleteSiteSql));
   delete_sites.BindString(0, version_token);
@@ -60,45 +71,60 @@ void RemoveOutdatedCatalogEntries(sql::Database* db,
   delete_categories.Run();
 }
 
-std::unique_ptr<GetCatalogTask::CategoryList> GetCatalogSync(
-    bool update_current,
-    sql::Database* db) {
+std::pair<GetCatalogStatus, std::unique_ptr<GetCatalogTask::CategoryList>>
+GetCatalogSync(bool update_current, sql::Database* db) {
   DCHECK(db);
   sql::MetaTable meta_table;
   if (!ExploreSitesSchema::InitMetaTable(db, &meta_table))
-    return nullptr;
+    return std::make_pair(GetCatalogStatus::kFailed, nullptr);
 
   // If we are downloading a catalog that is the same version as the one
   // currently in use, don't change it.  This is an error, should have been
   // caught before we got here.
-  std::string catalog_timestamp;
-  meta_table.GetValue("current_catalog", &catalog_timestamp);
+  std::string catalog_version_token;
+  if (!meta_table.GetValue("current_catalog", &catalog_version_token) ||
+      catalog_version_token.empty()) {
+    DVLOG(1)
+        << "Didn't find current catalog value. Attempting to use downloading.";
+    // If there is no current catalog, use downloading catalog and mark it as
+    // current.  If there is no downloading catalog, return no catalog.
+    meta_table.GetValue("downloading_catalog", &catalog_version_token);
+    if (catalog_version_token.empty())
+      return std::make_pair(GetCatalogStatus::kNoCatalog, nullptr);
+
+    update_current = true;
+  }
 
   if (update_current) {
+    DVLOG(1) << "Updating current catalog from " << catalog_version_token;
     sql::Transaction transaction(db);
     transaction.Begin();
-    catalog_timestamp =
-        UpdateCurrentCatalogIfNewer(&meta_table, catalog_timestamp);
-    if (catalog_timestamp == "")
-      return nullptr;
-    RemoveOutdatedCatalogEntries(db, catalog_timestamp);
+    catalog_version_token =
+        UpdateCurrentCatalogIfNewer(&meta_table, catalog_version_token);
+    if (catalog_version_token == "")
+      return std::make_pair(GetCatalogStatus::kFailed, nullptr);
+
+    RemoveOutdatedCatalogEntries(db, catalog_version_token);
+
     if (!transaction.Commit())
-      return nullptr;
+      return std::make_pair(GetCatalogStatus::kFailed, nullptr);
   }
 
+  DVLOG(1) << "Done updating. Catalog to use: " << catalog_version_token;
+
   sql::Statement category_statement(
       db->GetCachedStatement(SQL_FROM_HERE, kSelectCategorySql));
-  category_statement.BindString(0, catalog_timestamp);
+  category_statement.BindString(0, catalog_version_token);
 
   auto result = std::make_unique<GetCatalogTask::CategoryList>();
   while (category_statement.Step()) {
     result->emplace_back(category_statement.ColumnInt(0),  // category_id
-                         catalog_timestamp,
+                         catalog_version_token,
                          category_statement.ColumnInt(1),      // type
                          category_statement.ColumnString(2));  // label
   }
   if (!category_statement.Succeeded())
-    return nullptr;
+    return std::make_pair(GetCatalogStatus::kFailed, nullptr);
 
   for (auto& category : *result) {
     sql::Statement site_statement(
@@ -112,10 +138,10 @@ std::unique_ptr<GetCatalogTask::CategoryList> GetCatalogSync(
                                   site_statement.ColumnString(2));  // title
     }
     if (!site_statement.Succeeded())
-      return nullptr;
+      return std::make_pair(GetCatalogStatus::kFailed, nullptr);
   }
 
-  return result;
+  return std::make_pair(GetCatalogStatus::kSuccess, std::move(result));
 }
 
 GetCatalogTask::GetCatalogTask(ExploreSitesStore* store,
@@ -132,14 +158,16 @@ void GetCatalogTask::Run() {
   store_->Execute(base::BindOnce(&GetCatalogSync, update_current_),
                   base::BindOnce(&GetCatalogTask::FinishedExecuting,
                                  weak_ptr_factory_.GetWeakPtr()),
-                  std::unique_ptr<CategoryList>());
+                  std::make_pair(GetCatalogStatus::kFailed,
+                                 std::unique_ptr<CategoryList>()));
 }
 
 void GetCatalogTask::FinishedExecuting(
-    std::unique_ptr<CategoryList> categories) {
+    std::pair<GetCatalogStatus, std::unique_ptr<CategoryList>> result) {
   TaskComplete();
-  DVLOG(1) << "Finished getting the catalog, result: " << categories.get();
-  std::move(callback_).Run(std::move(categories));
+  DVLOG(1) << "Finished getting the catalog, result: "
+           << static_cast<int>(std::get<0>(result));
+  std::move(callback_).Run(std::get<0>(result), std::move(std::get<1>(result)));
 }
 
 }  // namespace explore_sites

chrome/browser/android/explore_sites/get_catalog_task.h

@@ -46,7 +46,8 @@ class GetCatalogTask : public Task {
   // Task implementation:
   void Run() override;
 
-  void FinishedExecuting(std::unique_ptr<CategoryList> categories);
+  void FinishedExecuting(
+      std::pair<GetCatalogStatus, std::unique_ptr<CategoryList>> result);
 
   ExploreSitesStore* store_;  // outlives this class.
 

chrome/browser/android/explore_sites/get_catalog_task_unittest.cc

@@ -21,6 +21,59 @@
 using offline_pages::TaskTestBase;
 
 namespace explore_sites {
+namespace {
+
+void ValidateTestingCatalog(GetCatalogTask::CategoryList* catalog) {
+  EXPECT_FALSE(catalog == nullptr);
+
+  EXPECT_EQ(2U, catalog->size());
+  ExploreSitesCategory* cat = &catalog->at(0);
+  EXPECT_EQ(3, cat->category_id);
+  EXPECT_EQ("5678", cat->version_token);
+  EXPECT_EQ(1, cat->category_type);
+  EXPECT_EQ("label_1", cat->label);
+
+  EXPECT_EQ(1U, cat->sites.size());
+  ExploreSitesSite* site = &cat->sites[0];
+  EXPECT_EQ("https://www.example.com/1", site->url.spec());
+  EXPECT_EQ(3, site->category_id);
+  EXPECT_EQ("example_1", site->title);
+
+  cat = &catalog->at(1);
+  EXPECT_EQ(4, cat->category_id);
+  EXPECT_EQ("5678", cat->version_token);
+  EXPECT_EQ(2, cat->category_type);
+  EXPECT_EQ("label_2", cat->label);
+
+  EXPECT_EQ(1U, cat->sites.size());
+  site = &cat->sites[0];
+  EXPECT_EQ("https://www.example.com/2", site->url.spec());
+  EXPECT_EQ(4, site->category_id);
+  EXPECT_EQ("example_2", site->title);
+}
+
+void ExpectSuccessGetCatalogResult(
+    GetCatalogStatus status,
+    std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
+  EXPECT_EQ(GetCatalogStatus::kSuccess, status);
+  ValidateTestingCatalog(catalog.get());
+}
+
+void ExpectEmptyGetCatalogResult(
+    GetCatalogStatus status,
+    std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
+  EXPECT_EQ(GetCatalogStatus::kNoCatalog, status);
+  EXPECT_TRUE(catalog == nullptr);
+}
+
+void ExpectFailedGetCatalogResult(
+    GetCatalogStatus status,
+    std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
+  EXPECT_EQ(GetCatalogStatus::kFailed, status);
+  EXPECT_TRUE(catalog == nullptr);
+}
+
+}  // namespace
 
 class ExploreSitesGetCatalogTaskTest : public TaskTestBase {
  public:
@@ -87,35 +140,6 @@ VALUES
   }));
 }
 
-void ExploreSitesGetCatalogTaskTest::ValidateTestingCatalog(
-    GetCatalogTask::CategoryList* catalog) {
-  EXPECT_FALSE(catalog == nullptr);
-
-  EXPECT_EQ(2U, catalog->size());
-  ExploreSitesCategory* cat = &catalog->at(0);
-  EXPECT_EQ(3, cat->category_id);
-  EXPECT_EQ("5678", cat->version_token);
-  EXPECT_EQ(1, cat->category_type);
-  EXPECT_EQ("label_1", cat->label);
-
-  EXPECT_EQ(1U, cat->sites.size());
-  ExploreSitesSite* site = &cat->sites[0];
-  EXPECT_EQ("https://www.example.com/1", site->url.spec());
-  EXPECT_EQ(3, site->category_id);
-  EXPECT_EQ("example_1", site->title);
-
-  cat = &catalog->at(1);
-  EXPECT_EQ(4, cat->category_id);
-  EXPECT_EQ("5678", cat->version_token);
-  EXPECT_EQ(2, cat->category_type);
-  EXPECT_EQ("label_2", cat->label);
-
-  EXPECT_EQ(1U, cat->sites.size());
-  site = &cat->sites[0];
-  EXPECT_EQ("https://www.example.com/2", site->url.spec());
-  EXPECT_EQ(4, site->category_id);
-  EXPECT_EQ("example_2", site->title);
-}
 
 void ExploreSitesGetCatalogTaskTest::SetDownloadingAndCurrentVersion(
     std::string downloading_version_token,
@@ -123,8 +147,16 @@ void ExploreSitesGetCatalogTaskTest::SetDownloadingAndCurrentVersion(
   ExecuteSync(base::BindLambdaForTesting([&](sql::Database* db) {
     sql::MetaTable meta_table;
     ExploreSitesSchema::InitMetaTable(db, &meta_table);
-    meta_table.SetValue("downloading_catalog", downloading_version_token);
-    meta_table.SetValue("current_catalog", current_version_token);
+    if (downloading_version_token.empty()) {
+      meta_table.DeleteKey("downloading_catalog");
+    } else {
+      meta_table.SetValue("downloading_catalog", downloading_version_token);
+    }
+    if (current_version_token.empty()) {
+      meta_table.DeleteKey("current_catalog");
+    } else {
+      meta_table.SetValue("current_catalog", current_version_token);
+    }
     return true;
   }));
 }
@@ -170,20 +202,14 @@ int ExploreSitesGetCatalogTaskTest::GetNumberOfSitesInDB() {
 TEST_F(ExploreSitesGetCatalogTaskTest, StoreFailure) {
   store()->SetInitializationStatusForTest(InitializationStatus::FAILURE);
 
-  auto callback = base::BindLambdaForTesting(
-      [](std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
-        EXPECT_TRUE(catalog == nullptr);
-      });
-  GetCatalogTask task(store(), false, callback);
+  GetCatalogTask task(store(), false,
+                      base::BindOnce(&ExpectFailedGetCatalogResult));
   RunTask(&task);
 }
 
 TEST_F(ExploreSitesGetCatalogTaskTest, EmptyTask) {
-  auto callback = base::BindLambdaForTesting(
-      [](std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
-        EXPECT_FALSE(catalog == nullptr);
-      });
-  GetCatalogTask task(store(), false, callback);
+  GetCatalogTask task(store(), false,
+                      base::BindOnce(&ExpectEmptyGetCatalogResult));
   RunTask(&task);
 }
 
@@ -192,11 +218,8 @@ TEST_F(ExploreSitesGetCatalogTaskTest, EmptyTask) {
 // is the "current" catalog, and where it is the "downloading" catalog.
 TEST_F(ExploreSitesGetCatalogTaskTest, SimpleCatalog) {
   PopulateTestingCatalog();
-  auto callback = base::BindLambdaForTesting(
-      [&](std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
-        ValidateTestingCatalog(catalog.get());
-      });
-  GetCatalogTask task(store(), false, callback);
+  GetCatalogTask task(store(), false,
+                      base::BindOnce(&ExpectSuccessGetCatalogResult));
   RunTask(&task);
   // Since |update_current| is false, we should not have changed any rows in the
   // DB.
@@ -209,11 +232,8 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithVersionUpdate) {
   // Update the testing catalog so that the older catalog is current and the
   // downloading catalog is ready to upgrade.
   SetDownloadingAndCurrentVersion("5678", "1234");
-  auto callback = base::BindLambdaForTesting(
-      [&](std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
-        ValidateTestingCatalog(catalog.get());
-      });
-  GetCatalogTask task(store(), true /* update_current */, callback);
+  GetCatalogTask task(store(), true /* update_current */,
+                      base::BindOnce(&ExpectSuccessGetCatalogResult));
   RunTask(&task);
 
   EXPECT_EQ(std::make_pair(std::string("5678"), std::string()),
@@ -228,11 +248,8 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithoutVersionUpdate) {
   // Make "1234" the downloading version, we should not see any changes in the
   // DB if the |update_current| flag is false.
   SetDownloadingAndCurrentVersion("1234", "5678");
-  auto callback = base::BindLambdaForTesting(
-      [&](std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
-        ValidateTestingCatalog(catalog.get());
-      });
-  GetCatalogTask task(store(), false /* update_current */, callback);
+  GetCatalogTask task(store(), false /* update_current */,
+                      base::BindOnce(&ExpectSuccessGetCatalogResult));
   RunTask(&task);
 
   EXPECT_EQ(std::make_pair(std::string("5678"), std::string("1234")),
@@ -244,12 +261,26 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithoutVersionUpdate) {
 TEST_F(ExploreSitesGetCatalogTaskTest, InvalidCatalogVersions) {
   PopulateTestingCatalog();
   SetDownloadingAndCurrentVersion("", "");
+  GetCatalogTask task(store(), false /* update_current */,
+                      base::BindOnce(&ExpectEmptyGetCatalogResult));
+  RunTask(&task);
+}
+
+TEST_F(ExploreSitesGetCatalogTaskTest,
+       GetCatalogWhenOnlyDownloadingCatalogExists) {
+  PopulateTestingCatalog();
+  SetDownloadingAndCurrentVersion("1234", "");
+  ExecuteSync(base::BindLambdaForTesting([&](sql::Database* db) {
+    sql::Statement cat_count(db->GetUniqueStatement(
+        "DELETE FROM categories where version_token <> \"1234\";"));
+    return cat_count.Run();
+  }));
   auto callback = base::BindLambdaForTesting(
-      [&](std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
-        EXPECT_EQ(0U, catalog->size());
+      [&](GetCatalogStatus status,
+          std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
+        EXPECT_NE(0U, catalog->size());
       });
   GetCatalogTask task(store(), false /* update_current */, callback);
-  RunTask(&task);
 }
 
 }  // namespace explore_sites

chrome/browser/android/explore_sites/import_catalog_task.cc

@@ -36,7 +36,7 @@ VALUES
 bool ImportCatalogSync(std::string version_token,
                        std::unique_ptr<Catalog> catalog_proto,
                        sql::Database* db) {
-  if (!db || !catalog_proto)
+  if (!db || !catalog_proto || version_token.empty())
     return false;
 
   sql::Transaction transaction(db);

chrome/browser/android/explore_sites/import_catalog_task_unittest.cc

@@ -78,7 +78,7 @@ TEST_F(ExploreSitesImportCatalogTaskTest, StoreFailure) {
                      base::Unretained(this)));
   RunTask(&task);
 
-  // A null catalog should be completed but return with an error.
+  // A database failure should be completed but return with an error.
   EXPECT_TRUE(task.complete());
   EXPECT_FALSE(task.result());
 }
@@ -95,6 +95,19 @@ TEST_F(ExploreSitesImportCatalogTaskTest, EmptyTask) {
   EXPECT_FALSE(task.result());
 }
 
+TEST_F(ExploreSitesImportCatalogTaskTest, EmptyVersion) {
+  std::string empty_version_token;
+  ImportCatalogTask task(
+      store(), empty_version_token, std::make_unique<Catalog>(),
+      base::BindOnce(&ExploreSitesImportCatalogTaskTest::OnImportTaskDone,
+                     base::Unretained(this)));
+  RunTask(&task);
+
+  // A catalog with no version should be completed but return with an error.
+  EXPECT_TRUE(task.complete());
+  EXPECT_FALSE(task.result());
+}
+
 // This tests the behavior of the catalog task when there is already a catalog
 // with the current version_token in the database. This tests both the case
 // where it is the "current" catalog, and where it is the "downloading" catalog.