Avoid multi-statements for Database::Execute(...)

This CL is deprecating the use of multi-statement for Execute(...).
The code should break the query in multiple calls.

For the unittests, it's common to load a script from a file.
For these case, the recommendation is to use
ExecuteScriptForTesting(...)

Bug: 40779018
Change-Id: Ia97a129b2e89b85bb31782a7c86833a86de66675
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6196597
Reviewed-by: Anthony Vallée-Dubois <anthonyvd@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Alex Turner <alexmt@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Nan Lin <linnan@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1416222}

chrome/browser/extensions/activity_log/counting_policy.cc

@@ -110,10 +110,11 @@ constexpr base::cstring_view kTableFieldTypes[] = {"INTEGER NOT NULL DEFAULT 1",
 
 // Miscellaneous SQL commands for initializing the database; these should be
 // idempotent.
-static const char kPolicyMiscSetup[] =
+constexpr char kPolicySetupDropView[] =
     // The activitylog_uncompressed view performs string lookups for simpler
     // access to the log data.
-    "DROP VIEW IF EXISTS activitylog_uncompressed;\n"
+    "DROP VIEW IF EXISTS activitylog_uncompressed";
+constexpr char kPolicySetupCreateView[] =
     "CREATE VIEW activitylog_uncompressed AS\n"
     "SELECT count,\n"
     "    x1.value AS extension_id,\n"
@@ -133,7 +134,8 @@ static const char kPolicyMiscSetup[] =
     "    LEFT JOIN url_ids    AS x4 ON (x4.id = page_url_x)\n"
     "    LEFT JOIN string_ids AS x5 ON (x5.id = page_title_x)\n"
     "    LEFT JOIN url_ids    AS x6 ON (x6.id = arg_url_x)\n"
-    "    LEFT JOIN string_ids AS x7 ON (x7.id = other_x);\n"
+    "    LEFT JOIN string_ids AS x7 ON (x7.id = other_x)";
+constexpr char kPolicySetupCreateIndex[] =
     // An index on all fields except count and time: all the fields that aren't
     // changed when incrementing a count.  This should accelerate finding the
     // rows to update (at worst several rows will need to be checked to find
@@ -144,7 +146,7 @@ static const char kPolicyMiscSetup[] =
 
 // SQL statements to clean old, unused entries out of the string and URL id
 // tables.
-static const char kStringTableCleanup[] =
+constexpr char kStringTableCleanup[] =
     "DELETE FROM string_ids WHERE id NOT IN\n"
     "(SELECT extension_id_x FROM activitylog_compressed\n"
     "    WHERE extension_id_x IS NOT NULL\n"
@@ -156,7 +158,7 @@ static const char kStringTableCleanup[] =
     "    WHERE page_title_x IS NOT NULL\n"
     " UNION SELECT other_x FROM activitylog_compressed\n"
     "    WHERE other_x IS NOT NULL)";
-static const char kUrlTableCleanup[] =
+constexpr char kUrlTableCleanup[] =
     "DELETE FROM url_ids WHERE id NOT IN\n"
     "(SELECT page_url_x FROM activitylog_compressed\n"
     "    WHERE page_url_x IS NOT NULL\n"
@@ -198,7 +200,9 @@ bool CountingPolicy::InitDatabase(sql::Database* db) {
 
   // Create a view for easily accessing the uncompressed form of the data, and
   // any necessary indexes if needed.
-  return db->Execute(kPolicyMiscSetup);
+  return db->Execute(kPolicySetupDropView) &&
+         db->Execute(kPolicySetupCreateView) &&
+         db->Execute(kPolicySetupCreateIndex);
 }
 
 void CountingPolicy::ProcessAction(scoped_refptr<Action> action) {

chrome/browser/extensions/activity_log/database_string_table.cc

@@ -24,12 +24,12 @@ DatabaseStringTable::~DatabaseStringTable() = default;
 bool DatabaseStringTable::Initialize(sql::Database* connection) {
   if (!connection->DoesTableExist(table_.c_str())) {
     return connection->Execute(base::StrCat(
-        {"CREATE TABLE ", table_,
-         "(id INTEGER PRIMARY KEY, value TEXT NOT NULL);",
-         "CREATE UNIQUE INDEX ", table_, "_index ON ", table_, "(value)"}));
-  } else {
-    return true;
+               {"CREATE TABLE ", table_,
+                "(id INTEGER PRIMARY KEY, value TEXT NOT NULL)"})) &&
+           connection->Execute(base::StrCat({"CREATE UNIQUE INDEX ", table_,
+                                             "_index ON ", table_, "(value)"}));
   }
+  return true;
 }
 
 bool DatabaseStringTable::StringToInt(sql::Database* connection,

components/autofill/core/browser/webdata/payments/payments_autofill_table_unittest.cc

@@ -1583,7 +1583,7 @@ TEST_F(PaymentsAutofillTableTest, RemoveAllVirtualCardUsageData) {
 
 TEST_F(PaymentsAutofillTableTest, GetMaskedBankAccounts) {
   // Populate masked_bank_accounts table.
-  ASSERT_TRUE(db_->GetSQLConnection()->Execute(
+  ASSERT_TRUE(db_->GetSQLConnection()->ExecuteScriptForTesting(
       "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', "
@@ -1627,7 +1627,7 @@ 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(
+  ASSERT_TRUE(db_->GetSQLConnection()->ExecuteScriptForTesting(
       "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', "
@@ -1649,7 +1649,7 @@ TEST_F(PaymentsAutofillTableTest,
 }
 
 TEST_F(PaymentsAutofillTableTest, SetMaskedBankAccounts) {
-  ASSERT_TRUE(db_->GetSQLConnection()->Execute(
+  ASSERT_TRUE(db_->GetSQLConnection()->ExecuteScriptForTesting(
       "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', "

components/history/core/browser/visit_annotations_database.cc

@@ -1242,9 +1242,11 @@ bool VisitAnnotationsDatabase::MigrateContentAnnotationsAddSearchMetadata() {
   // Add the `search_normalized_url` and `search_terms` columns to the older
   // versions of the table.
   return GetDB().Execute(
-      "ALTER TABLE content_annotations "
-      "ADD COLUMN search_normalized_url; \n"
-      "ALTER TABLE content_annotations ADD COLUMN search_terms LONGVARCHAR");
+             "ALTER TABLE content_annotations "
+             "ADD COLUMN search_normalized_url") &&
+         GetDB().Execute(
+             "ALTER TABLE content_annotations ADD COLUMN search_terms "
+             "LONGVARCHAR");
 }
 
 bool VisitAnnotationsDatabase::MigrateContentAnnotationsAddAlternativeTitle() {

components/history/core/test/history_unittest_base.cc

@@ -37,7 +37,7 @@ void HistoryUnitTestBase::ExecuteSQLScript(const base::FilePath& sql_path,
 
   sql::Database connection(sql::test::kTestTag);
   ASSERT_TRUE(connection.Open(db_path));
-  ASSERT_TRUE(connection.Execute(sql));
+  ASSERT_TRUE(connection.ExecuteScriptForTesting(sql));
 }
 
 HistoryUnitTestBase::HistoryUnitTestBase() {

components/offline_pages/core/offline_page_metadata_store.cc

@@ -316,15 +316,20 @@ bool UpgradeFromVersion3ToVersion4(sql::Database* db,
     return false;
   }
 
-  const char kSql[] = "ALTER TABLE " OFFLINE_PAGES_TABLE_NAME
-                      " ADD COLUMN snippet VARCHAR NOT NULL DEFAULT ''; "
-                      "ALTER TABLE " OFFLINE_PAGES_TABLE_NAME
-                      " ADD COLUMN attribution VARCHAR NOT NULL DEFAULT '';";
-  if (!db->Execute(kSql)) {
+  static constexpr char kAddSnippetColumnSql[] =
+      "ALTER TABLE " OFFLINE_PAGES_TABLE_NAME
+      " ADD COLUMN snippet VARCHAR NOT NULL DEFAULT ''";
+
+  static constexpr char kAddAttributionColumnSql[] =
+      "ALTER TABLE " OFFLINE_PAGES_TABLE_NAME
+      " ADD COLUMN attribution VARCHAR NOT NULL DEFAULT ''";
+
+  if (!db->Execute(kAddSnippetColumnSql) ||
+      !db->Execute(kAddAttributionColumnSql)) {
     return false;
   }
 
-  const char kUpgradeThumbnailsTableSql[] =
+  static constexpr char kUpgradeThumbnailsTableSql[] =
       "ALTER TABLE page_thumbnails"
       " ADD COLUMN favicon BLOB NOT NULL DEFAULT x''";
   if (!db->Execute(kUpgradeThumbnailsTableSql)) {

components/omnibox/browser/in_memory_url_index_unittest.cc

@@ -227,7 +227,7 @@ void InMemoryURLIndexTest::SetUp() {
         ASSERT_TRUE(base::ReadFileToString(golden_path, &sql));
         sql::Database& db(GetDB());
         ASSERT_TRUE(db.is_open());
-        ASSERT_TRUE(db.Execute(sql));
+        ASSERT_TRUE(db.ExecuteScriptForTesting(sql));
 
         // Update [urls.last_visit_time] and [visits.visit_time] to represent a
         // time relative to 'now'.

components/webdata/common/web_database_migration_unittest.cc

@@ -165,7 +165,7 @@ void WebDatabaseMigrationTest::LoadDatabase(
 
   sql::Database connection(sql::test::kTestTag);
   ASSERT_TRUE(connection.Open(GetDatabasePath()));
-  ASSERT_TRUE(connection.Execute(contents));
+  ASSERT_TRUE(connection.ExecuteScriptForTesting(contents));
 }
 
 // Tests that migrating from the golden files version_XX.sql results in the same

content/browser/aggregation_service/aggregation_service_storage_sql_unittest.cc

@@ -1454,7 +1454,7 @@ class AggregationServiceStorageSqlMigrationsTest
     sql::Database db(sql::test::kTestTag);
     // Use `db_path()` if none is specified.
     ASSERT_TRUE(db.Open(db_path ? *db_path : this->db_path()));
-    ASSERT_TRUE(db.Execute(contents));
+    ASSERT_TRUE(db.ExecuteScriptForTesting(contents));
   }
 
   std::string GetCurrentSchema() {

content/browser/attribution_reporting/attribution_storage_sql_migrations_unittest.cc

@@ -129,7 +129,7 @@ class AttributionStorageSqlMigrationsTest : public testing::Test {
 
     sql::Database db(sql::test::kTestTag);
     ASSERT_TRUE(db.Open(db_path));
-    ASSERT_TRUE(db.Execute(contents));
+    ASSERT_TRUE(db.ExecuteScriptForTesting(contents));
   }
 
   base::ScopedTempDir temp_directory_;

sql/database.cc

@@ -1515,6 +1515,8 @@ SqliteResultCode Database::ExecuteAndReturnResultCode(
     while (base::IsAsciiWhitespace(*sql)) {
       sql++;
     }
+    // Do not use multi-statement with Execute(...).
+    CHECK(!*sql);
   }
 
   // Most calls to Execute() modify the database.  The main exceptions would be

sql/test/test_helpers.cc

@@ -295,7 +295,7 @@ bool CreateDatabaseFromSQL(const base::FilePath& db_path,
   // http://crbug.com/307303 is for exploring this test issue.
   std::ignore = db.Execute("PRAGMA auto_vacuum = 0");
 
-  return db.Execute(sql);
+  return db.ExecuteScriptForTesting(sql);
 }
 
 std::string IntegrityCheck(Database& db) {

storage/browser/quota/quota_database_migrations_unittest.cc

@@ -70,7 +70,7 @@ class QuotaDatabaseMigrationsTest : public testing::Test {
 
     sql::Database db(sql::test::kTestTag);
     if (!base::CreateDirectory(db_path.DirName()) || !db.Open(db_path) ||
-        !db.Execute(contents)) {
+        !db.ExecuteScriptForTesting(contents)) {
       return false;
     }