cleanup and integration testing

Author: DarthB

Cargo.toml

@@ -147,10 +147,6 @@ redis-module = { path = "./", default-features = false }
 bindgen = { version = "0.66", default-features = false, features = ["logging", "prettyplease"]}
 cc = "1"
 
-#[build-dependencies.bindgen]
-#version = "0.71.1"
-#default-features = false
-
 [features]
 default = ["min-redis-compatibility-version-6-0", "bindgen-runtime"]
 min-redis-compatibility-version-7-4 = ["redis-module/min-redis-compatibility-version-7-4"]

build.rs

@@ -14,14 +14,6 @@ impl ParseCallbacks for RedisModuleCallback {
             Some(IntKind::U64)
         } else if name.starts_with("REDISMODULE_REPLY") {
             Some(IntKind::I32)
-        } else if name == "REDISMODULE_LIST_HEAD"
-            || name == "REDISMODULE_LIST_TAIL"
-        {
-            // These values are used as `enum` discriminants, and thus must be `isize`.
-            Some(IntKind::Custom {
-                name: "isize",
-                is_signed: true,
-            })
         } else if name.starts_with("REDISMODULE_NOTIFY_") {
             Some(IntKind::Int)
         } else {

examples/scan_keys.rs

@@ -11,42 +11,6 @@ use std::cell::RefCell;
 // `scan_key_it` always copies the field and value strings, while `scan_key_fe` uses references to the field and value strings. In that example
 // both implementations need to clone the strings, because we want to return them as an array of RedisString.
 //
-// ## Usage examples for scan_key:
-// 
-// First we need to setup a hash key with some fields and values:
-// 
-// ```
-// HSET user:123 name Alice age 29 location Austin
-// ```
-// 
-// We need to start redis-server with the module loaded (example for macOS, adjust path as needed):
-// 
-// ```
-// redis-server --loadmodule ./target/debug/examples/libscan_keys.dylib
-// ```
-// 
-// Afterwards we can call the commands via redis-cli:
-// 
-// ```
-// > SCAN_KEYS
-// 1) "user:123"
-// 
-// > SCAN_KEY_IT user:123
-// 1) "name"
-// 2) "Alice"
-// 3) "age"
-// 4) "29"
-// 5) "location"
-// 6) "Austin"
-// 
-// > SCAN_KEY_FE user:123
-// 1) "name"
-// 2) "Alice"
-// 3) "age"
-// 4) "29"
-// 5) "location"
-// 6) "Austin"
-// ```
 
 use redis_module::{
     key::{KeyFlags, RedisKey}, redis_module, Context, KeysCursor, RedisError, RedisResult, RedisString, RedisValue, ScanKeyCursor
@@ -77,16 +41,10 @@ fn scan_key_foreach(ctx: &Context, args: Vec<RedisString>) -> RedisResult {
 
     let key_name = &args[1];
     let key = ctx.open_key_with_flags(key_name, KeyFlags::NOEFFECTS | KeyFlags::NOEXPIRE | KeyFlags::ACCESS_EXPIRED );
-    let ty = key.key_type();
     let cursor  = ScanKeyCursor::new(key);
-    let out = format!("Cursor created for Scanning key: {}, type={:?}", key_name, ty);
-    ctx.log_notice(&out);
-
+    
     let res = RefCell::new(Vec::new());
     cursor.foreach(|_key, field, value| {
-        let out = format!("Field: {}, Value: {}", field, value);
-        ctx.log_notice(&out);
-
         let mut res = res.borrow_mut();
         res.push(RedisValue::BulkRedisString(field.clone()));
         res.push(RedisValue::BulkRedisString(value.clone()));
@@ -106,17 +64,13 @@ fn scan_key_iterator(ctx: &Context, args: Vec<RedisString>) -> RedisResult {
     let key_name = &args[1];
     let mut res = Vec::new();
     let key = ctx.open_key_with_flags(key_name, KeyFlags::NOEFFECTS | KeyFlags::NOEXPIRE | KeyFlags::ACCESS_EXPIRED );
-    let ty = key.key_type();
     let cursor  = ScanKeyCursor::new(key);
-    let out = format!("Cursor created for Scanning key: {}, type={:?}", key_name, ty);
-    ctx.log_notice(&out);
-
+    
     for (no, (field, value)) in cursor.iter().enumerate() {
-        let out = format!("It: {}, Field: {}, Value: {}", no, field, value);
-        ctx.log_notice(&out);
         res.push(RedisValue::BulkRedisString(field));
         res.push(RedisValue::BulkRedisString(value));
     }
+    
     Ok(RedisValue::Array(res))
 }
 

src/context/key_scan_cursor.rs

@@ -8,22 +8,23 @@ use crate::{key::RedisKey, raw, Context, RedisString};
 /// A cursor to scan field/value pairs of a (hash) key.
 ///
 /// This is a wrapper around the [RedisModule_ScanKey](https://redis.io/docs/latest/develop/reference/modules/modules-api-ref/#redismodule_scankey) 
-/// function from the C API. It provides access via [`ScanKeyCursor::foreach`] and provides a Rust iterator via [`ScanKeyCursor::iter`].
+/// function from the C API. It provides access via a closure given to [`ScanKeyCursor::foreach`] and alternatively 
+/// provides a Rust iterator via [`ScanKeyCursor::iter`].
 /// 
-/// Use the former if the operation can be performed in the callback, as it is more efficient. Use the latter if you need to collect the results and/or
+/// Use `foreach` if the operation requires no copies and high performance. Use the iterator variant if you need to collect the results and/or
 /// want to have access to the Rust iterator API.
 ///
 /// ## Example usage
 /// 
 /// Here we show how to extract values to communicate them back to the Redis client. We assume that the following hash key is setup:
 /// 
-/// ```no_run
+/// ```text
 /// HSET user:123 name Alice age 29 location Austin
 /// ```
 /// 
 /// For using the `foreach` method:
 /// 
-/// ```no_run
+/// ```ignore
 /// fn example_scan_key_foreach(ctx: &Context) -> RedisResult {
 ///    let key = ctx.open_key_with_flags("user:123", KeyFlags::NOEFFECTS | KeyFlags::NOEXPIRE | KeyFlags::ACCESS_EXPIRED );
 ///    let cursor  = ScanKeyCursor::new(key);
@@ -41,7 +42,7 @@ use crate::{key::RedisKey, raw, Context, RedisString};
 /// 
 /// For using the `iter` method:
 /// 
-/// ```no_run
+/// ```ignore
 /// fn example_scan_key_foreach(ctx: &Context) -> RedisResult {
 ///     let mut res = Vec::new();
 ///     let key = ctx.open_key_with_flags("user:123", KeyFlags::NOEFFECTS | KeyFlags::NOEXPIRE | KeyFlags::ACCESS_EXPIRED );
@@ -70,16 +71,18 @@ pub struct ScanKeyCursor {
 }
 
 impl ScanKeyCursor {
+    /// Creates a new scan cursor for the given key.
     pub fn new(key: RedisKey) -> Self {
         let inner_cursor = unsafe { raw::RedisModule_ScanCursorCreate.unwrap()() };
         Self { key, inner_cursor }
     }
 
+    /// Restarts the cursor from the beginning.
     pub fn restart(&self) {
         unsafe { raw::RedisModule_ScanCursorRestart.unwrap()(self.inner_cursor) };
     }
 
-    /// Implements a callback based foreach loop over all fields and values in the hash key, use for optimal performance.
+    /// Implements a callback based foreach loop over all fields and values in the hash key, use that for optimal performance.
     pub fn foreach<F: Fn(&RedisKey, &RedisString, &RedisString)>(&self, f: F) {
         // Safety: Assumption: c-side initialized the function ptr and it is is never changed,
         // i.e. after module initialization the function pointers stay valid till the end of the program.
@@ -98,9 +101,12 @@ impl ScanKeyCursor {
         }
     }
 
+    /// Returns an iterator over all fields and values in the hash key. 
+    /// 
+    /// As the rust loop scope is detached from the C callback
+    /// we need to buffer the field/value pairs. That has performance implications. They are lower if the field/value pairs are
+    /// copied anyway, but even in that case not as fast as using the [`ScanKeyCursor::foreach`] method.
     pub fn iter(&self) -> ScanKeyCursorIterator<'_> {
-        let ctx = Context::new(self.key.ctx);
-        ctx.log_notice("Starting ScanKeyCursor iteration");
         ScanKeyCursorIterator {
             cursor: self,
             buf: Vec::new(),
@@ -117,6 +123,7 @@ impl Drop for ScanKeyCursor {
 
 pub type ScanKeyIteratorItem = (RedisString, RedisString);
 
+/// An iterator (state) over the field/value pairs of a hash key.
 pub struct ScanKeyCursorIterator<'a> {
     /// The cursor that is used for the iteration
     cursor: &'a ScanKeyCursor,
@@ -199,9 +206,10 @@ impl ScanKeyCursorIterator<'_> {
     }
 }
 
-/// The callback that is called by `RedisModule_ScanKey` to return the field and value strings.
+/// The callback that is used by [`ScanKeyCursor::foreach`] as argument to `RedisModule_ScanKey`.
 ///
-/// The `data` pointer is a stack slot of type `RawData` that is used to pass the data back to the iterator.
+/// The `data` pointer is the closure given to [`ScanKeyCursor::foreach`] and the callback forwards
+/// references to the key, field and value to that closure.
 unsafe extern "C" fn foreach_callback<F: Fn(&RedisKey, &RedisString, &RedisString)>(
     key: *mut raw::RedisModuleKey,
     field: *mut raw::RedisModuleString,
@@ -224,43 +232,37 @@ unsafe extern "C" fn foreach_callback<F: Fn(&RedisKey, &RedisString, &RedisStrin
     key.take(); // we're not the owner of the key either
 }
 
-/// The callback that is called by `RedisModule_ScanKey` to return the field and value strings.
-///
-/// The `data` pointer is a stack slot of type `RawData` that is used to pass the data back to the iterator.
+/// The callback that is used inside the iterator variant accessible via [`ScanKeyCursor::iter`] for `RedisModule_ScanKey`. 
+/// 
+/// It buffers copies of the field and value strings as the lifetime of them ends when with the call
+/// to `RedisModule_ScanKey` going out of scope.
+/// 
+/// The `data` pointer is a stack slot of type [StackSlot] that is used to pass the data back to the iterator.
 unsafe extern "C" fn iterator_callback(
     _key: *mut raw::RedisModuleKey,
     field: *mut raw::RedisModuleString,
     value: *mut raw::RedisModuleString,
     data: *mut c_void,
 ) {
-    // SAFETY: this is the responsibility of the caller, see only usage below in `next()`
-    // `data` is a stack slot of type Data
+    // `data` is a stack slot
     let slot = data.cast::<StackSlot>();
     let slot = &mut (*slot);
 
-    // todo: use new-type with refcount handling for better performance
+    // todo: use new-type with refcount handling for better performance, otherwise we have to copy at this point
+    // we know that this callback will be called in a loop scope and that we 
+    // need to store the RedisString longer than the ScanKey invocation but not much
+    // longer and in case of batched results we don't need to store everything in memory
+    // but only the last batch.
     let field = raw::RedisModule_CreateStringFromString.unwrap()(slot.ctx.get_raw(), field);
     let value = raw::RedisModule_CreateStringFromString.unwrap()(slot.ctx.get_raw(), value);
 
     let field = RedisString::from_redis_module_string(slot.ctx.get_raw(), field);
     let value = RedisString::from_redis_module_string(slot.ctx.get_raw(), value);
 
-    if slot.buf.is_empty() {
-        let out = format!("CB - Value tu return - Field: {}, Value: {}", field, value);
-        slot.ctx.log_notice(&out);
-    } else {
-        // This is the case where the callback is called multiple times.
-        // We need to buffer the data in the iterator state.
-        let out = format!(
-            "CB - Buffer for future use - Field: {}, Value: {}",
-            field, value
-        );
-        slot.ctx.log_notice(&out);
-    }
     slot.buf.push((field, value));
 }
 
-// Implements an iterator for `KeysCursor` that yields (RedisKey, *mut RedisModuleString, *mut RedisModuleString) in a Rust for loop.
+// Implements an iterator for `KeysCursor` that yields (RedisString, RedisString) in a Rust for loop.
 // This is a wrapper around the RedisModule_ScanKey function from the C API and uses a pattern to get the values from the callback that
 // is also used in stack unwinding scenarios. There is not common term for that but here we can think of it as a "stack slot" pattern.
 impl Iterator for ScanKeyCursorIterator<'_> {

src/key.rs

@@ -110,8 +110,7 @@ impl RedisKey {
     /// Will panic if `RedisModule_KeyType` is missing in redismodule.h
     #[must_use]
     pub fn key_type(&self) -> raw::KeyType {
-        let discriminant = unsafe { raw::RedisModule_KeyType.unwrap()(self.key_inner) };
-        KeyType::from_repr(discriminant as u32).unwrap()
+        unsafe { raw::RedisModule_KeyType.unwrap()(self.key_inner) }.into()
     }
 
     /// Detects whether the key pointer given to us by Redis is null.
@@ -380,8 +379,7 @@ impl RedisKeyWritable {
     /// Will panic if `RedisModule_KeyType` is missing in redismodule.h
     #[must_use]
     pub fn key_type(&self) -> raw::KeyType {
-        let discriminant = unsafe { raw::RedisModule_KeyType.unwrap()(self.key_inner) };
-        KeyType::from_repr(discriminant as u32).unwrap()
+        unsafe { raw::RedisModule_KeyType.unwrap()(self.key_inner) }.into()
     }
 
     pub fn open_with_redis_string(
@@ -672,7 +670,7 @@ fn to_raw_mode(mode: KeyMode) -> raw::KeyMode {
 /// Will panic if `RedisModule_KeyType` or `RedisModule_ModuleTypeGetType` are missing in redismodule.h
 #[allow(clippy::not_unsafe_ptr_arg_deref)]
 pub fn verify_type(key_inner: *mut raw::RedisModuleKey, redis_type: &RedisType) -> RedisResult {
-    let key_type: KeyType = KeyType::from_repr(unsafe { raw::RedisModule_KeyType.unwrap()(key_inner) as u32 }).unwrap();
+    let key_type: KeyType = unsafe { raw::RedisModule_KeyType.unwrap()(key_inner) }.into();
 
     if key_type != KeyType::Empty {
         // The key exists; check its type

src/raw.rs

@@ -29,13 +29,6 @@ bitflags! {
     pub struct KeyMode: c_int {
         const READ = REDISMODULE_READ as c_int;
         const WRITE = REDISMODULE_WRITE as c_int;
-
-        const KEYOPEN_NOTOUCH = REDISMODULE_OPEN_KEY_NOTOUCH as c_int;
-        const KEYOPEN_NONOTIFY = REDISMODULE_OPEN_KEY_NONOTIFY as c_int;
-        const KEYOPEN_NOSTATS = REDISMODULE_OPEN_KEY_NOSTATS as c_int;
-        const KEYOPEN_NOEFFECTS = REDISMODULE_OPEN_KEY_NOEFFECTS as c_int;
-        const KEYOPEN_NOEXPIRE = REDISMODULE_OPEN_KEY_NOEXPIRE as c_int;
-        const KEYOPEN_ACCESSEXPIRED = REDISMODULE_OPEN_KEY_ACCESS_EXPIRED as c_int;
     }
 }
 
@@ -60,7 +53,14 @@ pub enum KeyType {
     Stream = REDISMODULE_KEYTYPE_STREAM,
 }
 
-#[derive(Primitive, Debug, PartialEq, Eq)]
+impl From<c_int> for KeyType {
+    fn from(v: c_int) -> Self {
+        Self::from_i32(v).unwrap()
+    }
+}
+
+#[repr(u32)]
+#[derive(Primitive, Debug, PartialEq, Eq, strum::FromRepr)]
 pub enum Where {
     ListHead = REDISMODULE_LIST_HEAD,
     ListTail = REDISMODULE_LIST_TAIL,

src/redismodule.rs

@@ -273,10 +273,6 @@ impl RedisString {
 impl Drop for RedisString {
     fn drop(&mut self) {
         if !self.inner.is_null() {
-            let ctx = Context::new(self.ctx);
-            let out = format!("Dropping RedisString '{}'", self.to_string_lossy());
-            ctx.log_notice(&out); 
-
             unsafe {
                 raw::RedisModule_FreeString.unwrap()(self.ctx, self.inner);
             }

tests/integration.rs

@@ -200,6 +200,41 @@ fn test_scan() -> Result<()> {
     Ok(())
 }
 
+#[test]
+fn test_scan_key_it() -> Result<()> {
+    let mut con = TestConnection::new("scan_keys");
+
+    redis::cmd("hset")
+        .arg(&[
+            "user:123", "name", "Alice", "age", "29", "location", "Austin",
+        ])
+        .query::<()>(&mut con)
+        .with_context(|| "failed to hset")?;
+
+    let res: Vec<String> = redis::cmd("scan_key_it")
+        .arg(&["user:123"])
+        .query(&mut con)?;
+    assert_eq!(&res, &["name", "Alice", "age", "29", "location", "Austin"]);
+    Ok(())
+}
+
+#[test]
+fn test_scan_key_foreach() -> Result<()> {
+    let mut con = TestConnection::new("scan_keys");
+    redis::cmd("hset")
+        .arg(&[
+            "user:123", "name", "Alice", "age", "29", "location", "Austin",
+        ])
+        .query::<()>(&mut con)
+        .with_context(|| "failed to hset")?;
+
+    let res: Vec<String> = redis::cmd("scan_key_fe")
+        .arg(&["user:123"])
+        .query(&mut con)?;
+    assert_eq!(&res, &["name", "Alice", "age", "29", "location", "Austin"]);
+    Ok(())
+}
+
 #[test]
 fn test_stream_reader() -> Result<()> {
     let mut con = TestConnection::new("stream");