Refactor request tracker following PR feedback (#9755)

Co-authored-by: pyamada (Remote Dev Environment) <pyamada@atlassian.com>

crates/parcel_config/src/parcel_rc_config_loader.rs

@@ -42,13 +42,8 @@ impl ParcelRcConfigLoader {
   fn find_config(&self, project_root: &Path, path: &PathBuf) -> Result<PathBuf, ConfigError> {
     let from = path.parent().unwrap_or(path);
 
-    find_ancestor_file(
-      Rc::clone(&self.fs),
-      vec![String::from(".parcelrc")],
-      from,
-      project_root,
-    )
-    .ok_or(ConfigError::MissingParcelRc(PathBuf::from(from)))
+    find_ancestor_file(&*self.fs, &[".parcelrc"], from, project_root)
+      .ok_or(ConfigError::MissingParcelRc(PathBuf::from(from)))
   }
 
   fn resolve_from(&self, project_root: &PathBuf) -> PathBuf {

crates/parcel_core/src/plugin/plugin_config.rs

@@ -29,8 +29,8 @@ impl PluginConfig {
     filename: &str,
   ) -> Result<(PathBuf, Config), anyhow::Error> {
     let config_path = find_ancestor_file(
-      Rc::clone(&self.fs),
-      vec![String::from(filename)],
+      &*self.fs,
+      &[filename],
       &self.search_path,
       &self.project_root,
     )

crates/parcel_core/src/request_tracker/mod.rs

@@ -3,7 +3,7 @@ mod request_graph;
 mod request_tracker;
 
 #[cfg(test)]
-mod _test;
+mod test;
 
 pub use self::request::*;
 pub use self::request_graph::*;

crates/parcel_core/src/request_tracker/request.rs

@@ -7,6 +7,33 @@ use dyn_hash::DynHash;
 
 use super::RequestTracker;
 
+pub struct RunRequestContext<'a, T> {
+  parent_request_hash: Option<u64>,
+  request_tracker: &'a mut RequestTracker<T>,
+}
+
+impl<'a, T: Clone> RunRequestContext<'a, T> {
+  pub(crate) fn new(
+    parent_request_hash: Option<u64>,
+    request_tracker: &'a mut RequestTracker<T>,
+  ) -> Self {
+    Self {
+      parent_request_hash,
+      request_tracker,
+    }
+  }
+
+  // TODO: Why is this boxed?
+  pub fn run_request(&mut self, request: Box<&dyn Request<T>>) -> anyhow::Result<T> {
+    self
+      .request_tracker
+      .run_child_request(request, self.parent_request_hash)
+  }
+}
+
+// We can type this properly
+pub type RunRequestError = anyhow::Error;
+
 pub trait Request<T: Clone>: DynHash {
   fn id(&self) -> u64 {
     let mut hasher = DefaultHasher::default();
@@ -15,7 +42,8 @@ pub trait Request<T: Clone>: DynHash {
     hasher.finish()
   }
 
-  fn run(&self, request_tracker: RequestTracker<T>) -> Result<RequestResult<T>, Vec<RequestError>>;
+  fn run(&self, request_tracker: RunRequestContext<T>)
+    -> Result<RequestResult<T>, RunRequestError>;
 }
 
 dyn_hash::hash_trait_object!(<T: Clone> Request<T>);

crates/parcel_core/src/request_tracker/request_graph.rs

@@ -1,12 +1,12 @@
 use petgraph::stable_graph::StableDiGraph;
 
-use super::RequestError;
+use super::RunRequestError;
 
 pub type RequestGraph<T> = StableDiGraph<RequestNode<T>, RequestEdgeType>;
 
 #[derive(Debug)]
 pub enum RequestNode<T> {
-  Error(Vec<RequestError>),
+  Error(RunRequestError),
   Root,
   Incomplete,
   Valid(T),

crates/parcel_core/src/request_tracker/request_tracker.rs

@@ -1,113 +1,128 @@
-use std::cell::RefCell;
+use anyhow::anyhow;
 use std::collections::HashMap;
-use std::rc::Rc;
 
 use petgraph::graph::NodeIndex;
 use petgraph::stable_graph::StableDiGraph;
 
-use super::Request;
 use super::RequestEdgeType;
-use super::RequestError;
 use super::RequestGraph;
 use super::RequestNode;
 use super::RequestResult;
+use super::{Request, RunRequestContext, RunRequestError};
 
-#[derive(Clone)]
-pub struct RequestTracker<T: Clone> {
-  parent_request_hash: Option<u64>,
-  graph: Rc<RefCell<RequestGraph<T>>>,
-  request_index: Rc<RefCell<HashMap<u64, NodeIndex>>>,
+pub struct RequestTracker<T> {
+  graph: RequestGraph<T>,
+  request_index: HashMap<u64, NodeIndex>,
 }
 
 impl<T: Clone> RequestTracker<T> {
   pub fn new() -> Self {
     let mut graph = StableDiGraph::<RequestNode<T>, RequestEdgeType>::new();
     graph.add_node(RequestNode::Root);
     RequestTracker {
-      parent_request_hash: None,
-      graph: Rc::new(RefCell::new(graph)),
-      request_index: Rc::new(RefCell::new(HashMap::new())),
+      graph,
+      request_index: HashMap::new(),
     }
   }
 
-  pub fn run_request(&self, request: Box<&dyn Request<T>>) -> Result<T, Vec<RequestError>> {
+  pub fn run_request(&mut self, request: Box<&dyn Request<T>>) -> anyhow::Result<T> {
+    self.run_child_request(request, None)
+  }
+
+  pub fn run_child_request(
+    &mut self,
+    request: Box<&dyn Request<T>>,
+    parent_request_hash: Option<u64>,
+  ) -> anyhow::Result<T> {
     let request_id = request.id();
 
-    if self.prepare_request(request_id.clone()) {
-      let mut rt = self.clone();
-      rt.parent_request_hash.replace(request_id.clone());
-      self.store_request(&request_id, request.run(rt));
+    if self.prepare_request(request_id.clone())? {
+      let result = request.run(RunRequestContext::new(Some(request_id), self));
+      self.store_request(&request_id, result)?;
     }
 
-    self.get_request(&request_id)
+    Ok(self.get_request(parent_request_hash, &request_id)?)
   }
 
-  fn prepare_request(&self, request_id: u64) -> bool {
-    let mut graph = self.graph.borrow_mut();
-    let mut request_index = self.request_index.borrow_mut();
-
-    let node_index = request_index
+  fn prepare_request(&mut self, request_id: u64) -> anyhow::Result<bool> {
+    let node_index = self
+      .request_index
       .entry(request_id)
-      .or_insert_with(|| graph.add_node(RequestNode::Incomplete));
+      .or_insert_with(|| self.graph.add_node(RequestNode::Incomplete));
 
-    let request_node = graph.node_weight_mut(*node_index).unwrap();
+    let request_node = self
+      .graph
+      .node_weight_mut(*node_index)
+      .ok_or_else(|| anyhow!("Failed to find request node"))?;
 
     // Don't run if already run
     if let RequestNode::<T>::Valid(_) = request_node {
-      return false;
+      return Ok(false);
     }
 
     *request_node = RequestNode::Incomplete;
-    true
+    Ok(true)
   }
 
-  fn store_request(&self, request_id: &u64, result: Result<RequestResult<T>, Vec<RequestError>>) {
-    let request_index = self.request_index.borrow();
-    let mut graph = self.graph.borrow_mut();
-
-    let node_index = request_index.get(&request_id).unwrap();
-
-    let request_node = graph.node_weight_mut(*node_index).unwrap();
+  fn store_request(
+    &mut self,
+    request_id: &u64,
+    result: Result<RequestResult<T>, RunRequestError>,
+  ) -> anyhow::Result<()> {
+    let node_index = self
+      .request_index
+      .get(&request_id)
+      .ok_or_else(|| anyhow!("Failed to find request"))?;
+    let request_node = self
+      .graph
+      .node_weight_mut(*node_index)
+      .ok_or_else(|| anyhow!("Failed to find request"))?;
     if let RequestNode::<T>::Valid(_) = request_node {
-      return;
+      return Ok(());
     }
     *request_node = match result {
       Ok(result) => RequestNode::Valid(result.result),
       Err(error) => RequestNode::Error(error),
     };
-  }
 
-  fn get_request(&self, request_id: &u64) -> Result<T, Vec<RequestError>> {
-    let mut graph = self.graph.borrow_mut();
-    let request_index = self.request_index.borrow();
+    Ok(())
+  }
 
-    let Some(node_index) = request_index.get(&request_id) else {
-      return Err(vec![RequestError::Impossible]);
+  fn get_request(
+    &mut self,
+    parent_request_hash: Option<u64>,
+    request_id: &u64,
+  ) -> anyhow::Result<T> {
+    let Some(node_index) = self.request_index.get(&request_id) else {
+      return Err(anyhow!("Impossible error"));
     };
 
-    if let Some(parent_request_id) = self.parent_request_hash {
-      let parent_node_index = request_index.get(&parent_request_id).unwrap();
-      graph.add_edge(
+    if let Some(parent_request_id) = parent_request_hash {
+      let parent_node_index = self
+        .request_index
+        .get(&parent_request_id)
+        .ok_or_else(|| anyhow!("Failed to find requests"))?;
+      self.graph.add_edge(
         parent_node_index.clone(),
         node_index.clone(),
         RequestEdgeType::SubRequest,
       );
     } else {
-      graph.add_edge(
+      self.graph.add_edge(
         NodeIndex::new(0),
         node_index.clone(),
         RequestEdgeType::SubRequest,
       );
     }
 
-    let Some(request_node) = graph.node_weight(node_index.clone()) else {
-      return Err(vec![RequestError::Impossible]);
+    let Some(request_node) = self.graph.node_weight(node_index.clone()) else {
+      return Err(anyhow!("Impossible"));
     };
 
     match request_node {
-      RequestNode::Root => Err(vec![RequestError::Impossible]),
-      RequestNode::Incomplete => Err(vec![RequestError::Impossible]),
-      RequestNode::Error(errors) => Err(errors.to_owned()),
+      RequestNode::Root => Err(anyhow!("Impossible")),
+      RequestNode::Incomplete => Err(anyhow!("Impossible")),
+      RequestNode::Error(_errors) => Err(anyhow!("Impossible")),
       RequestNode::Valid(value) => Ok(value.clone()),
     }
   }

crates/parcel_core/src/request_tracker/_test.rs → crates/parcel_core/src/request_tracker/test.rs

@@ -7,7 +7,7 @@ use super::*;
 
 #[test]
 fn should_run_request() {
-  let rt = RequestTracker::<Vec<String>>::new();
+  let mut rt = RequestTracker::<Vec<String>>::new();
 
   let request_c = TestRequest::new("C", &[]);
   let request_b = TestRequest::new("B", &[&request_c]);
@@ -22,7 +22,7 @@ fn should_run_request() {
 
 #[test]
 fn should_reuse_previously_run_request() {
-  let rt = RequestTracker::<Vec<String>>::new();
+  let mut rt = RequestTracker::<Vec<String>>::new();
 
   let request_c = TestRequest::new("C", &[]);
   let request_b = TestRequest::new("B", &[&request_c]);
@@ -42,7 +42,7 @@ fn should_reuse_previously_run_request() {
 
 #[test]
 fn should_run_request_once() {
-  let rt = RequestTracker::<Vec<String>>::new();
+  let mut rt = RequestTracker::<Vec<String>>::new();
 
   let request_a = TestRequest::new("A", &[]);
 
@@ -58,7 +58,7 @@ fn should_run_request_once() {
 
 #[test]
 fn should_run_request_once_2() {
-  let rt = RequestTracker::<Vec<String>>::new();
+  let mut rt = RequestTracker::<Vec<String>>::new();
 
   let request_b = TestRequest::new("B", &[]);
   let request_a = TestRequest::new("A", &[&request_b]);
@@ -116,8 +116,8 @@ impl<'a> std::hash::Hash for TestRequest<'a> {
 impl<'a> Request<Vec<String>> for TestRequest<'a> {
   fn run(
     &self,
-    rt: RequestTracker<Vec<String>>,
-  ) -> Result<RequestResult<Vec<String>>, Vec<RequestError>> {
+    mut context: RunRequestContext<Vec<String>>,
+  ) -> Result<RequestResult<Vec<String>>, RunRequestError> {
     self.runs.fetch_add(1, Ordering::Relaxed);
 
     let name = self.name.clone();
@@ -127,7 +127,7 @@ impl<'a> Request<Vec<String>> for TestRequest<'a> {
 
     while let Some(subrequest) = subrequets.pop() {
       let req = subrequest.clone();
-      let subrequest_result = rt.run_request(Box::new(&req))?;
+      let subrequest_result = context.run_request(Box::new(&req))?;
       result.extend(subrequest_result);
     }
 

crates/parcel_filesystem/src/search.rs

@@ -1,12 +1,11 @@
 use std::path::Path;
 use std::path::PathBuf;
-use std::rc::Rc;
 
 use crate::FileSystem;
 
 pub fn find_ancestor_file<P: AsRef<Path>>(
-  fs: Rc<dyn FileSystem>,
-  filenames: Vec<String>,
+  fs: &dyn FileSystem,
+  filenames: &[&str],
   from: P,
   root: P,
 ) -> Option<PathBuf> {
@@ -18,7 +17,7 @@ pub fn find_ancestor_file<P: AsRef<Path>>(
       }
     }
 
-    for name in &filenames {
+    for name in filenames {
       let fullpath = dir.join(name);
       if fs.is_file(&fullpath) {
         return Some(fullpath);