Skip to content

Commit be9d51c

Browse files
oshadmioshadmi
committed
Merge remote-tracking branch 'origin/master' into omer_gitsha
2 parents efc4d97 + e6da70b commit be9d51c

File tree

5 files changed

+151
-60
lines changed

5 files changed

+151
-60
lines changed

‎Cargo.lock

+16
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ serde = "1.0"
1616
libc = "0.2"
1717
jsonpath_lib = { git = "https://github.com/RedisJSON/jsonpath.git", branch = "generic_json_path" }
1818
redis-module = { version="0.24", features = ["experimental-api"]}
19+
itertools = "0.10.1"
1920
[features]
2021
# Workaround to allow cfg(feature = "test") in redismodue-rs dependencies:
2122
# https://github.com/RedisLabsModules/redismodule-rs/pull/68

‎src/commands.rs

+108-54
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ use crate::redisjson::{normalize_arr_indices, Format, Path};
44
use jsonpath_lib::select::select_value::{SelectValue, SelectValueType};
55
use redis_module::{Context, RedisValue};
66
use redis_module::{NextArg, RedisError, RedisResult, RedisString, REDIS_OK};
7+
use std::cmp::Ordering;
78

89
use jsonpath_lib::select::Selector;
910

@@ -15,8 +16,11 @@ use crate::redisjson::SetOptions;
1516

1617
use serde_json::{Number, Value};
1718

18-
use serde::Serialize;
19+
use itertools::FoldWhile::{Continue, Done};
20+
use itertools::Itertools;
21+
use serde::{Serialize, Serializer};
1922
use std::collections::HashMap;
23+
2024
const JSON_ROOT_PATH: &str = "$";
2125
const JSON_ROOT_PATH_LEGACY: &str = ".";
2226
const CMD_ARG_NOESCAPE: &str = "NOESCAPE";
@@ -53,6 +57,23 @@ const JSONGET_SUBCOMMANDS_MAXSTRLEN: usize = max_strlen(&[
5357
CMD_ARG_FORMAT,
5458
]);
5559

60+
enum Values<'a, V: SelectValue> {
61+
Single(&'a V),
62+
Multi(Vec<&'a V>),
63+
}
64+
65+
impl<'a, V: SelectValue> Serialize for Values<'a, V> {
66+
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
67+
where
68+
S: Serializer,
69+
{
70+
match self {
71+
Values::Single(v) => v.serialize(serializer),
72+
Values::Multi(v) => v.serialize(serializer),
73+
}
74+
}
75+
}
76+
5677
pub struct KeyValue<'a, V: SelectValue> {
5778
val: &'a V,
5879
}
@@ -151,71 +172,55 @@ impl<'a, V: SelectValue> KeyValue<'a, V> {
151172
String::from_utf8(out.into_inner()).unwrap()
152173
}
153174

