Repository: wasmerio/wasmer (language: rust)

Issue:
wasix: Improve the QueryError
The package resolution QueryError does not carry the queried identifier, which leads to very confusing errors propagating through the system.

The error should be refactored into a struct with the queried specified and an error kind.


Discussion / hints:

Fix patch:
diff --git a/lib/wasix/src/runtime/resolver/backend_source.rs b/lib/wasix/src/runtime/resolver/backend_source.rs
index 00fd3491b54..1d943ffc347 100644
--- a/lib/wasix/src/runtime/resolver/backend_source.rs
+++ b/lib/wasix/src/runtime/resolver/backend_source.rs
@@ -217,7 +217,7 @@ impl BackendSource {
     async fn query_by_hash(
         &self,
         hash: &PackageHash,
-    ) -> Result<Option<PackageSummary>, QueryError> {
+    ) -> Result<Option<PackageSummary>, anyhow::Error> {
         // FIXME: implementing caching!
 
         let Some(data) = self.query_graphql_by_hash(hash).await? else {
@@ -241,22 +241,31 @@ impl Source for BackendSource {
             ),
             PackageSource::Ident(PackageIdent::Hash(hash)) => {
                 // TODO: implement caching!
-                match self.query_by_hash(hash).await? {
-                    Some(summary) => return Ok(vec![summary]),
-                    None => {
+                match self.query_by_hash(hash).await {
+                    Ok(Some(summary)) => return Ok(vec![summary]),
+                    Ok(None) => {
                         return Err(QueryError::NoMatches {
+                            query: package.clone(),
                             archived_versions: Vec::new(),
                         });
                     }
+                    Err(error) => {
+                        return Err(QueryError::new_other(error, package));
+                    }
                 }
             }
-            _ => return Err(QueryError::Unsupported),
+            _ => {
+                return Err(QueryError::Unsupported {
+                    query: package.clone(),
+                })
+            }
         };
 
         if let Some(cache) = &self.cache {
             match cache.lookup_cached_query(&package_name) {
                 Ok(Some(cached)) => {
                     if let Ok(cached) = matching_package_summaries(
+                        package,
                         cached,
                         &version_constraint,
                         self.preferred_webc_version,
@@ -276,7 +285,10 @@ impl Source for BackendSource {
             }
         }
 
-        let response = self.query_graphql_named(&package_name).await?;
+        let response = self
+            .query_graphql_named(&package_name)
+            .await
+            .map_err(|error| QueryError::new_other(error, package))?;
 
         if let Some(cache) = &self.cache {
             if let Err(e) = cache.update(&package_name, &response) {
@@ -288,11 +300,18 @@ impl Source for BackendSource {
             }
         }
 
-        matching_package_summaries(response, &version_constraint, self.preferred_webc_version)
+        matching_package_summaries(
+            package,
+            response,
+            &version_constraint,
+            self.preferred_webc_version,
+        )
     }
 }
 
+#[allow(clippy::result_large_err)]
 fn matching_package_summaries(
+    query: &PackageSource,
     response: WebQuery,
     version_constraint: &VersionReq,
     preferred_webc_version: webc::Version,
@@ -304,7 +323,12 @@ fn matching_package_summaries(
         package_name,
         versions,
         ..
-    } = response.data.get_package.ok_or(QueryError::NotFound)?;
+    } = response
+        .data
+        .get_package
+        .ok_or_else(|| QueryError::NotFound {
+            query: query.clone(),
+        })?;
     let mut archived_versions = Vec::new();
 
     for pkg_version in versions {
@@ -349,7 +373,10 @@ fn matching_package_summaries(
     }
 
     if summaries.is_empty() {
-        Err(QueryError::NoMatches { archived_versions })
+        Err(QueryError::NoMatches {
+            query: query.clone(),
+            archived_versions,
+        })
     } else {
         Ok(summaries)
     }
diff --git a/lib/wasix/src/runtime/resolver/filesystem_source.rs b/lib/wasix/src/runtime/resolver/filesystem_source.rs
index 471fa18b9b9..1927f8af02a 100644
--- a/lib/wasix/src/runtime/resolver/filesystem_source.rs
+++ b/lib/wasix/src/runtime/resolver/filesystem_source.rs
@@ -10,29 +10,14 @@ use crate::runtime::resolver::{
 #[derive(Debug, Default, Clone, PartialEq, Eq)]
 pub struct FileSystemSource {}
 
-#[async_trait::async_trait]
-impl Source for FileSystemSource {
-    #[tracing::instrument(level = "debug", skip_all, fields(%package))]
-    async fn query(&self, package: &PackageSource) -> Result<Vec<PackageSummary>, QueryError> {
-        let path = match package {
-            PackageSource::Path(path) => {
-                let path = std::path::PathBuf::from(path);
-                path.canonicalize().with_context(|| {
-                    format!(
-                        "Unable to get the canonical form for \"{}\"",
-                        path.display()
-                    )
-                })?
-            }
-            _ => return Err(QueryError::Unsupported),
-        };
-
-        let webc_sha256 = crate::block_in_place(|| WebcHash::for_file(&path))
+impl FileSystemSource {
+    async fn load_path(path: &std::path::Path) -> Result<Vec<PackageSummary>, anyhow::Error> {
+        let webc_sha256 = crate::block_in_place(|| WebcHash::for_file(path))
             .with_context(|| format!("Unable to hash \"{}\"", path.display()))?;
-        let container = crate::block_in_place(|| Container::from_disk(&path))
+        let container = crate::block_in_place(|| Container::from_disk(path))
             .with_context(|| format!("Unable to parse \"{}\"", path.display()))?;
 
-        let url = crate::runtime::resolver::utils::url_from_file_path(&path)
+        let url = crate::runtime::resolver::utils::url_from_file_path(path)
             .ok_or_else(|| anyhow::anyhow!("Unable to turn \"{}\" into a URL", path.display()))?;
 
         let id = PackageInfo::package_id_from_manifest(container.manifest())
@@ -52,3 +37,32 @@ impl Source for FileSystemSource {
         Ok(vec![summary])
     }
 }
+
+#[async_trait::async_trait]
+impl Source for FileSystemSource {
+    #[tracing::instrument(level = "debug", skip_all, fields(%package))]
+    async fn query(&self, package: &PackageSource) -> Result<Vec<PackageSummary>, QueryError> {
+        let path = match package {
+            PackageSource::Path(path) => {
+                let path = std::path::PathBuf::from(path);
+                path.canonicalize()
+                    .with_context(|| {
+                        format!(
+                            "Unable to get the canonical form for \"{}\"",
+                            path.display()
+                        )
+                    })
+                    .map_err(|error| QueryError::new_other(error, package))?
+            }
+            _ => {
+                return Err(QueryError::Unsupported {
+                    query: package.clone(),
+                })
+            }
+        };
+
+        Self::load_path(&path)
+            .await
+            .map_err(|error| QueryError::new_other(error, package))
+    }
+}
diff --git a/lib/wasix/src/runtime/resolver/in_memory_source.rs b/lib/wasix/src/runtime/resolver/in_memory_source.rs
index 1505cf12099..1e97b0da0c4 100644
--- a/lib/wasix/src/runtime/resolver/in_memory_source.rs
+++ b/lib/wasix/src/runtime/resolver/in_memory_source.rs
@@ -157,13 +157,16 @@ impl Source for InMemorySource {
 
                         if matches.is_empty() {
                             return Err(QueryError::NoMatches {
+                                query: package.clone(),
                                 archived_versions: Vec::new(),
                             });
                         }
 
                         Ok(matches)
                     }
-                    None => Err(QueryError::NotFound),
+                    None => Err(QueryError::NotFound {
+                        query: package.clone(),
+                    }),
                 }
             }
             PackageSource::Ident(PackageIdent::Hash(hash)) => self
@@ -171,9 +174,12 @@ impl Source for InMemorySource {
                 .get(hash)
                 .map(|x| vec![x.clone()])
                 .ok_or_else(|| QueryError::NoMatches {
+                    query: package.clone(),
                     archived_versions: Vec::new(),
                 }),
-            PackageSource::Url(_) | PackageSource::Path(_) => Err(QueryError::Unsupported),
+            PackageSource::Url(_) | PackageSource::Path(_) => Err(QueryError::Unsupported {
+                query: package.clone(),
+            }),
         }
     }
 }
diff --git a/lib/wasix/src/runtime/resolver/inputs.rs b/lib/wasix/src/runtime/resolver/inputs.rs
index db0cea3b03f..8b5114b8126 100644
--- a/lib/wasix/src/runtime/resolver/inputs.rs
+++ b/lib/wasix/src/runtime/resolver/inputs.rs
@@ -2,7 +2,7 @@ use std::{
     fmt::{self, Display, Formatter},
     fs::File,
     io::{BufRead, BufReader, Read},
-    path::{Path, PathBuf},
+    path::Path,
 };
 
 use anyhow::Error;
@@ -290,9 +290,9 @@ impl WebcHash {
         Ok(Self(hash))
     }
 
-    pub fn for_file(path: &PathBuf) -> Result<Self, std::io::Error> {
+    pub fn for_file(path: &Path) -> Result<Self, std::io::Error> {
         // check for a hash at the file location
-        let mut path_hash = path.clone();
+        let mut path_hash = path.to_owned();
         path_hash.set_extension("webc.sha256");
         if let Ok(mut file) = File::open(&path_hash) {
             let mut hash = Vec::new();
diff --git a/lib/wasix/src/runtime/resolver/multi_source.rs b/lib/wasix/src/runtime/resolver/multi_source.rs
index e876b8225bf..ed450f5d24b 100644
--- a/lib/wasix/src/runtime/resolver/multi_source.rs
+++ b/lib/wasix/src/runtime/resolver/multi_source.rs
@@ -72,12 +72,12 @@ impl Source for MultiSource {
                         return Ok(summaries);
                     }
                 }
-                Err(QueryError::Unsupported)
+                Err(QueryError::Unsupported { .. })
                     if self.strategy.continue_if_unsupported || self.strategy.merge_results =>
                 {
                     continue
                 }
-                Err(QueryError::NotFound)
+                Err(QueryError::NotFound { .. })
                     if self.strategy.continue_if_not_found || self.strategy.merge_results =>
                 {
                     continue
@@ -87,6 +87,9 @@ impl Source for MultiSource {
                 {
                     continue
                 }
+                // Generic errors do not respect the `merge_results` strategy
+                // flag, because unexpected errors should be bubbled to the
+                // caller.
                 Err(e) => return Err(e),
             }
         }
@@ -96,7 +99,9 @@ impl Source for MultiSource {
 
             Ok(output)
         } else {
-            Err(QueryError::NotFound)
+            Err(QueryError::NotFound {
+                query: package.clone(),
+            })
         }
     }
 }
diff --git a/lib/wasix/src/runtime/resolver/source.rs b/lib/wasix/src/runtime/resolver/source.rs
index a3bb5b8d74a..1992750f1ca 100644
--- a/lib/wasix/src/runtime/resolver/source.rs
+++ b/lib/wasix/src/runtime/resolver/source.rs
@@ -1,4 +1,7 @@
-use std::fmt::{Debug, Display};
+use std::{
+    fmt::{Debug, Display},
+    sync::Arc,
+};
 
 use wasmer_config::package::{PackageIdent, PackageSource};
 
@@ -34,9 +37,13 @@ pub trait Source: Sync + Debug {
                     left_version.cmp(&right_version)
                 })
                 .ok_or(QueryError::NoMatches {
+                    query: pkg.clone(),
                     archived_versions: Vec::new(),
                 }),
-            _ => candidates.into_iter().next().ok_or(QueryError::NotFound),
+            _ => candidates
+                .into_iter()
+                .next()
+                .ok_or_else(|| QueryError::NotFound { query: pkg.clone() }),
         }
     }
 }
@@ -52,46 +59,76 @@ where
     }
 }
 
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 pub enum QueryError {
-    Unsupported,
-    NotFound,
+    Unsupported {
+        query: PackageSource,
+    },
+    NotFound {
+        query: PackageSource,
+    },
     NoMatches {
+        query: PackageSource,
         archived_versions: Vec<semver::Version>,
     },
-    Timeout,
-    Other(anyhow::Error),
+    Timeout {
+        query: PackageSource,
+    },
+    Other {
+        query: PackageSource,
+        // Arc to make it cloneable
+        // Cloning is important for some use-cases.
+        error: Arc<anyhow::Error>,
+    },
 }
 
-impl From<anyhow::Error> for QueryError {
-    fn from(value: anyhow::Error) -> Self {
-        Self::Other(value)
+impl QueryError {
+    pub fn query(&self) -> &PackageSource {
+        match self {
+            Self::Unsupported { query }
+            | Self::NotFound { query }
+            | Self::NoMatches { query, .. }
+            | Self::Timeout { query }
+            | Self::Other { query, .. } => query,
+        }
+    }
+
+    pub fn new_other(err: anyhow::Error, query: &PackageSource) -> Self {
+        Self::Other {
+            query: query.clone(),
+            error: Arc::new(err),
+        }
     }
 }
 
 impl Display for QueryError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "failed to query package '{}': ", self.query())?;
+
         match self {
-            Self::Unsupported => f.write_str("This type of package specifier isn't supported"),
-            Self::NotFound => f.write_str("Not found"),
-            Self::Timeout => f.write_str("Timed out"),
-            Self::NoMatches { archived_versions } => match archived_versions.as_slice() {
+            Self::Unsupported { .. } => f.write_str("unsupported package specifier"),
+            Self::NotFound { .. } => f.write_str("not found"),
+            Self::Timeout { .. } => f.write_str("timeout"),
+            Self::NoMatches {
+                query: _,
+                archived_versions,
+            } => match archived_versions.as_slice() {
                 [] => f.write_str(
-                    "The package was found, but no published versions matched the constraint",
+                    "the package was found, but no published versions matched the constraint",
                 ),
                 [version] => write!(
                     f,
-                    "The only version satisfying the constraint, {version}, is archived"
+                    "the only version satisfying the constraint, {version}, is archived"
                 ),
                 [first, rest @ ..] => {
                     let num_others = rest.len();
                     write!(
                         f,
-                        "Unable to satisfy the request. Version {first}, and {num_others} are all archived"
+                        "unable to satisfy the request - version {first}, and {num_others} are all archived"
                     )
                 }
             },
-            Self::Other(e) => Display::fmt(e, f),
+            Self::Other { error: e, query: _ } => Display::fmt(e, f),
         }
     }
 }
@@ -99,8 +136,11 @@ impl Display for QueryError {
 impl std::error::Error for QueryError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match self {
-            Self::Other(e) => Some(&**e),
-            Self::Unsupported | Self::NotFound | Self::NoMatches { .. } | Self::Timeout => None,
+            Self::Other { error, query: _ } => Some(&***error),
+            Self::Unsupported { .. }
+            | Self::NotFound { .. }
+            | Self::NoMatches { .. }
+            | Self::Timeout { .. } => None,
         }
     }
 }
