From a9c00024a7c6d38c2149ab704f371d3938ddc13d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 9 Mar 2024 18:42:23 -0800 Subject: [PATCH] Move `Error` methods off of `ErrorKind` (#2322) ## Summary Using `ErrorKind` is leaking an abstraction, since this only exists (IIUC) to box the variant. --- crates/uv-client/src/cached_client.rs | 2 +- crates/uv-client/src/error.rs | 139 +++++++++++++----------- crates/uv-client/src/flat_index.rs | 2 +- crates/uv-client/src/registry_client.rs | 2 +- 4 files changed, 79 insertions(+), 66 deletions(-) diff --git a/crates/uv-client/src/cached_client.rs b/crates/uv-client/src/cached_client.rs index a6efb74d9..cbcebf12c 100644 --- a/crates/uv-client/src/cached_client.rs +++ b/crates/uv-client/src/cached_client.rs @@ -356,7 +356,7 @@ impl CachedClient { Err(err) => { // When we know the cache entry doesn't exist, then things are // normal and we shouldn't emit a WARN. - if err.kind().is_file_not_exists() { + if err.is_file_not_exists() { trace!("No cache entry exists for {}", cache_entry.path().display()); } else { warn!( diff --git a/crates/uv-client/src/error.rs b/crates/uv-client/src/error.rs index 16830ee83..461ea709b 100644 --- a/crates/uv-client/src/error.rs +++ b/crates/uv-client/src/error.rs @@ -18,21 +18,93 @@ pub struct Error { } impl Error { + /// Convert this error into its [`ErrorKind`] variant. pub fn into_kind(self) -> ErrorKind { *self.kind } - pub fn kind(&self) -> &ErrorKind { - &self.kind - } - + /// Create a new error from a JSON parsing error. pub(crate) fn from_json_err(err: serde_json::Error, url: Url) -> Self { ErrorKind::BadJson { source: err, url }.into() } + /// Create a new error from an HTML parsing error. pub(crate) fn from_html_err(err: html::Error, url: Url) -> Self { ErrorKind::BadHtml { source: err, url }.into() } + + /// Returns `true` if this error corresponds to an offline error. + pub(crate) fn is_offline(&self) -> bool { + matches!(&*self.kind, ErrorKind::Offline(_)) + } + + /// Returns `true` if this error corresponds to an I/O "not found" error. + pub(crate) fn is_file_not_exists(&self) -> bool { + let ErrorKind::Io(ref err) = &*self.kind else { + return false; + }; + matches!(err.kind(), std::io::ErrorKind::NotFound) + } + + /// Returns `true` if the error is due to the server not supporting HTTP range requests. + pub fn is_http_range_requests_unsupported(&self) -> bool { + match &*self.kind { + // The server doesn't support range requests (as reported by the `HEAD` check). + ErrorKind::AsyncHttpRangeReader( + AsyncHttpRangeReaderError::HttpRangeRequestUnsupported, + ) => { + return true; + } + + // The server returned a "Method Not Allowed" error, indicating it doesn't support + // HEAD requests, so we can't check for range requests. + ErrorKind::ReqwestError(err) => { + if let Some(status) = err.status() { + // If the server doesn't support HEAD requests, we can't check for range + // requests. + if status == reqwest::StatusCode::METHOD_NOT_ALLOWED { + return true; + } + + // In some cases, registries return a 404 for HEAD requests when they're not + // supported. In the worst case, we'll now just proceed to attempt to stream the + // entire file, so it's fine to be somewhat lenient here. + if status == reqwest::StatusCode::NOT_FOUND { + return true; + } + } + } + + // The server doesn't support range requests, but we only discovered this while + // unzipping due to erroneous server behavior. + ErrorKind::Zip(_, ZipError::UpstreamReadError(err)) => { + if let Some(inner) = err.get_ref() { + if let Some(inner) = inner.downcast_ref::() { + if matches!( + inner, + AsyncHttpRangeReaderError::HttpRangeRequestUnsupported + ) { + return true; + } + } + } + } + + _ => {} + } + + false + } + + /// Returns `true` if the error is due to the server not supporting HTTP streaming. Most + /// commonly, this is due to serving ZIP files with features that are incompatible with + /// streaming, like data descriptors. + pub fn is_http_streaming_unsupported(&self) -> bool { + matches!( + &*self.kind, + ErrorKind::Zip(_, ZipError::FeatureNotSupported(_)) + ) + } } impl From for Error { @@ -148,65 +220,6 @@ pub enum ErrorKind { Offline(String), } -impl ErrorKind { - /// Returns true if this error kind corresponds to an I/O "not found" - /// error. - pub(crate) fn is_file_not_exists(&self) -> bool { - let Self::Io(ref err) = *self else { - return false; - }; - matches!(err.kind(), std::io::ErrorKind::NotFound) - } - - /// Returns `true` if the error is due to the server not supporting HTTP range requests. - pub(crate) fn is_http_range_requests_unsupported(&self) -> bool { - match self { - // The server doesn't support range requests (as reported by the `HEAD` check). - Self::AsyncHttpRangeReader(AsyncHttpRangeReaderError::HttpRangeRequestUnsupported) => { - return true; - } - - // The server returned a "Method Not Allowed" error, indicating it doesn't support - // HEAD requests, so we can't check for range requests. - Self::ReqwestError(err) => { - if let Some(status) = err.status() { - // If the server doesn't support HEAD requests, we can't check for range - // requests. - if status == reqwest::StatusCode::METHOD_NOT_ALLOWED { - return true; - } - - // In some cases, registries return a 404 for HEAD requests when they're not - // supported. In the worst case, we'll now just proceed to attempt to stream the - // entire file, so it's fine to be somewhat lenient here. - if status == reqwest::StatusCode::NOT_FOUND { - return true; - } - } - } - - // The server doesn't support range requests, but we only discovered this while - // unzipping due to erroneous server behavior. - Self::Zip(_, ZipError::UpstreamReadError(err)) => { - if let Some(inner) = err.get_ref() { - if let Some(inner) = inner.downcast_ref::() { - if matches!( - inner, - AsyncHttpRangeReaderError::HttpRangeRequestUnsupported - ) { - return true; - } - } - } - } - - _ => {} - } - - false - } -} - impl From for ErrorKind { fn from(error: reqwest::Error) -> Self { Self::ReqwestError(BetterReqwestError::from(error)) diff --git a/crates/uv-client/src/flat_index.rs b/crates/uv-client/src/flat_index.rs index d66c3a7e6..ca624f7b8 100644 --- a/crates/uv-client/src/flat_index.rs +++ b/crates/uv-client/src/flat_index.rs @@ -205,7 +205,7 @@ impl<'a> FlatIndexClient<'a> { .collect(); Ok(FlatIndexEntries::from_entries(files)) } - Err(CachedClientError::Client(err)) if matches!(err.kind(), ErrorKind::Offline(_)) => { + Err(CachedClientError::Client(err)) if err.is_offline() => { Ok(FlatIndexEntries::offline()) } Err(err) => Err(err.into()), diff --git a/crates/uv-client/src/registry_client.rs b/crates/uv-client/src/registry_client.rs index 0e553b96e..cdb3cff04 100644 --- a/crates/uv-client/src/registry_client.rs +++ b/crates/uv-client/src/registry_client.rs @@ -534,7 +534,7 @@ impl RegistryClient { match result { Ok(metadata) => return Ok(metadata), Err(err) => { - if err.kind().is_http_range_requests_unsupported() { + if err.is_http_range_requests_unsupported() { // The range request version failed. Fall back to streaming the file to search // for the METADATA file. warn!("Range requests not supported for {filename}; streaming wheel");