upgrade to redismodule-rs v2 (#964)

* upgrade to redismodule-rs v2 * refactor break KeyValue to its own file (#965) * fix build * fix build remove feature test * support RESP3 * use FormatOptions * add support for multi paths * set redismodule-rs to 2.0 * fix String serializtion * change json.get resutl from map tp array * format code and update deps * add system allocator when running unit test * format code * add initial tests * add more tests * add more tests * fix tests/pytest/requirements.txt * add tests for NUMMULTBY & NUMINCRBY * fix test for cluster * add support for JSON.TYPE * add EXPAND format on json.get * avoid using cutom formatter when not needed * remove bson code * fix test assert on unordered results * update Cargo.lock and add ignore words * revert deps/readies * revert test_resp3 * retrun bson * fix review commentsw
RedisJSON · oshadmi · Jun 1, 2023 · May 7, 2023 · May 7, 2023 · May 10, 2023
commit 76fc776f420e50442f3a0b9b3bb3b455478a3739
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -9,6 +9,7 @@ members = [
 ijson = "0.1.3"
 serde_json = { version="1.0", features = ["unbounded_depth"]}
 serde = "1.0"
+bson = "0.14"
 
 [workspace.package]
 edition = "2021"

diff --git a/json_path/Cargo.toml b/json_path/Cargo.toml
@@ -8,6 +8,7 @@ license.workspace = true
 [dependencies]
 pest = "2.1"
 pest_derive = "2.1"
+bson.workspace = true
 serde_json.workspace = true
 serde.workspace = true
 ijson.workspace = true

diff --git a/json_path/json_examples/giveme_every_thing_result.json b/json_path/json_examples/giveme_every_thing_result.json
@@ -113,4 +113,3 @@
   "red",
   19.95
 ]
-
diff --git a/json_path/tests/array_filter.rs b/json_path/tests/array_filter.rs
@@ -1,9 +1,9 @@
-/*
- * Copyright Redis Ltd. 2016 - present
- * Licensed under your choice of the Redis Source Available License 2.0 (RSALv2) or
- * the Server Side Public License v1 (SSPLv1).
- */
-
+/*
+ * Copyright Redis Ltd. 2016 - present
+ * Licensed under your choice of the Redis Source Available License 2.0 (RSALv2) or
+ * the Server Side Public License v1 (SSPLv1).
+ */
+
 #[macro_use]
 extern crate serde_json;
 
@@ -150,7 +150,7 @@ fn array_range_only_from_index() {
 }
 
 #[test]
-fn array_range_only_nagative_end_index() {
+fn array_range_only_negative_end_index() {
     setup();
 
     select_and_then_compare(

diff --git a/redis_json/Cargo.toml b/redis_json/Cargo.toml
@@ -20,7 +20,7 @@ env_logger = "0.10.0"
 
 [dependencies]
 bitflags = "1.3"
-bson = "0.14"
+bson.workspace = true
 ijson.workspace = true
 serde_json.workspace = true
 serde.workspace = true

diff --git a/redis_json/src/c_api.rs b/redis_json/src/c_api.rs
@@ -13,6 +13,7 @@ use std::{
     os::raw::{c_char, c_void},
 };
 
+use crate::formatter::FormatOptions;
 use crate::key_value::KeyValue;
 use json_path::select_value::{SelectValue, SelectValueType};
 use json_path::{compile, create};
@@ -132,7 +133,7 @@ pub fn json_api_get_json<M: Manager>(
     str: *mut *mut rawmod::RedisModuleString,
 ) -> c_int {
     let json = unsafe { &*(json.cast::<M::V>()) };
-    let res = KeyValue::<M::V>::serialize_object(json, None, None, None);
+    let res = KeyValue::<M::V>::serialize_object(json, &FormatOptions::default());
     create_rmstring(ctx, &res, str)
 }
 
@@ -146,7 +147,7 @@ pub fn json_api_get_json_from_iter<M: Manager>(
     if iter.pos >= iter.results.len() {
         Status::Err as c_int
     } else {
-        let res = KeyValue::<M::V>::serialize_object(&iter.results, None, None, None);
+        let res = KeyValue::<M::V>::serialize_object(&iter.results, &FormatOptions::default());
         create_rmstring(ctx, &res, str);
         Status::Ok as c_int
     }

diff --git a/redis_json/src/commands.rs b/redis_json/src/commands.rs
@@ -5,6 +5,7 @@
  */
 
 use crate::error::Error;
+use crate::formatter::FormatOptions;
 use crate::key_value::KeyValue;
 use crate::manager::err_msg_json_path_doesnt_exist_with_param;
 use crate::manager::err_msg_json_path_doesnt_exist_with_param_or;
@@ -79,6 +80,11 @@ impl<'a, V: SelectValue> Serialize for Values<'a, V> {
     }
 }
 
+fn is_resp3(ctx: &Context) -> bool {
+    ctx.get_flags()
+        .contains(redis_module::ContextFlags::FLAGS_RESP3)
+}
+
 ///
 /// JSON.GET <key>
 ///         [INDENT indentation-string]
@@ -93,23 +99,31 @@ pub fn json_get<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>) -
 
     // Set Capacity to 1 assuming the common case has one path
     let mut paths: Vec<Path> = Vec::with_capacity(1);
-    let mut format = Format::JSON;
-    let mut indent = None;
-    let mut space = None;
-    let mut newline = None;
+
+    let mut format_options = FormatOptions {
+        resp3: is_resp3(ctx),
+        ..Default::default()
+    };
+
     while let Ok(arg) = args.next_str() {
         match arg {
             // fast way to consider arg a path by using the max length of all possible subcommands
             // See #390 for the comparison of this function with/without this optimization
             arg if arg.len() > JSONGET_SUBCOMMANDS_MAXSTRLEN => paths.push(Path::new(arg)),
-            arg if arg.eq_ignore_ascii_case(CMD_ARG_INDENT) => indent = Some(args.next_str()?),
-            arg if arg.eq_ignore_ascii_case(CMD_ARG_NEWLINE) => newline = Some(args.next_str()?),
-            arg if arg.eq_ignore_ascii_case(CMD_ARG_SPACE) => space = Some(args.next_str()?),
-            // Silently ignore. Compatibility with ReJSON v1.0 which has this option. See #168 TODO add support
-            arg if arg.eq_ignore_ascii_case(CMD_ARG_NOESCAPE) => continue,
             arg if arg.eq_ignore_ascii_case(CMD_ARG_FORMAT) => {
-                format = Format::from_str(args.next_str()?)?;
+ format_options.format = Format::from_str(args.next_str()?)?;
             }
+            arg if arg.eq_ignore_ascii_case(CMD_ARG_INDENT) => {
+                format_options.indent = Some(args.next_str()?)
+            }
+            arg if arg.eq_ignore_ascii_case(CMD_ARG_NEWLINE) => {
+                format_options.newline = Some(args.next_str()?)
+            }
+            arg if arg.eq_ignore_ascii_case(CMD_ARG_SPACE) => {
+                format_options.space = Some(args.next_str()?)
+            }
+            // Silently ignore. Compatibility with ReJSON v1.0 which has this option. See #168 TODO add support
+            arg if arg.eq_ignore_ascii_case(CMD_ARG_NOESCAPE) => continue,
             _ => paths.push(Path::new(arg)),
         };
     }
@@ -121,7 +135,7 @@ pub fn json_get<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>) -
 
     let key = manager.open_key_read(ctx, &key)?;
     let value = match key.get_value()? {
-        Some(doc) => KeyValue::new(doc).to_json(&mut paths, indent, newline, space, format)?,
+        Some(doc) => KeyValue::new(doc).to_json(&mut paths, &format_options)?,
         None => RedisValue::Null,
     };
 
@@ -576,10 +590,15 @@ pub fn json_mget<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>)
         let path = Path::new(path.try_as_str()?);
         let keys = &args[1..args.len() - 1];
 
+        let format_options = FormatOptions {
+            resp3: is_resp3(ctx),
+            ..Default::default()
+        };
+
         let to_string =
-            |doc: &M::V| KeyValue::new(doc).to_string_multi(path.get_path(), None, None, None);
+            |doc: &M::V| KeyValue::new(doc).to_string_multi(path.get_path(), &format_options);
         let to_string_legacy =
-            |doc: &M::V| KeyValue::new(doc).to_string_single(path.get_path(), None, None, None);
+            |doc: &M::V| KeyValue::new(doc).to_string_single(path.get_path(), &format_options);
         let is_legacy = path.is_legacy();
 
         let results: Result<Vec<RedisValue>, RedisError> = keys
@@ -618,10 +637,17 @@ pub fn json_type<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>)
 
     let key = manager.open_key_read(ctx, &key)?;
 
-    if path.is_legacy() {
-        json_type_legacy::<M>(&key, path.get_path())
+    let value = if path.is_legacy() {
+        json_type_legacy::<M>(&key, path.get_path())?
+    } else {
+        json_type_impl::<M>(&key, path.get_path())?
+    };
+
+    // Check context flags to see if RESP3 is enabled and return the appropriate result
+    if is_resp3(ctx) {
+        Ok(vec![value].into())
     } else {
- json_type_impl::<M>(&key, path.get_path())
+ Ok(value)
     }
 }
 
@@ -630,7 +656,7 @@ where
     M: Manager,
 {
     let root = redis_key.get_value()?;
-    let res = match root {
+    let value = match root {
         Some(root) => KeyValue::new(root)
             .get_values(path)?
             .iter()
@@ -639,7 +665,7 @@ where
             .into(),
         None => RedisValue::Null,
     };
-    Ok(res)
+    Ok(value)
 }
 
 fn json_type_legacy<M>(redis_key: &M::ReadHolder, path: &str) -> RedisResult
@@ -654,7 +680,6 @@ where
                 .map_or(RedisValue::Null, |s| s.into())
         },
     );
