Skip to content

Commit 04551a9

Browse files
ephraimfeldblumephraimfeldblum
authored
MOD-7279 Fix SET commands with overlapping paths (#1225)
* not the solution. still brainstorming * some refactoring * more refactoring. rustification * more refactoring. rustification * missed variable renaming * making cargo fmt shut up * seriously, i wrote "if let None ="? * found where i changed logic. reverted changed logic without reverting refactor * some more refactoring * didn't even need try_for_each * "else { false }" grates on me * turbofish soup is disgusting * lazily index * fix the case of merge * fmt * per review * resolve unused using * turbofish soup is disgusting * hate fmt
1 parent dcc003d commit 04551a9

File tree

8 files changed

+182
-253
lines changed

8 files changed

+182
-253
lines changed

‎Cargo.lock

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

‎json_path/Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ serde.workspace = true
1414
ijson.workspace = true
1515
log = "0.4"
1616
regex = "1"
17+
itertools = "0.13"
1718

1819
[dev-dependencies]
1920
env_logger = "0.11"

‎json_path/src/json_path.rs

+3-2
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
* the Server Side Public License v1 (SSPLv1).
55
*/
66

7+
use itertools::Itertools;
78
use pest::iterators::{Pair, Pairs};
89
use pest::Parser;
910
use pest_derive::Parser;
@@ -161,7 +162,7 @@ pub(crate) fn compile(path: &str) -> Result<Query, QueryCompilationError> {
161162
positives
162163
.iter()
163164
.map(|v| format!("{v}"))
164-
.collect::<Vec<_>>()
165+
.collect_vec()
165166
.join(", "),
166167
)
167168
};
@@ -172,7 +173,7 @@ pub(crate) fn compile(path: &str) -> Result<Query, QueryCompilationError> {
172173
negatives
173174
.iter()
174175
.map(|v| format!("{v}"))
175-
.collect::<Vec<_>>()
176+
.collect_vec()
176177
.join(", "),
177178
)
178179
};

‎redis_json/src/commands.rs

+67-95
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,11 @@
77
use crate::error::Error;
88
use crate::formatter::ReplyFormatOptions;
99
use crate::key_value::KeyValue;
10-
use crate::manager::err_msg_json_path_doesnt_exist_with_param;
11-
use crate::manager::err_msg_json_path_doesnt_exist_with_param_or;
12-
use crate::manager::{Manager, ReadHolder, UpdateInfo, WriteHolder};
13-
use crate::redisjson::{Format, Path, ReplyFormat};
10+
use crate::manager::{
11+
err_msg_json_path_doesnt_exist_with_param, err_msg_json_path_doesnt_exist_with_param_or,
12+
Manager, ReadHolder, UpdateInfo, WriteHolder,
13+
};
14+
use crate::redisjson::{Format, Path, ReplyFormat, SetOptions};
1415
use json_path::select_value::{SelectValue, SelectValueType};
1516
use redis_module::{Context, RedisValue};
1617
use redis_module::{NextArg, RedisError, RedisResult, RedisString, REDIS_OK};
@@ -19,8 +20,6 @@ use std::str::FromStr;
1920

2021
use json_path::{calc_once_with_paths, compile, json_path::UserPathTracker};
2122

22-
use crate::redisjson::SetOptions;
23-
2423
use serde_json::{Number, Value};
2524

2625
use itertools::FoldWhile::{Continue, Done};
@@ -147,7 +146,7 @@ pub fn json_get<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>) -
147146

148147
let key = manager.open_key_read(ctx, &key)?;
149148
let value = match key.get_value()? {
150-
Some(doc) => KeyValue::new(doc).to_json(&mut paths, &format_options)?,
149+
Some(doc) => KeyValue::new(doc).to_json(paths, &format_options)?,
151150
None => RedisValue::Null,
152151
};
153152

