mirror of
https://github.com/storytold/lightcraft.git
synced 2026-11-01 05:57:09 -06:00
Merge PR #3123: LC-001: stop commands after failed imports
This commit is contained in:
aiongg
@@ -600,14 +600,7 @@ fn run(args: &[String]) -> Result<(), String> {
|
||||
None => Headless::default(),
|
||||
};
|
||||
if !imports.is_empty() {
|
||||
let paths = expand_paths(&imports);
|
||||
let r = h.session.execute("library.import", &json!({"paths": paths})).map_err(|e| e.to_string())?;
|
||||
// what follows acts on the imported photos (already-known files are reported as duplicates)
|
||||
let mut ids: Vec<Value> = r["imported"].as_array().cloned().unwrap_or_default();
|
||||
ids.extend(r["duplicates"].as_array().into_iter().flatten().filter_map(|d| d.get("existing").filter(|v| v.is_u64()).cloned()));
|
||||
if let Some(first) = ids.first().cloned() {
|
||||
h.session.execute("library.select", &json!({"ids": ids, "active": first})).map_err(|e| e.to_string())?;
|
||||
}
|
||||
import_and_select(&mut h.session, &imports)?;
|
||||
}
|
||||
Box::new(h)
|
||||
}
|
||||
@@ -632,6 +625,34 @@ fn run(args: &[String]) -> Result<(), String> {
|
||||
if failed > 0 { Err(format!("{failed} command(s) failed")) } else { Ok(()) }
|
||||
}
|
||||
|
||||
fn import_and_select(session: &mut Session, imports: &[String]) -> Result<(), String> {
|
||||
let selection = session.selection.clone();
|
||||
let paths = expand_paths(imports);
|
||||
let result = (|| {
|
||||
let r = session.execute("library.import", &json!({"paths": paths})).map_err(|e| format!("import failed: {e}; no commands were run"))?;
|
||||
let ids = import_targets(&r)?;
|
||||
let first = ids.first().cloned().ok_or("import found no photos to select; no commands were run")?;
|
||||
session.execute("library.select", &json!({"ids": ids, "active": first})).map_err(|e| e.to_string())?;
|
||||
Ok::<(), String>(())
|
||||
})();
|
||||
if result.is_err() {
|
||||
session.selection = selection;
|
||||
}
|
||||
result
|
||||
}
|
||||
|
||||
fn import_targets(report: &Value) -> Result<Vec<Value>, String> {
|
||||
if let Some(failed) = report["failed"].as_array().filter(|failed| !failed.is_empty()) {
|
||||
return Err(format!("import failed for {} item(s): {}; no commands were run", failed.len(), json!(failed)));
|
||||
}
|
||||
let mut ids: Vec<Value> = report["imported"].as_array().cloned().unwrap_or_default();
|
||||
ids.extend(report["duplicates"].as_array().into_iter().flatten().filter_map(|d| d.get("existing").filter(|v| v.is_u64()).cloned()));
|
||||
ids.extend(report["restored"].as_array().into_iter().flatten().filter(|id| id.is_u64()).cloned());
|
||||
let mut seen = std::collections::HashSet::new();
|
||||
ids.retain(|id| id.as_u64().is_none_or(|id| seen.insert(id)));
|
||||
if ids.is_empty() { Err("import found no photos to select; no commands were run".into()) } else { Ok(ids) }
|
||||
}
|
||||
|
||||
fn take_value<'a>(args: &'a [String], i: &mut usize, flag: &str) -> Result<&'a str, String> {
|
||||
*i += 1;
|
||||
args.get(*i).map(String::as_str).ok_or_else(|| format!("{flag} needs a value"))
|
||||
@@ -1038,3 +1059,54 @@ fn denoise_serve(args: &[String]) -> Result<(), String> {
|
||||
fn denoise_serve(_: &[String]) -> Result<(), String> {
|
||||
Err("denoise-serve: this build has no denoise support (build with the `denoise` feature)".into())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod import_target_tests {
|
||||
use super::{import_and_select, import_targets, json};
|
||||
|
||||
#[test]
|
||||
fn empty_reports_and_duplicates_without_ids_have_no_target() {
|
||||
assert!(import_targets(&json!({})).is_err());
|
||||
assert!(import_targets(&json!({"imported": [], "duplicates": [{"existing": null}]})).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn restored_only_and_mixed_reports_keep_target_order() {
|
||||
assert_eq!(import_targets(&json!({"restored": [9]})).unwrap(), vec![json!(9)]);
|
||||
assert_eq!(
|
||||
import_targets(&json!({"imported": [1], "duplicates": [{"existing": null}, {"existing": 2}], "restored": [3]})).unwrap(),
|
||||
vec![json!(1), json!(2), json!(3)]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn repeated_targets_are_selected_once_in_first_occurrence_order() {
|
||||
let report = json!({"imported": [5, 7], "duplicates": [{"existing": 5}, {"existing": 8}, {"existing": 8}, {"existing": 7}, {"existing": null}], "restored": [8, 9, 5]});
|
||||
assert_eq!(import_targets(&report).unwrap(), vec![json!(5), json!(7), json!(8), json!(9)]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn failed_items_reject_even_when_targets_exist() {
|
||||
let error = import_targets(&json!({"imported": [1], "failed": [["broken.png", "invalid"]]})).unwrap_err();
|
||||
assert!(error.contains("1") && error.contains("broken.png") && error.contains("no commands were run"), "{error}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn failed_partial_import_restores_empty_selection() {
|
||||
let dir = std::env::temp_dir().join(format!("lightcraft-import-selection-{}", std::process::id()));
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let input = dir.join("valid.png");
|
||||
let broken = dir.join("broken.png");
|
||||
let img = lightcraft_raster::Rgba8::from_fn(2, 2, |_, _| [20, 40, 60, 255]);
|
||||
let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&img), &Default::default()).unwrap();
|
||||
std::fs::write(&input, png).unwrap();
|
||||
std::fs::write(&broken, b"not a PNG").unwrap();
|
||||
let mut headless = lightcraft_mcp::Headless::default();
|
||||
let session = &mut headless.session;
|
||||
let selection = session.selection.clone();
|
||||
assert!(selection.ids.is_empty() && selection.active.is_none());
|
||||
assert!(import_and_select(session, &[input.to_string_lossy().into_owned(), broken.to_string_lossy().into_owned()]).is_err());
|
||||
assert_eq!(session.selection, selection);
|
||||
let _ = std::fs::remove_dir_all(dir);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -582,6 +582,190 @@ fn run_script_params_validate_before_any_commands() {
|
||||
std::fs::remove_file(path).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn run_failed_import_never_edits_existing_selection() {
|
||||
let dir = tmp("failed-import");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let lib = dir.join("library");
|
||||
let missing = dir.join("missing.png");
|
||||
let empty = dir.join("empty");
|
||||
std::fs::create_dir(&empty).unwrap();
|
||||
assert!(!missing.exists());
|
||||
let lib = lib.to_str().unwrap();
|
||||
let (ok, lines, err) = run_cli(&["--demo", "--library", lib, "library.state"], None);
|
||||
assert!(ok, "{err}");
|
||||
let id = lines[0]["result"]["selection"]["active"].as_u64().unwrap();
|
||||
let (ok, _, err) = run_cli(&["--library", lib, "photo.rate", &format!("ids=[{id}]"), "rating=0"], None);
|
||||
assert!(ok, "{err}");
|
||||
for path in [&missing, &empty] {
|
||||
let (ok, lines, err) = run_cli(&["--library", lib, "--import", path.to_str().unwrap(), "photo.rate", "rating=5"], None);
|
||||
assert!(!ok, "import unexpectedly succeeded for {}: {lines:?}", path.display());
|
||||
assert!(lines.is_empty(), "commands ran: {lines:?}");
|
||||
assert!(err.contains("import") && err.contains("no commands were run"), "{err}");
|
||||
if path == &missing {
|
||||
assert!(err.contains("failed") && err.contains("missing.png"), "{err}");
|
||||
}
|
||||
let (ok, lines, err) = run_cli(&["--library", lib, "photo.inspect", &format!("id={id}")], None);
|
||||
assert!(ok, "{err}");
|
||||
assert_eq!(lines[0]["result"]["rating"], 0);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn run_partial_import_stops_commands_and_import_targets_remain_usable() {
|
||||
let dir = tmp("partial-import");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let input = dir.join("valid.png");
|
||||
let broken = dir.join("broken.png");
|
||||
gradient_png(&input);
|
||||
std::fs::write(&broken, b"not a PNG").unwrap();
|
||||
let mixed_lib = dir.join("mixed");
|
||||
let mixed_lib = mixed_lib.to_str().unwrap();
|
||||
let input = input.to_str().unwrap();
|
||||
let (ok, lines, err) = run_cli(&["--demo", "--library", mixed_lib, "catalog.query"], None);
|
||||
assert!(ok, "{err}");
|
||||
let photos = lines[0]["result"]["photos"].as_array().unwrap();
|
||||
let a = photos[0]["id"].as_u64().unwrap();
|
||||
let b = photos[1]["id"].as_u64().unwrap();
|
||||
let (ok, lines, err) = run_cli(
|
||||
&[
|
||||
"--library",
|
||||
mixed_lib,
|
||||
"library.select",
|
||||
&format!("ids=[{a},{b}]"),
|
||||
&format!("active={a}"),
|
||||
"photo.rate",
|
||||
&format!("ids=[{a}]"),
|
||||
"rating=0",
|
||||
"library.state",
|
||||
],
|
||||
None,
|
||||
);
|
||||
assert!(ok, "{err}");
|
||||
let selection = lines[2]["result"]["selection"].clone();
|
||||
assert_eq!(selection["ids"], json!([a, b]));
|
||||
assert_eq!(selection["active"], a);
|
||||
let (ok, lines, err) =
|
||||
run_cli(&["--library", mixed_lib, "--import", input, "--import", broken.to_str().unwrap(), "photo.rate", "rating=5"], None);
|
||||
let (query_ok, after, query_err) =
|
||||
run_cli(&["--library", mixed_lib, "library.state", "photo.inspect", &format!("id={a}"), "catalog.query"], None);
|
||||
assert!(query_ok, "{query_err}");
|
||||
assert_eq!(after[0]["result"]["selection"], selection, "selection changed after partial import");
|
||||
assert!(!ok, "partial import unexpectedly succeeded: {lines:?}");
|
||||
assert!(lines.is_empty() && err.contains("failed") && err.contains("no commands were run"), "{err}: {lines:?}");
|
||||
assert_eq!(after[1]["result"]["rating"], 0);
|
||||
assert!(after[2]["result"]["photos"].as_array().unwrap().iter().any(|p| p["fileName"] == "valid.png"));
|
||||
|
||||
let lib = dir.join("duplicates");
|
||||
let lib = lib.to_str().unwrap();
|
||||
let (ok, lines, err) = run_cli(&["--demo", "--library", lib, "library.state"], None);
|
||||
assert!(ok, "{err}");
|
||||
let previous = lines[0]["result"]["selection"]["active"].as_u64().unwrap();
|
||||
let (ok, lines, err) = run_cli(&["--library", lib, "--import", input, "photo.rate", "rating=4", "library.state"], None);
|
||||
assert!(ok, "{err}");
|
||||
let id = lines[1]["result"]["selection"]["active"].as_u64().unwrap();
|
||||
assert_ne!(id, previous);
|
||||
let (ok, _, err) = run_cli(&["--library", lib, "library.select", &format!("ids=[{previous}]"), "photo.rate", "rating=0"], None);
|
||||
assert!(ok, "{err}");
|
||||
let (ok, _, err) = run_cli(&["--library", lib, "--import", input, "photo.rate", "rating=3"], None);
|
||||
assert!(ok, "{err}");
|
||||
let (ok, lines, err) = run_cli(&["--library", lib, "photo.inspect", &format!("id={id}"), "photo.inspect", &format!("id={previous}")], None);
|
||||
assert!(ok, "{err}");
|
||||
assert_eq!(lines[0]["result"]["rating"], 3);
|
||||
assert_eq!(lines[1]["result"]["rating"], 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn run_restored_import_is_selected() {
|
||||
let dir = tmp("restored-import");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let input = dir.join("valid.png");
|
||||
gradient_png(&input);
|
||||
let lib = dir.join("library");
|
||||
let (lib, input) = (lib.to_str().unwrap(), input.to_str().unwrap());
|
||||
let (ok, lines, err) = run_cli(&["--library", lib, "--import", input, "library.state"], None);
|
||||
assert!(ok, "{err}");
|
||||
let id = lines[0]["result"]["selection"]["active"].as_u64().unwrap();
|
||||
let (ok, _, err) = run_cli(&["--library", lib, "photo.delete"], None);
|
||||
assert!(ok, "{err}");
|
||||
let (ok, _, err) = run_cli(&["--library", lib, "--import", input, "photo.rate", "rating=4"], None);
|
||||
assert!(ok, "{err}");
|
||||
let (ok, lines, err) = run_cli(&["--library", lib, "catalog.query"], None);
|
||||
assert!(ok, "{err}");
|
||||
assert_eq!(lines[0]["result"]["photos"].as_array().unwrap().len(), 1);
|
||||
assert_eq!(lines[0]["result"]["photos"][0]["id"], id);
|
||||
assert_eq!(lines[0]["result"]["photos"][0]["rating"], 4);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn run_same_batch_duplicates_selects_photo_once() {
|
||||
let dir = tmp("same-batch-duplicates");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let first = dir.join("first.png");
|
||||
let second = dir.join("second.png");
|
||||
gradient_png(&first);
|
||||
std::fs::copy(&first, &second).unwrap();
|
||||
let library = dir.join("library");
|
||||
let (ok, lines, err) = run_cli(
|
||||
&[
|
||||
"--library",
|
||||
library.to_str().unwrap(),
|
||||
"--import",
|
||||
first.to_str().unwrap(),
|
||||
"--import",
|
||||
second.to_str().unwrap(),
|
||||
"photo.rate",
|
||||
"rating=4",
|
||||
"library.state",
|
||||
"catalog.query",
|
||||
],
|
||||
None,
|
||||
);
|
||||
assert!(ok, "{err}");
|
||||
let selection = &lines[1]["result"]["selection"];
|
||||
let ids = selection["ids"].as_array().unwrap();
|
||||
assert_eq!(ids.len(), 1, "same photo selected more than once: {selection}");
|
||||
assert_eq!(selection["active"], ids[0]);
|
||||
let photos = lines[2]["result"]["photos"].as_array().unwrap();
|
||||
assert_eq!(photos.len(), 1);
|
||||
assert_eq!(photos[0]["id"], ids[0]);
|
||||
assert_eq!(photos[0]["rating"], 4);
|
||||
let _ = std::fs::remove_dir_all(dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn run_mixed_new_and_restored_import_selects_both() {
|
||||
let dir = tmp("mixed-restored-import");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let restored = dir.join("restored.png");
|
||||
let new = dir.join("new.png");
|
||||
gradient_png(&restored);
|
||||
let image = lightcraft_raster::Rgba8::from_fn(120, 80, |x, y| [(x * 2) as u8, (y * 3) as u8, 200, 255]);
|
||||
let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&image), &Default::default()).unwrap();
|
||||
std::fs::write(&new, png).unwrap();
|
||||
let lib = dir.join("library");
|
||||
let (lib, restored, new) = (lib.to_str().unwrap(), restored.to_str().unwrap(), new.to_str().unwrap());
|
||||
let (ok, lines, err) = run_cli(&["--library", lib, "--import", restored, "library.state"], None);
|
||||
assert!(ok, "{err}");
|
||||
let restored_id = lines[0]["result"]["selection"]["active"].as_u64().unwrap();
|
||||
let (ok, _, err) = run_cli(&["--library", lib, "photo.delete"], None);
|
||||
assert!(ok, "{err}");
|
||||
let (ok, lines, err) = run_cli(&["--library", lib, "--import", new, "--import", restored, "photo.rate", "rating=4", "library.state"], None);
|
||||
assert!(ok, "{err}");
|
||||
assert_eq!(lines[1]["result"]["selection"]["ids"].as_array().unwrap().len(), 2);
|
||||
let (ok, lines, err) = run_cli(&["--library", lib, "catalog.query"], None);
|
||||
assert!(ok, "{err}");
|
||||
let photos = lines[0]["result"]["photos"].as_array().unwrap();
|
||||
assert_eq!(photos.len(), 2);
|
||||
assert!(photos.iter().any(|p| p["id"] == restored_id && p["rating"] == 4));
|
||||
assert!(photos.iter().any(|p| p["fileName"] == "new.png" && p["id"] != restored_id && p["rating"] == 4));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn run_subcommand_chains_commands_and_persists_a_library() {
|
||||
let input = tmp("run-in.png");
|
||||
|
||||
Reference in New Issue
Block a user