various: add Display impls for domain enums; improve contextual errors

Signed-off-by: NotAShelf <raf@notashelf.dev>
Change-Id: Ia16e7e34cda6ae3e12590ea1ea9268486a6a6964

crates/pinakes-core/src/config.rs

@@ -2,10 +2,25 @@ use std::path::{Path, PathBuf};
 
 use serde::{Deserialize, Serialize};
 
-/// Expand environment variables in a string.
+/// Expand environment variables in a string using `std::env::var` for lookup.
-/// Supports both ${VAR_NAME} and $VAR_NAME syntax.
+/// Supports both `${VAR_NAME}` and `$VAR_NAME` syntax.
 /// Returns an error if a referenced variable is not set.
 fn expand_env_var_string(input: &str) -> crate::error::Result<String> {
+  expand_env_vars(input, |name| {
+    std::env::var(name).map_err(|_| {
+      crate::error::PinakesError::Config(format!(
+        "environment variable not set: {name}"
+      ))
+    })
+  })
+}
+
+/// Expand environment variables in a string using the provided lookup function.
+/// Supports both `${VAR_NAME}` and `$VAR_NAME` syntax.
+fn expand_env_vars(
+  input: &str,
+  lookup: impl Fn(&str) -> crate::error::Result<String>,
+) -> crate::error::Result<String> {
   let mut result = String::new();
   let mut chars = input.chars().peekable();
 
@@ -44,16 +59,7 @@ fn expand_env_var_string(input: &str) -> crate::error::Result<String> {
         ));
       }
 
