Fetch incremental updates. Store new state in V4Store.

components/safe_browsing_db/v4_database_unittest.cc

Index: components/safe_browsing_db/v4_database_unittest.cc
diff --git a/components/safe_browsing_db/v4_database_unittest.cc b/components/safe_browsing_db/v4_database_unittest.cc
index 677dfcc27e7dd52e8edd432cb4ef2d6532ac4dca..882ab755ebfb9288e6ad51f9bb4c7255a4389dea 100644
--- a/components/safe_browsing_db/v4_database_unittest.cc
+++ b/components/safe_browsing_db/v4_database_unittest.cc
@@ -65,28 +65,31 @@ class SafeBrowsingV4DatabaseTest : public PlatformTest {
 
     created_but_not_called_back_ = false;
     created_and_called_back_ = false;
+
+    callback_db_updated_ = base::Bind(
+        &SafeBrowsingV4DatabaseTest::DatabaseUpdated, base::Unretained(this));
   }
 
-  void SetupInfoMapAndExpectedState() {
-    UpdateListIdentifier update_list_identifier;
+  void RegisterFactory(bool fails_first_reset) {
+    factory_.reset(new FakeV4StoreFactory(fails_first_reset));
+    V4Database::RegisterStoreFactoryForTest(factory_.get());
+  }
 
-    update_list_identifier.platform_type = WINDOWS_PLATFORM;
-    update_list_identifier.threat_entry_type = URL;
-    update_list_identifier.threat_type = MALWARE_THREAT;
-    store_file_name_map_[update_list_identifier] = "win_url_malware";
-    expected_identifiers_.push_back(update_list_identifier);
+  void SetupInfoMapAndExpectedState() {
+    UpdateListIdentifier win_malware_id(WINDOWS_PLATFORM, URL, MALWARE_THREAT);
+    store_file_name_map_[win_malware_id] = "win_url_malware";
+    expected_identifiers_.push_back(win_malware_id);

[Scott Hess - ex-Googler, 2016/06/21 22:23:25]

I guess that worked out well!

[vakh (use Gerrit instead), 2016/06/23 06:23:50]

Acknowledged.

     expected_store_paths_.push_back(
         database_dirname_.AppendASCII("win_url_malware.fake"));
 
-    update_list_identifier.platform_type = LINUX_PLATFORM;
-    update_list_identifier.threat_entry_type = URL;
-    update_list_identifier.threat_type = MALWARE_THREAT;
-    store_file_name_map_[update_list_identifier] = "linux_url_malware";
-    expected_identifiers_.push_back(update_list_identifier);
+    UpdateListIdentifier linux_malware_id(LINUX_PLATFORM, URL, MALWARE_THREAT);
+    store_file_name_map_[linux_malware_id] = "linux_url_malware";
+    expected_identifiers_.push_back(linux_malware_id);
     expected_store_paths_.push_back(
         database_dirname_.AppendASCII("linux_url_malware.fake"));
   }
 
+  void DatabaseUpdated() {}
   void NewDatabaseReadyWithExpectedStorePathsAndIds(
       std::vector<base::FilePath> expected_store_paths,
       std::vector<UpdateListIdentifier> expected_identifiers,
@@ -112,6 +115,57 @@ class SafeBrowsingV4DatabaseTest : public PlatformTest {
 
     EXPECT_FALSE(created_and_called_back_);
     created_and_called_back_ = true;
+
+    v4_database_ = std::move(v4_database);
+  }
+
+  void PopulateFakeUpdateResponse(StoreStateMap store_state_map,
+                                  std::vector<ListUpdateResponse>* responses) {
+    for (const auto& store_state_iter : store_state_map) {
+      UpdateListIdentifier identifier = store_state_iter.first;
+      ListUpdateResponse list_update_response;
+      list_update_response.set_platform_type(identifier.platform_type);
+      list_update_response.set_threat_entry_type(identifier.threat_entry_type);
+      list_update_response.set_threat_type(identifier.threat_type);
+      list_update_response.set_new_client_state(store_state_iter.second);
+      responses->push_back(list_update_response);
+    }
+  }
+
+  void SetupNewDatabaseReadyCallback(bool expected_resets_successfully) {
+    callback_db_ready_ =
+        base::Bind(&SafeBrowsingV4DatabaseTest::
+                       NewDatabaseReadyWithExpectedStorePathsAndIds,
+                   base::Unretained(this), expected_store_paths_,
+                   expected_identifiers_, expected_resets_successfully);
+  }

[Scott Hess - ex-Googler, 2016/06/21 22:23:25]

This feels odd to me.  It's setting a member variable to a combination of other
member variables and a parameter, and it's only ever called once per test
fixture.

At this point, it feels like SetupInfoMapAndExpectedState() just belongs up in
SetUp().  The callback happens on the logical test thread, so it can can just
access the expected_*_ variables directly, rather than having them as
parameters.  expected_resets_successfully could then also be a member variable
for the test fixture to set, and at that point this callback can also just be
moved into SetUp() also.

If that's entirely wrong, then instead of having this set the callback as a side
effect, it might be clearer if this returned the callback which the calling code
sets.

[vakh (use Gerrit instead), 2016/06/23 06:23:50]

Makes good sense. Done.

+
+  void VerifyExpectedStoresState(bool expect_new_stores) {
+    const StoreMap* new_store_map = v4_database_->store_map_.get();
+    std::unique_ptr<StoreStateMap> new_store_state_map =
+        v4_database_->GetStoreStateMap();
+    EXPECT_EQ(expected_store_state_map_.size(), new_store_map->size());
+    EXPECT_EQ(expected_store_state_map_.size(), new_store_state_map->size());
+    for (const auto& expected_iter : expected_store_state_map_) {
+      const UpdateListIdentifier& identifier = expected_iter.first;
+      const std::string& state = expected_iter.second;
+      EXPECT_EQ(1ul, new_store_map->count(identifier));
+      EXPECT_EQ(1ul, new_store_state_map->count(identifier));

[Scott Hess - ex-Googler, 2016/06/21 22:23:25]

The new_store_map case needs to be an ASSERT, because below you deref it.  The
new_store_state_map can remain EXPECT, but I think being consistent between the
two is probably better.

1u should be sufficient, and places less constraints on size_t.  [I hate that
there is no contains().  Yeah, checking count() or find()!=end() works, but it's
stupidly indirect.]

[vakh (use Gerrit instead), 2016/06/23 06:23:50]

Done
Wholeheartedly agree :-)

+
+      // Verify the expected state in the store map and the state map.
+      EXPECT_EQ(state, new_store_map->at(identifier)->state());
+      EXPECT_EQ(state, new_store_state_map->at(identifier));
+
+      if (expect_new_stores) {
+        // Verify that a new store was created.
+        EXPECT_NE(old_stores_map_[identifier],
+                  new_store_map->at(identifier).get());

[Scott Hess - ex-Googler, 2016/06/21 22:23:25]

I'd say either consistently use [] or at(), just to reduce the ??? in the
reader's head.  I'm neutral on which to use, (*new_store_map_)[].get() is
atrocious, too.  But *shrug*.

[vakh (use Gerrit instead), 2016/06/23 06:23:50]

I prefer to use [], but:
1. I really dislike: (*new_store_map_)[]
2. In order for the [] operator to be available, I need to change the
new_store_map from 'const StoreMap*' to 'StoreMap*', which IMO better to have
than the minor readability gains from using []

So, I've changed all occurences to use at()

+      } else {
+        // Verify that NO new store was created.
+        EXPECT_EQ(old_stores_map_[identifier],
+                  new_store_map->at(identifier).get());
+      }
+    }
   }
 
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
@@ -124,16 +178,21 @@ class SafeBrowsingV4DatabaseTest : public PlatformTest {
   StoreFileNameMap store_file_name_map_;
   std::vector<UpdateListIdentifier> expected_identifiers_;
   std::vector<base::FilePath> expected_store_paths_;
+  std::unique_ptr<FakeV4StoreFactory> factory_;
+  DatabaseUpdatedCallback callback_db_updated_;
+  NewDatabaseReadyCallback callback_db_ready_;
+  StoreStateMap expected_store_state_map_;
+  base::hash_map<UpdateListIdentifier, V4Store*> old_stores_map_;
 };
 
-// Test to set up the database with no stores.
-TEST_F(SafeBrowsingV4DatabaseTest, TestSetupDatabaseWithNoStores) {
-  NewDatabaseReadyCallback callback_db_ready = base::Bind(
-      &SafeBrowsingV4DatabaseTest::NewDatabaseReadyWithExpectedStorePathsAndIds,
-      base::Unretained(this), expected_store_paths_, expected_identifiers_,
-      true);
+// Test to set up the database with fake stores.
+TEST_F(SafeBrowsingV4DatabaseTest, TestSetupDatabaseWithFakeStores) {
+  RegisterFactory(false);
+  SetupInfoMapAndExpectedState();
+  SetupNewDatabaseReadyCallback(true);

[Scott Hess - ex-Googler, 2016/06/21 22:23:25]

My earlier suggestion would push the two Setup*() calls into SetUp() ... the
Register could also go there if you had SetUp() setup a factory with a pointer
to a boolean which the test fixture could set.  I could see that going either
way, though.

[vakh (use Gerrit instead), 2016/06/23 06:23:50]

I've moved the two Setup* methods to SetUp as you mentioned but the Register
suggestions seems a little bit of an overkill so not doing that for now. If you
feel strongly about it (I don't), I'll change it.

+
   V4Database::Create(task_runner_, database_dirname_, store_file_name_map_,
-                     callback_db_ready);
+                     callback_db_updated_, callback_db_ready_);
   created_but_not_called_back_ = true;
   task_runner_->RunPendingTasks();
 
@@ -141,20 +200,14 @@ TEST_F(SafeBrowsingV4DatabaseTest, TestSetupDatabaseWithNoStores) {
   EXPECT_EQ(true, created_and_called_back_);
 }
 
-// Test to set up the database with fake stores.
-TEST_F(SafeBrowsingV4DatabaseTest, TestSetupDatabaseWithFakeStores) {
+// Test to set up the database with fake stores that fail to reset.
+TEST_F(SafeBrowsingV4DatabaseTest, TestSetupDatabaseWithFakeStoresFailsReset) {
+  RegisterFactory(true);
   SetupInfoMapAndExpectedState();
+  SetupNewDatabaseReadyCallback(false);
 
-  NewDatabaseReadyCallback callback_db_ready = base::Bind(
-      &SafeBrowsingV4DatabaseTest::NewDatabaseReadyWithExpectedStorePathsAndIds,
-      base::Unretained(this), expected_store_paths_, expected_identifiers_,
-      true);
-
-  FakeV4StoreFactory* factory = new FakeV4StoreFactory(false);
-  ANNOTATE_LEAKING_OBJECT_PTR(factory);
-  V4Database::RegisterStoreFactoryForTest(factory);
   V4Database::Create(task_runner_, database_dirname_, store_file_name_map_,
-                     callback_db_ready);
+                     callback_db_updated_, callback_db_ready_);
   created_but_not_called_back_ = true;
   task_runner_->RunPendingTasks();
 
@@ -162,25 +215,99 @@ TEST_F(SafeBrowsingV4DatabaseTest, TestSetupDatabaseWithFakeStores) {
   EXPECT_EQ(true, created_and_called_back_);
 }
 
-// Test to set up the database with fake stores that fail to reset.
-TEST_F(SafeBrowsingV4DatabaseTest, TestSetupDatabaseWithFakeStoresFailsReset) {
+// Test to check database updates as expected.
+TEST_F(SafeBrowsingV4DatabaseTest, TestApplyUpdateWithNewStates) {
+  RegisterFactory(false);
+  SetupInfoMapAndExpectedState();
+  SetupNewDatabaseReadyCallback(true);
+
+  V4Database::Create(task_runner_, database_dirname_, store_file_name_map_,
+                     callback_db_updated_, callback_db_ready_);
+  created_but_not_called_back_ = true;
+  task_runner_->RunPendingTasks();
+  base::RunLoop().RunUntilIdle();
+
+  // The database has now been created. Time to try to update it.
+  EXPECT_TRUE(v4_database_);
+  const StoreMap* db_stores = v4_database_->store_map_.get();
+  EXPECT_EQ(expected_store_paths_.size(), db_stores->size());
+  for (const auto& store_iter : *db_stores) {
+    V4Store* store = store_iter.second.get();
+    expected_store_state_map_[store_iter.first] = store->state() + "_fake";
+    old_stores_map_[store_iter.first] = store;
+  }
+
+  std::vector<ListUpdateResponse> update_response;
+  PopulateFakeUpdateResponse(expected_store_state_map_, &update_response);
+  v4_database_->ApplyUpdate(update_response);
+
+  task_runner_->RunPendingTasks();
+  base::RunLoop().RunUntilIdle();
+
+  VerifyExpectedStoresState(true);
+}
+
+// Test to ensure no state updates leads to no store updates.
+TEST_F(SafeBrowsingV4DatabaseTest, TestApplyUpdateWithNoNewState) {
+  RegisterFactory(false);
   SetupInfoMapAndExpectedState();
+  SetupNewDatabaseReadyCallback(true);
+
+  V4Database::Create(task_runner_, database_dirname_, store_file_name_map_,
+                     callback_db_updated_, callback_db_ready_);
+  created_but_not_called_back_ = true;
+  task_runner_->RunPendingTasks();
+  base::RunLoop().RunUntilIdle();
+
+  // The database has now been created. Time to try to update it.
+  EXPECT_TRUE(v4_database_);
+  const StoreMap* db_stores = v4_database_->store_map_.get();
+  EXPECT_EQ(expected_store_paths_.size(), db_stores->size());
+  for (const auto& store_iter : *db_stores) {
+    V4Store* store = store_iter.second.get();
+    expected_store_state_map_[store_iter.first] = store->state();
+    old_stores_map_[store_iter.first] = store;
+  }
+
+  std::vector<ListUpdateResponse> update_response;
+  PopulateFakeUpdateResponse(expected_store_state_map_, &update_response);
+  v4_database_->ApplyUpdate(update_response);
+
+  task_runner_->RunPendingTasks();
+  base::RunLoop().RunUntilIdle();
+
+  VerifyExpectedStoresState(false);
+}
 
-  NewDatabaseReadyCallback callback_db_ready = base::Bind(
-      &SafeBrowsingV4DatabaseTest::NewDatabaseReadyWithExpectedStorePathsAndIds,
-      base::Unretained(this), expected_store_paths_, expected_identifiers_,
-      false);
+// Test to ensure no updates leads to no store updates.
+TEST_F(SafeBrowsingV4DatabaseTest, TestApplyUpdateWithEmptyUpdate) {
+  RegisterFactory(false);
+  SetupInfoMapAndExpectedState();
+  SetupNewDatabaseReadyCallback(true);
 
-  FakeV4StoreFactory* factory = new FakeV4StoreFactory(true);
-  ANNOTATE_LEAKING_OBJECT_PTR(factory);
-  V4Database::RegisterStoreFactoryForTest(factory);
   V4Database::Create(task_runner_, database_dirname_, store_file_name_map_,
-                     callback_db_ready);
+                     callback_db_updated_, callback_db_ready_);
   created_but_not_called_back_ = true;
   task_runner_->RunPendingTasks();
+  base::RunLoop().RunUntilIdle();
+
+  // The database has now been created. Time to try to update it.
+  EXPECT_TRUE(v4_database_);
+  const StoreMap* db_stores = v4_database_->store_map_.get();
+  EXPECT_EQ(expected_store_paths_.size(), db_stores->size());
+  for (const auto& store_iter : *db_stores) {
+    V4Store* store = store_iter.second.get();
+    expected_store_state_map_[store_iter.first] = store->state();
+    old_stores_map_[store_iter.first] = store;
+  }
+
+  std::vector<ListUpdateResponse> update_response;
+  v4_database_->ApplyUpdate(update_response);
 
+  task_runner_->RunPendingTasks();
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(true, created_and_called_back_);
+
+  VerifyExpectedStoresState(false);
 }
 
 }  // namespace safe_browsing