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

@@ -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<D: UserDao>(
     }
 }
 
-pub async fn login<D: UserDao>(creds: Json<LoginRequest>, user_dao: web::Data<Mutex<D>>) -> HttpResponse {
+pub async fn login<D: UserDao>(
+    creds: Json<LoginRequest>,
+    user_dao: web::Data<Mutex<D>>,
+) -> HttpResponse {
     debug!("Logging in: {}", creds.username);
 
     let mut user_dao = user_dao.lock().expect("Unable to get UserDao");

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};
 

src/database/schema.rs

@@ -33,9 +33,4 @@ table! {
 
 joinable!(tagged_photo -> tags (tag_id));
 
-allow_tables_to_appear_in_same_query!(
+allow_tables_to_appear_in_same_query!(favorites, tagged_photo, tags, users,);
-    favorites,
-    tagged_photo,
-    tags,
-    users,
-);

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(
+                .add_feature(add_tag_services::<_, SqliteTagDao>)
-                    web::resource("image/tags")
-                        .route(web::post().to(add_tag::<SqliteTagDao>))
-                        .route(web::get().to(get_tags::<SqliteTagDao>))
-                        .route(web::delete().to(remove_tagged_photo::<SqliteTagDao>)),
-                )
-                .service(web::resource("image/tags/all").route(web::get().to(get_all_tags::<SqliteTagDao>)))
-                .service(web::resource("image/tags/batch").route(web::post().to(update_tags::<SqliteTagDao>)))
                 .app_data(app_data.clone())
                 .app_data::<Data<Mutex<SqliteUserDao>>>(Data::new(Mutex::new(user_dao)))
                 .app_data::<Data<Mutex<Box<dyn FavoriteDao>>>>(Data::new(Mutex::new(Box::new(

src/service.rs

@@ -0,0 +1,16 @@
+use actix_web::App;
+
+pub trait ServiceBuilder<T> {
+    fn add_feature<F>(self, f: F) -> App<T>
+    where
+        F: Fn(App<T>) -> App<T>;
+}
+
+impl<T> ServiceBuilder<T> for App<T> {
+    fn add_feature<F>(self, create_feature: F) -> App<T>
+    where
+        F: Fn(App<T>) -> App<T>,
+    {
+        create_feature(self)
+    }
+}

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<D: TagDao>(
+pub fn add_tag_services<T, TagD: TagDao + 'static>(app: App<T>) -> App<T>
+where
+    T: ServiceFactory<ServiceRequest, Config = (), Error = actix_web::Error, InitError = ()>,
+{
+    app.service(
+        web::resource("image/tags")
+            .route(web::post().to(add_tag::<TagD>))
+            .route(web::get().to(get_tags::<TagD>))
+            .route(web::delete().to(remove_tagged_photo::<TagD>)),
+    )
+    .service(web::resource("image/tags/all").route(web::get().to(get_all_tags::<TagD>)))
+    .service(web::resource("image/tags/batch").route(web::post().to(update_tags::<TagD>)))
+}
+
+async fn add_tag<D: TagDao>(
     _: Claims,
     body: web::Json<AddTagRequest>,
     tag_dao: web::Data<Mutex<D>>,
@@ -32,7 +47,7 @@ pub async fn add_tag<D: TagDao>(
         .into_http_internal_err()
 }
 
-pub async fn get_tags<D: TagDao>(
+async fn get_tags<D: TagDao>(
     _: Claims,
     request: web::Query<ThumbnailRequest>,
     tag_dao: web::Data<Mutex<D>>,
@@ -44,7 +59,7 @@ pub async fn get_tags<D: TagDao>(
         .into_http_internal_err()
 }
 
-pub async fn get_all_tags<D: TagDao>(_: Claims, tag_dao: web::Data<Mutex<D>>) -> impl Responder {
+async fn get_all_tags<D: TagDao>(_: Claims, tag_dao: web::Data<Mutex<D>>) -> 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<D: TagDao>(_: Claims, tag_dao: web::Data<Mutex<D>>) ->
         .into_http_internal_err()
 }
 
-pub async fn remove_tagged_photo<D: TagDao>(
+async fn remove_tagged_photo<D: TagDao>(
     _: Claims,
     request: web::Json<AddTagRequest>,
     tag_dao: web::Data<Mutex<D>>,
@@ -70,39 +85,55 @@ pub async fn remove_tagged_photo<D: TagDao>(
         .into_http_internal_err()
 }
 
-pub async fn update_tags<D: TagDao>(_: Claims, tag_dao: web::Data<Mutex<D>>, request: web::Json<AddTagsRequest>) -> impl Responder {
+async fn update_tags<D: TagDao>(
-    let mut dao = tag_dao.lock()
+    _: Claims,
-        .expect("Unable to get TagDao");
+    tag_dao: web::Data<Mutex<D>>,
+    request: web::Json<AddTagsRequest>,
+) -> impl Responder {
+    let mut dao = tag_dao.lock().expect("Unable to get TagDao");
 
-    return dao.get_tags_for_path(&request.file_name)
+    return dao
-        .and_then(|existing_tags| {
+        .get_tags_for_path(&request.file_name)
-            dao.get_all_tags().map(|all| (existing_tags, all))
+        .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::<Vec<&Tag>>();
 
             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::<Vec<&Tag>>();
 
             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)]
@@ -256,16 +287,18 @@ impl TagDao for SqliteTagDao {
 
 #[cfg(test)]
 mod tests {
-    use actix_web::web::Data;
+    use actix_web::web::{Data, Json};
-    use std::{borrow::Borrow, cell::RefCell, collections::HashMap};
+    use std::{cell::RefCell, collections::HashMap};
 
     use diesel::result::Error::NotFound;
+    use log::warn;
 
     use super::*;
 
     struct TestTagDao {
         tags: RefCell<Vec<Tag>>,
         tagged_photos: RefCell<HashMap<String, Vec<Tag>>>,
+        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<Vec<Tag>> {
+            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<Tag> {
+            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<TaggedPhoto> {
+            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(),
                 };
 
+                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
+        );
+    }
 }