Get on non-existent key returns wrong error

[mercxry]

Hello, when I try to get a non-existent key I receive this error: Redis Error - kind: Parse, details: Cannot convert to number., but I believe this is the wrong type of error for the triggered action.

What I would expect

Since the Redis CLI returns nil maybe the library should just return an Option::None, or a different kind of RedisError like NotFound.

Personally I think both solutions are right and in an ideal world there should be an option for both behaviors, but for my use case a different kind of error it's preferred since I want to do a specific action on specific errors and the error propagation operator (?) would help me achieve this more simply and in a cleaner way.

[aembke]

Hi @mercxry

Can you include the code you're using to call GET? I'm curious what return type you're specifying for the output.

[mercxry]

My bad, without type casting it was i64, but I just tried manually using Option<i64> and it returns None like I was expecting.

I don't know if it would fit in your design choices, but would it be possible to return a NotFound error in case the type it's not an Option?

[aembke]

Actually reading this a bit more I think I understand better what you're saying. I agree that the Option<T> scenario is unique in that if a caller specifies that a non-nullable type is returned, yet redis returns a nil, that this should be handled differently than a "normal" parse error.

I'll look into this, this is good feedback.

[mercxry]

Perfect! Let me know if you need help implementing it, I started using the library recently but I already prefer it over the other Redis libraries.

Thanks for the awesome work :)

[aembke]

Thanks for the kind words @mercxry, I hope this library helps.

Just as a heads up, there's one or two type conversion implementations that might be somewhat controversial.

As it stands right now trying to cast a nil response from the server to a String will return an error, but I can see a somewhat compelling argument for maybe changing this to return "nil" (not an error) since on the surface that seems like a (maybe) sane type conversion. There's a similar story for bool where maybe on surface nil should be cast to false.

That being said, I'm going to make these use cases return a NotFound error to be more consistent with the scalar types, and to prevent cases where callers think a value exists but don't notice it doesn't until it causes some other issues somewhere else in their code. If folks want the string "nil", or false, they can always manually cast the value to a string or bool, or use Option<T> as the return type to remove any ambiguity.

That kind of thing also smells a lot like JavaScript and I'd like to avoid that as much as possible.

[aembke]

I just shipped some changes in 4.2.3 that I think should help here.

Let me know if this matches your use case and if so I'll close this out.

Thanks for the good feedback @mercxry, getting a sense of real use cases like this is really helpful.

[mercxry]

Awesome! I just tried the new version and the error kind shows up as NotFound when the type it's not an Option<T> as intended, but I noticed that the details are the same of the Parse error kind: Redis Error - kind: NotFound, details: Cannot convert nil to number.

It should say something related to the NotFound error kind, something along the lines Cannot find the specified key, but the new kind works perfectly fine with my error match, thanks for the speedy development of it!

[aembke]

Yeah I should have mentioned that in the last comment. I wanted to scope this change to just the error type variant since I'm in the middle of a major release (v5) that makes some drastic changes to errors and return values (RESP3 has value attributes, which are a big change to everything).

I'll include your feedback here re: error messages in the changes in that release, but for now I'm hoping to limit changes on the 4.x branch to be as small as possible.

[aembke]

Just so I don't forget I'm going to leave this open with a feature request tag as well.

[aembke]

Just a heads up @mercxry - I've been doing some more thinking on this and I think I'm going to keep the current behavior from 4.x. This will keep the NotFound error kind variant, but will not change the error message.

The type conversion logic is used in several places (such as the convert function on RedisValue) where it may not make sense to return a message indicating that a key is not found since there may not be any request in play. I'm hoping the type conversion logic can be useful outside the context of just converting a response type after an await point, and so I think it makes sense to keep the more targetted type-specific message over a generic "Could not find specified key" message.

There's also some cases (such as HGET) where "key" is the wrong term and "field" may be more precise, but I'd rather not try to enumerate all of those cases. Since the error kind variant is still there callers can still match against something to check for the NotFound case. It's not perfect, but there's a lot of potential ambiguity with generic code like this that is used for more than just response value conversion. Hopefully though the current behavior strikes a good enough balance that callers can make it work.

I might come back to this in a later version, but for now I think I'm going to keep the current message and error kind variants.