From 114542d127aae8a5d2b3150f7793d5028c9a9f12 Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Fri, 12 Jun 2020 23:29:42 +0100 Subject: [PATCH] Fixes ad conversions fail after a creative has expired --- .../browser/bundle_state_database.cc | 57 ++++++++++++++----- .../brave_ads/browser/bundle_state_database.h | 3 +- .../include/bat/ads/ad_conversion_info.h | 3 + .../resources/bundle-schema.json | 6 +- .../src/bat/ads/ad_conversion_info.cc | 10 +++- .../src/bat/ads/bundle_state.cc | 9 +++ .../src/bat/ads/internal/catalog_state.cc | 10 ++++ 7 files changed, 82 insertions(+), 16 deletions(-) diff --git a/components/brave_ads/browser/bundle_state_database.cc b/components/brave_ads/browser/bundle_state_database.cc index 100a710ec067..6ae9295f128c 100644 --- a/components/brave_ads/browser/bundle_state_database.cc +++ b/components/brave_ads/browser/bundle_state_database.cc @@ -25,8 +25,8 @@ namespace brave_ads { namespace { -const int kCurrentVersionNumber = 6; -const int kCompatibleVersionNumber = 6; +const int kCurrentVersionNumber = 7; +const int kCompatibleVersionNumber = 7; } // namespace @@ -342,24 +342,27 @@ bool BundleStateDatabase::CreateAdConversionsTable() { // new constraints to the schema const std::string sql = base::StringPrintf( "CREATE TABLE %s " - "(id INTEGER PRIMARY KEY, " - "creative_set_id LONGVARCHAR NOT NULL, " + "(creative_set_id LONGVARCHAR NOT NULL, " "type LONGVARCHAR NOT NULL, " "url_pattern LONGVARCHAR NOT NULL, " - "observation_window INTEGER NOT NULL)", + "observation_window INTEGER NOT NULL, " + "expiry_timestamp TIMESTAMP, " + "UNIQUE(creative_set_id, type, url_pattern), " + "PRIMARY KEY(creative_set_id, type, url_pattern))", table_name); return GetDB().Execute(sql.c_str()); } -bool BundleStateDatabase::TruncateAdConversionsTable() { +bool BundleStateDatabase::PurgeExpiredAdConversions() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); const bool is_initialized = Init(); DCHECK(is_initialized); const std::string sql = - "DELETE FROM ad_conversions"; + "DELETE FROM ad_conversions " + "WHERE strftime('%s','now') >= expiry_timestamp"; sql::Statement statement(GetDB().GetUniqueStatement(sql.c_str())); @@ -378,16 +381,18 @@ bool BundleStateDatabase::InsertOrUpdateAdConversion( "(creative_set_id, " "type, " "url_pattern, " - "observation_window) VALUES (%s)", - CreateBindingParameterPlaceholders(4).c_str()); + "observation_window, " + "expiry_timestamp) VALUES (%s)", + CreateBindingParameterPlaceholders(5).c_str()); sql::Statement statement(GetDB().GetUniqueStatement(sql.c_str())); statement.BindString(0, info.creative_set_id); statement.BindString(1, info.type); statement.BindString(2, info.url_pattern); - // Use BindInt64 for uint32_t types to avoid uint32_t to int32_t cast. + // Use BindInt64 for uint32_t types to avoid uint32_t to int32_t cast statement.BindInt64(3, info.observation_window); + statement.BindInt64(4, info.expiry_timestamp); return statement.Run(); } @@ -403,11 +408,10 @@ bool BundleStateDatabase::SaveBundleState( return false; } - // We are completely replacing the database here so truncate all the tables if (!TruncateCategoriesTable() || !TruncateCreativeAdNotificationCategoriesTable() || !TruncateCreativeAdNotificationsTable() || - !TruncateAdConversionsTable()) { + !PurgeExpiredAdConversions()) { GetDB().RollbackTransaction(); return false; } @@ -525,7 +529,8 @@ bool BundleStateDatabase::GetAdConversions( "ac.creative_set_id, " "ac.type, " "ac.url_pattern, " - "ac.observation_window " + "ac.observation_window, " + "ac.expiry_timestamp " "FROM ad_conversions AS ac"; sql::Statement statement(db_.GetUniqueStatement(sql.c_str())); @@ -536,6 +541,7 @@ bool BundleStateDatabase::GetAdConversions( info.type = statement.ColumnString(1); info.url_pattern = statement.ColumnString(2); info.observation_window = statement.ColumnInt(3); + info.expiry_timestamp = statement.ColumnInt64(4); ad_conversions->emplace_back(info); } @@ -642,6 +648,11 @@ bool BundleStateDatabase::Migrate() { break; } + case 6: { + success = MigrateV6toV7(); + break; + } + default: { NOTREACHED(); break; @@ -781,4 +792,24 @@ bool BundleStateDatabase::MigrateV5toV6() { return GetDB().Execute(create_ad_info_table_sql.c_str()); } +bool BundleStateDatabase::MigrateV6toV7() { + const std::string drop_ad_conversions_table_sql = + "DROP TABLE IF EXISTS ad_conversions"; + if (!GetDB().Execute(drop_ad_conversions_table_sql.c_str())) { + return false; + } + + const std::string create_ad_conversions_table_sql = + "CREATE TABLE ad_conversions " + "(creative_set_id LONGVARCHAR NOT NULL, " + "type LONGVARCHAR NOT NULL, " + "url_pattern LONGVARCHAR NOT NULL, " + "observation_window INTEGER NOT NULL, " + "expiry_timestamp TIMESTAMP, " + "UNIQUE(creative_set_id, type, url_pattern), " + "PRIMARY KEY(creative_set_id, type, url_pattern))"; + + return GetDB().Execute(create_ad_conversions_table_sql.c_str()); +} + } // namespace brave_ads diff --git a/components/brave_ads/browser/bundle_state_database.h b/components/brave_ads/browser/bundle_state_database.h index 1adad6139ecc..ba3b6fcdb7a9 100644 --- a/components/brave_ads/browser/bundle_state_database.h +++ b/components/brave_ads/browser/bundle_state_database.h @@ -87,7 +87,7 @@ class BundleStateDatabase { bool CreateCreativeAdNotificationCategoriesCategoryIndex(); bool CreateAdConversionsTable(); - bool TruncateAdConversionsTable(); + bool PurgeExpiredAdConversions(); bool InsertOrUpdateAdConversion( const ads::AdConversionInfo& info); @@ -103,6 +103,7 @@ class BundleStateDatabase { bool MigrateV3toV4(); bool MigrateV4toV5(); bool MigrateV5toV6(); + bool MigrateV6toV7(); sql::Database db_; sql::MetaTable meta_table_; diff --git a/vendor/bat-native-ads/include/bat/ads/ad_conversion_info.h b/vendor/bat-native-ads/include/bat/ads/ad_conversion_info.h index bff5c531d55a..fee89a58ea3a 100644 --- a/vendor/bat-native-ads/include/bat/ads/ad_conversion_info.h +++ b/vendor/bat-native-ads/include/bat/ads/ad_conversion_info.h @@ -6,6 +6,8 @@ #ifndef BAT_ADS_AD_CONVERSION_INFO_H_ #define BAT_ADS_AD_CONVERSION_INFO_H_ +#include + #include #include @@ -41,6 +43,7 @@ struct ADS_EXPORT AdConversionInfo { std::string type; std::string url_pattern; unsigned int observation_window = 0; + uint64_t expiry_timestamp = 0; }; using AdConversionList = std::vector; diff --git a/vendor/bat-native-ads/resources/bundle-schema.json b/vendor/bat-native-ads/resources/bundle-schema.json index 797206fed1ef..4c15a51d66f2 100644 --- a/vendor/bat-native-ads/resources/bundle-schema.json +++ b/vendor/bat-native-ads/resources/bundle-schema.json @@ -80,7 +80,8 @@ "creativeSetId", "urlPattern", "type", - "observationWindow" + "observationWindow", + "expiryTimestamp" ], "properties": { "creativeSetId": { @@ -94,6 +95,9 @@ }, "observationWindow": { "type": "number" + }, + "expiryTimestamp": { + "type": "number" } } } diff --git a/vendor/bat-native-ads/src/bat/ads/ad_conversion_info.cc b/vendor/bat-native-ads/src/bat/ads/ad_conversion_info.cc index a9da092b4b59..8785958749ca 100644 --- a/vendor/bat-native-ads/src/bat/ads/ad_conversion_info.cc +++ b/vendor/bat-native-ads/src/bat/ads/ad_conversion_info.cc @@ -21,7 +21,8 @@ bool AdConversionInfo::operator==( return creative_set_id == rhs.creative_set_id && type == rhs.type && url_pattern == rhs.url_pattern && - observation_window == rhs.observation_window; + observation_window == rhs.observation_window && + expiry_timestamp == rhs.expiry_timestamp; } bool AdConversionInfo::operator!=( @@ -65,6 +66,10 @@ Result AdConversionInfo::FromJson( observation_window = document["observation_window"].GetUint(); } + if (document.HasMember("expiry_timestamp")) { + expiry_timestamp = document["expiry_timestamp"].GetUint64(); + } + return SUCCESS; } @@ -83,6 +88,9 @@ void SaveToJson(JsonWriter* writer, const AdConversionInfo& info) { writer->String("observation_window"); writer->Uint(info.observation_window); + writer->String("expiry_timestamp"); + writer->Uint64(info.expiry_timestamp); + writer->EndObject(); } diff --git a/vendor/bat-native-ads/src/bat/ads/bundle_state.cc b/vendor/bat-native-ads/src/bat/ads/bundle_state.cc index 8c1198e1779f..8c52cac2f6d1 100644 --- a/vendor/bat-native-ads/src/bat/ads/bundle_state.cc +++ b/vendor/bat-native-ads/src/bat/ads/bundle_state.cc @@ -5,6 +5,7 @@ #include "bat/ads/bundle_state.h" +#include "base/time/time.h" #include "bat/ads/internal/json_helper.h" #include "bat/ads/internal/url_util.h" @@ -139,6 +140,11 @@ Result BundleState::FromJson( ad_conversion["observationWindow"].GetUint(); } + if (ad_conversion.HasMember("expiryTimestamp")) { + info.expiry_timestamp = + ad_conversion["expiryTimestamp"].GetUint64(); + } + new_ad_conversions.push_back(info); } } @@ -239,6 +245,9 @@ void SaveToJson( writer->String("observationWindow"); writer->Uint(ad_conversion.observation_window); + writer->String("expiryTimestamp"); + writer->Uint64(ad_conversion.expiry_timestamp); + writer->EndObject(); } diff --git a/vendor/bat-native-ads/src/bat/ads/internal/catalog_state.cc b/vendor/bat-native-ads/src/bat/ads/internal/catalog_state.cc index a2130661d5f2..6f5b642503f5 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/catalog_state.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/catalog_state.cc @@ -132,6 +132,16 @@ Result CatalogState::FromJson( ad_conversion.observation_window = conversion["observationWindow"].GetUint(); + base::Time end_at_timestamp; + if (!base::Time::FromUTCString(campaign_info.end_at.c_str(), + &end_at_timestamp)) { + continue; + } + + base::Time expiry_timestamp = end_at_timestamp + + base::TimeDelta::FromDays(ad_conversion.observation_window); + ad_conversion.expiry_timestamp = expiry_timestamp.ToDoubleT(); + creative_set_info.ad_conversions.push_back(ad_conversion); }