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

Incorrect Return Type from Context::call() #234

Open
phartleyp opened this issue Jun 15, 2022 · 2 comments
Open

Incorrect Return Type from Context::call() #234

phartleyp opened this issue Jun 15, 2022 · 2 comments

Comments

@phartleyp
Copy link
Contributor

The following Redis Module command simply wraps the Redis "GET" command and returns the value of the simple string key with the name "key". It contains a bug for some string values.

pub fn cmd_example(ctx: &Context, _args: Vec<RedisString>) -> RedisResult {
    ctx.call("GET", &["key"])
}
  • For single-line strings, it works fine.
  • For multi-line strings (i.e. those containing \r\n), it will cause the Redis client to fail. On the redis-py client, for example, the client hangs because RESP protocol is not respected.

Root cause is that Context::call() always returns strings as RedisValue::SimpleString. But strings containing \r\n should be RedisValue::BulkString.

@phartleyp
Copy link
Contributor Author

This is fixed by pull request #93.

There seems to be some hesitation by maintainers to implement the fix. Until then, here is a workaround that can be applied to the output of Context::call() that will work around this issue:

/// Remap `Context::call()` results to convert `SimpleString` into `BulkString`.
/// All other types are left alone.
pub fn fix_call_reply(result: RedisValue) -> RedisValue {
    match result {
        RedisValue::SimpleString(v) => RedisValue::BulkString(v),
        RedisValue::Array(a) => {
            RedisValue::Array(a.into_iter().map(|v| fix_call_reply(v)).collect())
        }
        v @ _ => v,
    }
}

@oshadmi
Copy link
Contributor

oshadmi commented Jun 15, 2022

Related #200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants