diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 1776c895c..59e9b0f52 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -348,6 +348,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 +612,37 @@ 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 nothing changes, and nothing is remembered of the op in what the + /// latest ops touched; only a batch that could not be rolled back whole is remembered as "anything may 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: anything may + // have changed, as after an op of a wider reach) + self.changes.torn = false; + self.changes.broad = 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 +988,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..f76d57bbc 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,69 @@ 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); + 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"); +}