Remote settings: remove the fetched column

This simple change is a good way to introduce the sql_support crate and
start the migration system.

Made a couple other fixes along the way:
 - Don't make the CLI `main` method async, that causes panics when
   running the `get` or `sync` commands since those will create a second
   runtime within the async runtime.
 - Added `remote-settings-data`, which is generated by the CLI, to .gitignore
 - Fixed sql_support testing bug for schemas with no indexes.

.gitignore

@@ -73,6 +73,9 @@ fml.py
 *.dylib
 *.so
 
+# Generated by remote-settings-cli
+remote-settings-data
+
 # Build static website files
 automation/swift-components-docs/.build
 automation/swift-components-docs/docs

components/remote_settings/Cargo.toml

@@ -22,6 +22,7 @@ serde = { version = "1", features=["derive"] }
 serde_json = "1"
 parking_lot = "0.12"
 error-support = { path = "../support/error" }
+sql-support = { path = "../support/sql" }
 viaduct = { path = "../viaduct" }
 url = "2"
 camino = "1.0"

components/remote_settings/src/error.rs

@@ -25,6 +25,8 @@ pub enum RemoteSettingsError {
 /// Internal error class, this is what we use inside this crate
 #[derive(Debug, thiserror::Error)]
 pub enum Error {
+    #[error("Error opening database: {0}")]
+    OpenDatabase(#[from] sql_support::open_database::Error),
     #[error("JSON Error: {0}")]
     JSONError(#[from] serde_json::Error),
     #[error("Error writing downloaded attachment: {0}")]

components/remote_settings/src/lib.rs

@@ -13,6 +13,7 @@ pub mod cache;
 pub mod client;
 pub mod config;
 pub mod error;
+pub mod schema;
 pub mod service;
 pub mod storage;
 

components/remote_settings/src/schema.rs

@@ -0,0 +1,105 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+use rusqlite::{Connection, Transaction};
+use sql_support::open_database::{self, ConnectionInitializer};
+
+/// The current gatabase schema version.
+///
+/// For any changes to the schema [`SQL`], please make sure to:
+///
+///  1. Bump this version.
+///  2. Add a migration from the old version to the new version in
+///     [`RemoteSettingsConnectionInitializer::upgrade_from`].
+pub const VERSION: u32 = 1;
+
+/// The current remote settings database schema.
+pub const SQL: &str = r#"
+CREATE TABLE IF NOT EXISTS records (
+    id TEXT PRIMARY KEY,
+    collection_url TEXT NOT NULL,
+    data BLOB NOT NULL);
+CREATE TABLE IF NOT EXISTS attachments (
+    id TEXT PRIMARY KEY,
+    collection_url TEXT NOT NULL,
+    data BLOB NOT NULL);
+CREATE TABLE IF NOT EXISTS collection_metadata (
+    collection_url TEXT PRIMARY KEY,
+    last_modified INTEGER);
+"#;
+
+/// Initializes an SQLite connection to the Remote Settings database, performing
+/// migrations as needed.
+#[derive(Default)]
+pub struct RemoteSettingsConnectionInitializer;
+
+impl ConnectionInitializer for RemoteSettingsConnectionInitializer {
+    const NAME: &'static str = "remote_settings";
+    const END_VERSION: u32 = 1;
+
+    fn prepare(&self, conn: &Connection, _db_empty: bool) -> open_database::Result<()> {
+        let initial_pragmas = "
+            -- Use in-memory storage for TEMP tables.
+            PRAGMA temp_store = 2;
+            PRAGMA journal_mode = WAL;
+        ";
+        conn.execute_batch(initial_pragmas)?;
+        sql_support::debug_tools::define_debug_functions(conn)?;
+
+        Ok(())
+    }
+
+    fn init(&self, db: &Transaction<'_>) -> open_database::Result<()> {
+        db.execute_batch(SQL)?;
+        Ok(())
+    }
+
+    fn upgrade_from(&self, tx: &Transaction<'_>, version: u32) -> open_database::Result<()> {
+        match version {
+            // Upgrade from a database created before this crate used sql-support.
+            0 => {
+                tx.execute("ALTER TABLE collection_metadata DROP column fetched", ())?;
+                Ok(())
+            }
+            _ => Err(open_database::Error::IncompatibleVersion(version)),
+        }
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use sql_support::open_database::test_utils::MigratedDatabaseFile;
+
+    // Snapshot of the v0 schema.  We use this to test that we can migrate from there to the
+    // current schema.
+    const V0_SCHEMA: &str = r#"
+CREATE TABLE IF NOT EXISTS records (
+    id TEXT PRIMARY KEY,
+    collection_url TEXT NOT NULL,
+    data BLOB NOT NULL);
+CREATE TABLE IF NOT EXISTS attachments (
+    id TEXT PRIMARY KEY,
+    collection_url TEXT NOT NULL,
+    data BLOB NOT NULL);
+CREATE TABLE IF NOT EXISTS collection_metadata (
+    collection_url TEXT PRIMARY KEY,
+    last_modified INTEGER,
+    fetched BOOLEAN);
+PRAGMA user_version=0;
+"#;
+
+    /// Test running all schema upgrades from V16, which was the first schema with a "real"
+    /// migration.
+    ///
+    /// If an upgrade fails, then this test will fail with a panic.
+    #[test]
+    fn test_all_upgrades() {
+        let db_file =
+            MigratedDatabaseFile::new(RemoteSettingsConnectionInitializer, V0_SCHEMA);
+        db_file.run_all_upgrades();
+        db_file.assert_schema_matches_new_database();
+    }
+}

components/remote_settings/src/storage.rs

@@ -2,12 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use crate::{Attachment, RemoteSettingsRecord, Result};
 use camino::Utf8PathBuf;
-use rusqlite::{params, Connection, OptionalExtension, Transaction};
+use rusqlite::{params, Connection, OpenFlags, OptionalExtension, Transaction};
 use serde_json;
 use sha2::{Digest, Sha256};
 
+use sql_support::{open_database::open_database_with_flags, ConnExt};
+
+use crate::{
+    schema::RemoteSettingsConnectionInitializer, Attachment, RemoteSettingsRecord, Result,
+};
+
 /// Internal storage type
 ///
 /// This will store downloaded records/attachments in a SQLite database.  Nothing is implemented
@@ -27,35 +32,12 @@ pub struct Storage {
 
 impl Storage {
     pub fn new(path: Utf8PathBuf) -> Result<Self> {
-        let conn = Connection::open(path)?;
-        let storage = Self { conn };
-        storage.initialize_database()?;
-
-        Ok(storage)
-    }
-
-    // Create the different tables for records and attachements for every new sqlite path
-    fn initialize_database(&self) -> Result<()> {
-        self.conn.execute_batch(
-            "
-        CREATE TABLE IF NOT EXISTS records (
-            id TEXT PRIMARY KEY,
-            collection_url TEXT NOT NULL,
-            data BLOB NOT NULL
-        );
-       CREATE TABLE IF NOT EXISTS attachments (
-            id TEXT PRIMARY KEY,
-            collection_url TEXT NOT NULL,
-            data BLOB NOT NULL
-        );
-        CREATE TABLE IF NOT EXISTS collection_metadata (
-            collection_url TEXT PRIMARY KEY,
-            last_modified INTEGER,
-            fetched BOOLEAN
-        );
-        ",
+        let conn = open_database_with_flags(
+            path,
+            OpenFlags::default(),
+            &RemoteSettingsConnectionInitializer,
         )?;
-        Ok(())
+        Ok(Self { conn })
     }
 
     /// Get the last modified timestamp for the stored records
@@ -82,23 +64,21 @@ impl Storage {
     ) -> Result<Option<Vec<RemoteSettingsRecord>>> {
         let tx = self.conn.transaction()?;
 
-        let fetched: Option<bool> = tx
-            .prepare("SELECT fetched FROM collection_metadata WHERE collection_url = ?")?
-            .query_row(params![collection_url], |row| row.get(0))
-            .optional()?;
-
-        let result = match fetched {
-            Some(true) => {
-                // If fetched before, get the records from the records table
-                let records: Vec<RemoteSettingsRecord> = tx
-                    .prepare("SELECT data FROM records WHERE collection_url = ?")?
-                    .query_map(params![collection_url], |row| row.get::<_, Vec<u8>>(0))?
-                    .map(|data| serde_json::from_slice(&data.unwrap()).unwrap())
-                    .collect();
-
-                Ok(Some(records))
-            }
-            _ => Ok(None),
+        let fetched = tx.exists(
+            "SELECT 1 FROM collection_metadata WHERE collection_url = ?",
+            (collection_url,),
+        )?;
+        let result = if fetched {
+            // If fetched before, get the records from the records table
+            let records: Vec<RemoteSettingsRecord> = tx
+                .prepare("SELECT data FROM records WHERE collection_url = ?")?
+                .query_map(params![collection_url], |row| row.get::<_, Vec<u8>>(0))?
+                .map(|data| serde_json::from_slice(&data.unwrap()).unwrap())
+                .collect();
+
+            Ok(Some(records))
+        } else {
+            Ok(None)
         };
 
         tx.commit()?;
@@ -220,10 +200,9 @@ impl Storage {
         last_modified: u64,
     ) -> Result<()> {
         // Update the metadata
-        let fetched = true;
         tx.execute(
-            "INSERT OR REPLACE INTO collection_metadata (collection_url, last_modified, fetched) VALUES (?, ?, ?)",
-            (collection_url, last_modified, fetched),
+            "INSERT OR REPLACE INTO collection_metadata (collection_url, last_modified) VALUES (?, ?)",
+            (collection_url, last_modified),
         )?;
         Ok(())
     }

components/support/sql/src/open_database.rs

@@ -442,11 +442,11 @@ pub mod test_utils {
         }
     }
 
-    fn get_sql(conn: &Connection, type_: &str) -> HashMap<String, String> {
+    fn get_sql(conn: &Connection, type_: &str) -> HashMap<String, Option<String>> {
         conn.query_rows_and_then(
             "SELECT name, sql FROM sqlite_master WHERE type=?",
             (type_,),
-            |row| -> rusqlite::Result<(String, String)> { Ok((row.get(0)?, row.get(1)?)) },
+            |row| -> rusqlite::Result<(String, Option<String>)> { Ok((row.get(0)?, row.get(1)?)) },
         )
         .unwrap()
         .into_iter()
@@ -455,8 +455,8 @@ pub mod test_utils {
 
     fn compare_sql_maps(
         type_: &str,
-        old_items: HashMap<String, String>,
-        new_items: HashMap<String, String>,
+        old_items: HashMap<String, Option<String>>,
+        new_items: HashMap<String, Option<String>>,
     ) {
         let old_db_keys: HashSet<&String> = old_items.keys().collect();
         let new_db_keys: HashSet<&String> = new_items.keys().collect();

examples/remote-settings-cli/src/main.rs

@@ -73,8 +73,7 @@ enum Commands {
     },
 }
 
-#[tokio::main]
-async fn main() -> Result<()> {
+fn main() -> Result<()> {
     let cli = Cli::parse();
     env_logger::init_from_env(env_logger::Env::default().filter_or(
         "RUST_LOG",
@@ -94,15 +93,17 @@ async fn main() -> Result<()> {
         } => get_records(service, collection, sync_if_empty),
         Commands::DumpSync { path, dry_run } => {
             let downloader = CollectionDownloader::new(path);
-            downloader.run(dry_run).await
+            let runtime = tokio::runtime::Runtime::new()?;
+            runtime.block_on(downloader.run(dry_run))
         }
         Commands::DumpGet {
             bucket,
             collection_name,
             path,
         } => {
             let downloader = CollectionDownloader::new(path);
-            downloader.download_single(&bucket, &collection_name).await
+            let runtime = tokio::runtime::Runtime::new()?;
+            runtime.block_on(downloader.download_single(&bucket, &collection_name))
         }
     }
 }