Forwarding to the whole ML, I hit the wrong button. Thanks Marcelo for noting it. ---------- Forwarded message --------- From: Ruben Perez <rubenperez038@gmail.com> Date: Mon, 23 Jan 2023 at 14:04 Subject: Re: [boost] aedis review starts today To: Marcelo Zimbres Silva <mzimbres@gmail.com> Hi Marcelo, I'll try to answer to the several points you made in a single email.
To fix this properly I would need a tool similar to what protobuff is to grpc: generate requests and responses stub from a schema.
Aedis however is not trying to do that, it provides the infrastructure with which such a tool could be created. Perhaps there is no place in Boost for such a library, but it is still necessary.
I do think there is a place in Boost for a library like this. I don't think we need neither a 100% type-safe solution, nor to completely hide the general runtime interface that request::push() gives us. But I think we do have some usability barrier here, and you need to have a plan on how to evolve it to make it better, before the library gets into Boost and you loose the ability to introduce big breaking changes. I don't think the type-safe solution should span *all* Redis commands, but only the most common ones. If the command the user wants does not have type-safety implemented, she can always use what it's in place now. Simple interfaces for simple uses, complex ones for advanced use cases (that's something I learnt in MySQL the bad way).
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.
Hand't think of that. Although I don't have data to speak, I get the impression that this feature will be used unintentionally more than intentionally. But it might just be an opinion.
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.
That's something I didn't take into account when I wrote the review, and that gets solving this harder. Is there a way, given a type T, to know whether the response to a given command (say "GET") is compatible with T? Without performing the actual parsing? I can think of the following solutions (excuse me if any of these have flaws I haven't taken into account): 1) A runtime checking option. For the most common commands, you provide a set of helper functions like request::push_set or request::push_hgetall(). request remembers which commands have been added. The deserializing function checks for type compatibility before proceeding to the actual deserialization. This would pick the std::optional<T> vs T mismatch. Not sure if it's worth paying the price, though. You could provide an option to disable this at runtime. 2) A compile-time checking option. I envision this as something like typed_request<commands::get, commands::hgetall, ...>. You can check for compatibility at compile-time. You will probably want something like commands::any to provide access to commands that don't have a type-safe interface. Note that I don't think this should replace aedis::request, but complement it. Do you find any of these solutions appealing? Knowing that a strategy is in-place to evolve the library towards type-safety would alleviate many of my concerns.
I am trying to understand how you avoid these risks in Boost.MySql but it seems it suffers from the same problems?
I mean this: https://github.com/boostorg/mysql/blob/ac7285c62163c0de40a466ecaeca13f7de5af...
conn.query("SELECT salary FROM employee WHERE first_name = 'Underpaid'", result); double salary = result.rows().at(0).at(0).as_double();
The query is not type safe [1] and the result can mismatch the the query. Why would it be considered ok for Boost.MySql and not Boost.Aedis?
Disclaimer: I am not trying to make a case or open a precedent, it is purely technical. I might be wrong but I would like to clarify anyway.
So we have two parts here. The "client -> server" interface has two flavours: connection::query(string_view), and statement::execute(const ParamsTuple&). query() should only be used for queries that don't have parameters provided by the user (I should change that example, BTW). statement::execute takes a tuple of parameters. The client will check for compatibility in terms of number of parameters. Types are not checked in the client because the server is *very* flexible with type conversions. The second part is the "server -> client" interface (mysql::resultset), which is the one I think you're referring to here. It's a purely variant-like interface. Still, there is code to runtime-check that what the server sent matches the query metadata, which is provided by the server. I view this variant-like interface similar to Boost.JSON parsing into a json::value. I'd say the differences I see from the Aedis case are: * There is no way for the MySQL client to know the resulting data structure from a query given the contents of a query. "SELECT product FROM products" can yield 10 different data types, and without the table definition, you can't know. I got the impression this is not the case in Redis - "HGETALL" always returns the same data structure (please correct me if I'm wrong). * From a user perspective, when I use a variant-like interface (like the one in Boost.JSON or in Python database access), my head is set to "type checking is my responsibility". I know I'm in runtime. When I use a compile-time interface, I get much more relaxed. I got the "I'm at compile-time safety" with Aedis, but that impression is not right. Again, this is just based on what I felt while using the library for the first time, other users might feel differently. I think if I had encountered an aedis::value, similar to json::value, I would have switched my mind to "I'm at runtime". I'm not saying this is the solution, I'm just trying to explain myself. * I still got criticism for this in MySQL, and I've got a safer compile-time interface planned (https://github.com/boostorg/mysql/issues/60). The idea is that you define a row type T, so you can then read a typed resultset<T> (or individual rows by calling connection::read_one_row(T&)). My idea is to fully check for type compatibility as soon as I've got the query metadata available - that would be similar to solution 1) I proposed you earlier in this email. * If you're referring to the ability to compose SQL queries in a type-safe way, as sqlpp11 does, I did determine this as out of scope. As SQL is a standard, you usually get this at the database compatibility layer, rather than in individual drivers. I see it as more involved than providing type-safety to Redis commands - please apologies if this statement is not correct.
This is to make it safe by default and in part because Aedis is centered around the adapter. In the same sense that you can write an adapter to receive responses in the data-structure that makes most sense to the application e.g. std::map, std::unordered_map, boost::flat_map etc. You can also customize the way errors are handled. For example,
* Errors like type mismatch are usually unrecoverable and you might want to fail fast and let the async_ operation itself fail instead of proceeding in an unknown state.
* If you can tolerate errors like that you can write an adapter that does not communicate an error to the parser. I did not make this the default because it looks unsafe, people will usually expect async_exec(req, adapt(resp)) to throw when the response can't be stored in resp.
I am perhaps at fault here for not documenting this feature. I thought however not many people would want this behaviour.
I think you're right and that this is the most common case. I had a look at the Python redis driver, and they do the following: * By default, fail the operation if any of the pipeline requests failed. * If you pass a certain config option, individual errors are reported as error response objects. So if you sent [C1, C2, C3] and C2 failed, you will get [C1_response, C2_error_info, C3_response]. I think your default is right. Maybe you could provide the option to report errors individually by having the user provide a) std::tuple<leaf::result<T1>, leaf::result<T2>> Instead of b) std::tuple<T1, T2> If the user did b), then do the same you're currently doing, otherwise report errors individually. I would like to know your thoughts on these two matters (type-safety and error handling), as having a plan would alleviate many of my concerns. Regards, Ruben.