Skip to content

Commit

Permalink
Merge pull request brave#25137 from brave/issues/39801
Browse files Browse the repository at this point in the history
[ads] Refactor database `BindInt64` to `BindTime`, `BindTimeDelta` and `TimeToSqlValue`
  • Loading branch information
tmancey authored Aug 15, 2024
2 parents e5a9866 + d67ce8c commit ef470d6
Show file tree
Hide file tree
Showing 26 changed files with 198 additions and 307 deletions.
2 changes: 0 additions & 2 deletions components/brave_ads/core/internal/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -468,8 +468,6 @@ static_library("internal") {
"common/subdivision/url_request/subdivision_url_request_json_reader_util.h",
"common/time/time_constraint_util.cc",
"common/time/time_constraint_util.h",
"common/time/time_delta_util.cc",
"common/time/time_delta_util.h",
"common/time/time_formatting_util.cc",
"common/time/time_formatting_util.h",
"common/time/time_util.cc",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,15 @@ void BindColumnTypes(mojom::DBStatementInfo* const mojom_statement) {
mojom::DBBindColumnType::kString, // creative_instance_id
mojom::DBBindColumnType::kString, // type
mojom::DBBindColumnType::kString, // ad_type
mojom::DBBindColumnType::kInt64, // created_at
mojom::DBBindColumnType::kTime, // created_at
mojom::DBBindColumnType::kString, // token
mojom::DBBindColumnType::kString, // blinded_token
mojom::DBBindColumnType::kString, // unblinded_token
mojom::DBBindColumnType::kString, // public_key
mojom::DBBindColumnType::kString, // signature
mojom::DBBindColumnType::kString, // credential_base64url
mojom::DBBindColumnType::kString, // user_data
mojom::DBBindColumnType::kInt64, // process_at
mojom::DBBindColumnType::kTime, // process_at
mojom::DBBindColumnType::kInt // retry_count
};
}
Expand Down Expand Up @@ -101,9 +101,8 @@ size_t BindColumns(mojom::DBStatementInfo* mojom_statement,

BindColumnString(mojom_statement, index++, ToString(confirmation.ad_type));

BindColumnInt64(mojom_statement, index++,
ToChromeTimestampFromTime(
confirmation.created_at.value_or(base::Time())));
BindColumnTime(mojom_statement, index++,
confirmation.created_at.value_or(base::Time()));

if (confirmation.reward) {
BindColumnString(mojom_statement, index++,
Expand Down Expand Up @@ -140,10 +139,8 @@ size_t BindColumns(mojom::DBStatementInfo* mojom_statement,
base::JSONWriter::Write(confirmation.user_data.fixed, &user_data_json));
BindColumnString(mojom_statement, index++, user_data_json);

BindColumnInt64(
mojom_statement, index++,
ToChromeTimestampFromTime(
confirmation_queue_item.process_at.value_or(base::Time())));
BindColumnTime(mojom_statement, index++,
confirmation_queue_item.process_at.value_or(base::Time()));

BindColumnInt(mojom_statement, index++,
confirmation_queue_item.retry_count);
Expand Down Expand Up @@ -172,8 +169,7 @@ ConfirmationQueueItemInfo FromMojomRow(
confirmation_queue_item.confirmation.ad_type =
ToAdType(ColumnString(mojom_row, 3));

const base::Time created_at =
ToTimeFromChromeTimestamp(ColumnInt64(mojom_row, 4));
const base::Time created_at = ColumnTime(mojom_row, 4);
if (!created_at.is_null()) {
confirmation_queue_item.confirmation.created_at = created_at;
}
Expand Down Expand Up @@ -210,8 +206,7 @@ ConfirmationQueueItemInfo FromMojomRow(
base::JSONReader::ReadDict(ColumnString(mojom_row, 11))
.value_or(base::Value::Dict());

const base::Time process_at =
ToTimeFromChromeTimestamp(ColumnInt64(mojom_row, 12));
const base::Time process_at = ColumnTime(mojom_row, 12);
if (!process_at.is_null()) {
confirmation_queue_item.process_at = process_at;
}
Expand Down Expand Up @@ -360,7 +355,8 @@ void ConfirmationQueue::Retry(const std::string& transaction_id,
// `kMaximumRetryDelay`.
mojom::DBTransactionInfoPtr mojom_transaction =
mojom::DBTransactionInfo::New();
Execute(&*mojom_transaction, R"(
Execute(
&*mojom_transaction, R"(
UPDATE
$1
SET
Expand All @@ -374,10 +370,8 @@ void ConfirmationQueue::Retry(const std::string& transaction_id,
)
WHERE
transaction_id = '$7';)",
{GetTableName(),
base::NumberToString(ToChromeTimestampFromTime(base::Time::Now())),
retry_after, max_retry_delay, retry_after, max_retry_delay,
transaction_id});
{GetTableName(), TimeToSqlValueAsString(base::Time::Now()), retry_after,
max_retry_delay, retry_after, max_retry_delay, transaction_id});

RunTransaction(std::move(mojom_transaction), std::move(callback));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include "base/check.h"
#include "base/debug/dump_without_crashing.h"
#include "base/functional/bind.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
#include "brave/components/brave_ads/core/internal/ads_client/ads_client_util.h"
#include "brave/components/brave_ads/core/internal/common/database/database_column_util.h"
Expand All @@ -34,7 +33,7 @@ void BindColumnTypes(mojom::DBStatementInfo* const mojom_statement) {
mojom_statement->bind_column_types = {
mojom::DBBindColumnType::kString, // creative_instance_id
mojom::DBBindColumnType::kDouble, // value
mojom::DBBindColumnType::kInt64 // expire_at
mojom::DBBindColumnType::kTime // expire_at
};
}

Expand All @@ -50,9 +49,8 @@ size_t BindColumns(mojom::DBStatementInfo* mojom_statement,
BindColumnString(mojom_statement, index++,
creative_ad.creative_instance_id);
BindColumnDouble(mojom_statement, index++, creative_ad.value);
BindColumnInt64(
mojom_statement, index++,
ToChromeTimestampFromTime(creative_ad.end_at + base::Days(7)));
BindColumnTime(mojom_statement, index++,
creative_ad.end_at + base::Days(7));

++row_count;
}
Expand All @@ -67,9 +65,7 @@ void BindColumns(mojom::DBStatementInfo* const mojom_statement,

BindColumnString(mojom_statement, 0, deposit.creative_instance_id);
BindColumnDouble(mojom_statement, 1, deposit.value);
BindColumnInt64(
mojom_statement, 2,
ToChromeTimestampFromTime(deposit.expire_at.value_or(base::Time())));
BindColumnTime(mojom_statement, 2, deposit.expire_at.value_or(base::Time()));
}

DepositInfo FromMojomRow(const mojom::DBRowInfo* const mojom_row) {
Expand All @@ -79,8 +75,7 @@ DepositInfo FromMojomRow(const mojom::DBRowInfo* const mojom_row) {

deposit.creative_instance_id = ColumnString(mojom_row, 0);
deposit.value = ColumnDouble(mojom_row, 1);
const base::Time expire_at =
ToTimeFromChromeTimestamp(ColumnInt64(mojom_row, 2));
const base::Time expire_at = ColumnTime(mojom_row, 2);
if (!expire_at.is_null()) {
deposit.expire_at = expire_at;
}
Expand Down Expand Up @@ -252,8 +247,7 @@ void Deposits::PurgeExpired(ResultCallback callback) const {
$1
WHERE
$2 >= expire_at;)",
{GetTableName(),
base::NumberToString(ToChromeTimestampFromTime(base::Time::Now()))});
{GetTableName(), TimeToSqlValueAsString(base::Time::Now())});

RunTransaction(std::move(mojom_transaction), std::move(callback));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "base/check.h"
#include "base/debug/dump_without_crashing.h"
#include "base/functional/bind.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
#include "base/time/time.h"
#include "brave/components/brave_ads/core/internal/ads_client/ads_client_util.h"
Expand All @@ -35,13 +34,13 @@ void BindColumnTypes(mojom::DBStatementInfo* const mojom_statement) {

mojom_statement->bind_column_types = {
mojom::DBBindColumnType::kString, // id
mojom::DBBindColumnType::kInt64, // created_at
mojom::DBBindColumnType::kTime, // created_at
mojom::DBBindColumnType::kString, // creative_instance_id
mojom::DBBindColumnType::kDouble, // value
mojom::DBBindColumnType::kString, // segment
mojom::DBBindColumnType::kString, // ad_type
mojom::DBBindColumnType::kString, // confirmation_type
mojom::DBBindColumnType::kInt64 // reconciled_at
mojom::DBBindColumnType::kTime // reconciled_at
};
}

Expand All @@ -61,19 +60,17 @@ size_t BindColumns(mojom::DBStatementInfo* mojom_statement,
}

BindColumnString(mojom_statement, index++, transaction.id);
BindColumnInt64(mojom_statement, index++,
ToChromeTimestampFromTime(
transaction.created_at.value_or(base::Time())));
BindColumnTime(mojom_statement, index++,
transaction.created_at.value_or(base::Time()));
BindColumnString(mojom_statement, index++,
transaction.creative_instance_id);
BindColumnDouble(mojom_statement, index++, transaction.value);
BindColumnString(mojom_statement, index++, transaction.segment);
BindColumnString(mojom_statement, index++, ToString(transaction.ad_type));
BindColumnString(mojom_statement, index++,
ToString(transaction.confirmation_type));
BindColumnInt64(mojom_statement, index++,
ToChromeTimestampFromTime(
transaction.reconciled_at.value_or(base::Time())));
BindColumnTime(mojom_statement, index++,
transaction.reconciled_at.value_or(base::Time()));

++row_count;
}
Expand All @@ -87,8 +84,7 @@ TransactionInfo FromMojomRow(const mojom::DBRowInfo* const mojom_row) {
TransactionInfo transaction;

transaction.id = ColumnString(mojom_row, 0);
const base::Time created_at =
ToTimeFromChromeTimestamp(ColumnInt64(mojom_row, 1));
const base::Time created_at = ColumnTime(mojom_row, 1);
if (!created_at.is_null()) {
transaction.created_at = created_at;
}
Expand All @@ -98,8 +94,7 @@ TransactionInfo FromMojomRow(const mojom::DBRowInfo* const mojom_row) {
transaction.ad_type = ToAdType(ColumnString(mojom_row, 5));
transaction.confirmation_type =
ToConfirmationType(ColumnString(mojom_row, 6));
const base::Time reconciled_at =
ToTimeFromChromeTimestamp(ColumnInt64(mojom_row, 7));
const base::Time reconciled_at = ColumnTime(mojom_row, 7);
if (!reconciled_at.is_null()) {
transaction.reconciled_at = reconciled_at;
}
Expand Down Expand Up @@ -333,9 +328,8 @@ void Transactions::GetForDateRange(const base::Time from_time,
$1
WHERE
created_at BETWEEN $2 AND $3;)",
{GetTableName(),
base::NumberToString(ToChromeTimestampFromTime(from_time)),
base::NumberToString(ToChromeTimestampFromTime(to_time))},
{GetTableName(), TimeToSqlValueAsString(from_time),
TimeToSqlValueAsString(to_time)},
nullptr);
BindColumnTypes(&*mojom_statement);
mojom_transaction->statements.push_back(std::move(mojom_statement));
Expand Down Expand Up @@ -367,8 +361,7 @@ void Transactions::Reconcile(const PaymentTokenList& payment_tokens,
id IN $3
OR creative_instance_id IN $4
);)",
{GetTableName(),
base::NumberToString(ToChromeTimestampFromTime(base::Time::Now())),
{GetTableName(), TimeToSqlValueAsString(base::Time::Now()),
BuildBindColumnPlaceholder(
/*column_count=*/transaction_ids.size()),
BuildBindColumnPlaceholder(/*column_count=*/1)},
Expand All @@ -393,16 +386,9 @@ void Transactions::PurgeExpired(ResultCallback callback) const {
$1
WHERE
reconciled_at != 0
AND DATETIME(
(created_at / 1000000) - 11644473600,
'unixepoch'
) <= DATETIME(
($2 / 1000000) - 11644473600,
'unixepoch',
'-3 months'
);)",
AND created_at <= $2;)",
{GetTableName(),
base::NumberToString(ToChromeTimestampFromTime(base::Time::Now()))});
TimeToSqlValueAsString(base::Time::Now() - base::Days(90))});

RunTransaction(std::move(mojom_transaction), std::move(callback));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#include "brave/components/brave_ads/core/internal/account/transactions/transactions_test_util.h"
#include "brave/components/brave_ads/core/internal/common/test/test_base.h"
#include "brave/components/brave_ads/core/internal/common/test/time_test_util.h"
#include "brave/components/brave_ads/core/internal/common/time/time_delta_util.h"
#include "brave/components/brave_ads/core/public/account/confirmations/confirmation_type.h"
#include "brave/components/brave_ads/core/public/ad_units/ad_type.h"
#include "brave/components/brave_ads/core/public/ads_client/ads_client_callback.h"
Expand Down Expand Up @@ -180,7 +179,7 @@ TEST_F(BraveAdsTransactionsDatabaseTableTest, PurgeExpired) {
/*should_generate_random_uuids=*/true);
transactions.push_back(transaction_1);

AdvanceClockBy(Months(3));
AdvanceClockBy(base::Days(90));

const TransactionInfo transaction_2 = test::BuildUnreconciledTransaction(
/*value=*/0.03, AdType::kNotificationAd, ConfirmationType::kClicked,
Expand Down Expand Up @@ -227,7 +226,7 @@ TEST_F(BraveAdsTransactionsDatabaseTableTest,
/*should_generate_random_uuids=*/true);
transactions.push_back(transaction_1);

AdvanceClockBy(Months(3) - base::Milliseconds(1));
AdvanceClockBy(base::Days(90) - base::Milliseconds(1));

const TransactionInfo transaction_2 = test::BuildTransaction(
/*value=*/0.01, AdType::kNotificationAd, ConfirmationType::kClicked,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ void BindColumn(sql::Statement* const statement,
mojom_bind_column.value_union->get_int_value());
break;
}

case mojom::DBColumnValueUnion::Tag::kInt64Value: {
statement->BindInt64(mojom_bind_column.index,
mojom_bind_column.value_union->get_int64_value());
Expand All @@ -73,6 +74,19 @@ void BindColumn(sql::Statement* const statement,
mojom_bind_column.value_union->get_string_value());
break;
}

case mojom::DBColumnValueUnion::Tag::kTimeValue: {
statement->BindTime(mojom_bind_column.index,
mojom_bind_column.value_union->get_time_value());
break;
}

case mojom::DBColumnValueUnion::Tag::kTimeDeltaValue: {
statement->BindTimeDelta(
mojom_bind_column.index,
mojom_bind_column.value_union->get_time_delta_value());
break;
}
}
}