-
     Ok(value)
 }
 
@@ -682,10 +707,30 @@ where
 
     let mut redis_key = manager.open_key_write(ctx, key)?;
 
-    if path.is_legacy() {
+    // check context flags to see if RESP3 is enabled
+    if is_resp3(ctx) {
+        let res = json_num_op_impl::<M>(&mut redis_key, ctx, path.get_path(), number, op, cmd)?
+            .drain(..)
+            .map(|v| {
+                v.map_or(RedisValue::Null, |v| {
+                    if let Some(i) = v.as_i64() {
+                        RedisValue::Integer(i)
+                    } else {
+                        RedisValue::Float(v.as_f64().unwrap_or_default())
+                    }
+                })
+            })
+            .collect::<Vec<RedisValue>>()
+            .into();
+        Ok(res)
+    } else if path.is_legacy() {
         json_num_op_legacy::<M>(&mut redis_key, ctx, path.get_path(), number, op, cmd)
     } else {
-        json_num_op_impl::<M>(&mut redis_key, ctx, path.get_path(), number, op, cmd)
+        let results = json_num_op_impl::<M>(&mut redis_key, ctx, path.get_path(), number, op, cmd)?;
+
+        // Convert to RESP2 format return as one JSON array
+        let values = to_json_value::<Number>(results, Value::Null);
+        Ok(KeyValue::<M::V>::serialize_object(&values, &FormatOptions::default()).into())
     }
 }
 
