Preserve multiline fields and accept a BOM in parameter CSV imports (#9732) (#9772)

* claim: #9732

* fix(params): preserve quoted CSV records and BOM headers (#9732)

crates/doc/src/params.rs

@@ -462,7 +462,7 @@ impl Document {
 
     /// User parameters as CSV (`name,unit,expression,value,comment`).
     pub fn params_csv(&self) -> String {
-        let q = |s: &str| if s.contains([',', '"', '\n']) { format!("\"{}\"", s.replace('"', "\"\"")) } else { s.to_string() };
+        let q = |s: &str| if s.contains([',', '"', '\r', '\n']) { format!("\"{}\"", s.replace('"', "\"\"")) } else { s.to_string() };
         let mut out = String::from("name,unit,expression,value,comment,favorite\n");
         for r in self.param_rows().into_iter().filter(|r| r.source == "user") {
             out += &format!(
@@ -497,23 +497,25 @@ impl Document {
                 })
                 .collect::<Result<_>>()?
         } else {
-            let mut lines = text.lines();
-            let header: Vec<String> = parse_csv_line(lines.next().unwrap_or_default()).into_iter().map(|h| h.trim().to_ascii_lowercase()).collect();
-            let col = |n: &str| header.iter().position(|h| h == n);
+            let mut records = split_csv(text.strip_prefix('\u{feff}').unwrap_or(text), '\n');
+            let header = csv_fields(records.next().unwrap_or_default())?;
+            let col = |n: &str| header.iter().position(|h| h.trim().eq_ignore_ascii_case(n));
             let (Some(ni), Some(ei)) = (col("name"), col("expression").or(col("value"))) else {
                 return Err(DocError::Invalid("the CSV needs `name` and `expression` columns".into()));
             };
             let (ui, ci) = (col("unit"), col("comment"));
-            lines
-                .filter(|l| !l.trim().is_empty())
-                .map(|l| {
-                    let c = parse_csv_line(l);
+            records
+                .filter(|record| !record.trim().is_empty())
+                .take(MAX_PARAMS + 1)
+                .map(|record| {
+                    let c = csv_fields(record)?;
                     let get = |i: Option<usize>| i.and_then(|i| c.get(i)).map(|s| s.trim().to_string());
-                    (get(Some(ni)).unwrap_or_default(), get(Some(ei)).unwrap_or_default(), get(ui), get(ci))
+                    let comment = ci.and_then(|i| c.get(i)).cloned();
+                    Ok((get(Some(ni)).unwrap_or_default(), get(Some(ei)).unwrap_or_default(), get(ui), comment))
                 })
-                .collect()
+                .collect::<Result<_>>()?
         };
-        if rows.len() > 10_000 {
+        if rows.len() > MAX_PARAMS {
             return Err(DocError::Invalid("too many parameters".into()));
         }
         // Each row is checked and applied as `change_param` would, but the parameters are
@@ -1069,25 +1071,32 @@ impl Document {
     }
 }
 
-/// One CSV line with quoted fields.
-fn parse_csv_line(l: &str) -> Vec<String> {
-    let mut out = Vec::new();
-    let mut cur = String::new();
+/// Split records or fields only outside quotes. A doubled quote toggles twice
+/// before the next separator, leaving it inside the same field.
+fn split_csv(text: &str, separator: char) -> impl Iterator<Item = &str> {
     let mut quoted = false;
-    let mut chars = l.chars().peekable();
-    while let Some(c) = chars.next() {
-        match (c, quoted) {
-            ('"', true) if chars.peek() == Some(&'"') => {
-                cur.push('"');
-                chars.next();
-            }
-            ('"', _) => quoted = !quoted,
-            (',', false) => out.push(std::mem::take(&mut cur)),
-            (c, _) => cur.push(c),
+    text.split(move |c| {
+        if c == '"' {
+            quoted = !quoted;
         }
-    }
-    out.push(cur);
-    out
+        c == separator && !quoted
+    })
+}
+
+fn csv_fields(record: &str) -> Result<Vec<String>> {
+    split_csv(record, ',')
+        .map(|raw| {
+            let field = raw.trim();
+            if !field.contains('"') {
+                return Ok(field.to_string());
+            }
+            let inner = field.strip_prefix('"').and_then(|s| s.strip_suffix('"'));
+            match inner {
+                Some(s) if s.split("\"\"").all(|part| !part.contains('"')) => Ok(s.replace("\"\"", "\"")),
+                _ => Err(DocError::Invalid("invalid quoting in parameter CSV".into())),
+            }
+        })
+        .collect()
 }
 
 /// Parameters whose definitions depend on each other in a loop.

crates/engine/src/cmd/parameters_tests.rs

@@ -326,6 +326,43 @@ fn csv_and_json_round_trip() {
     let _ = std::fs::remove_dir_all(&dir);
 }
 
+#[test]
+fn csv_round_trips_comments_with_line_breaks() {
+    for comment in ["line one\nline two", "\r\nfirst, \"quoted\"\r\n\r\nlast\r\n", "one\rtwo"] {
+        let mut s = Session::default();
+        run(&mut s, "parameters.add", json!({"name": "L", "expression": "30", "comment": comment}));
+        run(&mut s, "parameters.add", json!({"name": "W", "expression": "10"}));
+        let exported = run(&mut s, "parameters.export", json!({"format": "csv"}));
+
+        let mut fresh = Session::default();
+        let imported = run(&mut fresh, "parameters.import", json!({"text": exported["text"]}));
+        assert_eq!(imported["imported"], json!(["L", "W"]));
+        let list = run(&mut fresh, "parameters.list", json!({}));
+        assert_eq!(row(&list, "L")["comment"], comment);
+        assert_eq!(row(&list, "L")["value"], 30.0);
+        assert_eq!(row(&list, "W")["value"], 10.0);
+    }
+}
+
+#[test]
+fn csv_import_accepts_a_bom_before_the_header() {
+    let mut s = Session::default();
+    let csv = "\u{feff}\"NAME\",expression\r\n\r\nA,5";
+    let imported = run(&mut s, "parameters.import", json!({"text": csv}));
+    assert_eq!(imported["imported"], json!(["A"]));
+    assert_eq!(row(&run(&mut s, "parameters.list", json!({})), "A")["value"], 5.0);
+}
+
+#[test]
+fn csv_import_with_an_unclosed_quote_leaves_parameters_alone() {
+    let mut s = Session::default();
+    run(&mut s, "parameters.add", json!({"name": "L", "expression": "10"}));
+    let before = s.doc.to_json();
+    let csv = "name,expression,comment\nL,30,updated\nW,5,\"unfinished\n";
+    assert!(s.execute("parameters.import", &json!({"text": csv})).is_err());
+    assert_eq!(s.doc.to_json(), before);
+}
+
 /// #8960: the text marker accepts ASCII case variants without changing numeric units.
 #[test]
 fn text_parameter_imports_accept_mixed_case_units() {