Expand Down Expand Up @@ -189,4 +203,50 @@ std::string ColumnString(const mojom::DBRowInfo* const mojom_row,
return mojom_row->column_values_union.at(column)->get_string_value();
}

void BindColumnTime(mojom::DBStatementInfo* const mojom_statement,
const int32_t index,
const base::Time value) {
CHECK(mojom_statement);

mojom::DBBindColumnInfoPtr mojom_bind_column = mojom::DBBindColumnInfo::New();
mojom_bind_column->index = index;
mojom_bind_column->value_union =
mojom::DBColumnValueUnion::NewTimeValue(value);

mojom_statement->bind_columns.push_back(std::move(mojom_bind_column));
}

base::Time ColumnTime(const mojom::DBRowInfo* const mojom_row,
const size_t column) {
CHECK(mojom_row);
CHECK_LT(column, mojom_row->column_values_union.size());
CHECK_EQ(mojom::DBColumnValueUnion::Tag::kTimeValue,
mojom_row->column_values_union.at(column)->which());

return mojom_row->column_values_union.at(column)->get_time_value();
}

void BindColumnTimeDelta(mojom::DBStatementInfo* const mojom_statement,
const int32_t index,
const base::TimeDelta value) {
CHECK(mojom_statement);

mojom::DBBindColumnInfoPtr mojom_bind_column = mojom::DBBindColumnInfo::New();
mojom_bind_column->index = index;
mojom_bind_column->value_union =
mojom::DBColumnValueUnion::NewTimeDeltaValue(value);

mojom_statement->bind_columns.push_back(std::move(mojom_bind_column));
}