@@ -696,7 +741,7 @@ fn json_num_op_impl<M>(
     number: &str,
     op: NumOp,
     cmd: &str,
-) -> RedisResult
+) -> Result<Vec<Option<Number>>, RedisError>
 where
     M: Manager,
 {
@@ -728,9 +773,7 @@ where
     if need_notify {
         redis_key.apply_changes(ctx, cmd)?;
     }
-
-    let res = to_json_value::<Number>(res, Value::Null);
-    Ok(KeyValue::<M::V>::serialize_object(&res, None, None, None).into())
+    Ok(res)
 }
 
 fn json_num_op_legacy<M>(

diff --git a/redis_json/src/formatter.rs b/redis_json/src/formatter.rs
@@ -35,6 +35,38 @@ DEALINGS IN THE SOFTWARE.
 use serde_json::ser::Formatter;
 use std::io;
 
+use crate::redisjson::Format;
+
+pub struct FormatOptions<'a> {
+    pub format: Format,
+    pub indent: Option<&'a str>,
+    pub space: Option<&'a str>,
+    pub newline: Option<&'a str>,
+    pub resp3: bool,
+}
+
+impl PartialEq for &FormatOptions<'_> {
+    fn eq(&self, other: &Self) -> bool {
+        self.format == other.format
+            && self.indent == other.indent
+            && self.space == other.space
+            && self.newline == other.newline
+            && self.resp3 == other.resp3
+    }
+}
+
+impl Default for FormatOptions<'_> {
+    fn default() -> Self {
+        Self {
+            format: Format::JSON,
+            indent: None,
+            space: None,
+            newline: None,
+            resp3: false,
+        }
+    }
+}
+
 pub struct RedisJsonFormatter<'a> {
     current_indent: usize,
     has_value: bool,
