From 63aef524b040693fafb27d8ec49f88917561910a Mon Sep 17 00:00:00 2001 From: benny-n Date: Sat, 17 Sep 2022 15:09:32 +0300 Subject: [PATCH 1/5] Change jwt-google's test-helper to a Dev dependecy --- packages/server/Cargo.toml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/server/Cargo.toml b/packages/server/Cargo.toml index 31de4402..12a3cfb1 100644 --- a/packages/server/Cargo.toml +++ b/packages/server/Cargo.toml @@ -19,7 +19,7 @@ actix-cors = "0.6" actix-service = "2.0" futures-util = "0.3" jsonwebtoken = "8.0" -jsonwebtoken-google = {version = "0.1", features = ["test-helper"]} +jsonwebtoken-google = "0.1" env_logger = "0.9" toml = "0.5" dotenv = "0.15" @@ -27,5 +27,6 @@ lazy_static = "1.4" petgraph = "0.6" derive_more = "0.99" -[patch.crates-io] -jsonwebtoken-google = { git = 'https://github.com/cheetah-games/jsonwebtoken-google' } +[dev-dependencies.jsonwebtoken-google] +version = "0.1" +features = ["test-helper"] From b772e9fba52e3b62fbcf4641ccd3f55bcf47b847 Mon Sep 17 00:00:00 2001 From: benny-n Date: Sat, 17 Sep 2022 15:10:27 +0300 Subject: [PATCH 2/5] Refactor error.rs to avoid cloning the error --- packages/server/src/error.rs | 53 ++++++++++++++---------------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/packages/server/src/error.rs b/packages/server/src/error.rs index 7865a843..ef1ae08a 100644 --- a/packages/server/src/error.rs +++ b/packages/server/src/error.rs @@ -1,4 +1,4 @@ -use actix_web::{HttpResponse, ResponseError}; +use actix_web::{http::StatusCode, HttpResponse, ResponseError}; use colored::Colorize; use derive_more::Display; @@ -15,38 +15,27 @@ pub enum AppError { impl ResponseError for AppError { fn error_response(&self) -> HttpResponse { - let error; - let resp = match self { - AppError::BadRequest(e) => { - error = e.to_owned(); - HttpResponse::BadRequest().body(error.clone()) - } - AppError::Bson(e) => { - error = format!("Bson error: {}", e); - HttpResponse::BadRequest().body(error.clone()) - } - AppError::Parser(e) => { - error = format!("Parser error: {}", e); - HttpResponse::BadRequest().body(error.clone()) - } - AppError::NotFound(e) => { - error = format!("{} not found", e); - HttpResponse::NotFound().body(error.clone()) - } - AppError::InternalServer(e) => { - error = e.to_owned(); - HttpResponse::InternalServerError().body(error.clone()) - } - AppError::Middleware(e) => { - error = format!("Middleware error: {}", e); - HttpResponse::InternalServerError().body(error.clone()) - } - AppError::MongoDriver(e) => { - error = format!("MongoDB driver error: {}", e); - HttpResponse::InternalServerError().body(error.clone()) - } + let (status_code, error) = match self { + AppError::BadRequest(e) => (StatusCode::BAD_REQUEST, e.to_owned()), + AppError::Bson(e) => (StatusCode::BAD_REQUEST, format!("Bson error: {}", e)), + AppError::Parser(e) => (StatusCode::BAD_REQUEST, format!("Parser error: {}", e)), + AppError::NotFound(e) => (StatusCode::NOT_FOUND, format!("{} not found", e)), + AppError::InternalServer(e) => (StatusCode::INTERNAL_SERVER_ERROR, e.to_owned()), + AppError::Middleware(e) => ( + StatusCode::INTERNAL_SERVER_ERROR, + format!("Middleware error: {}", e), + ), + AppError::MongoDriver(e) => ( + StatusCode::INTERNAL_SERVER_ERROR, + format!("MongoDB driver error: {}", e), + ), }; log::error!("{}", error.bold().red()); - resp + match status_code { + StatusCode::BAD_REQUEST => HttpResponse::BadRequest().body(error), + StatusCode::NOT_FOUND => HttpResponse::NotFound().body(error), + StatusCode::INTERNAL_SERVER_ERROR => HttpResponse::InternalServerError().body(error), + _ => unreachable!(), + } } } From e16e0467ece3d60f85ecb41edeb204e81df378aa Mon Sep 17 00:00:00 2001 From: benny-n Date: Sat, 17 Sep 2022 15:12:29 +0300 Subject: [PATCH 3/5] Remove `Option` from `user_details` in `User` --- packages/server/src/api/students.rs | 89 ++++++++++++--------------- packages/server/src/api/tests.rs | 47 ++------------ packages/server/src/resources/user.rs | 18 +----- 3 files changed, 46 insertions(+), 108 deletions(-) diff --git a/packages/server/src/api/students.rs b/packages/server/src/api/students.rs index 8c83d257..b74251c6 100644 --- a/packages/server/src/api/students.rs +++ b/packages/server/src/api/students.rs @@ -5,7 +5,7 @@ use actix_web::{ web::{Data, Json, Query}, HttpMessage, HttpRequest, HttpResponse, }; -use bson::doc; +use bson::{doc, to_bson}; use crate::{ core::{degree_status::DegreeStatus, parser}, @@ -40,8 +40,15 @@ pub async fn login( .get::() .ok_or_else(|| AppError::Middleware("No sub found in request extensions".into()))?; - let document = doc! {"$setOnInsert" : User::new_document(user_id)}; + let user = User { + sub: user_id.to_string(), + ..Default::default() + }; + let document = + doc! {"$setOnInsert" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}; + let updated_user = db::services::find_and_update_user(user_id, document, &client).await?; + Ok(HttpResponse::Ok().json(updated_user)) } @@ -51,24 +58,19 @@ pub async fn add_catalog( catalog_id: String, client: Data, ) -> Result { - match &mut user.details { - Some(details) => { - let obj_id = bson::oid::ObjectId::from_str(&catalog_id) - .map_err(|e| AppError::Bson(e.to_string()))?; - let catalog = db::services::get_catalog_by_id(&obj_id, &client).await?; - details.catalog = Some(DisplayCatalog::from(catalog)); - details.degree_status = DegreeStatus::default(); - details.modified = true; - let updated_user = db::services::find_and_update_user( - &user.sub.clone(), - doc! {"$set" : user.into_document()}, - &client, - ) - .await?; - Ok(HttpResponse::Ok().json(updated_user)) - } - None => Err(AppError::InternalServer("No data exists for user".into())), - } + let obj_id = + bson::oid::ObjectId::from_str(&catalog_id).map_err(|e| AppError::Bson(e.to_string()))?; + let catalog = db::services::get_catalog_by_id(&obj_id, &client).await?; + user.details.catalog = Some(DisplayCatalog::from(catalog)); + user.details.degree_status = DegreeStatus::default(); + user.details.modified = true; + let updated_user = db::services::find_and_update_user( + &user.sub.clone(), + doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}, + &client, + ) + .await?; + Ok(HttpResponse::Ok().json(updated_user)) } #[get("/students/courses")] @@ -99,21 +101,16 @@ pub async fn add_courses( data: String, client: Data, ) -> Result { - match &mut user.details { - Some(details) => { - details.degree_status = DegreeStatus::default(); - details.degree_status.course_statuses = parser::parse_copy_paste_data(&data)?; - details.modified = true; - let updated_user = db::services::find_and_update_user( - &user.sub.clone(), - doc! {"$set" : user.into_document()}, - &client, - ) - .await?; - Ok(HttpResponse::Ok().json(updated_user)) - } - None => Err(AppError::InternalServer("No data exists for user".into())), - } + user.details.degree_status = DegreeStatus::default(); + user.details.degree_status.course_statuses = parser::parse_copy_paste_data(&data)?; + user.details.modified = true; + let updated_user = db::services::find_and_update_user( + &user.sub.clone(), + doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}, + &client, + ) + .await?; + Ok(HttpResponse::Ok().json(updated_user)) } // here "modified" becomes false @@ -122,12 +119,8 @@ pub async fn compute_degree_status( mut user: User, client: Data, ) -> Result { - let mut user_details = user + let catalog_id = user .details - .as_mut() - .ok_or_else(|| AppError::InternalServer("No data exists for user".into()))?; - - let catalog_id = user_details .catalog .as_ref() .ok_or_else(|| AppError::InternalServer("No catalog chosen for user".into()))? @@ -135,7 +128,7 @@ pub async fn compute_degree_status( let catalog = db::services::get_catalog_by_id(&catalog_id, &client).await?; - user_details.modified = false; + user.details.modified = false; let vec_courses = db::services::get_courses_by_ids(catalog.get_all_course_ids(), &client).await?; @@ -145,18 +138,18 @@ pub async fn compute_degree_status( let mut course_list = Vec::new(); if user.settings.compute_in_progress { - course_list = user_details.degree_status.set_in_progress_to_complete(); + course_list = user.details.degree_status.set_in_progress_to_complete(); } - user_details + user.details .degree_status .compute(catalog, course::vec_to_map(vec_courses), malag_courses); if user.settings.compute_in_progress { - user_details.degree_status.set_to_in_progress(course_list); + user.details.degree_status.set_to_in_progress(course_list); } let user_id = user.sub.clone(); - let document = doc! {"$set" : user.clone().into_document()}; + let document = doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}; db::services::find_and_update_user(&user_id, document, &client).await?; Ok(HttpResponse::Ok().json(user)) } @@ -169,8 +162,8 @@ pub async fn update_details( client: Data, ) -> Result { let user_id = user.sub.clone(); - user.details = Some(details.into_inner()); - let document = doc! {"$set" : user.into_document()}; + user.details = details.into_inner(); + let document = doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}; db::services::find_and_update_user(&user_id, document, &client).await?; Ok(HttpResponse::Ok().finish()) } @@ -183,7 +176,7 @@ pub async fn update_settings( ) -> Result { let user_id = user.sub.clone(); user.settings = settings.into_inner(); - let document = doc! {"$set" : user.into_document()}; + let document = doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}; db::services::find_and_update_user(&user_id, document, &client).await?; Ok(HttpResponse::Ok().finish()) } diff --git a/packages/server/src/api/tests.rs b/packages/server/src/api/tests.rs index 89164b4b..2142377e 100644 --- a/packages/server/src/api/tests.rs +++ b/packages/server/src/api/tests.rs @@ -87,7 +87,7 @@ async fn test_students_api_full_flow() { .await; assert!(res.status().is_success()); let user: User = test::read_body_json(res).await; - let mut user_details: UserDetails = user.details.expect("No user details"); + let mut user_details: UserDetails = user.details; // get /catalogs res = test::TestRequest::get() @@ -165,7 +165,7 @@ async fn test_compute_in_progress() { let res = test::call_service(&app, get_degree_status_before).await; let mut user: User = test::read_body_json(res).await; - assert_eq!(user.details.unwrap().degree_status.total_credit, 0.0); + assert_eq!(user.details.degree_status.total_credit, 0.0); user.settings.compute_in_progress = true; let put_user_settings = test::TestRequest::put() @@ -188,7 +188,7 @@ async fn test_compute_in_progress() { let res = test::call_service(&app, get_degree_status_after).await; let mut user: User = test::read_body_json(res).await; - assert_eq!(user.details.unwrap().degree_status.total_credit, 2.5); + assert_eq!(user.details.degree_status.total_credit, 2.5); user.settings.compute_in_progress = false; let put_user_settings = test::TestRequest::put() @@ -328,45 +328,6 @@ async fn test_student_login_no_sub() { ); } -#[test] -async fn test_students_api_no_user_details() { - // *** IMPORTANT: This should NEVER happen, but the tests are added anyway for coverage - dotenv().ok(); - // Init env and app - let client = init_mongodb_client!(); - let app = test::init_service( - App::new() - .app_data(Data::new(client.clone())) - .service(students::add_courses) - .service(students::add_catalog) - .service(students::compute_degree_status), - ) - .await; - let post_courses = test::TestRequest::post() - .uri("/students/courses") - .to_request(); - let put_catalog = test::TestRequest::put() - .uri("/students/catalog") - .to_request(); - let get_degree_status = test::TestRequest::get() - .uri("/students/degree-status") - .to_request(); - - let requests = vec![post_courses, put_catalog, get_degree_status]; - for request in requests.into_iter() { - // Manually insert a sub of a fake user with no user details - request - .extensions_mut() - .insert::("bugo-the-debugo".to_string()); - let resp = test::call_service(&app, request).await; - assert!(resp.status().is_server_error()); - assert_eq!( - Bytes::from("No data exists for user"), - test::read_body(resp).await - ); - } -} - #[test] async fn test_students_api_no_catalog() { // *** IMPORTANT: This should NEVER happen, but the tests are added anyway for coverage @@ -383,7 +344,7 @@ async fn test_students_api_no_catalog() { .uri("/students/degree-status") .to_request(); - // Manually insert a sub of a fake user with no user details + // Manually insert a sub of a fake user with no catalog request .extensions_mut() .insert::("bugo-the-debugo-junior".to_string()); diff --git a/packages/server/src/resources/user.rs b/packages/server/src/resources/user.rs index 5f3388d0..9c714df9 100644 --- a/packages/server/src/resources/user.rs +++ b/packages/server/src/resources/user.rs @@ -27,24 +27,8 @@ pub struct UserSettings { pub struct User { #[serde(rename(serialize = "_id", deserialize = "_id"))] pub sub: String, - pub details: Option, + pub details: UserDetails, pub settings: UserSettings, } -impl User { - pub fn new_document(sub: &str) -> bson::Document { - let user = User { - sub: sub.into(), - details: Some(UserDetails::default()), - ..Default::default() - }; - // Should always unwrap successfully here.. - bson::to_document(&user).unwrap_or(doc! {"sub" : sub, "details": null}) - } - pub fn into_document(self) -> bson::Document { - // Should always unwrap successfully here.. - bson::to_document(&self).unwrap_or(doc! {"sub" : self.sub, "details": null}) - } -} - impl_from_request!(resource = User, getter = get_user_by_id); From 6a86164100feeb65c3e80c1bce8598eaa38f6175 Mon Sep 17 00:00:00 2001 From: benny-n Date: Sat, 17 Sep 2022 15:59:49 +0300 Subject: [PATCH 4/5] Introduce 'Db' struct and refactor db handling --- packages/server/src/api/bo.rs | 33 ++- packages/server/src/api/students.rs | 73 +++--- packages/server/src/api/tests.rs | 47 ++-- packages/server/src/core/tests.rs | 17 +- packages/server/src/db/mod.rs | 30 ++- packages/server/src/db/services.rs | 232 +++++++++--------- packages/server/src/db/tests.rs | 28 ++- packages/server/src/main.rs | 8 +- .../server/src/middleware/from_request.rs | 6 +- packages/server/src/middleware/tests.rs | 11 +- packages/server/src/resources/admin.rs | 2 +- packages/server/src/resources/user.rs | 2 +- 12 files changed, 239 insertions(+), 250 deletions(-) diff --git a/packages/server/src/api/bo.rs b/packages/server/src/api/bo.rs index 6085baa1..a9e08a28 100644 --- a/packages/server/src/api/bo.rs +++ b/packages/server/src/api/bo.rs @@ -1,11 +1,9 @@ use std::str::FromStr; +use crate::db::Db; use crate::error::AppError; use crate::resources::catalog::Catalog; -use crate::{ - db, - resources::{admin::Admin, course::Course}, -}; +use crate::resources::{admin::Admin, course::Course}; use actix_web::web::{Data, Json, Path}; use actix_web::{delete, get, put, HttpResponse}; use bson::doc; @@ -15,11 +13,8 @@ use bson::doc; ///////////////////////////////////////////////////////////////////////////// #[get("/courses")] -pub async fn get_all_courses( - _: Admin, - client: Data, -) -> Result { - db::services::get_all_courses(&client) +pub async fn get_all_courses(_: Admin, db: Data) -> Result { + db.get_all_courses() .await .map(|courses| HttpResponse::Ok().json(courses)) } @@ -28,9 +23,9 @@ pub async fn get_all_courses( pub async fn get_course_by_id( _: Admin, id: Path, - client: Data, + db: Data, ) -> Result { - db::services::get_course_by_id(&id, &client) + db.get_course_by_id(&id) .await .map(|course| HttpResponse::Ok().json(course)) } @@ -40,11 +35,11 @@ pub async fn create_or_update_course( _: Admin, id: Path, course: Json, - client: Data, + db: Data, ) -> Result { let course_doc = bson::to_document(&course).map_err(|e| AppError::Bson(e.to_string()))?; let document = doc! {"$setOnInsert" : course_doc}; - db::services::find_and_update_course(&id, document, &client) + db.find_and_update_course(&id, document) .await .map(|course| HttpResponse::Ok().json(course)) } @@ -53,9 +48,9 @@ pub async fn create_or_update_course( pub async fn delete_course( _: Admin, id: Path, - client: Data, + db: Data, ) -> Result { - db::services::delete_course(&id, &client) + db.delete_course(&id) .await .map(|_| HttpResponse::Ok().finish()) } @@ -68,10 +63,10 @@ pub async fn delete_course( pub async fn get_catalog_by_id( _: Admin, id: Path, - client: Data, + db: Data, ) -> Result { let obj_id = bson::oid::ObjectId::from_str(&id).map_err(|e| AppError::Bson(e.to_string()))?; - db::services::get_catalog_by_id(&obj_id, &client) + db.get_catalog_by_id(&obj_id) .await .map(|course| HttpResponse::Ok().json(course)) } @@ -81,12 +76,12 @@ pub async fn create_or_update_catalog( _: Admin, id: Path, catalog: Json, - client: Data, + db: Data, ) -> Result { let obj_id = bson::oid::ObjectId::from_str(&id).map_err(|e| AppError::Bson(e.to_string()))?; let catalog_doc = bson::to_document(&catalog).map_err(|e| AppError::Bson(e.to_string()))?; let document = doc! {"$setOnInsert" : catalog_doc}; - db::services::find_and_update_catalog(&obj_id, document, &client) + db.find_and_update_catalog(&obj_id, document) .await .map(|catalog| HttpResponse::Ok().json(catalog)) } diff --git a/packages/server/src/api/students.rs b/packages/server/src/api/students.rs index b74251c6..60b002e2 100644 --- a/packages/server/src/api/students.rs +++ b/packages/server/src/api/students.rs @@ -9,7 +9,7 @@ use bson::{doc, to_bson}; use crate::{ core::{degree_status::DegreeStatus, parser}, - db, + db::Db, error::AppError, middleware::auth::Sub, resources::{ @@ -22,19 +22,16 @@ use crate::{ #[get("/catalogs")] pub async fn get_all_catalogs( _: User, //TODO think about whether this is necessary - client: Data, + db: Data, ) -> Result { - db::services::get_all_catalogs(&client) + db.get_all_catalogs() .await .map(|catalogs| HttpResponse::Ok().json(catalogs)) } //TODO: maybe this should be "PUT" because it will ALWAYS create a user if one doesn't exist? #[get("/students/login")] -pub async fn login( - client: Data, - req: HttpRequest, -) -> Result { +pub async fn login(db: Data, req: HttpRequest) -> Result { let extensions = req.extensions(); let user_id = extensions .get::() @@ -47,7 +44,7 @@ pub async fn login( let document = doc! {"$setOnInsert" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}; - let updated_user = db::services::find_and_update_user(user_id, document, &client).await?; + let updated_user = db.find_and_update_user(user_id, document).await?; Ok(HttpResponse::Ok().json(updated_user)) } @@ -56,20 +53,20 @@ pub async fn login( pub async fn add_catalog( mut user: User, catalog_id: String, - client: Data, + db: Data, ) -> Result { let obj_id = bson::oid::ObjectId::from_str(&catalog_id).map_err(|e| AppError::Bson(e.to_string()))?; - let catalog = db::services::get_catalog_by_id(&obj_id, &client).await?; + let catalog = db.get_catalog_by_id(&obj_id).await?; user.details.catalog = Some(DisplayCatalog::from(catalog)); user.details.degree_status = DegreeStatus::default(); user.details.modified = true; - let updated_user = db::services::find_and_update_user( - &user.sub.clone(), - doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}, - &client, - ) - .await?; + let updated_user = db + .find_and_update_user( + &user.sub.clone(), + doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}, + ) + .await?; Ok(HttpResponse::Ok().json(updated_user)) } @@ -77,17 +74,17 @@ pub async fn add_catalog( pub async fn get_courses_by_filter( _: User, req: HttpRequest, - client: Data, + db: Data, ) -> Result { let params = Query::>::from_query(req.query_string()) .map_err(|e| AppError::BadRequest(e.to_string()))?; match (params.get("name"), params.get("number")) { (Some(name), None) => { - let courses = db::services::get_courses_filtered_by_name(name, &client).await?; + let courses = db.get_courses_filtered_by_name(name).await?; Ok(HttpResponse::Ok().json(courses)) } (None, Some(number)) => { - let courses = db::services::get_courses_filtered_by_number(number, &client).await?; + let courses = db.get_courses_filtered_by_number(number).await?; Ok(HttpResponse::Ok().json(courses)) } (Some(_), Some(_)) => Err(AppError::BadRequest("Invalid query params".into())), @@ -99,26 +96,23 @@ pub async fn get_courses_by_filter( pub async fn add_courses( mut user: User, data: String, - client: Data, + db: Data, ) -> Result { user.details.degree_status = DegreeStatus::default(); user.details.degree_status.course_statuses = parser::parse_copy_paste_data(&data)?; user.details.modified = true; - let updated_user = db::services::find_and_update_user( - &user.sub.clone(), - doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}, - &client, - ) - .await?; + let updated_user = db + .find_and_update_user( + &user.sub.clone(), + doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}, + ) + .await?; Ok(HttpResponse::Ok().json(updated_user)) } // here "modified" becomes false #[get("/students/degree-status")] -pub async fn compute_degree_status( - mut user: User, - client: Data, -) -> Result { +pub async fn compute_degree_status(mut user: User, db: Data) -> Result { let catalog_id = user .details .catalog @@ -126,15 +120,12 @@ pub async fn compute_degree_status( .ok_or_else(|| AppError::InternalServer("No catalog chosen for user".into()))? .id; - let catalog = db::services::get_catalog_by_id(&catalog_id, &client).await?; + let catalog = db.get_catalog_by_id(&catalog_id).await?; user.details.modified = false; - let vec_courses = - db::services::get_courses_by_ids(catalog.get_all_course_ids(), &client).await?; - let malag_courses = db::services::get_all_malags(&client).await?[0] - .malag_list - .clone(); // The collection malags contain one item with the list of all malags + let vec_courses = db.get_courses_by_ids(catalog.get_all_course_ids()).await?; + let malag_courses = db.get_all_malags().await?[0].malag_list.clone(); // The collection malags contain one item with the list of all malags let mut course_list = Vec::new(); if user.settings.compute_in_progress { @@ -150,7 +141,7 @@ pub async fn compute_degree_status( } let user_id = user.sub.clone(); let document = doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}; - db::services::find_and_update_user(&user_id, document, &client).await?; + db.find_and_update_user(&user_id, document).await?; Ok(HttpResponse::Ok().json(user)) } @@ -159,12 +150,12 @@ pub async fn compute_degree_status( pub async fn update_details( mut user: User, details: Json, - client: Data, + db: Data, ) -> Result { let user_id = user.sub.clone(); user.details = details.into_inner(); let document = doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}; - db::services::find_and_update_user(&user_id, document, &client).await?; + db.find_and_update_user(&user_id, document).await?; Ok(HttpResponse::Ok().finish()) } @@ -172,11 +163,11 @@ pub async fn update_details( pub async fn update_settings( mut user: User, settings: Json, - client: Data, + db: Data, ) -> Result { let user_id = user.sub.clone(); user.settings = settings.into_inner(); let document = doc! {"$set" : to_bson(&user).map_err(|e| AppError::Bson(e.to_string()))?}; - db::services::find_and_update_user(&user_id, document, &client).await?; + db.find_and_update_user(&user_id, document).await?; Ok(HttpResponse::Ok().finish()) } diff --git a/packages/server/src/api/tests.rs b/packages/server/src/api/tests.rs index 2142377e..4c2e1f1e 100644 --- a/packages/server/src/api/tests.rs +++ b/packages/server/src/api/tests.rs @@ -1,11 +1,10 @@ use crate::{ - api::{bo, students::login}, - config::CONFIG, - init_mongodb_client, - middleware::{ - self, - auth::{self}, + api::{ + bo, + students::{self, login}, }, + db::Db, + middleware::{self, auth}, resources::{ catalog::DisplayCatalog, course::{Course, CourseStatus}, @@ -21,9 +20,6 @@ use actix_web::{ }; use actix_web_lab::middleware::from_fn; use dotenv::dotenv; -use mongodb::Client; - -use super::students::{self}; #[test] pub async fn test_get_all_catalogs() { @@ -31,10 +27,10 @@ pub async fn test_get_all_catalogs() { let token_claims = jsonwebtoken_google::test_helper::TokenClaims::new(); let (jwt, parser, _server) = jsonwebtoken_google::test_helper::setup(&token_claims); dotenv().ok(); - let client = init_mongodb_client!(); + let db = Db::new().await; let app = test::init_service( App::new() - .app_data(Data::new(client.clone())) + .app_data(Data::new(db.clone())) .app_data(auth::JwtDecoder::new_with_parser(parser)) .wrap(from_fn(middleware::auth::authenticate)) .service(students::get_all_catalogs), @@ -63,10 +59,10 @@ async fn test_students_api_full_flow() { let token_claims = jsonwebtoken_google::test_helper::TokenClaims::new(); let (jwt, parser, _server) = jsonwebtoken_google::test_helper::setup(&token_claims); // Init env and app - let client = init_mongodb_client!(); + let db = Db::new().await; let app = test::init_service( App::new() - .app_data(Data::new(client.clone())) + .app_data(Data::new(db.clone())) .app_data(auth::JwtDecoder::new_with_parser(parser)) .wrap(from_fn(middleware::auth::authenticate)) .service(students::get_all_catalogs) @@ -147,10 +143,10 @@ async fn test_compute_in_progress() { dotenv().ok(); // Init env and app - let client = init_mongodb_client!(); + let db = Db::new().await; let app = test::init_service( App::new() - .app_data(Data::new(client.clone())) + .app_data(Data::new(db.clone())) .service(students::compute_degree_status) .service(students::update_settings), ) @@ -210,10 +206,10 @@ async fn test_bo_api_courses() { let token_claims = jsonwebtoken_google::test_helper::TokenClaims::new(); let (jwt, parser, _server) = jsonwebtoken_google::test_helper::setup(&token_claims); // Init env and app - let client = init_mongodb_client!(); + let db = Db::new().await; let app = test::init_service( App::new() - .app_data(Data::new(client.clone())) + .app_data(Data::new(db.clone())) .app_data(auth::JwtDecoder::new_with_parser(parser)) .wrap(from_fn(middleware::auth::authenticate)) .service(bo::get_all_courses) @@ -280,10 +276,10 @@ async fn test_bo_api_catalogs() { let token_claims = jsonwebtoken_google::test_helper::TokenClaims::new(); let (jwt, parser, _server) = jsonwebtoken_google::test_helper::setup(&token_claims); // Init env and app - let client = init_mongodb_client!(); + let db = Db::new().await; let app = test::init_service( App::new() - .app_data(Data::new(client.clone())) + .app_data(Data::new(db.clone())) .app_data(auth::JwtDecoder::new_with_parser(parser)) .wrap(from_fn(middleware::auth::authenticate)) .service(bo::get_catalog_by_id), @@ -307,13 +303,8 @@ async fn test_bo_api_catalogs() { async fn test_student_login_no_sub() { dotenv().ok(); // Init env and app - let client = init_mongodb_client!(); - let app = test::init_service( - App::new() - .app_data(Data::new(client.clone())) - .service(login), - ) - .await; + let db = Db::new().await; + let app = test::init_service(App::new().app_data(Data::new(db.clone())).service(login)).await; // Create and send request let resp = test::TestRequest::get() @@ -333,10 +324,10 @@ async fn test_students_api_no_catalog() { // *** IMPORTANT: This should NEVER happen, but the tests are added anyway for coverage dotenv().ok(); // Init env and app - let client = init_mongodb_client!(); + let db = Db::new().await; let app = test::init_service( App::new() - .app_data(Data::new(client.clone())) + .app_data(Data::new(db.clone())) .service(students::compute_degree_status), ) .await; diff --git a/packages/server/src/core/tests.rs b/packages/server/src/core/tests.rs index 75aecfae..54a3f002 100644 --- a/packages/server/src/core/tests.rs +++ b/packages/server/src/core/tests.rs @@ -1,16 +1,14 @@ -use crate::config::CONFIG; use crate::core::bank_rule::BankRuleHandler; use crate::core::degree_status::DegreeStatus; use crate::core::parser; +use crate::db::Db; use crate::resources::catalog::Catalog; use crate::resources::course::CourseState::NotComplete; use crate::resources::course::Grade::Numeric; use crate::resources::course::{self, Course, CourseState, CourseStatus, Grade}; -use crate::{db, init_mongodb_client}; use actix_rt::test; use dotenv::dotenv; use lazy_static::lazy_static; -use mongodb::Client; use std::collections::HashMap; use std::str::FromStr; @@ -415,22 +413,21 @@ async fn test_duplicated_courses() { async fn get_catalog(catalog: &str) -> Catalog { dotenv().ok(); - let client = init_mongodb_client!(); + let db = Db::new().await; let obj_id = bson::oid::ObjectId::from_str(catalog).expect("failed to create oid"); - db::services::get_catalog_by_id(&obj_id, &client) + db.get_catalog_by_id(&obj_id) .await .expect("failed to get catalog") } async fn run_degree_status(mut degree_status: DegreeStatus, catalog: Catalog) -> DegreeStatus { dotenv().ok(); - let client = init_mongodb_client!(); - let vec_courses = db::services::get_all_courses(&client) + let db = Db::new().await; + let vec_courses = db + .get_all_courses() .await .expect("failed to get all courses"); - let malag_courses = db::services::get_all_malags(&client) - .await - .expect("failed to get all malags")[0] + let malag_courses = db.get_all_malags().await.expect("failed to get all malags")[0] .malag_list .clone(); degree_status.compute(catalog, course::vec_to_map(vec_courses), malag_courses); diff --git a/packages/server/src/db/mod.rs b/packages/server/src/db/mod.rs index 92e96798..b0860d8f 100644 --- a/packages/server/src/db/mod.rs +++ b/packages/server/src/db/mod.rs @@ -1,17 +1,31 @@ +use mongodb::Client; + +use crate::config::CONFIG; + pub mod services; #[cfg(test)] pub mod tests; -#[macro_export] -macro_rules! init_mongodb_client { - () => {{ - let client = Client::with_uri_str(CONFIG.uri) +#[derive(Debug, Clone)] +pub struct Db { + client: Client, +} + +impl Db { + pub async fn new() -> Self { + let client = Client::with_uri_str(&CONFIG.uri) .await .expect("Failed to connect to MongoDB"); + Self { client } + } + pub fn client(&self) -> &Client { + &self.client + } +} - log::info!("Connected to MongoDB"); - - client - }}; +impl From for Db { + fn from(client: Client) -> Self { + Self { client } + } } diff --git a/packages/server/src/db/services.rs b/packages/server/src/db/services.rs index 11bb3d30..4df7140e 100644 --- a/packages/server/src/db/services.rs +++ b/packages/server/src/db/services.rs @@ -8,17 +8,18 @@ use bson::oid::ObjectId; pub use bson::{doc, Bson, Document}; use futures_util::TryStreamExt; use mongodb::options::{FindOneAndUpdateOptions, ReturnDocument, UpdateModifications}; -use mongodb::Client; -#[macro_export] +use super::Db; + macro_rules! impl_get { ( fn_name=$fn_name:ident, db_item=$db_item:ty, db_key_type=$db_key_type:ty ) => { - pub async fn $fn_name(id: $db_key_type, client: &Client) -> Result<$db_item, AppError> { - match client + pub async fn $fn_name(&self, id: $db_key_type) -> Result<$db_item, AppError> { + match self + .client() .database(CONFIG.profile) .collection::<$db_item>(format!("{}s", stringify!($db_item)).as_str()) .find_one(doc! {"_id": id}, None) @@ -36,15 +37,15 @@ macro_rules! impl_get { }; } -#[macro_export] macro_rules! impl_get_all { ( fn_name=$fn_name:ident, db_item=$db_item:ty, db_coll_name=$db_coll_name:literal ) => { - pub async fn $fn_name(client: &Client) -> Result, AppError> { - match client + pub async fn $fn_name(&self) -> Result, AppError> { + match self + .client() .database(CONFIG.profile) .collection::<$db_item>($db_coll_name) .find(None, None) @@ -60,7 +61,6 @@ macro_rules! impl_get_all { }; } -#[macro_export] macro_rules! impl_get_filtered { ( fn_name=$fn_name:ident, @@ -69,11 +69,9 @@ macro_rules! impl_get_filtered { filter_by=$filter_by:literal, filter_type=$filter_type:literal ) => { - pub async fn $fn_name( - filter: impl Into, - client: &Client, - ) -> Result, AppError> { - match client + pub async fn $fn_name(&self, filter: impl Into) -> Result, AppError> { + match self + .client() .database(CONFIG.profile) .collection::<$db_item>($db_coll_name) .find(doc! {$filter_by: { $filter_type: filter.into()}}, None) @@ -89,7 +87,6 @@ macro_rules! impl_get_filtered { }; } -#[macro_export] macro_rules! impl_update { ( fn_name=$fn_name:ident, @@ -98,11 +95,12 @@ macro_rules! impl_update { db_coll_name=$db_coll_name:literal ) => { pub async fn $fn_name( + &self, id: $db_key_type, document: Document, - client: &Client, ) -> Result<$db_item, AppError> { - match client + match self + .client() .database(CONFIG.profile) .collection::<$db_item>($db_coll_name) .find_one_and_update( @@ -125,7 +123,6 @@ macro_rules! impl_update { }; } -#[macro_export] macro_rules! impl_delete { ( fn_name=$fn_name:ident, @@ -133,8 +130,9 @@ macro_rules! impl_delete { db_key_type=$db_key_type:ty, db_coll_name=$db_coll_name:literal ) => { - pub async fn $fn_name(id: $db_key_type, client: &Client) -> Result<(), AppError> { - match client + pub async fn $fn_name(&self, id: $db_key_type) -> Result<(), AppError> { + match self + .client() .database(CONFIG.profile) .collection::<$db_item>($db_coll_name) .delete_one(doc! {"_id": id}, None) @@ -147,100 +145,102 @@ macro_rules! impl_delete { }; } -// =============== CATALOG CRUD =============== - -impl_get!( - fn_name = get_catalog_by_id, - db_item = Catalog, - db_key_type = &ObjectId -); - -impl_get_all!( - fn_name = get_all_catalogs, - db_item = DisplayCatalog, - db_coll_name = "Catalogs" -); - -impl_update!( - fn_name = find_and_update_catalog, - db_item = Catalog, - db_key_type = &ObjectId, - db_coll_name = "Catalogs" -); - -// =============== COURSE CRUD =============== - -impl_get!( - fn_name = get_course_by_id, - db_item = Course, - db_key_type = &str -); - -impl_get_all!( - fn_name = get_all_courses, - db_item = Course, - db_coll_name = "Courses" -); - -impl_get_filtered!( - fn_name = get_courses_by_ids, - db_item = Course, - db_coll_name = "Courses", - filter_by = "_id", - filter_type = "$in" -); - -impl_get_filtered!( - fn_name = get_courses_filtered_by_name, - db_item = Course, - db_coll_name = "Courses", - filter_by = "name", - filter_type = "$regex" -); - -impl_get_filtered!( - fn_name = get_courses_filtered_by_number, - db_item = Course, - db_coll_name = "Courses", - filter_by = "_id", - filter_type = "$regex" -); - -impl_update!( - fn_name = find_and_update_course, - db_item = Course, - db_key_type = &str, - db_coll_name = "Courses" -); - -impl_delete!( - fn_name = delete_course, - db_item = Course, - db_key_type = &str, - db_coll_name = "Courses" -); - -impl_get_all!( - fn_name = get_all_malags, - db_item = Malags, - db_coll_name = "Malags" -); - -// =============== USER CRUD =============== - -impl_get!(fn_name = get_user_by_id, db_item = User, db_key_type = &str); - -impl_update!( - fn_name = find_and_update_user, - db_item = User, - db_key_type = &str, - db_coll_name = "Users" -); - -// =============== ADMIN CRUD =============== - -impl_get!( - fn_name = get_admin_by_id, - db_item = Admin, - db_key_type = &str -); +impl Db { + // =============== CATALOG CRUD =============== + + impl_get!( + fn_name = get_catalog_by_id, + db_item = Catalog, + db_key_type = &ObjectId + ); + + impl_get_all!( + fn_name = get_all_catalogs, + db_item = DisplayCatalog, + db_coll_name = "Catalogs" + ); + + impl_update!( + fn_name = find_and_update_catalog, + db_item = Catalog, + db_key_type = &ObjectId, + db_coll_name = "Catalogs" + ); + + // =============== COURSE CRUD =============== + + impl_get!( + fn_name = get_course_by_id, + db_item = Course, + db_key_type = &str + ); + + impl_get_all!( + fn_name = get_all_courses, + db_item = Course, + db_coll_name = "Courses" + ); + + impl_get_filtered!( + fn_name = get_courses_by_ids, + db_item = Course, + db_coll_name = "Courses", + filter_by = "_id", + filter_type = "$in" + ); + + impl_get_filtered!( + fn_name = get_courses_filtered_by_name, + db_item = Course, + db_coll_name = "Courses", + filter_by = "name", + filter_type = "$regex" + ); + + impl_get_filtered!( + fn_name = get_courses_filtered_by_number, + db_item = Course, + db_coll_name = "Courses", + filter_by = "_id", + filter_type = "$regex" + ); + + impl_update!( + fn_name = find_and_update_course, + db_item = Course, + db_key_type = &str, + db_coll_name = "Courses" + ); + + impl_delete!( + fn_name = delete_course, + db_item = Course, + db_key_type = &str, + db_coll_name = "Courses" + ); + + impl_get_all!( + fn_name = get_all_malags, + db_item = Malags, + db_coll_name = "Malags" + ); + + // =============== USER CRUD =============== + + impl_get!(fn_name = get_user_by_id, db_item = User, db_key_type = &str); + + impl_update!( + fn_name = find_and_update_user, + db_item = User, + db_key_type = &str, + db_coll_name = "Users" + ); + + // =============== ADMIN CRUD =============== + + impl_get!( + fn_name = get_admin_by_id, + db_item = Admin, + db_key_type = &str + ); +} diff --git a/packages/server/src/db/tests.rs b/packages/server/src/db/tests.rs index 33389b1e..c62bca30 100644 --- a/packages/server/src/db/tests.rs +++ b/packages/server/src/db/tests.rs @@ -1,4 +1,4 @@ -use crate::{config::CONFIG, db, init_mongodb_client}; +use crate::{config::CONFIG, db::Db}; use actix_rt::test; use actix_web::{body::MessageBody, http::StatusCode, web::Bytes, ResponseError}; @@ -8,8 +8,6 @@ use mongodb::{ Client, }; -use super::services; - #[test] pub async fn test_db_internal_error() { dotenv().ok(); @@ -28,18 +26,19 @@ pub async fn test_db_internal_error() { // Create mongodb client let client = Client::with_options(client_options).expect("Failed to create client"); + // Initialize db + let db = Db::from(client); + // Assert that all db requests cause an internal server error let errors = vec![ - services::get_course_by_id("124400", &client) - .await - .expect_err("Expected error"), - services::get_all_courses(&client) + db.get_course_by_id("124400") .await .expect_err("Expected error"), - services::find_and_update_course("124400", bson::doc! {"$setOnInsert": {}}, &client) + db.get_all_courses().await.expect_err("Expected error"), + db.find_and_update_course("124400", bson::doc! {"$setOnInsert": {}}) .await .expect_err("Expected error"), - services::delete_course("124400", &client) + db.delete_course("124400") .await .expect_err("Expected error"), ]; @@ -57,9 +56,10 @@ pub async fn test_db_internal_error() { pub async fn test_get_courses_by_filters() { dotenv().ok(); - let client = init_mongodb_client!(); + let db = Db::new().await; - let courses = db::services::get_courses_filtered_by_name("חשבון אינפיניטסימלי 1מ'", &client) + let courses = db + .get_courses_filtered_by_name("חשבון אינפיניטסימלי 1מ'") .await .expect("Failed to get courses by name"); @@ -67,7 +67,8 @@ pub async fn test_get_courses_by_filters() { assert_eq!(courses[0].name, "חשבון אינפיניטסימלי 1מ'"); assert_eq!(courses[0].id, "104031"); - let courses = db::services::get_courses_filtered_by_number("104031", &client) + let courses = db + .get_courses_filtered_by_number("104031") .await .expect("Failed to get courses by number"); @@ -75,7 +76,8 @@ pub async fn test_get_courses_by_filters() { assert_eq!(courses[0].name, "חשבון אינפיניטסימלי 1מ'"); assert_eq!(courses[0].id, "104031"); - let courses = db::services::get_courses_by_ids(vec!["104031", "104166"], &client) + let courses = db + .get_courses_by_ids(vec!["104031", "104166"]) .await .expect("Failed to get courses by number"); diff --git a/packages/server/src/main.rs b/packages/server/src/main.rs index 73752623..f66cda7c 100644 --- a/packages/server/src/main.rs +++ b/packages/server/src/main.rs @@ -1,9 +1,9 @@ use crate::config::CONFIG; use actix_web::{web, App, HttpServer}; use actix_web_lab::middleware::from_fn; +use db::Db; use dotenv::dotenv; use middleware::auth; -use mongodb::Client; mod api; mod config; @@ -23,13 +23,13 @@ async fn main() -> std::io::Result<()> { // Initialize logger logger::init_env_logger(); - // Initialize MongoDB client - let client = init_mongodb_client!(); + // Initialize DB client + let db = Db::new().await; // Start the server HttpServer::new(move || { App::new() - .app_data(web::Data::new(client.clone())) + .app_data(web::Data::new(db.clone())) .app_data(auth::JwtDecoder::new()) .wrap(from_fn(auth::authenticate)) .wrap(cors::cors()) diff --git a/packages/server/src/middleware/from_request.rs b/packages/server/src/middleware/from_request.rs index c9f111f1..1b41b39f 100644 --- a/packages/server/src/middleware/from_request.rs +++ b/packages/server/src/middleware/from_request.rs @@ -8,8 +8,8 @@ macro_rules! impl_from_request { fn from_request(req: &HttpRequest, _: &mut Payload) -> Self::Future { let req = req.clone(); Box::pin(async move { - let client = match req.app_data::>() { - Some(client) => client, + let db = match req.app_data::>() { + Some(db) => db, None => { return Err(AppError::InternalServer( "Mongodb client not found in application data".into(), @@ -17,7 +17,7 @@ macro_rules! impl_from_request { } }; match req.extensions().get::() { - Some(key) => db::services::$get_fn(key, client).await, + Some(key) => db.$get_fn(key).await, None => Err(AppError::Middleware( "Sub not found in request extensions".into(), )), diff --git a/packages/server/src/middleware/tests.rs b/packages/server/src/middleware/tests.rs index 0fe63d34..830d5c41 100644 --- a/packages/server/src/middleware/tests.rs +++ b/packages/server/src/middleware/tests.rs @@ -1,4 +1,4 @@ -use crate::{config::CONFIG, init_mongodb_client, middleware, resources::user::User}; +use crate::{db::Db, middleware, resources::user::User}; use actix_rt::test; use actix_web::{ http::StatusCode, @@ -8,7 +8,6 @@ use actix_web::{ }; use actix_web_lab::middleware::from_fn; use dotenv::dotenv; -use mongodb::Client; #[test] async fn test_from_request_no_db_client() { @@ -46,8 +45,8 @@ async fn test_from_request_no_db_client() { async fn test_from_request_no_auth_mw() { //Init env and app dotenv().ok(); - let client = init_mongodb_client!(); - let app = test::init_service(App::new().app_data(web::Data::new(client.clone())).service( + let db = Db::new().await; + let app = test::init_service(App::new().app_data(web::Data::new(db.clone())).service( web::resource("/").route(web::get().to(|_: User| async { "Shouldn't get here" })), )) .await; @@ -71,10 +70,10 @@ async fn test_from_request_no_auth_mw() { async fn test_auth_mw_no_jwt_decoder() { //Init env and app dotenv().ok(); - let client = init_mongodb_client!(); + let db = Db::new().await; let app = test::init_service( App::new() - .app_data(web::Data::new(client.clone())) + .app_data(web::Data::new(db.clone())) .wrap(from_fn(middleware::auth::authenticate)) .service(web::resource("/").route(web::get().to(|| async { "Shouldn't get here" }))), ) diff --git a/packages/server/src/resources/admin.rs b/packages/server/src/resources/admin.rs index c328d947..aaccfda4 100644 --- a/packages/server/src/resources/admin.rs +++ b/packages/server/src/resources/admin.rs @@ -1,7 +1,7 @@ -use crate::db; use crate::error::AppError; use crate::impl_from_request; use crate::middleware::auth::Sub; +use crate::Db; use actix_web::dev::Payload; use actix_web::{web::Data, FromRequest, HttpMessage, HttpRequest}; use bson::doc; diff --git a/packages/server/src/resources/user.rs b/packages/server/src/resources/user.rs index 9c714df9..fa418c2f 100644 --- a/packages/server/src/resources/user.rs +++ b/packages/server/src/resources/user.rs @@ -1,9 +1,9 @@ use super::catalog::DisplayCatalog; use crate::core::degree_status::DegreeStatus; -use crate::db; use crate::error::AppError; use crate::impl_from_request; use crate::middleware::auth::Sub; +use crate::Db; use actix_web::dev::Payload; use actix_web::{web::Data, FromRequest, HttpMessage, HttpRequest}; use bson::doc; From 3d5b5aa92083d1ad008d8fd6247a2b2b12269d37 Mon Sep 17 00:00:00 2001 From: benny-n Date: Sat, 17 Sep 2022 17:22:54 +0300 Subject: [PATCH 5/5] Remove unnecessary `unwrap()`s --- packages/server/src/config.rs | 2 +- .../core/bank_rule/specialization_groups.rs | 3 +-- .../src/core/degree_status/compute_bank.rs | 4 ++-- .../src/core/degree_status/preprocessing.rs | 4 +++- packages/server/src/core/messages.rs | 16 +++++++------- packages/server/src/core/parser.rs | 2 +- packages/server/src/core/tests.rs | 6 +++-- packages/server/src/db/services.rs | 6 +++-- packages/server/src/resources/course.rs | 22 ++++++++++++------- 9 files changed, 38 insertions(+), 27 deletions(-) diff --git a/packages/server/src/config.rs b/packages/server/src/config.rs index 51432bb4..6ee4cdd9 100644 --- a/packages/server/src/config.rs +++ b/packages/server/src/config.rs @@ -22,7 +22,7 @@ impl Config<'_> { pub fn from_env() -> Self { Config { ip: &IP, - port: PORT.parse::().unwrap(), + port: PORT.parse::().expect("Failed to parse port"), uri: &URI, client_id: &CLIENT_ID, profile: &PROFILE, diff --git a/packages/server/src/core/bank_rule/specialization_groups.rs b/packages/server/src/core/bank_rule/specialization_groups.rs index 8a22ceb5..72e02c23 100644 --- a/packages/server/src/core/bank_rule/specialization_groups.rs +++ b/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> { diff --git a/packages/server/src/core/degree_status/compute_bank.rs b/packages/server/src/core/degree_status/compute_bank.rs index 902a9fc5..ae6e3009 100644 --- a/packages/server/src/core/degree_status/compute_bank.rs +++ b/packages/server/src/core/degree_status/compute_bank.rs @@ -65,7 +65,7 @@ impl<'a> DegreeStatusHandler<'a> { sum_credit = bank_rule_handler.chain(chains, &mut chain_done); completed = !chain_done.is_empty(); if completed { - msg = Some(messages::completed_chain_msg(&chain_done)); + msg = Some(messages::completed_chain_msg(chain_done)); } } Rule::SpecializationGroups(specialization_groups) => { @@ -73,7 +73,7 @@ impl<'a> DegreeStatusHandler<'a> { .specialization_group(specialization_groups, &mut groups_done_list); completed = groups_done_list.len() >= specialization_groups.groups_number; msg = Some(messages::completed_specialization_groups_msg( - &groups_done_list, + groups_done_list, specialization_groups.groups_number, )); } diff --git a/packages/server/src/core/degree_status/preprocessing.rs b/packages/server/src/core/degree_status/preprocessing.rs index d15a14fa..77ce356e 100644 --- a/packages/server/src/core/degree_status/preprocessing.rs +++ b/packages/server/src/core/degree_status/preprocessing.rs @@ -81,7 +81,9 @@ impl DegreeStatus { self.course_statuses.sort_by(|c1, c2| { c1.extract_semester() .partial_cmp(&c2.extract_semester()) - .unwrap() // unwrap can't fail because we compare only integers or "half integers" (0.5,1,1.5,2,2.5...) + .unwrap_or(std::cmp::Ordering::Equal) + // partial_cmp returns None if one of the two values are NaN, which should never happen + // still, to be on the safe side, we use Ordering::Equal in that case instead of unwrapping }); } } diff --git a/packages/server/src/core/messages.rs b/packages/server/src/core/messages.rs index 7f3753f8..53cf2f10 100644 --- a/packages/server/src/core/messages.rs +++ b/packages/server/src/core/messages.rs @@ -55,11 +55,11 @@ pub fn missing_credit_msg(overflow: f32, from: &str, to: &str) -> String { } } -pub fn completed_chain_msg(chain: &[String]) -> String { +pub fn completed_chain_msg(mut chain: Vec) -> String { let mut msg = "השלמת את השרשרת: ".to_string(); - for course in chain { - if course == chain.last().unwrap() { - msg += course; + while let Some(course) = chain.pop() { + if chain.is_empty() { + msg += &course; } else { msg += &format!("{}, ", course); } @@ -67,7 +67,7 @@ pub fn completed_chain_msg(chain: &[String]) -> String { msg } -pub fn completed_specialization_groups_msg(groups: &[String], needed: usize) -> String { +pub fn completed_specialization_groups_msg(mut groups: Vec, needed: usize) -> String { let mut msg = if groups.len() == ZERO as usize { "לא השלמת אף קבוצת התמחות".to_string() } else if groups.len() == SINGLE as usize { @@ -75,9 +75,9 @@ pub fn completed_specialization_groups_msg(groups: &[String], needed: usize) -> } else { format!("השלמת {} (מתוך {}) קבוצות התמחות: ", groups.len(), needed) }; - for group in groups { - if group == groups.last().unwrap() { - msg += group; + while let Some(group) = groups.pop() { + if groups.is_empty() { + msg += &group; } else { msg += &format!("{}, ", group); } diff --git a/packages/server/src/core/parser.rs b/packages/server/src/core/parser.rs index 3899f7bc..cafe84e1 100644 --- a/packages/server/src/core/parser.rs +++ b/packages/server/src/core/parser.rs @@ -157,7 +157,7 @@ fn parse_course_status_pdf_format(line: &str) -> Result<(Course, Option), .rev() .collect::() .parse::() - .unwrap(); + .map_err(|e| AppError::Parser(format!("Bad Format: {}", e)))?; break; } index += 1; diff --git a/packages/server/src/core/tests.rs b/packages/server/src/core/tests.rs index 54a3f002..49fe3ea2 100644 --- a/packages/server/src/core/tests.rs +++ b/packages/server/src/core/tests.rs @@ -615,7 +615,9 @@ async fn test_overflow_credit() { ); assert_eq!( degree_status.course_bank_requirements[5].message, - Some(messages::completed_chain_msg(&["פיסיקה 2פ'".to_string()])) + Some(messages::completed_chain_msg( + vec!["פיסיקה 2פ'".to_string()] + )) ); assert_eq!( @@ -709,7 +711,7 @@ async fn test_software_engineer_itinerary() { ); assert_eq!( degree_status.course_bank_requirements[5].message, - Some(messages::completed_chain_msg(&[ + Some(messages::completed_chain_msg(vec![ "פיסיקה 2".to_string(), "פיסיקה 3".to_string() ])) diff --git a/packages/server/src/db/services.rs b/packages/server/src/db/services.rs index 4df7140e..602729c8 100644 --- a/packages/server/src/db/services.rs +++ b/packages/server/src/db/services.rs @@ -115,8 +115,10 @@ macro_rules! impl_update { ) .await { - // We can safely unwrap here thanks to upsert=true and ReturnDocument::After - Ok(item) => Ok(item.unwrap()), + Ok(item) => item.ok_or_else(|| { + // This should never happen, but to avoid unwrapping we return an explicit error + AppError::NotFound(format!("{}: {}", stringify!($db_item), id.to_string())) + }), Err(err) => Err(AppError::MongoDriver(err.to_string())), } } diff --git a/packages/server/src/resources/course.rs b/packages/server/src/resources/course.rs index a08c4c6c..cbe0641d 100644 --- a/packages/server/src/resources/course.rs +++ b/packages/server/src/resources/course.rs @@ -103,13 +103,17 @@ impl CourseStatus { } pub fn extract_semester(&self) -> f32 { - match self.semester.clone() { - Some(semester) => { - let semester: Vec<&str> = semester.split('_').collect(); - semester.last().unwrap().parse::().unwrap_or(0.0) //TODO explain - } - None => 0.0, - } + self.semester + .as_ref() + .map(|semester| { + semester + .split('_') + .last() + .unwrap_or("0.0") + .parse::() + .unwrap_or(0.0) + }) + .unwrap_or(0.0) } pub fn valid_for_bank(&self, bank_name: &str) -> bool { @@ -206,7 +210,9 @@ impl<'de> Visitor<'de> for GradeStrVisitor { "פטור ללא ניקוד" => Ok(Grade::ExemptionWithoutCredit), "פטור עם ניקוד" => Ok(Grade::ExemptionWithCredit), "לא השלים" => Ok(Grade::NotComplete), - _ if v.parse::().is_ok() => Ok(Grade::Numeric(v.parse::().unwrap())), + _ if v.parse::().is_ok() => Ok(Grade::Numeric( + v.parse::().map_err(|e| Err::custom(e.to_string()))?, + )), _ => { let err: E = Err::invalid_type(Unexpected::Str(v), &self); log::error!("Json deserialize error: {}", err.to_string());