From f61908560906ad2c9704cb69d11b0d90d84717e0 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Sat, 24 Oct 2026 08:06:38 -0400 Subject: [PATCH] LR-LIB-SMARTALBUM: the catalog refuses an edit that makes a smart album include itself Session::commit now goes through Catalog::apply_new, which looks at what an op would do to which album tests which (AlbumGraph, foresee) before doing it, and refuses one that puts a smart album on a loop: alone, in a batch, or as a new album. Undo, redo, replay and opening a library still bring back what was, loops included. --- crates/catalog/src/lib.rs | 187 ++++++++++++++++++ crates/catalog/src/tests_album_loops.rs | 243 ++++++++++++++++++++++++ crates/engine/src/lib.rs | 15 +- crates/engine/src/tests.rs | 1 + crates/engine/src/tests/album_loops.rs | 102 ++++++++++ 5 files changed, 545 insertions(+), 3 deletions(-) create mode 100644 crates/catalog/src/tests_album_loops.rs create mode 100644 crates/engine/src/tests/album_loops.rs diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 453b68a69..9047849e5 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -652,6 +652,44 @@ impl Catalog { Ok(inv) } + /// [`Self::apply`] for a new edit (not an undo, a redo or the replay of a saved log, which bring + /// back what was): refused when it would put a smart album on a loop, one that includes itself + /// through the albums it tests. THE place that rule is kept, whichever command, importer or + /// task makes the edit, alone or in a batch. A library that already holds a loop still opens, + /// and an edit that puts no album on one it was not on goes through, also to an album on a loop. + /// + /// The check is made on what the edit would do to the albums, before it is done: a refused + /// edit never touched the library, so there is nothing to take back. + pub fn apply_new(&mut self, op: Op) -> Result { + if let Some((id, name)) = self.would_put_on_a_loop(&op) { + return Err(CatalogError::Invalid(format!( + "smart album “{name}” (album {}) would include itself: its rules lead back to it through the albums they test", + id.0 + ))); + } + self.apply(op) + } + + /// The album `op` would newly put on a loop, as its id and name: the album the op is about + /// when that is one of them. `None` when it makes none (or changes no album's rules). + fn would_put_on_a_loop(&self, op: &Op) -> Option<(AlbumId, String)> { + if !changes_what_albums_test(op) { + return None; + } + let mut graph = AlbumGraph::of(self.albums.values()); + let now = graph.on_a_loop(); + let mut edited = Vec::new(); + foresee(&mut graph, op, &mut edited); + let then = graph.on_a_loop(); + let name_of = |id: AlbumId| self.albums.get(&id).map(|a| a.name.clone()).unwrap_or_default(); + let new_edited = edited.iter().find(|(id, _)| then.contains(id) && !now.contains(id)); + match new_edited { + Some((id, name)) => Some((*id, name.map_or_else(|| name_of(*id), str::to_string))), + // an album the edit is not about (it tests one that now leads back to it) + None => then.difference(&now).next().map(|id| (*id, name_of(*id))), + } + } + fn apply_inner(&mut self, op: Op) -> Result { // An op that changes one photo says which (`photo_mut`, and adding or removing it below). Every other kind // can change what any photo matches or where it sorts: an album's photos or rules, a stack, a keyword's @@ -1180,6 +1218,153 @@ impl Catalog { } } +/// Whether `op` changes which albums a smart album tests: it adds or removes an album, or changes +/// rules (alone or in a batch). +fn changes_what_albums_test(op: &Op) -> bool { + // listed kind by kind (as in `apply_inner`), so a new kind has to be put on one side + match op { + Op::AddAlbum { .. } | Op::RemoveAlbum { .. } | Op::SetAlbumRules { .. } => true, + Op::Batch { ops } => ops.iter().any(changes_what_albums_test), + Op::AddPhoto { .. } + | Op::RemovePhoto { .. } + | Op::SetDeleted { .. } + | Op::SetLocal { .. } + | Op::SetContent { .. } + | Op::SetEmbeddedLens { .. } + | Op::SetAsShotWb { .. } + | Op::SetRating { .. } + | Op::SetFlag { .. } + | Op::SetLabel { .. } + | Op::SetMeta { .. } + | Op::SetCaptured { .. } + | Op::SetFile { .. } + | Op::Relink { .. } + | Op::SetAnalysis { .. } + | Op::SetDevelop { .. } + | Op::SetVersions { .. } + | Op::SetHistory { .. } + | Op::PushHistory { .. } + | Op::SetHistoryCursor { .. } + | Op::RenameAlbum { .. } + | Op::MoveAlbum { .. } + | Op::SetAlbumOrder { .. } + | Op::SetAlbumPhotos { .. } + | Op::SetAlbumCover { .. } + | Op::AddStack { .. } + | Op::RemoveStack { .. } + | Op::SetStack { .. } + | Op::SetLabelName { .. } + | Op::SetBrowsed { .. } + | Op::SetKeyword { .. } + | Op::SetFolderRecord { .. } => false, + } +} + +/// What `op` would do to `graph`, as far as loops go (which album tests which), without the +/// checks `apply` makes: an op it would refuse is refused there. `edited` gets the albums the op +/// is about, with their name when the op brings it. +fn foresee<'a>(graph: &mut AlbumGraph, op: &'a Op, edited: &mut Vec<(AlbumId, Option<&'a str>)>) { + match op { + Op::AddAlbum { album } => { + // an id that is taken stays as it is: `apply` refuses the add + if let std::collections::hash_map::Entry::Vacant(slot) = graph.leads_to.entry(album.id) { + slot.insert(album.smart.as_deref().map(Filter::albums_tested)); + edited.push((album.id, Some(album.name.as_str()))); + } + } + Op::RemoveAlbum { id } => { + graph.leads_to.remove(id); + } + Op::SetAlbumRules { id, rules } => { + // only a smart album has rules: `apply` refuses them for another + if let Some(tested) = graph.leads_to.get_mut(id).filter(|t| t.is_some()) { + *tested = Some(rules.albums_tested()); + edited.push((*id, None)); + } + } + Op::Batch { ops } => ops.iter().for_each(|op| foresee(graph, op, edited)), + _ => {} + } +} + +/// Which album tests which: what a smart album's photos depend on beyond its own rules. Made in +/// one look at the albums, for questions about all of them. +struct AlbumGraph { + /// Every album; `Some(albums its rules test)` for a smart album, `None` for any other. + leads_to: std::collections::HashMap>>, +} + +impl AlbumGraph { + fn of<'a>(albums: impl Iterator) -> AlbumGraph { + AlbumGraph { leads_to: albums.map(|a| (a.id, a.smart.as_deref().map(Filter::albums_tested))).collect() } + } + + /// The smart albums that lead back to themselves: those in a ring of albums leading to one + /// another (a strongly connected part of more than one album), or testing themselves. One + /// walk over the albums (Tarjan's, with its own stack rather than recursion: a chain of + /// thousands of albums must not overflow the call stack). + fn on_a_loop(&self) -> std::collections::BTreeSet { + // only what tests an album can be on a ring: number those + let testing: Vec<(AlbumId, &[AlbumId])> = + self.leads_to.iter().filter_map(|(id, tested)| tested.as_deref().filter(|t| !t.is_empty()).map(|t| (*id, t))).collect(); + let number: std::collections::HashMap = testing.iter().enumerate().map(|(i, (id, _))| (*id, i)).collect(); + let leads: Vec> = testing.iter().map(|(_, t)| t.iter().filter_map(|to| number.get(to).copied()).collect()).collect(); + const UNSEEN: usize = usize::MAX; + let n = testing.len(); + // when each was first reached, the earliest still-open album it leads back to, the open + // ones in the order reached, and whether each ended up on a ring + let (mut reached, mut back) = (vec![UNSEEN; n], vec![0usize; n]); + let (mut open, mut is_open, mut ringed) = (Vec::new(), vec![false; n], vec![false; n]); + let mut clock = 0usize; + for root in 0..n { + if reached[root] != UNSEEN { + continue; + } + // (album, how many of its leads were followed) + let mut walk: Vec<(usize, usize)> = vec![(root, 0)]; + while let Some(&(v, followed)) = walk.last() { + if followed == 0 && reached[v] == UNSEEN { + reached[v] = clock; + back[v] = clock; + clock += 1; + open.push(v); + is_open[v] = true; + } + if let Some(&w) = leads[v].get(followed) { + if let Some(top) = walk.last_mut() { + top.1 += 1; + } + if reached[w] == UNSEEN { + walk.push((w, 0)); + } else if is_open[w] { + back[v] = back[v].min(reached[w]); + } + continue; + } + // every lead of `v` followed: it closes, and with it the ring it is the first of + walk.pop(); + if let Some(&(parent, _)) = walk.last() { + back[parent] = back[parent].min(back[v]); + } + if back[v] == reached[v] { + let mut ring = Vec::new(); + while let Some(w) = open.pop() { + is_open[w] = false; + ring.push(w); + if w == v { + break; + } + } + if ring.len() > 1 || leads[v].contains(&v) { + ring.into_iter().for_each(|w| ringed[w] = true); + } + } + } + } + testing.iter().zip(&ringed).filter(|(_, ringed)| **ringed).map(|((id, _), _)| *id).collect() + } +} + /// How siblings are listed: folders first, then the ones with a place of their own by it, then by /// name; the id settles any tie. fn album_order_key(a: &Album) -> (bool, bool, u32, String, AlbumId) { @@ -1189,6 +1374,8 @@ fn album_order_key(a: &Album) -> (bool, bool, u32, String, AlbumId) { #[cfg(test)] mod tests; #[cfg(test)] +mod tests_album_loops; +#[cfg(test)] mod tests_album_order; #[cfg(test)] mod tests_background; diff --git a/crates/catalog/src/tests_album_loops.rs b/crates/catalog/src/tests_album_loops.rs new file mode 100644 index 000000000..c378ca4f7 --- /dev/null +++ b/crates/catalog/src/tests_album_loops.rs @@ -0,0 +1,243 @@ +//! Smart albums that would include themselves, through the albums they test. +//! +//! The rule is the catalog's: a new edit that would put a smart album on such a loop is refused +//! (`Catalog::apply_new`), whoever makes it and however it is packed (alone, in a batch, as a +//! new album). A library that already holds a loop opens and keeps working (`Catalog::apply`, +//! `replay` and snapshots take it). +//! +//! Scenarios: +//! - When a new edit (adding a smart album, changing rules, alone or in a batch) would put a +//! smart album on a loop, then it is refused, names the album, and nothing changed. +//! - When a new edit makes no loop, then it goes through, whatever else is in the library. +//! - Given a library that already holds a loop, then it opens, and edits that add no loop go +//! through (also to the albums on it), and the loop can be mended. +//! - Given a ring of thousands of smart albums, then the edit that closes it is refused without +//! running out of stack. + +use crate::*; + +fn smart(c: &mut Catalog, name: &str, rules: Filter) -> AlbumId { + let id = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album { smart: Some(Box::new(rules)), ..Album::new(id, name) } }).unwrap(); + id +} + +fn plain(c: &mut Catalog, name: &str) -> AlbumId { + let id = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album::new(id, name) }).unwrap(); + id +} + +/// Rules of Album rules: `(op, album)` joined by `all`. +fn tests(rules: &[(&str, AlbumId)]) -> Filter { + let rules: Vec = rules.iter().map(|(op, a)| serde_json::json!({"field": "album", "op": op, "value": a.0})).collect(); + serde_json::from_value(serde_json::json!({"ruleSet": {"match": "all", "rules": rules}})).unwrap() +} + +/// The same, with the albums in a group of the rules. +fn tests_in_a_group(album: AlbumId) -> Filter { + serde_json::from_value(serde_json::json!({"ruleSet": {"match": "all", "rules": [ + {"field": "rating", "op": "is", "value": 3}, + {"group": {"match": "any", "rules": [{"field": "album", "op": "is", "value": album.0}]}} + ]}})) + .unwrap() +} + +fn set_rules(id: AlbumId, rules: Filter) -> Op { + Op::SetAlbumRules { id, rules: Box::new(rules) } +} + +fn rated(rating: u8) -> Filter { + Filter { rating, ..Default::default() } +} + +/// What the albums are: the whole library as saved. +fn saved(c: &Catalog) -> String { + c.to_snapshot() +} + +#[test] +fn a_new_edit_that_would_make_a_loop_is_refused() { + let mut c = Catalog::new(); + let a = smart(&mut c, "A", rated(1)); + let b = smart(&mut c, "B", tests(&[("is", a)])); + let chain = smart(&mut c, "Chain", tests(&[("isNot", b)])); + let (x, y) = (smart(&mut c, "X", rated(2)), smart(&mut c, "Y", rated(3))); + // F tests E; E is removed, so F tests an album that is not there (a saved rule may) + let e = smart(&mut c, "E", rated(4)); + let f = smart(&mut c, "F", tests(&[("is", e)])); + c.apply(Op::RemoveAlbum { id: e }).unwrap(); + let (before, revision) = (saved(&c), c.revision); + let refused: Vec<(&str, Op, &str)> = vec![ + ("tests an album that tests it", set_rules(a, tests(&[("is", b)])), "A"), + ("tests itself", set_rules(a, tests(&[("is", a)])), "A"), + ("tests itself through a group", set_rules(a, tests_in_a_group(a)), "A"), + ("tests the end of a chain that leads to it", set_rules(a, tests(&[("isNot", chain)])), "A"), + ("limited to itself by its album field", set_rules(a, Filter { album: Some(a), ..Default::default() }), "A"), + ("two edits, each alone without a loop", Op::Batch { ops: vec![set_rules(x, tests(&[("is", y)])), set_rules(y, tests(&[("is", x)]))] }, "X"), + ("a rename and a loop", Op::Batch { ops: vec![Op::RenameAlbum { id: a, name: "Renamed".into() }, set_rules(a, tests(&[("is", b)]))] }, "A"), + ( + "a batch in a batch", + Op::Batch { + ops: vec![Op::Batch { ops: vec![set_rules(x, tests(&[("is", y)]))] }, Op::Batch { ops: vec![set_rules(y, tests(&[("is", x)]))] }], + }, + "X", + ), + ( + "a new album that takes the id an album was tested by", + Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", f)]))), ..Album::new(e, "Back") } }, + "Back", + ), + ( + "a new album and its own rules in one batch", + Op::Batch { + ops: vec![ + Op::AddAlbum { album: Album { smart: Some(Box::new(rated(1))), ..Album::new(AlbumId(90), "Fresh") } }, + set_rules(AlbumId(90), tests(&[("is", AlbumId(90))])), + ], + }, + "Fresh", + ), + ]; + for (what, op, name) in refused { + let e = c.apply_new(op).unwrap_err().to_string(); + assert!(e.contains(&format!("“{name}”")) && e.contains("include itself"), "{what}: {e}"); + assert_eq!(saved(&c), before, "{what}: nothing changed"); + assert_eq!(c.revision, revision, "{what}: not even looked at as changed"); + } +} + +#[test] +fn a_new_edit_that_makes_no_loop_goes_through() { + let mut c = Catalog::new(); + let a = smart(&mut c, "A", rated(1)); + let b = smart(&mut c, "B", tests(&[("is", a)])); + let list = plain(&mut c, "List"); + let c1 = smart(&mut c, "C", tests(&[("isNot", b)])); + let allowed = vec![ + ("a chain gets longer", set_rules(a, rated(2))), + ("testing a plain album", set_rules(a, tests(&[("is", list)]))), + ("a smart album testing another, the other way round", set_rules(c1, tests(&[("is", a), ("is", b)]))), + ( + "a new smart album testing others", + Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", c1)]))), ..Album::new(AlbumId(50), "New") } }, + ), + ("a new plain album", Op::AddAlbum { album: Album::new(AlbumId(51), "Plain") }), + ("a rename", Op::RenameAlbum { id: b, name: "B2".into() }), + ("a removal", Op::RemoveAlbum { id: c1 }), + ("photos of a plain album", Op::SetAlbumPhotos { id: list, photos: vec![] }), + ("the album field of the rules", set_rules(b, Filter { album: Some(list), ..Default::default() })), + ( + "a batch of edits that don't meet", + Op::Batch { ops: vec![set_rules(a, rated(3)), Op::RenameAlbum { id: a, name: "A3".into() }, set_rules(b, tests(&[("is", a)]))] }, + ), + ( + "a batch that passes through a loop and ends without one", + Op::Batch { ops: vec![set_rules(a, tests(&[("is", b)])), set_rules(b, rated(1))] }, + ), + ]; + for (what, op) in allowed { + let revision = c.revision; + let undo = c.apply_new(op).unwrap_or_else(|e| panic!("{what}: {e}")); + assert_eq!(c.revision, revision + 1, "{what}"); + assert!(c.albums().all(|al| !c.smart_album_problems(al.id).iter().any(|p| p.issue == rules::Issue::AlbumLoop)), "{what}: no loop"); + // and it is undone by what it returned + c.apply(undo).unwrap(); + } +} + +/// The refusal is the only difference between the two ways in: an edit that makes no loop does +/// the same through either. +#[test] +fn apply_new_changes_what_apply_changes() { + let mut a = Catalog::new(); + let first = smart(&mut a, "A", rated(1)); + let second = smart(&mut a, "B", tests(&[("is", first)])); + let mut b = Catalog::from_snapshot(&saved(&a)).unwrap(); + let ops = vec![set_rules(first, rated(4)), Op::RenameAlbum { id: second, name: "B2".into() }, set_rules(second, tests(&[("isNot", first)]))]; + for op in ops { + let (ia, ib) = (a.apply(op.clone()).unwrap(), b.apply_new(op).unwrap()); + assert_eq!(Catalog::op_to_log_line(&ia), Catalog::op_to_log_line(&ib)); + } + assert_eq!(saved(&a), saved(&b)); +} + +/// A library saved by a version without the rule: it may hold a loop. It opens (a snapshot, a +/// replayed log), its albums still list and count, edits that make no loop go through, also +/// those to the albums on the loop, and an edit that closes another loop is still refused. +#[test] +fn a_library_that_already_holds_a_loop_opens_and_can_be_mended() { + let mut old = Catalog::new(); + let mut log = String::new(); + let mut record = |c: &mut Catalog, op: Op| { + log.push_str(&Catalog::op_to_log_line(&op)); + c.apply(op).unwrap(); + }; + let (first, second, other, third) = (AlbumId(1), AlbumId(2), AlbumId(3), AlbumId(4)); + record(&mut old, Op::AddAlbum { album: Album { smart: Some(Box::default()), ..Album::new(first, "First") } }); + record(&mut old, Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", first)]))), ..Album::new(second, "Second") } }); + record(&mut old, set_rules(first, tests(&[("is", second)]))); + record(&mut old, Op::AddAlbum { album: Album { smart: Some(Box::default()), ..Album::new(other, "Other") } }); + record(&mut old, Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", other)]))), ..Album::new(third, "Third") } }); + let mut c = Catalog::new(); + assert_eq!(c.replay(&log).unwrap(), 5, "the log replays, loop and all"); + let mut from_snapshot = Catalog::from_snapshot(&c.to_snapshot()).unwrap(); + for lib in [&mut c, &mut from_snapshot] { + // they still answer + for id in [first, second, other, third] { + let _ = (lib.album_photos(id), lib.album_count(id), lib.smart_album_problems(id)); + } + // edits that make no loop go through: elsewhere, and to the albums on the loop + lib.apply_new(Op::RenameAlbum { id: first, name: "First, renamed".into() }).unwrap(); + lib.apply_new(set_rules(other, rated(2))).unwrap(); + lib.apply_new(set_rules(third, tests(&[("isNot", other)]))).unwrap(); + lib.apply_new(Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", second)]))), ..Album::new(AlbumId(60), "Reads the loop") } }) + .unwrap(); + lib.apply_new(set_rules(first, Filter { rating: 3, rule_set: tests(&[("is", second)]).rule_set, ..Default::default() })).unwrap(); + // not one that closes another + let e = lib.apply_new(set_rules(other, tests(&[("is", third)]))).unwrap_err().to_string(); + assert!(e.contains("“Other”") && e.contains("include itself"), "{e}"); + // mended by an edit that makes no loop + lib.apply_new(set_rules(second, rated(1))).unwrap(); + assert!(!lib.smart_album_problems(first).iter().any(|p| p.issue == rules::Issue::AlbumLoop), "mended"); + assert!(!lib.smart_album_problems(second).iter().any(|p| p.issue == rules::Issue::AlbumLoop), "mended"); + } +} + +/// What an album on a loop holds is whatever it held before the refusal existed: the rule keeps +/// new loops out and decides nothing about the photos of old ones. +#[test] +fn what_an_album_on_a_loop_holds_is_left_as_it_was() { + let mut c = Catalog::new(); + let id = c.alloc_photo_id(); + c.apply(Op::AddPhoto { photo: Box::new(Photo::new(id, Source::Demo { scene: 1 }, "p.jpg", "JPEG", 60, 40, "2026-02-01T10:00:00")) }).unwrap(); + c.apply(Op::SetRating { id, rating: 3 }).unwrap(); + let first = smart(&mut c, "First", Filter::default()); + let second = smart(&mut c, "Second", tests(&[("is", first)])); + c.apply(set_rules(first, tests(&[("is", second)]))).unwrap(); + let held = |c: &Catalog| [first, second].map(|a| (c.album_photos(a), c.album_count(a))); + let looped = held(&c); + // edits that go through the check, one of them to an album on the loop + c.apply_new(set_rules(first, Filter { rule_set: tests(&[("is", second)]).rule_set, ..Default::default() })).unwrap(); + let id = c.alloc_album_id(); + c.apply_new(Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", second)]))), ..Album::new(id, "Reads the loop") } }).unwrap(); + assert_eq!(held(&c), looped); +} + +/// The search for loops walks the albums with its own stack: a ring of thousands of smart albums +/// is found, not overflowed. +#[test] +fn a_very_long_ring_is_refused_without_running_out_of_stack() { + const N: usize = 20_000; + let mut c = Catalog::new(); + let ids: Vec = (0..N).map(|i| smart(&mut c, &format!("S{i}"), Filter::default())).collect(); + // each tests the next; the last one is left open + for pair in ids.windows(2) { + c.apply(set_rules(pair[0], tests(&[("is", pair[1])]))).unwrap(); + } + let (first, last) = (ids[0], ids[N - 1]); + let e = c.apply_new(set_rules(last, tests(&[("is", first)]))).unwrap_err().to_string(); + assert!(e.contains("include itself"), "{e}"); + // and what makes no ring goes through + c.apply_new(set_rules(last, rated(5))).unwrap(); +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index acaa18070..163a10268 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -628,10 +628,12 @@ impl Session { // ---------------------------------------------------------------- ops, undo - /// Apply an op as one undoable step. + /// Apply an op as one undoable step. A new edit: one that would make a smart album include + /// itself is refused here, whichever command, task or caller asked ([`Catalog::apply_new`]); + /// undo, redo and opening a library bring back what was and are not held to that. pub fn commit(&mut self, label: &str, op: Op) -> Result<()> { let fwd = op.clone(); - let inv = self.apply_catalog(op)?; + let inv = self.apply_through(op, Catalog::apply_new)?; self.pending_log.push(fwd); self.undo.push(UndoEntry { label: label.to_string(), op: inv, folder: None, retry_inverse: Vec::new(), retry_sidecars: Vec::new() }); self.commits = self.commits.wrapping_add(1); @@ -645,7 +647,14 @@ impl Session { Ok(()) } + /// [`Catalog::apply`] for what brings back what was (an undo or a redo). fn apply_catalog(&mut self, op: Op) -> Result { + self.apply_through(op, Catalog::apply) + } + + /// Apply `op` with `apply` (the catalog's way in for a new edit or for bringing back what + /// was), then forget what the session cached of the photos it replaced. + fn apply_through(&mut self, op: Op, apply: fn(&mut Catalog, Op) -> std::result::Result) -> Result { // Retain only cache IDs while the catalog consumes the operation. Cloning an import // batch here also cloned every photo, although commit already keeps the journal copy. let mut invalidated = Vec::new(); @@ -664,7 +673,7 @@ impl Session { _ => {} } } - let inv = self.catalog.apply(op)?; + let inv = apply(&mut self.catalog, op)?; for id in invalidated { self.media.forget(id); } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae03859d2..057956c25 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2,6 +2,7 @@ use serde_json::{Value, json}; use crate::{Selection, Session, command_specs}; +mod album_loops; mod album_order; mod album_remove; mod albums_local; diff --git a/crates/engine/src/tests/album_loops.rs b/crates/engine/src/tests/album_loops.rs new file mode 100644 index 000000000..f010d8ae1 --- /dev/null +++ b/crates/engine/src/tests/album_loops.rs @@ -0,0 +1,102 @@ +//! A smart album never comes to include itself by a new edit, whatever makes the edit +//! (`Session::commit` keeps the catalog's rule, `Catalog::apply_new`); a library that already +//! holds such a loop opens, works and can be mended, and undo and redo move across it freely. +//! +//! Scenarios: +//! - When an edit that no command checked (an op committed on its own, in a batch of edits that +//! are fine one by one, or as a new album) would put a smart album on a loop, then it is +//! refused, and there is nothing to undo or to journal. +//! - When commands make the edits, then every way to a loop stays refused and ordinary edits go +//! on, one undo step each. +//! - Given a library that holds a loop, then edits that add none go through, and undo and redo +//! bring the loop back and take it away without being refused. + +use lightcraft_catalog::{Album, AlbumId, Filter, Op}; +use serde_json::json; + +use crate::Session; + +fn tests(albums: &[(&str, u64)]) -> Filter { + let rules: Vec<_> = albums.iter().map(|(op, a)| json!({"field": "album", "op": op, "value": a})).collect(); + serde_json::from_value(json!({"ruleSet": {"match": "all", "rules": rules}})).unwrap() +} + +fn smart(s: &mut Session, name: &str, rules: serde_json::Value) -> u64 { + s.execute("album.createSmart", &json!({"name": name, "rules": rules})).unwrap()["id"].as_u64().unwrap() +} + +fn set(id: u64, rules: Filter) -> Op { + Op::SetAlbumRules { id: AlbumId(id), rules: Box::new(rules) } +} + +#[test] +fn an_edit_no_command_checked_cant_make_a_loop() { + let mut s = Session::new(); + let a = smart(&mut s, "A", json!({"rating": 1})); + let b = smart(&mut s, "B", json!({"ruleSet": {"rules": [{"field": "album", "op": "is", "value": a}]}})); + let (x, y) = (smart(&mut s, "X", json!({"rating": 2})), smart(&mut s, "Y", json!({"rating": 3}))); + let gone = smart(&mut s, "Gone", json!({"rating": 4})); + let waits = smart(&mut s, "Waits", json!({"ruleSet": {"rules": [{"field": "album", "op": "is", "value": gone}]}})); + s.execute("album.delete", &json!({"id": gone})).unwrap(); + let (steps, logged, snapshot) = (s.undo.len(), s.pending_log.len(), s.catalog.to_snapshot()); + let back = Album { smart: Some(Box::new(tests(&[("is", waits)]))), ..Album::new(AlbumId(gone), "Back") }; + for (what, op) in [ + ("rules that test an album that tests it", set(a, tests(&[("is", b)]))), + ("two edits, each fine alone", Op::Batch { ops: vec![set(x, tests(&[("is", y)])), set(y, tests(&[("is", x)]))] }), + ("a new album that takes the id of one a rule tests", Op::AddAlbum { album: back }), + ] { + let e = s.commit("Edit", op).unwrap_err().to_string(); + assert!(e.contains("include itself"), "{what}: {e}"); + assert_eq!((s.undo.len(), s.pending_log.len()), (steps, logged), "{what}: nothing to undo, nothing journaled"); + assert_eq!(s.catalog.to_snapshot(), snapshot, "{what}: nothing changed"); + } +} + +#[test] +fn commands_refuse_every_way_to_a_loop_and_ordinary_edits_go_on() { + let mut s = Session::with_demo(); + let a = smart(&mut s, "A", json!({"rating": 1})); + let b = smart(&mut s, "B", json!({"ruleSet": {"rules": [{"field": "album", "op": "is", "value": a}]}})); + let steps = s.undo.len(); + let looping = |s: &mut Session, params: serde_json::Value| { + let e = s.execute("album.setRules", ¶ms).unwrap_err().to_string(); + assert!(e.contains("include itself"), "{e}"); + }; + looping(&mut s, json!({"id": a, "rules": {"ruleSet": {"rules": [{"field": "album", "op": "is", "value": b}]}}})); + looping(&mut s, json!({"id": a, "name": "Renamed too", "rules": {"ruleSet": {"rules": [{"field": "album", "op": "is", "value": a}]}}})); + assert_eq!((s.undo.len(), s.catalog.album(AlbumId(a)).map(|x| x.name.clone())), (steps, Some("A".into()))); + // ordinary edits: a rename together with new rules is one undo step + s.execute("album.setRules", &json!({"id": b, "name": "B2", "rules": {"rating": 2}})).unwrap(); + s.execute("album.rename", &json!({"id": a, "name": "A2"})).unwrap(); + assert_eq!(s.undo.len(), steps + 2); + s.execute("edit.undo", &json!({})).unwrap(); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.catalog.album(AlbumId(b)).map(|x| x.name.clone()), Some("B".into())); +} + +#[test] +fn a_library_that_holds_a_loop_goes_on_and_undo_and_redo_cross_it() { + let mut s = Session::with_demo(); + let a = smart(&mut s, "A", json!({"rating": 1})); + let b = smart(&mut s, "B", json!({"ruleSet": {"rules": [{"field": "album", "op": "is", "value": a}]}})); + // as a version without the rule saved it + s.catalog.apply(set(a, tests(&[("is", b)]))).unwrap(); + let on_a_loop = |s: &Session| { + [a, b] + .into_iter() + .filter(|id| s.catalog.smart_album_problems(AlbumId(*id)).iter().any(|p| p.issue == lightcraft_catalog::rules::Issue::AlbumLoop)) + .count() + }; + assert_eq!(on_a_loop(&s), 2); + // edits that add no loop: a rename, a change that leaves the rule as it is, a new album + s.execute("album.rename", &json!({"id": a, "name": "Still looping"})).unwrap(); + s.execute("album.setRules", &json!({"id": a, "rules": {"rating": 3}})).unwrap(); + smart(&mut s, "Reads the loop", json!({"ruleSet": {"rules": [{"field": "album", "op": "is", "value": a}]}})); + // mending it is one step; undo brings back what was and is not refused; redo mends again + s.execute("album.setRules", &json!({"id": b, "replace": true, "rules": {"rating": 2}})).unwrap(); + assert_eq!(on_a_loop(&s), 0); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(on_a_loop(&s), 2, "undo is no new edit"); + s.execute("edit.redo", &json!({})).unwrap(); + assert_eq!(on_a_loop(&s), 0); +}