Skip to content

Commit 03b166f

Browse files
ephraimfeldblumgabsow
ephraimfeldblum
authored and
gabsow
committed
MOD-13014 Remove user info from errors (#1474)
* path info * value types * query compilation error * query compilation error * cleaning up * stop using Error when we have RedisError
1 parent c6cac5e commit 03b166f

16 files changed

Lines changed: 239 additions & 606 deletions

File tree

‎Cargo.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ members = [
77
]
88

99
[workspace.dependencies]
10+
redis-module = { git="https://github.com/RedisLabsModules/redismodule-rs", tag="v2.1.3", default-features = false, features = ["min-redis-compatibility-version-7-4"] }
11+
redis-module-macros = { git="https://github.com/RedisLabsModules/redismodule-rs", tag="v2.1.3" }
1012
ijson = { git="https://github.com/RedisJSON/ijson", rev="5676f5929863113c2d0e38c8dc27632624217231", default-features=false}
1113
serde_json = { version="1", features = ["unbounded_depth"]}
1214
serde = { version = "1", features = ["derive"] }

‎json_path/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ bson.workspace = true
1212
serde_json.workspace = true
1313
serde.workspace = true
1414
ijson.workspace = true
15+
redis-module.workspace = true
1516
log = "0.4"
1617
regex = "1"
1718
itertools = "0.13"

‎json_path/src/json_path.rs‎

Lines changed: 36 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,16 @@
77
* GNU Affero General Public License v3 (AGPLv3).
88
*/
99

10+
use crate::select_value::{SelectValue, SelectValueType, ValueRef};
1011
use itertools::Itertools;
12+
use log::trace;
1113
use pest::iterators::{Pair, Pairs};
1214
use pest::Parser;
1315
use pest_derive::Parser;
16+
use redis_module::rediserror::RedisError;
17+
use regex::Regex;
1418
use std::borrow::Cow;
1519
use std::cmp::Ordering;
16-
17-
use crate::select_value::{SelectValue, SelectValueType, ValueRef};
18-
use log::trace;
19-
use regex::Regex;
2020
use std::fmt::Debug;
2121

2222
// Macro to handle items() iterator for both Borrowed and Owned ValueRef cases
@@ -26,16 +26,16 @@ macro_rules! value_ref_items {
2626
ValueRef::Borrowed(borrowed_val) => {
2727
// For borrowed values, convert keys to owned for consistent return type
2828
let iter = borrowed_val.items().unwrap();
29-
let collected: Vec<_> = iter.map(|(k, v)| (Cow::Borrowed(k), v)).collect();
29+
let collected = iter.map(|(k, v)| (Cow::Borrowed(k), v)).collect_vec();
3030
Box::new(collected.into_iter())
3131
as Box<dyn Iterator<Item = (Cow<'_, str>, ValueRef<'_, S>)>>
3232
}
3333
ValueRef::Owned(owned_val) => {
3434
// For owned values, collect first to avoid lifetime issues
3535
let iter = owned_val.items().unwrap();
36-
let collected: Vec<_> = iter
36+
let collected = iter
3737
.map(|(k, v)| (Cow::Owned(k.to_string()), ValueRef::Owned(v.inner_cloned())))
38-
.collect();
38+
.collect_vec();
3939
Box::new(collected.into_iter())
4040
as Box<dyn Iterator<Item = (Cow<'_, str>, ValueRef<'_, S>)>>
4141
}
@@ -55,7 +55,9 @@ macro_rules! value_ref_values {
5555
ValueRef::Owned(owned_val) => {
5656
// For owned values, we need to collect first to avoid lifetime issues
5757
let iter = owned_val.values().unwrap();
58-
let collected: Vec<_> = iter.map(|v| ValueRef::Owned(v.inner_cloned())).collect();
58+
let collected = iter
59+
.map(|v| ValueRef::Owned(v.inner_cloned()))
60+
.collect_vec();
5961
Box::new(collected.into_iter()) as Box<dyn Iterator<Item = ValueRef<'_, S>>>
6062
}
6163
}
@@ -105,11 +107,16 @@ pub struct QueryCompilationError {
105107
message: String,
106108
}
107109

110+
impl From<QueryCompilationError> for RedisError {
111+
fn from(e: QueryCompilationError) -> Self {
112+
Self::String(e.to_string())
113+
}
114+
}
115+
108116
impl<'i> Query<'i> {
109117
/// Pop the last element from the compiled json path.
110118
/// For example, if the json path is $.foo.bar then `pop_last`
111-
/// will return bar and leave the json path query with foo only
112-
/// ($.foo)
119+
/// will return bar and leave the json path query with $.foo
113120
#[allow(dead_code)]
114121
pub fn pop_last(&mut self) -> Option<(String, JsonPathToken)> {
115122
self.root.next_back().and_then(|last| match last.as_rule() {
@@ -123,7 +130,7 @@ impl<'i> Query<'i> {
123130
let first_on_list = last.into_inner().next();
124131
first_on_list.map(|first| (first.as_str().to_string(), JsonPathToken::String))
125132
}
126-
_ => panic!("pop last was used in a none static path"),
133+
_ => panic!("pop last was used in a non-static path"),
127134
})
128135
}
129136

@@ -138,11 +145,11 @@ impl<'i> Query<'i> {
138145
self.size()
139146
}
140147

141-
/// Results whether or not the compiled json path is static
142-
/// Static path is a path that is promised to have at most a single result.
148+
/// Returns whether the compiled json path is static
149+
/// A static path is a path that is promised to have at most a single result.
143150
/// Example:
144151
/// static path: $.foo.bar
145-
/// none static path: $.*.bar
152+
/// non-static path: $.*.bar
146153
#[allow(dead_code)]
147154
pub fn is_static(&mut self) -> bool {
148155
if self.is_static.is_some() {
@@ -174,7 +181,7 @@ impl std::fmt::Display for QueryCompilationError {
174181
fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
175182
write!(
176183
f,
177-
"Error occurred on position {}, {}",
184+
"Error occurred at position {}, {}",
178185
self.location, self.message
179186
)
180187
}
@@ -211,59 +218,31 @@ pub(crate) fn compile(path: &str) -> Result<Query<'_>, QueryCompilationError> {
211218
}
212219
// pest::error::Error
213220
Err(e) => {
214-
let pos = match e.location {
221+
let location = match e.location {
215222
pest::error::InputLocation::Pos(pos) => pos,
216223
pest::error::InputLocation::Span((pos, _end)) => pos,
217224
};
218-
let msg = match e.variant {
225+
let msg = match &e.variant {
219226
pest::error::ErrorVariant::ParsingError {
220-
ref positives,
221-
ref negatives,
227+
positives,
228+
negatives,
222229
} => {
223-
let positives = if positives.is_empty() {
224-
None
225-
} else {
226-
Some(
227-
positives
228-
.iter()
229-
.map(|v| format!("{v}"))
230-
.collect_vec()
231-
.join(", "),
232-
)
233-
};
234-
let negatives = if negatives.is_empty() {
235-
None
236-
} else {
237-
Some(
238-
negatives
239-
.iter()
240-
.map(|v| format!("{v}"))
241-
.collect_vec()
242-
.join(", "),
243-
)
244-
};
245-
246-
match (positives, negatives) {
247-
(None, None) => "parsing error".to_string(),
248-
(Some(p), None) => format!("expected one of the following: {p}"),
249-
(None, Some(n)) => format!("unexpected tokens found: {n}"),
250-
(Some(p), Some(n)) => format!(
230+
let p = positives.into_iter().join(", ");
231+
let n = negatives.into_iter().join(", ");
232+
match (p.len(), n.len()) {
233+
(0, 0) => "parsing error".to_string(),
234+
(_, 0) => format!("expected one of the following: {p}"),
235+
(0, _) => format!("unexpected tokens found: {n}"),
236+
(_, _) => format!(
251237
"expected one of the following: {p}, unexpected tokens found: {n}"
252238
),
253239
}
254240
}
255-
pest::error::ErrorVariant::CustomError { ref message } => message.clone(),
241+
pest::error::ErrorVariant::CustomError { message } => message.clone(),
256242
};
257243

258-
let final_msg = if pos == path.len() {
259-
format!("\"{path} <<<<----\", {msg}.")
260-
} else {
261-
format!("\"{} ---->>>> {}\", {}.", &path[..pos], &path[pos..], msg)
262-
};
263-
Err(QueryCompilationError {
264-
location: pos,
265-
message: final_msg,
266-
})
244+
let message = format!("Error at position {}: {}", location, msg);
245+
Err(QueryCompilationError { location, message })
267246
}
268247
}
269248
}

‎json_path/src/select_value.rs‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* GNU Affero General Public License v3 (AGPLv3).
88
*/
99

10-
use serde::Serialize;
10+
use serde::{Serialize, Serializer};
1111
use std::fmt::Debug;
1212

1313
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -28,10 +28,7 @@ pub enum ValueRef<'a, T: SelectValue> {
2828
}
2929

3030
impl<'a, T: SelectValue> Serialize for ValueRef<'a, T> {
31-
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
32-
where
33-
S: serde::Serializer,
34-
{
31+
fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
3532
self.as_ref().serialize(serializer)
3633
}
3734
}

‎redis_json/Cargo.toml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,10 @@ bson.workspace = true
2121
ijson.workspace = true
2222
serde_json.workspace = true
2323
serde.workspace = true
24+
redis-module.workspace = true
25+
redis-module-macros.workspace = true
2426
half = "2.0.0"
2527
libc = "0.2"
26-
redis-module ={ git="https://github.com/RedisLabsModules/redismodule-rs", tag="v2.1.2", default-features = false, features = ["min-redis-compatibility-version-7-4"] }
27-
redis-module-macros = { git="https://github.com/RedisLabsModules/redismodule-rs", tag="v2.1.2" }
2828
itertools = "0.13"
2929
json_path = {path="../json_path"}
3030
linkme = "0.3"

‎redis_json/src/backward.rs‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,11 @@
1010
use std::vec::Vec;
1111

1212
use redis_module::raw;
13+
use redis_module::{RedisError, RedisResult};
1314
use serde_json::map::Map;
1415
use serde_json::Number;
1516
use serde_json::Value;
1617

17-
use crate::error::Error;
18-
1918
#[derive(Debug, PartialEq)]
2019
enum NodeType {
2120
Null,
@@ -47,7 +46,7 @@ impl From<u64> for NodeType {
4746
}
4847
}
4948

50-
pub fn json_rdb_load(rdb: *mut raw::RedisModuleIO) -> Result<Value, Error> {
49+
pub fn json_rdb_load(rdb: *mut raw::RedisModuleIO) -> RedisResult<Value> {
5150
let node_type = raw::load_unsigned(rdb)?.into();
5251
match node_type {
5352
NodeType::Null => Ok(Value::Null),
@@ -62,7 +61,7 @@ pub fn json_rdb_load(rdb: *mut raw::RedisModuleIO) -> Result<Value, Error> {
6261
NodeType::Number => {
6362
let n = raw::load_double(rdb)?;
6463
Ok(Value::Number(
65-
Number::from_f64(n).ok_or_else(|| Error::from("Can't load as float"))?,
64+
Number::from_f64(n).ok_or_else(|| RedisError::Str("Can't load as float"))?,
6665
))
6766
}
6867
NodeType::String => {
@@ -75,7 +74,7 @@ pub fn json_rdb_load(rdb: *mut raw::RedisModuleIO) -> Result<Value, Error> {
7574
for _ in 0..len {
7675
let t: NodeType = raw::load_unsigned(rdb)?.into();
7776
if t != NodeType::KeyVal {
78-
return Err(Error::from("Can't load old RedisJSON RDB"));
77+
return Err(RedisError::Str("Can't load old RedisJSON RDB"));
7978
}
8079
let buffer = raw::load_string_buffer(rdb)?;
8180
m.insert(buffer.to_string()?, json_rdb_load(rdb)?);
@@ -91,6 +90,6 @@ pub fn json_rdb_load(rdb: *mut raw::RedisModuleIO) -> Result<Value, Error> {
9190
}
9291
Ok(Value::Array(v))
9392
}
94-
NodeType::KeyVal => Err(Error::from("Can't load old RedisJSON RDB")),
93+
NodeType::KeyVal => Err(RedisError::Str("Can't load old RedisJSON RDB")),
9594
}
9695
}

0 commit comments

Comments
 (0)