From c4910bb434b2deb0d9adafb6fb9a1e924c3cae4b Mon Sep 17 00:00:00 2001 From: Till Wegmueller Date: Mon, 8 Dec 2025 23:13:27 +0100 Subject: [PATCH] Refactor error handling in repository module to include structured error variants with detailed context. - Replaced string-based `FileReadError`, `FileWriteError`, and `DirectoryCreateError` with structured error types using `PathBuf` and `source`. - Updated all affected methods in `rest_backend`, `file_backend`, and tests to use the new structured error variants. - Improved error messages for clarity and debugging by embedding file paths and sources directly into error data. --- libips/src/repository/file_backend.rs | 47 +++++++++++++-------------- libips/src/repository/mod.rs | 26 +++++++++++---- libips/src/repository/rest_backend.rs | 6 ++-- libips/src/repository/tests.rs | 2 +- 4 files changed, 46 insertions(+), 35 deletions(-) diff --git a/libips/src/repository/file_backend.rs b/libips/src/repository/file_backend.rs index 4e12f96..3ff3ae6 100644 --- a/libips/src/repository/file_backend.rs +++ b/libips/src/repository/file_backend.rs @@ -342,20 +342,19 @@ impl Transaction { if temp_file_path.exists() { // If it exists, remove it to avoid any issues with existing content fs::remove_file(&temp_file_path).map_err(|e| { - RepositoryError::FileWriteError(format!( - "Failed to remove existing temp file: {}", - e - )) + RepositoryError::FileWriteError { + path: temp_file_path.clone(), + source: e, + } })?; } // Read the file content let file_content = fs::read(file_path).map_err(|e| { - RepositoryError::FileReadError(format!( - "Failed to read file {}: {}", - file_path.display(), - e - )) + RepositoryError::FileReadError { + path: file_path.to_path_buf(), + source: e, + } })?; // Create a payload with the hash information if it doesn't exist @@ -383,10 +382,10 @@ impl Transaction { // Write the compressed data to the temp file fs::write(&temp_file_path, &compressed_data).map_err(|e| { - RepositoryError::FileWriteError(format!( - "Failed to write compressed data to temp file: {}", - e - )) + RepositoryError::FileWriteError { + path: temp_file_path.clone(), + source: e, + } })?; // Calculate hash of the compressed data @@ -410,10 +409,10 @@ impl Transaction { // Write the compressed data to the temp file fs::write(&temp_file_path, &compressed_data).map_err(|e| { - RepositoryError::FileWriteError(format!( - "Failed to write LZ4 compressed data to temp file: {}", - e - )) + RepositoryError::FileWriteError { + path: temp_file_path.clone(), + source: e, + } })?; // Calculate hash of the compressed data @@ -663,7 +662,7 @@ impl ReadableRepository for FileBackend { // Load the repository configuration let config_path = path.join(REPOSITORY_CONFIG_FILENAME); - let config_data = fs::read_to_string(config_path)?; + let config_data = fs::read_to_string(&config_path).map_err(|e| RepositoryError::ConfigReadError(format!("{}: {}", config_path.display(), e)))?; let config: RepositoryConfig = serde_json::from_str(&config_data)?; Ok(FileBackend { @@ -1280,7 +1279,7 @@ impl ReadableRepository for FileBackend { // If destination already exists and matches digest, do nothing if dest.exists() { - let bytes = fs::read(dest)?; + let bytes = fs::read(dest).map_err(|e| RepositoryError::FileReadError { path: dest.to_path_buf(), source: e })?; match crate::digest::Digest::from_bytes(&bytes, algo.clone(), crate::digest::DigestSource::PrimaryPayloadHash) { Ok(comp) if comp.hash == hash => return Ok(()), _ => { /* fall through to overwrite */ } @@ -1288,7 +1287,7 @@ impl ReadableRepository for FileBackend { } // Read source content and verify digest - let bytes = fs::read(&source_path)?; + let bytes = fs::read(&source_path).map_err(|e| RepositoryError::FileReadError { path: source_path.clone(), source: e })?; match crate::digest::Digest::from_bytes(&bytes, algo, crate::digest::DigestSource::PrimaryPayloadHash) { Ok(comp) => { if comp.hash != hash { @@ -1674,14 +1673,14 @@ impl FileBackend { // Preferred path: publisher-scoped manifest path let path = Self::construct_manifest_path(&self.path, publisher, fmri.stem(), &version); if path.exists() { - return std::fs::read_to_string(&path).map_err(|e| RepositoryError::FileReadError(format!("{}", e))); + return std::fs::read_to_string(&path).map_err(|e| RepositoryError::FileReadError { path, source: e }); } // Fallbacks: global pkg layout without publisher let encoded_stem = Self::url_encode(fmri.stem()); let encoded_version = Self::url_encode(&version); let alt1 = self.path.join("pkg").join(&encoded_stem).join(&encoded_version); if alt1.exists() { - return std::fs::read_to_string(&alt1).map_err(|e| RepositoryError::FileReadError(format!("{}", e))); + return std::fs::read_to_string(&alt1).map_err(|e| RepositoryError::FileReadError { path: alt1, source: e }); } let alt2 = self .path @@ -1691,7 +1690,7 @@ impl FileBackend { .join(&encoded_stem) .join(&encoded_version); if alt2.exists() { - return std::fs::read_to_string(&alt2).map_err(|e| RepositoryError::FileReadError(format!("{}", e))); + return std::fs::read_to_string(&alt2).map_err(|e| RepositoryError::FileReadError { path: alt2, source: e }); } Err(RepositoryError::NotFound(format!("manifest for {} not found", fmri))) } @@ -2079,7 +2078,7 @@ impl FileBackend { } // Read the manifest content for hash calculation - let manifest_content = fs::read_to_string(&manifest_path)?; + let manifest_content = fs::read_to_string(&manifest_path).map_err(|e| RepositoryError::FileReadError { path: manifest_path.clone(), source: e })?; // Parse the manifest using parse_file which handles JSON correctly let manifest = Manifest::parse_file(&manifest_path)?; diff --git a/libips/src/repository/mod.rs b/libips/src/repository/mod.rs index fcac099..d0c4d72 100644 --- a/libips/src/repository/mod.rs +++ b/libips/src/repository/mod.rs @@ -6,7 +6,7 @@ use miette::Diagnostic; use std::collections::HashMap; use std::io; -use std::path::{Path, StripPrefixError}; +use std::path::{Path, PathBuf, StripPrefixError}; use thiserror::Error; /// Result type for repository operations @@ -57,26 +57,38 @@ pub enum RepositoryError { )] ConfigWriteError(String), - #[error("failed to create directory: {0}")] + #[error("failed to create directory {path}: {source}")] #[diagnostic( code(ips::repository_error::directory_create), help("Check that the parent directory exists and is writable") )] - DirectoryCreateError(String), + DirectoryCreateError { + path: PathBuf, + #[source] + source: io::Error, + }, - #[error("failed to read file: {0}")] + #[error("failed to read file {path}: {source}")] #[diagnostic( code(ips::repository_error::file_read), help("Check that the file exists and is readable") )] - FileReadError(String), + FileReadError { + path: PathBuf, + #[source] + source: io::Error, + }, - #[error("failed to write file: {0}")] + #[error("failed to write file {path}: {source}")] #[diagnostic( code(ips::repository_error::file_write), help("Check that the directory is writable") )] - FileWriteError(String), + FileWriteError { + path: PathBuf, + #[source] + source: io::Error, + }, #[error("failed to parse JSON: {0}")] #[diagnostic( diff --git a/libips/src/repository/rest_backend.rs b/libips/src/repository/rest_backend.rs index df1b65c..472b422 100644 --- a/libips/src/repository/rest_backend.rs +++ b/libips/src/repository/rest_backend.rs @@ -917,14 +917,14 @@ impl RestBackend { .map_err(|e| { // Report failure progress.finish(&progress_info); - RepositoryError::FileWriteError(format!("Failed to create file: {}", e)) + RepositoryError::FileWriteError { path: file_path.clone(), source: e } })?; file.write_all(&content) .map_err(|e| { // Report failure progress.finish(&progress_info); - RepositoryError::FileWriteError(format!("Failed to write file: {}", e)) + RepositoryError::FileWriteError { path: file_path.clone(), source: e } })?; debug!("Stored catalog file: {}", file_path.display()); @@ -981,7 +981,7 @@ impl RestBackend { let attrs_content = fs::read_to_string(&attrs_path) .map_err(|e| { progress_reporter.finish(&overall_progress); - RepositoryError::FileReadError(format!("Failed to read catalog.attrs: {}", e)) + RepositoryError::FileReadError { path: attrs_path.clone(), source: e } })?; let attrs: Value = serde_json::from_str(&attrs_content) diff --git a/libips/src/repository/tests.rs b/libips/src/repository/tests.rs index 3760db9..2dec5a5 100644 --- a/libips/src/repository/tests.rs +++ b/libips/src/repository/tests.rs @@ -81,7 +81,7 @@ mod tests { // Check if the manifest file exists if !manifest_path.exists() { println!("Error: Manifest file does not exist"); - return Err(RepositoryError::FileReadError(format!( + return Err(RepositoryError::NotFound(format!( "Manifest file does not exist: {}", manifest_path.display() )));