On Wed, 11 May 2022 at 07:13, Klemens Morgenstern via Boost
On Tue, 2022-05-10 at 09:14 +0200, Richard Hodges via Boost wrote:
Here's my review
Reviewing a database connector in depth will require setting up an instance of a MySQL database. Fortunately most (all?) Linux distributions carry a MySQL and/or MariaDB package. MySQL community edition is available for download on all platforms here: https://dev.mysql.com/downloads/
I've spun up a small test program https://gist.github.com/klemens-morgenstern/0195a502340bd33029437d33eb914f2c
which worked very well & was intuitive given my asio & sql experience.
This library should definitely be accepted into boost.
Thanks for your time writing your review and your positive comments.
A few things are odd (like the .async_handshake on the ssl-conn; I would have expected that to require .next_layer().async_handshake).
This is an unfortunate oddity of the MySQL client/server protocol. The protocol defines an initial, unencrypted set of messages exchanged by the client and the server, where they negotiate whether TLS will be used or not. Once this is decided, the TLS handshake is invoked. After that, the handshake operation continues with a set of encrypted messages until it either succeeds or fails. Having the user call .next_layer().async_handshake directly would make the handshake operation more complex; it would need to be split in two: bool requires_ssl = connection.handshake_part1(params); if (requires_ssl) connection.next_layer().handshake(); connection.handshake_part2(params); I thought the added complexity wasn't worth it.
The ssl option also seems to be a runtime parameter in many functions, which confused me a bit. Is that so I can use an ssl::stream<> but turn it off at runtime? If so, why is that needed, why wouldn't I just us a tcp::socket directly?
This option controls TLS negotiation. By specifying ssl_mode::enable, you allow the client to not use TLS if the server doesn't support it. The default, ssl_mode::require, will make the handshake fail if the server doesn't support TLS, which is the most secure option. ssl_mode::disable is provided for consistency with the official mysql command line client. It isn't really required, and if there is consensus that it causes more confusion than help, I'm happy to remove it. Docs: https://anarthal.github.io/mysql/mysql/connparams.html#mysql.connparams.ssl
I think returning objects through completion handlers is absolutely fine and Asio-ish; but the library will surely grow to include more overloads in the future.
Some other questions you might want to consider answering:
- Will the library bring additional out-of-the-box utility to Boost?
Yes.
- What is your evaluation of the implementation?
It looks to be based on async_compose & asio::coroutine, which is a proven pattern . In addition, I am impressed and happy that it supports cancellation.
Thanks, but that's really something you get by using async_compose, and the library doesn't provide special support for it.
The prepared statements are easy to use, so that one can prevent injection attacks properly.
- What is your evaluation of the documentation?
It desribes the library quite well. I would like a section on transactions, because I feel like there might be some sharp edges there.
AFAIK transactions are just managed using regular SQL statements, so using `connection::query` with those statements should be enough. You aren't the first user asking about it though, so I've raised https://github.com/anarthal/mysql/issues/65 to add docs/examples on it.
- Will the choice of API abstraction model ease the development of software that must talk to a MySQL database?
It's exactly the right amount of detail for what it is: a database connector. It's not an ORM nor some wrapper that tries to prevent users from doing stupid things (like forwarding stdin to the DB). Either of those problems should be solved in another library that could be built on top of boost.mysql.
- Are there any immediate improvements that could be made after acceptance, if acceptance should happen?
I think additional overloads, depending on user requests. I also wonder if there is some useful transaction support, but this largely depends on the mysql protocol. It could be that there is no point to that.
AFAIK this is handled as regular SQL statements, so this shouldn't need special support. The integration test suite actually relies on transactions to maintain test runtime below adequate levels.
- Did you try to use the library? With which compiler(s)? Did you have any problems?
Clang-13 with libc++ and C++20.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Build a test program and read the doc for ~1h.
- Are you knowledgeable about the problem domain?
Yes.
I vote to accept the library into boost.
Thanks.
PS: Isn't mysql a registered trademark owned by oracle?
I'm no lawyer, so I don't know how much trouble this can cause. I chose this name because it made obvious what the library was about. I'm open to suggestions.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost