refactor(ext/cache): use concrete error type (denoland#26109)

crowlKats · web-flow · commit 938a8ebe3476 · 2024-10-12T09:15:10.000-07:00
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/ext/cache/Cargo.toml b/ext/cache/Cargo.toml
@@ -19,4 +19,5 @@ deno_core.workspace = true
 rusqlite.workspace = true
 serde.workspace = true
 sha2.workspace = true
+thiserror.workspace = true
 tokio.workspace = true
diff --git a/ext/cache/lib.rs b/ext/cache/lib.rs
@@ -7,7 +7,6 @@ use std::sync::Arc;
 
 use async_trait::async_trait;
 use deno_core::error::type_error;
-use deno_core::error::AnyError;
 use deno_core::op2;
 use deno_core::serde::Deserialize;
 use deno_core::serde::Serialize;
@@ -19,6 +18,20 @@ use deno_core::ResourceId;
 mod sqlite;
 pub use sqlite::SqliteBackedCache;
 
+#[derive(Debug, thiserror::Error)]
+pub enum CacheError {
+  #[error(transparent)]
+  Sqlite(#[from] rusqlite::Error),
+  #[error(transparent)]
+  JoinError(#[from] tokio::task::JoinError),
+  #[error(transparent)]
+  Resource(deno_core::error::AnyError),
+  #[error(transparent)]
+  Other(deno_core::error::AnyError),
+  #[error(transparent)]
+  Io(#[from] std::io::Error),
+}
+
 #[derive(Clone)]
 pub struct CreateCache<C: Cache + 'static>(pub Arc<dyn Fn() -> C>);
 
@@ -92,34 +105,39 @@ pub struct CacheDeleteRequest {
 pub trait Cache: Clone + 'static {
   type CacheMatchResourceType: Resource;
 
-  async fn storage_open(&self, cache_name: String) -> Result<i64, AnyError>;
-  async fn storage_has(&self, cache_name: String) -> Result<bool, AnyError>;
-  async fn storage_delete(&self, cache_name: String) -> Result<bool, AnyError>;
+  async fn storage_open(&self, cache_name: String) -> Result<i64, CacheError>;
+  async fn storage_has(&self, cache_name: String) -> Result<bool, CacheError>;
+  async fn storage_delete(
+    &self,
+    cache_name: String,
+  ) -> Result<bool, CacheError>;
 
   /// Put a resource into the cache.
   async fn put(
     &self,
     request_response: CachePutRequest,
     resource: Option<Rc<dyn Resource>>,
-  ) -> Result<(), AnyError>;
+  ) -> Result<(), CacheError>;
 
   async fn r#match(
     &self,
     request: CacheMatchRequest,
   ) -> Result<
     Option<(CacheMatchResponseMeta, Option<Self::CacheMatchResourceType>)>,
-    AnyError,
+    CacheError,
   >;
-  async fn delete(&self, request: CacheDeleteRequest)
-    -> Result<bool, AnyError>;
+  async fn delete(
+    &self,
+    request: CacheDeleteRequest,
+  ) -> Result<bool, CacheError>;
 }
 
 #[op2(async)]
 #[number]
 pub async fn op_cache_storage_open<CA>(
   state: Rc<RefCell<OpState>>,
   #[string] cache_name: String,
-) -> Result<i64, AnyError>
+) -> Result<i64, CacheError>
 where
   CA: Cache,
 {
@@ -131,7 +149,7 @@ where
 pub async fn op_cache_storage_has<CA>(
   state: Rc<RefCell<OpState>>,
   #[string] cache_name: String,
-) -> Result<bool, AnyError>
+) -> Result<bool, CacheError>
 where
   CA: Cache,
 {
@@ -143,7 +161,7 @@ where
 pub async fn op_cache_storage_delete<CA>(
   state: Rc<RefCell<OpState>>,
   #[string] cache_name: String,
-) -> Result<bool, AnyError>
+) -> Result<bool, CacheError>
 where
   CA: Cache,
 {
@@ -155,13 +173,19 @@ where
 pub async fn op_cache_put<CA>(
   state: Rc<RefCell<OpState>>,
   #[serde] request_response: CachePutRequest,
-) -> Result<(), AnyError>
+) -> Result<(), CacheError>
 where
   CA: Cache,
 {
   let cache = get_cache::<CA>(&state)?;
   let resource = match request_response.response_rid {
-    Some(rid) => Some(state.borrow_mut().resource_table.take_any(rid)?),
+    Some(rid) => Some(
+      state
+        .borrow_mut()
+        .resource_table
+        .take_any(rid)
+        .map_err(CacheError::Resource)?,
+    ),
     None => None,
   };
   cache.put(request_response, resource).await
@@ -172,7 +196,7 @@ where
 pub async fn op_cache_match<CA>(
   state: Rc<RefCell<OpState>>,
   #[serde] request: CacheMatchRequest,
-) -> Result<Option<CacheMatchResponse>, AnyError>
+) -> Result<Option<CacheMatchResponse>, CacheError>
 where
   CA: Cache,
 {
@@ -191,15 +215,15 @@ where
 pub async fn op_cache_delete<CA>(
   state: Rc<RefCell<OpState>>,
   #[serde] request: CacheDeleteRequest,
-) -> Result<bool, AnyError>
+) -> Result<bool, CacheError>
 where
   CA: Cache,
 {
   let cache = get_cache::<CA>(&state)?;
   cache.delete(request).await
 }
 
-pub fn get_cache<CA>(state: &Rc<RefCell<OpState>>) -> Result<CA, AnyError>
+pub fn get_cache<CA>(state: &Rc<RefCell<OpState>>) -> Result<CA, CacheError>
 where
   CA: Cache,
 {
@@ -211,7 +235,9 @@ where
     state.put(cache);
     Ok(state.borrow::<CA>().clone())
   } else {
-    Err(type_error("CacheStorage is not available in this context"))
+    Err(CacheError::Other(type_error(
+      "CacheStorage is not available in this context",
+    )))
   }
 }
 
