Skip to content

Commit

Permalink
Merge pull request brave#24426 from brave/issues/39466
Browse files Browse the repository at this point in the history 
[ads] Remove invalid legacy transactions
  • Loading branch information
@tmancey
tmancey authored Jun 28, 2024
2 parents b749af2 + 7ffcc45 commit 7be1b9f
Show file tree
Hide file tree
Showing 13 changed files with 138 additions and 94 deletions.
12 changes: 8 additions & 4 deletions components/brave_ads/core/internal/account/account_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,8 @@ TEST_F(BraveAdsAccountTest, AddTransactionWhenDepositingCashForRewardsUser) {
EXPECT_CALL(callback,
Run(/*success=*/true, /*transactions=*/::testing::SizeIs(1)));
const database::table::Transactions database_table;
database_table.GetAll(callback.Get());
database_table.GetForDateRange(/*from_time=*/DistantPast(),
/*to_time=*/DistantFuture(), callback.Get());
}

TEST_F(BraveAdsAccountTest, AddTransactionWhenDepositingNonCashForRewardsUser) {
Expand All @@ -346,7 +347,8 @@ TEST_F(BraveAdsAccountTest, AddTransactionWhenDepositingNonCashForRewardsUser) {
EXPECT_CALL(callback,
Run(/*success=*/true, /*transactions=*/::testing::SizeIs(1)));
const database::table::Transactions database_table;
database_table.GetAll(callback.Get());
database_table.GetForDateRange(/*from_time=*/DistantPast(),
/*to_time=*/DistantFuture(), callback.Get());
}

TEST_F(BraveAdsAccountTest,
Expand All @@ -371,7 +373,8 @@ TEST_F(BraveAdsAccountTest,
EXPECT_CALL(callback,
Run(/*success=*/true, /*transactions=*/::testing::IsEmpty()));
const database::table::Transactions database_table;
database_table.GetAll(callback.Get());
database_table.GetForDateRange(/*from_time=*/DistantPast(),
/*to_time=*/DistantFuture(), callback.Get());
}

TEST_F(BraveAdsAccountTest,
Expand All @@ -396,7 +399,8 @@ TEST_F(BraveAdsAccountTest,
EXPECT_CALL(callback,
Run(/*success=*/true, /*transactions=*/::testing::IsEmpty()));
const database::table::Transactions database_table;
database_table.GetAll(callback.Get());
database_table.GetForDateRange(/*from_time=*/DistantPast(),
/*to_time=*/DistantFuture(), callback.Get());
}

} // namespace brave_ads
80 changes: 39 additions & 41 deletions .../brave_ads/core/internal/account/confirmations/queue/confirmation_queue_database_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,45 @@ void GetCallback(GetConfirmationQueueCallback callback,
std::move(callback).Run(/*success=*/true, confirmation_queue_items);
}

void MigrateToV36(mojom::DBTransactionInfo* const transaction) {
CHECK(transaction);

mojom::DBCommandInfoPtr command = mojom::DBCommandInfo::New();
command->type = mojom::DBCommandInfo::Type::EXECUTE;
command->sql =
R"(
CREATE TABLE confirmation_queue (
id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL,
transaction_id TEXT NOT NULL,
creative_instance_id TEXT NOT NULL,
type TEXT NOT NULL,
ad_type TEXT NOT NULL,
created_at TIMESTAMP NOT NULL,
token TEXT,
blinded_token TEXT,
unblinded_token TEXT,
public_key TEXT,
signature TEXT,
credential_base64url TEXT,
user_data TEXT NOT NULL,
process_at TIMESTAMP NOT NULL,
retry_count INTEGER DEFAULT 0
);)";
transaction->commands.push_back(std::move(command));

// Optimize database query for `GetNext`.
CreateTableIndex(transaction, /*table_name=*/"confirmation_queue",
/*columns=*/{"process_at"});
}

void MigrateToV38(mojom::DBTransactionInfo* const transaction) {
CHECK(transaction);

// The conversion queue is deprecated since all confirmations are now being
// added to the confirmation queue.
DropTable(transaction, "conversion_queue");
}

} // namespace

