Merge pull request #1787 from Silvertree2010/fix/h5159-variable-movemode-silently-turns

Reject negative and fractional index in variable.moveMode

crates/engine/src/cmd/variables.rs

@@ -12,7 +12,7 @@ use serde_json::{Value, json};
 
 use super::lists::BoundList;
 use super::{CommandSpec, ok};
-use crate::params::{boolean, get, num, req_string, string};
+use crate::params::{boolean, get, req_index, req_string, string};
 use crate::{Result, Session, bad, spec};
 
 pub fn specs() -> Vec<CommandSpec> {
@@ -258,13 +258,13 @@ fn remove_mode(s: &mut Session, p: &Value) -> Result<Value> {
 
 fn move_mode(s: &mut Session, p: &Value) -> Result<Value> {
     let cmd = "variable.moveMode";
-    let to = num(p, "to").filter(|x| x.is_finite()).ok_or_else(|| bad(cmd, "`to` must be a number"))?;
+    let to = req_index(cmd, p, "to")?;
     let doc = s.doc_mut()?;
     let (ci, mode) = mode_of(doc, cmd, p)?;
     let coll = doc.variables.collections.get_mut(ci).ok_or_else(|| bad(cmd, "no such collection"))?;
     let from = coll.modes.iter().position(|m| m.id == mode).ok_or_else(|| bad(cmd, "no such mode"))?;
     let m = coll.modes.remove(from);
-    let at = (to.max(0.0) as usize).min(coll.modes.len());
+    let at = to.min(coll.modes.len());
     coll.modes.insert(at, m);
     ok()
 }

crates/engine/src/variables_behaviour_tests.rs

@@ -68,6 +68,20 @@ fn types_and_their_starting_values() {
     assert!(s.execute("variable.create", &json!({"collection": "C", "name": "y", "type": "number", "value": "wide"})).is_err());
 }
 
+#[test]
+fn move_mode_rejects_negative_and_fractional_indexes() {
+    let mut s = Session::new();
+    theme(&mut s);
+    run(&mut s, "variable.addMode", json!({"collection": "Theme", "name": "Dim"}));
+    let order =
+        |s: &Session| -> Vec<String> { s.doc().unwrap().variables.collection("Theme").unwrap().modes.iter().map(|m| m.name.clone()).collect() };
+    let before = order(&s);
+    for to in [json!(-1), json!(1.9)] {
+        assert!(s.execute("variable.moveMode", &json!({"collection": "Theme", "mode": "Dim", "to": to})).is_err(), "{to}");
+        assert_eq!(order(&s), before, "{to}");
+    }
+}
+
 #[test]
 fn modes_new_duplicated_reordered_and_the_default() {
     // Documented: a new mode starts with the default mode's values and a duplicated one with its