collective
Browse / github.com/storytold/lightcraft / patch
Issue #2165: a Lightroom collection that can't nest goes under the nearest set instead of failing the import
Lightroom import fails during collection finalization: "parent must be an existing folder" <html><body> <!--StartFragment--><html><head></head><body><h2><span>Bug: Lightroom import fails during collection finalization — </span><code><span>parent must be an existing folder</span></code></h2><h3><span>Summary</span></h3><p><span>Importing my Lightroom Classic catalog consistently imports the photos but fails to create any Lightroom collections/albums.</span></p><p><span>This has now been reproduced using both LightCraft 0.6.0 (GUI) and development builds with the optimized, batched Lightroom importer discussed in #832.</span></p><p><span>A headless CLI import using the branch from #20
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- THEtheChad (imported from pull request #3145, open)
- Base
662df66f1c08bdb1b1f1d0c1c638c4cef936d0b0- Size
- +336 −6 in 4 files
- Review
- pending
- Shared
- 2026-10-28
- Id
d75fe91331eee91ebf953433
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/lightroom_catalog.rs crates/engine/src/lightroom_catalog/tests.rs docs/lightroom-catalog-import.md docs/whats-new.md
@@ -1023,22 +1023,40 @@ pub(crate) fn apply_prepared(s: &mut crate::Session, mut data: CatalogImport, co by_path.entry(key(&src.path)).or_insert(photo_id); } let mut albums = HashMap::new();+ // Two collections never share an album. An index that names one album for two gives the second a new one; the+ // counter saturates, so a damaged one hands out an id that's taken, and that stops the import instead.+ let mut assigned = HashSet::new(); for collection in &data.collections {- if number(collection, "systemOnly") != 0 {+ // a collection listed twice keeps one album; the album loop below tells a repeat from a conflict+ if number(collection, "systemOnly") != 0 || albums.contains_key(&number(collection, "id_local")) { continue; } let collection_key = format!("{}:{}", key(&data.source), number(collection, "id_local"));- let id = index+ let indexed = index .collections .get(&collection_key) .copied() .map(lightcraft_catalog::AlbumId)- .filter(|id| s.catalog.album(*id).is_some())- .unwrap_or_else(|| s.catalog.alloc_album_id());+ .filter(|id| s.catalog.album(*id).is_some() && !assigned.contains(id));+ let id = match indexed {+ Some(id) => id,+ None => {+ let id = s.catalog.alloc_album_id();+ if s.catalog.album(id).is_some() || assigned.contains(&id) {+ return Err(error(format!(+ "collection “{}”: the library has no unused album id left for it (its album numbering is exhausted or damaged)",+ text(collection, "name")+ )));+ }+ id+ }+ };+ assigned.insert(id); index.collections.insert(collection_key, id.0); albums.insert(number(collection, "id_local"), id); } let parents: HashMap<_, _> = data.collections.iter().map(|r| (number(r, "id_local"), number(r, "parent"))).collect();+ let collections: HashMap<_, _> = data.collections.iter().map(|r| (number(r, "id_local"), r)).collect(); let mut grouped_members: HashMap<i64, Vec<&Row>> = HashMap::new(); for member in &data.members { grouped_members.entry(number(member, "collection")).or_default().push(member);@@ -1056,10 +1074,33 @@ pub(crate) fn apply_prepared(s: &mut crate::Session, mut data: CatalogImport, co } depth });+ // Collections whose albums are folders: a collection set, or an earlier import's album when it is a folder.+ let sets: HashSet<i64> = albums+ .iter()+ .filter(|(lr, id)| match s.catalog.album(**id) {+ Some(album) => album.folder,+ None => collections.get(*lr).is_some_and(|r| text(r, "creationId").contains("group")),+ })+ .map(|(lr, _)| *lr)+ .collect();+ // The collection each album was staged for: a collection listed twice is skipped the second time only when the+ // rows are the same, never a different collection silently dropped.+ let mut staged: HashMap<_, &Row> = HashMap::new();+ let mut added_folders = HashSet::new(); for collection in ordered { let Some(id) = albums.get(&number(collection, "id_local")).copied() else { continue };+ if let Some(first) = staged.insert(id, collection) {+ if first == collection {+ continue;+ }+ return Err(error(format!(+ "the catalog lists collection {} twice, as “{}” and as “{}”",+ number(collection, "id_local"),+ text(first, "name"),+ text(collection, "name")+ )));+ } let mut album = Album::new(id, text(collection, "name"));- album.parent = albums.get(&number(collection, "parent")).copied(); album.folder = text(collection, "creationId").contains("group"); if text(collection, "creationId").contains("smart") { warnings.push(format!("{}: smart rules preserved in archive; imported current membership", album.name));@@ -1085,6 +1126,45 @@ pub(crate) fn apply_prepared(s: &mut crate::Session, mut data: CatalogImport, co ops.push(Op::SetAlbumPhotos { id, photos }); } } else {+ let (set, moved) = collection_parent(collection, &collections, &sets);+ album.parent = set.and_then(|set| albums.get(&set).copied());+ // `AddAlbum` rejects a parent that isn't a folder yet, and that would roll back the whole batch: every+ // album and every catalog-only photo. `collection_parent` already prevents it; this keeps a bug there+ // (or a cycle `validate` missed) from doing so.+ if let Some(parent) = album.parent+ && !added_folders.contains(&parent)+ && !s.catalog.album(parent).is_some_and(|a| a.folder)+ {+ album.parent = None;+ warnings.push(format!(+ "{}: its collection set could not be created first; imported at the top level (Lightroom collection {})",+ album.name,+ number(collection, "id_local")+ ));+ } else if moved {+ let lr_parent = number(collection, "parent");+ let why = match collections.get(&lr_parent) {+ None => "its parent collection isn't in the catalog".to_string(),+ Some(r) => match albums.get(&lr_parent).and_then(|id| s.catalog.album(*id)) {+ // a set imported earlier whose album is no longer a folder+ Some(a) if text(r, "creationId").contains("group") => {+ format!("its collection set “{}” is the album “{}” in this library, which isn't a folder", text(r, "name"), a.name)+ }+ _ => format!("Lightroom keeps it inside “{}”, which isn't a collection set", text(r, "name")),+ },+ };+ // the set's name in this library: the user may have renamed it since an earlier import+ let to = match (album.parent.and_then(|id| s.catalog.album(id)), set.and_then(|set| collections.get(&set))) {+ (Some(a), _) => format!("into “{}”", a.name),+ (None, Some(r)) => format!("into “{}”", text(r, "name")),+ (None, None) => "at the top level".to_string(),+ };+ // the Lightroom id tells apart collections that share a name (#2165)+ warnings.push(format!("{}: {why}; imported {to} (Lightroom collection {})", album.name, number(collection, "id_local")));+ }+ if album.folder {+ added_folders.insert(id);+ } ops.push(Op::AddAlbum { album }); } }@@ -1098,5 +1178,22 @@ pub(crate) fn apply_prepared(s: &mut crate::Session, mut data: CatalogImport, co }) } +/// The collection set a Lightroom collection's album goes into (`None`: the top level), and whether that isn't its+/// Lightroom parent. A LightCraft album nests only in a folder, but Lightroom also keeps a print, book, slideshow or+/// web gallery (or, in an odd catalog, a collection) inside a plain or smart collection, and a parent can be+/// system-only (not imported) or gone from the catalog. Such an album joins the nearest collection set above it.+fn collection_parent(collection: &Row, collections: &HashMap<i64, &Row>, sets: &HashSet<i64>) -> (Option<i64>, bool) {+ let mut at = number(collection, "parent");+ let mut seen = HashSet::from([number(collection, "id_local")]);+ while at != 0 && !sets.contains(&at) {+ // `validate` rejects cycles and deep chains; stop at the top level if one got here anyway.+ if seen.len() > 64 || !seen.insert(at) {+ return (None, true);+ }+ at = collections.get(&at).map_or(0, |r| number(r, "parent"));+ }+ ((at != 0).then_some(at), at != number(collection, "parent"))+}+ #[cfg(test)] mod tests;@@ -157,6 +157,225 @@ fn imports_copies_collections_ratings_and_mapped_edits_in_one_undo_step() { assert!(s.catalog.albums().next().is_none()); assert!(data.photos[1].settings.contains("CameraProfile")); // unmapped source stays intact }+/// The import report's warnings about `album`.+fn warnings_naming(r: &Value, album: &str) -> Vec<String> {+ r["warnings"].as_array().unwrap().iter().filter_map(Value::as_str).filter(|w| w.starts_with(&format!("{album}:"))).map(String::from).collect()+}+fn album_named<'a>(s: &'a crate::Session, name: &str) -> &'a lightcraft_catalog::Album {+ s.catalog.albums().find(|a| a.name == name).unwrap_or_else(|| panic!("no album {name}"))+}+/// A LightCraft album nests only in a folder. A collection Lightroom keeps inside a plain collection used to fail the+/// whole batch with "parent must be an existing folder", taking every album with it (#2165, #2532).+#[test]+fn a_collection_inside_a_plain_collection_joins_the_set_above_it() {+ let mut s = crate::Session::new();+ let mut data = sample();+ data.collections.push(row(json!({"id_local":11,"parent":10,"name":"Picks","creationId":"com.adobe.ag.library.collection"})));+ data.members.push(row(json!({"collection":11,"image":1,"positionInCollection":"a"})));+ let r = apply(&mut s, data, false).unwrap();+ assert_eq!(r["collections"], 3);+ assert_eq!(s.catalog.photos().count(), 2);+ let travel = album_named(&s, "Travel").id;+ assert_eq!(album_named(&s, "Keepers").parent, Some(travel));+ assert_eq!(album_named(&s, "Keepers").photos.len(), 2);+ assert_eq!(album_named(&s, "Picks").parent, Some(travel));+ assert_eq!(album_named(&s, "Picks").photos.len(), 1);+ let w = warnings_naming(&r, "Picks");+ assert_eq!(w.len(), 1, "{w:?}");+ assert!(w[0].contains("Keepers") && w[0].contains("into “Travel”"), "{w:?}");+ assert!(warnings_naming(&r, "Keepers").is_empty());+}+/// Lightroom Classic saves a print, book, slideshow or web gallery inside a collection; with no set above it, the+/// creation lands at the top level.+#[test]+fn a_creation_saved_inside_a_top_level_collection_lands_at_the_top_level() {+ let mut s = crate::Session::new();+ let mut data = sample();+ data.collections.push(row(json!({"id_local":12,"name":"Loose","creationId":"com.adobe.ag.library.collection"})));+ data.collections.push(row(json!({"id_local":13,"parent":12,"name":"Poster","creationId":"com.adobe.ag.print"})));+ data.collections.push(row(json!({"id_local":14,"parent":10,"name":"Album Book","creationId":"com.adobe.ag.layout.book"})));+ data.members.push(row(json!({"collection":13,"image":1,"positionInCollection":"a"})));+ let r = apply(&mut s, data, false).unwrap();+ assert_eq!(r["collections"], 5);+ let poster = album_named(&s, "Poster");+ assert_eq!((poster.parent, poster.folder, poster.photos.len()), (None, false, 1));+ assert_eq!(album_named(&s, "Album Book").parent, Some(album_named(&s, "Travel").id));+ let w = warnings_naming(&r, "Poster");+ assert!(w.len() == 1 && w[0].contains("Loose") && w[0].contains("at the top level"), "{w:?}");+ assert_eq!(warnings_naming(&r, "Album Book").len(), 1);+ assert_eq!(album_named(&s, "Keepers").photos.len(), 2);+}+/// A parent that's gone from the catalog or system-only (Quick Collection, an unsaved print…) isn't imported; the+/// album lands at the top level, and the report says why it isn't where Lightroom had it.+#[test]+fn a_collection_whose_parent_is_missing_or_system_only_lands_at_the_top_level_with_a_warning() {+ let mut s = crate::Session::new();+ let mut data = sample();+ data.collections.push(row(json!({"id_local":11,"parent":99,"name":"Orphan","creationId":"com.adobe.ag.library.collection"})));+ data.collections.push(row(json!({"id_local":5,"name":"quick collection","creationId":"com.adobe.ag.library.collection","systemOnly":1})));+ data.collections.push(row(json!({"id_local":12,"parent":5,"name":"Stray","creationId":"com.adobe.ag.print"})));+ let r = apply(&mut s, data, false).unwrap();+ assert_eq!(r["collections"], 4);+ assert_eq!(album_named(&s, "Orphan").parent, None);+ assert_eq!(album_named(&s, "Stray").parent, None);+ assert!(s.catalog.albums().all(|a| a.name != "quick collection"));+ let w = warnings_naming(&r, "Orphan");+ assert!(w.len() == 1 && w[0].contains("isn't in the catalog"), "{w:?}");+ let w = warnings_naming(&r, "Stray");+ assert!(w.len() == 1 && w[0].contains("quick collection") && w[0].contains("at the top level"), "{w:?}");+ assert_eq!(album_named(&s, "Keepers").parent, Some(album_named(&s, "Travel").id));+}+/// Imports `data` keeping `index` between imports, as a library on disk does.+fn import_indexed(s: &mut crate::Session, data: CatalogImport, index: &mut ImportIndex) -> crate::Result<Value> {+ let undo0 = s.undo.len();+ let missing_sources = HashSet::new();+ let context =+ ApplyPrepared { update_existing: false, files: Default::default(), index, archive_path: None, missing_sources: &missing_sources, undo0 };+ apply_prepared(s, data, context).map(|applied| applied.report)+}+/// The sample with “Picks”, a collection Lightroom keeps inside the plain collection “Keepers”.+fn with_picks() -> CatalogImport {+ let mut data = sample();+ data.collections.push(row(json!({"id_local":11,"parent":10,"name":"Picks","creationId":"com.adobe.ag.library.collection"})));+ data.members.push(row(json!({"collection":11,"image":1,"positionInCollection":"a"})));+ data+}+/// Importing the catalog again finds the moved album where the first import put it, adds nothing twice and doesn't+/// warn again: the album is already in the library.+#[test]+fn a_relocated_collection_keeps_its_place_on_reimport_without_duplicates_or_a_second_warning() {+ let mut s = crate::Session::new();+ let mut index = ImportIndex::default();+ let first = import_indexed(&mut s, with_picks(), &mut index).unwrap();+ assert_eq!(warnings_naming(&first, "Picks").len(), 1);+ let second = import_indexed(&mut s, with_picks(), &mut index).unwrap();+ assert_eq!(second["collections"], 3);+ assert_eq!(s.catalog.albums().count(), 3);+ assert_eq!(album_named(&s, "Picks").parent, Some(album_named(&s, "Travel").id));+ assert_eq!(album_named(&s, "Picks").photos.len(), 1);+ assert_eq!(album_named(&s, "Keepers").photos.len(), 2);+ assert!(warnings_naming(&second, "Picks").is_empty(), "{second}");+}+/// Albums the user moved after an import stay where the user put them when the catalog is imported again.+#[test]+fn an_album_the_user_moved_keeps_its_place_on_reimport() {+ let mut s = crate::Session::new();+ let mut index = ImportIndex::default();+ import_indexed(&mut s, with_picks(), &mut index).unwrap();+ for name in ["Picks", "Keepers"] {+ let id = album_named(&s, name).id;+ s.commit("Move Album", Op::MoveAlbum { id, parent: None }).unwrap();+ }+ let r = import_indexed(&mut s, with_picks(), &mut index).unwrap();+ assert_eq!(s.catalog.albums().count(), 3);+ assert_eq!(album_named(&s, "Picks").parent, None);+ assert_eq!(album_named(&s, "Keepers").parent, None);+ assert!(warnings_naming(&r, "Picks").is_empty() && warnings_naming(&r, "Keepers").is_empty(), "{r}");+}+/// A collection new since the last import goes into the set's album, and the warning names that album as the+/// library calls it now, not by its Lightroom name.+#[test]+fn a_relocation_warning_names_the_set_as_the_user_renamed_it() {+ let mut s = crate::Session::new();+ let mut index = ImportIndex::default();+ import_indexed(&mut s, sample(), &mut index).unwrap();+ let travel = album_named(&s, "Travel").id;+ s.commit("Rename Album", Op::RenameAlbum { id: travel, name: "Trips".into() }).unwrap();+ let r = import_indexed(&mut s, with_picks(), &mut index).unwrap();+ assert_eq!(album_named(&s, "Picks").parent, Some(travel));+ let w = warnings_naming(&r, "Picks");+ assert!(w.len() == 1 && w[0].contains("inside “Keepers”") && w[0].ends_with("imported into “Trips” (Lightroom collection 11)"), "{w:?}");+}+/// The album an earlier import made for a Lightroom set may no longer be a folder (here: an index that names a plain+/// album). A collection in that set lands at the top level, and the warning says why without calling the set+/// something it isn't.+#[test]+fn a_set_whose_album_is_not_a_folder_puts_its_collections_at_the_top_level() {+ let mut s = crate::Session::new();+ let plain = s.catalog.alloc_album_id();+ s.commit("New Album", Op::AddAlbum { album: Album::new(plain, "Travel picks") }).unwrap();+ let mut index = ImportIndex::default();+ index.collections.insert(format!("{}:9", key("test.lrcat")), plain.0);+ let r = import_indexed(&mut s, sample(), &mut index).unwrap();+ assert_eq!(s.catalog.albums().count(), 2);+ assert!(!s.catalog.album(plain).unwrap().folder);+ let keepers = album_named(&s, "Keepers");+ assert_eq!((keepers.parent, keepers.photos.len()), (None, 2));+ let w = warnings_naming(&r, "Keepers");+ assert_eq!(w.len(), 1, "{w:?}");+ assert!(w[0].contains("“Travel” is the album “Travel picks”") && w[0].contains("isn't a folder"), "{w:?}");+ assert!(!w[0].contains("isn't a collection set") && w[0].ends_with("at the top level (Lightroom collection 10)"), "{w:?}");+}+/// Only the outermost set inside a plain collection moves; the sets and creations inside it keep their places.+#[test]+fn sets_nested_under_a_plain_collection_keep_their_own_nesting() {+ let mut s = crate::Session::new();+ let mut data = sample();+ data.collections.push(row(json!({"id_local":12,"name":"Loose","creationId":"com.adobe.ag.library.collection"})));+ data.collections.push(row(json!({"id_local":13,"parent":12,"name":"Outer","creationId":"com.adobe.ag.library.group"})));+ data.collections.push(row(json!({"id_local":14,"parent":13,"name":"Inner","creationId":"com.adobe.ag.library.group"})));+ data.collections.push(row(json!({"id_local":15,"parent":14,"name":"Poster","creationId":"com.adobe.ag.print"})));+ data.members.push(row(json!({"collection":15,"image":1,"positionInCollection":"a"})));+ let r = apply(&mut s, data, false).unwrap();+ assert_eq!(r["collections"], 6);+ let outer = album_named(&s, "Outer");+ assert_eq!((outer.parent, outer.folder), (None, true));+ let inner = album_named(&s, "Inner");+ assert_eq!((inner.parent, inner.folder), (Some(outer.id), true));+ assert_eq!(album_named(&s, "Poster").parent, Some(inner.id));+ assert_eq!(album_named(&s, "Poster").photos.len(), 1);+ let w = warnings_naming(&r, "Outer");+ assert!(w.len() == 1 && w[0].contains("inside “Loose”") && w[0].ends_with("at the top level (Lightroom collection 13)"), "{w:?}");+ assert!(warnings_naming(&r, "Inner").is_empty() && warnings_naming(&r, "Poster").is_empty(), "{r}");+}+/// The album counter saturates: one at its end hands every collection the same id. The second collection used to be+/// dropped without a word while the report counted it; the import now stops and names it.+#[test]+fn an_exhausted_album_counter_stops_the_import_naming_the_collection() {+ let mut s = crate::Session::new();+ s.commit("New Album", Op::AddAlbum { album: Album::new(lightcraft_catalog::AlbumId(u64::MAX - 1), "Last") }).unwrap();+ let err = apply(&mut s, sample(), false).unwrap_err().to_string();+ assert!(err.contains("“Keepers”") && err.contains("no unused album id"), "{err}");+ assert_eq!(s.catalog.albums().count(), 1);+ assert!(s.catalog.is_empty());+}+/// A collection listed twice imports once when both rows are the same; two different collections under one Lightroom+/// id stop the import instead of one of them disappearing.+#[test]+fn a_collection_listed_twice_imports_once_and_a_conflicting_one_stops_the_import() {+ let mut s = crate::Session::new();+ let mut data = sample();+ data.collections.push(data.collections[1].clone());+ let r = apply(&mut s, data, false).unwrap();+ assert_eq!(r["collections"], 2);+ assert_eq!(s.catalog.albums().filter(|a| a.name == "Keepers").count(), 1);+ assert_eq!(album_named(&s, "Keepers").photos.len(), 2);++ let mut s = crate::Session::new();+ let mut data = sample();+ data.collections.push(row(json!({"id_local":10,"parent":9,"name":"Rejects","creationId":"collection"})));+ let err = apply(&mut s, data, false).unwrap_err().to_string();+ assert!(err.contains("“Keepers”") && err.contains("“Rejects”") && err.contains("collection 10 twice"), "{err}");+ assert!(s.catalog.albums().next().is_none() && s.catalog.is_empty());+}+/// A damaged index that names one existing album for two collections gives the second an album of its own rather+/// than merging its photos into the first's.+#[test]+fn an_index_naming_one_album_for_two_collections_gives_the_second_its_own() {+ let mut s = crate::Session::new();+ let old = s.catalog.alloc_album_id();+ s.commit("New Album", Op::AddAlbum { album: Album::new(old, "Old") }).unwrap();+ let mut index = ImportIndex::default();+ for lr in [10, 11] {+ index.collections.insert(format!("{}:{lr}", key("test.lrcat")), old.0);+ }+ let r = import_indexed(&mut s, with_picks(), &mut index).unwrap();+ assert_eq!(r["collections"], 3);+ assert_eq!(s.catalog.album(old).unwrap().photos.len(), 2, "Keepers merges into the album the index names");+ let picks = album_named(&s, "Picks");+ assert_ne!(picks.id, old);+ assert_eq!(picks.photos.len(), 1);+} #[test] fn preserves_existing_edits_and_rejects_invalid_structure_before_mutation() { let mut s = crate::Session::new();@@ -104,7 +104,12 @@ fields are reported. Lightroom's `ProcessVersion` is not mapped: newly imported LightCraft's latest rendering process, and photos already in the library keep theirs ([process-versions.md](process-versions.md)). When a recovery archive is saved, it retains source settings/history/snapshots and original smart-collection rules. Smart collections become regular albums with current-membership. Archived history and snapshots are source data, not native LightCraft history yet.+membership, and so do saved prints, books, slideshows and web galleries (with the photos they hold). A LightCraft+album nests only inside a folder (a Lightroom collection set). A collection or creation Lightroom keeps elsewhere,+such as a print saved inside a plain collection, or one whose parent is system-only or gone from the catalog, joins the+nearest collection set above it, or the top level, and the result names it under warnings (issues #2165, #2532).+Quick Collection and unsaved creations are system-only and not imported; publish collections live in tables the+importer doesn't read. Archived history and snapshots are source data, not native LightCraft history yet. A Custom white balance on a raw LightCraft develops relative to its as-shot look (ARW, NEF, RW2, RAF, CR3, CR2, PEF, SRW; Lightroom names them all `RAW`) can't be taken as Kelvin: the catalog keeps no as-shot white for Custom photos, so the photo stays As Shot and `Temperature, Tint` are reported@@ -1,5 +1,14 @@ # What's new in LightCraft +## 2026-10-26++### Lightroom catalogs with a print inside a collection import their collections+- Lightroom Classic can keep a saved print, book, slideshow or web gallery inside a plain collection, but a+ LightCraft album sits only inside a folder. Importing such a catalog stopped at the end with "parent must be an+ existing folder" and brought over no collections, and photos whose originals were offline were lost with them+ (issues #2165, #2532). Such a collection now joins the nearest collection set above it, or the top level, and the+ import result names it under warnings; so does one whose parent is system-only or gone from the catalog.+ ## 2026-10-24 ### Relative-date filters and smart albums follow the clock