ConfirmationQueue::ConfirmationQueue() : batch_size_(kDefaultBatchSize) {}
Expand Down Expand Up @@ -459,47 +498,6 @@ void ConfirmationQueue::Migrate(mojom::DBTransactionInfo* const transaction,

///////////////////////////////////////////////////////////////////////////////

void ConfirmationQueue::MigrateToV36(
mojom::DBTransactionInfo* const transaction) const {
CHECK(transaction);

mojom::DBCommandInfoPtr command = mojom::DBCommandInfo::New();
command->type = mojom::DBCommandInfo::Type::EXECUTE;
command->sql =
R"(
CREATE TABLE confirmation_queue (
id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL,
transaction_id TEXT NOT NULL,
creative_instance_id TEXT NOT NULL,
type TEXT NOT NULL,
ad_type TEXT NOT NULL,
created_at TIMESTAMP NOT NULL,
token TEXT,
blinded_token TEXT,
unblinded_token TEXT,
public_key TEXT,
signature TEXT,
credential_base64url TEXT,
user_data TEXT NOT NULL,
process_at TIMESTAMP NOT NULL,
retry_count INTEGER DEFAULT 0
);)";
transaction->commands.push_back(std::move(command));

// Optimize database query for `GetNext`.
CreateTableIndex(transaction, GetTableName(), /*columns=*/{"process_at"});
}

// static
void ConfirmationQueue::MigrateToV38(
mojom::DBTransactionInfo* const transaction) {
CHECK(transaction);

// The conversion queue is deprecated since all confirmations are now being
// added to the confirmation queue.
DropTable(transaction, "conversion_queue");
}

void ConfirmationQueue::InsertOrUpdate(
mojom::DBTransactionInfo* transaction,
const ConfirmationQueueItemList& confirmation_queue_items) const {
Expand Down
3 changes: 0 additions & 3 deletions ...s/brave_ads/core/internal/account/confirmations/queue/confirmation_queue_database_table.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,6 @@ class ConfirmationQueue final : public TableInterface {
void Migrate(mojom::DBTransactionInfo* transaction, int to_version) override;

private:
void MigrateToV36(mojom::DBTransactionInfo* transaction) const;
static void MigrateToV38(mojom::DBTransactionInfo* transaction);

void InsertOrUpdate(
mojom::DBTransactionInfo* transaction,
const ConfirmationQueueItemList& confirmation_queue_items) const;
Expand Down
6 changes: 3 additions & 3 deletions components/brave_ads/core/internal/account/transactions/transaction_info.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ bool TransactionInfo::operator!=(const TransactionInfo& other) const {
}

bool TransactionInfo::IsValid() const {
return !id.empty() && !creative_instance_id.empty() &&
ad_type != AdType::kUndefined &&
confirmation_type != ConfirmationType::kUndefined && created_at;
return !id.empty() && created_at && !creative_instance_id.empty() &&
!segment.empty() && ad_type != AdType::kUndefined &&
confirmation_type != ConfirmationType::kUndefined;
}

} // namespace brave_ads
96 changes: 67 additions & 29 deletions components/brave_ads/core/internal/account/transactions/transactions_database_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,8 @@ void MigrateToV35(mojom::DBTransactionInfo* const transaction) {
CHECK(transaction);

// Optimize database query for `GetForDateRange`.
CreateTableIndex(transaction, "transactions", /*columns=*/{"created_at"});
CreateTableIndex(transaction, /*table_name=*/"transactions",
/*columns=*/{"created_at"});
}

void MigrateToV39(mojom::DBTransactionInfo* const transaction) {
Expand All @@ -294,6 +295,63 @@ void MigrateToV39(mojom::DBTransactionInfo* const transaction) {
transaction->commands.push_back(std::move(command));
}

void MigrateToV40(mojom::DBTransactionInfo* const transaction) {
CHECK(transaction);

{
// Delete legacy transactions with an undefined `creative_instance_id`,
// `segment` or `ad_type`.
mojom::DBCommandInfoPtr command = mojom::DBCommandInfo::New();
command->type = mojom::DBCommandInfo::Type::EXECUTE;
command->sql =
R"(
DELETE FROM
transactions
WHERE
creative_instance_id == ''
OR segment == ''
OR ad_type == '';)";
transaction->commands.push_back(std::move(command));
}

{
// Create a temporary table:
// - with a new `creative_instance_id` column constraint.
// - with a new `segment` column constraint.
// - with a new `reconciled_at` default value.
mojom::DBCommandInfoPtr command = mojom::DBCommandInfo::New();
command->type = mojom::DBCommandInfo::Type::EXECUTE;
command->sql =
R"(
CREATE TABLE transactions_temp (
id TEXT NOT NULL PRIMARY KEY ON CONFLICT REPLACE,
created_at TIMESTAMP NOT NULL,
creative_instance_id TEXT NOT NULL,
value DOUBLE NOT NULL,
segment TEXT NOT NULL,
ad_type TEXT NOT NULL,
confirmation_type TEXT NOT NULL,
reconciled_at TIMESTAMP DEFAULT 0
);)";
transaction->commands.push_back(std::move(command));

// Copy legacy columns to the temporary table, drop the legacy table, rename
// the temporary table and create an index.
const std::vector<std::string> columns = {
"id", "created_at", "creative_instance_id", "value",
"segment", "ad_type", "confirmation_type", "reconciled_at"};

CopyTableColumns(transaction, "transactions", "transactions_temp", columns,
/*should_drop=*/true);

RenameTable(transaction, "transactions_temp", "transactions");
}

// Optimize database query for `GetForDateRange`.
CreateTableIndex(transaction, /*table_name=*/"transactions",
/*columns=*/{"created_at"});
}

} // namespace

