diff --git a/core/dr-ingest/src/lib.rs b/core/dr-ingest/src/lib.rs index 7b25506..92fabab 100644 --- a/core/dr-ingest/src/lib.rs +++ b/core/dr-ingest/src/lib.rs @@ -26,6 +26,7 @@ //! **It does not know what a duplicate is.** That question is a catalog query //! (FR-CAT-11), so it arrives through [`DuplicateCheck`]. +use std::collections::HashMap; use std::io::Read; use dr_plat::{DirRef, NewFile, SeekableRead, Storage, StorageError, WritableStorage}; @@ -130,6 +131,13 @@ impl Default for Options { } } +/// What a destination folder already holds: folder key -> (name -> size). +/// +/// Built lazily as the template sends files to new folders, and kept for the +/// run so that re-importing a card costs one listing per day rather than one +/// per photograph. +type Listings = HashMap>; + /// One file offered for import. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Candidate { @@ -298,6 +306,11 @@ impl Ingest<'_> { ) -> Report { let mut report = Report::default(); let total = candidates.len(); + // What each destination folder already holds, listed once per folder + // rather than probed per file: a card is two thousand files landing in + // a handful of days, so this is a handful of listings against two + // thousand round trips through the storage layer. + let mut listings: Listings = HashMap::new(); for candidate in candidates { if cancel() { @@ -328,7 +341,7 @@ impl Ingest<'_> { continue; } - match self.one(candidate, &shot, &key, is_duplicate) { + match self.one(candidate, &shot, &key, is_duplicate, &mut listings) { Ok(Some(imported)) => { report.bytes += imported.size; if imported.dated_from == DateSource::Modified { @@ -359,6 +372,7 @@ impl Ingest<'_> { shot: &Shot, key: &DupKey, is_duplicate: &DuplicateCheck<'_>, + listings: &mut Listings, ) -> Result, IngestError> { let destination = layout::expand(&self.options.folder_template, shot); @@ -375,6 +389,31 @@ impl Ingest<'_> { None => None, }; + // TRACES: FR-CAT-11 + // The card that has already been imported into this very folder. + // + // Distinct from the catalog's answer and needed alongside it: on a + // library whose catalog describes a *server*, a file sitting in the + // local destination has no row to be found by, so the question "have I + // already copied this" is only answerable by looking. Without this, + // re-importing a card fills the folder with `-1` copies of everything + // — the rename below is for a genuinely different photograph that + // happens to share a name, and it cannot tell the two cases apart on + // its own. + // + // Name plus size, not a digest: this runs before any transfer, and + // hashing to answer it would read the whole card to save reading the + // whole card. A file of the same name and the same length in the folder + // this import would have written it to is the same photograph in every + // case that is not deliberately constructed; the digest tier below + // still catches the same frame under a *different* name. + let existing = self.folder_listing(listings, &dir)?; + if self.options.on_duplicate == DuplicatePolicy::Skip + && existing.get(&candidate.name) == Some(&candidate.size) + { + return Ok(None); + } + let name = self.free_name(self.dest, &dir, &candidate.name)?; let mut reader = self @@ -474,6 +513,33 @@ impl Ingest<'_> { Ok(dir) } + /// What a destination folder holds, listed at most once per run. + /// + /// A folder that cannot be listed yields an empty map rather than an + /// error: the consequence is a re-import that copies what was already + /// there, which the digest tier then removes — where failing the import + /// would cost the user the photographs that are not yet anywhere. + fn folder_listing<'a>( + &self, + listings: &'a mut Listings, + dir: &DirRef, + ) -> Result<&'a HashMap, IngestError> { + let key = format!("{}:{}", dir.root_id().0, dir.key()); + Ok(listings + .entry(key) + .or_insert_with(|| match self.dest.list(dir) { + Ok(entries) => entries + .into_iter() + .filter(|e| !e.meta.is_dir) + .map(|e| (e.meta.name, e.meta.size)) + .collect(), + Err(e) => { + log::warn!("could not list {dir}: {e}"); + HashMap::new() + } + })) + } + /// A name not already taken in the destination. /// /// Two cards can hold `IMG_0001.CR3` and they are different photographs. @@ -704,13 +770,13 @@ mod tests { use dr_plat::{Entry, LocalStorage}; use dr_types::{ByteRange, DirState, RootId}; - const CARD: RootId = RootId(1); - const LIB: RootId = RootId(2); + pub(super) const CARD: RootId = RootId(1); + pub(super) const LIB: RootId = RootId(2); /// 2026-08-22T14:00:00Z. const AUG_22: i64 = 1_787_407_200; /// A throwaway directory, removed when the test ends. - struct Tree(PathBuf); + pub(super) struct Tree(PathBuf); impl Tree { fn new(name: &str) -> Self { @@ -738,15 +804,15 @@ mod tests { } /// A card with files on it, and the library it imports into. - struct Fixture { + pub(super) struct Fixture { _tree: Tree, - card: LocalStorage, + pub(super) card: LocalStorage, card_path: PathBuf, - lib: LocalStorage, - lib_path: PathBuf, + pub(super) lib: LocalStorage, + pub(super) lib_path: PathBuf, } - fn fixture(name: &str) -> Fixture { + pub(super) fn fixture(name: &str) -> Fixture { let tree = Tree::new(name); let card_path = tree.sub("card/DCIM/100CANON"); let lib_path = tree.sub("library"); @@ -761,7 +827,7 @@ mod tests { impl Fixture { /// Put a file on the card and return the candidate for it. - fn shoot(&self, name: &str, bytes: &[u8]) -> Candidate { + pub(super) fn shoot(&self, name: &str, bytes: &[u8]) -> Candidate { std::fs::write(self.card_path.join(name), bytes).expect("write"); Candidate { source: SourceRef::Local { @@ -774,11 +840,11 @@ mod tests { } } - fn lib_root(&self) -> DirRef { + pub(super) fn lib_root(&self) -> DirRef { DirRef::root(LIB) } - fn read(&self, rel: &str) -> Vec { + pub(super) fn read(&self, rel: &str) -> Vec { std::fs::read(self.lib_path.join(rel)).expect("read imported file") } } @@ -793,7 +859,7 @@ mod tests { } } - fn never(_: &DupKey) -> bool { + pub(super) fn never(_: &DupKey) -> bool { false } @@ -1316,4 +1382,175 @@ mod tests { assert_eq!(report.imported[0].size, bytes.len() as u64); assert_eq!(f.read("2026/2026-08-22/IMG_0001.CR3"), bytes); } + // ---- shared with `already_there_tests` ------------------------------- + + /// A probe that dates everything to the 22nd of August 2026. + pub(super) fn probe_22nd(_: &Candidate) -> Option { + Some(on_the_22nd()) + } + + /// Import a card with the defaults, against a catalog that knows nothing. + pub(super) fn run_once(f: &Fixture, candidates: &[Candidate]) -> Report { + let root = f.lib_root(); + let ingest = Ingest { + source: &f.card, + dest: &f.lib, + dest_root: &root, + backup: None, + options: Options::default(), + }; + ingest.run(candidates, &probe_22nd, &never, &|| false, &mut |_| {}) + } + + /// A destination that counts how many times it is listed. + /// + /// The listing is the whole cost of knowing what a folder already holds, + /// so "once per folder" is a property worth a test rather than a comment. + pub(super) struct CountingList { + inner: LocalStorage, + listings: std::cell::Cell, + } + + impl CountingList { + pub(super) fn new(inner: LocalStorage) -> Self { + Self { + inner, + listings: std::cell::Cell::new(0), + } + } + pub(super) fn listings(&self) -> usize { + self.listings.get() + } + } + + // `Cell` is not `Sync`, and `Storage` requires it. Sound here because the + // test drives this from one thread; spelled out rather than left implicit. + unsafe impl Sync for CountingList {} + + impl Storage for CountingList { + fn roots(&self) -> Vec { + self.inner.roots() + } + fn root_dir(&self, root: RootId) -> Result { + self.inner.root_dir(root) + } + fn dir_state(&self, dir: &DirRef) -> Result { + self.inner.dir_state(dir) + } + fn list(&self, dir: &DirRef) -> Result, StorageError> { + self.listings.set(self.listings.get() + 1); + self.inner.list(dir) + } + fn open(&self, src: &SourceRef) -> Result, StorageError> { + self.inner.open(src) + } + fn read_range(&self, src: &SourceRef, range: ByteRange) -> Result, StorageError> { + self.inner.read_range(src, range) + } + } + + impl WritableStorage for CountingList { + fn create_dir(&self, parent: &DirRef, name: &str) -> Result { + self.inner.create_dir(parent, name) + } + fn create_file(&self, parent: &DirRef, name: &str) -> Result { + self.inner.create_file(parent, name) + } + fn exists(&self, parent: &DirRef, name: &str) -> Result { + self.inner.exists(parent, name) + } + fn remove_file(&self, src: &SourceRef) -> Result<(), StorageError> { + self.inner.remove_file(src) + } + } +} + +#[cfg(test)] +mod already_there_tests { + use super::tests::*; + use super::*; + use dr_plat::LocalStorage; + + #[test] + fn re_importing_the_same_card_copies_nothing() { + let f = fixture("already-there"); + let c = f.shoot("IMG_0001.CR3", b"raw bytes"); + + // First import: it lands. + let first = run_once(&f, std::slice::from_ref(&c)); + assert_eq!(first.imported.len(), 1); + + // Second import of the same card, with a catalog that knows nothing — + // which is the state on a library whose catalog describes a server. + // Without the folder listing this produced IMG_0001-1.CR3. + let second = run_once(&f, &[c]); + assert_eq!(second.duplicates.len(), 1); + assert!(second.imported.is_empty()); + assert!( + !f.lib_path.join("2026/2026-08-22/IMG_0001-1.CR3").exists(), + "a second copy was written under a new name" + ); + } + + #[test] + fn a_different_photograph_of_the_same_name_still_lands() { + let f = fixture("same-name-different-frame"); + let first = f.shoot("IMG_0001.CR3", b"the first frame"); + assert_eq!(run_once(&f, &[first]).imported.len(), 1); + + // Camera filenames wrap at IMG_9999, so this is ordinary. Different + // length, so the size check separates it from the one already there. + let second = f.shoot("IMG_0001.CR3", b"a different and longer frame"); + let report = run_once(&f, &[second]); + assert_eq!(report.imported.len(), 1); + assert_eq!(report.imported[0].name, "IMG_0001-1.CR3"); + } + + #[test] + fn importing_again_deliberately_still_works() { + let f = fixture("already-there-forced"); + let c = f.shoot("IMG_0001.CR3", b"raw bytes"); + assert_eq!(run_once(&f, std::slice::from_ref(&c)).imported.len(), 1); + + // The user asked for it, so the folder listing must not veto them. + let root = f.lib_root(); + let ingest = Ingest { + source: &f.card, + dest: &f.lib, + dest_root: &root, + backup: None, + options: Options { + on_duplicate: DuplicatePolicy::ImportAsNew, + ..Default::default() + }, + }; + let report = ingest.run(&[c], &probe_22nd, &never, &|| false, &mut |_| {}); + assert_eq!(report.imported.len(), 1); + assert_eq!(report.imported[0].name, "IMG_0001-1.CR3"); + } + + #[test] + fn a_folder_is_listed_once_however_many_files_land_in_it() { + // The property that keeps this affordable: a card of two thousand + // frames from one day is one listing, not two thousand. + let f = fixture("one-listing"); + let candidates: Vec = (0..12) + .map(|i| f.shoot(&format!("IMG_{i:04}.CR3"), format!("frame {i}").as_bytes())) + .collect(); + + let counting = CountingList::new(LocalStorage::with_root(LIB, f.lib_path.clone())); + let root = DirRef::root(LIB); + let ingest = Ingest { + source: &f.card, + dest: &counting, + dest_root: &root, + backup: None, + options: Options::default(), + }; + let report = ingest.run(&candidates, &probe_22nd, &never, &|| false, &mut |_| {}); + + assert_eq!(report.imported.len(), 12); + // One for the day folder. `create_dir` and `exists` do not list. + assert_eq!(counting.listings(), 1); + } } diff --git a/core/dr-sync/src/lib.rs b/core/dr-sync/src/lib.rs index dc8c2ea..9616f6f 100644 --- a/core/dr-sync/src/lib.rs +++ b/core/dr-sync/src/lib.rs @@ -35,7 +35,7 @@ pub use types::{ Cursor, EntryKind, Identity, Precondition, RemoteChange, RemoteEntry, RemoteId, RemotePath, Validator, }; -pub use upload::{destination, upload_original}; +pub use upload::{destination, upload_original, Placed}; /// TRACES: FR-NC-12 /// A remote storage backend. diff --git a/core/dr-sync/src/upload.rs b/core/dr-sync/src/upload.rs index 65591e3..2d035c6 100644 --- a/core/dr-sync/src/upload.rs +++ b/core/dr-sync/src/upload.rs @@ -37,64 +37,123 @@ pub fn destination(library: &RemotePath, folders: &[String]) -> RemotePath { .fold(library.clone(), |acc, seg| acc.join(seg)) } -/// Upload one original into its dated folder. +/// TRACES: FR-NC-7a | FR-CAT-11 +/// What became of an original the caller offered. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Placed { + /// Sent now. + Uploaded { + path: RemotePath, + validator: Validator, + }, + /// The server already held this photograph, so nothing was transferred. + /// + /// The case a re-inserted card produces: the shoot was imported and + /// uploaded last week, and the card has not been formatted since. Worth a + /// variant of its own rather than folding into `Uploaded`, because the two + /// are different answers to "is my work backed up" — and because a caller + /// counting bytes moved must not count these. + AlreadyThere { path: RemotePath }, +} + +impl Placed { + /// Where the photograph is on the server, however it got there. + pub fn path(&self) -> &RemotePath { + match self { + Placed::Uploaded { path, .. } | Placed::AlreadyThere { path } => path, + } + } +} + +/// Upload one original into its dated folder, unless it is already there. /// /// Creates the folders if they are missing — `create_dir` makes parents and /// succeeds on one that already exists, so the second import of a day costs /// one request rather than a conflict. /// -/// Returns where it actually went, which may not be the name that was asked +/// Returns where the photograph is, which may not be the name that was asked /// for: two cameras produce `IMG_0001.CR3` and the second must not overwrite /// the first. The caller records the returned path, never the one it passed. +/// +/// `size` is what decides between "already there" and "a different photograph +/// with the same name". See [`resolve`]. pub async fn upload_original( backend: &dyn RemoteBackend, library: &RemotePath, folders: &[String], name: &str, body: Vec, -) -> Result<(RemotePath, Validator), RemoteError> { +) -> Result { let dir = destination(library, folders); backend.create_dir(&dir).await?; - let name = free_name(backend, &dir, name).await?; - let path = dir.join(&name); - let validator = backend.put(&path, body, None).await?; - Ok((path, validator)) + match resolve(backend, &dir, name, body.len() as u64).await? { + Resolution::AlreadyThere => Ok(Placed::AlreadyThere { + path: dir.join(name), + }), + Resolution::Use(name) => { + let path = dir.join(&name); + let validator = backend.put(&path, body, None).await?; + Ok(Placed::Uploaded { path, validator }) + } + } } -/// A name not already taken in a remote folder. +/// What to do about a name in the destination folder. +#[derive(Debug, Clone, PartialEq, Eq)] +enum Resolution { + /// Write under this name. + Use(String), + /// The photograph is already on the server. Send nothing. + AlreadyThere, +} + +/// Decide what to call an original in a remote folder, or that it is already +/// there. /// /// One listing rather than a probe per candidate: a day folder is listed in a /// single `PROPFIND` and the answer covers every collision in it, where /// probing would be a round trip per attempt over a link that may be mobile /// data. /// +/// **Name plus length decides "already there".** A card that was imported and +/// uploaded last week and has not been formatted since is the ordinary case, +/// and re-uploading it would double the storage and fill the folder with `-1` +/// copies of a shoot that is already safe. The same length under the same name +/// in the folder this photograph belongs in is that photograph in every case +/// that is not deliberately constructed — and a camera that reuses a filename +/// after `IMG_9999` writes a *different* number of bytes essentially always, +/// which is what leaves the rename below for the case it is actually for. +/// /// A folder that cannot be listed yields the original name rather than an /// error. That is the safe direction on a server that has just been asked to /// create the folder: the alternative is failing an upload because a listing -/// raced the `MKCOL`, and a genuine collision still fails at the `PUT` if the -/// caller asked for a precondition. -async fn free_name( +/// raced the `MKCOL`. +async fn resolve( backend: &dyn RemoteBackend, dir: &RemotePath, name: &str, -) -> Result { - let taken: std::collections::HashSet = match backend.list(dir, None).await { + size: u64, +) -> Result { + let taken: std::collections::HashMap = match backend.list(dir, None).await { Ok(entries) => entries .into_iter() .filter(|e| e.kind == EntryKind::File) - .map(|e| e.path.name().to_string()) + .map(|e| (e.path.name().to_string(), e.size)) .collect(), Err(RemoteError::NotFound(_)) => Default::default(), Err(e) => return Err(e), }; - if !taken.contains(name) { - return Ok(name.to_string()); + match taken.get(name) { + None => return Ok(Resolution::Use(name.to_string())), + Some(&there) if there == size => return Ok(Resolution::AlreadyThere), + Some(_) => {} } - // The same suffix convention the local import uses, so a photograph that - // collided on both sides carries the same name in both places. + // A different photograph that happens to share a name. The same suffix + // convention the local import uses, so one that collided on both sides + // carries the same name in both places. let (stem, ext) = match name.rsplit_once('.') { Some((s, e)) if !s.is_empty() => (s, Some(e)), _ => (name, None), @@ -104,8 +163,12 @@ async fn free_name( Some(ext) => format!("{stem}-{n}.{ext}"), None => format!("{stem}-{n}"), }; - if !taken.contains(&candidate) { - return Ok(candidate); + match taken.get(&candidate) { + None => return Ok(Resolution::Use(candidate)), + // The renamed copy is itself already up there: this is the second + // re-import of a card that legitimately collided once. + Some(&there) if there == size => return Ok(Resolution::AlreadyThere), + Some(_) => {} } } Err(RemoteError::Unsupported( @@ -147,7 +210,8 @@ mod tests { // fake in `dr-sync-nextcloud`'s tests and against a real server in S8. // What is worth pinning here is the collision rule, since it decides // whether a second camera's `IMG_0001.CR3` overwrites the first. - struct Listing(Vec<&'static str>); + /// A folder holding files of given name and length. + struct Listing(Vec<(&'static str, u64)>); #[async_trait::async_trait] impl RemoteBackend for Listing { @@ -165,12 +229,12 @@ mod tests { Ok(self .0 .iter() - .map(|n| RemoteEntry { + .map(|(n, size)| RemoteEntry { id: RemoteId::Path(dir.join(n)), path: dir.join(n), kind: EntryKind::File, validator: Validator::new("v"), - size: 0, + size: *size, modified: None, has_preview: false, }) @@ -237,9 +301,12 @@ mod tests { } #[test] - fn a_second_cameras_img_0001_does_not_overwrite_the_first() { - let backend = Listing(vec!["IMG_0001.CR3"]); - let (path, _) = block_on(upload_original( + fn a_card_already_on_the_server_transfers_nothing() { + // The ordinary case: last week's shoot was imported and uploaded, and + // the card has not been formatted since. Re-uploading would double the + // storage and litter the folder with `-1` copies of safe work. + let backend = Listing(vec![("IMG_0001.CR3", 3)]); + let placed = block_on(upload_original( &backend, &RemotePath::new("PhotosRaw"), &segs(&["2026", "2026-08-22"]), @@ -247,13 +314,38 @@ mod tests { vec![1, 2, 3], )) .unwrap(); - assert_eq!(path.as_str(), "PhotosRaw/2026/2026-08-22/IMG_0001-1.CR3"); + assert_eq!( + placed, + Placed::AlreadyThere { + path: RemotePath::new("PhotosRaw/2026/2026-08-22/IMG_0001.CR3") + } + ); + } + + #[test] + fn a_second_cameras_img_0001_does_not_overwrite_the_first() { + // Same name, different length: a genuinely different photograph, which + // is what the rename is for. + let backend = Listing(vec![("IMG_0001.CR3", 99)]); + let placed = block_on(upload_original( + &backend, + &RemotePath::new("PhotosRaw"), + &segs(&["2026", "2026-08-22"]), + "IMG_0001.CR3", + vec![1, 2, 3], + )) + .unwrap(); + assert_eq!( + placed.path().as_str(), + "PhotosRaw/2026/2026-08-22/IMG_0001-1.CR3" + ); + assert!(matches!(placed, Placed::Uploaded { .. })); } #[test] fn a_free_name_is_used_as_it_is() { - let backend = Listing(vec!["IMG_0002.CR3"]); - let (path, _) = block_on(upload_original( + let backend = Listing(vec![("IMG_0002.CR3", 3)]); + let placed = block_on(upload_original( &backend, &RemotePath::new("PhotosRaw"), &segs(&["2026", "2026-08-22"]), @@ -261,6 +353,41 @@ mod tests { vec![1, 2, 3], )) .unwrap(); - assert_eq!(path.as_str(), "PhotosRaw/2026/2026-08-22/IMG_0001.CR3"); + assert_eq!( + placed.path().as_str(), + "PhotosRaw/2026/2026-08-22/IMG_0001.CR3" + ); + assert!(matches!(placed, Placed::Uploaded { .. })); + } + + #[test] + fn a_renamed_copy_that_is_already_up_there_is_not_sent_twice() { + // The card that collided once and is now being re-imported: both the + // original name and the renamed copy are on the server, and this + // photograph is the renamed one. + let backend = Listing(vec![("IMG_0001.CR3", 99), ("IMG_0001-1.CR3", 3)]); + let placed = block_on(upload_original( + &backend, + &RemotePath::new("PhotosRaw"), + &segs(&["2026", "2026-08-22"]), + "IMG_0001.CR3", + vec![1, 2, 3], + )) + .unwrap(); + assert!(matches!(placed, Placed::AlreadyThere { .. }), "{placed:?}"); + } + + #[test] + fn an_empty_folder_takes_the_name_as_offered() { + let backend = Listing(vec![]); + let placed = block_on(upload_original( + &backend, + &RemotePath::new("PhotosRaw"), + &segs(&["2026", "2026-08-22"]), + "IMG_0001.CR3", + vec![1, 2, 3], + )) + .unwrap(); + assert!(matches!(placed, Placed::Uploaded { .. })); } }