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.

Sign up for GitHub

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

Adding a wrong type error message when performing a bloom operation on a non bloom key #30

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Adding a wrong type error message when performing a bloom operation on a non bloom key #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 7 additions & 7 deletions src/bloom/command_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ pub fn bloom_filter_add_value(
let value = match filter_key.get_value::<BloomFilterType>(&BLOOM_FILTER_TYPE) {
Ok(v) => v,
Err(_) => {
Copy link
Member

Choose a reason for hiding this comment

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

The best practice is to handle every case in a match. I understand that the SDK does not make this easy for us because they return a specific string message when an ENUM would have made matching easier.

In the future, if it is possible to update the SDK, we should be able to update our handling to make it stricter.

https://github.com/valkey-io/valkeymodule-rs/blob/0a1fb4e8e9b5cc3e26d8fcdbab0857c8bfd87f33/src/key.rs#L678

return Err(ValkeyError::Str(utils::ERROR));
return Err(ValkeyError::WrongType);
}
};
// Skip bloom filter size validation on replicated cmds.
Expand Down Expand Up @@ -173,7 +173,7 @@ pub fn bloom_filter_exists(
let value = match filter_key.get_value::<BloomFilterType>(&BLOOM_FILTER_TYPE) {
Ok(v) => v,
Err(_) => {
return Err(ValkeyError::Str(utils::ERROR));
return Err(ValkeyError::WrongType);
}
};
if !multi {
Expand Down Expand Up @@ -201,7 +201,7 @@ pub fn bloom_filter_card(ctx: &Context, input_args: &[ValkeyString]) -> ValkeyRe
let value = match filter_key.get_value::<BloomFilterType>(&BLOOM_FILTER_TYPE) {
Ok(v) => v,
Err(_) => {
return Err(ValkeyError::Str(utils::ERROR));
return Err(ValkeyError::WrongType);
}
};
match value {
Expand Down Expand Up @@ -270,7 +270,7 @@ pub fn bloom_filter_reserve(ctx: &Context, input_args: &[ValkeyString]) -> Valke
let value = match filter_key.get_value::<BloomFilterType>(&BLOOM_FILTER_TYPE) {
Ok(v) => v,
Err(_) => {
return Err(ValkeyError::Str(utils::ERROR));
return Err(ValkeyError::WrongType);
}
};
match value {
Expand Down Expand Up @@ -383,7 +383,7 @@ pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> Valkey
let value = match filter_key.get_value::<BloomFilterType>(&BLOOM_FILTER_TYPE) {
Ok(v) => v,
Err(_) => {
return Err(ValkeyError::Str(utils::ERROR));
return Err(ValkeyError::WrongType);
}
};
// Skip bloom filter size validation on replicated cmds.
Expand Down Expand Up @@ -451,7 +451,7 @@ pub fn bloom_filter_info(ctx: &Context, input_args: &[ValkeyString]) -> ValkeyRe
let value = match filter_key.get_value::<BloomFilterType>(&BLOOM_FILTER_TYPE) {
Ok(v) => v,
Err(_) => {
return Err(ValkeyError::Str(utils::ERROR));
return Err(ValkeyError::WrongType);
}
};
match value {
Expand Down Expand Up @@ -513,7 +513,7 @@ pub fn bloom_filter_load(ctx: &Context, input_args: &[ValkeyString]) -> ValkeyRe
Ok(v) => v,
Err(_) => {
// error
return Err(ValkeyError::Str(utils::ERROR));
return Err(ValkeyError::WrongType);
}
};
match filter {
Expand Down
25 changes: 25 additions & 0 deletions tests/test_basic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,3 +297,28 @@ def test_debug_cmd(self):
assert madd_scenario_object_digest != madd_default_object_digest
else:
madd_scenario_object_digest == madd_default_object_digest

def test_bloom_wrong_type(self):
# List of all bloom commands
bloom_commands = [
'BF.ADD key item',
'BF.EXISTS key item',
'BF.MADD key item1 item2 item3',
'BF.MEXISTS key item2 item3 item4',
'BF.INSERT key ITEMS item',
'BF.INFO key filters',
'BF.CARD key',
'BF.RESERVE key 0.01 1000',
]
client = self.server.get_new_client()
# Set the key we try to perform bloom commands on
client.execute_command("set key value")
# Run each command and check we get the correct error returned
for cmd in bloom_commands:
cmd_name = cmd.split()[0]
try:
result = client.execute_command(cmd)
assert False, f"{cmd_name} on existing non bloom object should fail, instead: {result}"
except Exception as e:

assert str(e) == f"WRONGTYPE Operation against a key holding the wrong kind of value"
Loading