RedisLabsModules / redismodule-rs

Rust API for Redis Modules API
BSD 3-Clause "New" or "Revised" License
269 stars 64 forks source link

Is there an API for RedisKeyWritable to remove expiration for a key #275

Open QuChen88 opened 1 year ago

QuChen88 commented 1 year ago

I noticed that when I open a key for writing via context.open_key_writable(), I can only call set_expire() with a duration that is >= 0. However, I noticed the low level API raw::set_expire() takes the expiration timestamp as c_long_long which can be REDISMODULE_NO_EXPIRE to make the key not expire. In this case there doesn't seem to be a way for me to do this using the set_expire() API. See https://github.com/RedisLabsModules/redismodule-rs/blob/cc0532783ab9ff2302ef819d7c7c8977ff69e801/src/key.rs#L269

i.e.

let key = ctx.open_key_writable(key);
match key.set_expire(Duration::new(5, 0)) {
     Ok(v) => Ok(v),
     Err(e) => Err(e)
}

Is there a way for me to remove the expiration of the key using the RedisKeyWritable APIs? I tried to call the raw::set_expire() method but it seems like it needs RedisModuleKey as parameter. But the RedisModuleKey field inside RedisKeyWritable is private and can't be accessed. See https://github.com/RedisLabsModules/redismodule-rs/blob/cc0532783ab9ff2302ef819d7c7c8977ff69e801/src/key.rs#L169

QuChen88 commented 1 year ago

Submitted PR to address this https://github.com/RedisLabsModules/redismodule-rs/pull/276

oshadmi commented 1 year ago

@gkorland @MeirShpilraien Another option instead of adding a remove_expire method (in PR #276) is to change the signature of set_expire to receive a c_longlong instead of Duration, so the value REDISMODULE_NO_EXPIRE could be passed as well, which will allow to remove expiration. Currently Duration is unsigned, and when receiving values beyond i64, i64::try_from will fail. Changing to c_longlong would fail in build time instead of run time with values beyond i64. However since changing the API (to be stricter) is a breaking change it can only be done in a major version, so we should also add a new remove_expire method. Agree?

BTW, this would not entirely prevent failures even with c_longlong value, but would narrow down the current range of erroneous values (i64::try_from would still be able to fail since value is multiplied by 1000 when converting to nanoseconds)