diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 8539a5b6a..b2a095891 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -284,7 +284,8 @@ pub struct Catalog { /// What the library keeps about its folders (folder identity → record), see [`folders`]. #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] folder_records: BTreeMap, - /// Increments on every applied op. + /// Increments on every applied op, and when a batch fails and its rollback cannot undo it whole (the catalog + /// changed, so what is keyed by the revision is out of date). #[serde(skip)] pub revision: u64, /// Which photos the latest ops touched ([`Catalog::touched_since`]). @@ -348,6 +349,8 @@ struct Changes { /// outside (a library just loaded takes a range of its own, so that nothing kept from another library is /// taken for this one's): nothing is known of how the catalog got to where it is. seen: u64, + /// A batch that failed could not be rolled back whole: what it changed stays, so what it noted must too. + torn: bool, } impl PartialEq for Changes { @@ -610,13 +613,40 @@ impl Catalog { self.albums.get_mut(&id).ok_or(CatalogError::NoAlbum(id)) } - /// Apply an op; returns its inverse. On error nothing changes. + /// Apply an op; returns its inverse. On error the catalog is as it was, the revision included, and nothing is + /// remembered of the op in what the latest ops touched. The one exception is a batch whose rollback fails: the + /// catalog did change, so the revision advances and anything may be taken to have changed. pub fn apply(&mut self, op: Op) -> Result { if self.changes.seen != self.revision { // the revision was set from outside: what is remembered is of another past self.changes = Changes { floor: self.revision, seen: self.revision, ..Default::default() }; } - let inv = self.apply_inner(op)?; + let broad = self.changes.broad; + self.changes.torn = false; + let inv = match self.apply_inner(op) { + Ok(inv) => inv, + Err(e) => { + let made = self.revision.saturating_add(1); + if self.changes.torn { + // (a batch the rollback of which failed changed something and says not what: it makes a + // revision, so what is kept for the one before is dropped, and anything may have changed, as + // after an op of a wider reach) + self.changes.torn = false; + self.changes.broad = made; + self.revision = made; + self.changes.seen = made; + } else { + // (everything the failed op, or the rollback of a batch, noted was made for the revision it + // would have made: nothing earlier has that one. A note the cap already dropped stays + // dropped, which only says less than is known, as the cap does.) + while self.changes.touched.back().is_some_and(|(r, _, _)| *r == made) { + self.changes.touched.pop_back(); + } + self.changes.broad = broad; + } + return Err(e); + } + }; self.revision += 1; self.changes.seen = self.revision; Ok(inv) @@ -962,7 +992,9 @@ impl Catalog { Err(e) => { // roll back what was applied for inv in inverses.into_iter().rev() { - let _ = self.apply_inner(inv); + if self.apply_inner(inv).is_err() { + self.changes.torn = true; + } } return Err(e); } diff --git a/crates/catalog/src/tests.rs b/crates/catalog/src/tests.rs index f8b72dad8..579175086 100644 --- a/crates/catalog/src/tests.rs +++ b/crates/catalog/src/tests.rs @@ -952,12 +952,13 @@ fn the_catalog_remembers_which_photos_its_latest_ops_changed_and_what_of_them() c.apply(Op::SetRating { id: a, rating: 1 }).unwrap(); assert_eq!(c.touched_since(loaded), Some(vec![a])); assert_eq!(c.touched_since(old), None); - // an op that fails changes nothing, and what is remembered never says less than what happened + // an op that fails changes nothing and is not remembered let before = c.revision; assert!(c.apply(Op::SetRating { id: PhotoId(9_999), rating: 1 }).is_err()); assert_eq!(c.revision, before); + assert_eq!(c.touched_since(before), Some(vec![])); c.apply(Op::SetFlag { id: a, flag: Flag::Pick }).unwrap(); - assert!(c.touched_since(before).is_some_and(|ids| ids.contains(&a))); + assert_eq!(c.touched_since(before), Some(vec![a])); assert!(c.aspects_since(before).is_some_and(|x| x.any_of(Aspects::FLAG))); // a batch that fails midway is undone as a whole; a batch in a batch is remembered photo by photo; a batch // with an op that changes more than photos is not @@ -965,8 +966,8 @@ fn the_catalog_remembers_which_photos_its_latest_ops_changed_and_what_of_them() assert!(c.apply(Op::Batch { ops: vec![Op::SetRating { id: a, rating: 5 }, Op::SetRating { id: PhotoId(9_999), rating: 1 }] }).is_err()); assert_eq!((c.revision, c.photo(a).map(|p| p.rating)), (before, Some(1))); c.apply(Op::Batch { ops: vec![Op::Batch { ops: vec![Op::SetRating { id: a, rating: 2 }] }] }).unwrap(); - // (the photo the failed batch asked for and never found may be named too: more is said, never less) - assert!(c.touched_since(before).is_some_and(|ids| ids.contains(&a))); + // (the failed batch is not remembered: only the batch that worked) + assert_eq!(c.touched_since(before), Some(vec![a])); let before = c.revision; c.apply(Op::Batch { ops: vec![Op::SetRating { id: a, rating: 3 }, Op::RenameAlbum { id: album, name: "Journey".into() }] }).unwrap(); assert_eq!(c.touched_since(before), None); @@ -978,3 +979,70 @@ fn the_catalog_remembers_which_photos_its_latest_ops_changed_and_what_of_them() assert_eq!(c.touched_since(long_ago), None); assert_eq!(c.touched_since(c.revision - 10), Some(vec![a])); } + +/// An op that fails leaves nothing behind in what the catalog remembers of its ops: no photo it never changed (one +/// that does not even exist), and, for a batch rolled back, none of what its first ops had touched. +#[test] +fn an_op_that_fails_leaves_no_touched_photos_behind() { + let mut c = Catalog::default(); + let add = |c: &mut Catalog, name: &str| { + let id = c.alloc_photo_id(); + c.apply(Op::AddPhoto { + photo: Box::new(Photo::new(id, Source::File { path: format!("/p/{name}") }, name, "JPEG", 60, 40, "2026-01-01T10:00:00")), + }) + .unwrap(); + id + }; + let (a, b) = (add(&mut c, "a.jpg"), add(&mut c, "b.jpg")); + let missing = PhotoId(9_999); + let start = c.revision; + // every op that names a photo, on one that does not exist + for op in [Op::SetRating { id: missing, rating: 1 }, Op::SetFlag { id: missing, flag: Flag::Pick }, Op::RemovePhoto { id: missing }] { + assert!(c.apply(op).is_err()); + assert_eq!(c.revision, start, "a failed op makes no revision"); + assert_eq!(c.touched_since(start), Some(vec![])); + assert_eq!(c.aspects_since(start), Some(Aspects::NONE)); + } + // an op that changes more than photos and fails does not make earlier revisions unknown + assert!(c.apply(Op::RenameAlbum { id: AlbumId(77), name: "x".into() }).is_err()); + assert_eq!(c.touched_since(start), Some(vec![])); + // a batch that fails on its second op names nothing of its first, nor of the rollback + assert!(c.apply(Op::Batch { ops: vec![Op::SetRating { id: a, rating: 5 }, Op::SetRating { id: missing, rating: 1 }] }).is_err()); + assert_eq!(c.revision, start); + assert_eq!(c.touched_since(start), Some(vec![])); + assert_eq!(c.aspects_since(start), Some(Aspects::NONE)); + assert_eq!(c.photo(a).map(|p| p.rating), Some(0)); + // a batch that ends in a failed removal of a photo that exists + assert!(c.apply(Op::Batch { ops: vec![Op::RemovePhoto { id: b }, Op::RemovePhoto { id: b }] }).is_err()); + assert!(c.photo(b).is_some()); + assert_eq!(c.touched_since(start), Some(vec![])); + // what succeeded after the failures is remembered, and only that + c.apply(Op::SetRating { id: a, rating: 2 }).unwrap(); + assert_eq!(c.touched_since(start), Some(vec![a])); + assert_eq!(c.aspects_since(start), Some(Aspects::RATING)); +} + +/// A batch the rollback of which fails halfway leaves the catalog changed (here a stack is lost), so what the +/// catalog remembers must not say that nothing changed: anything may have. +#[test] +fn a_batch_that_cannot_be_rolled_back_whole_makes_earlier_changes_unknown() { + let mut c = Catalog::default(); + let mut ids = Vec::new(); + for name in ["a.jpg", "b.jpg"] { + let id = c.alloc_photo_id(); + let photo = Photo::new(id, Source::File { path: format!("/p/{name}") }, name, "JPEG", 60, 40, "2026-01-01T10:00:00"); + c.apply(Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + ids.push(id); + } + let stack = c.alloc_stack_id(); + c.apply(Op::AddStack { stack: Stack { id: stack, photos: ids.clone(), collapsed: false } }).unwrap(); + let start = c.revision; + // the photo goes, then its stack, then an op fails: putting the stack back needs the photo, which comes later + let failing = + Op::Batch { ops: vec![Op::RemovePhoto { id: ids[0] }, Op::RemoveStack { id: stack }, Op::SetRating { id: PhotoId(9_999), rating: 1 }] }; + assert!(c.apply(failing).is_err()); + assert_eq!(c.revision, start + 1, "the catalog changed, so what is kept for the revision before is out of date"); + assert!(c.photo(ids[0]).is_some() && c.stack(stack).is_none(), "the rollback went only part of the way"); + assert_eq!(c.touched_since(start), None, "so nothing is said of what changed"); + assert_eq!(c.touched_since(c.revision), Some(vec![]), "from after it, it is known again"); +}