From de4041bd17bfed7f0da4e78b6969dfacc0440c1f Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Sun, 19 Mar 2023 15:05:20 -0400 Subject: [PATCH] Refactor tags services and added tests Implemented some functionality which will allow the service configuration of the app to be split among the features for readability and testability. --- src/auth.rs | 7 +- src/database/mod.rs | 4 +- src/database/schema.rs | 7 +- src/main.rs | 11 +-- src/service.rs | 16 +++++ src/tags.rs | 148 ++++++++++++++++++++++++++++++++--------- 6 files changed, 143 insertions(+), 50 deletions(-) create mode 100644 src/service.rs diff --git a/src/auth.rs b/src/auth.rs index b49dcce..5c64312 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -1,4 +1,3 @@ -use std::sync::Mutex; use actix_web::Responder; use actix_web::{ web::{self, Json}, @@ -7,6 +6,7 @@ use actix_web::{ use chrono::{Duration, Utc}; use jsonwebtoken::{encode, EncodingKey, Header}; use log::{debug, error}; +use std::sync::Mutex; use crate::{ data::{secret_key, Claims, CreateAccountRequest, LoginRequest, Token}, @@ -32,7 +32,10 @@ async fn register( } } -pub async fn login(creds: Json, user_dao: web::Data>) -> HttpResponse { +pub async fn login( + creds: Json, + user_dao: web::Data>, +) -> HttpResponse { debug!("Logging in: {}", creds.username); let mut user_dao = user_dao.lock().expect("Unable to get UserDao"); diff --git a/src/database/mod.rs b/src/database/mod.rs index 073bbf7..df97315 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -1,10 +1,8 @@ use bcrypt::{hash, verify, DEFAULT_COST}; use diesel::prelude::*; use diesel::sqlite::SqliteConnection; -use std::{ - sync::{Arc, Mutex}, -}; use std::ops::DerefMut; +use std::sync::{Arc, Mutex}; use crate::database::models::{Favorite, InsertFavorite, InsertUser, User}; diff --git a/src/database/schema.rs b/src/database/schema.rs index 340c604..25a217e 100644 --- a/src/database/schema.rs +++ b/src/database/schema.rs @@ -33,9 +33,4 @@ table! { joinable!(tagged_photo -> tags (tag_id)); -allow_tables_to_appear_in_same_query!( - favorites, - tagged_photo, - tags, - users, -); +allow_tables_to_appear_in_same_query!(favorites, tagged_photo, tags, users,); diff --git a/src/main.rs b/src/main.rs index f340a01..d2ed44f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -36,6 +36,7 @@ use crate::auth::login; use crate::data::*; use crate::database::*; use crate::files::{is_image_or_video, is_valid_full_path}; +use crate::service::ServiceBuilder; use crate::state::AppState; use crate::tags::*; use crate::video::*; @@ -49,6 +50,7 @@ mod state; mod tags; mod video; +mod service; #[cfg(test)] mod testhelpers; @@ -482,14 +484,7 @@ fn main() -> std::io::Result<()> { .service(put_add_favorite) .service(delete_favorite) .service(get_file_metadata) - .service( - web::resource("image/tags") - .route(web::post().to(add_tag::)) - .route(web::get().to(get_tags::)) - .route(web::delete().to(remove_tagged_photo::)), - ) - .service(web::resource("image/tags/all").route(web::get().to(get_all_tags::))) - .service(web::resource("image/tags/batch").route(web::post().to(update_tags::))) + .add_feature(add_tag_services::<_, SqliteTagDao>) .app_data(app_data.clone()) .app_data::>>(Data::new(Mutex::new(user_dao))) .app_data::>>>(Data::new(Mutex::new(Box::new( diff --git a/src/service.rs b/src/service.rs new file mode 100644 index 0000000..d31b51e --- /dev/null +++ b/src/service.rs @@ -0,0 +1,16 @@ +use actix_web::App; + +pub trait ServiceBuilder { + fn add_feature(self, f: F) -> App + where + F: Fn(App) -> App; +} + +impl ServiceBuilder for App { + fn add_feature(self, create_feature: F) -> App + where + F: Fn(App) -> App, + { + create_feature(self) + } +} diff --git a/src/tags.rs b/src/tags.rs index 916336f..a55fbe5 100644 --- a/src/tags.rs +++ b/src/tags.rs @@ -1,5 +1,6 @@ use crate::{connect, data::AddTagRequest, error::IntoHttpError, schema, Claims, ThumbnailRequest}; -use actix_web::{web, HttpResponse, Responder}; +use actix_web::dev::{ServiceFactory, ServiceRequest}; +use actix_web::{web, App, HttpResponse, Responder}; use anyhow::Context; use chrono::Utc; use diesel::prelude::*; @@ -9,7 +10,21 @@ use serde::{Deserialize, Serialize}; use std::borrow::BorrowMut; use std::sync::Mutex; -pub async fn add_tag( +pub fn add_tag_services(app: App) -> App +where + T: ServiceFactory, +{ + app.service( + web::resource("image/tags") + .route(web::post().to(add_tag::)) + .route(web::get().to(get_tags::)) + .route(web::delete().to(remove_tagged_photo::)), + ) + .service(web::resource("image/tags/all").route(web::get().to(get_all_tags::))) + .service(web::resource("image/tags/batch").route(web::post().to(update_tags::))) +} + +async fn add_tag( _: Claims, body: web::Json, tag_dao: web::Data>, @@ -32,7 +47,7 @@ pub async fn add_tag( .into_http_internal_err() } -pub async fn get_tags( +async fn get_tags( _: Claims, request: web::Query, tag_dao: web::Data>, @@ -44,7 +59,7 @@ pub async fn get_tags( .into_http_internal_err() } -pub async fn get_all_tags(_: Claims, tag_dao: web::Data>) -> impl Responder { +async fn get_all_tags(_: Claims, tag_dao: web::Data>) -> impl Responder { let mut tag_dao = tag_dao.lock().expect("Unable to get TagDao"); tag_dao .get_all_tags() @@ -52,7 +67,7 @@ pub async fn get_all_tags(_: Claims, tag_dao: web::Data>) -> .into_http_internal_err() } -pub async fn remove_tagged_photo( +async fn remove_tagged_photo( _: Claims, request: web::Json, tag_dao: web::Data>, @@ -70,39 +85,55 @@ pub async fn remove_tagged_photo( .into_http_internal_err() } -pub async fn update_tags(_: Claims, tag_dao: web::Data>, request: web::Json) -> impl Responder { - let mut dao = tag_dao.lock() - .expect("Unable to get TagDao"); +async fn update_tags( + _: Claims, + tag_dao: web::Data>, + request: web::Json, +) -> impl Responder { + let mut dao = tag_dao.lock().expect("Unable to get TagDao"); - return dao.get_tags_for_path(&request.file_name) - .and_then(|existing_tags| { - dao.get_all_tags().map(|all| (existing_tags, all)) - }) + return dao + .get_tags_for_path(&request.file_name) + .and_then(|existing_tags| dao.get_all_tags().map(|all| (existing_tags, all))) .and_then(|(existing_tags, all_tags)| { - let tags_to_remove = existing_tags.iter() + let tags_to_remove = existing_tags + .iter() .filter(|&t| !request.tag_ids.contains(&t.id)) .collect::>(); for tag in tags_to_remove { - info!("Removing tag {:?} from file: {:?}", tag.name, request.file_name); + info!( + "Removing tag {:?} from file: {:?}", + tag.name, request.file_name + ); dao.remove_tag(&tag.name, &request.file_name) .expect(&format!("Unable to remove tag {:?}", &tag.name)); } - let new_tags = all_tags.iter() + let new_tags = all_tags + .iter() .filter(|&t| !existing_tags.contains(t) && request.tag_ids.contains(&t.id)) .collect::>(); for new_tag in new_tags { - info!("Adding tag {:?} to file: {:?}", new_tag.name, request.file_name); + info!( + "Adding tag {:?} to file: {:?}", + new_tag.name, request.file_name + ); dao.tag_file(&request.file_name, new_tag.id) - .with_context(|| format!("Unable to tag file {:?} with tag: {:?}", request.file_name, new_tag.name)) + .with_context(|| { + format!( + "Unable to tag file {:?} with tag: {:?}", + request.file_name, new_tag.name + ) + }) .unwrap(); } Ok(HttpResponse::Ok()) - }).into_http_internal_err(); + }) + .into_http_internal_err(); } #[derive(Serialize, Queryable, Clone, Debug, PartialEq)] @@ -225,9 +256,9 @@ impl TagDao for SqliteTagDao { .filter(tagged_photo::tag_id.eq(tag.id)) .filter(tagged_photo::photo_name.eq(path)), ) - .execute(&mut self.connection) - .with_context(|| format!("Unable to delete tag: '{}'", &tag.name)) - .map(|_| Some(())) + .execute(&mut self.connection) + .with_context(|| format!("Unable to delete tag: '{}'", &tag.name)) + .map(|_| Some(())) } else { info!("No tag found with name '{}'", tag_name); Ok(None) @@ -256,16 +287,18 @@ impl TagDao for SqliteTagDao { #[cfg(test)] mod tests { - use actix_web::web::Data; - use std::{borrow::Borrow, cell::RefCell, collections::HashMap}; + use actix_web::web::{Data, Json}; + use std::{cell::RefCell, collections::HashMap}; use diesel::result::Error::NotFound; + use log::warn; use super::*; struct TestTagDao { tags: RefCell>, tagged_photos: RefCell>>, + tag_count: i32, } impl TestTagDao { @@ -273,6 +306,7 @@ mod tests { Self { tags: RefCell::new(vec![]), tagged_photos: RefCell::new(HashMap::new()), + tag_count: 0, } } } @@ -283,6 +317,9 @@ mod tests { } fn get_tags_for_path(&mut self, path: &str) -> anyhow::Result> { + info!("Getting test tags for: {:?}", path); + warn!("Tags for path: {:?}", self.tagged_photos); + Ok(self .tagged_photos .borrow() @@ -292,13 +329,18 @@ mod tests { } fn create_tag(&mut self, name: &str) -> anyhow::Result { + self.tag_count += 1; + let tag_id = self.tag_count; + let tag = Tag { - id: 0, + id: tag_id as i32, name: name.to_string(), created_time: Utc::now().timestamp(), }; self.tags.borrow_mut().push(tag.clone()); + debug!("Created tag: {:?}", tag); + Ok(tag) } @@ -323,7 +365,11 @@ mod tests { } fn tag_file(&mut self, path: &str, tag_id: i32) -> anyhow::Result { + debug!("Tagging file: {:?} with tag_id: {:?}", path, tag_id); + if let Some(tag) = self.tags.borrow().iter().find(|t| t.id == tag_id) { + debug!("Found tag: {:?}", tag); + let tagged_photo = TaggedPhoto { id: self.tagged_photos.borrow().len() as i32, tag_id: tag.id, @@ -331,10 +377,19 @@ mod tests { photo_name: path.to_string(), }; - //TODO: Add to existing tags (? huh) - self.tagged_photos - .borrow_mut() - .insert(path.to_string(), vec![tag.clone()]); + if self.tagged_photos.borrow().contains_key(path) { + let mut photo_tags = self.tagged_photos.borrow()[path].clone(); + photo_tags.push(tag.clone()); + + self.tagged_photos + .borrow_mut() + .insert(path.to_string(), photo_tags); + } else { + //TODO: Add to existing tags (? huh) + self.tagged_photos + .borrow_mut() + .insert(path.to_string(), vec![tag.clone()]); + } Ok(tagged_photo) } else { @@ -345,14 +400,14 @@ mod tests { #[actix_rt::test] async fn add_new_tag_test() { - let mut tag_dao = TestTagDao::new(); + let tag_dao = TestTagDao::new(); let claims = Claims::valid_user(String::from("1")); let body = AddTagRequest { file_name: String::from("test.png"), tag_name: String::from("test-tag"), }; - let mut tag_data = Data::new(Mutex::new(tag_dao)); + let tag_data = Data::new(Mutex::new(tag_dao)); add_tag(claims, web::Json(body), tag_data.clone()).await; let mut tag_dao = tag_data.lock().unwrap(); @@ -365,7 +420,7 @@ mod tests { #[actix_rt::test] async fn remove_tag_test() { - let mut tag_dao = TestTagDao::new(); + let tag_dao = TestTagDao::new(); let claims = Claims::valid_user(String::from("1")); let add_request = AddTagRequest { file_name: String::from("test.png"), @@ -388,4 +443,35 @@ mod tests { let previously_added_tagged_photo = tagged_photos.get("test.png").unwrap(); assert_eq!(previously_added_tagged_photo.len(), 0) } + + #[actix_rt::test] + async fn replace_tags_keeps_existing_tags_removes_extras_adds_missing_test() { + let mut tag_dao = TestTagDao::new(); + let new_tag = tag_dao.create_tag("Test").unwrap(); + let new_tag2 = tag_dao.create_tag("Test2").unwrap(); + let _ = tag_dao.create_tag("Test3").unwrap(); + + tag_dao.tag_file("test.jpg", new_tag.id).unwrap(); + tag_dao.tag_file("test.jpg", new_tag2.id).unwrap(); + + let claims = Claims::valid_user(String::from("1")); + let tag_data = Data::new(Mutex::new(tag_dao)); + + let add_tags_request = AddTagsRequest { + tag_ids: vec![1, 3], + file_name: String::from("test.jpg"), + }; + + update_tags(claims, tag_data.clone(), Json(add_tags_request)).await; + + let tag_dao = tag_data.lock().unwrap(); + let tags_for_test_photo = &tag_dao.tagged_photos.borrow()["test.jpg"]; + + assert_eq!(tags_for_test_photo.len(), 2); + // ID of 2 was removed and 3 was added + assert_eq!( + tags_for_test_photo.iter().find(|&t| t.name == "Test2"), + None + ); + } }