Replace autofill table methods add/update/remove with set/get
On receiving an update, Chrome Sync rewrites the entire bank accounts table, thus we no longer need the individual add/update/remove methods
and instead we just need the set/get methods.
Bug: 1472125
Change-Id: I9a1c0a424d96795f44af46bb47d25f076be42b90
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5252088
Commit-Queue: Siddharth Shah <siashah@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Olivia Saul <jsaul@google.com>
Cr-Commit-Position: refs/heads/main@{#1258302}
diff --git a/components/autofill/core/browser/webdata/payments/payments_autofill_table.cc b/components/autofill/core/browser/webdata/payments/payments_autofill_table.cc
index e6eef00..5b28049 100644
--- a/components/autofill/core/browser/webdata/payments/payments_autofill_table.cc
+++ b/components/autofill/core/browser/webdata/payments/payments_autofill_table.cc
@@ -533,94 +533,58 @@
return true;
}
-std::unique_ptr<BankAccount> PaymentsAutofillTable::GetMaskedBankAccount(
- int64_t instrument_id) {
- sql::Statement s;
- SelectBuilder(db_, s, kMaskedBankAccountsTable,
- {kBankName, kAccountNumberSuffix, kAccountType, kNickname,
- kDisplayIconUrl},
- "WHERE instrument_id = ?");
- s.BindInt64(0, instrument_id);
-
- if (!s.Step()) {
- return nullptr;
- }
- int index = 0;
- auto bank_name = s.ColumnString16(index++);
- auto account_number_suffix = s.ColumnString16(index++);
- int account_type = s.ColumnInt(index++);
- auto nickname = s.ColumnString16(index++);
- auto display_icon_url = s.ColumnString16(index++);
- if (account_type >
- static_cast<int>(BankAccount::AccountType::kTransactingAccount) ||
- account_type < static_cast<int>(BankAccount::AccountType::kUnknown)) {
- return nullptr;
- }
- return std::make_unique<BankAccount>(
- instrument_id, nickname, GURL(display_icon_url), bank_name,
- account_number_suffix,
- static_cast<BankAccount::AccountType>(account_type));
-}
-
-bool PaymentsAutofillTable::AddMaskedBankAccount(
- const BankAccount& bank_account) {
+bool PaymentsAutofillTable::SetMaskedBankAccounts(
+ const std::vector<BankAccount>& bank_accounts) {
sql::Transaction transaction(db_);
if (!transaction.Begin()) {
return false;
}
- // Add bank account.
+ // Deletes all old values.
+ Delete(db_, kMaskedBankAccountsTable);
+
+ // Add bank accounts.
sql::Statement insert;
InsertBuilder(db_, insert, kMaskedBankAccountsTable,
{kInstrumentId, kBankName, kAccountNumberSuffix, kAccountType,
kNickname, kDisplayIconUrl});
- BindMaskedBankAccountToStatement(bank_account, &insert);
- if (!insert.Run()) {
- return false;
+ for (BankAccount bank_account : bank_accounts) {
+ BindMaskedBankAccountToStatement(bank_account, &insert);
+ if (!insert.Run()) {
+ return false;
+ }
+ insert.Reset(/*clear_bound_vars=*/true);
}
-
return transaction.Commit();
}
-bool PaymentsAutofillTable::UpdateMaskedBankAccount(
- const BankAccount& bank_account) {
- sql::Transaction transaction(db_);
- if (!transaction.Begin()) {
- return false;
- }
+bool PaymentsAutofillTable::GetMaskedBankAccounts(
+ std::vector<std::unique_ptr<BankAccount>>& bank_accounts) {
+ sql::Statement s;
+ bank_accounts.clear();
- // Update bank account.
- sql::Statement update;
- UpdateBuilder(db_, update, kMaskedBankAccountsTable,
+ SelectBuilder(db_, s, kMaskedBankAccountsTable,
{kInstrumentId, kBankName, kAccountNumberSuffix, kAccountType,
- kNickname, kDisplayIconUrl},
- base::StrCat({kInstrumentId, "=?1"}));
- BindMaskedBankAccountToStatement(bank_account, &update);
- if (!update.Run()) {
- return false;
+ kNickname, kDisplayIconUrl});
+ while (s.Step()) {
+ int index = 0;
+ auto instrument_id = s.ColumnInt64(index++);
+ auto bank_name = s.ColumnString16(index++);
+ auto account_number_suffix = s.ColumnString16(index++);
+ int account_type = s.ColumnInt(index++);
+ auto nickname = s.ColumnString16(index++);
+ auto display_icon_url = s.ColumnString16(index++);
+ if (account_type >
+ static_cast<int>(BankAccount::AccountType::kTransactingAccount) ||
+ account_type < static_cast<int>(BankAccount::AccountType::kUnknown)) {
+ continue;
+ }
+ bank_accounts.push_back(std::make_unique<BankAccount>(
+ instrument_id, nickname, GURL(display_icon_url), bank_name,
+ account_number_suffix,
+ static_cast<BankAccount::AccountType>(account_type)));
}
-
- return transaction.Commit();
-}
-
-bool PaymentsAutofillTable::RemoveMaskedBankAccount(
- const BankAccount& bank_account) {
- sql::Transaction transaction(db_);
- if (!transaction.Begin()) {
- return false;
- }
-
- sql::Statement bank_accounts_delete;
- DeleteBuilder(db_, bank_accounts_delete, kMaskedBankAccountsTable,
- base::StrCat({kInstrumentId, "=?"}));
- bank_accounts_delete.BindInt64(
- 0, bank_account.payment_instrument().instrument_id());
- if (!bank_accounts_delete.Run()) {
- return false;
- }
- // TODO(crbug.com/1475426) : Delete row from `masked_bank_accounts_metadata`
- // table.
- return transaction.Commit();
+ return s.Succeeded();
}
bool PaymentsAutofillTable::AddLocalIban(const Iban& iban) {
diff --git a/components/autofill/core/browser/webdata/payments/payments_autofill_table.h b/components/autofill/core/browser/webdata/payments/payments_autofill_table.h
index 3fc71b1..0b269252 100644
--- a/components/autofill/core/browser/webdata/payments/payments_autofill_table.h
+++ b/components/autofill/core/browser/webdata/payments/payments_autofill_table.h
@@ -352,16 +352,12 @@
bool CreateTablesIfNecessary() override;
bool MigrateToVersion(int version, bool* update_compatible_version) override;
- // Records a single BankAccount in the bank accounts table. Returns true if
- // the BankAccount was successfully added to the database.
- bool AddMaskedBankAccount(const BankAccount& bank_account);
- // Returns true if the BankAccount was successfully updated in the database.
- bool UpdateMaskedBankAccount(const BankAccount& bank_account);
- // Delete the bank account from the database.
- bool RemoveMaskedBankAccount(const BankAccount& bank_account);
- // Retrieve the data from the `masked_bank_accounts` table and return a
- // BankAccount object.
- std::unique_ptr<BankAccount> GetMaskedBankAccount(int64_t instrument_id);
+ // Rewrites the bank accounts table. Returns true if all bank accounts were
+ // successfully added to the database.
+ bool SetMaskedBankAccounts(const std::vector<BankAccount>& bank_accounts);
+ // Retrieve all bank accounts from the database.
+ bool GetMaskedBankAccounts(
+ std::vector<std::unique_ptr<BankAccount>>& bank_accounts);
// Records a single IBAN in the local_ibans table.
bool AddLocalIban(const Iban& iban);
diff --git a/components/autofill/core/browser/webdata/payments/payments_autofill_table_unittest.cc b/components/autofill/core/browser/webdata/payments/payments_autofill_table_unittest.cc
index c3e728cd..85c39dc6 100644
--- a/components/autofill/core/browser/webdata/payments/payments_autofill_table_unittest.cc
+++ b/components/autofill/core/browser/webdata/payments/payments_autofill_table_unittest.cc
@@ -1590,89 +1590,103 @@
EXPECT_TRUE(usage_data.empty());
}
-TEST_F(PaymentsAutofillTableTest, AddMaskedBankAccount) {
- BankAccount bank_account_to_store_1 = test::CreatePixBankAccount(100);
- BankAccount bank_account_to_store_2 = test::CreatePixBankAccount(200);
- table_.get()->AddMaskedBankAccount(bank_account_to_store_1);
- table_.get()->AddMaskedBankAccount(bank_account_to_store_2);
+TEST_F(PaymentsAutofillTableTest, GetMaskedBankAccounts) {
+ // Populate masked_bank_accounts table.
+ ASSERT_TRUE(db_->GetSQLConnection()->Execute(
+ "INSERT INTO masked_bank_accounts (instrument_id, bank_name, "
+ "account_number_suffix, account_type, nickname, display_icon_url) "
+ "VALUES(100, 'bank_name', 'account_number_suffix', 1, 'nickname', "
+ "'http://display-icon-url.com');"
+ "INSERT INTO masked_bank_accounts (instrument_id, bank_name, "
+ "account_number_suffix, account_type, nickname, display_icon_url) "
+ "VALUES(200, 'bank_name_2', 'account_number_suffix_2', 3, 'nickname_2', "
+ "'http://display-icon-url2.com');"));
- // Verify bank account 1 is correctly retrieved.
- std::unique_ptr<BankAccount> bank_account_from_db_1 =
- table_->GetMaskedBankAccount(
- bank_account_to_store_1.payment_instrument().instrument_id());
+ std::vector<std::unique_ptr<BankAccount>> bank_accounts_from_db;
+ table_->GetMaskedBankAccounts(bank_accounts_from_db);
- ASSERT_TRUE(bank_account_from_db_1);
- EXPECT_EQ(bank_account_to_store_1, *bank_account_from_db_1);
+ EXPECT_EQ(2u, bank_accounts_from_db.size());
- // Verify bank account 2 is correctly retrieved.
- std::unique_ptr<BankAccount> bank_account_from_db_2 =
- table_->GetMaskedBankAccount(
- bank_account_to_store_2.payment_instrument().instrument_id());
-
- ASSERT_TRUE(bank_account_from_db_2);
- EXPECT_EQ(bank_account_to_store_2, *bank_account_from_db_2);
-}
-
-TEST_F(PaymentsAutofillTableTest, UpdateMaskedBankAccount) {
- BankAccount bank_account_to_store = test::CreatePixBankAccount(100);
- ASSERT_TRUE(table_.get()->AddMaskedBankAccount(bank_account_to_store));
-
- BankAccount updated_bank_account_to_store(
- 100, u"updated_nickname", GURL("http://www.updated-example.com"),
- u"updated_bank_name", u"updated_account_number_suffix",
- BankAccount::AccountType::kSalary);
- ASSERT_TRUE(
- table_.get()->UpdateMaskedBankAccount(updated_bank_account_to_store));
-
- std::unique_ptr<BankAccount> bank_account_from_db =
- table_->GetMaskedBankAccount(
- bank_account_to_store.payment_instrument().instrument_id());
-
- ASSERT_TRUE(bank_account_from_db);
- EXPECT_EQ(updated_bank_account_to_store, *bank_account_from_db);
-}
-
-TEST_F(PaymentsAutofillTableTest, RemoveMaskedBankAccount) {
- BankAccount bank_account_to_store = test::CreatePixBankAccount(100);
- ASSERT_TRUE(table_.get()->AddMaskedBankAccount(bank_account_to_store));
-
- // Remove bank account from db.
- ASSERT_TRUE(table_.get()->RemoveMaskedBankAccount(bank_account_to_store));
-
- // Verify row is deleted from bank_accounts table.
- sql::Statement bank_accounts_select(
- db_->GetSQLConnection()->GetUniqueStatement(
- "SELECT COUNT(*) "
- "FROM masked_bank_accounts WHERE instrument_id = ?"));
- bank_accounts_select.BindInt64(
- 0, bank_account_to_store.payment_instrument().instrument_id());
- EXPECT_TRUE(bank_accounts_select.Step());
- EXPECT_EQ(0, bank_accounts_select.ColumnInt(0));
-}
-
-TEST_F(PaymentsAutofillTableTest, GetPaymentInstrument) {
- sql::Statement bank_accounts_insert(
- db_->GetSQLConnection()->GetUniqueStatement(
- "INSERT INTO masked_bank_accounts (instrument_id, bank_name, "
- "account_number_suffix, account_type, nickname, display_icon_url) "
- "VALUES(100, 'bank_name', "
- "'account_number_suffix', 1, 'nickname', "
- "'http://display-icon-url.com')"));
- EXPECT_TRUE(bank_accounts_insert.Run());
-
- std::unique_ptr<BankAccount> bank_account_from_db =
- table_->GetMaskedBankAccount(100);
-
- ASSERT_TRUE(bank_account_from_db);
- EXPECT_EQ(100, bank_account_from_db->payment_instrument().instrument_id());
- EXPECT_EQ(u"nickname", bank_account_from_db->payment_instrument().nickname());
- EXPECT_EQ(static_cast<BankAccount::AccountType>(1),
- bank_account_from_db->account_type());
+ BankAccount bank_account_from_db_1 = *bank_accounts_from_db.at(0).get();
+ EXPECT_EQ(100, bank_account_from_db_1.payment_instrument().instrument_id());
+ EXPECT_EQ(u"bank_name", bank_account_from_db_1.bank_name());
EXPECT_EQ(u"account_number_suffix",
- bank_account_from_db->account_number_suffix());
+ bank_account_from_db_1.account_number_suffix());
+ EXPECT_EQ(static_cast<BankAccount::AccountType>(1),
+ bank_account_from_db_1.account_type());
+ EXPECT_EQ(u"nickname",
+ bank_account_from_db_1.payment_instrument().nickname());
EXPECT_EQ(GURL("http://display-icon-url.com"),
- bank_account_from_db->payment_instrument().display_icon_url());
- EXPECT_EQ(u"bank_name", bank_account_from_db->bank_name());
+ bank_account_from_db_1.payment_instrument().display_icon_url());
+
+ BankAccount bank_account_from_db_2 = *bank_accounts_from_db.at(1).get();
+ EXPECT_EQ(200, bank_account_from_db_2.payment_instrument().instrument_id());
+ EXPECT_EQ(u"bank_name_2", bank_account_from_db_2.bank_name());
+ EXPECT_EQ(u"account_number_suffix_2",
+ bank_account_from_db_2.account_number_suffix());
+ EXPECT_EQ(static_cast<BankAccount::AccountType>(3),
+ bank_account_from_db_2.account_type());
+ EXPECT_EQ(u"nickname_2",
+ bank_account_from_db_2.payment_instrument().nickname());
+ EXPECT_EQ(GURL("http://display-icon-url2.com"),
+ bank_account_from_db_2.payment_instrument().display_icon_url());
+}
+
+TEST_F(PaymentsAutofillTableTest,
+ GetMaskedBankAccounts_BankAccountTypeOutOfBounds) {
+ // Populate masked_bank_accounts table with the first row to have an invalid
+ // bank account type with value 100.
+ ASSERT_TRUE(db_->GetSQLConnection()->Execute(
+ "INSERT INTO masked_bank_accounts (instrument_id, bank_name, "
+ "account_number_suffix, account_type, nickname, display_icon_url) "
+ "VALUES(100, 'bank_name', 'account_number_suffix', 100, 'nickname', "
+ "'http://display-icon-url.com');"
+ "INSERT INTO masked_bank_accounts (instrument_id, bank_name, "
+ "account_number_suffix, account_type, nickname, display_icon_url) "
+ "VALUES(200, 'bank_name_2', 'account_number_suffix_2', 3, 'nickname_2', "
+ "'http://display-icon-url2.com');"));
+
+ std::vector<std::unique_ptr<BankAccount>> bank_accounts_from_db;
+ table_->GetMaskedBankAccounts(bank_accounts_from_db);
+
+ // Expect only one bank account since the other one has an invalid bank
+ // account type.
+ EXPECT_EQ(1u, bank_accounts_from_db.size());
+ // Verify that the returned bank account maps to the second row in the table.
+ BankAccount bank_account_from_db = *bank_accounts_from_db.at(0).get();
+ EXPECT_EQ(200, bank_account_from_db.payment_instrument().instrument_id());
+}
+
+TEST_F(PaymentsAutofillTableTest, SetMaskedBankAccounts) {
+ ASSERT_TRUE(db_->GetSQLConnection()->Execute(
+ "INSERT INTO masked_bank_accounts (instrument_id, bank_name, "
+ "account_number_suffix, account_type, nickname, display_icon_url) "
+ "VALUES(100, 'bank_name', 'account_number_suffix', 1, 'nickname', "
+ "'http://display-icon-url.com');"
+ "INSERT INTO masked_bank_accounts (instrument_id, bank_name, "
+ "account_number_suffix, account_type, nickname, display_icon_url) "
+ "VALUES(200, 'bank_name_2', 'account_number_suffix_2', 3, 'nickname_2', "
+ "'http://display-icon-url2.com');"));
+
+ // Verify that GetMaskedBankAccounts returns 2 bank accounts.
+ std::vector<std::unique_ptr<BankAccount>> bank_accounts_from_db;
+ table_->GetMaskedBankAccounts(bank_accounts_from_db);
+ EXPECT_EQ(2u, bank_accounts_from_db.size());
+
+ // Create bank account with different id from the ones above.
+ BankAccount bank_account_to_store = test::CreatePixBankAccount(8000);
+ std::vector<BankAccount> bank_accounts_to_store;
+ bank_accounts_to_store.push_back(bank_account_to_store);
+ table_->SetMaskedBankAccounts(bank_accounts_to_store);
+
+ // Verify that GetMaskedBankAccounts returns 1 bank account.
+ table_->GetMaskedBankAccounts(bank_accounts_from_db);
+ EXPECT_EQ(1u, bank_accounts_from_db.size());
+
+ // Verify that the instrument id of the returned bank account matches the one
+ // that was stored.
+ BankAccount bank_account_from_db = *bank_accounts_from_db.at(0).get();
+ EXPECT_EQ(8000, bank_account_from_db.payment_instrument().instrument_id());
}
} // namespace autofill
