Merge pull request 'Improve test coverage and logging' (#7) from feature/improve-test-coverage into master

Reviewed-on: #7

Cargo.lock

@@ -60,20 +60,6 @@ dependencies = [
  "trust-dns-resolver",
 ]
 
-[[package]]
-name = "actix-cors"
-version = "0.5.4"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "36b133d8026a9f209a9aeeeacd028e7451bcca975f592881b305d37983f303d7"
-dependencies = [
- "actix-web",
- "derive_more",
- "futures-util",
- "log",
- "once_cell",
- "tinyvec",
-]
-
 [[package]]
 name = "actix-files"
 version = "0.5.0"
@@ -1200,7 +1186,6 @@ name = "image-api"
 version = "0.1.0"
 dependencies = [
  "actix",
- "actix-cors",
  "actix-files",
  "actix-multipart",
  "actix-rt 2.1.0",
@@ -1221,7 +1206,6 @@ dependencies = [
  "serde",
  "serde_json",
  "sha2",
- "tokio 1.2.0",
  "walkdir",
 ]
 

Cargo.toml

@@ -12,7 +12,6 @@ actix-web = "3"
 actix-rt = "2"
 actix-files = "0.5"
 actix-multipart = "0.3.0"
-actix-cors = "0.5"
 futures = "0.3.5"
 jsonwebtoken = "7.2.0"
 serde = "1"
@@ -27,7 +26,6 @@ image = "0.23.7"
 walkdir = "2"
 rayon = "1.3"
 notify = "4.0"
-tokio = "1"
 path-absolutize = "3.0.6"
 log="0.4"
 env_logger="0.8"

src/auth.rs

@@ -1,19 +1,23 @@
-use actix_web::web::{HttpResponse, Json};
+use actix_web::web::{self, HttpResponse, Json};
 use actix_web::{post, Responder};
 use chrono::{Duration, Utc};
 use jsonwebtoken::{encode, EncodingKey, Header};
-use log::debug;
+use log::{debug, error};
 
-use crate::data::LoginRequest;
+use crate::{
-use crate::data::{secret_key, Claims, CreateAccountRequest, Token};
+    data::{secret_key, Claims, CreateAccountRequest, LoginRequest, Token},
-use crate::database::{create_user, get_user, user_exists};
+    database::UserDao,
+};
 
 #[post("/register")]
-async fn register(user: Json<CreateAccountRequest>) -> impl Responder {
+async fn register(
+    user: Json<CreateAccountRequest>,
+    user_dao: web::Data<Box<dyn UserDao>>,
+) -> impl Responder {
     if !user.username.is_empty() && user.password.len() > 5 && user.password == user.confirmation {
-        if user_exists(&user.username) {
+        if user_dao.user_exists(&user.username) {
             HttpResponse::BadRequest()
-        } else if let Some(_user) = create_user(&user.username, &user.password) {
+        } else if let Some(_user) = user_dao.create_user(&user.username, &user.password) {
             HttpResponse::Ok()
         } else {
             HttpResponse::InternalServerError()
@@ -23,10 +27,12 @@ async fn register(user: Json<CreateAccountRequest>) -> impl Responder {
     }
 }
 
-#[post("/login")]
+pub async fn login(
-async fn login(creds: Json<LoginRequest>) -> impl Responder {
+    creds: Json<LoginRequest>,
+    user_dao: web::Data<Box<dyn UserDao>>,
+) -> HttpResponse {
     debug!("Logging in: {}", creds.username);
-    if let Some(user) = get_user(&creds.username, &creds.password) {
+    if let Some(user) = user_dao.get_user(&creds.username, &creds.password) {
         let claims = Claims {
             sub: user.id.to_string(),
             exp: (Utc::now() + Duration::days(5)).timestamp(),
@@ -39,6 +45,59 @@ async fn login(creds: Json<LoginRequest>) -> impl Responder {
         .unwrap();
         HttpResponse::Ok().json(Token { token: &token })
     } else {
+        error!("User not found during login: '{}'", creds.username);
         HttpResponse::NotFound().finish()
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::database::testhelpers::{BodyReader, TestUserDao};
+
+    #[actix_rt::test]
+    async fn test_login_reports_200_when_user_exists() {
+        let dao = TestUserDao::new();
+        dao.create_user("user", "pass");
+
+        let j = Json(LoginRequest {
+            username: "user".to_string(),
+            password: "pass".to_string(),
+        });
+
+        let response = login(j, web::Data::new(Box::new(dao))).await;
+
+        assert_eq!(response.status(), 200);
+    }
+
+    #[actix_rt::test]
+    async fn test_login_returns_token_on_success() {
+        let dao = TestUserDao::new();
+        dao.create_user("user", "password");
+
+        let j = Json(LoginRequest {
+            username: "user".to_string(),
+            password: "password".to_string(),
+        });
+
+        let response = login(j, web::Data::new(Box::new(dao))).await;
+
+        assert_eq!(response.status(), 200);
+        assert!(response.body().read_to_str().contains("\"token\""));
+    }
+
+    #[actix_rt::test]
+    async fn test_login_reports_400_when_user_does_not_exist() {
+        let dao = TestUserDao::new();
+        dao.create_user("user", "password");
+
+        let j = Json(LoginRequest {
+            username: "doesnotexist".to_string(),
+            password: "password".to_string(),
+        });
+
+        let response = login(j, web::Data::new(Box::new(dao))).await;
+
+        assert_eq!(response.status(), 404);
+    }
+}

src/database/mod.rs

@@ -1,38 +1,48 @@
 use bcrypt::{hash, verify, DEFAULT_COST};
 use diesel::prelude::*;
 use diesel::sqlite::SqliteConnection;
-use dotenv::dotenv;
 
 use crate::database::models::{Favorite, InsertFavorite, InsertUser, User};
 
 mod models;
 mod schema;
 
-fn connect() -> SqliteConnection {
+pub trait UserDao {
-    dotenv().ok();
+    fn create_user(&self, user: &str, password: &str) -> Option<User>;
-
+    fn get_user(&self, user: &str, password: &str) -> Option<User>;
-    let db_url = dotenv::var("DATABASE_URL").expect("DATABASE_URL must be set");
+    fn user_exists(&self, user: &str) -> bool;
-    SqliteConnection::establish(&db_url).expect("Error connecting to DB")
 }
 
-// TODO: Should probably use Result here
+pub struct SqliteUserDao {
-pub fn create_user(user: &str, pass: &str) -> Option<User> {
+    connection: SqliteConnection,
+}
+
+impl SqliteUserDao {
+    pub fn new() -> Self {
+        Self {
+            connection: connect(),
+        }
+    }
+}
+
+impl UserDao for SqliteUserDao {
+    // TODO: Should probably use Result here
+    fn create_user(&self, user: &str, pass: &str) -> std::option::Option<User> {
         use schema::users::dsl::*;
 
         let hashed = hash(pass, DEFAULT_COST);
         if let Ok(hash) = hashed {
-        let connection = connect();
             diesel::insert_into(users)
                 .values(InsertUser {
                     username: user,
                     password: &hash,
                 })
-            .execute(&connection)
+                .execute(&self.connection)
                 .unwrap();
 
             match users
-            .filter(username.eq(user))
+                .filter(username.eq(username))
-            .load::<User>(&connection)
+                .load::<User>(&self.connection)
                 .unwrap()
                 .first()
             {
@@ -42,31 +52,37 @@ pub fn create_user(user: &str, pass: &str) -> Option<User> {
         } else {
             None
         }
-}
+    }
 
-pub fn get_user(user: &str, pass: &str) -> Option<User> {
+    fn get_user(&self, user: &str, pass: &str) -> Option<User> {
         use schema::users::dsl::*;
 
         match users
             .filter(username.eq(user))
-        .load::<User>(&connect())
+            .load::<User>(&self.connection)
             .unwrap_or_default()
             .first()
         {
             Some(u) if verify(pass, &u.password).unwrap_or(false) => Some(u.clone()),
             _ => None,
         }
-}
+    }
 
-pub fn user_exists(name: &str) -> bool {
+    fn user_exists(&self, user: &str) -> bool {
         use schema::users::dsl::*;
 
         users
-        .filter(username.eq(name))
+            .filter(username.eq(user))
-        .load::<User>(&connect())
+            .load::<User>(&self.connection)
             .unwrap_or_default()
             .first()
             .is_some()
+    }
+}
+
+fn connect() -> SqliteConnection {
+    let db_url = dotenv::var("DATABASE_URL").expect("DATABASE_URL must be set");
+    SqliteConnection::establish(&db_url).expect("Error connecting to DB")
 }
 
 pub fn add_favorite(user_id: i32, favorite_path: String) {
@@ -90,3 +106,81 @@ pub fn get_favorites(user_id: i32) -> Vec<Favorite> {
         .load::<Favorite>(&connect())
         .unwrap_or_default()
 }
+
+#[cfg(test)]
+pub mod testhelpers {
+    use actix_web::dev::{Body, ResponseBody};
+
+    use super::{models::User, UserDao};
+    use std::cell::RefCell;
+    use std::option::Option;
+
+    pub struct TestUserDao {
+        pub user_map: RefCell<Vec<User>>,
+    }
+
+    impl TestUserDao {
+        pub fn new() -> Self {
+            Self {
+                user_map: RefCell::new(Vec::new()),
+            }
+        }
+    }
+
+    impl UserDao for TestUserDao {
+        fn create_user(&self, username: &str, password: &str) -> Option<User> {
+            let u = User {
+                id: (self.user_map.borrow().len() + 1) as i32,
+                username: username.to_string(),
+                password: password.to_string(),
+            };
+
+            self.user_map.borrow_mut().push(u.clone());
+
+            Some(u)
+        }
+
+        fn get_user(&self, user: &str, pass: &str) -> Option<User> {
+            match self
+                .user_map
+                .borrow()
+                .iter()
+                .filter(|u| u.username == user && u.password == pass)
+                .collect::<Vec<&User>>()
+                .first()
+            {
+                Some(u) => {
+                    let copy = (*u).clone();
+                    Some(copy)
+                }
+                None => None,
+            }
+        }
+
+        fn user_exists(&self, user: &str) -> bool {
+            self.user_map
+                .borrow()
+                .iter()
+                .filter(|u| u.username == user)
+                .collect::<Vec<&User>>()
+                .first()
+                .is_some()
+        }
+    }
+
+    pub trait BodyReader {
+        fn read_to_str(&self) -> &str;
+    }
+
+    impl BodyReader for ResponseBody<Body> {
+        fn read_to_str(&self) -> &str {
+            match self {
+                ResponseBody::Body(body) => match body {
+                    Body::Bytes(b) => std::str::from_utf8(&b).unwrap(),
+                    _ => panic!("Unknown response body content"),
+                },
+                _ => panic!("Unknown response body"),
+            }
+        }
+    }
+}

src/main.rs

@@ -3,6 +3,7 @@ extern crate diesel;
 extern crate rayon;
 
 use crate::auth::login;
+use database::{SqliteUserDao, UserDao};
 use futures::stream::StreamExt;
 use std::fs::File;
 use std::io::prelude::*;
@@ -13,8 +14,11 @@ use std::sync::Arc;
 use actix::{Actor, Addr};
 use actix_files::NamedFile;
 use actix_multipart as mp;
-use actix_web::web::{HttpRequest, HttpResponse, Json};
 use actix_web::{get, post, web, App, HttpServer, Responder};
+use actix_web::{
+    middleware,
+    web::{HttpRequest, HttpResponse, Json},
+};
 use notify::{watcher, DebouncedEvent, RecursiveMode, Watcher};
 use rayon::prelude::*;
 use serde::Serialize;
@@ -156,7 +160,7 @@ async fn generate_video(
         let filename = name.to_str().expect("Filename should convert to string");
         let playlist = format!("tmp/{}.m3u8", filename);
         if let Some(path) = is_valid_path(&body.path) {
-            if let Ok(child) = create_playlist(&path.to_str().unwrap(), &playlist) {
+            if let Ok(child) = create_playlist(&path.to_str().unwrap(), &playlist).await {
                 data.stream_manager
                     .do_send(ProcessMessage(playlist.clone(), child));
             }
@@ -328,8 +332,10 @@ fn main() -> std::io::Result<()> {
     });
 
     HttpServer::new(move || {
+        let user_dao = SqliteUserDao::new();
         App::new()
-            .service(login)
+            .wrap(middleware::Logger::default())
+            .service(web::resource("/login").route(web::post().to(login)))
             .service(list_photos)
             .service(get_image)
             .service(upload_image)
@@ -339,6 +345,7 @@ fn main() -> std::io::Result<()> {
             .service(favorites)
             .service(post_add_favorite)
             .app_data(app_data.clone())
+            .data::<Box<dyn UserDao>>(Box::new(user_dao))
     })
     .bind(dotenv::var("BIND_URL").unwrap())?
     .bind("localhost:8088")?

src/video.rs

@@ -39,7 +39,7 @@ impl Handler<ProcessMessage> for StreamActor {
     }
 }
 
-pub fn create_playlist(video_path: &str, playlist_file: &str) -> Result<Child> {
+pub async fn create_playlist(video_path: &str, playlist_file: &str) -> Result<Child> {
     if Path::new(playlist_file).exists() {
         debug!("Playlist already exists: {}", playlist_file);
         return Err(std::io::Error::from(std::io::ErrorKind::AlreadyExists));
@@ -51,7 +51,7 @@ pub fn create_playlist(video_path: &str, playlist_file: &str) -> Result<Child> {
         .arg("-c:v")
         .arg("h264")
         .arg("-crf")
-        .arg("23")
+        .arg("21")
         .arg("-preset")
         .arg("veryfast")
         .arg("-hls_time")
@@ -67,7 +67,7 @@ pub fn create_playlist(video_path: &str, playlist_file: &str) -> Result<Child> {
 
     let start_time = std::time::Instant::now();
     loop {
-        std::thread::sleep(std::time::Duration::from_secs(1));
+        actix::clock::delay_for(std::time::Duration::from_secs(1)).await;
 
         if Path::new(playlist_file).exists()
             || std::time::Instant::now() - start_time > std::time::Duration::from_secs(5)