Extract FileSystem to a trait for better testability

Added some tests around filtering and searching by Tags. Added the
ability to use an in-memory Sqlite DB for more integration tests.

src/database/mod.rs

@@ -27,6 +27,24 @@ impl SqliteUserDao {
     }
 }
 
+#[cfg(test)]
+pub mod test {
+    use diesel::{Connection, SqliteConnection};
+    use diesel_migrations::{embed_migrations, EmbeddedMigrations, MigrationHarness};
+
+    const DB_MIGRATIONS: EmbeddedMigrations = embed_migrations!();
+
+    pub fn in_memory_db_connection() -> SqliteConnection {
+        let mut connection = SqliteConnection::establish(":memory:")
+            .expect("Unable to create in-memory db connection");
+        connection
+            .run_pending_migrations(DB_MIGRATIONS)
+            .expect("Failure running DB migrations");
+
+        return connection;
+    }
+}
+
 impl UserDao for SqliteUserDao {
     // TODO: Should probably use Result here
     fn create_user(&mut self, user: &str, pass: &str) -> Option<User> {

src/files.rs

@@ -19,16 +19,17 @@ use crate::AppState;
 use crate::tags::TagDao;
 use path_absolutize::*;
 
-pub async fn list_photos<TagD: TagDao>(
+pub async fn list_photos<TagD: TagDao, FS: FileSystemAccess>(
     _: Claims,
     req: Query<FilesRequest>,
     app_state: web::Data<AppState>,
+    file_system: web::Data<FS>,
     tag_dao: web::Data<Mutex<TagD>>,
 ) -> HttpResponse {
     let path = &req.path;
-    if let Some(path) = is_valid_full_path(&PathBuf::from(&app_state.base_path), path) {
+
+    if let Ok(files) = file_system.get_files_for_path(path) {
         debug!("Valid path: {:?}", path);
-        let files = list_files(&path).unwrap_or_default();
 
         let photos = files
             .iter()
@@ -36,7 +37,7 @@ pub async fn list_photos<TagD: TagDao>(
                 f.metadata().map_or_else(
                     |e| {
                         error!("Failed getting file metadata: {:?}", e);
-                        false
+                        f.extension().is_some()
                     },
                     |md| md.is_file(),
                 )
@@ -110,10 +111,13 @@ pub fn is_image_or_video(path: &Path) -> bool {
         || extension == "nef"
 }
 
-pub fn is_valid_full_path<P: AsRef<Path> + Debug>(base: &P, path: &str) -> Option<PathBuf> {
+pub fn is_valid_full_path<P: AsRef<Path> + Debug + AsRef<std::ffi::OsStr>>(
-    debug!("Base: {:?}. Path: {}", base, path);
+    base: &P,
+    path: &P,
+) -> Option<PathBuf> {
+    debug!("Base: {:?}. Path: {:?}", base, path);
 
-    let path = PathBuf::from(path);
+    let path = PathBuf::from(&path);
     let mut path = if path.is_relative() {
         let mut full_path = PathBuf::new();
         full_path.push(base);
@@ -153,31 +157,95 @@ fn is_path_above_base_dir<P: AsRef<Path> + Debug>(
         )
 }
 
+pub trait FileSystemAccess {
+    fn get_files_for_path(&self, path: &str) -> anyhow::Result<Vec<PathBuf>>;
+}
+
+pub struct RealFileSystem {
+    base_path: String,
+}
+
+impl RealFileSystem {
+    pub(crate) fn new(base_path: String) -> RealFileSystem {
+        RealFileSystem { base_path }
+    }
+}
+
+impl FileSystemAccess for RealFileSystem {
+    fn get_files_for_path(&self, path: &str) -> anyhow::Result<Vec<PathBuf>> {
+        is_valid_full_path(&PathBuf::from(&self.base_path), &PathBuf::from(path))
+            .map(|path| {
+                debug!("Valid path: {:?}", path);
+                list_files(&path).unwrap_or_default()
+            })
+            .context("Invalid path")
+    }
+}
+
 #[cfg(test)]
 mod tests {
+    use crate::database::connect;
+    use crate::tags::SqliteTagDao;
+    use actix_web::web::Data;
+    use diesel::{Connection, SqliteConnection};
+    use std::collections::HashMap;
     use std::env;
+    use std::ffi::OsStr;
     use std::fs::File;
 
     use super::*;
 
+    struct FakeFileSystem {
+        files: HashMap<String, Vec<String>>,
+        err: bool,
+    }
+
+    impl FakeFileSystem {
+        fn with_error() -> FakeFileSystem {
+            FakeFileSystem {
+                files: HashMap::new(),
+                err: true,
+            }
+        }
+
+        fn new(files: HashMap<String, Vec<String>>) -> FakeFileSystem {
+            FakeFileSystem { files, err: false }
+        }
+    }
+
+    impl FileSystemAccess for FakeFileSystem {
+        fn get_files_for_path(&self, path: &str) -> anyhow::Result<Vec<PathBuf>> {
+            if self.err {
+                Err(anyhow!("Error for test"))
+            } else {
+                if let Some(files) = self.files.get(path) {
+                    Ok(files
+                        .iter()
+                        .map(|p| PathBuf::from(p))
+                        .collect::<Vec<PathBuf>>())
+                } else {
+                    Ok(Vec::new())
+                }
+            }
+        }
+    }
+
     mod api {
         use super::*;
         use actix::Actor;
-        use actix_web::{
+        use actix_web::{web::Query, HttpResponse};
-            web::{self, Query},
-            HttpResponse,
-        };
 
         use crate::{
-            data::{Claims, PhotosResponse, ThumbnailRequest},
+            data::{Claims, PhotosResponse},
             testhelpers::BodyReader,
             video::StreamActor,
             AppState,
         };
 
-        use std::{fs, sync::Arc};
+        use crate::database::test::in_memory_db_connection;
-        use actix_web::web::Data;
         use crate::tags::SqliteTagDao;
+        use actix_web::web::Data;
+        use std::{fs, sync::Arc};
 
         fn setup() {
             let _ = env_logger::builder().is_test(true).try_init();
@@ -194,7 +262,7 @@ mod tests {
 
             let request: Query<FilesRequest> = Query::from_query("path=").unwrap();
 
-            let mut temp_photo = std::env::temp_dir();
+            let mut temp_photo = env::temp_dir();
             let mut tmp = temp_photo.clone();
 
             tmp.push("test-dir");
@@ -202,24 +270,25 @@ mod tests {
 
             temp_photo.push("photo.jpg");
 
-            fs::File::create(temp_photo.clone()).unwrap();
+            File::create(temp_photo.clone()).unwrap();
 
             let response: HttpResponse = list_photos(
                 claims,
                 request,
-                web::Data::new(AppState::new(
+                Data::new(AppState::new(
                     Arc::new(StreamActor {}.start()),
                     String::from("/tmp"),
                     String::from("/tmp/thumbs"),
                 )),
+                Data::new(RealFileSystem::new(String::from("/tmp"))),
                 Data::new(Mutex::new(SqliteTagDao::default())),
             )
-                .await;
+            .await;
             let status = response.status();
+            assert_eq!(status, 200);
 
             let body: PhotosResponse = serde_json::from_str(&response.read_to_str()).unwrap();
 
-            assert_eq!(status, 200);
             assert!(body.photos.contains(&String::from("photo.jpg")));
             assert!(body.dirs.contains(&String::from("test-dir")));
             assert!(body
@@ -246,28 +315,145 @@ mod tests {
             let response = list_photos(
                 claims,
                 request,
-                web::Data::new(AppState::new(
+                Data::new(AppState::new(
                     Arc::new(StreamActor {}.start()),
                     String::from("/tmp"),
                     String::from("/tmp/thumbs"),
                 )),
+                Data::new(RealFileSystem::new(String::from("./"))),
                 Data::new(Mutex::new(SqliteTagDao::default())),
             )
-                .await;
+            .await;
 
             assert_eq!(response.status(), 400);
         }
+
+        #[actix_rt::test]
+        async fn get_files_with_tag_any_filter() {
+            setup();
+
+            let claims = Claims {
+                sub: String::from("1"),
+                exp: 12345,
+            };
+
+            let request: Query<FilesRequest> = Query::from_query("path=&tag_ids=1,3").unwrap();
+
+            let mut tag_dao = SqliteTagDao::new(in_memory_db_connection());
+
+            let tag1 = tag_dao.create_tag("tag1").unwrap();
+            let tag2 = tag_dao.create_tag("tag2").unwrap();
+            let tag3 = tag_dao.create_tag("tag3").unwrap();
+
+            &tag_dao.tag_file("test.jpg", tag1.id).unwrap();
+            &tag_dao.tag_file("test.jpg", tag3.id).unwrap();
+
+            let mut files = HashMap::new();
+            files.insert(
+                String::from(""),
+                vec![
+                    String::from("file1.txt"),
+                    String::from("test.jpg"),
+                    String::from("some-other.jpg"),
+                ],
+            );
+
+            let response: HttpResponse = list_photos(
+                claims,
+                request,
+                Data::new(AppState::new(
+                    Arc::new(StreamActor {}.start()),
+                    String::from(""),
+                    String::from("/tmp/thumbs"),
+                )),
+                Data::new(FakeFileSystem::new(files)),
+                Data::new(Mutex::new(tag_dao)),
+            )
+            .await;
+
+            assert_eq!(200, response.status());
+
+            let body: PhotosResponse = serde_json::from_str(&response.read_to_str()).unwrap();
+            assert_eq!(1, body.photos.len());
+            assert!(body.photos.contains(&String::from("test.jpg")));
+        }
+
+        #[actix_rt::test]
+        async fn get_files_with_tag_all_filter() {
+            setup();
+
+            let claims = Claims {
+                sub: String::from("1"),
+                exp: 12345,
+            };
+
+            let mut tag_dao = SqliteTagDao::new(in_memory_db_connection());
+
+            let tag1 = tag_dao.create_tag("tag1").unwrap();
+            let tag2 = tag_dao.create_tag("tag2").unwrap();
+            let tag3 = tag_dao.create_tag("tag3").unwrap();
+
+            &tag_dao.tag_file("test.jpg", tag1.id).unwrap();
+            &tag_dao.tag_file("test.jpg", tag3.id).unwrap();
+
+            // Should get filtered since it doesn't have tag3
+            tag_dao.tag_file("some-other.jpg", tag1.id).unwrap();
+
+            let mut files = HashMap::new();
+            files.insert(
+                String::from(""),
+                vec![
+                    String::from("file1.txt"),
+                    String::from("test.jpg"),
+                    String::from("some-other.jpg"),
+                ],
+            );
+
+            let request: Query<FilesRequest> =
+                Query::from_query("path=&tag_ids=1,3&tag_filter_mode=All").unwrap();
+
+            let response: HttpResponse = list_photos(
+                claims,
+                request,
+                Data::new(AppState::new(
+                    Arc::new(StreamActor {}.start()),
+                    String::from(""),
+                    String::from("/tmp/thumbs"),
+                )),
+                Data::new(FakeFileSystem::new(files)),
+                Data::new(Mutex::new(tag_dao)),
+            )
+            .await;
+
+            assert_eq!(200, response.status());
+
+            let body: PhotosResponse = serde_json::from_str(&response.read_to_str()).unwrap();
+            assert_eq!(1, body.photos.len());
+            assert!(body.photos.contains(&String::from("test.jpg")));
+        }
     }
 
     #[test]
     fn directory_traversal_test() {
         let base = env::temp_dir();
-        assert_eq!(None, is_valid_full_path(&base, "../"));
+        assert_eq!(None, is_valid_full_path(&base, &PathBuf::from("../")));
-        assert_eq!(None, is_valid_full_path(&base, ".."));
+        assert_eq!(None, is_valid_full_path(&base, &PathBuf::from("..")));
-        assert_eq!(None, is_valid_full_path(&base, "fake/../../../"));
+        assert_eq!(
-        assert_eq!(None, is_valid_full_path(&base, "../../../etc/passwd"));
+            None,
-        assert_eq!(None, is_valid_full_path(&base, "..//etc/passwd"));
+            is_valid_full_path(&base, &PathBuf::from("fake/../../../"))
-        assert_eq!(None, is_valid_full_path(&base, "../../etc/passwd"));
+        );
+        assert_eq!(
+            None,
+            is_valid_full_path(&base, &PathBuf::from("../../../etc/passwd"))
+        );
+        assert_eq!(
+            None,
+            is_valid_full_path(&base, &PathBuf::from("..//etc/passwd"))
+        );
+        assert_eq!(
+            None,
+            is_valid_full_path(&base, &PathBuf::from("../../etc/passwd"))
+        );
     }
 
     #[test]
@@ -277,7 +463,7 @@ mod tests {
         test_file.push("test.png");
         File::create(test_file).unwrap();
 
-        assert!(is_valid_full_path(&base, "test.png").is_some());
+        assert!(is_valid_full_path(&base, &PathBuf::from("test.png")).is_some());
     }
 
     #[test]
@@ -288,7 +474,7 @@ mod tests {
         let mut test_file = PathBuf::from(&base);
         test_file.push(path);
 
-        assert_eq!(None, is_valid_full_path(&base, path));
+        assert_eq!(None, is_valid_full_path(&base, &test_file));
     }
 
     #[test]
@@ -298,11 +484,11 @@ mod tests {
         test_file.push("test.png");
         File::create(&test_file).unwrap();
 
-        assert!(is_valid_full_path(&base, test_file.to_str().unwrap()).is_some());
+        assert!(is_valid_full_path(&base, &test_file).is_some());
 
         assert_eq!(
             Some(PathBuf::from("/tmp/test.png")),
-            is_valid_full_path(&base, "/tmp/test.png")
+            is_valid_full_path(&base, &PathBuf::from("/tmp/test.png"))
         );
     }
 

src/main.rs

@@ -35,7 +35,7 @@ use log::{debug, error, info};
 use crate::auth::login;
 use crate::data::*;
 use crate::database::*;
-use crate::files::{is_image_or_video, is_valid_full_path};
+use crate::files::{is_image_or_video, is_valid_full_path, RealFileSystem};
 use crate::service::ServiceBuilder;
 use crate::state::AppState;
 use crate::tags::*;
@@ -156,9 +156,10 @@ async fn upload_image(
     let path = file_path.unwrap_or_else(|| app_state.base_path.clone());
     if !file_content.is_empty() {
         let full_path = PathBuf::from(&path).join(file_name.unwrap());
-        if let Some(full_path) =
+        if let Some(full_path) = is_valid_full_path(
-            is_valid_full_path(&app_state.base_path, full_path.to_str().unwrap_or(""))
+            &app_state.base_path,
-        {
+            &full_path.to_str().unwrap().to_string(),
+        ) {
             if !full_path.is_file() && is_image_or_video(&full_path) {
                 let mut file = File::create(full_path).unwrap();
                 file.write_all(&file_content).unwrap();
@@ -474,7 +475,10 @@ fn main() -> std::io::Result<()> {
             App::new()
                 .wrap(middleware::Logger::default())
                 .service(web::resource("/login").route(web::post().to(login::<SqliteUserDao>)))
-                .service(web::resource("/photos").route(web::get().to(files::list_photos::<SqliteTagDao>)))
+                .service(
+                    web::resource("/photos")
+                        .route(web::get().to(files::list_photos::<SqliteTagDao, RealFileSystem>)),
+                )
                 .service(get_image)
                 .service(upload_image)
                 .service(generate_video)
@@ -486,6 +490,7 @@ fn main() -> std::io::Result<()> {
                 .service(get_file_metadata)
                 .add_feature(add_tag_services::<_, SqliteTagDao>)
                 .app_data(app_data.clone())
+                .app_data::<Data<RealFileSystem>>(Data::new(RealFileSystem::new(app_data.base_path.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(
                     favorites_dao,