Multipath - DEL - order by array index even if path lengths are different

oshadmi · oshadmi · commit ce2371727ee8 · 2021-11-14T01:46:08.000+02:00
diff --git a/src/commands.rs b/src/commands.rs
@@ -16,7 +16,7 @@ use crate::redisjson::SetOptions;
 use serde_json::{Number, Value};
 
 use itertools::FoldWhile::{Continue, Done};
-use itertools::Itertools;
+use itertools::{EitherOrBoth, Itertools};
 use serde::{Serialize, Serializer};
 use std::collections::HashMap;
 
@@ -758,40 +758,43 @@ where
         .collect::<Vec<Value>>()
 }
 
-/// Sort the paths so higher indices precede lower indices on the same on the same array
-/// And objects with higher hierarchy (closer to the top-level) preceded objects with deeper hierarchy
+/// Sort the paths so higher indices precede lower indices on the same array,
+/// And if one path is a sub-path of the other, then paths with shallower hierarchy (closer to the top-level) precedes paths with deeper hierarchy
 fn prepare_paths_for_deletion(paths: &mut Vec<Vec<String>>) {
-    paths.sort_by(|v1, v2| match (v1.len(), v2.len()) {
-        (l1, l2) if l1 < l2 => Ordering::Less, // Shorter paths before longer paths
-        (l1, l2) if l1 > l2 => Ordering::Greater, // Shorter paths before longer paths
-        _ => v1
-            .iter()
-            .zip(v2.iter())
-            .fold_while(Ordering::Equal, |_acc, (p1, p2)| {
-                let i1 = p1.parse::<usize>();
-                let i2 = p2.parse::<usize>();
-                match (i1, i2) {
-                    (Err(_), Err(_)) => match p1.cmp(p2) {
-                        // String compare
-                        Ordering::Less => Done(Ordering::Less),
-                        Ordering::Equal => Continue(Ordering::Equal),
-                        Ordering::Greater => Done(Ordering::Greater),
-                    },
-                    (Ok(_), Err(_)) => Done(Ordering::Greater), //String before Numeric
-                    (Err(_), Ok(_)) => Done(Ordering::Less),    //String before Numeric
-                    (Ok(i1), Ok(i2)) => {
-                        // Numeric compare - higher indices before lower ones
-                        if i1 < i2 {
-                            Done(Ordering::Greater)
-                        } else if i2 < i1 {
-                            Done(Ordering::Less)
-                        } else {
-                            Continue(Ordering::Equal)
+    paths.sort_by(|v1, v2| {
+        v1.iter()
+            .zip_longest(v2.iter())
+            .fold_while(Ordering::Equal, |_acc, v| {
+                match v {
+                    EitherOrBoth::Left(_) => Done(Ordering::Greater), // Shorter paths before longer paths
+                    EitherOrBoth::Right(_) => Done(Ordering::Less), // Shorter paths before longer paths
+                    EitherOrBoth::Both(p1, p2) => {
+                        let i1 = p1.parse::<usize>();
+                        let i2 = p2.parse::<usize>();
+                        match (i1, i2) {
+                            (Err(_), Err(_)) => match p1.cmp(p2) {
+                                // String compare
+                                Ordering::Less => Done(Ordering::Less),
+                                Ordering::Equal => Continue(Ordering::Equal),
+                                Ordering::Greater => Done(Ordering::Greater),
+                            },
+                            (Ok(_), Err(_)) => Done(Ordering::Greater), //String before Numeric
+                            (Err(_), Ok(_)) => Done(Ordering::Less),    //String before Numeric
+                            (Ok(i1), Ok(i2)) => {
+                                // Numeric compare - higher indices before lower ones
+                                if i1 < i2 {
+                                    Done(Ordering::Greater)
+                                } else if i2 < i1 {
+                                    Done(Ordering::Less)
+                                } else {
+                                    Continue(Ordering::Equal)
+                                }
+                            }
                         }
                     }
                 }
             })
-            .into_inner(),
+            .into_inner()
     });
 }
 
diff --git a/tests/pytest/test_multi.py b/tests/pytest/test_multi.py
@@ -66,7 +66,7 @@ def testDelCommand(env):
     r.assertEqual(res, 3)
 
     r.assertOk(r.execute_command('JSON.SET', 'doc2', '$', '[1, 2, 3]'))
-    res = r.execute_command('JSON.DEL', 'doc2', '$[2,1,0]')
+    res = r.execute_command('JSON.DEL', 'doc2', '$[1,2,0]')
     r.assertEqual(res, 3)
 
     r.assertOk(r.execute_command('JSON.SET', 'doc2', '$', '{"b": [1,2,3], "a": {"b": [1, 2, 3], "c": [1, 2, 3]}, "x": {"b": [1, 2, 3], "c": [1, 2, 3]}}'))
@@ -88,6 +88,14 @@ def testDelCommand(env):
     res = r.execute_command('JSON.GET', 'doc2', '$')
     r.assertEqual(json.loads(res), [[True, {"answer": 42}]])
 
+def testDelCommand_issue529(env):
+    r = env
+    r.assertOk(r.execute_command('JSON.SET', 'doc1', '$', '[{"a00": [{"a00": "a00_00"}, {"a01": "a00_01"}, {"a02": "a00_02"}, {"a03": "a00_03"}]}, {"a01": [{"a00": "a01_00"}, {"a01": "a01_01"}, {"a02": "a01_02"}, {"a03": "a01_03"}]}, {"a02": [{"a00": "a02_00"}, {"a01": "a02_01"}, {"a02": "a02_02"}, {"a03": "a02_03"}]}, {"a03": [{"a00": "a03_00"}, {"a01": "a03_01"}, {"a02": "a03_02"}, {"a03": "a03_03"}]}]'))
+    res = r.execute_command('JSON.DEL', 'doc1', '$..[2]')
+    r.assertEqual(res, 4)
+    res = r.execute_command('JSON.ARRLEN', 'doc1', '$.*[*]')
+    r.assertEqual(res, [3, 3, 3])
+
 
 def testForgetCommand(env):
     """Test REJSON.FORGET command"""
