Commit dfed5e52 authored by Justin DeWitt's avatar Justin DeWitt Committed by Commit Bot

[EoS] Change version to a string, and update the protobufs to match

Version as integer timestamp has problems when considering  caching versions
that are actually filtered dynamically on the server, so we switch to
an opaque string version_token that the server can choose.

Client loses the ability to determine if a new version is "newer" or
"older" but this doesn't seem like much of a loss.

Bug: 867488
Change-Id: I43fadaf2ccfecec8143f45473d1b863f38345cac
Reviewed-on: https://chromium-review.googlesource.com/1241531
Commit-Queue: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: default avatarCathy Li <chili@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593807}
parent cdd5ca7f
...@@ -77,21 +77,27 @@ message Site { ...@@ -77,21 +77,27 @@ message Site {
} }
message GetCatalogRequest { message GetCatalogRequest {
// The latest timestamp of the catalog on the client. reserved 1;
int64 created_at_millis = 1;
// The country code for the catalog that was returned. // The country code for the catalog that was returned.
string country_code = 2; string country_code = 2;
// The latest version token the client currently has cached as received in
// GetCatalogResponse.
string version_token = 3;
} }
message GetCatalogResponse { message GetCatalogResponse {
reserved 2;
// Catalog of categories and sites that are appropriate for the client. Will // Catalog of categories and sites that are appropriate for the client. Will
// be empty if the client sends a request with the latest timestamp. // be empty if the client sends a request with the latest timestamp.
Catalog catalog = 1; Catalog catalog = 1;
// The latest timestamp of the catalof for the client on the server.
int64 created_at_millis = 2;
// The country code for the catalog that was returned. // The country code for the catalog that was returned.
string country_code = 3; string country_code = 3;
}
\ No newline at end of file // The version token for the latest ExploreSites Catalog. This is a
// server-generated opaque string.
string version_token = 4;
}
...@@ -66,7 +66,7 @@ constexpr net::NetworkTrafficAnnotationTag traffic_annotation = ...@@ -66,7 +66,7 @@ constexpr net::NetworkTrafficAnnotationTag traffic_annotation =
std::unique_ptr<ExploreSitesFetcher> ExploreSitesFetcher::CreateForGetCatalog( std::unique_ptr<ExploreSitesFetcher> ExploreSitesFetcher::CreateForGetCatalog(
Callback callback, Callback callback,
const int64_t catalog_version, const std::string catalog_version,
const std::string country_code, const std::string country_code,
scoped_refptr<network::SharedURLLoaderFactory> loader_factory) { scoped_refptr<network::SharedURLLoaderFactory> loader_factory) {
GURL url = GetCatalogURL(); GURL url = GetCatalogURL();
...@@ -77,7 +77,7 @@ std::unique_ptr<ExploreSitesFetcher> ExploreSitesFetcher::CreateForGetCatalog( ...@@ -77,7 +77,7 @@ std::unique_ptr<ExploreSitesFetcher> ExploreSitesFetcher::CreateForGetCatalog(
std::unique_ptr<ExploreSitesFetcher> std::unique_ptr<ExploreSitesFetcher>
ExploreSitesFetcher::CreateForGetCategories( ExploreSitesFetcher::CreateForGetCategories(
Callback callback, Callback callback,
const int64_t catalog_version, const std::string catalog_version,
const std::string country_code, const std::string country_code,
scoped_refptr<network::SharedURLLoaderFactory> loader_factory) { scoped_refptr<network::SharedURLLoaderFactory> loader_factory) {
GURL url = GetCategoriesURL(); GURL url = GetCategoriesURL();
...@@ -88,7 +88,7 @@ ExploreSitesFetcher::CreateForGetCategories( ...@@ -88,7 +88,7 @@ ExploreSitesFetcher::CreateForGetCategories(
ExploreSitesFetcher::ExploreSitesFetcher( ExploreSitesFetcher::ExploreSitesFetcher(
Callback callback, Callback callback,
const GURL& url, const GURL& url,
const int64_t catalog_version, const std::string catalog_version,
const std::string country_code, const std::string country_code,
scoped_refptr<network::SharedURLLoaderFactory> loader_factory) scoped_refptr<network::SharedURLLoaderFactory> loader_factory)
: callback_(std::move(callback)), : callback_(std::move(callback)),
...@@ -116,7 +116,7 @@ ExploreSitesFetcher::ExploreSitesFetcher( ...@@ -116,7 +116,7 @@ ExploreSitesFetcher::ExploreSitesFetcher(
traffic_annotation); traffic_annotation);
GetCatalogRequest request; GetCatalogRequest request;
request.set_created_at_millis(catalog_version); request.set_version_token(catalog_version);
request.set_country_code(country_code); request.set_country_code(country_code);
std::string request_message; std::string request_message;
request.SerializeToString(&request_message); request.SerializeToString(&request_message);
......
...@@ -32,14 +32,14 @@ class ExploreSitesFetcher { ...@@ -32,14 +32,14 @@ class ExploreSitesFetcher {
// Creates a fetcher for the GetCatalog RPC. // Creates a fetcher for the GetCatalog RPC.
static std::unique_ptr<ExploreSitesFetcher> CreateForGetCatalog( static std::unique_ptr<ExploreSitesFetcher> CreateForGetCatalog(
Callback callback, Callback callback,
const int64_t catalog_version, const std::string catalog_version,
const std::string country_code, const std::string country_code,
scoped_refptr<network::SharedURLLoaderFactory> loader_factory); scoped_refptr<network::SharedURLLoaderFactory> loader_factory);
// Creates a fetcher for the GetCategories RPC. // Creates a fetcher for the GetCategories RPC.
static std::unique_ptr<ExploreSitesFetcher> CreateForGetCategories( static std::unique_ptr<ExploreSitesFetcher> CreateForGetCategories(
Callback callback, Callback callback,
const int64_t catalog_version, const std::string catalog_version,
const std::string country_code, const std::string country_code,
scoped_refptr<network::SharedURLLoaderFactory> loader_factory); scoped_refptr<network::SharedURLLoaderFactory> loader_factory);
...@@ -49,7 +49,7 @@ class ExploreSitesFetcher { ...@@ -49,7 +49,7 @@ class ExploreSitesFetcher {
explicit ExploreSitesFetcher( explicit ExploreSitesFetcher(
Callback callback, Callback callback,
const GURL& url, const GURL& url,
const int64_t catalog_version, const std::string catalog_version,
const std::string country_code, const std::string country_code,
scoped_refptr<network ::SharedURLLoaderFactory> loader_factory); scoped_refptr<network ::SharedURLLoaderFactory> loader_factory);
......
...@@ -153,7 +153,7 @@ ExploreSitesRequestStatus ExploreSitesFetcherTest::RunFetcher( ...@@ -153,7 +153,7 @@ ExploreSitesRequestStatus ExploreSitesFetcherTest::RunFetcher(
base::OnceCallback<void(void)> respond_callback, base::OnceCallback<void(void)> respond_callback,
std::string* data_received) { std::string* data_received) {
std::unique_ptr<ExploreSitesFetcher> fetcher = std::unique_ptr<ExploreSitesFetcher> fetcher =
ExploreSitesFetcher::CreateForGetCatalog(StoreResult(), 0, "KE", ExploreSitesFetcher::CreateForGetCatalog(StoreResult(), "", "KE",
test_shared_url_loader_factory_); test_shared_url_loader_factory_);
std::move(respond_callback).Run(); std::move(respond_callback).Run();
......
...@@ -24,8 +24,9 @@ static const char kCategoriesTableCreationSql[] = ...@@ -24,8 +24,9 @@ static const char kCategoriesTableCreationSql[] =
// ID is *not* retained // ID is *not* retained
// across catalog // across catalog
// updates. // updates.
"version INTEGER NOT NULL, " // matches an entry in the meta table: "version_token TEXT NOT NULL, " // matches an entry in the meta table:
// ‘current_catalog’ or ‘downloading_catalog’. // ‘current_catalog’ or
// ‘downloading_catalog’.
"type INTEGER NOT NULL, " "type INTEGER NOT NULL, "
"label TEXT NOT NULL, " "label TEXT NOT NULL, "
"image BLOB, " // can be NULL if no image is available, but must be "image BLOB, " // can be NULL if no image is available, but must be
......
...@@ -66,13 +66,13 @@ void ExploreSitesServiceImpl::Shutdown() {} ...@@ -66,13 +66,13 @@ void ExploreSitesServiceImpl::Shutdown() {}
void ExploreSitesServiceImpl::OnTaskQueueIsIdle() {} void ExploreSitesServiceImpl::OnTaskQueueIsIdle() {}
void ExploreSitesServiceImpl::AddUpdatedCatalog( void ExploreSitesServiceImpl::AddUpdatedCatalog(
int64_t catalog_timestamp, std::string version_token,
std::unique_ptr<Catalog> catalog_proto) { std::unique_ptr<Catalog> catalog_proto) {
if (!IsExploreSitesEnabled()) if (!IsExploreSitesEnabled())
return; return;
task_queue_.AddTask(std::make_unique<ImportCatalogTask>( task_queue_.AddTask(std::make_unique<ImportCatalogTask>(
explore_sites_store_.get(), catalog_timestamp, std::move(catalog_proto))); explore_sites_store_.get(), version_token, std::move(catalog_proto)));
} }
} // namespace explore_sites } // namespace explore_sites
...@@ -38,7 +38,7 @@ class ExploreSitesServiceImpl : public ExploreSitesService, ...@@ -38,7 +38,7 @@ class ExploreSitesServiceImpl : public ExploreSitesService,
// TaskQueue::Delegate implementation: // TaskQueue::Delegate implementation:
void OnTaskQueueIsIdle() override; void OnTaskQueueIsIdle() override;
void AddUpdatedCatalog(int64_t catalog_timestamp, void AddUpdatedCatalog(std::string version_token,
std::unique_ptr<Catalog> catalog_proto); std::unique_ptr<Catalog> catalog_proto);
// True when Chrome starts up, this is reset after the catalog is requested // True when Chrome starts up, this is reset after the catalog is requested
......
...@@ -17,11 +17,11 @@ ExploreSitesSite::ExploreSitesSite(ExploreSitesSite&& other) = default; ...@@ -17,11 +17,11 @@ ExploreSitesSite::ExploreSitesSite(ExploreSitesSite&& other) = default;
ExploreSitesSite::~ExploreSitesSite() = default; ExploreSitesSite::~ExploreSitesSite() = default;
ExploreSitesCategory::ExploreSitesCategory(int category_id, ExploreSitesCategory::ExploreSitesCategory(int category_id,
int version, std::string version_token,
int category_type, int category_type,
std::string label) std::string label)
: category_id(category_id), : category_id(category_id),
version(version), version_token(version_token),
category_type(category_type), category_type(category_type),
label(label) {} label(label) {}
......
...@@ -38,14 +38,14 @@ struct ExploreSitesSite { ...@@ -38,14 +38,14 @@ struct ExploreSitesSite {
struct ExploreSitesCategory { struct ExploreSitesCategory {
// Creates a category. Sites should be populated separately. // Creates a category. Sites should be populated separately.
ExploreSitesCategory(int category_id, ExploreSitesCategory(int category_id,
int version, std::string version_token,
int category_type, int category_type,
std::string label); std::string label);
ExploreSitesCategory(ExploreSitesCategory&& other); ExploreSitesCategory(ExploreSitesCategory&& other);
virtual ~ExploreSitesCategory(); virtual ~ExploreSitesCategory();
int category_id; int category_id;
int version; std::string version_token;
int category_type; int category_type;
std::string label; std::string label;
......
...@@ -15,7 +15,7 @@ namespace { ...@@ -15,7 +15,7 @@ namespace {
static const char kSelectCategorySql[] = R"(SELECT category_id, type, label static const char kSelectCategorySql[] = R"(SELECT category_id, type, label
FROM categories FROM categories
WHERE version = ? WHERE version_token = ?
ORDER BY category_id ASC;)"; ORDER BY category_id ASC;)";
static const char kSelectSiteSql[] = R"(SELECT site_id, url, title static const char kSelectSiteSql[] = R"(SELECT site_id, url, title
...@@ -24,39 +24,39 @@ WHERE category_id = ?)"; ...@@ -24,39 +24,39 @@ WHERE category_id = ?)";
const char kDeleteSiteSql[] = R"(DELETE FROM sites const char kDeleteSiteSql[] = R"(DELETE FROM sites
WHERE category_id NOT IN WHERE category_id NOT IN
(SELECT category_id FROM categories WHERE version = ?);)"; (SELECT category_id FROM categories WHERE version_token = ?);)";
const char kDeleteCategorySql[] = "DELETE FROM categories WHERE version <> ?;"; const char kDeleteCategorySql[] =
"DELETE FROM categories WHERE version_token <> ?;";
} // namespace } // namespace
int64_t UpdateCurrentCatalogIfNewer(sql::MetaTable* meta_table, std::string UpdateCurrentCatalogIfNewer(sql::MetaTable* meta_table,
int64_t current_catalog_timestamp) { std::string current_version_token) {
DCHECK(meta_table); DCHECK(meta_table);
int64_t downloading_catalog_timestamp = -1LL; std::string downloading_version_token;
if (!meta_table->GetValue("downloading_catalog", if (!meta_table->GetValue("downloading_catalog",
&downloading_catalog_timestamp) || &downloading_version_token)) {
downloading_catalog_timestamp < 0 || return current_version_token;
downloading_catalog_timestamp <= current_catalog_timestamp) {
return current_catalog_timestamp;
} }
if (!meta_table->SetValue("current_catalog", downloading_catalog_timestamp)) if (!meta_table->SetValue("current_catalog", downloading_version_token))
return -1; return "";
meta_table->DeleteKey("downloading_catalog"); meta_table->DeleteKey("downloading_catalog");
return downloading_catalog_timestamp; return downloading_version_token;
} }
void RemoveOutdatedCatalogEntries(sql::Database* db, int64_t timestamp) { void RemoveOutdatedCatalogEntries(sql::Database* db,
std::string version_token) {
sql::Statement delete_sites( sql::Statement delete_sites(
db->GetCachedStatement(SQL_FROM_HERE, kDeleteSiteSql)); db->GetCachedStatement(SQL_FROM_HERE, kDeleteSiteSql));
delete_sites.BindInt64(0, timestamp); delete_sites.BindString(0, version_token);
delete_sites.Run(); delete_sites.Run();
sql::Statement delete_categories( sql::Statement delete_categories(
db->GetCachedStatement(SQL_FROM_HERE, kDeleteCategorySql)); db->GetCachedStatement(SQL_FROM_HERE, kDeleteCategorySql));
delete_categories.BindInt64(0, timestamp); delete_categories.BindString(0, version_token);
delete_categories.Run(); delete_categories.Run();
} }
...@@ -71,7 +71,7 @@ std::unique_ptr<GetCatalogTask::CategoryList> GetCatalogSync( ...@@ -71,7 +71,7 @@ std::unique_ptr<GetCatalogTask::CategoryList> GetCatalogSync(
// If we are downloading a catalog that is the same version as the one // 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 // currently in use, don't change it. This is an error, should have been
// caught before we got here. // caught before we got here.
int64_t catalog_timestamp = 0; std::string catalog_timestamp;
meta_table.GetValue("current_catalog", &catalog_timestamp); meta_table.GetValue("current_catalog", &catalog_timestamp);
if (update_current) { if (update_current) {
...@@ -79,6 +79,8 @@ std::unique_ptr<GetCatalogTask::CategoryList> GetCatalogSync( ...@@ -79,6 +79,8 @@ std::unique_ptr<GetCatalogTask::CategoryList> GetCatalogSync(
transaction.Begin(); transaction.Begin();
catalog_timestamp = catalog_timestamp =
UpdateCurrentCatalogIfNewer(&meta_table, catalog_timestamp); UpdateCurrentCatalogIfNewer(&meta_table, catalog_timestamp);
if (catalog_timestamp == "")
return nullptr;
RemoveOutdatedCatalogEntries(db, catalog_timestamp); RemoveOutdatedCatalogEntries(db, catalog_timestamp);
if (!transaction.Commit()) if (!transaction.Commit())
return nullptr; return nullptr;
...@@ -86,7 +88,7 @@ std::unique_ptr<GetCatalogTask::CategoryList> GetCatalogSync( ...@@ -86,7 +88,7 @@ std::unique_ptr<GetCatalogTask::CategoryList> GetCatalogSync(
sql::Statement category_statement( sql::Statement category_statement(
db->GetCachedStatement(SQL_FROM_HERE, kSelectCategorySql)); db->GetCachedStatement(SQL_FROM_HERE, kSelectCategorySql));
category_statement.BindInt64(0, catalog_timestamp); category_statement.BindString(0, catalog_timestamp);
auto result = std::make_unique<GetCatalogTask::CategoryList>(); auto result = std::make_unique<GetCatalogTask::CategoryList>();
while (category_statement.Step()) { while (category_statement.Step()) {
......
...@@ -42,9 +42,9 @@ class ExploreSitesGetCatalogTaskTest : public TaskTestBase { ...@@ -42,9 +42,9 @@ class ExploreSitesGetCatalogTaskTest : public TaskTestBase {
void PopulateTestingCatalog(); void PopulateTestingCatalog();
void ValidateTestingCatalog(GetCatalogTask::CategoryList* catalog); void ValidateTestingCatalog(GetCatalogTask::CategoryList* catalog);
void SetDownloadingAndCurrentVersion(int downloading_version, void SetDownloadingAndCurrentVersion(std::string downloading_version_token,
int current_version); std::string current_version_token);
std::pair<int, int> GetCurrentAndDownloadingVersion(); std::pair<std::string, std::string> GetCurrentAndDownloadingVersion();
int GetNumberOfCategoriesInDB(); int GetNumberOfCategoriesInDB();
int GetNumberOfSitesInDB(); int GetNumberOfSitesInDB();
...@@ -61,16 +61,16 @@ void ExploreSitesGetCatalogTaskTest::PopulateTestingCatalog() { ...@@ -61,16 +61,16 @@ void ExploreSitesGetCatalogTaskTest::PopulateTestingCatalog() {
ExecuteSync(base::BindLambdaForTesting([](sql::Database* db) { ExecuteSync(base::BindLambdaForTesting([](sql::Database* db) {
sql::MetaTable meta_table; sql::MetaTable meta_table;
ExploreSitesSchema::InitMetaTable(db, &meta_table); ExploreSitesSchema::InitMetaTable(db, &meta_table);
meta_table.SetValue("current_catalog", 5678); meta_table.SetValue("current_catalog", "5678");
meta_table.DeleteKey("downloading_catalog"); meta_table.DeleteKey("downloading_catalog");
sql::Statement insert(db->GetUniqueStatement(R"( sql::Statement insert(db->GetUniqueStatement(R"(
INSERT INTO categories INSERT INTO categories
(category_id, version, type, label) (category_id, version_token, type, label)
VALUES VALUES
(1, 1234, 1, "label_1"), -- older catalog (1, "1234", 1, "label_1"), -- older catalog
(2, 1234, 2, "label_2"), -- older catalog (2, "1234", 2, "label_2"), -- older catalog
(3, 5678, 1, "label_1"), -- current catalog (3, "5678", 1, "label_1"), -- current catalog
(4, 5678, 2, "label_2"); -- current catalog)")); (4, "5678", 2, "label_2"); -- current catalog)"));
if (!insert.Run()) if (!insert.Run())
return false; return false;
...@@ -94,7 +94,7 @@ void ExploreSitesGetCatalogTaskTest::ValidateTestingCatalog( ...@@ -94,7 +94,7 @@ void ExploreSitesGetCatalogTaskTest::ValidateTestingCatalog(
EXPECT_EQ(2U, catalog->size()); EXPECT_EQ(2U, catalog->size());
ExploreSitesCategory* cat = &catalog->at(0); ExploreSitesCategory* cat = &catalog->at(0);
EXPECT_EQ(3, cat->category_id); EXPECT_EQ(3, cat->category_id);
EXPECT_EQ(5678, cat->version); EXPECT_EQ("5678", cat->version_token);
EXPECT_EQ(1, cat->category_type); EXPECT_EQ(1, cat->category_type);
EXPECT_EQ("label_1", cat->label); EXPECT_EQ("label_1", cat->label);
...@@ -106,7 +106,7 @@ void ExploreSitesGetCatalogTaskTest::ValidateTestingCatalog( ...@@ -106,7 +106,7 @@ void ExploreSitesGetCatalogTaskTest::ValidateTestingCatalog(
cat = &catalog->at(1); cat = &catalog->at(1);
EXPECT_EQ(4, cat->category_id); EXPECT_EQ(4, cat->category_id);
EXPECT_EQ(5678, cat->version); EXPECT_EQ("5678", cat->version_token);
EXPECT_EQ(2, cat->category_type); EXPECT_EQ(2, cat->category_type);
EXPECT_EQ("label_2", cat->label); EXPECT_EQ("label_2", cat->label);
...@@ -118,21 +118,21 @@ void ExploreSitesGetCatalogTaskTest::ValidateTestingCatalog( ...@@ -118,21 +118,21 @@ void ExploreSitesGetCatalogTaskTest::ValidateTestingCatalog(
} }
void ExploreSitesGetCatalogTaskTest::SetDownloadingAndCurrentVersion( void ExploreSitesGetCatalogTaskTest::SetDownloadingAndCurrentVersion(
int downloading_version, std::string downloading_version_token,
int current_version) { std::string current_version_token) {
ExecuteSync(base::BindLambdaForTesting([&](sql::Database* db) { ExecuteSync(base::BindLambdaForTesting([&](sql::Database* db) {
sql::MetaTable meta_table; sql::MetaTable meta_table;
ExploreSitesSchema::InitMetaTable(db, &meta_table); ExploreSitesSchema::InitMetaTable(db, &meta_table);
meta_table.SetValue("downloading_catalog", downloading_version); meta_table.SetValue("downloading_catalog", downloading_version_token);
meta_table.SetValue("current_catalog", current_version); meta_table.SetValue("current_catalog", current_version_token);
return true; return true;
})); }));
} }
std::pair<int, int> std::pair<std::string, std::string>
ExploreSitesGetCatalogTaskTest::GetCurrentAndDownloadingVersion() { ExploreSitesGetCatalogTaskTest::GetCurrentAndDownloadingVersion() {
int64_t current_catalog = -1; std::string current_catalog = "";
int64_t downloading_catalog = -1; std::string downloading_catalog = "";
ExecuteSync(base::BindLambdaForTesting([&](sql::Database* db) { ExecuteSync(base::BindLambdaForTesting([&](sql::Database* db) {
sql::MetaTable meta_table; sql::MetaTable meta_table;
ExploreSitesSchema::InitMetaTable(db, &meta_table); ExploreSitesSchema::InitMetaTable(db, &meta_table);
...@@ -208,7 +208,7 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithVersionUpdate) { ...@@ -208,7 +208,7 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithVersionUpdate) {
PopulateTestingCatalog(); PopulateTestingCatalog();
// Update the testing catalog so that the older catalog is current and the // Update the testing catalog so that the older catalog is current and the
// downloading catalog is ready to upgrade. // downloading catalog is ready to upgrade.
SetDownloadingAndCurrentVersion(5678, 1234); SetDownloadingAndCurrentVersion("5678", "1234");
auto callback = base::BindLambdaForTesting( auto callback = base::BindLambdaForTesting(
[&](std::unique_ptr<GetCatalogTask::CategoryList> catalog) { [&](std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
ValidateTestingCatalog(catalog.get()); ValidateTestingCatalog(catalog.get());
...@@ -216,7 +216,8 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithVersionUpdate) { ...@@ -216,7 +216,8 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithVersionUpdate) {
GetCatalogTask task(store(), true /* update_current */, callback); GetCatalogTask task(store(), true /* update_current */, callback);
RunTask(&task); RunTask(&task);
EXPECT_EQ(std::make_pair(5678, -1), GetCurrentAndDownloadingVersion()); EXPECT_EQ(std::make_pair(std::string("5678"), std::string()),
GetCurrentAndDownloadingVersion());
// The task should have pruned the database. // The task should have pruned the database.
EXPECT_EQ(2, GetNumberOfCategoriesInDB()); EXPECT_EQ(2, GetNumberOfCategoriesInDB());
EXPECT_EQ(2, GetNumberOfSitesInDB()); EXPECT_EQ(2, GetNumberOfSitesInDB());
...@@ -226,7 +227,7 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithoutVersionUpdate) { ...@@ -226,7 +227,7 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithoutVersionUpdate) {
PopulateTestingCatalog(); PopulateTestingCatalog();
// Make "1234" the downloading version, we should not see any changes in the // Make "1234" the downloading version, we should not see any changes in the
// DB if the |update_current| flag is false. // DB if the |update_current| flag is false.
SetDownloadingAndCurrentVersion(1234, 5678); SetDownloadingAndCurrentVersion("1234", "5678");
auto callback = base::BindLambdaForTesting( auto callback = base::BindLambdaForTesting(
[&](std::unique_ptr<GetCatalogTask::CategoryList> catalog) { [&](std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
ValidateTestingCatalog(catalog.get()); ValidateTestingCatalog(catalog.get());
...@@ -234,14 +235,15 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithoutVersionUpdate) { ...@@ -234,14 +235,15 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithoutVersionUpdate) {
GetCatalogTask task(store(), false /* update_current */, callback); GetCatalogTask task(store(), false /* update_current */, callback);
RunTask(&task); RunTask(&task);
EXPECT_EQ(std::make_pair(5678, 1234), GetCurrentAndDownloadingVersion()); EXPECT_EQ(std::make_pair(std::string("5678"), std::string("1234")),
GetCurrentAndDownloadingVersion());
EXPECT_EQ(4, GetNumberOfCategoriesInDB()); EXPECT_EQ(4, GetNumberOfCategoriesInDB());
EXPECT_EQ(4, GetNumberOfSitesInDB()); EXPECT_EQ(4, GetNumberOfSitesInDB());
} }
TEST_F(ExploreSitesGetCatalogTaskTest, InvalidCatalogVersions) { TEST_F(ExploreSitesGetCatalogTaskTest, InvalidCatalogVersions) {
PopulateTestingCatalog(); PopulateTestingCatalog();
SetDownloadingAndCurrentVersion(-1, -1); SetDownloadingAndCurrentVersion("", "");
auto callback = base::BindLambdaForTesting( auto callback = base::BindLambdaForTesting(
[&](std::unique_ptr<GetCatalogTask::CategoryList> catalog) { [&](std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
EXPECT_EQ(0U, catalog->size()); EXPECT_EQ(0U, catalog->size());
......
...@@ -67,10 +67,10 @@ void ExploreSitesGetImagesTaskTest::PopulateTestingCatalog() { ...@@ -67,10 +67,10 @@ void ExploreSitesGetImagesTaskTest::PopulateTestingCatalog() {
meta_table.DeleteKey("downloading_catalog"); meta_table.DeleteKey("downloading_catalog");
sql::Statement insert(db->GetUniqueStatement(R"( sql::Statement insert(db->GetUniqueStatement(R"(
INSERT INTO categories INSERT INTO categories
(category_id, version, type, label) (category_id, version_token, type, label)
VALUES VALUES
(3, 5678, 1, "label_1"), (3, "5678", 1, "label_1"),
(4, 5678, 2, "label_2");)")); (4, "5678", 2, "label_2");)"));
if (!insert.Run()) if (!insert.Run())
return false; return false;
......
...@@ -14,17 +14,17 @@ namespace explore_sites { ...@@ -14,17 +14,17 @@ namespace explore_sites {
namespace { namespace {
static const char kDeleteExistingCategorySql[] = static const char kDeleteExistingCategorySql[] =
"DELETE FROM categories WHERE version = ?"; "DELETE FROM categories WHERE version_token = ?";
static const char kInsertCategorySql[] = R"(INSERT INTO categories static const char kInsertCategorySql[] = R"(INSERT INTO categories
(version, type, label, image) (version_token, type, label, image)
VALUES VALUES
(?, ?, ?, ?);)"; (?, ?, ?, ?);)";
static const char kDeleteExistingSiteSql[] = R"(DELETE FROM sites static const char kDeleteExistingSiteSql[] = R"(DELETE FROM sites
WHERE ( WHERE (
SELECT COUNT(1) FROM categories SELECT COUNT(1) FROM categories
WHERE category_id = sites.category_id AND categories.version = ?) > 0)"; WHERE category_id = sites.category_id AND categories.version_token = ?) > 0)";
static const char kInsertSiteSql[] = R"(INSERT INTO sites static const char kInsertSiteSql[] = R"(INSERT INTO sites
(url, category_id, title, favicon) (url, category_id, title, favicon)
...@@ -33,7 +33,7 @@ VALUES ...@@ -33,7 +33,7 @@ VALUES
} // namespace } // namespace
bool ImportCatalogSync(int64_t catalog_timestamp, bool ImportCatalogSync(std::string version_token,
std::unique_ptr<Catalog> catalog_proto, std::unique_ptr<Catalog> catalog_proto,
sql::Database* db) { sql::Database* db) {
if (!db || !catalog_proto) if (!db || !catalog_proto)
...@@ -50,9 +50,9 @@ bool ImportCatalogSync(int64_t catalog_timestamp, ...@@ -50,9 +50,9 @@ bool ImportCatalogSync(int64_t catalog_timestamp,
// If we are downloading a catalog that is the same version as the one // 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 // currently in use, don't change it. This is an error, should have been
// caught before we got here. // caught before we got here.
int64_t current_catalog_timestamp = 0; std::string current_version_token;
if (meta_table.GetValue("current_catalog", &current_catalog_timestamp) && if (meta_table.GetValue("current_catalog", &current_version_token) &&
current_catalog_timestamp == catalog_timestamp) { current_version_token == version_token) {
return false; return false;
} }
...@@ -60,17 +60,17 @@ bool ImportCatalogSync(int64_t catalog_timestamp, ...@@ -60,17 +60,17 @@ bool ImportCatalogSync(int64_t catalog_timestamp,
// the catalog with the timestamp that matches the catalog we are importing. // the catalog with the timestamp that matches the catalog we are importing.
sql::Statement site_clear_statement( sql::Statement site_clear_statement(
db->GetCachedStatement(SQL_FROM_HERE, kDeleteExistingSiteSql)); db->GetCachedStatement(SQL_FROM_HERE, kDeleteExistingSiteSql));
site_clear_statement.BindInt64(0, catalog_timestamp); site_clear_statement.BindString(0, version_token);
site_clear_statement.Run(); site_clear_statement.Run();
sql::Statement category_clear_statement( sql::Statement category_clear_statement(
db->GetCachedStatement(SQL_FROM_HERE, kDeleteExistingCategorySql)); db->GetCachedStatement(SQL_FROM_HERE, kDeleteExistingCategorySql));
category_clear_statement.BindInt64(0, catalog_timestamp); category_clear_statement.BindString(0, version_token);
category_clear_statement.Run(); category_clear_statement.Run();
// Update the downloading catalog version number to match what we are // Update the downloading catalog version number to match what we are
// importing. // importing.
if (!meta_table.SetValue("downloading_catalog", catalog_timestamp)) if (!meta_table.SetValue("downloading_catalog", version_token))
return false; return false;
// Then insert each category. // Then insert each category.
...@@ -79,7 +79,7 @@ bool ImportCatalogSync(int64_t catalog_timestamp, ...@@ -79,7 +79,7 @@ bool ImportCatalogSync(int64_t catalog_timestamp,
db->GetCachedStatement(SQL_FROM_HERE, kInsertCategorySql)); db->GetCachedStatement(SQL_FROM_HERE, kInsertCategorySql));
int col = 0; int col = 0;
category_statement.BindInt64(col++, catalog_timestamp); category_statement.BindString(col++, version_token);
category_statement.BindInt(col++, static_cast<int>(category.type())); category_statement.BindInt(col++, static_cast<int>(category.type()));
category_statement.BindString(col++, category.localized_title()); category_statement.BindString(col++, category.localized_title());
category_statement.BindString(col++, category.icon()); category_statement.BindString(col++, category.icon());
...@@ -106,17 +106,17 @@ bool ImportCatalogSync(int64_t catalog_timestamp, ...@@ -106,17 +106,17 @@ bool ImportCatalogSync(int64_t catalog_timestamp,
} }
ImportCatalogTask::ImportCatalogTask(ExploreSitesStore* store, ImportCatalogTask::ImportCatalogTask(ExploreSitesStore* store,
int64_t catalog_timestamp, std::string version_token,
std::unique_ptr<Catalog> catalog_proto) std::unique_ptr<Catalog> catalog_proto)
: store_(store), : store_(store),
catalog_timestamp_(catalog_timestamp), version_token_(version_token),
catalog_proto_(std::move(catalog_proto)), catalog_proto_(std::move(catalog_proto)),
weak_ptr_factory_(this) {} weak_ptr_factory_(this) {}
ImportCatalogTask::~ImportCatalogTask() = default; ImportCatalogTask::~ImportCatalogTask() = default;
void ImportCatalogTask::Run() { void ImportCatalogTask::Run() {
store_->Execute(base::BindOnce(&ImportCatalogSync, catalog_timestamp_, store_->Execute(base::BindOnce(&ImportCatalogSync, version_token_,
std::move(catalog_proto_)), std::move(catalog_proto_)),
base::BindOnce(&ImportCatalogTask::FinishedExecuting, base::BindOnce(&ImportCatalogTask::FinishedExecuting,
weak_ptr_factory_.GetWeakPtr()), weak_ptr_factory_.GetWeakPtr()),
......
...@@ -23,7 +23,7 @@ namespace explore_sites { ...@@ -23,7 +23,7 @@ namespace explore_sites {
class ImportCatalogTask : public Task { class ImportCatalogTask : public Task {
public: public:
ImportCatalogTask(ExploreSitesStore* store, ImportCatalogTask(ExploreSitesStore* store,
int64_t catalog_timestamp, std::string version_token,
std::unique_ptr<Catalog> catalog_proto); std::unique_ptr<Catalog> catalog_proto);
~ImportCatalogTask() override; ~ImportCatalogTask() override;
...@@ -37,7 +37,7 @@ class ImportCatalogTask : public Task { ...@@ -37,7 +37,7 @@ class ImportCatalogTask : public Task {
void FinishedExecuting(bool result); void FinishedExecuting(bool result);
ExploreSitesStore* store_; // outlives this class. ExploreSitesStore* store_; // outlives this class.
int64_t catalog_timestamp_; std::string version_token_;
std::unique_ptr<Catalog> catalog_proto_; std::unique_ptr<Catalog> catalog_proto_;
bool complete_ = false; bool complete_ = false;
......
...@@ -23,7 +23,7 @@ using offline_pages::TaskTestBase; ...@@ -23,7 +23,7 @@ using offline_pages::TaskTestBase;
namespace explore_sites { namespace explore_sites {
constexpr int64_t timestamp = 12345L; const char kVersionToken[] = "12345";
const char kGoogleUrl[] = "https://www.google.com"; const char kGoogleUrl[] = "https://www.google.com";
const char kGoogleTitle[] = "Google Search"; const char kGoogleTitle[] = "Google Search";
const char kGmailUrl[] = "https://mail.google.com/mail"; const char kGmailUrl[] = "https://mail.google.com/mail";
...@@ -59,7 +59,7 @@ class ExploreSitesImportCatalogTaskTest : public TaskTestBase { ...@@ -59,7 +59,7 @@ class ExploreSitesImportCatalogTaskTest : public TaskTestBase {
TEST_F(ExploreSitesImportCatalogTaskTest, StoreFailure) { TEST_F(ExploreSitesImportCatalogTaskTest, StoreFailure) {
store()->SetInitializationStatusForTest(InitializationStatus::FAILURE); store()->SetInitializationStatusForTest(InitializationStatus::FAILURE);
ImportCatalogTask task(store(), timestamp, std::make_unique<Catalog>()); ImportCatalogTask task(store(), kVersionToken, std::make_unique<Catalog>());
RunTask(&task); RunTask(&task);
// A null catalog should be completed but return with an error. // A null catalog should be completed but return with an error.
...@@ -68,7 +68,7 @@ TEST_F(ExploreSitesImportCatalogTaskTest, StoreFailure) { ...@@ -68,7 +68,7 @@ TEST_F(ExploreSitesImportCatalogTaskTest, StoreFailure) {
} }
TEST_F(ExploreSitesImportCatalogTaskTest, EmptyTask) { TEST_F(ExploreSitesImportCatalogTaskTest, EmptyTask) {
ImportCatalogTask task(store(), timestamp, std::unique_ptr<Catalog>()); ImportCatalogTask task(store(), kVersionToken, std::unique_ptr<Catalog>());
RunTask(&task); RunTask(&task);
// A null catalog should be completed but return with an error. // A null catalog should be completed but return with an error.
...@@ -77,17 +77,17 @@ TEST_F(ExploreSitesImportCatalogTaskTest, EmptyTask) { ...@@ -77,17 +77,17 @@ TEST_F(ExploreSitesImportCatalogTaskTest, EmptyTask) {
} }
// This tests the behavior of the catalog task when there is already a catalog // This tests the behavior of the catalog task when there is already a catalog
// with the current timestamp in the database. This tests both the case where it // with the current version_token in the database. This tests both the case
// is the "current" catalog, and where it is the "downloading" catalog. // where it is the "current" catalog, and where it is the "downloading" catalog.
TEST_F(ExploreSitesImportCatalogTaskTest, CatalogAlreadyInUse) { TEST_F(ExploreSitesImportCatalogTaskTest, CatalogAlreadyInUse) {
// Successfully import a catalog with "timestamp". // Successfully import a catalog with "version_token".
ImportCatalogTask task(store(), timestamp, std::make_unique<Catalog>()); ImportCatalogTask task(store(), kVersionToken, std::make_unique<Catalog>());
RunTask(&task); RunTask(&task);
ASSERT_TRUE(task.result()); ASSERT_TRUE(task.result());
// Importing the same catalog again should cause a successful import, // Importing the same catalog again should cause a successful import,
// since the catalog was not "current". // since the catalog was not "current".
ImportCatalogTask task2(store(), timestamp, std::make_unique<Catalog>()); ImportCatalogTask task2(store(), kVersionToken, std::make_unique<Catalog>());
RunTask(&task2); RunTask(&task2);
EXPECT_TRUE(task2.result()); EXPECT_TRUE(task2.result());
...@@ -95,13 +95,13 @@ TEST_F(ExploreSitesImportCatalogTaskTest, CatalogAlreadyInUse) { ...@@ -95,13 +95,13 @@ TEST_F(ExploreSitesImportCatalogTaskTest, CatalogAlreadyInUse) {
ExecuteSync(base::BindLambdaForTesting([&](sql::Database* db) { ExecuteSync(base::BindLambdaForTesting([&](sql::Database* db) {
sql::MetaTable meta_table; sql::MetaTable meta_table;
ExploreSitesSchema::InitMetaTable(db, &meta_table); ExploreSitesSchema::InitMetaTable(db, &meta_table);
meta_table.SetValue("current_catalog", timestamp); meta_table.SetValue("current_catalog", kVersionToken);
meta_table.DeleteKey("downloading_catalog"); meta_table.DeleteKey("downloading_catalog");
return true; return true;
})); }));
// Now it should fail to import another copy of the same catalog. // Now it should fail to import another copy of the same catalog.
ImportCatalogTask task3(store(), timestamp, std::make_unique<Catalog>()); ImportCatalogTask task3(store(), kVersionToken, std::make_unique<Catalog>());
RunTask(&task3); RunTask(&task3);
EXPECT_TRUE(task3.complete()); EXPECT_TRUE(task3.complete());
EXPECT_FALSE(task3.result()); EXPECT_FALSE(task3.result());
...@@ -125,7 +125,7 @@ TEST_F(ExploreSitesImportCatalogTaskTest, BasicCatalog) { ...@@ -125,7 +125,7 @@ TEST_F(ExploreSitesImportCatalogTaskTest, BasicCatalog) {
gmail->set_site_url(kGmailUrl); gmail->set_site_url(kGmailUrl);
gmail->set_icon(kIcon); gmail->set_icon(kIcon);
ImportCatalogTask task(store(), timestamp, std::move(catalog)); ImportCatalogTask task(store(), kVersionToken, std::move(catalog));
RunTask(&task); RunTask(&task);
EXPECT_TRUE(task.complete()); EXPECT_TRUE(task.complete());
...@@ -137,10 +137,11 @@ TEST_F(ExploreSitesImportCatalogTaskTest, BasicCatalog) { ...@@ -137,10 +137,11 @@ TEST_F(ExploreSitesImportCatalogTaskTest, BasicCatalog) {
cat_count_s.Step(); cat_count_s.Step();
EXPECT_EQ(1, cat_count_s.ColumnInt(0)); EXPECT_EQ(1, cat_count_s.ColumnInt(0));
sql::Statement cat_data_s(db->GetUniqueStatement( sql::Statement cat_data_s(
"SELECT version, type, label, image, category_id FROM categories")); db->GetUniqueStatement("SELECT version_token, type, label, image, "
"category_id FROM categories"));
cat_data_s.Step(); cat_data_s.Step();
EXPECT_EQ(timestamp, cat_data_s.ColumnInt64(0)); EXPECT_EQ(kVersionToken, cat_data_s.ColumnString(0));
EXPECT_EQ(static_cast<int>(Category_CategoryType_GOOGLE), EXPECT_EQ(static_cast<int>(Category_CategoryType_GOOGLE),
cat_data_s.ColumnInt(1)); cat_data_s.ColumnInt(1));
EXPECT_EQ(kGoogleCategoryTitle, cat_data_s.ColumnString(2)); EXPECT_EQ(kGoogleCategoryTitle, cat_data_s.ColumnString(2));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment