Variables: Import Data Sets reads CSV files that are not UTF-8 as Windows-1252

Squashed from #5116 onto main.

crates/engine/src/cmd/place.rs

@@ -36,7 +36,7 @@ mod doc;
 pub(crate) mod document;
 pub(crate) mod docx;
 mod rtf;
-mod text;
+pub(super) mod text;
 
 use std::sync::Arc;
 

crates/engine/src/cmd/place/text.rs

@@ -96,7 +96,7 @@ fn decode(bytes: &[u8], o: TextOptions) -> String {
 }
 
 /// Byte `b` of Windows-1252.
-pub(super) fn windows_1252(b: u8) -> char {
+pub(in crate::cmd) fn windows_1252(b: u8) -> char {
     const HIGH: [char; 32] = [
         '€', '\u{81}', '‚', 'ƒ', '„', '…', '†', '‡', 'ˆ', '‰', 'Š', '‹', 'Œ', '\u{8D}', 'Ž', '\u{8F}', //
         '\u{90}', '‘', '’', '“', '”', '•', '–', '—', '˜', '™', 'š', '›', 'œ', '\u{9D}', 'ž', 'Ÿ',

crates/engine/src/cmd/variablecmds.rs

@@ -799,7 +799,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Import Data Sets…",
             ["Window", "Variables"],
             None,
-            "{path? | data? | dataBase64? | csv?} load datasets from CSV, comma- or tab-separated (header: name, then variable names; one row per dataset; without the name column every header is a variable and the rows take the first free names Data Set 1, 2…). A header that names no variable defines one, unbound: `#Name` defines a visibility variable, `@Name` defines an image variable, and a header without a prefix defines a text variable. A prefixed header whose name is a variable of another kind, and a `%` header that isn't a variable's exact name (graph data files), are refused, and so are two headers that name one variable and a header that would define a variable while the variables are locked. Rows whose name already exists are replaced → {imported, updated, names, defined}",
+            "{path? | data? | dataBase64? | csv?} load datasets from CSV, comma- or tab-separated, in UTF-16 with a byte-order mark, UTF-8 or else Windows-1252 (header: name, then variable names; one row per dataset; without the name column every header is a variable and the rows take the first free names Data Set 1, 2…). A header that names no variable defines one, unbound: `#Name` defines a visibility variable, `@Name` defines an image variable, and a header without a prefix defines a text variable. A prefixed header whose name is a variable of another kind, and a `%` header that isn't a variable's exact name (graph data files), are refused, and so are two headers that name one variable and a header that would define a variable while the variables are locked. Rows whose name already exists are replaced → {imported, updated, names, defined}",
             has_doc,
             dataset_import
         ),

crates/engine/src/cmd/variablecmds/csv.rs

@@ -2,6 +2,7 @@ use super::{
     BTreeMap, BTreeSet, DataSet, DataValue, EngineError, MAX_NAME, Result, Session, Value, Variable, VariableKind, Variables, absolute_path, bad,
     json, read_file_head, str_param, variables_unlocked, write_file,
 };
+use crate::cmd::place::text::windows_1252;
 
 /// Cap on a graph variable's CSV (MCP and files are untrusted).
 pub(super) const MAX_GRAPH_CSV: usize = 256 * 1024;
@@ -13,20 +14,45 @@ const MAX_CSV_ROWS: usize = 1_000;
 
 const MAX_CSV_COLS: usize = 256;
 
-/// Bytes of a datasets CSV from `path`, `data`, `dataBase64` or `csv`.
-fn datasets_csv_bytes(p: &Value, cmd: &str) -> Result<Vec<u8>> {
-    let bytes = match (str_param(p, "path"), str_param(p, "data"), str_param(p, "dataBase64"), str_param(p, "csv")) {
+/// Bytes of a datasets CSV from `path`, `data`, `dataBase64` or `csv`, and whether they came
+/// as text (`data` or `csv`).
+fn datasets_csv_bytes(p: &Value, cmd: &str) -> Result<(Vec<u8>, bool)> {
+    let (bytes, text) = match (str_param(p, "path"), str_param(p, "data"), str_param(p, "dataBase64"), str_param(p, "csv")) {
         // One byte past the cap is enough to refuse the file below.
-        (Some(path), ..) => read_file_head(&absolute_path(path), MAX_CSV_BYTES + 1)?,
-        (None, Some(text), ..) => text.as_bytes().to_vec(),
-        (None, None, Some(b64), _) => vectorcraft_format::base64_decode(b64).ok_or_else(|| bad(cmd, "bad dataBase64"))?,
-        (None, None, None, Some(csv)) => csv.as_bytes().to_vec(),
+        (Some(path), ..) => (read_file_head(&absolute_path(path), MAX_CSV_BYTES + 1)?, false),
+        (None, Some(text), ..) => (text.as_bytes().to_vec(), true),
+        (None, None, Some(b64), _) => (vectorcraft_format::base64_decode(b64).ok_or_else(|| bad(cmd, "bad dataBase64"))?, false),
+        (None, None, None, Some(csv)) => (csv.as_bytes().to_vec(), true),
         _ => return Err(bad(cmd, "give `path`, `data`, `dataBase64` or `csv`")),
     };
     if bytes.len() > MAX_CSV_BYTES {
         return Err(bad(cmd, format!("CSV is larger than {MAX_CSV_BYTES} bytes")));
     }
-    Ok(bytes)
+    Ok((bytes, text))
+}
+
+/// The text of a datasets CSV given as a file or as `dataBase64`: UTF-16 after a UTF-16
+/// byte-order mark, else UTF-8, with or without a byte-order mark, else Windows-1252. A file
+/// that starts with a byte-order mark must be valid in its encoding, and a file without one must
+/// not hold NUL bytes (UTF-16 without a byte-order mark).
+fn datasets_csv_text(bytes: &[u8], cmd: &str) -> Result<String> {
+    let utf16 = |rest: &[u8], le: bool| {
+        let (units, odd) = rest.as_chunks::<2>();
+        let units = units.iter().map(|&c| if le { u16::from_le_bytes(c) } else { u16::from_be_bytes(c) });
+        let text = char::decode_utf16(units).collect::<std::result::Result<String, _>>();
+        text.ok().filter(|_| odd.is_empty()).ok_or_else(|| bad(cmd, "CSV is not UTF-16"))
+    };
+    match bytes {
+        [0xFF, 0xFE, rest @ ..] => return utf16(rest, true),
+        [0xFE, 0xFF, rest @ ..] => return utf16(rest, false),
+        _ if bytes.contains(&0) => return Err(bad(cmd, "CSV holds NUL bytes: save it as UTF-8, or as UTF-16 with a byte-order mark")),
+        _ => {}
+    }
+    match std::str::from_utf8(bytes) {
+        Ok(text) => Ok(text.to_string()),
+        Err(_) if bytes.starts_with(b"\xEF\xBB\xBF") => Err(bad(cmd, "CSV is not UTF-8")),
+        Err(_) => Ok(bytes.iter().map(|&b| windows_1252(b)).collect()),
+    }
 }
 
 /// A row of a datasets CSV: the file line it starts on, its cells, and for each cell whether it was quoted.
@@ -310,9 +336,10 @@ fn header_variable(
 /// name are replaced; new names are appended. One undo step.
 pub(super) fn dataset_import(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "dataset.import";
-    let bytes = datasets_csv_bytes(p, C)?;
-    let text = std::str::from_utf8(&bytes).map_err(|_| bad(C, "CSV is not UTF-8"))?;
-    let rows = parse_datasets_csv(text, C)?;
+    let (bytes, given_as_text) = datasets_csv_bytes(p, C)?;
+    // `data` and `csv` are already text, NUL characters included (`dataset.export` writes them).
+    let text = if given_as_text { String::from_utf8(bytes).map_err(|_| bad(C, "CSV is not UTF-8"))? } else { datasets_csv_text(&bytes, C)? };
+    let rows = parse_datasets_csv(&text, C)?;
     let Some((_, header, _)) = rows.first() else {
         return Err(bad(C, "CSV is empty"));
     };

crates/engine/src/tests/variables.rs

@@ -1729,6 +1729,62 @@ fn datasets_csv_round_trips_and_rejects_bad_input() {
     s.execute("edit.undo", &json!({})).unwrap();
 }
 
+/// A CSV that isn't UTF-8 is read as Windows-1252. UTF-8 files, with or without a byte-order mark,
+/// read as before, and a file that starts with the UTF-8 byte-order mark but isn't UTF-8 is refused.
+#[test]
+fn datasets_csv_reads_windows_1252_files() {
+    let mut s = session();
+    s.execute("variable.define", &json!({"name": "City", "kind": "text"})).unwrap();
+    let import = |s: &mut Session, bytes: &[u8]| s.execute("dataset.import", &json!({"dataBase64": vectorcraft_format::base64_encode(bytes)}));
+    let value = |s: &Session, name: &str| s.doc().unwrap().doc.variables.dataset(name).and_then(|d| d.values.get("City")).cloned();
+    let text_value = |v: &str| Some(vectorcraft_doc::DataValue::Text(v.into()));
+    let imp = import(&mut s, b"name,City\r\nA,Jos\xe9\r\nB,\x80 5 \x96 \x93Caf\xe9\x94\r\n").unwrap();
+    assert_eq!(imp["names"], json!(["A", "B"]));
+    assert_eq!(value(&s, "A"), text_value("José"));
+    assert_eq!(value(&s, "B"), text_value("€ 5 – “Café”"));
+    import(&mut s, "name,City\nC,José\n".as_bytes()).unwrap();
+    assert_eq!(value(&s, "C"), text_value("José"));
+    import(&mut s, "\u{feff}name,City\nD,Zoë\n".as_bytes()).unwrap();
+    assert_eq!(value(&s, "D"), text_value("Zoë"));
+    let e = import(&mut s, b"\xef\xbb\xbfname,City\nE,Jos\xe9\n").unwrap_err().to_string();
+    assert!(e.contains("CSV is not UTF-8"), "{e}");
+    assert_eq!(value(&s, "E"), None);
+}
+
+/// A CSV that starts with a UTF-16 byte-order mark is read as UTF-16, and
+/// one that isn't valid UTF-16 is refused. A file with NUL bytes and no byte-order mark is refused.
+/// A text value holding NUL imports back from the `csv` that `dataset.export` returns.
+#[test]
+fn datasets_csv_reads_utf16_files() {
+    let mut s = session();
+    s.execute("variable.define", &json!({"name": "City", "kind": "text"})).unwrap();
+    let import = |s: &mut Session, bytes: &[u8]| s.execute("dataset.import", &json!({"dataBase64": vectorcraft_format::base64_encode(bytes)}));
+    let value = |s: &Session, name: &str| s.doc().unwrap().doc.variables.dataset(name).and_then(|d| d.values.get("City")).cloned();
+    let text_value = |v: &str| Some(vectorcraft_doc::DataValue::Text(v.into()));
+    let utf16 = |bom: &[u8], text: &str, le: bool| -> Vec<u8> {
+        let units = text.encode_utf16().flat_map(|u| if le { u.to_le_bytes() } else { u.to_be_bytes() });
+        bom.iter().copied().chain(units).collect()
+    };
+    import(&mut s, &utf16(b"\xff\xfe", "name\tCity\r\nA\tZoë 😀\r\n", true)).unwrap();
+    assert_eq!(value(&s, "A"), text_value("Zoë 😀"));
+    import(&mut s, &utf16(b"\xfe\xff", "name,City\nB,José\n", false)).unwrap();
+    assert_eq!(value(&s, "B"), text_value("José"));
+    let mut odd = utf16(b"\xff\xfe", "name,City\nC,x\n", true);
+    odd.push(b'y');
+    let e = import(&mut s, &odd).unwrap_err().to_string();
+    assert!(e.contains("CSV is not UTF-16"), "{e}");
+    let e = import(&mut s, &[b"\xff\xfe".as_slice(), "name,City\nD,".as_bytes(), b"\x00\xd8"].concat()).unwrap_err().to_string();
+    assert!(e.contains("CSV is not UTF-16"), "{e}");
+    let e = import(&mut s, &utf16(b"", "name,City\nE,x\n", true)).unwrap_err().to_string();
+    assert!(e.contains("CSV holds NUL bytes"), "{e}");
+    assert_eq!((value(&s, "C"), value(&s, "D"), value(&s, "E")), (None, None, None));
+    s.execute("dataset.new", &json!({"name": "F", "values": {"City": "a\u{0}b"}})).unwrap();
+    let csv = s.execute("dataset.export", &json!({})).unwrap()["csv"].as_str().unwrap().to_string();
+    s.execute("dataset.delete", &json!({"names": ["F"]})).unwrap();
+    s.execute("dataset.import", &json!({"csv": csv})).unwrap();
+    assert_eq!(value(&s, "F"), text_value("a\u{0}b"));
+}
+
 /// A data set of [`library`]: its name, and each variable's name with its paragraphs.
 type LibrarySet<'a> = (&'a str, &'a [(&'a str, &'a [&'a str])]);
 

docs/mcp.md

@@ -1658,6 +1658,9 @@ already walks Image Trace. Short agent entry points:
   data files) is refused, and so are two cells that name one variable (`City` twice, or `Shown` and `#Shown`). The
   reply's `defined` lists the variables the import defined. While the variables are
   locked, an import whose header would define a variable fails with "variables are locked".
+  A `dataset.import` file that isn't UTF-8 is read as Windows-1252, unless it starts with the UTF-8 byte-order mark.
+  A file that starts with a UTF-16 byte-order mark is read as UTF-16, and a file without one that holds NUL bytes is
+  refused. `csv` and `data` are read as the text they hold.
 - Variable libraries: `variable.loadLibrary {path|xml|dataBase64}` (the Variables panel menu's Load Variable
   Library…) replaces the document's variables and data sets with a variable library's in one undo step. A load, like a
   `dataset.import`, adds no undo step when it leaves the variables and data sets as they were. The library