void Transactions::Save(const TransactionList& transactions,
Expand All @@ -309,31 +367,6 @@ void Transactions::Save(const TransactionList& transactions,
RunTransaction(std::move(transaction), std::move(callback));
}

void Transactions::GetAll(GetTransactionsCallback callback) const {
mojom::DBTransactionInfoPtr transaction = mojom::DBTransactionInfo::New();
mojom::DBCommandInfoPtr command = mojom::DBCommandInfo::New();
command->type = mojom::DBCommandInfo::Type::READ;
command->sql = base::ReplaceStringPlaceholders(
R"(
SELECT
id,
created_at,
creative_instance_id,
value,
segment,
ad_type,
confirmation_type,
reconciled_at
FROM
$1;)",
{GetTableName()}, nullptr);
BindRecords(&*command);
transaction->commands.push_back(std::move(command));

RunDBTransaction(std::move(transaction),
base::BindOnce(&GetCallback, std::move(callback)));
}

void Transactions::GetForDateRange(const base::Time from_time,
const base::Time to_time,
GetTransactionsCallback callback) const {
Expand Down Expand Up @@ -427,12 +460,12 @@ void Transactions::Create(mojom::DBTransactionInfo* const transaction) {
CREATE TABLE transactions (
id TEXT NOT NULL PRIMARY KEY ON CONFLICT REPLACE,
created_at TIMESTAMP NOT NULL,
creative_instance_id TEXT,
creative_instance_id TEXT NOT NULL,
value DOUBLE NOT NULL,
segment TEXT,
segment TEXT NOT NULL,
ad_type TEXT NOT NULL,
confirmation_type TEXT NOT NULL,
reconciled_at TIMESTAMP
reconciled_at TIMESTAMP DEFAULT 0
);)";
transaction->commands.push_back(std::move(command));

Expand Down Expand Up @@ -474,6 +507,11 @@ void Transactions::Migrate(mojom::DBTransactionInfo* transaction,
MigrateToV39(transaction);
break;
}

case 40: {
MigrateToV40(transaction);
break;
}
}
}

