Improve DB API (#189)

* Improve DB API

* Remove comment

* Rename some symbols in `services`

packages/server/src/api/bo.rs

@@ -7,7 +7,6 @@ use crate::resources::catalog::Catalog;
 use crate::resources::{admin::Admin, course::Course};
 use actix_web::web::{Data, Json, Path};
 use actix_web::{delete, get, put, HttpResponse};
-use bson::doc;
 
 /////////////////////////////////////////////////////////////////////////////
 // Course API
@@ -34,13 +33,11 @@ pub async fn get_course_by_id(
 #[put("/courses/{id}")]
 pub async fn create_or_update_course(
     _: Admin,
-    id: Path<String>,
+    _id: Path<String>,
     course: Json<Course>,
     db: Data<Db>,
 ) -> Result<HttpResponse, AppError> {
-    let course_doc = bson::to_document(&course).map_err(|e| AppError::Bson(e.to_string()))?;
-    let document = doc! {"$setOnInsert" : course_doc};
-    db.update::<Course>(id.as_str(), document)
+    db.create_or_update::<Course>(course.into_inner())
         .await
         .map(|course| HttpResponse::Ok().json(course))
 }
@@ -75,15 +72,12 @@ pub async fn get_catalog_by_id(
 #[put("/catalogs/{id}")]
 pub async fn create_or_update_catalog(
     _: Admin,
-    id: Path<String>,
+    _id: Path<String>,
     catalog: Json<Catalog>,
     db: Data<Db>,
 ) -> Result<HttpResponse, AppError> {
     catalog_validations::validate_catalog(&catalog)?;
-    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.update::<Catalog>(&obj_id, document)
+    db.create_or_update::<Catalog>(catalog.into_inner())
         .await
         .map(|catalog| HttpResponse::Ok().json(catalog))
 }

packages/server/src/api/students.rs

@@ -5,11 +5,10 @@ use actix_web::{
     web::{Data, Json, Query},
     HttpMessage, HttpRequest, HttpResponse,
 };
-use bson::{doc, to_bson};
 
 use crate::{
     core::{degree_status::DegreeStatus, parser},
-    db::{Db, FilterType},
+    db::{Db, FilterOption},
     error::AppError,
     middleware::auth::Sub,
     resources::{
@@ -46,9 +45,8 @@ pub async fn login(db: Data<Db>, req: HttpRequest) -> Result<HttpResponse, AppEr
         sub: user_id,
         ..Default::default()
     };
-    let document = doc! {"$setOnInsert" : to_bson(&user)?};
 
-    let updated_user = db.update::<User>(&user.sub, document).await?;
+    let updated_user = db.create_or_update::<User>(user).await?;
 
     Ok(HttpResponse::Ok().json(updated_user))
 }
@@ -76,9 +74,7 @@ pub async fn update_catalog(
             cs.additional_msg = None;
         });
 
-    let updated_user = db
-        .update::<User>(&user.sub.clone(), doc! {"$set" : to_bson(&user)?})
-        .await?;
+    let updated_user = db.update::<User>(user).await?;
     Ok(HttpResponse::Ok().json(updated_user))
 }
 
@@ -93,13 +89,13 @@ pub async fn get_courses_by_filter(
     match (params.get("name"), params.get("number")) {
         (Some(name), None) => {
             let courses = db
-                .get_filtered::<Course>(FilterType::Regex, "name", name)
+                .get_filtered::<Course>(FilterOption::Regex, "name", name)
                 .await?;
             Ok(HttpResponse::Ok().json(courses))
         }
         (None, Some(number)) => {
             let courses = db
-                .get_filtered::<Course>(FilterType::Regex, "_id", number)
+                .get_filtered::<Course>(FilterOption::Regex, "_id", number)
                 .await?;
             Ok(HttpResponse::Ok().json(courses))
         }
@@ -117,9 +113,7 @@ pub async fn add_courses(
     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
-        .update::<User>(&user.sub.clone(), doc! {"$set" : to_bson(&user)?})
-        .await?;
+    let updated_user = db.update::<User>(user).await?;
     Ok(HttpResponse::Ok().json(updated_user))
 }
 
@@ -139,7 +133,7 @@ pub async fn compute_degree_status(mut user: User, db: Data<Db>) -> Result<HttpR
 
     let courses = db
         .get_filtered::<Course>(
-            FilterType::In,
+            FilterOption::In,
             "_id",
             catalog
                 .get_all_course_ids()
@@ -179,9 +173,7 @@ pub async fn compute_degree_status(mut user: User, db: Data<Db>) -> Result<HttpR
     if user.details.compute_in_progress {
         user.details.degree_status.set_to_in_progress(course_list);
     }
-    let user_id = user.sub.clone();
-    let document = doc! {"$set" : to_bson(&user)?};
-    db.update::<User>(&user_id, document).await?;
+    db.update::<User>(user.clone()).await?;
     Ok(HttpResponse::Ok().json(user))
 }
 
@@ -192,10 +184,8 @@ pub async fn update_details(
     details: Json<UserDetails>,
     db: Data<Db>,
 ) -> Result<HttpResponse, AppError> {
-    let user_id = user.sub.clone();
     user.details = details.into_inner();
-    let document = doc! {"$set" : to_bson(&user)?};
-    db.update::<User>(&user_id, document).await?;
+    db.update::<User>(user).await?;
     Ok(HttpResponse::Ok().finish())
 }
 
@@ -205,9 +195,7 @@ pub async fn update_settings(
     settings: Json<UserSettings>,
     db: Data<Db>,
 ) -> Result<HttpResponse, AppError> {
-    let user_id = user.sub.clone();
     user.settings = settings.into_inner();
-    let document = doc! {"$set" : to_bson(&user)?};
-    db.update::<User>(&user_id, document).await?;
+    db.update::<User>(user).await?;
     Ok(HttpResponse::Ok().finish())
 }

packages/server/src/db/mod.rs

@@ -1,4 +1,6 @@
+use bson::Document;
 use mongodb::Client;
+use serde::{de::DeserializeOwned, Serialize};
 
 use crate::config::CONFIG;
 
@@ -30,20 +32,35 @@ impl From<Client> for Db {
     }
 }
 
-pub enum FilterType {
+pub enum FilterOption {
     Regex,
     In,
 }
 
-impl AsRef<str> for FilterType {
+pub enum InsertOption {
+    Set,
+    SetOnInsert,
+}
+
+impl AsRef<str> for FilterOption {
+    fn as_ref(&self) -> &str {
+        match self {
+            FilterOption::Regex => "$regex",
+            FilterOption::In => "$in",
+        }
+    }
+}
+
+impl AsRef<str> for InsertOption {
     fn as_ref(&self) -> &str {
         match self {
-            FilterType::Regex => "$regex",
-            FilterType::In => "$in",
+            InsertOption::Set => "$set",
+            InsertOption::SetOnInsert => "$setOnInsert",
         }
     }
 }
 
-pub trait CollectionName {
+pub trait Resource: Serialize + DeserializeOwned {
     fn collection_name() -> &'static str;
+    fn key(&self) -> Document;
 }

packages/server/src/db/services.rs

@@ -1,74 +1,73 @@
 use crate::config::CONFIG;
 use crate::error::AppError;
+use bson::to_bson;
 pub use bson::{doc, Bson, Document};
 use futures_util::TryStreamExt;
 use mongodb::options::{FindOneAndUpdateOptions, ReturnDocument, UpdateModifications};
-use serde::de::DeserializeOwned;
-use serde::Serialize;
+use serde::{de::DeserializeOwned, Serialize};
 
-use super::{CollectionName, Db, FilterType};
+use super::{Db, FilterOption, InsertOption, Resource};
 
 impl Db {
-    pub async fn get<T>(&self, id: impl Serialize) -> Result<T, AppError>
+    pub async fn get<R>(&self, id: impl Serialize) -> Result<R, AppError>
     where
-        T: CollectionName + DeserializeOwned + Send + Sync + Unpin,
+        R: Resource + Send + Sync + Unpin,
     {
         let id = bson::to_bson(&id)?;
         self.client()
             .database(CONFIG.profile)
-            .collection::<T>(T::collection_name())
+            .collection::<R>(R::collection_name())
             .find_one(doc! {"_id": id.clone()}, None)
             .await?
-            .ok_or_else(|| AppError::NotFound(format!("{}: {}", T::collection_name(), id)))
+            .ok_or_else(|| AppError::NotFound(format!("{}: {}", R::collection_name(), id)))
     }
 
-    pub async fn get_all<T>(&self) -> Result<Vec<T>, AppError>
+    pub async fn get_all<R>(&self) -> Result<Vec<R>, AppError>
     where
-        T: CollectionName + DeserializeOwned + Send + Sync + Unpin,
+        R: Resource + Send + Sync + Unpin,
     {
         Ok(self
             .client()
             .database(CONFIG.profile)
-            .collection::<T>(T::collection_name())
+            .collection::<R>(R::collection_name())
             .find(None, None)
             .await?
-            .try_collect::<Vec<T>>()
+            .try_collect::<Vec<R>>()
             .await?)
     }
 
-    pub async fn get_filtered<T>(
+    pub async fn get_filtered<R>(
         &self,
-        filter_type: FilterType,
+        filter_option: FilterOption,
         field_to_filter: impl AsRef<str>,
         filter: impl Into<Bson>,
-    ) -> Result<Vec<T>, AppError>
+    ) -> Result<Vec<R>, AppError>
     where
-        T: CollectionName + DeserializeOwned + Send + Sync + Unpin,
+        R: Resource + DeserializeOwned + Send + Sync + Unpin,
     {
         Ok(self
             .client()
             .database(CONFIG.profile)
-            .collection::<T>(T::collection_name())
+            .collection::<R>(R::collection_name())
             .find(
-                doc! {field_to_filter.as_ref(): { filter_type.as_ref(): filter.into()}},
+                doc! {field_to_filter.as_ref(): { filter_option.as_ref(): filter.into()}},
                 None,
             )
             .await?
-            .try_collect::<Vec<T>>()
+            .try_collect::<Vec<R>>()
             .await?)
     }
 
-    pub async fn update<T>(&self, id: impl Serialize, document: Document) -> Result<T, AppError>
+    async fn _update<R>(&self, resource: R, insert_option: InsertOption) -> Result<R, AppError>
     where
-        T: CollectionName + DeserializeOwned + Send + Sync + Unpin,
+        R: Resource + Send + Sync + Unpin,
     {
-        let id = bson::to_bson(&id)?;
         self.client()
             .database(CONFIG.profile)
-            .collection::<T>(T::collection_name())
+            .collection::<R>(R::collection_name())
             .find_one_and_update(
-                doc! {"_id": id.clone()},
-                UpdateModifications::Document(document),
+                resource.key(),
+                UpdateModifications::Document(doc! { insert_option.as_ref(): to_bson(&resource)? }),
                 Some(
                     FindOneAndUpdateOptions::builder()
                         .upsert(true)
@@ -79,19 +78,33 @@ impl Db {
             .await?
             .ok_or_else(|| {
                 // This should never happen, but to avoid unwrapping we return an explicit error
-                AppError::NotFound(format!("{}: {}", T::collection_name(), id))
+                AppError::NotFound(R::collection_name().to_string())
             })
     }
 
-    pub async fn delete<T>(&self, id: impl Serialize) -> Result<(), AppError>
+    pub async fn update<R>(&self, resource: R) -> Result<R, AppError>
+    where
+        R: Resource + Send + Sync + Unpin,
+    {
+        self._update::<R>(resource, InsertOption::Set).await
+    }
+
+    pub async fn create_or_update<R>(&self, resource: R) -> Result<R, AppError>
+    where
+        R: Resource + Send + Sync + Unpin,
+    {
+        self._update::<R>(resource, InsertOption::SetOnInsert).await
+    }
+
+    pub async fn delete<R>(&self, id: impl Serialize) -> Result<(), AppError>
     where
-        T: CollectionName + DeserializeOwned + Send + Sync + Unpin,
+        R: Resource + Send + Sync + Unpin,
     {
         let id = bson::to_bson(&id)?;
         Ok(self
             .client()
             .database(CONFIG.profile)
-            .collection::<T>(T::collection_name())
+            .collection::<R>(R::collection_name())
             .delete_one(doc! {"_id": id.clone()}, None)
             .await
             .map(|_| ())?) // Discard the result of the delete operation

packages/server/src/db/tests.rs

@@ -1,6 +1,6 @@
 use crate::{
     config::CONFIG,
-    db::{Db, FilterType},
+    db::{Db, FilterOption},
     resources::course::Course,
 };
 use actix_rt::test;
@@ -39,9 +39,12 @@ pub async fn test_db_internal_error() {
             .await
             .expect_err("Expected error"),
         db.get_all::<Course>().await.expect_err("Expected error"),
-        db.update::<Course>("124400", bson::doc! {"$setOnInsert": {}})
-            .await
-            .expect_err("Expected error"),
+        db.update::<Course>(Course {
+            id: "124400".into(),
+            ..Default::default()
+        })
+        .await
+        .expect_err("Expected error"),
         db.delete::<Course>("124400")
             .await
             .expect_err("Expected error"),
@@ -63,7 +66,7 @@ pub async fn test_get_courses_by_filters() {
     let db = Db::new().await;
 
     let courses = db
-        .get_filtered::<Course>(FilterType::Regex, "name", "חשבון אינפיניטסימלי 1מ'")
+        .get_filtered::<Course>(FilterOption::Regex, "name", "חשבון אינפיניטסימלי 1מ'")
         .await
         .expect("Failed to get courses by name");
 
@@ -72,7 +75,7 @@ pub async fn test_get_courses_by_filters() {
     assert_eq!(courses[0].id, "104031");
 
     let courses = db
-        .get_filtered::<Course>(FilterType::Regex, "_id", "104031")
+        .get_filtered::<Course>(FilterOption::Regex, "_id", "104031")
         .await
         .expect("Failed to get courses by number");
 
@@ -81,7 +84,7 @@ pub async fn test_get_courses_by_filters() {
     assert_eq!(courses[0].id, "104031");
 
     let courses = db
-        .get_filtered::<Course>(FilterType::In, "_id", vec!["104031", "104166"])
+        .get_filtered::<Course>(FilterOption::In, "_id", vec!["104031", "104166"])
         .await
         .expect("Failed to get courses by number");