diff --git a/lib/wasix/src/runtime/resolver/web_source.rs b/lib/wasix/src/runtime/resolver/web_source.rs
index 0660e752d90..75371bef1e4 100644
--- a/lib/wasix/src/runtime/resolver/web_source.rs
+++ b/lib/wasix/src/runtime/resolver/web_source.rs
@@ -224,17 +224,8 @@ impl WebSource {
 
         Ok((body, etag))
     }
-}
-
-#[async_trait::async_trait]
-impl Source for WebSource {
-    #[tracing::instrument(level = "debug", skip_all, fields(%package))]
-    async fn query(&self, package: &PackageSource) -> Result<Vec<PackageSummary>, QueryError> {
-        let url = match package {
-            PackageSource::Url(url) => url,
-            _ => return Err(QueryError::Unsupported),
-        };
 
+    async fn load_url(&self, url: &Url) -> Result<Vec<PackageSummary>, anyhow::Error> {
         let local_path = self
             .get_locally_cached_file(url)
             .await
@@ -263,6 +254,25 @@ impl Source for WebSource {
     }
 }
 
+#[async_trait::async_trait]
+impl Source for WebSource {
+    #[tracing::instrument(level = "debug", skip_all, fields(%package))]
+    async fn query(&self, package: &PackageSource) -> Result<Vec<PackageSummary>, QueryError> {
+        let url = match package {
+            PackageSource::Url(url) => url,
+            _ => {
+                return Err(QueryError::Unsupported {
+                    query: package.clone(),
+                })
+            }
+        };
+
+        self.load_url(url)
+            .await
+            .map_err(|error| QueryError::new_other(error, package))
+    }
+}
+
 fn sha256(bytes: &[u8]) -> String {
     let mut hasher = Sha256::default();
     hasher.update(bytes);


Test patch:
diff --git a/tests/compilers/serialize.rs b/tests/compilers/serialize.rs
index fce0451402a..bc60a2b9376 100644
--- a/tests/compilers/serialize.rs
+++ b/tests/compilers/serialize.rs
@@ -62,7 +62,7 @@ fn test_deserialize(config: crate::Config) -> Result<()> {
     );
     let f0 = Function::new(&mut store, &func_type, |params| {
         let param_0: i64 = params[0].unwrap_i32() as i64;
-        let param_1: i64 = params[1].unwrap_i64() as i64;
+        let param_1: i64 = params[1].unwrap_i64();
         let param_2: i64 = params[2].unwrap_i32() as i64;
         let param_3: i64 = params[3].unwrap_f32() as i64;
         let param_4: i64 = params[4].unwrap_f64() as i64;