base::TimeDelta ColumnTimeDelta(const mojom::DBRowInfo* const mojom_row,
const size_t column) {
CHECK(mojom_row);
CHECK_LT(column, mojom_row->column_values_union.size());
CHECK_EQ(mojom::DBColumnValueUnion::Tag::kTimeDeltaValue,
mojom_row->column_values_union.at(column)->which());

return mojom_row->column_values_union.at(column)->get_time_delta_value();
}

} // namespace brave_ads::database
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,18 @@ void BindColumnString(mojom::DBStatementInfo* mojom_statement,
[[nodiscard]] std::string ColumnString(const mojom::DBRowInfo* mojom_row,
size_t column);

void BindColumnTime(mojom::DBStatementInfo* mojom_statement,
int32_t index,
base::Time value);
[[nodiscard]] base::Time ColumnTime(const mojom::DBRowInfo* mojom_row,
size_t column);

void BindColumnTimeDelta(mojom::DBStatementInfo* mojom_statement,
int32_t index,
base::TimeDelta value);
[[nodiscard]] base::TimeDelta ColumnTimeDelta(const mojom::DBRowInfo* mojom_row,
size_t column);

} // namespace brave_ads::database

#endif // BRAVE_COMPONENTS_BRAVE_ADS_CORE_INTERNAL_COMMON_DATABASE_DATABASE_COLUMN_UTIL_H_
Loading

0 comments on commit ef470d6

Please sign in to comment.