Expand Down
1 change: 0 additions & 1 deletion components/brave_ads/core/internal/account/transactions/transactions_database_table.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ class Transactions final : public TableInterface {
public:
void Save(const TransactionList& transactions, ResultCallback callback);

void GetAll(GetTransactionsCallback callback) const;
void GetForDateRange(base::Time from_time,
base::Time to_time,
GetTransactionsCallback callback) const;
Expand Down
15 changes: 10 additions & 5 deletions ...ents/brave_ads/core/internal/account/transactions/transactions_database_table_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ TEST_F(BraveAdsTransactionsDatabaseTableTest, SaveEmptyTransactions) {
EXPECT_CALL(callback, Run(/*success=*/true,
/*transactions=*/::testing::IsEmpty()));
const Transactions database_table;
database_table.GetAll(callback.Get());
database_table.GetForDateRange(/*from_time=*/DistantPast(),
/*to_time=*/DistantFuture(), callback.Get());
}

TEST_F(BraveAdsTransactionsDatabaseTableTest, SaveTransactions) {
Expand Down Expand Up @@ -55,7 +56,8 @@ TEST_F(BraveAdsTransactionsDatabaseTableTest, SaveTransactions) {
EXPECT_CALL(callback,
Run(/*success=*/true, ::testing::ElementsAreArray(transactions)));
const Transactions database_table;
database_table.GetAll(callback.Get());
database_table.GetForDateRange(/*from_time=*/DistantPast(),
/*to_time=*/DistantFuture(), callback.Get());
}

TEST_F(BraveAdsTransactionsDatabaseTableTest, DoNotSaveDuplicateTransactions) {
Expand All @@ -77,7 +79,8 @@ TEST_F(BraveAdsTransactionsDatabaseTableTest, DoNotSaveDuplicateTransactions) {
base::MockCallback<GetTransactionsCallback> callback;
EXPECT_CALL(callback, Run(/*success=*/true, transactions));
const Transactions database_table;
database_table.GetAll(callback.Get());
database_table.GetForDateRange(/*from_time=*/DistantPast(),
/*to_time=*/DistantFuture(), callback.Get());
}

TEST_F(BraveAdsTransactionsDatabaseTableTest, GetTransactionsForDateRange) {
Expand Down Expand Up @@ -146,7 +149,8 @@ TEST_F(BraveAdsTransactionsDatabaseTableTest, ReconcileTransactions) {
Run(/*success=*/true,
::testing::UnorderedElementsAreArray(
TransactionList{transaction_1, transaction_2})));
database_table.GetAll(callback.Get());
database_table.GetForDateRange(/*from_time=*/DistantPast(),
/*to_time=*/DistantFuture(), callback.Get());
}

TEST_F(BraveAdsTransactionsDatabaseTableTest, DeleteTransactions) {
Expand Down Expand Up @@ -178,7 +182,8 @@ TEST_F(BraveAdsTransactionsDatabaseTableTest, DeleteTransactions) {
base::MockCallback<GetTransactionsCallback> callback;
EXPECT_CALL(callback, Run(/*success=*/true,
/*transactions=*/::testing::IsEmpty()));
database_table.GetAll(callback.Get());
database_table.GetForDateRange(/*from_time=*/DistantPast(),
/*to_time=*/DistantFuture(), callback.Get());
}

TEST_F(BraveAdsTransactionsDatabaseTableTest, GetTableName) {
Expand Down
3 changes: 2 additions & 1 deletion components/brave_ads/core/internal/account/transactions/transactions_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ TEST_F(BraveAdsTransactionsTest, Add) {
base::MockCallback<database::table::GetTransactionsCallback> callback;
EXPECT_CALL(callback, Run(/*success=*/true, TransactionList{transaction}));
const database::table::Transactions database_table;
database_table.GetAll(callback.Get());
database_table.GetForDateRange(/*from_time=*/DistantPast(),
/*to_time=*/DistantFuture(), callback.Get());
}

TEST_F(BraveAdsTransactionsTest, GetForDateRange) {
Expand Down
2 changes: 1 addition & 1 deletion ...s/brave_ads/core/internal/creatives/conversions/creative_set_conversion_database_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ void MigrateToV35(mojom::DBTransactionInfo* const transaction) {
CHECK(transaction);

// Optimize database query for `GetUnexpired`.
CreateTableIndex(transaction, "creative_set_conversions",
CreateTableIndex(transaction, /*table_name=*/"creative_set_conversions",
/*columns=*/{"expire_at"});
}

Expand Down
4 changes: 2 additions & 2 deletions components/brave_ads/core/internal/legacy_migration/database/database_constants.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@

namespace brave_ads::database {

inline constexpr int kVersion = 39;
inline constexpr int kCompatibleVersion = 39;
inline constexpr int kVersion = 40;
inline constexpr int kCompatibleVersion = 40;

} // namespace brave_ads::database

Expand Down
Loading

0 comments on commit 7be1b9f

Please sign in to comment.