redis
Implement strongly typed error checking
Go-Redis should return errors that can be checked programmatically without relying on comparing error strings.
Expected Behavior
I can check errors from a client library with well-defined functions or type checks (generally using errors.Is or errors.As, or maybe a package function along the lines of os.IsExist)
Current Behavior
We have to check error strings vs. a hard coded string. See this test for an example: https://github.com/go-redis/redis/blob/f1d6bc91b7352cc9e33f50f2ea180def3f17c515/commands_test.go#L4644 And these uses in real code: https://github.com/search?q=%22BUSYGROUP+Consumer+Group+name+already+exists%22+language%3Ago&type=code
Possible Solution
This should be handled by go-redis, and the error created in such a way that you can programmatically check what kind of error it represents.
Context (Environment)
A colleague who is new to Go asked how she was supposed to handle different types of errors from go-redis aside from checking the error strings. Specifically, how to check if an error returned from XGroupCreateMkStream indicated the group already exists. I assured her that checking error strings isn't a thing anyone does anymore, and almost all libraries implement some kind of strongly typed error handling. However, it appears that go-redis provides no such way to do this.
Detailed Description
I can find a large number of projects in the wild with the hard coded string "BUSYGROUP Consumer Group name already exists" in their code in order to check if the error returned from go-redis indicates the group already exists. I hope we all know that's bad.
Even if there is no structured data returned from redis in this case, this kind of knowledge of the exact redis error strings is something that should be encapsulated in go-redis, so that external consumers can rely on this library to handle that for them, rather than having to bake low level redis strings into their application logic.
Not only did this take way too much time for us to figure out, but now we have low-level redis strings baked into our application code. If go-redis ever starts wrapping their error messages with added detail, our code will break.
Possible Implementation
// code in go-redis
// ErrExists indicates the thing you were trying to create already exists.
type ErrExists string
func (e ErrExists) Error() string { return string(e) }
const (
itemExistsMsg = "BUSYGROUP Consumer Group name already exists"
okMsg = "ok"
)
// CreateGroup creates a new group with the given name. If a group
// with that name already exists, the error returned will report
// true from errors.Is(ErrExist).
func CreateGroup(name string) error {
output := run("XGROUP", "create", name)
switch output {
case itemExistMsg:
return ErrExists(output)
case okMsg:
return nil
default:
// unknown response, treat as unknown error
return errors.New(output)
}
}
// consumer application code
err := redis.CreateGroup(name)
if errors.Is(err, redis.ErrExists) {
// group already exists
// notify the user
}
if err != nil {
// some other error, maybe retry?
return err
}
All Redis errors are returned as a single string, for example, BUSYGROUP Consumer Group name already exists. We could use the first word BUSYGROUP as a category, but it probably does not help much since you want a type in Go.
Overall the problem here is that there are hundreds of Redis commands and each of them may return multiple errors. Supporting all of those errors in go-redis sounds daunting and I am not sure how pleasant it will be for users to scroll over a list of possibly outdated Redis errors.
Only supporting common cases like redis.ErrExists may be a viable solution, but I don't know enough to say for sure. It definitely will require some work and scrutiny. On the other hand, checking strings.HasPrefix(err.Error(), "BUSYGROUP") is rather easy.
Perhaps we can sweeten the pill by extending redis.Error with some API like err.HasPrefix("BUSYGROUP"), but that is a marginal improvement at best...
Suggestions are welcome.
My main concern is that if the library doesn't do the work to analyze the return strings, then library consumers are forced to. And that's kinda why we use libraries in the first place, so we don't have to figure out the nitty gritty of the service communication.
I think there are some common cases that could be handled by the library by implementing the return code checking. Then they'd be implemented in one place, rather than making dozens or hundreds of users of this library do it themselves and repeating the work of everyone else. Things like already exists, not found, etc.
For the record here is a list of exception in redis-py: https://redis-py.readthedocs.io/en/stable/exceptions.html . The seem to handle some classes of problems, not individual errors.
For now I will add something like redis.HasErrorPrefix("BUSYGROUP") to beautify the code a bit. Still not sure about the rest.
I think there are some common cases that could be handled by the library by implementing the return code checking. Then they'd be implemented in one place, rather than making dozens or hundreds of users of this library do it themselves and repeating the work of everyone else. Things like already exists, not found, etc.
I totally agree. It is just that I don't see a good solution, but perhaps we just should start with something small?
@vmihailenco are we looking for PRs for fixing this more strongly by return error in form of codes, as suggested by @natefinch?
It would be nice if redis.Nil worked again for redis "Not Found" responses, even if typing errors is intractable.
