Fix string verbatim reply to work correctly. #315
Conversation
src/context/call_reply.rs
Outdated
| let mut res = VerbatimStringFormat::default(); | ||
| value | ||
| .chars() | ||
| .into_iter() | ||
| .take(3) | ||
| .enumerate() | ||
| .for_each(|(i, c)| res.0[i] = c as c_char); | ||
| res |
There was a problem hiding this comment.
I have a few doubts about this. So, the value is guaranteed to be a valid UTF-8 string, which means it also may contain some ASCII symbols inside. However, when we do c as c_char within the for_each, we don't check if the symbol converted was an ASCII symbol. The "valid" symbol table for ASCII ends at the 127th (half-byte) symbol, and after that, there is the extended table which varies, and we shouldn't rely on it. As we convert a char of variable byte-width to a c_char of the constant length of 1 byte, we don't check that the actual value is correct. What should we do if the format string contains a non-valid ASCII-table symbol (the symbol with code > 127)?
There was a problem hiding this comment.
So do you suggested to change the From into TryFrom and verify that the value is less eq 127?
There was a problem hiding this comment.
@vityafx done
src/context/call_reply.rs
Outdated
| fn try_from(value: &str) -> Result<Self, Self::Error> { | ||
| if value.len() != 3 { | ||
| return Err(RedisError::String( | ||
| "Verbatim format len must be 3".to_owned(), |
There was a problem hiding this comment.
As this is already a String, it can as well be a formatted string with the value we have:
format!("Verbatim format length must be {VERBATIM_FORMAT_LENGTH}.")
Current behaviour is to append to format to the data. This is wrong because the format need to pass separately to Redis. Also, a wrong RedisModuleAPI was used, we need to use the API that allows to pass the format (otherwise Redis uses the default
txtformat).