PR comments

packages/server/src/core/bank_rule/specialization_groups.rs

@@ -188,8 +188,7 @@ fn run_exhaustive_search(
         &courses,
         &mut best_match,
     )
-    .or(Some(best_match))
-    .unwrap() // unwraping is safe since the line above always returns Some(_)
+    .unwrap_or(best_match)
 }
 
 impl<'a> BankRuleHandler<'a> {

packages/server/src/core/catalog_validations/mod.rs → packages/server/src/core/catalog_validations.rs

@@ -3,10 +3,6 @@ use crate::{error::AppError, resources::catalog::Catalog};
 use super::credit_transfer_graph::validate_acyclic_credit_transfer_graph;
 
 pub fn validate_catalog(catalog: &Catalog) -> Result<(), AppError> {
-    validate_acyclic_credit_transfer_graph(&catalog)?;
+    validate_acyclic_credit_transfer_graph(catalog)?;
     Ok(())
 }
-
-#[allow(clippy::float_cmp)]
-#[cfg(test)]
-pub mod tests;

packages/server/src/core/credit_transfer_graph.rs

@@ -11,37 +11,54 @@ use super::types::CreditOverflow;
 fn build_credit_transfer_graph(
     course_banks: &[CourseBank],
     credit_overflow_rules: &[CreditOverflow],
-) -> Graph<String, ()> {
+) -> Result<Graph<String, ()>, AppError> {
     let mut g = Graph::<String, ()>::new();
     for course_bank in course_banks {
         g.add_node(course_bank.name.clone());
     }
     for credit_rule in credit_overflow_rules {
-        g.add_edge(
-            // unwrap cannot fail because the credit rules are taken from bank names
-            g.node_indices()
-                .find(|i| g[*i] == credit_rule.from)
-                .unwrap(),
-            g.node_indices().find(|i| g[*i] == credit_rule.to).unwrap(),
-            (),
-        );
+        match (
+            g.node_indices().find(|i| g[*i] == credit_rule.from),
+            g.node_indices().find(|i| g[*i] == credit_rule.to),
+        ) {
+            (Some(from), Some(to)) => g.add_edge(from, to, ()),
+            (Some(_), None) | (None, Some(_)) | (None, None) => {
+                return Err(AppError::BadRequest(
+                    messages::build_credit_transfer_graph_failed(),
+                ))
+            }
+        };
     }
-    g
+    Ok(g)
 }
 
 pub fn find_traversal_order(catalog: &Catalog) -> Vec<CourseBank> {
-    let g = build_credit_transfer_graph(&catalog.course_banks, &catalog.credit_overflows);
-    let order = toposort(&g, None).unwrap(); // unwrap can't fail because if the catalog exists in the database it means it is valid, i.e no cycles.
+    let g = match build_credit_transfer_graph(&catalog.course_banks, &catalog.credit_overflows) {
+        Ok(graph) => graph,
+        Err(_) => {
+            log::error!("{}", messages::build_credit_transfer_graph_failed());
+            // Build graph failed, return empty vector
+            return vec![];
+        }
+    };
+    let order = toposort(&g, None).unwrap_or_else(|_| {
+        log::error!(
+            "{}",
+            "corrupted catalog in the database - course banks will be set in an arbitrary order"
+        );
+        g.node_indices().into_iter().collect::<Vec<_>>()
+    });
     let mut ordered_course_banks = Vec::<CourseBank>::new();
     for node in order {
-        // unwrap cannot fail because the graph was built from bank names
-        ordered_course_banks.push(catalog.get_course_bank_by_name(&g[node]).unwrap().clone());
+        if let Some(bank) = catalog.get_course_bank_by_name(&g[node]) {
+            ordered_course_banks.push(bank.clone());
+        }
     }
     ordered_course_banks
 }
 
 pub fn validate_acyclic_credit_transfer_graph(catalog: &Catalog) -> Result<(), AppError> {
-    let g = build_credit_transfer_graph(&catalog.course_banks, &catalog.credit_overflows);
+    let g = build_credit_transfer_graph(&catalog.course_banks, &catalog.credit_overflows)?;
     match toposort(&g, None) {
         Ok(_) => Ok(()),
         Err(e) => Err(AppError::BadRequest(

packages/server/src/core/messages.rs

@@ -1,3 +1,5 @@
+use std::fmt::Write as _;
+
 const ZERO: f32 = 0.0;
 const HALF: f32 = 0.5;
 const SINGLE: f32 = 1.0;
@@ -61,7 +63,7 @@ pub fn completed_chain_msg(chain: &[String]) -> String {
         if course == chain.last().unwrap() {
             msg += course;
         } else {
-            msg += &format!("{}, ", course);
+            let _ = write!(msg, "{}, ", course);
         }
     }
     msg
@@ -79,7 +81,7 @@ pub fn completed_specialization_groups_msg(groups: &[String], needed: usize) ->
         if group == groups.last().unwrap() {
             msg += group;
         } else {
-            msg += &format!("{}, ", group);
+            let _ = write!(msg, "{}, ", group);
         }
     }
     msg
@@ -107,7 +109,11 @@ pub fn cannot_find_course() -> String {
 
 pub fn cyclic_credit_transfer_graph(bank_in_cycle: &str) -> String {
     format!(
-        "קיימת תלות מעגלית במעברי הנקודות שמכילה את הבנק {}",
+        "קיימת תלות מעגלית במעברי הנקודות שנקבעו. התלות המעגלית מתחילה ונגמרת ב{}",
         bank_in_cycle
     )
 }
+
+pub fn build_credit_transfer_graph_failed() -> String {
+    "בניית הגרף נכשלה".to_string()
+}

packages/server/src/core/tests.rs

@@ -1,7 +1,9 @@
 use crate::config::CONFIG;
 use crate::core::bank_rule::BankRuleHandler;
+use crate::core::catalog_validations::validate_catalog;
 use crate::core::degree_status::DegreeStatus;
 use crate::core::parser;
+use crate::core::types::CreditOverflow;
 use crate::resources::catalog::Catalog;
 use crate::resources::course::CourseState::NotComplete;
 use crate::resources::course::Grade::Numeric;
@@ -430,7 +432,7 @@ async fn test_duplicated_courses() {
 // Test core function in a full flow
 // ------------------------------------------------------------------------------------------------------
 
-pub async fn get_catalog(catalog: &str) -> Catalog {
+async fn get_catalog(catalog: &str) -> Catalog {
     dotenv().ok();
     let client = init_mongodb_client!();
     let obj_id = bson::oid::ObjectId::from_str(catalog).expect("failed to create oid");
@@ -785,3 +787,28 @@ async fn test_software_engineer_itinerary() {
         messages::credit_leftovers_msg(0.0)
     );
 }
+
+// ------------------------------------------------------------------------------------------------------
+// Test catalog validations
+// ------------------------------------------------------------------------------------------------------
+
+#[test]
+async fn test_catalog_validations() {
+    let mut catalog = get_catalog(COMPUTER_SCIENCE_3_YEARS_18_19_CATALOG_ID).await;
+    assert!(validate_catalog(&catalog).is_ok());
+
+    // Add credit transfer between בחירה חופשית to רשימה א to close a cycle
+    catalog.credit_overflows.push(CreditOverflow {
+        from: "בחירה חופשית".to_string(),
+        to: "רשימה א".to_string(),
+    });
+
+    let result = validate_catalog(&catalog);
+    assert!(result.is_err());
+    if let Err(e) = result {
+        assert_eq!(
+            e.to_string(),
+            messages::cyclic_credit_transfer_graph("רשימה א")
+        )
+    }
+}

packages/server/src/resources/catalog.rs

@@ -39,7 +39,7 @@ impl Catalog {
     }
 
     pub fn get_bank_traversal_order(&self) -> Vec<CourseBank> {
-        find_traversal_order(&self)
+        find_traversal_order(self)
     }
 
     pub fn get_all_course_ids(&self) -> Vec<CourseId> {