Re: Boost MySQL Review Starts Today

From: Ruben Perez (rubenperez038_at_[hidden])
Date: 2022-05-11 20:44:18

On Wed, 11 May 2022 at 07:13, Klemens Morgenstern via Boost
<boost_at_[hidden]> wrote:
>
> 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