Replace FTS in the history_service with a brute force text search.

chrome/browser/history/history_querying_unittest.cc

 // Copyright (c) 2012 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 "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/path_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/history/history_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::Time;
 using base::TimeDelta;
 
 // Tests the history service for querying functionality.
 
 namespace history {
 
 namespace {
 
 struct TestEntry {
   const char* url;
   const char* title;
   const int days_ago;
-  const char* body;
   Time time;  // Filled by SetUp.
 } test_entries[] = {
   // This one is visited super long ago so it will be in a different database
   // from the next appearance of it at the end.
-  {"http://example.com/", "Other", 180, "Other"},
+  {"http://example.com/", "Other", 180},
 
   // These are deliberately added out of chronological order. The history
   // service should sort them by visit time when returning query results.
   // The correct index sort order is 4 2 3 1 7 6 5 0.
-  {"http://www.google.com/1", "Title 1", 10,
-   "PAGEONE FOO some body text"},
-  {"http://www.google.com/3", "Title 3", 8,
-   "PAGETHREE BAR some hello world for you"},
-  {"http://www.google.com/2", "Title 2", 9,
-   "PAGETWO FOO some more blah blah blah Title"},
+  {"http://www.google.com/1", "Title PAGEONE FOO some text", 10},
+  {"http://www.google.com/3", "Title PAGETHREE BAR some hello world", 8},
+  {"http://www.google.com/2", "Title PAGETWO FOO some more blah blah blah", 9},
 
   // A more recent visit of the first one.
-  {"http://example.com/", "Other", 6, "Other"},
+  {"http://example.com/", "Other", 6},
 
-  {"http://www.google.com/6", "Title 6", 13, "I'm the second oldest"},
-  {"http://www.google.com/4", "Title 4", 12, "four"},
-  {"http://www.google.com/5", "Title 5", 11, "five"},
+  {"http://www.google.com/6", "Title I'm the second oldest", 13},
+  {"http://www.google.com/4", "Title four", 12},
+  {"http://www.google.com/5", "Title five", 11},
 };
 
 // Returns true if the nth result in the given results set matches. It will
 // return false on a non-match or if there aren't enough results.
 bool NthResultIs(const QueryResults& results,
                  int n,  // Result index to check.
                  int test_entry_index) {  // Index of test_entries to compare.
   if (static_cast<int>(results.size()) <= n)
     return false;
 
@@ skipping 22 matching lines @@
     history_->QueryHistory(
         UTF8ToUTF16(text_query), options, &consumer_,
         base::Bind(&HistoryQueryTest::QueryHistoryComplete,
                    base::Unretained(this)));
     // Will go until ...Complete calls Quit.
     base::MessageLoop::current()->Run();
     results->Swap(&last_query_results_);
   }
 
   // Test paging through results, with a fixed number of results per page.
-  // Defined here so code can be shared for the FTS version and the non-FTS
-  // version.
+  // Defined here so code can be shared for the text search and the non-text
+  // seach versions.
   void TestPaging(const std::string& query_text,
                   const int* expected_results,
                   int results_length) {
     ASSERT_TRUE(history_.get());
 
     QueryOptions options;
     QueryResults results;
 
     options.max_count = 1;
     for (int i = 0; i < results_length; i++) {
@@ skipping 12 matching lines @@
     for (int i = 0; i < results_length / 2; i++) {
       SCOPED_TRACE(testing::Message() << "i = " << i);
       QueryHistory(query_text, options, &results);
       ASSERT_EQ(2U, results.size());
       EXPECT_TRUE(NthResultIs(results, 0, expected_results[i * 2]));
       EXPECT_TRUE(NthResultIs(results, 1, expected_results[i * 2 + 1]));
       options.end_time = results.back().visit_time();
     }
 
     // Add a couple of entries with duplicate timestamps. Use |query_text| as
-    // the body of both entries so that they match a full-text query.
+    // the title of both entries so that they match a text query.
     TestEntry duplicates[] = {
-      { "http://www.google.com/x", "", 1, query_text.c_str() },
-      { "http://www.google.com/y", "", 1, query_text.c_str() }
+      { "http://www.google.com/x",  query_text.c_str(), 1, },
+      { "http://www.google.com/y",  query_text.c_str(), 1, }
     };
     AddEntryToHistory(duplicates[0]);
     AddEntryToHistory(duplicates[1]);
 
     // Make sure that paging proceeds even if there are duplicate timestamps.
     options.end_time = base::Time();
     do {
       QueryHistory(query_text, options, &results);
       ASSERT_NE(options.end_time, results.back().visit_time());
       options.end_time = results.back().visit_time();
     } while (!results.reached_beginning());
   }
 
  protected:
   scoped_ptr<HistoryService> history_;
 
   // Counter used to generate a unique ID for each page added to the history.
   int32 page_id_;
 
   void AddEntryToHistory(const TestEntry& entry) {
     // We need the ID scope and page ID so that the visit tracker can find it.
     const void* id_scope = reinterpret_cast<void*>(1);
     GURL url(entry.url);
 
     history_->AddPage(url, entry.time, id_scope, page_id_++, GURL(),
                       history::RedirectList(), content::PAGE_TRANSITION_LINK,
                       history::SOURCE_BROWSED, false);
     history_->SetPageTitle(url, UTF8ToUTF16(entry.title));
-    history_->SetPageContents(url, UTF8ToUTF16(entry.body));
   }
 
  private:
   virtual void SetUp() {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     history_dir_ = temp_dir_.path().AppendASCII("HistoryTest");
     ASSERT_TRUE(file_util::CreateDirectory(history_dir_));
 
     history_.reset(new HistoryService);
     if (!history_->Init(history_dir_, NULL)) {
@@ skipping 135 matching lines @@
   EXPECT_TRUE(results.reached_beginning());
   options.max_count = results.size();
   QueryHistory(std::string(), options, &results);
   EXPECT_TRUE(results.reached_beginning());
 
   options.max_count = 100;
   QueryHistory("some", options, &results);
   EXPECT_TRUE(results.reached_beginning());
   options.max_count = results.size();
   QueryHistory("some", options, &results);
-  // Since the query didn't cover the oldest visit in the database, we
-  // expect false here.
-  EXPECT_FALSE(results.reached_beginning());
+  EXPECT_TRUE(results.reached_beginning());

[Scott Hess - ex-Googler, 2013/06/20 19:50:22]

I think the previous EXPECT_FALSE() case was trying to test something additional
to the EXPECT_TRUE() up three lines.  I _think_ it was saying that the oldest
result with "some" wasn't the oldest result possible (such as for std::string()
above).

[rmcilroy, 2013/06/20 21:48:08]

I _think_ this was not reaching the oldest in this case because previously, the
http://example.com/ visited 180 days ago would be in a separate text_database
index file (since they are split by month).  Since this query reached capacity,
the previous FTS search wouldn't try searching the older index file, and
wouldn't know whether there were any other matches in that index, so would
return false for reached_beginning.  Now the text search will have searched all
timescales, and will know that there are no further results, so
reached_beginning should be true.  Maybe we should just remove this test now?

[Scott Hess - ex-Googler, 2013/06/20 22:18:31]

<no-idea-dog/>, but it sounds like you're on top of that.  I think these lines
look like they duplicate the intersection of the std::string() test stanza and
the max_count w/"some" test stanza, so they probably aren't necessary.

 }
 
-// This does most of the same tests above, but searches for a FTS string that
-// will match the pages in question. This will trigger a different code path.
-TEST_F(HistoryQueryTest, FTS) {
+// This does most of the same tests above, but performs a text searches for a
+// string that will match the pages in question. This will trigger a
+// different code path.
+TEST_F(HistoryQueryTest, TextSearch) {
   ASSERT_TRUE(history_.get());
 
   QueryOptions options;
   QueryResults results;
 
   // Query all of them to make sure they are there and in order. Note that
   // this query will return the starred item twice since we requested all
   // starred entries and no de-duping.
   QueryHistory("some", options, &results);
   EXPECT_EQ(3U, results.size());
   EXPECT_TRUE(NthResultIs(results, 0, 2));
   EXPECT_TRUE(NthResultIs(results, 1, 3));
   EXPECT_TRUE(NthResultIs(results, 2, 1));
 
   // Do a query that should only match one of them.
   QueryHistory("PAGETWO", options, &results);
   EXPECT_EQ(1U, results.size());
   EXPECT_TRUE(NthResultIs(results, 0, 3));
 
   // Next query a time range. The beginning should be inclusive, the ending
   // should be exclusive.
   options.begin_time = test_entries[1].time;
   options.end_time = test_entries[3].time;
   QueryHistory("some", options, &results);
   EXPECT_EQ(1U, results.size());
   EXPECT_TRUE(NthResultIs(results, 0, 1));
 }
 
-// Searches titles.
-TEST_F(HistoryQueryTest, FTSTitle) {
+// Tests prefix searching for text search queries.
+TEST_F(HistoryQueryTest, TextSearchPrefix) {
   ASSERT_TRUE(history_.get());
 
   QueryOptions options;
-  QueryResults results;
-
-  // First execute a body-only query, to ensure that it works and that that
-  // version of the statement is not cached for the next query.
-  options.body_only = true;

[Scott Hess - ex-Googler, 2013/06/20 19:50:22]

Is |body_only| still present as an option?

[rmcilroy, 2013/06/20 21:48:08]

I will remove it in the followup CL which removes all traces of FTS.

[Scott Hess - ex-Googler, 2013/06/20 22:18:31]

OK.  Mostly wanted to make sure there wasn't an alternate meaning.

-  QueryHistory("Title", options, &results);
-  EXPECT_EQ(1U, results.size());
-  EXPECT_TRUE(NthResultIs(results, 0, 3));
-  options.body_only = false;
-
-  // Query all time but with a limit on the number of entries. We should
-  // get the N most recent entries.
-  options.max_count = 3;
-  QueryHistory("title", options, &results);
-  EXPECT_EQ(3U, results.size());
-  EXPECT_TRUE(NthResultIs(results, 0, 2));
-  EXPECT_TRUE(NthResultIs(results, 1, 3));
-  EXPECT_TRUE(NthResultIs(results, 2, 1));
-}
-
-// Tests prefix searching for Full Text Search queries.
-TEST_F(HistoryQueryTest, FTSPrefix) {
-  ASSERT_TRUE(history_.get());
-
-  QueryOptions options;
   QueryResults results;
 
   // Query with a prefix search.  Should return matches for "PAGETWO" and
   // "PAGETHREE".
   QueryHistory("PAGET", options, &results);
   EXPECT_EQ(2U, results.size());
   EXPECT_TRUE(NthResultIs(results, 0, 2));
   EXPECT_TRUE(NthResultIs(results, 1, 3));
 }
 
-// Tests max_count feature for Full Text Search queries.
-TEST_F(HistoryQueryTest, FTSCount) {
+// Tests max_count feature for text search queries.
+TEST_F(HistoryQueryTest, TextSearchCount) {
   ASSERT_TRUE(history_.get());
 
   QueryOptions options;
   QueryResults results;
 
   // Query all time but with a limit on the number of entries. We should
   // get the N most recent entries.
   options.max_count = 2;
   QueryHistory("some", options, &results);
   EXPECT_EQ(2U, results.size());
   EXPECT_TRUE(NthResultIs(results, 0, 2));
   EXPECT_TRUE(NthResultIs(results, 1, 3));
 
   // Now query a subset of the pages and limit by N items. "FOO" should match
   // the 2nd & 3rd pages, but we should only get the 3rd one because of the one
   // page max restriction.
   options.max_count = 1;
   QueryHistory("FOO", options, &results);
   EXPECT_EQ(1U, results.size());
   EXPECT_TRUE(NthResultIs(results, 0, 3));
 }
 
-// Tests that FTS queries can find URLs when they exist only in the archived
-// database. This also tests that imported URLs can be found, since we use
-// AddPageWithDetails just like the importer.
-TEST_F(HistoryQueryTest, FTSArchived) {
+// Tests that text search queries can find URLs when they exist only in the
+// archived database. This also tests that imported URLs can be found, since
+// we use AddPageWithDetails just like the importer.
+TEST_F(HistoryQueryTest, TextSearchArchived) {
   ASSERT_TRUE(history_.get());
 
   URLRows urls_to_add;
 
   URLRow row1(GURL("http://foo.bar/"));
-  row1.set_title(UTF8ToUTF16("archived title"));
+  row1.set_title(UTF8ToUTF16("archived title same"));
   row1.set_last_visit(Time::Now() - TimeDelta::FromDays(365));
   urls_to_add.push_back(row1);
 
   URLRow row2(GURL("http://foo.bar/"));
-  row2.set_title(UTF8ToUTF16("nonarchived title"));
+  row2.set_title(UTF8ToUTF16("nonarchived title same"));
   row2.set_last_visit(Time::Now());
   urls_to_add.push_back(row2);
 
   history_->AddPagesWithDetails(urls_to_add, history::SOURCE_BROWSED);
 
   QueryOptions options;
   QueryResults results;
 
-  // Query all time. The title we get should be the one in the full text
-  // database and not the most current title (since otherwise highlighting in
+  // Query all time. The title we get should be the one in the archived and
+  // not the most current title (since otherwise highlighting in
   // the title might be wrong).
   QueryHistory("archived", options, &results);
   ASSERT_EQ(1U, results.size());
   EXPECT_TRUE(row1.url() == results[0].url());
   EXPECT_TRUE(row1.title() == results[0].title());
+
+  // Check query is ordered correctly when split between archived and
+  // non-archived database.
+  QueryHistory("same", options, &results);

[Scott Hess - ex-Googler, 2013/06/20 19:50:22]

Or "title".

[rmcilroy, 2013/06/20 21:48:08]

"title" also finds the other results which contain the text "Title" which are
added from test_entries in Setup(), so that doesn't work here.

[Scott Hess - ex-Googler, 2013/06/20 22:18:31]

In that case, nevermind my point.

+  ASSERT_EQ(2U, results.size());
+  EXPECT_TRUE(row2.url() == results[0].url());
+  EXPECT_TRUE(row2.title() == results[0].title());
+  EXPECT_TRUE(row1.url() == results[1].url());
+  EXPECT_TRUE(row1.title() == results[1].title());
 }
 
 /* TODO(brettw) re-enable this. It is commented out because the current history
    code prohibits adding more than one indexed page with the same URL. When we
    have tiered history, there could be a dupe in the archived history which
    won't get picked up by the deletor and it can happen again. When this is the
    case, we should fix this test to duplicate that situation.
 
-// Tests duplicate collapsing and not in Full Text Search situations.
-TEST_F(HistoryQueryTest, FTSDupes) {
+// Tests duplicate collapsing and not in text search situations.
+TEST_F(HistoryQueryTest, TextSearchDupes) {
   ASSERT_TRUE(history_.get());
 
   QueryOptions options;
   QueryResults results;
 
   QueryHistory("Other", options, &results);
-  EXPECT_EQ(1, results.urls().size());
+  EXPECT_EQ(1U, results.size());
   EXPECT_TRUE(NthResultIs(results, 0, 4));
 }
 */
 
 // Test iterating over pages of results.
 TEST_F(HistoryQueryTest, Paging) {
   // Since results are fetched 1 and 2 at a time, entry #0 and #6 will not
   // be de-duplicated.
   int expected_results[] = { 4, 2, 3, 1, 7, 6, 5, 0 };
   TestPaging(std::string(), expected_results, arraysize(expected_results));
 }
 
-TEST_F(HistoryQueryTest, FTSPaging) {
+TEST_F(HistoryQueryTest, TextSearchPaging) {
   // Since results are fetched 1 and 2 at a time, entry #0 and #6 will not
   // be de-duplicated. Entry #4 does not contain the text "title", so it
   // shouldn't appear.
   int expected_results[] = { 2, 3, 1, 7, 6, 5 };
   TestPaging("title", expected_results, arraysize(expected_results));
 }
 
 }  // namespace history