Re: [boost] aedis review starts today

On Sun, Jan 15, 2023 at 9:17 AM Klemens Morgenstern via Boost wrote:

[snip]

Review ========

Please explicitly state that you either *accept* or *reject* the inclusion of this library into boost. Also please indicate the time & effort spent on the evaluation and give the reasons for your decision.

The review is open to anyone who is prepared to put in the work of evaluating and reviewing the library. Prior experience in contributing to Boost reviews is not a requirement.

Some questions to consider:

- Does this library bring real benefit to C++ developers for real world use-case?

I think so, yes.

- Do you have an application for this library?

I do not.

- Does the API match with current best practices?

No. In particular, the API requires the user to write lots of Redis strings by hand. A typo will not be detected until runtime. The set of Redis keywords is fixed; there's no reason not to make an enumeration of them all, like this: enum class cmd { ACL_CAT, // List the ACL categories or the commands inside a category ACL_DELUSER, // Remove the specified ACL users and the associated rules ACL_DRYRUN, // Returns whether the user can execute the given command // without executing the command. ACL GENPASS, // Generate a pseudorandom secure password to use for ACL users ACL_GETUSER, // Get the rules for a specific ACL user ACL_LIST, // List the current ACL rules in ACL config file format ACL_LOAD, // Reload the ACLs from the configured ACL file ACL_LOG, // List latest events denied because of ACLs in place // ... etc. }; I actually did all of them to see how much effort it would take. It took me about 20 or 30 minutes, working from the list on the Redis website (I just copy/pasted the commands listed at https://redis.io/commands/). Further, when enqueuing commands, there's no checking that a command and its arguments make sense together. For instance, here is some example code from the docs: resp3::request req; req.push("HELLO", 3); req.push("HGETALL", "hset-key"); req.push("QUIT"); If I change that last line to: req.push("QUITE"); or to: req.push("QUIT", "oops"); or even to: auto it = 42, getting = 13; req.push("QUIT", this, is, getting, "ridiculous"); ... the code is well formed, but nonsensical. I'd like the lib to know that "QUITE" isn't a thing, and that "QUIT" takes no arguments, and fail to compile if I mess any of this up via typo or copy/paste. One possible fix would be to change resp3::request::push*() to take a non-type template parameter that can be used to check the args: template void push(Ts const &... args) { static_assert(args_are_compatible_v); // Do the real work here... } template void push_range( Key const & key, ForwardIterator begin, ForwardIterator end, typename std::iterator_traits<ForwardIterator>::value_type * = nullptr) { static_assert(key_and_range_are_compatible_v< command, std::remove_cvref_t<Key>, ForwardIterator>); // Do the real work here... } A more thorough-going change that I would like even more would be this: template struct command_t { static constexpr cmd command = c; command_t(Params &&... params) params_{(Params &&) params} {} std::tuple params_; }; std::string_view string_name(cmd c) { return /* mapping from cmd to strings must exist somewhere, obvs. */; } // value_or_reference_wrapper_t should produce a reference_wrapper<T> when // std::is_reference_v<T> is true, and T otherwise. IOW, move rvalue // references to small things like ints and std::string_views into the // command_t, and refer to lvalue references like std::vector<int> const & // (examples below). inline constexpr struct HELLO_t { using return_type = /* whatever HELLO returns from Redis, as a C++ type */; // HELLO might need multiple overloads, and/or to derive from command_t<...> // (see below); the https://redis.io/commands description says it takes optional parameters. auto operator()(int param) { return command_t(std::move(param)); } } HELLO; inline constexpr struct HGETALL_t { using return_type = /* whatever HGETALL returns from Redis, as a C++ type */; auto operator()(std::string_view param) { return command_t(std::move(param)); } } HGETALL; // "nullary" commands should inherit from command_tcmd::NAME to avoid // superfluous parens at the call site. inline constexpr struct QUIT_t : command_tcmd::QUIT { using return_type = /* whatever QUIT returns from Redis, as a C++ type */; } QUIT; // etc. All the commands need this treatment. If you want to keep the result // type flexible, you can make return_type a kind-enumerator instead. For // instance, instead of std::vector<int> it might be // return_kind::number_array, where return_kind is some enumeration. The // kind-enumumerator would need to be static constexpr. Though this looks // like a lot of work, you can pretty easily make macros that do all this. template void resp3::request::push(command_t command) { // Process the command as the code previously did, except now the params // *must* match the command, because of how the command_t is constructed // at the call site. } void user_code_1() { resp3::request req; req.push(HELLO(3)); req.push(HGETALL("hset-key")); req.push(QUIT); } // Even better, we can write this directly: void user_code_v2() { // No need to allocate a request object for smallish requests; you only // need to allocate the underlying buffer that the bytes are serialized // into. If you insist, you can always overload with an initial allocator // param. co_await conn->async_exec_request(HELLO(3), HGETALL("hset-key"), QUIT); } void user_code_2() { using exec_resp_type = std::tuple< std::optionalstd::string, // get std::optionalstd::string>, // lrange std::optional> // hgetall >; std::tuple< aedis::ignore, // multi aedis::ignore, // get aedis::ignore, // lrange aedis::ignore, // hgetall exec_resp_type, // exec > resp; // Since each command is a single object, we can also do this: co_await conn->async_exec_transaction( MULTI, GET("key1"), LRANGE("key2", 0, -1), HGETALL("key3"), EXEC, resp); // resp might need to be the first param, but you get the idea. // As before, we don't actually need to allocate a request object, just // the buffer of bytes that it would normally fill. // We can also use the statically-available return type (or return kind // enumerator) of each arg, and knowledge of the fact that this is a transaction, to // static_assert that the type of resp is compatible. } // Even better, we can use the fact that resp is the only meaningful value to // allow the user to write this instead: void user_code_2_v2() { using exec_resp_type = std::tuple< std::optionalstd::string, // get std::optionalstd::string>, // lrange std::optional> // hgetall >; exec_resp_type resp; co_await conn->async_exec_transaction( MULTI, GET("key1"), LRANGE("key2", 0, -1), HGETALL("key3"), EXEC, resp); } These are the kinds of easy-to-use-correctly and hard-to-use-incorrectly interfaces that I would expect of such a library. I think the marshalling/unmarshalling and handling of ASIO parts of the existing design make my life easier, but with raw string literals in the interface, I consider the library interface work only half-done.

- Is the documentation helpful and clear?

It is geared more towards users who already know all about Redis, and it is more reference-oriented. There is a real lack of the tutorial-style docs that Boost is known for. Boost reaches a really wide audience. There are a lot of people who might not be very DB-savvy, who might want to use Aedis, since "Hey, I already have Boost, why not use this DB library?" There are no docs for such users. For Redis aficionados, there should also be a step-by-step guide to how to do real work with the lib. The examples seem pretty good. I think one or more of them need to be integrated into the docs as a tutorial. There are lots of examples of other Boost libs that do this. The reference documentation is often too spare. For instance: https://mzimbres.github.io/aedis/classaedis_1_1resp3_1_1request.html#a9951b1... I must convert data to a string. Are there any requirements on the string? Can it have internal nulls, characters requiring escaping, etc.? I don't necessarily think that the docs need to talk about this in the reference section, but since this is not covered in the also-spare tutorial section, how am I supposed to know what the string requirements are? The Serialization section in https://mzimbres.github.io/aedis/index.html is similarly problematic. This is the example: // User defined type. struct mystruct {...}; // Serialize it in to_bulk. void to_bulk(std::pmr::string& to, mystruct const& obj) { std::string dummy = "Dummy serializaiton string."; aedis::resp3::to_bulk(to, dummy); } I was baffled by this at first reading. Why is obj there? It is unused. If "Dummy serializaiton string." were replaced with /* stringify obj here */, the example makes sense. I did not understand that I was supposed to do that until I read the reference mentioned earlier. There are a lot of things like this in the docs -- more than I have time to list. I don't think the docs are up to the Boost standard.

- Did you try to use it? What problems or surprises did you encounter?

I did not.

- What is your evaluation of the implementation?

I did not look at much of the implementation. What little I did see seemed reasonable.

Additionally, stating your knowledge of redis and asio would be helpful for me.

I've used ASIO a lot, and never used Redis. I vote to REJECT. If the library could be refactored so that the user-facing interface was more friendly, I would change my vote. Zach