diff --git a/ext/cache/sqlite.rs b/ext/cache/sqlite.rs
@@ -30,6 +30,7 @@ use crate::serialize_headers;
 use crate::vary_header_matches;
 use crate::Cache;
 use crate::CacheDeleteRequest;
+use crate::CacheError;
 use crate::CacheMatchRequest;
 use crate::CacheMatchResponseMeta;
 use crate::CachePutRequest;
@@ -102,7 +103,7 @@ impl Cache for SqliteBackedCache {
   /// Open a cache storage. Internally, this creates a row in the
   /// sqlite db if the cache doesn't exist and returns the internal id
   /// of the cache.
-  async fn storage_open(&self, cache_name: String) -> Result<i64, AnyError> {
+  async fn storage_open(&self, cache_name: String) -> Result<i64, CacheError> {
     let db = self.connection.clone();
     let cache_storage_dir = self.cache_storage_dir.clone();
     spawn_blocking(move || {
@@ -121,14 +122,14 @@ impl Cache for SqliteBackedCache {
       )?;
       let responses_dir = get_responses_dir(cache_storage_dir, cache_id);
       std::fs::create_dir_all(responses_dir)?;
-      Ok::<i64, AnyError>(cache_id)
+      Ok::<i64, CacheError>(cache_id)
     })
     .await?
   }
 
   /// Check if a cache with the provided name exists.
   /// Note: this doesn't check the disk, it only checks the sqlite db.
-  async fn storage_has(&self, cache_name: String) -> Result<bool, AnyError> {
+  async fn storage_has(&self, cache_name: String) -> Result<bool, CacheError> {
     let db = self.connection.clone();
     spawn_blocking(move || {
       let db = db.lock();
@@ -140,13 +141,16 @@ impl Cache for SqliteBackedCache {
           Ok(count > 0)
         },
       )?;
-      Ok::<bool, AnyError>(cache_exists)
+      Ok::<bool, CacheError>(cache_exists)
     })
     .await?
   }
 
   /// Delete a cache storage. Internally, this deletes the row in the sqlite db.
-  async fn storage_delete(&self, cache_name: String) -> Result<bool, AnyError> {
+  async fn storage_delete(
+    &self,
+    cache_name: String,
+  ) -> Result<bool, CacheError> {
     let db = self.connection.clone();
     let cache_storage_dir = self.cache_storage_dir.clone();
     spawn_blocking(move || {
@@ -167,7 +171,7 @@ impl Cache for SqliteBackedCache {
           std::fs::remove_dir_all(cache_dir)?;
         }
       }
-      Ok::<bool, AnyError>(maybe_cache_id.is_some())
+      Ok::<bool, CacheError>(maybe_cache_id.is_some())
     })
     .await?
   }
@@ -176,10 +180,12 @@ impl Cache for SqliteBackedCache {
     &self,
     request_response: CachePutRequest,
     resource: Option<Rc<dyn Resource>>,
-  ) -> Result<(), AnyError> {
+  ) -> Result<(), CacheError> {
     let db = self.connection.clone();
     let cache_storage_dir = self.cache_storage_dir.clone();
-    let now = SystemTime::now().duration_since(UNIX_EPOCH)?;
+    let now = SystemTime::now()
+      .duration_since(UNIX_EPOCH)
+      .expect("SystemTime is before unix epoch");
 
     if let Some(resource) = resource {
       let body_key = hash(&format!(
@@ -193,7 +199,11 @@ impl Cache for SqliteBackedCache {
       let mut file = tokio::fs::File::create(response_path).await?;
       let mut buf = BufMutView::new(64 * 1024);
       loop {
-        let (size, buf2) = resource.clone().read_byob(buf).await?;
+        let (size, buf2) = resource
+          .clone()
+          .read_byob(buf)
+          .await
+          .map_err(CacheError::Other)?;
         if size == 0 {
           break;
         }
@@ -224,7 +234,7 @@ impl Cache for SqliteBackedCache {
     request: CacheMatchRequest,
   ) -> Result<
     Option<(CacheMatchResponseMeta, Option<CacheResponseResource>)>,
-    AnyError,
+    CacheError,
   > {
     let db = self.connection.clone();
     let cache_storage_dir = self.cache_storage_dir.clone();
@@ -290,19 +300,17 @@ impl Cache for SqliteBackedCache {
           }
           Err(err) => return Err(err.into()),
         };
-        return Ok(Some((cache_meta, Some(CacheResponseResource::new(file)))));
-      }
-      Some((cache_meta, None)) => {
-        return Ok(Some((cache_meta, None)));
+        Ok(Some((cache_meta, Some(CacheResponseResource::new(file)))))
       }
-      None => return Ok(None),
+      Some((cache_meta, None)) => Ok(Some((cache_meta, None))),
+      None => Ok(None),
     }
   }
 
   async fn delete(
     &self,
     request: CacheDeleteRequest,
-  ) -> Result<bool, AnyError> {
+  ) -> Result<bool, CacheError> {
     let db = self.connection.clone();
     spawn_blocking(move || {
       // TODO(@satyarohith): remove the response body from disk if one exists
@@ -311,17 +319,17 @@ impl Cache for SqliteBackedCache {
         "DELETE FROM request_response_list WHERE cache_id = ?1 AND request_url = ?2",
         (request.cache_id, &request.request_url),
       )?;
-      Ok::<bool, AnyError>(rows_effected > 0)
+      Ok::<bool, CacheError>(rows_effected > 0)
     })
     .await?
   }
 }
 
 async fn insert_cache_asset(
-  db: Arc<Mutex<rusqlite::Connection>>,
+  db: Arc<Mutex<Connection>>,
   put: CachePutRequest,
   response_body_key: Option<String>,
-) -> Result<Option<String>, deno_core::anyhow::Error> {
+) -> Result<Option<String>, CacheError> {
   spawn_blocking(move || {
     let maybe_response_body = {
       let db = db.lock();
@@ -339,15 +347,15 @@ async fn insert_cache_asset(
           response_body_key,
           put.response_status,
           put.response_status_text,
-          SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(),
+          SystemTime::now().duration_since(UNIX_EPOCH).expect("SystemTime is before unix epoch").as_secs(),
         ),
         |row| {
           let response_body_key: Option<String> = row.get(0)?;
           Ok(response_body_key)
         },
       )?
     };
-      Ok::<Option<String>, AnyError>(maybe_response_body)
+    Ok::<Option<String>, CacheError>(maybe_response_body)
   }).await?
 }
 
diff --git a/runtime/errors.rs b/runtime/errors.rs
@@ -10,6 +10,7 @@
 //!   exceptions.
 
 use deno_broadcast_channel::BroadcastChannelError;
+use deno_cache::CacheError;
 use deno_core::error::AnyError;
 use deno_core::serde_json;
 use deno_core::url;
@@ -154,6 +155,18 @@ pub fn get_nix_error_class(error: &nix::Error) -> &'static str {
   }
 }
 
+pub fn get_cache_error(error: &CacheError) -> &'static str {
+  match error {
+    CacheError::Sqlite(_) => "Error",
+    CacheError::JoinError(_) => "Error",
+    CacheError::Resource(err) => {
+      deno_core::error::get_custom_error_class(err).unwrap_or("Error")
+    }
+    CacheError::Other(e) => get_error_class_name(e).unwrap_or("Error"),
+    CacheError::Io(err) => get_io_error_class(err),
+  }
+}
+
 fn get_broadcast_channel_error(error: &BroadcastChannelError) -> &'static str {
   match error {
     BroadcastChannelError::Resource(err) => {
@@ -173,6 +186,7 @@ pub fn get_error_class_name(e: &AnyError) -> Option<&'static str> {
     .or_else(|| deno_web::get_error_class_name(e))
     .or_else(|| deno_webstorage::get_not_supported_error_class_name(e))
     .or_else(|| deno_websocket::get_network_error_class_name(e))
+    .or_else(|| e.downcast_ref::<CacheError>().map(get_cache_error))
     .or_else(|| {
       e.downcast_ref::<BroadcastChannelError>()
         .map(get_broadcast_channel_error)
