Reject colliding data-set export filenames before writing (#2433)

crates/engine/src/variables_cmds.rs

@@ -363,7 +363,6 @@ fn split_csv(line: &str, delim: u8) -> Vec<String> {
 fn export_as_files(s: &mut Session, p: &Value) -> Result<Value> {
     let dir = p.get("dir").and_then(Value::as_str).ok_or_else(|| bad("file.export.dataSetsAsFiles", "need `dir`"))?;
     let format = p.get("format").and_then(Value::as_str).unwrap_or("png");
-    std::fs::create_dir_all(dir).map_err(|e| EngineError::Other(format!("mkdir `{dir}`: {e}")))?;
     let st = s.active().ok_or(EngineError::NoDocument)?;
     let original = st.doc.clone();
     let vars = original.variables.clone();
@@ -375,17 +374,29 @@ fn export_as_files(s: &mut Session, p: &Value) -> Result<Value> {
     // Filename template: `{name}` (sanitised data-set name), `{index}` (1-based). Default `{name}`.
     let template = p.get("naming").and_then(Value::as_str).unwrap_or("{name}");
     let doc_stem = original.name.rsplit_once('.').map_or(original.name.as_str(), |(a, _)| a).to_string();
+    // Plan the whole batch before writing: sanitised names or a constant template may collide.
+    // Match the case-insensitive comparison used by the other batch exporters.
+    let mut claimed = std::collections::HashMap::new();
     let mut files = Vec::new();
     for (n, set) in sets.iter().enumerate() {
-        let mut doc = (*original).clone();
-        apply_to_doc(&mut doc, &vars, set)?;
         let safe: String = set.name.chars().map(|c| if c.is_alphanumeric() || c == '-' || c == '_' { c } else { '_' }).collect();
         let stem = template.replace("{name}", &safe).replace("{index}", &format!("{:03}", n + 1)).replace("{document}", &doc_stem);
         let path = format!("{dir}/{stem}.{format}");
+        if let Some(first) = claimed.insert(path.to_lowercase(), &set.name) {
+            return Err(bad(
+                "file.export.dataSetsAsFiles",
+                format!("data sets `{first}` and `{}` resolve to the same output `{path}`; include {{index}} in `naming` to use distinct filenames", set.name),
+            ));
+        }
+        files.push(path);
+    }
+    std::fs::create_dir_all(dir).map_err(|e| EngineError::Other(format!("mkdir `{dir}`: {e}")))?;
+    for (set, path) in sets.iter().zip(&files) {
+        let mut doc = (*original).clone();
+        apply_to_doc(&mut doc, &vars, set)?;
         let opts = photocraft_io::ExportOptions::default();
         let bytes = photocraft_io::export(&doc, format, &opts).map(|r| r.bytes).map_err(|e| EngineError::Other(format!("export `{}`: {e}", set.name)))?;
-        crate::file_cmds::write_file(&path, &bytes)?;
-        files.push(path);
+        crate::file_cmds::write_file(path, &bytes)?;
     }
     Ok(json!({"files": files, "count": files.len()}))
 }
@@ -446,7 +457,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "file.export.dataSetsAsFiles",
             "Data Sets as Files…",
             &["File", "Export"],
-            "{dir, format?:png, dataSets?[names], naming?:\"{name}|{index}|{document}\"} → {files,count}: apply each data set and export the flattened document",
+            "{dir, format?:png, dataSets?[names], naming?:\"{name}|{index}|{document}\"} → {files,count}: apply each data set and export the flattened document; colliding filenames are rejected before writing (include {index} to disambiguate)",
             |s, p| export_as_files(s, p)
         ),
         CommandSpec {

crates/engine/src/variables_cmds/tests.rs

@@ -321,6 +321,88 @@ fn export_data_sets_as_files() {
     assert_eq!(text_of(&s, title), "Old");
 }
 
+fn visibility_export_session(names: &[&str]) -> Session {
+    let mut s = Session::new();
+    s.execute("file.new", json!({"width": 8, "height": 8, "background": "white", "name": "card"})).unwrap();
+    let layer = doc(&s).layers[0].id;
+    s.execute("image.variables.define", json!({"defs": [{"name": "show", "layer": layer.0, "type": "visibility"}]})).unwrap();
+    let sets: Vec<_> =
+        names.iter().enumerate().map(|(i, name)| json!({"name": name, "values": [{"variable": "show", "kind": "visibility", "value": i == 0}]})).collect();
+    s.execute("image.variables.dataSets", json!({"dataSets": sets})).unwrap();
+    s
+}
+
+#[test]
+fn export_data_sets_rejects_collisions_before_writing() {
+    for (label, names, naming, collision) in [
+        ("sanitized", ["unique", "a b", "a_b"], "{name}", "a_b.png"),
+        ("constant", ["unique", "a b", "a_b"], "{document}", "card.png"),
+        ("case", ["unique", "Card", "card"], "{name}", "card.png"),
+    ] {
+        let mut s = visibility_export_session(&names);
+        let before = s.active().unwrap().doc.clone();
+        let revision = s.active().unwrap().revision;
+        let history = s.active().unwrap().history.entries();
+        let past = s.active().unwrap().history.past_len();
+        let redo = s.active().unwrap().history.can_redo();
+        let dir = tmp(&format!("collision-{label}"));
+        let first = format!("{dir}/unique.png");
+        let collided = format!("{dir}/{collision}");
+        std::fs::write(&first, b"keep earlier export").unwrap();
+        std::fs::write(&collided, b"keep colliding export").unwrap();
+        let err = s.execute("file.export.dataSetsAsFiles", json!({"dir": dir, "naming": naming})).unwrap_err();
+        assert!(matches!(err, EngineError::BadParams { .. }), "{err}");
+        let message = err.to_string();
+        assert!(message.contains(collision) && message.contains("{index}"), "{message}");
+        assert_eq!(std::fs::read(&first).unwrap(), b"keep earlier export");
+        assert_eq!(std::fs::read(&collided).unwrap(), b"keep colliding export");
+        assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 2);
+        let absent = format!("{dir}/not-created");
+        assert!(s.execute("file.export.dataSetsAsFiles", json!({"dir": absent, "naming": naming})).is_err());
+        assert!(!std::path::Path::new(&absent).exists());
+        let after = s.active().unwrap();
+        assert!(Arc::ptr_eq(&before, &after.doc));
+        assert_eq!(after.revision, revision);
+        assert_eq!(after.history.entries(), history);
+        assert_eq!(after.history.past_len(), past);
+        assert_eq!(after.history.can_redo(), redo);
+        std::fs::remove_dir_all(dir).unwrap();
+    }
+}
+
+#[test]
+fn export_data_sets_unique_paths_preserve_variations_and_document() {
+    for (label, names, naming, selected, expected) in [
+        ("indexed", ["a b", "a_b"], "card-{index}", None, vec!["card-001.png", "card-002.png"]),
+        ("distinct", ["a-b", "a_b"], "{name}", None, vec!["a-b.png", "a_b.png"]),
+        ("subset", ["a b", "a_b"], "{name}", Some(vec!["a b"]), vec!["a_b.png"]),
+    ] {
+        let mut s = visibility_export_session(&names);
+        let before = s.active().unwrap().doc.clone();
+        let revision = s.active().unwrap().revision;
+        let history = s.active().unwrap().history.entries();
+        let past = s.active().unwrap().history.past_len();
+        let redo = s.active().unwrap().history.can_redo();
+        let dir = tmp(&format!("unique-{label}"));
+        let result = s.execute("file.export.dataSetsAsFiles", json!({"dir": dir, "naming": naming, "dataSets": selected})).unwrap();
+        let paths: Vec<_> = expected.iter().map(|name| format!("{dir}/{name}")).collect();
+        assert_eq!(result, json!({"files": paths, "count": paths.len()}));
+        assert_eq!(std::fs::read_dir(&dir).unwrap().count(), paths.len());
+        for (i, path) in paths.iter().enumerate() {
+            let exported = photocraft_io::import(path, &std::fs::read(path).unwrap()).unwrap().document;
+            let pixel = photocraft_compose::render(&exported, Rect::new(0, 0, 1, 1)).px[0];
+            assert_eq!(pixel, if i == 0 { [1.0; 4] } else { [0.0; 4] });
+        }
+        let after = s.active().unwrap();
+        assert!(Arc::ptr_eq(&before, &after.doc));
+        assert_eq!(after.revision, revision);
+        assert_eq!(after.history.entries(), history);
+        assert_eq!(after.history.past_len(), past);
+        assert_eq!(after.history.can_redo(), redo);
+        std::fs::remove_dir_all(dir).unwrap();
+    }
+}
+
 #[test]
 fn pixel_replacement_changes_the_layer() {
     let (mut s, photo, _b, _t) = session();