@@ -199,23 +198,18 @@ pub fn json_set<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>) -
199198
Ok(RedisValue::Null)
200199
}
201200
} else {
202-
let update_info = KeyValue::new(doc).find_paths(path.get_path(), &op)?;
203-
if !update_info.is_empty() {
204-
let updated = apply_updates::<M>(&mut redis_key, val, update_info);
205-
if updated {
206-
redis_key.notify_keyspace_event(ctx, "json.set")?;
207-
manager.apply_changes(ctx);
208-
REDIS_OK
209-
} else {
210-
Ok(RedisValue::Null)
211-
}
201+
let update_info = KeyValue::new(doc).find_paths(path.get_path(), op)?;
202+
if !update_info.is_empty() && apply_updates::<M>(&mut redis_key, val, update_info) {
203+
redis_key.notify_keyspace_event(ctx, "json.set")?;
204+
manager.apply_changes(ctx);
205+
REDIS_OK
212206
} else {
213207
Ok(RedisValue::Null)
214208
}
215209
}
216210
}
217211
(None, SetOptions::AlreadyExists) => Ok(RedisValue::Null),
218-
(None, _) => {
212+
_ => {
219213
if path.get_path() == JSON_ROOT_PATH {
220214
redis_key.set_value(Vec::new(), val)?;
221215
redis_key.notify_keyspace_event(ctx, "json.set")?;
@@ -265,7 +259,7 @@ pub fn json_merge<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>)
265259
REDIS_OK
266260
} else {
267261
let mut update_info =
268-
KeyValue::new(doc).find_paths(path.get_path(), &SetOptions::None)?;
262+
KeyValue::new(doc).find_paths(path.get_path(), SetOptions::MergeExisting)?;
269263
if !update_info.is_empty() {
270264
let mut res = false;
271265
if update_info.len() == 1 {
@@ -336,7 +330,7 @@ pub fn json_mset<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>)
336330
let update_info = if path.get_path() == JSON_ROOT_PATH {
337331
None
338332
} else if let Some(value) = key_value {
339-
Some(KeyValue::new(value).find_paths(path.get_path(), &SetOptions::None)?)
333+
Some(KeyValue::new(value).find_paths(path.get_path(), SetOptions::None)?)
340334
} else {
341335
return Err(RedisError::Str(
342336
"ERR new objects must be created at the root",
@@ -377,24 +371,18 @@ fn apply_updates<M: Manager>(
377371
// If there is only one update info, we can avoid cloning the value
378372
if update_info.len() == 1 {
379373
match update_info.pop().unwrap() {
380-
UpdateInfo::SUI(sui) => redis_key.set_value(sui.path, value).unwrap_or(false),
381-
UpdateInfo::AUI(aui) => redis_key
382-
.dict_add(aui.path, &aui.key, value)
383-
.unwrap_or(false),
374+
UpdateInfo::SUI(sui) => redis_key.set_value(sui.path, value),
375+
UpdateInfo::AUI(aui) => redis_key.dict_add(aui.path, &aui.key, value),
384376
}
377+
.unwrap_or(false)
385378
} else {
386-
let mut updated = false;
387-
for ui in update_info {
388-
updated = match ui {
389-
UpdateInfo::SUI(sui) => redis_key
390-
.set_value(sui.path, value.clone())
391-
.unwrap_or(false),
392-
UpdateInfo::AUI(aui) => redis_key
393-
.dict_add(aui.path, &aui.key, value.clone())
394-
.unwrap_or(false),
395-
} || updated
396-
}
397-
updated
379+
update_info.into_iter().fold(false, |updated, ui| {
380+
match ui {
381+
UpdateInfo::SUI(sui) => redis_key.set_value(sui.path, value.clone()),
382+
UpdateInfo::AUI(aui) => redis_key.dict_add(aui.path, &aui.key, value.clone()),
383+
}
384+
.unwrap_or(updated)
385+
})
398386
}
399387
}
400388

@@ -438,11 +426,8 @@ where
438426
{
439427
values_and_paths
440428
.into_iter()
441-
.map(|(v, p)| match f(v) {
442-
true => Some(p),
443-
_ => None,
444-
})
445-
.collect::<Vec<Option<Vec<String>>>>()
429+
.map(|(v, p)| f(v).then_some(p))
430+
.collect()
446431
}
447432

448433
/// Returns a Vec of Values with `None` for Values that do not match the filter
@@ -452,11 +437,8 @@ where
452437
{
453438
values_and_paths
454439
.into_iter()
455-
.map(|(v, _)| match f(v) {
456-
true => Some(v),
457-
_ => None,
458-
})
459-
.collect::<Vec<Option<&T>>>()
440+
.map(|(v, _)| f(v).then_some(v))
441+
.collect()
460442
}
461443

462444
fn find_all_paths<T: SelectValue, F>(
@@ -496,13 +478,13 @@ where
496478
values
497479
.into_iter()
498480
.map(|n| n.map_or_else(|| none_value.clone(), |t| t.into()))
499-
.collect::<Vec<Value>>()
481+
.collect()
500482
}
501483

502484
/// Sort the paths so higher indices precede lower indices on the same array,
503485
/// And longer paths precede shorter paths
504486
/// And if a path is a sub-path of the other, then only paths with shallower hierarchy (closer to the top-level) remain
505-
fn prepare_paths_for_deletion(paths: &mut Vec<Vec<String>>) {
487+
pub fn prepare_paths_for_updating(paths: &mut Vec<Vec<String>>) {
506488
if paths.len() < 2 {
507489
// No need to reorder when there are less than 2 paths
508490
return;
@@ -542,21 +524,17 @@ fn prepare_paths_for_deletion(paths: &mut Vec<Vec<String>>) {
542524
});
543525
// Remove paths which are nested by others (on each sub-tree only top most ancestor should be deleted)
544526
// (TODO: Add a mode in which the jsonpath selector will already skip nested paths)
545-
let mut string_paths = Vec::new();
546-
paths.iter().for_each(|v| {
547-
string_paths.push(v.join(","));
548-
});
527+
let mut string_paths = paths.iter().map(|v| v.join(",")).collect_vec();
549528
string_paths.sort();
550529

551530
paths.retain(|v| {
552531
let path = v.join(",");
553-
let found = string_paths.binary_search(&path).unwrap();
554-
for p in string_paths.iter().take(found) {
555-
if path.starts_with(p.as_str()) {
556-
return false;
557-
}
558-
}
559-
true
532+
string_paths
533+
.iter()
534+
.skip_while(|p| !path.starts_with(*p))
535+
.next()
536+
.map(|found| path == *found)
537+
.unwrap_or(false)
560538
});
561539
}
562540

@@ -580,7 +558,7 @@ pub fn json_del<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>) -
580558
1
581559
} else {
582560
let mut paths = find_paths(path.get_path(), doc, |_| true)?;
583-
prepare_paths_for_deletion(&mut paths);
561+
prepare_paths_for_updating(&mut paths);
584562
let mut changed = 0;
585563
for p in paths {
586564
if redis_key.delete_path(p)? {
@@ -676,8 +654,8 @@ where
676654
Some(root) => KeyValue::new(root)
677655
.get_values(path)?
678656
.iter()
679-
.map(|v| (KeyValue::value_name(*v)).into())
680-
.collect::<Vec<RedisValue>>()
657+
.map(|v| RedisValue::from(KeyValue::value_name(*v)))
658+
.collect_vec()
681659
.into(),
682660
None => RedisValue::Null,
683661
};
@@ -688,14 +666,11 @@ fn json_type_legacy<M>(redis_key: &M::ReadHolder, path: &str) -> RedisResult
688666
where
689667
M: Manager,
690668
{
691-
let value = redis_key.get_value()?.map_or_else(
692-
|| RedisValue::Null,
693-
|doc| {
694-
KeyValue::new(doc)
695-
.get_type(path)
696-
.map_or(RedisValue::Null, |s| s.into())
697-
},
698-
);
669+
let value = redis_key.get_value()?.map_or(RedisValue::Null, |doc| {
670+
KeyValue::new(doc)
671+
.get_type(path)
672+
.map_or(RedisValue::Null, |s| s.into())
673+
});
699674
Ok(value)
700675
}
701676

@@ -734,7 +709,7 @@ where
734709
op,
735710
cmd,
736711
)?
737-
.drain(..)
712+
.into_iter()
738713
.map(|v| {
739714
v.map_or(RedisValue::Null, |v| {
740715
if let Some(i) = v.as_i64() {
@@ -744,7 +719,7 @@ where
744719
}
745720
})
746721
})
747-
.collect::<Vec<RedisValue>>()
722+
.collect_vec()
748723
.into();
749724
Ok(res)
750725
} else if path.is_legacy() {
@@ -796,21 +771,21 @@ where
796771
)
797772
})?;
798773

799-
let mut res = vec![];
800774
let mut need_notify = false;
801-
for p in paths {
802-
res.push(match p {
803-
Some(p) => {
775+
let res = paths
776+
.into_iter()
777+
.map(|p| {
778+
p.map(|p| {
804779
need_notify = true;
805-
Some(match op {
806-
NumOp::Incr => redis_key.incr_by(p, number)?,
807-
NumOp::Mult => redis_key.mult_by(p, number)?,
808-
NumOp::Pow => redis_key.pow_by(p, number)?,
809-
})
810-
}
811-
_ => None,
812-
});
813-
}
780+
match op {
781+
NumOp::Incr => redis_key.incr_by(p, number),
782+
NumOp::Mult => redis_key.mult_by(p, number),
783+
NumOp::Pow => redis_key.pow_by(p, number),
784+
}
785+
})
786+
.transpose()
787+
})
788+
.try_collect()?;
814789
if need_notify {
815790
redis_key.notify_keyspace_event(ctx, cmd)?;
816791
manager.apply_changes(ctx);
@@ -1676,9 +1651,7 @@ where
16761651
let values = find_all_values(path, root, |v| v.get_type() == SelectValueType::Object)?;
16771652
let mut res: Vec<RedisValue> = vec![];
16781653
for v in values {
1679-
res.push(v.map_or(RedisValue::Null, |v| {
1680-
v.keys().unwrap().collect::<Vec<&str>>().into()
1681-
}));
1654+
res.push(v.map_or(RedisValue::Null, |v| v.keys().unwrap().collect_vec().into()));
16821655
}
16831656
res.into()
16841657
};
@@ -1695,7 +1668,7 @@ where
16951668
};
16961669
let value = match KeyValue::new(root).get_first(path) {
16971670
Ok(v) => match v.get_type() {
1698-
SelectValueType::Object => v.keys().unwrap().collect::<Vec<&str>>().into(),
1671+
SelectValueType::Object => v.keys().unwrap().collect_vec().into(),
16991672
_ => {
17001673
return Err(RedisError::String(
17011674
err_msg_json_path_doesnt_exist_with_param_or(path, "not an object"),
@@ -1737,7 +1710,7 @@ where
17371710
RedisValue::Integer(v.len().unwrap() as i64)
17381711
})
17391712
})
1740-
.collect::<Vec<RedisValue>>()
1713+
.collect_vec()
17411714
.into(),
17421715
None => {
17431716
return Err(RedisError::String(
@@ -1838,7 +1811,7 @@ pub fn json_debug<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>)
18381811
.get_values(path.get_path())?
18391812
.iter()
18401813
.map(|v| manager.get_memory(v).unwrap())
1841-
.collect::<Vec<usize>>(),
1814+
.collect(),
18421815
None => vec![],
18431816
}
18441817
.into())
@@ -1870,8 +1843,7 @@ pub fn json_resp<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>)
18701843
};
18711844

18721845
let key = manager.open_key_read(ctx, &key)?;
1873-
key.get_value()?.map_or_else(
1874-
|| Ok(RedisValue::Null),
1875-
|doc| KeyValue::new(doc).resp_serialize(path),
1876-
)
1846+
key.get_value()?.map_or(Ok(RedisValue::Null), |doc| {
1847+
KeyValue::new(doc).resp_serialize(path)
1848+
})
18771849
}

0 commit comments

Comments
 (0)