Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to delete hash fields #130

Merged
merged 1 commit into from
Mar 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,10 @@ impl RedisKeyWritable {
raw::hash_set(self.key_inner, field, value.inner)
}

pub fn hash_del(&self, field: &str) -> raw::Status {
raw::hash_del(self.key_inner, field)
}

pub fn hash_get(&self, field: &str) -> Result<Option<RedisString>, RedisError> {
Ok(hash_mget_key(self.ctx, self.key_inner, &[field])?
.pop()
Expand Down
24 changes: 24 additions & 0 deletions src/raw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ extern "C" {

pub const FMT: *const c_char = b"v\0".as_ptr() as *const c_char;

// REDISMODULE_HASH_DELETE is defined explicitly here because bindgen cannot
// parse typecasts in C macro constants yet.
// See https://github.com/rust-lang/rust-bindgen/issues/316
pub const REDISMODULE_HASH_DELETE: *const RedisModuleString = 1 as *const RedisModuleString;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to figure out why this was needed -- I expected bindgen to generate this automatically.
It turns out it doesn't because it can't parse the typecast, see rust-lang/rust-bindgen#316.
Please add a comment here that mentions this.


// Helper functions for the raw bindings.

pub fn call_reply_type(reply: *mut RedisModuleCallReply) -> ReplyType {
Expand Down Expand Up @@ -348,6 +353,25 @@ pub fn hash_set(key: *mut RedisModuleKey, field: &str, value: *mut RedisModuleSt
}
}

pub fn hash_del(key: *mut RedisModuleKey, field: &str) -> Status {
let field = CString::new(field).unwrap();

// TODO: Add hash_del_multi()
// Support to pass multiple fields is desired but is complicated.
// See hash_get_multi() and https://github.com/redis/redis/issues/7860

unsafe {
RedisModule_HashSet.unwrap()(
Copy link
Contributor Author

@prashanthpai prashanthpai Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the RedisModule_HashSet C API has been overloaded with multiple things and it does a lot more than merely setting a hash value.

I'm very new to Rust. I don't know where to draw the line between this project providing a user-friendly Rust API to modules vs this project being a very thin and pass-through wrapper around the C modules API. Does it make sense to extend hash_set to take a additional flags/options instead?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aim of this module is to provide an idiomatic Rust API for the Redis modules API. Using flag arguments to fundamentally change the functionality of a method isn't very common even in C, doubly so in Rust; so I think the approach you took here is the right one.

One thing I would change is rename all the instances of hash_del_single to simply hash_del, to match the existing style of hash_set/hash_get.

Oh, and thank you for the PR!

key,
REDISMODULE_HASH_CFIELDS as i32,
field.as_ptr(),
REDISMODULE_HASH_DELETE,
ptr::null::<c_char>(),
)
.into()
}
}

// Returns pointer to the C string, and sets len to its length
pub fn string_ptr_len(s: *mut RedisModuleString, len: *mut size_t) -> *const c_char {
unsafe { RedisModule_StringPtrLen.unwrap()(s, len) }
Expand Down