-      // Look up the environment variable
+      result.push_str(&lookup(&var_name)?);
-      match std::env::var(&var_name) {
-        Ok(value) => result.push_str(&value),
-        Err(_) => {
-          return Err(crate::error::PinakesError::Config(format!(
-            "environment variable not set: {}",
-            var_name
-          )));
-        },
-      }
     } else if ch == '\\' {
       // Handle escaped characters
       if let Some(&next_ch) = chars.peek() {
@@ -769,6 +775,21 @@ pub enum StorageBackendType {
   Postgres,
 }
 
+impl StorageBackendType {
+  pub fn as_str(&self) -> &'static str {
+    match self {
+      Self::Sqlite => "sqlite",
+      Self::Postgres => "postgres",
+    }
+  }
+}
+
+impl std::fmt::Display for StorageBackendType {
+  fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+    f.write_str(self.as_str())
+  }
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct SqliteConfig {
   pub path: PathBuf,
@@ -1200,64 +1221,58 @@ mod tests {
     assert!(config.validate().is_ok());
   }
 
-  // Environment variable expansion tests
+  // Environment variable expansion tests using expand_env_vars with a
+  // HashMap lookup. This avoids unsafe std::env::set_var and is
+  // thread-safe for parallel test execution.
+  fn test_lookup<'a>(
+    vars: &'a std::collections::HashMap<&str, &str>,
+  ) -> impl Fn(&str) -> crate::error::Result<String> + 'a {
+    move |name| {
+      vars.get(name).map(|v| v.to_string()).ok_or_else(|| {
+        crate::error::PinakesError::Config(format!(
+          "environment variable not set: {name}"
+        ))
+      })
+    }
+  }
+
   #[test]
   fn test_expand_env_var_simple() {
-    unsafe {
+    let vars =
-      std::env::set_var("TEST_VAR_SIMPLE", "test_value");
+      std::collections::HashMap::from([("TEST_VAR_SIMPLE", "test_value")]);
-    }
+    let result = expand_env_vars("$TEST_VAR_SIMPLE", test_lookup(&vars));
-    let result = expand_env_var_string("$TEST_VAR_SIMPLE");
-    assert!(result.is_ok());
     assert_eq!(result.unwrap(), "test_value");
-    unsafe {
-      std::env::remove_var("TEST_VAR_SIMPLE");
-    }
   }
 
   #[test]
   fn test_expand_env_var_braces() {
-    unsafe {
+    let vars =
-      std::env::set_var("TEST_VAR_BRACES", "test_value");
+      std::collections::HashMap::from([("TEST_VAR_BRACES", "test_value")]);
-    }
+    let result = expand_env_vars("${TEST_VAR_BRACES}", test_lookup(&vars));
-    let result = expand_env_var_string("${TEST_VAR_BRACES}");
-    assert!(result.is_ok());
     assert_eq!(result.unwrap(), "test_value");
-    unsafe {
-      std::env::remove_var("TEST_VAR_BRACES");
-    }
   }
 
   #[test]
   fn test_expand_env_var_embedded() {
-    unsafe {
+    let vars =
-      std::env::set_var("TEST_VAR_EMBEDDED", "value");
+      std::collections::HashMap::from([("TEST_VAR_EMBEDDED", "value")]);
-    }
+    let result =
-    let result = expand_env_var_string("prefix_${TEST_VAR_EMBEDDED}_suffix");
+      expand_env_vars("prefix_${TEST_VAR_EMBEDDED}_suffix", test_lookup(&vars));
-    assert!(result.is_ok());
     assert_eq!(result.unwrap(), "prefix_value_suffix");
-    unsafe {
-      std::env::remove_var("TEST_VAR_EMBEDDED");
-    }
   }
 
   #[test]
   fn test_expand_env_var_multiple() {
-    unsafe {
+    let vars =
-      std::env::set_var("VAR1", "value1");
+      std::collections::HashMap::from([("VAR1", "value1"), ("VAR2", "value2")]);
-      std::env::set_var("VAR2", "value2");
+    let result = expand_env_vars("${VAR1}_${VAR2}", test_lookup(&vars));
-    }
-    let result = expand_env_var_string("${VAR1}_${VAR2}");
-    assert!(result.is_ok());
     assert_eq!(result.unwrap(), "value1_value2");
-    unsafe {
-      std::env::remove_var("VAR1");
-      std::env::remove_var("VAR2");
-    }
   }
 
   #[test]
   fn test_expand_env_var_missing() {
-    let result = expand_env_var_string("${NONEXISTENT_VAR}");
+    let vars = std::collections::HashMap::new();
+    let result = expand_env_vars("${NONEXISTENT_VAR}", test_lookup(&vars));
     assert!(result.is_err());
     assert!(
       result
@@ -1269,7 +1284,8 @@ mod tests {
 
   #[test]
   fn test_expand_env_var_empty_name() {
-    let result = expand_env_var_string("${}");
+    let vars = std::collections::HashMap::new();
+    let result = expand_env_vars("${}", test_lookup(&vars));
     assert!(result.is_err());
     assert!(
       result
@@ -1281,43 +1297,33 @@ mod tests {
 
   #[test]
   fn test_expand_env_var_escaped() {
-    let result = expand_env_var_string("\\$NOT_A_VAR");
+    let vars = std::collections::HashMap::new();
-    assert!(result.is_ok());
+    let result = expand_env_vars("\\$NOT_A_VAR", test_lookup(&vars));
     assert_eq!(result.unwrap(), "$NOT_A_VAR");
   }
 
   #[test]
   fn test_expand_env_var_no_vars() {
-    let result = expand_env_var_string("plain_text");
+    let vars = std::collections::HashMap::new();
-    assert!(result.is_ok());
+    let result = expand_env_vars("plain_text", test_lookup(&vars));
     assert_eq!(result.unwrap(), "plain_text");
   }
 
   #[test]
   fn test_expand_env_var_underscore() {
-    unsafe {
+    let vars = std::collections::HashMap::from([("TEST_VAR_NAME", "value")]);
-      std::env::set_var("TEST_VAR_NAME", "value");
+    let result = expand_env_vars("$TEST_VAR_NAME", test_lookup(&vars));
-    }
-    let result = expand_env_var_string("$TEST_VAR_NAME");
-    assert!(result.is_ok());
     assert_eq!(result.unwrap(), "value");
-    unsafe {
-      std::env::remove_var("TEST_VAR_NAME");
-    }
   }
 
   #[test]
   fn test_expand_env_var_mixed_syntax() {
-    unsafe {
+    let vars = std::collections::HashMap::from([
-      std::env::set_var("VAR1_MIXED", "v1");
+      ("VAR1_MIXED", "v1"),
-      std::env::set_var("VAR2_MIXED", "v2");
+      ("VAR2_MIXED", "v2"),
-    }
+    ]);
-    let result = expand_env_var_string("$VAR1_MIXED and ${VAR2_MIXED}");
+    let result =
-    assert!(result.is_ok());
+      expand_env_vars("$VAR1_MIXED and ${VAR2_MIXED}", test_lookup(&vars));
     assert_eq!(result.unwrap(), "v1 and v2");
-    unsafe {
-      std::env::remove_var("VAR1_MIXED");
-      std::env::remove_var("VAR2_MIXED");
-    }
   }
 }

crates/pinakes-core/src/error.rs

@@ -105,6 +105,9 @@ pub enum PinakesError {
 
   #[error("insufficient share permissions")]
   InsufficientSharePermissions,
+
+  #[error("serialization error: {0}")]
+  Serialization(String),
 }
 
 impl From<rusqlite::Error> for PinakesError {
@@ -121,8 +124,19 @@ impl From<tokio_postgres::Error> for PinakesError {
 
 impl From<serde_json::Error> for PinakesError {
   fn from(e: serde_json::Error) -> Self {
-    PinakesError::Database(format!("JSON serialization error: {}", e))
+    PinakesError::Serialization(e.to_string())
   }
 }
 
+/// Build a closure that wraps a database error with operation context.
+///
+/// Usage: `stmt.execute(params).map_err(db_ctx("insert_media", media_id))?;`
+pub fn db_ctx<E: std::fmt::Display>(
+  operation: &str,
+  entity: impl std::fmt::Display,
+) -> impl FnOnce(E) -> PinakesError {
+  let context = format!("{operation} [{entity}]");
+  move |e| PinakesError::Database(format!("{context}: {e}"))
+}
+
 pub type Result<T> = std::result::Result<T, PinakesError>;

crates/pinakes-core/src/model.rs

@@ -181,6 +181,23 @@ pub enum CustomFieldType {
   Boolean,
 }
 
+impl CustomFieldType {
+  pub fn as_str(&self) -> &'static str {
+    match self {
+      Self::Text => "text",
+      Self::Number => "number",
+      Self::Date => "date",
+      Self::Boolean => "boolean",
+    }
+  }
+}
+
+impl std::fmt::Display for CustomFieldType {
+  fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+    f.write_str(self.as_str())
+  }
+}
+
 /// A tag that can be applied to media items.
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct Tag {
@@ -210,6 +227,21 @@ pub enum CollectionKind {
   Virtual,
 }
 
+impl CollectionKind {
+  pub fn as_str(&self) -> &'static str {
+    match self {
+      Self::Manual => "manual",
+      Self::Virtual => "virtual",
+    }
+  }
+}
+
+impl std::fmt::Display for CollectionKind {
+  fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+    f.write_str(self.as_str())
+  }
+}
+
 /// A member of a collection with position tracking.
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct CollectionMember {

crates/pinakes-core/src/users.rs

@@ -109,6 +109,20 @@ impl LibraryPermission {
   pub fn can_admin(&self) -> bool {
     matches!(self, Self::Admin)
   }
+
+  pub fn as_str(&self) -> &'static str {
+    match self {
+      Self::Read => "read",
+      Self::Write => "write",
+      Self::Admin => "admin",
+    }
+  }
+}
+
+impl std::fmt::Display for LibraryPermission {
+  fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+    f.write_str(self.as_str())
+  }
 }
 
 /// User's access to a specific library root

crates/pinakes-server/src/dto.rs

@@ -547,7 +547,7 @@ impl From<pinakes_core::model::MediaItem> for MediaResponse {
         .into_iter()
         .map(|(k, v)| {
           (k, CustomFieldResponse {
-            field_type: format!("{:?}", v.field_type).to_lowercase(),
+            field_type: v.field_type.to_string(),
             value:      v.value,
           })
         })
@@ -587,7 +587,7 @@ impl From<pinakes_core::model::Collection> for CollectionResponse {
       id:           col.id.to_string(),
       name:         col.name,
       description:  col.description,
-      kind:         format!("{:?}", col.kind).to_lowercase(),
+      kind:         col.kind.to_string(),
       filter_query: col.filter_query,
       created_at:   col.created_at,
       updated_at:   col.updated_at,
@@ -715,7 +715,7 @@ impl From<pinakes_core::users::UserLibraryAccess> for UserLibraryResponse {
     Self {
       user_id:    access.user_id.0.to_string(),
       root_path:  access.root_path,
-      permission: format!("{:?}", access.permission).to_lowercase(),
+      permission: access.permission.to_string(),
       granted_at: access.granted_at,
     }
   }

crates/pinakes-server/src/routes/config.rs

@@ -33,7 +33,7 @@ pub async fn get_config(
   };
 
   Ok(Json(ConfigResponse {
-    backend: format!("{:?}", config.storage.backend).to_lowercase(),
+    backend: config.storage.backend.to_string(),
     database_path: config
       .storage
       .sqlite
@@ -146,7 +146,7 @@ pub async fn update_scanning_config(
   };
 
   Ok(Json(ConfigResponse {
-    backend: format!("{:?}", config.storage.backend).to_lowercase(),
+    backend: config.storage.backend.to_string(),
     database_path: config
       .storage
       .sqlite