Refactor: Database connection setup code

This patch refactors the database connection setup code to involve less copying
of data in memory.

It also changes the configuration value type of the database port and the
database connection timeout setting to be `u16` instead of `String`, which is
another improvement.

Because the `DbConnectionConfig` type no longer holds owned values, it must be
annotated with a lifetime. Usages were adapted.

Signed-off-by: Matthias Beyer <matthias.beyer@atos.net>

5 files changed, 66 insertions, 61 deletions

diff --git a/src/commands/db.rs b/src/commands/db.rs
index 896caa1..fa009db 100644
--- a/src/commands/db.rs
+++ b/src/commands/db.rs
@@ -36,7 +36,7 @@ use crate::schema;
 
 /// Implementation of the "db" subcommand
 pub fn db(
-    db_connection_config: DbConnectionConfig,
+    db_connection_config: DbConnectionConfig<'_>,
     config: &Configuration,
     matches: &ArgMatches,
 ) -> Result<()> {
@@ -56,14 +56,14 @@ pub fn db(
     }
 }
 
-fn cli(db_connection_config: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
+fn cli(db_connection_config: DbConnectionConfig<'_>, matches: &ArgMatches) -> Result<()> {
     trait PgCliCommand {
-        fn run_for_uri(&self, dbcc: DbConnectionConfig) -> Result<()>;
+        fn run_for_uri(&self, dbcc: DbConnectionConfig<'_>) -> Result<()>;
     }
 
     struct Psql(PathBuf);
     impl PgCliCommand for Psql {
-        fn run_for_uri(&self, dbcc: DbConnectionConfig) -> Result<()> {
+        fn run_for_uri(&self, dbcc: DbConnectionConfig<'_>) -> Result<()> {
             Command::new(&self.0)
                 .arg(format!("--dbname={}", dbcc.database_name()))
                 .arg(format!("--host={}", dbcc.database_host()))
@@ -92,12 +92,12 @@ fn cli(db_connection_config: DbConnectionConfig, matches: &ArgMatches) -> Result
 
     struct PgCli(PathBuf);
     impl PgCliCommand for PgCli {
-        fn run_for_uri(&self, dbcc: DbConnectionConfig) -> Result<()> {
+        fn run_for_uri(&self, dbcc: DbConnectionConfig<'_>) -> Result<()> {
             Command::new(&self.0)
                 .arg("--host")
                 .arg(dbcc.database_host())
                 .arg("--port")
-                .arg(dbcc.database_port())
+                .arg(dbcc.database_port().to_string())
                 .arg("--username")
                 .arg(dbcc.database_user())
                 .arg(dbcc.database_name())
@@ -139,7 +139,7 @@ fn cli(db_connection_config: DbConnectionConfig, matches: &ArgMatches) -> Result
         .run_for_uri(db_connection_config)
 }
 
-fn artifacts(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
+fn artifacts(conn_cfg: DbConnectionConfig<'_>, matches: &ArgMatches) -> Result<()> {
     use crate::schema::artifacts::dsl;
 
     let csv = matches.is_present("csv");
@@ -188,7 +188,7 @@ fn artifacts(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
     Ok(())
 }
 
-fn envvars(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
+fn envvars(conn_cfg: DbConnectionConfig<'_>, matches: &ArgMatches) -> Result<()> {
     use crate::schema::envvars::dsl;
 
     let csv = matches.is_present("csv");
@@ -209,7 +209,7 @@ fn envvars(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
     Ok(())
 }
 
-fn images(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
+fn images(conn_cfg: DbConnectionConfig<'_>, matches: &ArgMatches) -> Result<()> {
     use crate::schema::images::dsl;
 
     let csv = matches.is_present("csv");
@@ -230,7 +230,7 @@ fn images(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
     Ok(())
 }
 
-fn submit(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
+fn submit(conn_cfg: DbConnectionConfig<'_>, matches: &ArgMatches) -> Result<()> {
     let conn = crate::db::establish_connection(conn_cfg)?;
     let submit_id = matches.value_of("submit")
         .map(uuid::Uuid::from_str)
@@ -300,7 +300,7 @@ fn submit(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
     crate::commands::util::display_data(header, data, false)
 }
 
-fn submits(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
+fn submits(conn_cfg: DbConnectionConfig<'_>, matches: &ArgMatches) -> Result<()> {
     let csv = matches.is_present("csv");
     let limit = matches.value_of("limit").map(i64::from_str).transpose()?;
     let hdrs = crate::commands::util::mk_header(vec!["id", "time", "uuid"]);
@@ -366,7 +366,7 @@ fn submits(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
     Ok(())
 }
 
-fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
+fn jobs(conn_cfg: DbConnectionConfig<'_>, matches: &ArgMatches) -> Result<()> {
     let csv = matches.is_present("csv");
     let hdrs = crate::commands::util::mk_header(vec![
         "id",
@@ -457,7 +457,7 @@ fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
     Ok(())
 }
 
-fn job(conn_cfg: DbConnectionConfig, config: &Configuration, matches: &ArgMatches) -> Result<()> {
+fn job(conn_cfg: DbConnectionConfig<'_>, config: &Configuration, matches: &ArgMatches) -> Result<()> {
     let script_highlight = !matches.is_present("no_script_highlight");
     let script_line_numbers = !matches.is_present("no_script_line_numbers");
     let configured_theme = config.script_highlight_theme();
@@ -628,7 +628,7 @@ fn job(conn_cfg: DbConnectionConfig, config: &Configuration, matches: &ArgMatche
 }
 
 /// Implementation of the subcommand "db log-of"
-fn log_of(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
+fn log_of(conn_cfg: DbConnectionConfig<'_>, matches: &ArgMatches) -> Result<()> {
     let conn   = crate::db::establish_connection(conn_cfg)?;
     let job_uuid = matches
         .value_of("job_uuid")
@@ -650,7 +650,7 @@ fn log_of(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> {
         .map(|_| ())
 }
 
-fn releases(conn_cfg: DbConnectionConfig, config: &Configuration, matches: &ArgMatches) -> Result<()> {
+fn releases(conn_cfg: DbConnectionConfig<'_>, config: &Configuration, matches: &ArgMatches) -> Result<()> {
     let csv    = matches.is_present("csv");
     let conn   = crate::db::establish_connection(conn_cfg)?;
     let header = crate::commands::util::mk_header(["Package", "Version", "Date", "Path"].to_vec());
@@ -674,7 +674,7 @@ fn releases(conn_cfg: DbConnectionConfig, config: &Configuration, matches: &ArgM
         .load::<(models::Artifact, models::Package, models::Release, models::ReleaseStore)>(&conn)?
         .into_iter()
         .filter_map(|(art, pack, rel, rstore)| {
-            let p = config.releases_directory().join(rstore.store_name).join(&art.path);
+        let p = config.releases_directory().join(rstore.store_name).join(&art.path);
 
             if p.is_file() {
                 Some(vec![
diff --git a/src/commands/release.rs b/src/commands/release.rs
index 0628d40..ec12d30 100644
--- a/src/commands/release.rs
+++ b/src/commands/release.rs
@@ -25,7 +25,7 @@ use crate::db::DbConnectionConfig;
 
 /// Implementation of the "release" subcommand
 pub async fn release(
-    db_connection_config: DbConnectionConfig,
+    db_connection_config: DbConnectionConfig<'_>,
     config: &Configuration,
     matches: &ArgMatches,
 ) -> Result<()> {
@@ -39,7 +39,7 @@ pub async fn release(
 
 
 async fn new_release(
-    db_connection_config: DbConnectionConfig,
+    db_connection_config: DbConnectionConfig<'_>,
     config: &Configuration,
     matches: &ArgMatches,
 ) -> Result<()> {
@@ -188,7 +188,7 @@ async fn new_release(
 }
 
 pub async fn rm_release(
-    db_connection_config: DbConnectionConfig,
+    db_connection_config: DbConnectionConfig<'_>,
     config: &Configuration,
     matches: &ArgMatches,
 ) -> Result<()> {
diff --git a/src/config/not_validated.rs b/src/config/not_validated.rs
index 14399c4..b005aca 100644
--- a/src/config/not_validated.rs
+++ b/src/config/not_validated.rs
@@ -82,7 +82,7 @@ pub struct NotValidatedConfiguration {
 
     #[getset(get = "pub")]
     #[serde(rename = "database_port")]
-    database_port: String,
+    database_port: u16,
 
     #[getset(get = "pub")]
     #[serde(rename = "database_user")]
diff --git a/src/db/connection.rs b/src/db/connection.rs
index fb18e32..5e91e24 100644
--- a/src/db/connection.rs
+++ b/src/db/connection.rs
@@ -8,6 +8,8 @@
 // SPDX-License-Identifier: EPL-2.0
 //
 
+use std::str::FromStr;
+
 use anyhow::Error;
 use anyhow::Result;
 use clap::ArgMatches;
@@ -19,27 +21,27 @@ use log::debug;
 use crate::config::Configuration;
 
 #[derive(Debug, Getters)]
-pub struct DbConnectionConfig {
+pub struct DbConnectionConfig<'a> {
     #[getset(get = "pub")]
-    database_host: String,
+    database_host: &'a str,
 
     #[getset(get = "pub")]
-    database_port: String,
+    database_port: u16,
 
     #[getset(get = "pub")]
-    database_user: String,
+    database_user: &'a str,
 
     #[getset(get = "pub")]
-    database_password: String,
+    database_password: &'a str,
 
     #[getset(get = "pub")]
-    database_name: String,
+    database_name: &'a str,
 
     #[getset(get = "pub")]
-    database_connection_timeout: String,
+    database_connection_timeout: u16,
 }
 
-impl Into<String> for DbConnectionConfig {
+impl<'a> Into<String> for DbConnectionConfig<'a> {
     fn into(self) -> String {
         format!(
             "postgres://{user}:{password}@{host}:{port}/{name}?connect_timeout={timeout}",
@@ -53,40 +55,43 @@ impl Into<String> for DbConnectionConfig {
     }
 }
 
-pub fn parse_db_connection_config(config: &Configuration, cli: &ArgMatches) -> DbConnectionConfig {
-    fn find_value<F>(cli: &ArgMatches, key: &str, alternative: F) -> String
-    where
-        F: FnOnce() -> String,
-    {
-        cli.value_of(key)
-            .map(String::from)
-            .unwrap_or_else(alternative)
-    }
-
-    let database_host = find_value(cli, "database_host", || config.database_host().to_string());
-    let database_port = find_value(cli, "database_port", || config.database_port().to_string());
-    let database_user = find_value(cli, "database_user", || config.database_user().to_string());
-    let database_password = find_value(cli, "database_password", || {
-        config.database_password().to_string()
-    });
-    let database_name = find_value(cli, "database_name", || config.database_name().to_string());
-    let database_connection_timeout = find_value(cli, "database_connection_timeout",
-        || {
-            // hardcoded default of 30 seconds database timeout
-            config.database_connection_timeout().unwrap_or(30).to_string()
-        });
-
-    DbConnectionConfig {
-        database_host,
-        database_port,
-        database_user,
-        database_password,
-        database_name,
-        database_connection_timeout,
-    }
+pub fn parse_db_connection_config<'a>(config: &'a Configuration, cli: &'a ArgMatches) -> Result<DbConnectionConfig<'a>> {
+    Ok(DbConnectionConfig {
+        database_host: {
+            cli.value_of("database_host")
+                .unwrap_or_else(|| config.database_host())
+        },
+        database_port: {
+            cli.value_of("database_port")
+                .map(u16::from_str)
+                .transpose()?
+                .unwrap_or_else(|| *config.database_port())
+        },
+        database_user: {
+            cli.value_of("database_user")
+                .unwrap_or_else(|| config.database_user())
+        },
+        database_password: {
+            cli.value_of("database_password")
+                .unwrap_or_else(|| config.database_password())
+        },
+        database_name: {
+            cli.value_of("database_name")
+                .unwrap_or_else(|| config.database_name())
+        },
+        database_connection_timeout: {
+            cli.value_of("database_connection_timeout")
+                .map(u16::from_str)
+                .transpose()?
+                .unwrap_or_else( || {
+                    // hardcoded default of 30 seconds database timeout
+                    config.database_connection_timeout().unwrap_or(30)
+                })
+        },
+    })
 }
 
-pub fn establish_connection(conn_config: DbConnectionConfig) -> Result<PgConnection> {
+pub fn establish_connection<'a>(conn_config: DbConnectionConfig<'a>) -> Result<PgConnection> {
     let database_uri: String = conn_config.into();
     debug!("Trying to connect to database: {}", database_uri);
     PgConnection::establish(&database_uri).map_err(Error::from)
diff --git a/src/main.rs b/src/main.rs
index 8547375..69dd0fa 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -146,7 +146,7 @@ async fn main() -> Result<()> {
         Ok(repo)
     };
 
-    let db_connection_config = crate::db::parse_db_connection_config(&config, &cli);
+    let db_connection_config = crate::db::parse_db_connection_config(&config, &cli)?;
     match cli.subcommand() {
         Some(("generate-completions", matches)) => generate_completions(matches),
         Some(("db", matches)) => {