From 3d9568a44055f451d1e155850f7e3086e3cc37c7 Mon Sep 17 00:00:00 2001 From: Grant Azure Date: Sat, 27 Sep 2025 14:40:41 -0700 Subject: [PATCH 1/2] refactor: reduce map_err by adding some From implementations --- .../src/configuration/zonky/extractor.rs | 6 ++--- postgresql_archive/src/error.rs | 25 +++++++++++++++++++ postgresql_archive/src/extractor/registry.rs | 21 +++++----------- .../src/extractor/zip_extractor.rs | 6 ++--- postgresql_archive/src/hasher/registry.rs | 20 ++++----------- postgresql_archive/src/matcher/registry.rs | 20 ++++----------- .../src/repository/maven/repository.rs | 5 ++-- postgresql_archive/src/repository/registry.rs | 20 ++++----------- postgresql_extensions/src/error.rs | 9 +++++++ .../src/repository/registry.rs | 21 ++++------------ 10 files changed, 66 insertions(+), 87 deletions(-) diff --git a/postgresql_archive/src/configuration/zonky/extractor.rs b/postgresql_archive/src/configuration/zonky/extractor.rs index bacdab6..474d348 100644 --- a/postgresql_archive/src/configuration/zonky/extractor.rs +++ b/postgresql_archive/src/configuration/zonky/extractor.rs @@ -43,12 +43,10 @@ pub fn extract(bytes: &Vec, extract_directories: &ExtractDirectories) -> Res debug!("Extracting archive to {}", extract_dir.to_string_lossy()); let reader = Cursor::new(bytes); - let mut archive = ZipArchive::new(reader).map_err(|error| Unexpected(error.to_string()))?; + let mut archive = ZipArchive::new(reader)?; let mut archive_bytes = Vec::new(); for i in 0..archive.len() { - let mut file = archive - .by_index(i) - .map_err(|error| Unexpected(error.to_string()))?; + let mut file = archive.by_index(i)?; let file_name = file.name().to_string(); if file_name.ends_with(".txz") { debug!("Found archive file: {file_name}"); diff --git a/postgresql_archive/src/error.rs b/postgresql_archive/src/error.rs index 7c364e9..da8ae1b 100644 --- a/postgresql_archive/src/error.rs +++ b/postgresql_archive/src/error.rs @@ -1,3 +1,5 @@ +use std::sync::PoisonError; + /// PostgreSQL archive result type pub type Result = core::result::Result; @@ -111,6 +113,29 @@ impl From for Error { } } +#[cfg(feature = "maven")] +/// Converts a [`quick_xml::DeError`] into a [`ParseError`](Error::ParseError) +impl From for Error { + fn from(error: quick_xml::DeError) -> Self { + Error::ParseError(error.to_string()) + } +} + +#[cfg(feature = "zip")] +/// Converts a [`zip::result::ZipError`] into a [`ParseError`](Error::Unexpected) +impl From for Error { + fn from(error: zip::result::ZipError) -> Self { + Error::Unexpected(error.to_string()) + } +} + +/// Converts a [`std::sync::PoisonError`] into a [`ParseError`](Error::PoisonedLock) +impl From> for Error { + fn from(value: PoisonError) -> Self { + Error::PoisonedLock(value.to_string()) + } +} + /// These are relatively low value tests; they are here to reduce the coverage gap and /// ensure that the error conversions are working as expected. #[cfg(test)] diff --git a/postgresql_archive/src/extractor/registry.rs b/postgresql_archive/src/extractor/registry.rs index ea95715..0419b56 100644 --- a/postgresql_archive/src/extractor/registry.rs +++ b/postgresql_archive/src/extractor/registry.rs @@ -1,4 +1,4 @@ -use crate::Error::{PoisonedLock, UnsupportedExtractor}; +use crate::Error::UnsupportedExtractor; use crate::Result; #[cfg(feature = "theseus")] use crate::configuration::theseus; @@ -45,13 +45,10 @@ impl RepositoryRegistry { /// * If the URL is not supported. fn get(&self, url: &str) -> Result { for (supports_fn, extractor_fn) in &self.extractors { - let supports_function = supports_fn - .read() - .map_err(|error| PoisonedLock(error.to_string()))?; + let supports_function = supports_fn.read()?; + if supports_function(url)? { - let extractor_function = extractor_fn - .read() - .map_err(|error| PoisonedLock(error.to_string()))?; + let extractor_function = extractor_fn.read()?; return Ok(*extractor_function); } } @@ -77,10 +74,7 @@ impl Default for RepositoryRegistry { /// # Errors /// * If the registry is poisoned. pub fn register(supports_fn: SupportsFn, extractor_fn: ExtractFn) -> Result<()> { - let mut registry = REGISTRY - .lock() - .map_err(|error| PoisonedLock(error.to_string()))?; - registry.register(supports_fn, extractor_fn); + REGISTRY.lock()?.register(supports_fn, extractor_fn); Ok(()) } @@ -89,10 +83,7 @@ pub fn register(supports_fn: SupportsFn, extractor_fn: ExtractFn) -> Result<()> /// # Errors /// * If the URL is not supported. pub fn get(url: &str) -> Result { - let registry = REGISTRY - .lock() - .map_err(|error| PoisonedLock(error.to_string()))?; - registry.get(url) + REGISTRY.lock()?.get(url) } #[cfg(test)] diff --git a/postgresql_archive/src/extractor/zip_extractor.rs b/postgresql_archive/src/extractor/zip_extractor.rs index 4419422..3bc4d7a 100644 --- a/postgresql_archive/src/extractor/zip_extractor.rs +++ b/postgresql_archive/src/extractor/zip_extractor.rs @@ -16,13 +16,11 @@ use zip::ZipArchive; pub fn extract(bytes: &Vec, extract_directories: &ExtractDirectories) -> Result> { let mut files = Vec::new(); let reader = Cursor::new(bytes); - let mut archive = ZipArchive::new(reader).map_err(|_| io::Error::other("Zip error"))?; + let mut archive = ZipArchive::new(reader)?; let mut extracted_bytes = 0; for i in 0..archive.len() { - let mut file = archive - .by_index(i) - .map_err(|_| io::Error::other("Zip error"))?; + let mut file = archive.by_index(i)?; let file_path = PathBuf::from(file.name()); let file_path = PathBuf::from(file_path.file_name().unwrap_or_default()); let file_name = file_path.to_string_lossy(); diff --git a/postgresql_archive/src/hasher/registry.rs b/postgresql_archive/src/hasher/registry.rs index 79a0d78..c7d124f 100644 --- a/postgresql_archive/src/hasher/registry.rs +++ b/postgresql_archive/src/hasher/registry.rs @@ -1,4 +1,4 @@ -use crate::Error::{PoisonedLock, UnsupportedHasher}; +use crate::Error::UnsupportedHasher; use crate::Result; #[cfg(feature = "theseus")] use crate::configuration::theseus; @@ -54,13 +54,9 @@ impl HasherRegistry { let url = url.as_ref(); let extension = extension.as_ref(); for (supports_fn, hasher_fn) in &self.hashers { - let supports_function = supports_fn - .read() - .map_err(|error| PoisonedLock(error.to_string()))?; + let supports_function = supports_fn.read()?; if supports_function(url, extension)? { - let hasher_function = hasher_fn - .read() - .map_err(|error| PoisonedLock(error.to_string()))?; + let hasher_function = hasher_fn.read()?; return Ok(*hasher_function); } } @@ -109,10 +105,7 @@ impl Default for HasherRegistry { /// # Errors /// * If the registry is poisoned. pub fn register(supports_fn: SupportsFn, hasher_fn: HasherFn) -> Result<()> { - let mut registry = REGISTRY - .lock() - .map_err(|error| PoisonedLock(error.to_string()))?; - registry.register(supports_fn, hasher_fn); + REGISTRY.lock()?.register(supports_fn, hasher_fn); Ok(()) } @@ -121,10 +114,7 @@ pub fn register(supports_fn: SupportsFn, hasher_fn: HasherFn) -> Result<()> { /// # Errors /// * If the registry is poisoned. pub fn get>(url: S, extension: S) -> Result { - let registry = REGISTRY - .lock() - .map_err(|error| PoisonedLock(error.to_string()))?; - registry.get(url, extension) + REGISTRY.lock()?.get(url, extension) } #[cfg(test)] diff --git a/postgresql_archive/src/matcher/registry.rs b/postgresql_archive/src/matcher/registry.rs index 2b64f72..c34bfb7 100644 --- a/postgresql_archive/src/matcher/registry.rs +++ b/postgresql_archive/src/matcher/registry.rs @@ -1,4 +1,4 @@ -use crate::Error::{PoisonedLock, UnsupportedMatcher}; +use crate::Error::UnsupportedMatcher; use crate::Result; #[cfg(feature = "theseus")] use crate::configuration::theseus; @@ -46,13 +46,9 @@ impl MatchersRegistry { fn get>(&self, url: S) -> Result { let url = url.as_ref(); for (supports_fn, matcher_fn) in &self.matchers { - let supports_function = supports_fn - .read() - .map_err(|error| PoisonedLock(error.to_string()))?; + let supports_function = supports_fn.read()?; if supports_function(url)? { - let matcher_function = matcher_fn - .read() - .map_err(|error| PoisonedLock(error.to_string()))?; + let matcher_function = matcher_fn.read()?; return Ok(*matcher_function); } } @@ -79,10 +75,7 @@ impl Default for MatchersRegistry { /// # Errors /// * If the registry is poisoned. pub fn register(supports_fn: SupportsFn, matcher_fn: MatcherFn) -> Result<()> { - let mut registry = REGISTRY - .lock() - .map_err(|error| PoisonedLock(error.to_string()))?; - registry.register(supports_fn, matcher_fn); + REGISTRY.lock()?.register(supports_fn, matcher_fn); Ok(()) } @@ -91,10 +84,7 @@ pub fn register(supports_fn: SupportsFn, matcher_fn: MatcherFn) -> Result<()> { /// # Errors /// * If the registry is poisoned. pub fn get>(url: S) -> Result { - let registry = REGISTRY - .lock() - .map_err(|error| PoisonedLock(error.to_string()))?; - registry.get(url) + REGISTRY.lock()?.get(url) } #[cfg(test)] diff --git a/postgresql_archive/src/repository/maven/repository.rs b/postgresql_archive/src/repository/maven/repository.rs index 7eafe59..b9c6caa 100644 --- a/postgresql_archive/src/repository/maven/repository.rs +++ b/postgresql_archive/src/repository/maven/repository.rs @@ -1,4 +1,4 @@ -use crate::Error::{ArchiveHashMismatch, ParseError, RepositoryFailure, VersionNotFound}; +use crate::Error::{ArchiveHashMismatch, RepositoryFailure, VersionNotFound}; use crate::repository::Archive; use crate::repository::maven::models::Metadata; use crate::repository::model::Repository; @@ -60,8 +60,7 @@ impl Maven { let request = client.get(&url).headers(Self::headers()); let response = request.send().await?.error_for_status()?; let text = response.text().await?; - let metadata: Metadata = - quick_xml::de::from_str(&text).map_err(|error| ParseError(error.to_string()))?; + let metadata: Metadata = quick_xml::de::from_str(&text)?; let artifact = metadata.artifact_id; let mut result = None; for version in &metadata.versioning.versions.version { diff --git a/postgresql_archive/src/repository/registry.rs b/postgresql_archive/src/repository/registry.rs index fd34936..7a43c65 100644 --- a/postgresql_archive/src/repository/registry.rs +++ b/postgresql_archive/src/repository/registry.rs @@ -1,4 +1,4 @@ -use crate::Error::{PoisonedLock, UnsupportedRepository}; +use crate::Error::UnsupportedRepository; use crate::Result; #[cfg(feature = "theseus")] use crate::configuration::theseus; @@ -46,13 +46,9 @@ impl RepositoryRegistry { /// * If the URL is not supported. fn get(&self, url: &str) -> Result> { for (supports_fn, new_fn) in &self.repositories { - let supports_function = supports_fn - .read() - .map_err(|error| PoisonedLock(error.to_string()))?; + let supports_function = supports_fn.read()?; if supports_function(url)? { - let new_function = new_fn - .read() - .map_err(|error| PoisonedLock(error.to_string()))?; + let new_function = new_fn.read()?; return new_function(url); } } @@ -84,10 +80,7 @@ impl Default for RepositoryRegistry { /// # Errors /// * If the registry is poisoned. pub fn register(supports_fn: SupportsFn, new_fn: Box) -> Result<()> { - let mut registry = REGISTRY - .lock() - .map_err(|error| PoisonedLock(error.to_string()))?; - registry.register(supports_fn, new_fn); + REGISTRY.lock()?.register(supports_fn, new_fn); Ok(()) } @@ -96,10 +89,7 @@ pub fn register(supports_fn: SupportsFn, new_fn: Box) -> Result<()> { /// # Errors /// * If the URL is not supported. pub fn get(url: &str) -> Result> { - let registry = REGISTRY - .lock() - .map_err(|error| PoisonedLock(error.to_string()))?; - registry.get(url) + REGISTRY.lock()?.get(url) } #[cfg(test)] diff --git a/postgresql_extensions/src/error.rs b/postgresql_extensions/src/error.rs index fbf4ec9..185013a 100644 --- a/postgresql_extensions/src/error.rs +++ b/postgresql_extensions/src/error.rs @@ -1,3 +1,5 @@ +use std::sync::PoisonError; + /// PostgreSQL extensions result type pub type Result = core::result::Result; @@ -29,3 +31,10 @@ pub enum Error { #[error("unsupported namespace '{0}'")] UnsupportedNamespace(String), } + +/// Converts a [`std::sync::PoisonError`] into a [`ParseError`](Error::PoisonedLock) +impl From> for Error { + fn from(value: PoisonError) -> Self { + Error::PoisonedLock(value.to_string()) + } +} diff --git a/postgresql_extensions/src/repository/registry.rs b/postgresql_extensions/src/repository/registry.rs index d7c4174..a1ef0b6 100644 --- a/postgresql_extensions/src/repository/registry.rs +++ b/postgresql_extensions/src/repository/registry.rs @@ -1,4 +1,4 @@ -use crate::Error::{PoisonedLock, UnsupportedNamespace}; +use crate::Error::UnsupportedNamespace; use crate::Result; use crate::repository::model::Repository; #[cfg(feature = "portal-corp")] @@ -44,9 +44,7 @@ impl RepositoryRegistry { let Some(new_fn) = self.repositories.get(&namespace) else { return Err(UnsupportedNamespace(namespace.to_string())); }; - let new_function = new_fn - .read() - .map_err(|error| PoisonedLock(error.to_string()))?; + let new_function = new_fn.read()?; new_function() } } @@ -79,10 +77,7 @@ impl Default for RepositoryRegistry { /// # Errors /// * If the registry is poisoned. pub fn register(namespace: &str, new_fn: Box) -> Result<()> { - let mut registry = REGISTRY - .lock() - .map_err(|error| PoisonedLock(error.to_string()))?; - registry.register(namespace, new_fn); + REGISTRY.lock()?.register(namespace, new_fn); Ok(()) } @@ -91,10 +86,7 @@ pub fn register(namespace: &str, new_fn: Box) -> Result<()> { /// # Errors /// * If the namespace is not supported. pub fn get(namespace: &str) -> Result> { - let registry = REGISTRY - .lock() - .map_err(|error| PoisonedLock(error.to_string()))?; - registry.get(namespace) + REGISTRY.lock()?.get(namespace) } /// Gets the namespaces of the registered repositories. @@ -102,10 +94,7 @@ pub fn get(namespace: &str) -> Result> { /// # Errors /// * If the registry is poisoned. pub fn get_namespaces() -> Result> { - let registry = REGISTRY - .lock() - .map_err(|error| PoisonedLock(error.to_string()))?; - Ok(registry.repositories.keys().cloned().collect()) + Ok(REGISTRY.lock()?.repositories.keys().cloned().collect()) } /// Gets all the registered repositories. From 36c556a732f75aba78ddfa715916087b1d6abf8f Mon Sep 17 00:00:00 2001 From: Grant Azure Date: Sat, 27 Sep 2025 14:40:41 -0700 Subject: [PATCH 2/2] refactor: reduce map_err by adding some From implementations --- postgresql_archive/src/error.rs | 29 +++++++++++++++++++++++++++++ postgresql_extensions/src/error.rs | 12 ++++++++++++ 2 files changed, 41 insertions(+) diff --git a/postgresql_archive/src/error.rs b/postgresql_archive/src/error.rs index da8ae1b..3143fc1 100644 --- a/postgresql_archive/src/error.rs +++ b/postgresql_archive/src/error.rs @@ -224,4 +224,33 @@ mod test { let error = Error::from(parse_error); assert_eq!(error.to_string(), "empty host"); } + + #[cfg(feature = "maven")] + #[test] + fn test_from_quick_xml_error() { + let xml = ""; + let quick_xml_error = quick_xml::de::from_str::(xml).expect_err("quick_xml error"); + let error = Error::from(quick_xml_error); + assert!(matches!(error, Error::ParseError(_))); + } + + #[cfg(feature = "zip")] + #[test] + fn test_from_zip_error() { + let zip_error = zip::result::ZipError::FileNotFound; + let error = Error::from(zip_error); + assert!(matches!(error, Error::Unexpected(_))); + assert!( + error + .to_string() + .contains("specified file not found in archive") + ); + } + + #[test] + fn test_from_poisoned_lock() { + let error = Error::from(std::sync::PoisonError::new(())); + assert!(matches!(error, Error::PoisonedLock(_))); + assert!(error.to_string().contains("poisoned lock")); + } } diff --git a/postgresql_extensions/src/error.rs b/postgresql_extensions/src/error.rs index 185013a..cdd4f6a 100644 --- a/postgresql_extensions/src/error.rs +++ b/postgresql_extensions/src/error.rs @@ -38,3 +38,15 @@ impl From> for Error { Error::PoisonedLock(value.to_string()) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_from_poison_error() { + let error = Error::from(std::sync::PoisonError::new(())); + assert!(matches!(error, Error::PoisonedLock(_))); + assert!(error.to_string().contains("poisoned lock")); + } +}