@@ -44,17 +76,13 @@ pub struct RedisJsonFormatter<'a> {
 }
 
 impl<'a> RedisJsonFormatter<'a> {
-    pub const fn new(
-        indent: Option<&'a str>,
-        space: Option<&'a str>,
-        newline: Option<&'a str>,
-    ) -> Self {
+    pub const fn new(format: &'a FormatOptions) -> Self {
         RedisJsonFormatter {
             current_indent: 0,
             has_value: false,
-            indent,
-            space,
-            newline,
+            indent: format.indent,
+            space: format.space,
+            newline: format.newline,
         }
     }
 
@@ -177,7 +205,13 @@ mod tests {
     #[test]
     #[allow(clippy::cognitive_complexity)]
     fn test_default_formatter() {
-        let mut formatter = RedisJsonFormatter::new(None, None, None);
+        let mut formatter = RedisJsonFormatter::new(&FormatOptions {
+            format: Format::JSON,
+            indent: None,
+            space: None,
+            newline: None,
+            resp3: false,
+        });
         let mut writer = vec![];
 
         assert!(matches!(formatter.begin_array(&mut writer), Ok(())));
@@ -235,7 +269,13 @@ mod tests {
     #[test]
     #[allow(clippy::cognitive_complexity)]
     fn test_ident_formatter() {
-        let mut formatter = RedisJsonFormatter::new(Some("_"), None, None);
+        let mut formatter = RedisJsonFormatter::new(&FormatOptions {
+            format: Format::JSON,
+            indent: Some("_"),
+            space: None,
+            newline: None,
+            resp3: false,
+        });
         let mut writer = vec![];
 
         assert!(matches!(formatter.begin_array(&mut writer), Ok(())));
@@ -293,7 +333,13 @@ mod tests {
     #[test]
     #[allow(clippy::cognitive_complexity)]
     fn test_space_formatter() {
-        let mut formatter = RedisJsonFormatter::new(None, Some("s"), None);
+        let mut formatter = RedisJsonFormatter::new(&FormatOptions {
+            format: Format::JSON,
+            indent: None,
+            space: Some("s"),
+            newline: None,
+            resp3: false,
+        });
         let mut writer = vec![];
 
         assert!(matches!(formatter.begin_array(&mut writer), Ok(())));
@@ -351,7 +397,13 @@ mod tests {
     #[test]
     #[allow(clippy::cognitive_complexity)]
     fn test_new_line_formatter() {
-        let mut formatter = RedisJsonFormatter::new(None, None, Some("n"));
+        let mut formatter = RedisJsonFormatter::new(&FormatOptions {
+            format: Format::JSON,
+            indent: None,
+            space: None,
+            newline: Some("n"),
+            resp3: false,
+        });
         let mut writer = vec![];
 
         assert!(matches!(formatter.begin_array(&mut writer), Ok(())));