error handling

Author: Heliozoa

.github/workflows/test.yml

@@ -32,7 +32,7 @@ jobs:
 
   windows:
     runs-on: windows-latest
-    steps: # TODO: install valgrind and check?
+    steps: # TODO: install missing dependencies and run all tests on windows
       - uses: actions/checkout@v2
       - name: Build test binary
         run: cargo test --no-run --verbose
@@ -41,7 +41,7 @@ jobs:
 
   macos:
     runs-on: macos-latest
-    steps: # TODO: install valgrind and check?
+    steps: # TODO: install missing dependencies and run all tests on macos
       - uses: actions/checkout@v2
       - name: Build test binary
         run: cargo test --no-run --verbose

tmc-langs-core/src/error.rs

@@ -27,12 +27,20 @@ pub enum CoreError {
     // network
     #[error("HTTP error {1} for {0}: {2}")]
     HttpStatus(Url, StatusCode, String),
-    #[error("OAuth2 password exchange error: {0}")]
-    Token(Box<TokenError>),
+    #[error("OAuth2 password exchange error")]
+    Token(#[source] Box<TokenError>),
     #[error("OAuth2 unexpected token response: {0}")]
     TokenParse(String, #[source] serde_json::error::Error),
     #[error("Failed to parse as URL: {0}")]
     UrlParse(String, #[source] url::ParseError),
+    #[error("Failed to GET {0}")]
+    HttpGet(Url, #[source] reqwest::Error),
+    #[error("Failed to POST {0}")]
+    HttpPost(Url, #[source] reqwest::Error),
+    #[error("Failed to write response to {0}")]
+    HttpWriteResponse(PathBuf, #[source] reqwest::Error),
+    #[error("Failed to deserialize response as JSON")]
+    HttpJsonResponse(#[source] reqwest::Error),
 
     #[error("Already authenticated")]
     AlreadyAuthenticated,
@@ -44,13 +52,9 @@ pub enum CoreError {
     #[error(transparent)]
     TmcLangs(#[from] tmc_langs_util::Error),
     #[error(transparent)]
-    Request(#[from] reqwest::Error),
-    #[error(transparent)]
     Response(#[from] response::ResponseError),
     #[error(transparent)]
     ResponseErrors(#[from] response::ResponseErrors),
-    #[error(transparent)]
-    SystemTime(#[from] std::time::SystemTimeError),
 }
 
 impl From<TokenError> for CoreError {

tmc-langs-core/src/tmc_core/api.rs

@@ -31,14 +31,14 @@ trait CoreExt {
 impl CoreExt for ReqwestResponse {
     #[cfg(not(test))]
     fn json_res<T: DeserializeOwned>(self) -> Result<T> {
-        let res: Response<T> = self.json()?;
+        let res: Response<T> = self.json().map_err(|e| CoreError::HttpJsonResponse(e))?;
         res.into_result()
     }
 
     // logs received JSON for easier debugging in tests
     #[cfg(test)]
     fn json_res<T: DeserializeOwned>(self) -> Result<T> {
-        let res: Value = self.json()?;
+        let res: Value = self.json().map_err(|e| CoreError::HttpJsonResponse(e))?;
         log::debug!("JSON {}", res);
         let res: Response<T> = serde_json::from_value(res).unwrap();
         res.into_result()
@@ -86,7 +86,8 @@ impl TmcCore {
         self.client
             .get(url.clone())
             .authenticate(&self.token)
-            .send()?
+            .send()
+            .map_err(|e| CoreError::HttpGet(url.clone(), e))?
             .check_error(url)?
             .json_res()
     }
@@ -104,9 +105,11 @@ impl TmcCore {
         self.client
             .get(url.clone())
             .authenticate(&self.token)
-            .send()?
+            .send()
+            .map_err(|e| CoreError::HttpGet(url.clone(), e))?
             .check_error(url)?
-            .copy_to(&mut target_file)?;
+            .copy_to(&mut target_file)
+            .map_err(|e| CoreError::HttpWriteResponse(target.to_path_buf(), e))?;
         Ok(())
     }
 
@@ -118,9 +121,11 @@ impl TmcCore {
         self.client
             .get(url.clone())
             .authenticate(&self.token)
-            .send()?
+            .send()
+            .map_err(|e| CoreError::HttpGet(url.clone(), e))?
             .check_error(url)?
-            .copy_to(&mut target_file)?;
+            .copy_to(&mut target_file)
+            .map_err(|e| CoreError::HttpWriteResponse(target.to_path_buf(), e))?;
         Ok(())
     }
 
@@ -552,11 +557,19 @@ impl TmcCore {
         form = form
             .text(
                 "client_time",
-                SystemTime::UNIX_EPOCH.elapsed()?.as_secs().to_string(),
+                SystemTime::UNIX_EPOCH
+                    .elapsed()
+                    .unwrap()
+                    .as_secs()
+                    .to_string(),
             )
             .text(
                 "client_nanotime",
-                SystemTime::UNIX_EPOCH.elapsed()?.as_nanos().to_string(),
+                SystemTime::UNIX_EPOCH
+                    .elapsed()
+                    .unwrap()
+                    .as_nanos()
+                    .to_string(),
             )
             .file("submission[file]", submission)
             .map_err(|e| CoreError::FileOpen(submission.to_path_buf(), e))?;
@@ -573,7 +586,8 @@ impl TmcCore {
             .post(submission_url.clone())
             .multipart(form)
             .authenticate(&self.token)
-            .send()?
+            .send()
+            .map_err(|e| CoreError::HttpPost(submission_url.clone(), e))?
             .check_error(submission_url)?
             .json_res()?;
         log::debug!("received {:?}", res);
@@ -612,7 +626,8 @@ impl TmcCore {
             .post(feedback_url.clone())
             .multipart(form)
             .authenticate(&self.token)
-            .send()?
+            .send()
+            .map_err(|e| CoreError::HttpPost(feedback_url.clone(), e))?
             .check_error(feedback_url)?
             .json_res()
     }
@@ -636,7 +651,8 @@ impl TmcCore {
             .query(&[("review[review_body]", review_body)])
             .query(&[("review[points]", review_points)])
             .authenticate(&self.token)
-            .send()?
+            .send()
+            .map_err(|e| CoreError::HttpPost(url.clone(), e))?
             .check_error(url)?
             .json_res()?;
         log::trace!("received {:?}", res);
@@ -657,7 +673,8 @@ impl TmcCore {
         self.client
             .post(url.clone())
             .multipart(form)
-            .send()?
+            .send()
+            .map_err(|e| CoreError::HttpPost(url.clone(), e))?
             .check_error(url)?
             .json_res()
     }

tmc-langs-framework/src/domain/meta_syntax.rs

@@ -1,7 +1,7 @@
 //! Contains utilities for parsing text files, separating lines into
 //! strings, stubs and solutions so that they can be more easily filtered accordingly
 
-use crate::Result;
+use crate::{Error, Result};
 use lazy_static::lazy_static;
 use log::debug;
 use regex::{Captures, Regex};
@@ -183,7 +183,7 @@ impl<B: BufRead> Iterator for MetaSyntaxParser<B> {
                     Some(Ok(MetaString::String(s)))
                 }
             }
-            Err(err) => Some(Err(err.into())),
+            Err(err) => Some(Err(Error::ReadLine(err))),
         }
     }
 }

tmc-langs-framework/src/error.rs

@@ -1,6 +1,7 @@
 use crate::io::zip;
 
 use std::path::PathBuf;
+use std::time::Duration;
 use thiserror::Error;
 
 #[derive(Error, Debug)]
@@ -10,15 +11,37 @@ pub enum Error {
     OpenFile(PathBuf, #[source] std::io::Error),
     #[error("Failed to create file at {0}")]
     CreateFile(PathBuf, #[source] std::io::Error),
+    #[error("Failed to remove file at {0}")]
+    RemoveFile(PathBuf, #[source] std::io::Error),
     #[error("Failed to create dir(s) at {0}")]
     CreateDir(PathBuf, #[source] std::io::Error),
+    #[error("Failed to remove dir at {0}")]
+    RemoveDir(PathBuf, #[source] std::io::Error),
     #[error("Failed to rename {0} to {1}")]
     Rename(PathBuf, PathBuf, #[source] std::io::Error),
     #[error("Failed to write to {0}")]
     Write(PathBuf, #[source] std::io::Error),
+    #[error("Error appending to tar")]
+    TarAppend(#[source] std::io::Error),
+    #[error("Error finishing tar")]
+    TarFinish(#[source] std::io::Error),
+    #[error("Failed to read line")]
+    ReadLine(#[source] std::io::Error),
+    #[error("Failed to copy file from {0} to {1}")]
+    FileCopy(PathBuf, PathBuf, #[source] std::io::Error),
+    #[error("Failed to open file at {0}")]
+    FileOpen(PathBuf, #[source] std::io::Error),
+    #[error("Failed to read file at {0}")]
+    FileRead(PathBuf, #[source] std::io::Error),
+    #[error("Failed to canonicalize path {0}")]
+    Canonicalize(PathBuf, #[source] std::io::Error),
+    #[error("Error occurred in a child process")]
+    Process(#[source] std::io::Error),
 
     #[error("Path {0} contained invalid UTF8")]
     UTF8(PathBuf),
+    #[error("Path {0} contained no file name")]
+    NoFileName(PathBuf),
 
     #[error("No matching plugin found for {0}")]
     PluginNotFound(PathBuf),
@@ -29,14 +52,12 @@ pub enum Error {
 
     #[error("Failed to spawn command: {0}")]
     CommandSpawn(&'static str, #[source] std::io::Error),
-    #[error("Test timed out")]
-    TestTimeout,
+    #[error("Test timed out after {} seconds", .0.as_secs())]
+    TestTimeout(Duration),
 
     #[error("Error in plugin: {0}")]
     Plugin(#[source] Box<dyn std::error::Error + 'static + Send + Sync>),
 
-    #[error(transparent)]
-    Io(#[from] std::io::Error),
     #[error(transparent)]
     YamlDeserialization(#[from] serde_yaml::Error),
     #[error(transparent)]

tmc-langs-framework/src/io/submission_processing.rs

@@ -126,7 +126,8 @@ fn copy_file<F: Fn(&MetaString) -> bool>(
             entry.path(),
             dest_path
         );
-        fs::copy(entry.path(), dest_path)?;
+        fs::copy(entry.path(), &dest_path)
+            .map_err(|e| Error::FileCopy(entry.path().to_path_buf(), dest_path, e))?;
     } else {
         // filter text files
         debug!(

tmc-langs-framework/src/io/zip.rs

@@ -31,10 +31,11 @@ pub fn zip(policy: Box<dyn StudentFilePolicy>, root_directory: &Path) -> Result<
                 log::trace!("adding directory {}", path.display());
                 writer.add_directory_from_path(path, FileOptions::default())?;
             } else {
-                let file = File::open(entry.path())?;
-                let bytes = file
-                    .bytes()
-                    .collect::<std::result::Result<Vec<_>, std::io::Error>>()?;
+                let mut file = File::open(entry.path())
+                    .map_err(|e| Error::FileOpen(entry.path().to_path_buf(), e))?;
+                let mut bytes = vec![];
+                file.read_to_end(&mut bytes)
+                    .map_err(|e| Error::FileRead(entry.path().to_path_buf(), e))?;
                 log::trace!("writing file {}", path.display());
                 writer.start_file_from_path(path, FileOptions::default())?;
                 writer
@@ -62,7 +63,7 @@ pub fn unzip<P: StudentFilePolicy>(policy: P, zip: &Path, target: &Path) -> Resu
     let mut unzipped_paths = HashSet::new();
 
     for i in 0..zip_archive.len() {
-        let file = zip_archive.by_index(i)?;
+        let mut file = zip_archive.by_index(i)?;
         let file_path = file.sanitized_name();
         if !file_path.starts_with(&project_dir) {
             log::trace!("skip {}, not in project dir", file.name());
@@ -76,22 +77,29 @@ pub fn unzip<P: StudentFilePolicy>(policy: P, zip: &Path, target: &Path) -> Resu
             log::trace!("creating {:?}", path_in_target);
             fs::create_dir_all(&path_in_target)
                 .map_err(|e| Error::CreateDir(path_in_target.clone(), e))?;
-            unzipped_paths.insert(path_in_target.canonicalize()?);
+            unzipped_paths.insert(
+                path_in_target
+                    .canonicalize()
+                    .map_err(|e| Error::Canonicalize(path_in_target, e))?,
+            );
         } else {
             let mut write = true;
-            let file_contents = file.bytes().collect::<std::result::Result<Vec<_>, _>>()?;
+            let mut file_contents = vec![];
+            file.read_to_end(&mut file_contents)
+                .map_err(|e| Error::FileRead(file_path, e))?;
             // always overwrite .tmcproject.yml
             if path_in_target.exists()
                 && !path_in_target
                     .file_name()
                     .map(|o| o == ".tmcproject.yml")
                     .unwrap_or_default()
             {
-                let target_file = File::open(&path_in_target)
+                let mut target_file = File::open(&path_in_target)
                     .map_err(|e| Error::OpenFile(path_in_target.clone(), e))?;
-                let target_file_contents = target_file
-                    .bytes()
-                    .collect::<std::result::Result<Vec<_>, _>>()?;
+                let mut target_file_contents = vec![];
+                target_file
+                    .read_to_end(&mut target_file_contents)
+                    .map_err(|e| Error::FileRead(path_in_target.clone(), e))?;
                 if file_contents == target_file_contents
                     || (policy.is_student_file(&path_in_target, &target, &tmc_project_yml)?
                         && !policy.is_updating_forced(&path_in_target, &tmc_project_yml)?)
@@ -101,35 +109,45 @@ pub fn unzip<P: StudentFilePolicy>(policy: P, zip: &Path, target: &Path) -> Resu
             }
             if write {
                 log::trace!("writing to {}", path_in_target.display());
-                if let Some(res) = path_in_target.parent().map(fs::create_dir_all) {
-                    res?;
+                if let Some(res) = path_in_target.parent() {
+                    fs::create_dir_all(res).map_err(|e| Error::CreateDir(res.to_path_buf(), e))?;
                 }
                 let mut overwrite_target = File::create(&path_in_target)
                     .map_err(|e| Error::CreateFile(path_in_target.clone(), e))?;
                 overwrite_target
                     .write_all(&file_contents)
                     .map_err(|e| Error::Write(path_in_target.clone(), e))?;
-                unzipped_paths.insert(path_in_target.canonicalize()?);
+                unzipped_paths.insert(
+                    path_in_target
+                        .canonicalize()
+                        .map_err(|e| Error::Canonicalize(path_in_target, e))?,
+                );
             }
         }
     }
 
     // delete non-student files that were not in zip
     log::debug!("deleting non-student files not in zip");
     for entry in WalkDir::new(target).into_iter().filter_map(|e| e.ok()) {
-        if !unzipped_paths.contains(&entry.path().canonicalize()?)
-            && (policy.is_updating_forced(entry.path(), &tmc_project_yml)?
-                || !policy.is_student_file(entry.path(), &project_dir, &tmc_project_yml)?)
+        if !unzipped_paths.contains(
+            &entry
+                .path()
+                .canonicalize()
+                .map_err(|e| Error::Canonicalize(entry.path().to_path_buf(), e))?,
+        ) && (policy.is_updating_forced(entry.path(), &tmc_project_yml)?
+            || !policy.is_student_file(entry.path(), &project_dir, &tmc_project_yml)?)
         {
             if entry.path().is_dir() {
                 // delete if empty
                 if WalkDir::new(entry.path()).max_depth(1).into_iter().count() == 1 {
                     log::debug!("deleting empty directory {}", entry.path().display());
-                    fs::remove_dir(entry.path())?;
+                    fs::remove_dir(entry.path())
+                        .map_err(|e| Error::RemoveDir(entry.path().to_path_buf(), e))?;
                 }
             } else {
                 log::debug!("removing file {}", entry.path().display());
-                fs::remove_file(entry.path())?;
+                fs::remove_file(entry.path())
+                    .map_err(|e| Error::RemoveFile(entry.path().to_path_buf(), e))?;
             }
         }
     }