Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes ad conversions fail after a creative has expired - 1.11.x #5837

Merged
merged 1 commit into from
Jun 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 44 additions & 13 deletions components/brave_ads/browser/bundle_state_database.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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()));

Expand All @@ -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();
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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()));
Expand All @@ -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);
}

Expand Down Expand Up @@ -642,6 +648,11 @@ bool BundleStateDatabase::Migrate() {
break;
}

case 6: {
success = MigrateV6toV7();
break;
}

default: {
NOTREACHED();
break;
Expand Down Expand Up @@ -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
3 changes: 2 additions & 1 deletion components/brave_ads/browser/bundle_state_database.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class BundleStateDatabase {
bool CreateCreativeAdNotificationCategoriesCategoryIndex();

bool CreateAdConversionsTable();
bool TruncateAdConversionsTable();
bool PurgeExpiredAdConversions();
bool InsertOrUpdateAdConversion(
const ads::AdConversionInfo& info);

Expand All @@ -103,6 +103,7 @@ class BundleStateDatabase {
bool MigrateV3toV4();
bool MigrateV4toV5();
bool MigrateV5toV6();
bool MigrateV6toV7();

sql::Database db_;
sql::MetaTable meta_table_;
Expand Down
3 changes: 3 additions & 0 deletions vendor/bat-native-ads/include/bat/ads/ad_conversion_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
#ifndef BAT_ADS_AD_CONVERSION_INFO_H_
#define BAT_ADS_AD_CONVERSION_INFO_H_

#include <stdint.h>

#include <string>
#include <vector>

Expand Down Expand Up @@ -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<AdConversionInfo>;
Expand Down
6 changes: 5 additions & 1 deletion vendor/bat-native-ads/resources/bundle-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@
"creativeSetId",
"urlPattern",
"type",
"observationWindow"
"observationWindow",
"expiryTimestamp"
],
"properties": {
"creativeSetId": {
Expand All @@ -94,6 +95,9 @@
},
"observationWindow": {
"type": "number"
},
"expiryTimestamp": {
"type": "number"
}
}
}
Expand Down
10 changes: 9 additions & 1 deletion vendor/bat-native-ads/src/bat/ads/ad_conversion_info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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!=(
Expand Down Expand Up @@ -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;
}

Expand All @@ -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();
}

Expand Down
9 changes: 9 additions & 0 deletions vendor/bat-native-ads/src/bat/ads/bundle_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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();
}

Expand Down
10 changes: 10 additions & 0 deletions vendor/bat-native-ads/src/bat/ads/internal/catalog_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down