From 636701a69ef53f5dbb88b7ff9295bdd8e48cbbc1 Mon Sep 17 00:00:00 2001 From: Cameron Date: Tue, 23 Dec 2025 22:30:53 -0500 Subject: [PATCH] Refactor file type checking for better consistency Fix tests --- src/cleanup/phase1.rs | 12 ++--- src/cleanup/phase2.rs | 22 +------- src/exif.rs | 11 ++-- src/file_types.rs | 85 +++++++++++++++++++++++++++++ src/files.rs | 123 +++++++++++++++++++----------------------- src/lib.rs | 6 +-- src/main.rs | 43 ++++++--------- 7 files changed, 170 insertions(+), 132 deletions(-) create mode 100644 src/file_types.rs diff --git a/src/cleanup/phase1.rs b/src/cleanup/phase1.rs index 9717a7b..f3aed33 100644 --- a/src/cleanup/phase1.rs +++ b/src/cleanup/phase1.rs @@ -1,13 +1,12 @@ use crate::cleanup::database_updater::DatabaseUpdater; use crate::cleanup::types::{CleanupConfig, CleanupStats}; +use crate::file_types::IMAGE_EXTENSIONS; use anyhow::Result; use log::{error, warn}; use std::path::PathBuf; // All supported image extensions to try -const SUPPORTED_EXTENSIONS: &[&str] = &[ - "jpg", "jpeg", "png", "webp", "tiff", "tif", "heif", "heic", "avif", "nef", -]; +const SUPPORTED_EXTENSIONS: &[&str] = IMAGE_EXTENSIONS; /// Phase 1: Resolve missing files by searching for alternative extensions pub fn resolve_missing_files( @@ -111,9 +110,10 @@ fn find_file_with_alternative_extension( if test_path.exists() { // Convert back to relative path if let Ok(rel) = test_path.strip_prefix(base_path) - && let Some(rel_str) = rel.to_str() { - return Some(rel_str.to_string()); - } + && let Some(rel_str) = rel.to_str() + { + return Some(rel_str.to_string()); + } } } diff --git a/src/cleanup/phase2.rs b/src/cleanup/phase2.rs index 3503f66..209a2f0 100644 --- a/src/cleanup/phase2.rs +++ b/src/cleanup/phase2.rs @@ -183,26 +183,8 @@ pub fn validate_file_types( /// Check if a file is a supported media file based on extension fn is_supported_media_file(path: &Path) -> bool { - if let Some(ext) = path.extension() - && let Some(ext_str) = ext.to_str() { - let ext_lower = ext_str.to_lowercase(); - return matches!( - ext_lower.as_str(), - "jpg" - | "jpeg" - | "png" - | "webp" - | "tiff" - | "tif" - | "heif" - | "heic" - | "avif" - | "nef" - | "mp4" - | "mov" - ); - } - false + use crate::file_types::is_media_file; + is_media_file(path) } #[derive(Debug)] diff --git a/src/exif.rs b/src/exif.rs index 7bb0cbc..c096f71 100644 --- a/src/exif.rs +++ b/src/exif.rs @@ -7,8 +7,7 @@ use exif::{In, Reader, Tag, Value}; use log::debug; use serde::{Deserialize, Serialize}; -#[derive(Debug, Clone, Serialize, Deserialize)] -#[derive(Default)] +#[derive(Debug, Clone, Serialize, Deserialize, Default)] pub struct ExifData { pub camera_make: Option, pub camera_model: Option, @@ -26,7 +25,6 @@ pub struct ExifData { pub date_taken: Option, } - pub fn supports_exif(path: &Path) -> bool { if let Some(ext) = path.extension() { let ext_lower = ext.to_string_lossy().to_lowercase(); @@ -248,9 +246,10 @@ fn extract_gps_altitude(exif: &exif::Exif) -> Option { // Check if below sea level if let Some(ref_field) = exif.get_field(Tag::GPSAltitudeRef, In::PRIMARY) && let Some(ref_val) = get_u32_value(ref_field) - && ref_val == 1 { - return Some(-altitude); - } + && ref_val == 1 + { + return Some(-altitude); + } Some(altitude) } diff --git a/src/file_types.rs b/src/file_types.rs new file mode 100644 index 0000000..ac99085 --- /dev/null +++ b/src/file_types.rs @@ -0,0 +1,85 @@ +use std::path::Path; +use walkdir::DirEntry; + +/// Supported image file extensions +pub const IMAGE_EXTENSIONS: &[&str] = &[ + "jpg", "jpeg", "png", "webp", "tiff", "tif", "heif", "heic", "avif", "nef", +]; + +/// Supported video file extensions +pub const VIDEO_EXTENSIONS: &[&str] = &["mp4", "mov", "avi", "mkv"]; + +/// Check if a path has an image extension +pub fn is_image_file(path: &Path) -> bool { + if let Some(ext) = path.extension().and_then(|e| e.to_str()) { + let ext_lower = ext.to_lowercase(); + IMAGE_EXTENSIONS.contains(&ext_lower.as_str()) + } else { + false + } +} + +/// Check if a path has a video extension +pub fn is_video_file(path: &Path) -> bool { + if let Some(ext) = path.extension().and_then(|e| e.to_str()) { + let ext_lower = ext.to_lowercase(); + VIDEO_EXTENSIONS.contains(&ext_lower.as_str()) + } else { + false + } +} + +/// Check if a path has a supported media extension (image or video) +pub fn is_media_file(path: &Path) -> bool { + is_image_file(path) || is_video_file(path) +} + +/// Check if a DirEntry is an image file (for walkdir usage) +pub fn direntry_is_image(entry: &DirEntry) -> bool { + is_image_file(&entry.path()) +} + +/// Check if a DirEntry is a video file (for walkdir usage) +pub fn direntry_is_video(entry: &DirEntry) -> bool { + is_video_file(&entry.path()) +} + +/// Check if a DirEntry is a media file (for walkdir usage) +pub fn direntry_is_media(entry: &DirEntry) -> bool { + is_media_file(&entry.path()) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::Path; + + #[test] + fn test_is_image_file() { + assert!(is_image_file(Path::new("photo.jpg"))); + assert!(is_image_file(Path::new("photo.JPG"))); + assert!(is_image_file(Path::new("photo.png"))); + assert!(is_image_file(Path::new("photo.nef"))); + assert!(!is_image_file(Path::new("video.mp4"))); + assert!(!is_image_file(Path::new("document.txt"))); + } + + #[test] + fn test_is_video_file() { + assert!(is_video_file(Path::new("video.mp4"))); + assert!(is_video_file(Path::new("video.MP4"))); + assert!(is_video_file(Path::new("video.mov"))); + assert!(is_video_file(Path::new("video.avi"))); + assert!(!is_video_file(Path::new("photo.jpg"))); + assert!(!is_video_file(Path::new("document.txt"))); + } + + #[test] + fn test_is_media_file() { + assert!(is_media_file(Path::new("photo.jpg"))); + assert!(is_media_file(Path::new("video.mp4"))); + assert!(is_media_file(Path::new("photo.PNG"))); + assert!(!is_media_file(Path::new("document.txt"))); + assert!(!is_media_file(Path::new("no_extension"))); + } +} diff --git a/src/files.rs b/src/files.rs index 7f1049c..15250c4 100644 --- a/src/files.rs +++ b/src/files.rs @@ -12,6 +12,7 @@ use anyhow::{Context, anyhow}; use crate::data::{Claims, FilesRequest, FilterMode, MediaType, PhotosResponse, SortType}; use crate::database::ExifDao; +use crate::file_types; use crate::geo::{gps_bounding_box, haversine_distance}; use crate::memories::extract_date_from_filename; use crate::{AppState, create_thumbnails}; @@ -652,49 +653,22 @@ pub fn list_files_recursive(dir: &Path) -> io::Result> { } pub fn is_image_or_video(path: &Path) -> bool { - let extension = path - .extension() - .and_then(|p| p.to_str()) - .map_or(String::from(""), |p| p.to_lowercase()); - - extension == "png" - || extension == "jpg" - || extension == "jpeg" - || extension == "mp4" - || extension == "mov" - || extension == "nef" - || extension == "webp" - || extension == "tiff" - || extension == "tif" - || extension == "heif" - || extension == "heic" - || extension == "avif" + file_types::is_media_file(path) } /// Check if a file matches the media type filter fn matches_media_type(path: &Path, media_type: &MediaType) -> bool { + let result = match media_type { + MediaType::All => file_types::is_image_file(path) || file_types::is_video_file(path), + MediaType::Photo => file_types::is_image_file(path), + MediaType::Video => file_types::is_video_file(path), + }; + let extension = path .extension() .and_then(|p| p.to_str()) .map_or(String::from(""), |p| p.to_lowercase()); - let result = match media_type { - MediaType::All => true, - MediaType::Photo => { - extension == "png" - || extension == "jpg" - || extension == "jpeg" - || extension == "nef" - || extension == "webp" - || extension == "tiff" - || extension == "tif" - || extension == "heif" - || extension == "heic" - || extension == "avif" - } - MediaType::Video => extension == "mp4" || extension == "mov", - }; - debug!( "Media type check: path={:?}, extension='{}', type={:?}, match={}", path, extension, media_type, result @@ -873,6 +847,7 @@ mod tests { struct FakeFileSystem { files: HashMap>, + base_path: String, err: bool, } @@ -880,12 +855,19 @@ mod tests { fn with_error() -> FakeFileSystem { FakeFileSystem { files: HashMap::new(), + base_path: String::new(), err: true, } } fn new(files: HashMap>) -> FakeFileSystem { - FakeFileSystem { files, err: false } + // Use temp dir as base path for consistency + let base_path = env::temp_dir(); + FakeFileSystem { + files, + base_path: base_path.to_str().unwrap().to_string(), + err: false, + } } } @@ -894,7 +876,11 @@ mod tests { if self.err { Err(anyhow!("Error for test")) } else if let Some(files) = self.files.get(path) { - Ok(files.iter().map(PathBuf::from).collect::>()) + // Prepend base_path to all returned files + Ok(files + .iter() + .map(|f| PathBuf::from(&self.base_path).join(f)) + .collect::>()) } else { Ok(Vec::new()) } @@ -1043,22 +1029,36 @@ mod tests { let request: Query = Query::from_query("path=").unwrap(); - let mut temp_photo = env::temp_dir(); - let mut tmp = temp_photo.clone(); + // Create a dedicated test directory to avoid interference from other files in system temp + let mut test_base = env::temp_dir(); + test_base.push("image_api_test_list_photos"); + fs::create_dir_all(&test_base).unwrap(); - tmp.push("test-dir"); - fs::create_dir_all(tmp).unwrap(); + let mut test_dir = test_base.clone(); + test_dir.push("test-dir"); + fs::create_dir_all(&test_dir).unwrap(); - temp_photo.push("photo.jpg"); + let mut photo_path = test_base.clone(); + photo_path.push("photo.jpg"); + File::create(&photo_path).unwrap(); - File::create(temp_photo.clone()).unwrap(); + // Create AppState with the same base_path as RealFileSystem + use actix::Actor; + let test_state = AppState::new( + std::sync::Arc::new(crate::video::actors::StreamActor {}.start()), + test_base.to_str().unwrap().to_string(), + test_base.join("thumbnails").to_str().unwrap().to_string(), + test_base.join("videos").to_str().unwrap().to_string(), + test_base.join("gifs").to_str().unwrap().to_string(), + Vec::new(), + ); let response: HttpResponse = list_photos( claims, TestRequest::default().to_http_request(), request, - Data::new(AppState::test_state()), - Data::new(RealFileSystem::new(String::from("/tmp"))), + Data::new(test_state), + Data::new(RealFileSystem::new(test_base.to_str().unwrap().to_string())), Data::new(Mutex::new(SqliteTagDao::default())), Data::new(Mutex::new( Box::new(MockExifDao) as Box @@ -1082,6 +1082,9 @@ mod tests { .collect::>() .is_empty() ); + + // Cleanup + let _ = fs::remove_dir_all(test_base); } #[actix_rt::test] @@ -1095,12 +1098,13 @@ mod tests { let request: Query = Query::from_query("path=..").unwrap(); + let temp_dir = env::temp_dir(); let response = list_photos( claims, TestRequest::default().to_http_request(), request, Data::new(AppState::test_state()), - Data::new(RealFileSystem::new(String::from("./"))), + Data::new(RealFileSystem::new(temp_dir.to_str().unwrap().to_string())), Data::new(Mutex::new(SqliteTagDao::default())), Data::new(Mutex::new( Box::new(MockExifDao) as Box @@ -1120,7 +1124,8 @@ mod tests { exp: 12345, }; - let request: Query = Query::from_query("path=&tag_ids=1,3").unwrap(); + let request: Query = + Query::from_query("path=&tag_ids=1,3&recursive=true").unwrap(); let mut tag_dao = SqliteTagDao::new(in_memory_db_connection()); @@ -1141,22 +1146,12 @@ mod tests { .tag_file(&opentelemetry::Context::current(), "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, TestRequest::default().to_http_request(), request, Data::new(AppState::test_state()), - Data::new(FakeFileSystem::new(files)), + Data::new(FakeFileSystem::new(HashMap::new())), Data::new(Mutex::new(tag_dao)), Data::new(Mutex::new( Box::new(MockExifDao) as Box @@ -1208,18 +1203,8 @@ mod tests { ) .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 = Query::from_query(&format!( - "path=&tag_ids={},{}&tag_filter_mode=All", + "path=&tag_ids={},{}&tag_filter_mode=All&recursive=true", tag1.id, tag3.id )) .unwrap(); @@ -1229,7 +1214,7 @@ mod tests { TestRequest::default().to_http_request(), request, Data::new(AppState::test_state()), - Data::new(FakeFileSystem::new(files)), + Data::new(FakeFileSystem::new(HashMap::new())), Data::new(Mutex::new(tag_dao)), Data::new(Mutex::new( Box::new(MockExifDao) as Box diff --git a/src/lib.rs b/src/lib.rs index bed9574..03760e2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,6 +7,7 @@ pub mod data; pub mod database; pub mod error; pub mod exif; +pub mod file_types; pub mod files; pub mod geo; pub mod memories; @@ -36,7 +37,6 @@ pub fn update_media_counts(_media_dir: &Path) { // Stub - implemented in main.rs } -pub fn is_video(_entry: &DirEntry) -> bool { - // Stub - implemented in main.rs - false +pub fn is_video(entry: &DirEntry) -> bool { + file_types::direntry_is_video(entry) } diff --git a/src/main.rs b/src/main.rs index 6f48f2a..3187bea 100644 --- a/src/main.rs +++ b/src/main.rs @@ -54,6 +54,7 @@ mod data; mod database; mod error; mod exif; +mod file_types; mod files; mod geo; mod state; @@ -142,14 +143,8 @@ async fn get_image( } fn is_video_file(path: &Path) -> bool { - if let Some(extension) = path.extension() { - matches!( - extension.to_str().unwrap_or("").to_lowercase().as_str(), - "mp4" | "mov" | "avi" | "mkv" - ) - } else { - false - } + use image_api::file_types; + file_types::is_video_file(path) } #[get("/image/metadata")] @@ -176,9 +171,10 @@ async fn get_file_metadata( // Query EXIF data if available if let Ok(mut dao) = exif_dao.lock() - && let Ok(Some(exif)) = dao.get_exif(&path.path) { - response.exif = Some(exif.into()); - } + && let Ok(Some(exif)) = dao.get_exif(&path.path) + { + response.exif = Some(exif.into()); + } span.add_event( "Metadata fetched", @@ -678,23 +674,13 @@ fn update_media_counts(media_dir: &Path) { } fn is_image(entry: &DirEntry) -> bool { - entry - .path() - .extension() - .and_then(|ext| ext.to_str()) - .map(|ext| ext.to_lowercase()) - .map(|ext| ext == "jpg" || ext == "jpeg" || ext == "png" || ext == "nef") - .unwrap_or(false) + use image_api::file_types; + file_types::direntry_is_image(entry) } fn is_video(entry: &DirEntry) -> bool { - entry - .path() - .extension() - .and_then(|ext| ext.to_str()) - .map(|ext| ext.to_lowercase()) - .map(|ext| ext == "mp4" || ext == "mov") - .unwrap_or(false) + use image_api::file_types; + file_types::direntry_is_video(entry) } fn main() -> std::io::Result<()> { @@ -903,9 +889,10 @@ fn process_new_files( // Filter by modification time if specified if let Some(since) = modified_since { if let Ok(metadata) = entry.metadata() - && let Ok(modified) = metadata.modified() { - return modified >= since; - } + && let Ok(modified) = metadata.modified() + { + return modified >= since; + } // If we can't get metadata, include the file to be safe return true; }