Re: aedis review starts today

From: Marcelo Zimbres Silva (mzimbres_at_[hidden])
Date: 2023-01-22 07:43:12

On Sat, 21 Jan 2023 at 20:00, Ruben Perez <rubenperez038_at_[hidden]> wrote:
>
> I like how adapt() works. Parsing into compile-time structures is
> always good.  However, as a Redis novice I am, I've had a hard time
> debugging things when I got a mismatch between the type passed to
> adapt() and what the command expects.  The error happens at parse
> time, which I think is pretty dangerous, as it's very easy to get
> things overlooked.

Solving this would require knowing at compile time what data-structure
each command returns at compile time, but this is the kind of
information that is only available in the documentation of each
command (IIRC not even in a form I could use in a generator).

As an alternative I could lock-in users on resp3 types: maps, sets,
etc. But then how to represent data structures that have no
corresponding C++ types. It would also be inefficient as it would
require first copying and then converting.

In other words, this should be addressed by a higher level library.

> As an example of this, the library's own example cpp20_containers,
> in the hgetall function, uses the type std::map<std::string,
> std::string> for the hgetall response, which will be okay unless the
> key is not set, in which case you will get an error. I think this
> has an enormous potential for causing hidden bugs (and even
> vulnerabilities). I've also noticed that if you mismatch the sizes
> of the pipeline request and response, you get an assertion (so UB).

To avoid this you can wrap the response type in std::optional, for example

std::optional<std::map<std::string, std::string>>

I don't lock users in this behaviour because they might not want it.
Perhaps it is desirable to have an error instead of having to check
whether the optional has a value. The user must know what he needs.

Regards,
Marcelo