diff --git a/src/autorelay.rs b/src/autorelay.rs index e92b06096..056b764d9 100644 --- a/src/autorelay.rs +++ b/src/autorelay.rs @@ -16,7 +16,6 @@ //! [`Config::AutorelayFinished`] is set and nothing is ever added again, //! so deleting a transport later does not pull in a replacement. -use std::collections::{BTreeMap, BTreeSet}; use std::pin::Pin; use anyhow::Result; @@ -29,6 +28,7 @@ use tokio::task::JoinSet; use crate::config::{self, Config}; use crate::log::{LogExt, warn}; use crate::login_param::{EnteredCertificateChecks, EnteredImapLoginParam}; +use crate::sql::TransactionExt as _; use crate::{configure::EnteredLoginParam, context::Context, tools::time}; /// The target number of transports. @@ -54,14 +54,6 @@ pub(crate) async fn init_transports_inner( context: &Context, addrs_from_qr: Vec, ) -> Result<(), anyhow::Error> { - // TODO The default relay candidates need to be updated in the database, too. - // It would be annoying to have to write a migration everytime a relay candidate comes or goes; - // The solution is to make the relay candidates list into a const, - // and check in `load_relay_candidates()` whether any of them should be added - // rather than in a migration - // (if we need to remove some later, we will then need another const `REMOVED_RELAY_CANDIDATES` which are ignored; - // or alternatively we could use `relay_candidates` table only for saving the last used timestamps, - // and if we later want to add other sources for relay candidates then we need another table for that) let mut candidates: Vec<&str> = DEFAULT_RELAY_CANDIDATES.into(); candidates.shuffle(&mut rng()); @@ -226,23 +218,33 @@ async fn maybe_add_additional_relays_inner(context: &Context, skip_network: bool pub(crate) async fn load_relay_candidates(context: &Context, now: i64) -> Result> { let cutoff_timestamp = now.saturating_sub(BACKOFF_PERIOD_FOR_NOT_WORKING_RELAY); + let candidates: Vec = context .sql - .query_map_vec( - // This also selects candidates which have last_tried in the future, - // essentially treating them as never tried, - // so if some timestamp far in the future is accidentally stored, - // we are not stuck never trying the candidate. - // After trying the candidate, last_tried will be corrected to the current time. - "SELECT host FROM relay_candidates WHERE (last_tried?) + .transaction(|transaction| { + // Add any relay candidates that are not in the database yet + let mut statement = + transaction.prepare("INSERT OR IGNORE INTO relay_candidates(host) VALUES (?)")?; + for host in DEFAULT_RELAY_CANDIDATES { + statement.execute((host,))?; + } + + transaction.query_map_vec( + // This also selects candidates which have last_tried in the future, + // essentially treating them as never tried, + // so if some timestamp far in the future is accidentally stored, + // we are not stuck never trying the candidate. + // After trying the candidate, last_tried will be corrected to the current time. + "SELECT host FROM relay_candidates WHERE (last_tried?) AND NOT EXISTS ( SELECT 1 FROM transports WHERE substr(addr, instr(addr, '@') + 1) = host )", - (cutoff_timestamp, now), - |row| Ok(row.get::<_, String>(0)?), - ) + (cutoff_timestamp, now), + |row| Ok(row.get::<_, String>(0)?), + ) + }) .await?; Ok(candidates) diff --git a/src/autorelay/autorelay_tests.rs b/src/autorelay/autorelay_tests.rs index 21dde7980..2e796d8ca 100644 --- a/src/autorelay/autorelay_tests.rs +++ b/src/autorelay/autorelay_tests.rs @@ -10,7 +10,12 @@ async fn test_load_relay_candidates_single() -> Result<()> { enable_config(t).await; let now = time(); - t.sql.execute("DELETE FROM relay_candidates", ()).await?; + // Fill the default candidates, and make sure that they + // are not used by setting last_used to now: + load_relay_candidates(t, now).await?; + t.sql + .execute("UPDATE relay_candidates SET last_tried=?", (now,)) + .await?; // This host should be returned by load_relay_candidates(): t.sql @@ -49,8 +54,9 @@ async fn test_load_relay_candidates_multiple() -> Result<()> { enable_config(t).await; let now = time(); - t.sql.execute("DELETE FROM relay_candidates", ()).await?; - for host in ["a.example", "b.example", "c.example"] { + const EXAMPLE_CANDIDATES: &[&str] = &["a.example", "b.example", "c.example"]; + + for host in EXAMPLE_CANDIDATES { t.sql .execute( "INSERT INTO relay_candidates (host, last_tried) VALUES (?, ?)", @@ -62,14 +68,11 @@ async fn test_load_relay_candidates_multiple() -> Result<()> { let mut candidates = load_relay_candidates(t, now).await?; candidates.sort(); - assert_eq!( - candidates, - vec![ - "a.example".to_string(), - "b.example".to_string(), - "c.example".to_string() - ] - ); + let mut expected = EXAMPLE_CANDIDATES.to_vec(); + expected.extend(DEFAULT_RELAY_CANDIDATES); + expected.sort(); + + assert_eq!(candidates, expected); Ok(()) } @@ -160,7 +163,13 @@ async fn test_maybe_add_additional_relays_add_one() -> Result<()> { enable_config(t).await; let now = time(); - t.sql.execute("DELETE FROM relay_candidates", ()).await?; + // Fill the default candidates, and make sure that they + // are not used by setting last_used to now: + load_relay_candidates(t, now).await?; + t.sql + .execute("UPDATE relay_candidates SET last_tried=?", (now,)) + .await?; + t.sql .execute( "INSERT INTO relay_candidates (host, last_tried) VALUES (?, ?)", @@ -218,7 +227,13 @@ async fn test_maybe_add_additional_relays_failure() -> Result<()> { enable_config(t).await; let now = time(); - t.sql.execute("DELETE FROM relay_candidates", ()).await?; + // Fill the default candidates, and make sure that they + // are not used by setting last_used to now: + load_relay_candidates(t, now).await?; + t.sql + .execute("UPDATE relay_candidates SET last_tried=?", (now,)) + .await?; + for i in 1..10 { t.sql .execute( diff --git a/src/sql.rs b/src/sql.rs index bc8030cf8..d13f72810 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -684,6 +684,38 @@ impl Sql { } } +pub(crate) trait TransactionExt { + /// Prepares and executes the statement and maps a function over the resulting rows. + /// + /// Collects the resulting rows into a `Vec`. + fn query_map_vec( + &self, + sql: &str, + params: impl rusqlite::Params + Send, + f: F, + ) -> Result> + where + T: Send + 'static, + F: Send + FnMut(&rusqlite::Row) -> Result; +} + +impl TransactionExt for rusqlite::Transaction<'_> { + fn query_map_vec( + &self, + sql: &str, + params: impl rusqlite::Params + Send, + f: F, + ) -> Result> + where + T: Send + 'static, + F: Send + FnMut(&rusqlite::Row) -> Result, + { + let mut stmt = self.prepare(sql)?; + let res = stmt.query_and_then(params, f)?; + res.collect() + } +} + /// Creates a new SQLite connection. /// /// `path` is the database path. diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 1b8870853..c5f008196 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -2673,6 +2673,13 @@ CREATE TABLE smtp2 ( .await?; } + inc_and_check(&mut migration_version, 167)?; + if dbversion < migration_version { + // The previous relay candidates were only for testing + sql.execute_migration("DELETE FROM relay_candidates;", migration_version) + .await?; + } + let new_version = sql .get_raw_config_int(VERSION_CFG) .await?