154-
fn to_json_legacy(
175+
fn to_json_multi(
155176
&'a self,
156177
paths: &mut Vec<Path>,
157178
indent: Option<&str>,
158179
newline: Option<&str>,
159180
space: Option<&str>,
181+
is_legacy: bool,
160182
) -> Result<RedisValue, Error> {
161-
if paths.len() > 1 {
162-
// TODO: Creating a temp doc here duplicates memory usage. This can be very memory inefficient.
163-
// A better way would be to create a doc of references to the original doc but no current support
164-
// in serde_json. I'm going for this implementation anyway because serde_json isn't supposed to be
165-
// memory efficient and we're using it anyway. See https://github.com/serde-rs/json/issues/635.
166-
let mut missing_path = None;
167-
let temp_doc = paths.drain(..).fold(HashMap::new(), |mut acc, path| {
168-
let mut selector = Selector::new();
169-
selector.value(self.val);
170-
if selector.str_path(path.get_path()).is_err() {
171-
return acc;
172-
}
173-
let value = match selector.select() {
174-
Ok(s) => s.first().copied(),
175-
Err(_) => None,
176-
};
177-
if value.is_none() && missing_path.is_none() {
178-
missing_path = Some(path.get_original().to_string());
179-
}
180-
acc.insert(path.get_original(), value);
181-
acc
182-
});
183-
if let Some(p) = missing_path {
184-
return Err(format!("ERR path {} does not exist", p).into());
183+
// TODO: Creating a temp doc here duplicates memory usage. This can be very memory inefficient.
184+
// A better way would be to create a doc of references to the original doc but no current support
185+
// in serde_json. I'm going for this implementation anyway because serde_json isn't supposed to be
186+
// memory efficient and we're using it anyway. See https://github.com/serde-rs/json/issues/635.
187+
let mut missing_path = None;
188+
let temp_doc = paths.drain(..).fold(HashMap::new(), |mut acc, path: Path| {
189+
let mut selector = Selector::new();
190+
selector.value(self.val);
191+
if selector.str_path(path.get_path()).is_err() {
192+
return acc;
185193
}
194+
let value = match selector.select() {
195+
Ok(s) if is_legacy && !s.is_empty() => Some(Values::Single(s[0])),
196+
Ok(s) if !is_legacy => Some(Values::Multi(s)),
197+
_ => None,
198+
};
186199

187-
Ok(Self::serialize_object(&temp_doc, indent, newline, space).into())
188-
} else {
189-
Ok(
190-
Self::serialize_object(
191-
self.get_first(paths[0].get_path())?,
192-
indent,
193-
newline,
194-
space,
195-
)
196-
.into(),
197-
)
200+
if value.is_none() && missing_path.is_none() {
201+
missing_path = Some(path.get_original().to_string());
202+
}
203+
acc.insert(path.get_original(), value);
204+
acc
205+
});
206+
if let Some(p) = missing_path {
207+
return Err(format!("ERR path {} does not exist", p).into());
198208
}
209+
Ok(Self::serialize_object(&temp_doc, indent, newline, space).into())
199210
}
200211

201-
fn to_json_multi(
212+
fn to_json_single(
202213
&'a self,
203-
paths: &Vec<Path>,
214+
path: &str,
204215
indent: Option<&str>,
205216
newline: Option<&str>,
206217
space: Option<&str>,
218+
is_legacy: bool,
207219
) -> Result<RedisValue, Error> {
208-
if paths.len() > 1 {
209-
let mut res: Vec<Vec<&V>> = vec![];
210-
for path in paths {
211-
let values = self.get_values(path.get_path())?;
212-
res.push(values);
213-
}
214-
Ok(Self::serialize_object(&res, indent, newline, space).into())
220+
if is_legacy {
221+
Ok(self.to_string_single(path, indent, newline, space)?.into())
215222
} else {
216-
Ok(self
217-
.to_string_multi(paths[0].get_path(), indent, newline, space)?
218-
.into())
223+
Ok(self.to_string_multi(path, indent, newline, space)?.into())
219224
}
220225
}
221226

@@ -231,10 +236,10 @@ impl<'a, V: SelectValue> KeyValue<'a, V> {
231236
return Err("Soon to come...".into());
232237
}
233238
let is_legacy = !paths.iter().any(|p| !p.is_legacy());
234-
if is_legacy {
235-
self.to_json_legacy(paths, indent, newline, space)
239+
if paths.len() > 1 {
240+
self.to_json_multi(paths, indent, newline, space, is_legacy)
236241
} else {
237-
self.to_json_multi(paths, indent, newline, space)
242+
self.to_json_single(paths[0].get_path(), indent, newline, space, is_legacy)
238243
}
239244
}
240245

@@ -330,6 +335,17 @@ impl<'a, V: SelectValue> KeyValue<'a, V> {
330335
Self::serialize(results, format)
331336
}
332337

338+
pub fn to_string_single(
339+
&self,
340+
path: &str,
341+
indent: Option<&str>,
342+
newline: Option<&str>,
343+
space: Option<&str>,
344+
) -> Result<String, Error> {
345+
let result = self.get_first(path)?;
346+
Ok(Self::serialize_object(&result, indent, newline, space))
347+
}
348+
333349
pub fn to_string_multi(
334350
&self,
335351
path: &str,
@@ -730,6 +746,43 @@ where
730746
.collect::<Vec<Value>>()
731747
}
732748

749+
/// Sort the paths so higher indices precede lower indices on the same on the same array
750+
/// And objects with higher hierarchy (closer to the top-level) preceded objects with deeper hierarchy
751+
fn prepare_paths_for_deletion(paths: &mut Vec<Vec<String>>) {
752+
paths.sort_by(|v1, v2| match (v1.len(), v2.len()) {
753+
(l1, l2) if l1 < l2 => Ordering::Less, // Shorter paths before longer paths
754+
(l1, l2) if l1 > l2 => Ordering::Greater, // Shorter paths before longer paths
755+
_ => v1
756+
.iter()
757+
.zip(v2.iter())
758+
.fold_while(Ordering::Equal, |_acc, (p1, p2)| {
759+
let i1 = p1.parse::<usize>();
760+
let i2 = p2.parse::<usize>();
761+
match (i1, i2) {
762+
(Err(_), Err(_)) => match p1.cmp(p2) {
763+
// String compare
764+
Ordering::Less => Done(Ordering::Less),
765+
Ordering::Equal => Continue(Ordering::Equal),
766+
Ordering::Greater => Done(Ordering::Greater),
767+
},
768+
(Ok(_), Err(_)) => Done(Ordering::Greater), //String before Numeric
769+
(Err(_), Ok(_)) => Done(Ordering::Less), //String before Numeric
770+
(Ok(i1), Ok(i2)) => {
771+
// Numeric compare - higher indices before lower ones
772+
if i1 < i2 {
773+
Done(Ordering::Greater)
774+
} else if i2 < i1 {
775+
Done(Ordering::Less)
776+
} else {
777+
Continue(Ordering::Equal)
778+
}
779+
}
780+
}
781+
})
782+
.into_inner(),
783+
});
784+
}
785+
733786
pub fn command_json_del<M: Manager>(
734787
manager: M,
735788
ctx: &Context,
@@ -750,7 +803,8 @@ pub fn command_json_del<M: Manager>(
750803
redis_key.delete()?;
751804
1
752805
} else {
753-
let paths = find_paths(path.get_path(), doc, |_| true)?;
806+
let mut paths = find_paths(path.get_path(), doc, |_| true)?;
807+
prepare_paths_for_deletion(&mut paths);
754808
let mut changed = 0;
755809
for p in paths {
756810
if redis_key.delete_path(p)? {
@@ -785,7 +839,7 @@ pub fn command_json_mget<M: Manager>(
785839
let to_string =
786840
|doc: &M::V| KeyValue::new(doc).to_string_multi(path.get_path(), None, None, None);
787841
let to_string_legacy =
788-
|doc: &M::V| KeyValue::new(doc).to_string(path.get_path(), Format::JSON);
842+
|doc: &M::V| KeyValue::new(doc).to_string_single(path.get_path(), None, None, None);
789843
let is_legacy = path.is_legacy();
790844

791845
let results: Result<Vec<RedisValue>, RedisError> = keys

‎system-setup.py

+1
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ def fedora(self):
4343

4444
def macos(self):
4545
self.install_gnu_utils()
46+
self.install("binutils")
4647
self.run("%s/bin/getgcc" % READIES)
4748

4849
def common_last(self):

‎tests/pytest/test_multi.py

+25-6
Original file line numberDiff line numberDiff line change
@@ -63,11 +63,23 @@ def testDelCommand(env):
6363

6464
r.assertOk(r.execute_command('JSON.SET', 'doc2', '$', '[1, 2, 3]'))
6565
res = r.execute_command('JSON.DEL', 'doc2', '$[*]')
66-
r.assertGreater(res, 0)
66+
r.assertEqual(res, 3)
6767

6868
r.assertOk(r.execute_command('JSON.SET', 'doc2', '$', '[1, 2, 3]'))
6969
res = r.execute_command('JSON.DEL', 'doc2', '$[2,1,0]')
70-
r.assertGreater(res, 0)
70+
r.assertEqual(res, 3)
71+
72+
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]}}'))
73+
res = r.execute_command('JSON.DEL', 'doc2', '$..x.b[*]')
74+
r.assertEqual(res, 3)
75+
res = r.execute_command('JSON.GET', 'doc2', '$')
76+
r.assertEqual(json.loads(res), [{"b": [1, 2, 3], "a": {"b": [1, 2, 3], "c": [1, 2, 3]}, "x": {"b": [], "c": [1, 2, 3]}}])
77+
78+
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]}}'))
79+
res = r.execute_command('JSON.DEL', 'doc2', '$..x.b[1,0,2]')
80+
r.assertEqual(res, 3)
81+
res = r.execute_command('JSON.GET', 'doc2', '$')
82+
r.assertEqual(json.loads(res), [{"b": [1, 2, 3], "a": {"b": [1, 2, 3], "c": [1, 2, 3]}, "x": {"b": [], "c": [1, 2, 3]}}])
7183

7284
# Test deleting a null value
7385
r.assertOk(r.execute_command('JSON.SET', 'doc2', '$', '[ true, { "answer": 42}, null ]'))
@@ -148,21 +160,28 @@ def testSetAndGetCommands(env):
148160

149161
# Test multi paths
150162
res = r.execute_command('JSON.GET', 'doc1', '$..tm', '$..nu')
151-
r.assertEqual(res, '[[[46,876.85],[134.761,"jcoels",null]],[[377,"qda",true]]]')
163+
r.assertEqual(json.loads(res), {"$..tm": [[46, 876.85], [134.761, "jcoels", None]], "$..nu": [[377, "qda", True]]})
152164
# Test multi paths - if one path is none-legacy - result format is not legacy
153165
res = r.execute_command('JSON.GET', 'doc1', '..tm', '$..nu')
154-
r.assertEqual(res, '[[[46,876.85],[134.761,"jcoels",null]],[[377,"qda",true]]]')
166+
r.assertEqual(json.loads(res), {"..tm": [[46, 876.85], [134.761, "jcoels", None]], "$..nu": [[377, "qda", True]]})
167+
# Test multi paths with formatting (using the same path in order to get a map and still avoid failure due to undefined ordering between map keys)
168+
res = r.execute_command('JSON.GET', 'doc2', 'INDENT', '\\t', 'NEWLINE', '\\n', 'SPACE', ' ', '$..a', '$..a')
169+
r.assertEqual(res, '{\\n\\t"$..a": [\\n\\t\\t4.2,\\n\\t\\t4.2\\n\\t]\\n}')
170+
155171
# Test missing key
156172
r.assertIsNone(r.execute_command('JSON.GET', 'docX', '..tm', '$..nu'))
157173
# Test missing path
158174
res = r.execute_command('JSON.GET', 'doc1', '..tm', '$..back_in_nov')
159-
r.assertEqual(res, '[[[46,876.85],[134.761,"jcoels",null]],[]]')
175+
r.assertEqual(json.loads(res), {"$..back_in_nov": [], "..tm": [[46, 876.85], [134.761, "jcoels", None]]})
160176
res = r.execute_command('JSON.GET', 'doc2', '..a', '..b', '$.back_in_nov')
161-
r.assertEqual(res, '[[4.2,4.2],[3],[]]')
177+
r.assertEqual(json.loads(res), {"$.back_in_nov": [], "..a": [4.2, 4.2], "..b": [3]})
162178

163179
# Test legacy multi path (all paths are legacy)
164180
res = r.execute_command('JSON.GET', 'doc1', '..nu', '..tm')
165181
r.assertEqual(json.loads(res), json.loads('{"..nu":[377,"qda",true],"..tm":[46,876.85]}'))
182+
# Test multi paths with formatting (using the same path in order to get a map and still avoid failure due to undefined ordering between map keys)
183+
res = r.execute_command('JSON.GET', 'doc2', 'INDENT', '\\t', 'NEWLINE', '\\n', 'SPACE', ' ', '..a', '..a')
184+
r.assertEqual(res, '{\\n\\t"..a": 4.2\\n}')
166185
# Test legacy single path
167186
res = r.execute_command('JSON.GET', 'doc1', '..tm')
168187
r.assertEqual(res, '[46,876.85]')

0 commit comments

Comments
 (0)